From 26c1bb6a7757304afc91e99267c4461e254c3fb1 Mon Sep 17 00:00:00 2001 From: Brandon Palm Date: Wed, 6 May 2026 13:47:59 -0500 Subject: [PATCH] CM-1040: Extract shared ApplyResource helper and migrate istiocsr to SSA Both istiocsr and trustmanager had 8-10 nearly identical createOrApply methods with duplicated reconciliation boilerplate. trustmanager used Server-Side Apply while istiocsr used Create/UpdateWithRetry. Add a generic common.ApplyResource[T]() helper that handles the common Exists/drift-check/Patch pattern with a pluggable hasChanged callback. The helper derives the Kubernetes kind from the type parameter for clear error messages and uses client.ObjectKeyFromObject for consistent resource name formatting. Migrate all trustmanager methods and istiocsr simple methods to use it. ClusterRole/ClusterRoleBinding methods in istiocsr are kept as-is since they use GenerateName with List fallback and Delete for immutable RoleRef changes. The istioCSRCreateRecon warning events are dropped from the migrated methods since SSA is inherently idempotent. --- pkg/controller/common/applier.go | 54 ++++++ pkg/controller/istiocsr/certificates.go | 38 +---- pkg/controller/istiocsr/certificates_test.go | 24 +-- pkg/controller/istiocsr/constants.go | 3 + .../istiocsr/install_instiocsr_test.go | 54 ++++-- pkg/controller/istiocsr/install_istiocsr.go | 10 +- pkg/controller/istiocsr/networkpolicies.go | 55 +----- pkg/controller/istiocsr/rbacs.go | 156 +++--------------- pkg/controller/istiocsr/rbacs_test.go | 48 +++--- pkg/controller/istiocsr/serviceaccounts.go | 39 +---- .../istiocsr/serviceaccounts_test.go | 81 ++------- pkg/controller/istiocsr/services.go | 43 +---- pkg/controller/istiocsr/services_test.go | 26 +-- pkg/controller/trustmanager/certificates.go | 50 +----- .../trustmanager/certificates_test.go | 8 +- .../trustmanager/controller_test.go | 2 +- pkg/controller/trustmanager/deployments.go | 22 +-- .../trustmanager/deployments_test.go | 4 +- pkg/controller/trustmanager/rbacs.go | 145 ++-------------- pkg/controller/trustmanager/rbacs_test.go | 10 +- .../trustmanager/serviceaccounts.go | 25 +-- .../trustmanager/serviceaccounts_test.go | 4 +- pkg/controller/trustmanager/services.go | 24 +-- pkg/controller/trustmanager/services_test.go | 6 +- pkg/controller/trustmanager/webhooks.go | 30 +--- pkg/controller/trustmanager/webhooks_test.go | 4 +- 26 files changed, 231 insertions(+), 734 deletions(-) create mode 100644 pkg/controller/common/applier.go diff --git a/pkg/controller/common/applier.go b/pkg/controller/common/applier.go new file mode 100644 index 000000000..55d33bb8c --- /dev/null +++ b/pkg/controller/common/applier.go @@ -0,0 +1,54 @@ +package common + +import ( + "context" + "reflect" + + "github.com/go-logr/logr" + + corev1 "k8s.io/api/core/v1" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// ApplyResource reconciles a desired Kubernetes object using Server-Side Apply. +// It checks whether the resource already exists and whether managed fields have +// drifted. If no drift is detected it returns early. Otherwise it patches the +// object with SSA using the given fieldOwner. +// +// T is the concrete object type (e.g. *corev1.Service). The hasChanged callback +// receives typed desired and existing objects so callers can compare fields +// without type assertions. +func ApplyResource[T client.Object]( + ctx context.Context, + c CtrlClient, + log logr.Logger, + recorder record.EventRecorder, + owner client.Object, + desired T, + existing T, + fieldOwner string, + hasChanged func(desired, existing T) bool, +) error { + key := client.ObjectKeyFromObject(desired) + kind := reflect.TypeOf(desired).Elem().Name() + + log.V(4).Info("reconciling resource", "kind", kind, "name", key) + + exists, err := c.Exists(ctx, key, existing) + if err != nil { + return FromClientError(err, "failed to check if %s %q exists", kind, key) + } + if exists && !hasChanged(desired, existing) { + log.V(4).Info("resource exists and is in desired state", "kind", kind, "name", key) + return nil + } + + log.V(2).Info("applying resource to desired state", "kind", kind, "name", key) + if err := c.Patch(ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { + return FromClientError(err, "failed to apply %s %q", kind, key) + } + + recorder.Eventf(owner, corev1.EventTypeNormal, "Reconciled", "%s %s applied", kind, key) + return nil +} diff --git a/pkg/controller/istiocsr/certificates.go b/pkg/controller/istiocsr/certificates.go index a77bb3987..931e56614 100644 --- a/pkg/controller/istiocsr/certificates.go +++ b/pkg/controller/istiocsr/certificates.go @@ -4,9 +4,7 @@ import ( "fmt" "maps" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" certmanagermetav1 "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" @@ -16,43 +14,15 @@ import ( "github.com/openshift/cert-manager-operator/pkg/operator/assets" ) -func (r *Reconciler) createOrApplyCertificates(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string, istioCSRCreateRecon bool) error { +func (r *Reconciler) createOrApplyCertificates(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error { desired, err := r.getCertificateObject(istiocsr, resourceLabels) if err != nil { return fmt.Errorf("failed to generate certificate resource for creation in %s: %w", istiocsr.GetNamespace(), err) } - certificateName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling certificate resource", "name", certificateName) - fetched := &certmanagerv1.Certificate{} - exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), fetched) - if err != nil { - return common.FromClientError(err, "failed to check %s certificate resource already exists", certificateName) - } - - if exist { - if istioCSRCreateRecon { - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s certificate resource already exists, maybe from previous installation", certificateName) - } - if hasObjectChanged(desired, fetched) { - r.log.V(1).Info("certificate has been modified, updating to desired state", "name", certificateName) - if err := r.UpdateWithRetry(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to update %s certificate resource", certificateName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "certificate resource %s reconciled back to desired state", certificateName) - } else { - r.log.V(4).Info("certificate resource already exists and is in expected state", "name", certificateName) - } - } - - if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to create %s certificate resource", certificateName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "certificate resource %s created", certificateName) - } - - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, desired, &certmanagerv1.Certificate{}, fieldOwner, + func(d, e *certmanagerv1.Certificate) bool { return hasObjectChanged(d, e) }, + ) } func (r *Reconciler) getCertificateObject(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) (*certmanagerv1.Certificate, error) { diff --git a/pkg/controller/istiocsr/certificates_test.go b/pkg/controller/istiocsr/certificates_test.go index e9bb417eb..a735b6ec8 100644 --- a/pkg/controller/istiocsr/certificates_test.go +++ b/pkg/controller/istiocsr/certificates_test.go @@ -39,7 +39,7 @@ func TestCreateOrApplyCertificates(t *testing.T) { return true, nil }) }, - wantErr: `failed to check istio-test-ns/istiod certificate resource already exists: test client error`, + wantErr: `failed to check if Certificate "istio-test-ns/istiod" exists: test client error`, }, { name: "reconciliation of certificate fails while restoring to expected state", @@ -61,15 +61,9 @@ func TestCreateOrApplyCertificates(t *testing.T) { } return true, nil }) - m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { - switch obj.(type) { - case *certmanagerv1.Certificate: - return errTestClient - } - return nil - }) + m.PatchReturns(errTestClient) }, - wantErr: `failed to update istio-test-ns/istiod certificate resource: test client error`, + wantErr: `failed to apply Certificate "istio-test-ns/istiod": test client error`, }, { name: "reconciliation of certificate which already exists in expected state", @@ -110,15 +104,9 @@ func TestCreateOrApplyCertificates(t *testing.T) { } return true, nil }) - m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { - switch obj.(type) { - case *certmanagerv1.Certificate: - return errTestClient - } - return nil - }) + m.PatchReturns(errTestClient) }, - wantErr: `failed to create istio-test-ns/istiod certificate resource: test client error`, + wantErr: `failed to apply Certificate "istio-test-ns/istiod": test client error`, }, { name: "reconciliation of certificate when revisions are configured", @@ -223,7 +211,7 @@ func TestCreateOrApplyCertificates(t *testing.T) { }, istiocsr); err != nil { t.Errorf("test error: %v", err) } - err := r.createOrApplyCertificates(istiocsr, controllerDefaultResourceLabels, false) + err := r.createOrApplyCertificates(istiocsr, controllerDefaultResourceLabels) if (tt.wantErr != "" || err != nil) && (err == nil || err.Error() != tt.wantErr) { t.Errorf("createOrApplyCertificates() err: %v, wantErr: %v", err, tt.wantErr) } diff --git a/pkg/controller/istiocsr/constants.go b/pkg/controller/istiocsr/constants.go index bfdc78ef8..8cf4ab3a4 100644 --- a/pkg/controller/istiocsr/constants.go +++ b/pkg/controller/istiocsr/constants.go @@ -30,6 +30,9 @@ const ( // defaultRequeueTime is the default reconcile requeue time. defaultRequeueTime = time.Second * 30 + // fieldOwner identifies this controller when using Server-Side Apply. + fieldOwner = "istio-csr-controller" + // istiocsrObjectName is the name of the istio-csr resource created by user. // istio-csr CRD enforces name to be `default`. istiocsrObjectName = "default" diff --git a/pkg/controller/istiocsr/install_instiocsr_test.go b/pkg/controller/istiocsr/install_instiocsr_test.go index d128b1f00..3fe15f5f9 100644 --- a/pkg/controller/istiocsr/install_instiocsr_test.go +++ b/pkg/controller/istiocsr/install_instiocsr_test.go @@ -48,13 +48,31 @@ func TestReconcileIstioCSRDeployment(t *testing.T) { } return nil }) + // SSA-migrated resources (Service, ServiceAccount, Certificate, Role, RoleBinding) use Patch + m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + switch o := obj.(type) { + case *corev1.Service, *corev1.ServiceAccount: + if !reflect.DeepEqual(o.GetLabels(), labels) { + return fmt.Errorf("labels mismatch in %v resource; got: %v, want: %v", o, o.GetLabels(), labels) + } + case *certmanagerv1.Certificate, *rbacv1.Role, *rbacv1.RoleBinding: + l := make(map[string]string) + maps.Copy(l, labels) + l[istiocsrNamespaceMappingLabelName] = testIstioCSRNamespace + if !reflect.DeepEqual(o.GetLabels(), l) { + return fmt.Errorf("labels mismatch in %v resource; got: %v, want: %v", o, o.GetLabels(), l) + } + } + return nil + }) + // Non-SSA resources (Deployment, ClusterRole, ClusterRoleBinding) still use Create m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error { switch o := obj.(type) { - case *appsv1.Deployment, *corev1.Service, *corev1.ServiceAccount: + case *appsv1.Deployment: if !reflect.DeepEqual(o.GetLabels(), labels) { return fmt.Errorf("labels mismatch in %v resource; got: %v, want: %v", o, o.GetLabels(), labels) } - case *certmanagerv1.Certificate, *rbacv1.Role, *rbacv1.RoleBinding, *rbacv1.ClusterRole, *rbacv1.ClusterRoleBinding: + case *rbacv1.ClusterRole, *rbacv1.ClusterRoleBinding: l := make(map[string]string) maps.Copy(l, labels) l[istiocsrNamespaceMappingLabelName] = testIstioCSRNamespace @@ -67,9 +85,9 @@ func TestReconcileIstioCSRDeployment(t *testing.T) { }, }, { - name: "istiocsr reconciliation fails with serviceaccount creation error", + name: "istiocsr reconciliation fails with serviceaccount apply error", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { - m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error { + m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { switch obj.(type) { case *corev1.ServiceAccount: return errTestClient @@ -77,15 +95,20 @@ func TestReconcileIstioCSRDeployment(t *testing.T) { return nil }) }, - wantErr: `failed to create istiocsr-test-ns/cert-manager-istio-csr serviceaccount resource: test client error`, + wantErr: `failed to apply ServiceAccount "istiocsr-test-ns/cert-manager-istio-csr": test client error`, }, { - name: "istiocsr reconciliation fails with role creation error", + name: "istiocsr reconciliation fails with role apply error", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { - m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error { - switch o := obj.(type) { + m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + switch obj.(type) { case *rbacv1.Role: return errTestClient + } + return nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error { + switch o := obj.(type) { case *rbacv1.ClusterRoleBinding: roleBinding := testClusterRoleBinding() roleBinding.DeepCopyInto(o) @@ -93,15 +116,20 @@ func TestReconcileIstioCSRDeployment(t *testing.T) { return nil }) }, - wantErr: `failed to create istio-test-ns/cert-manager-istio-csr role resource: test client error`, + wantErr: `failed to apply Role "istio-test-ns/cert-manager-istio-csr": test client error`, }, { - name: "istiocsr reconciliation fails with certificate creation error", + name: "istiocsr reconciliation fails with certificate apply error", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { - m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error { - switch o := obj.(type) { + m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + switch obj.(type) { case *certmanagerv1.Certificate: return errTestClient + } + return nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, option ...client.CreateOption) error { + switch o := obj.(type) { case *rbacv1.ClusterRoleBinding: roleBinding := testClusterRoleBinding() roleBinding.DeepCopyInto(o) @@ -109,7 +137,7 @@ func TestReconcileIstioCSRDeployment(t *testing.T) { return nil }) }, - wantErr: `failed to create istio-test-ns/istiod certificate resource: test client error`, + wantErr: `failed to apply Certificate "istio-test-ns/istiod": test client error`, }, } diff --git a/pkg/controller/istiocsr/install_istiocsr.go b/pkg/controller/istiocsr/install_istiocsr.go index 05f1d2457..a9dc30620 100644 --- a/pkg/controller/istiocsr/install_istiocsr.go +++ b/pkg/controller/istiocsr/install_istiocsr.go @@ -13,25 +13,23 @@ func (r *Reconciler) reconcileIstioCSRDeployment(istiocsr *v1alpha1.IstioCSR, is return common.NewIrrecoverableError(err, "%s/%s configuration validation failed", istiocsr.GetNamespace(), istiocsr.GetName()) } - // if user has set custom labels to be added to all resources created by the controller - // merge it with the controller's own default labels. resourceLabels := make(map[string]string) if istiocsr.Spec.ControllerConfig != nil && len(istiocsr.Spec.ControllerConfig.Labels) != 0 { maps.Copy(resourceLabels, istiocsr.Spec.ControllerConfig.Labels) } maps.Copy(resourceLabels, controllerDefaultResourceLabels) - if err := r.createOrApplyNetworkPolicies(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyNetworkPolicies(istiocsr, resourceLabels); err != nil { r.log.Error(err, "failed to reconcile network policy resources") return err } - if err := r.createOrApplyServices(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyServices(istiocsr, resourceLabels); err != nil { r.log.Error(err, "failed to reconcile service resource") return err } - if err := r.createOrApplyServiceAccounts(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyServiceAccounts(istiocsr, resourceLabels); err != nil { r.log.Error(err, "failed to reconcile serviceaccount resource") return err } @@ -41,7 +39,7 @@ func (r *Reconciler) reconcileIstioCSRDeployment(istiocsr *v1alpha1.IstioCSR, is return err } - if err := r.createOrApplyCertificates(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyCertificates(istiocsr, resourceLabels); err != nil { r.log.Error(err, "failed to reconcile certificate resource") return err } diff --git a/pkg/controller/istiocsr/networkpolicies.go b/pkg/controller/istiocsr/networkpolicies.go index 03bbca008..8fb361ade 100644 --- a/pkg/controller/istiocsr/networkpolicies.go +++ b/pkg/controller/istiocsr/networkpolicies.go @@ -4,27 +4,26 @@ import ( "fmt" "maps" - corev1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/controller/common" "github.com/openshift/cert-manager-operator/pkg/operator/assets" ) -func (r *Reconciler) createOrApplyNetworkPolicies(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string, istioCSRCreateRecon bool) error { +func (r *Reconciler) createOrApplyNetworkPolicies(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error { r.log.V(4).Info("reconciling istio-csr network policies", "namespace", istiocsr.GetNamespace(), "name", istiocsr.GetName()) - // Apply static network policy assets for istio-csr for _, assetPath := range istioCSRNetworkPolicyAssets { obj, err := r.getNetworkPolicyFromAsset(assetPath, istiocsr, resourceLabels) if err != nil { return fmt.Errorf("failed to get network policy from asset %s: %w", assetPath, err) } - if err := r.createOrUpdateNetworkPolicy(obj, istioCSRCreateRecon); err != nil { - return fmt.Errorf("failed to create/update network policy from %s: %w", assetPath, err) + if err := common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, obj, &networkingv1.NetworkPolicy{}, fieldOwner, + func(d, e *networkingv1.NetworkPolicy) bool { return hasObjectChanged(d, e) }, + ); err != nil { + return fmt.Errorf("failed to apply network policy from %s: %w", assetPath, err) } } @@ -32,13 +31,11 @@ func (r *Reconciler) createOrApplyNetworkPolicies(istiocsr *v1alpha1.IstioCSR, r } func (r *Reconciler) getNetworkPolicyFromAsset(assetPath string, istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) (*networkingv1.NetworkPolicy, error) { - // Get the target namespace for istio-csr deployment namespace := istiocsr.GetNamespace() if namespace == "" { namespace = istiocsr.Spec.IstioCSRConfig.Istio.Namespace } - // Read the asset and decode it assetBytes := assets.MustAsset(assetPath) obj, err := runtime.Decode(codecs.UniversalDeserializer(), assetBytes) if err != nil { @@ -50,10 +47,8 @@ func (r *Reconciler) getNetworkPolicyFromAsset(assetPath string, istiocsr *v1alp return nil, fmt.Errorf("decoded object is not a NetworkPolicy, got %T", obj) } - // Set the correct namespace policy.Namespace = namespace - // Merge resource labels if policy.Labels == nil { policy.Labels = make(map[string]string) } @@ -61,43 +56,3 @@ func (r *Reconciler) getNetworkPolicyFromAsset(assetPath string, istiocsr *v1alp return policy, nil } - -func (r *Reconciler) createOrUpdateNetworkPolicy(policy *networkingv1.NetworkPolicy, istioCSRCreateRecon bool) error { - desired := policy.DeepCopy() - policyName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling network policy resource", "name", policyName) - - fetched := &networkingv1.NetworkPolicy{} - key := types.NamespacedName{ - Name: desired.GetName(), - Namespace: desired.GetNamespace(), - } - exist, err := r.Exists(r.ctx, key, fetched) - if err != nil { - return common.FromClientError(err, "failed to check %s network policy resource already exists", policyName) - } - - if exist { - if istioCSRCreateRecon { - r.eventRecorder.Eventf(policy, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s network policy resource already exists, maybe from previous installation", policyName) - } - if hasObjectChanged(desired, fetched) { - r.log.V(1).Info("network policy has been modified, updating to desired state", "name", policyName) - if err := r.UpdateWithRetry(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to update %s network policy resource", policyName) - } - r.eventRecorder.Eventf(policy, corev1.EventTypeNormal, "Reconciled", "network policy resource %s reconciled back to desired state", policyName) - } else { - r.log.V(4).Info("network policy resource already exists and is in expected state", "name", policyName) - } - } - - if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to create %s network policy resource", policyName) - } - r.eventRecorder.Eventf(policy, corev1.EventTypeNormal, "Reconciled", "network policy resource %s created", policyName) - } - - return nil -} diff --git a/pkg/controller/istiocsr/rbacs.go b/pkg/controller/istiocsr/rbacs.go index 1d77c1a63..e448b3bf4 100644 --- a/pkg/controller/istiocsr/rbacs.go +++ b/pkg/controller/istiocsr/rbacs.go @@ -31,22 +31,22 @@ func (r *Reconciler) createOrApplyRBACResource(istiocsr *v1alpha1.IstioCSR, reso return err } - if err := r.createOrApplyRoles(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyRoles(istiocsr, resourceLabels); err != nil { r.log.Error(err, "failed to reconcile role resource") return err } - if err := r.createOrApplyRoleBindings(istiocsr, serviceAccount, resourceLabels, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyRoleBindings(istiocsr, serviceAccount, resourceLabels); err != nil { r.log.Error(err, "failed to reconcile rolebinding resource") return err } - if err := r.createOrApplyRoleForLeases(istiocsr, resourceLabels, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyRoleForLeases(istiocsr, resourceLabels); err != nil { r.log.Error(err, "failed to reconcile role for leases resource") return err } - if err := r.createOrApplyRoleBindingForLeases(istiocsr, serviceAccount, resourceLabels, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyRoleBindingForLeases(istiocsr, serviceAccount, resourceLabels); err != nil { r.log.Error(err, "failed to reconcile rolebinding for leases resource") return err } @@ -248,40 +248,11 @@ func (r *Reconciler) updateClusterRoleBindingNameInStatus(istiocsr *v1alpha1.Ist return r.updateStatus(r.ctx, istiocsr) } -func (r *Reconciler) createOrApplyRoles(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string, istioCSRCreateRecon bool) error { +func (r *Reconciler) createOrApplyRoles(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error { desired := r.getRoleObject(istiocsr.GetNamespace(), istiocsr.Spec.IstioCSRConfig.Istio.Namespace, resourceLabels) - - roleName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling role resource", "name", roleName) - fetched := &rbacv1.Role{} - exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), fetched) - if err != nil { - return common.FromClientError(err, "failed to check %s role resource already exists", roleName) - } - - if exist { - if istioCSRCreateRecon { - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s role resource already exists, maybe from previous installation", roleName) - } - if hasObjectChanged(desired, fetched) { - r.log.V(1).Info("role has been modified, updating to desired state", "name", roleName) - if err := r.UpdateWithRetry(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to update %s role resource", roleName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "role resource %s reconciled back to desired state", roleName) - } else { - r.log.V(4).Info("role resource already exists and is in expected state", "name", roleName) - } - } - - if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to create %s role resource", roleName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "role resource %s created", roleName) - } - - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, desired, &rbacv1.Role{}, fieldOwner, + func(d, e *rbacv1.Role) bool { return hasObjectChanged(d, e) }, + ) } func (r *Reconciler) getRoleObject(istiocsrNamespace, roleNamespace string, resourceLabels map[string]string) *rbacv1.Role { @@ -291,40 +262,11 @@ func (r *Reconciler) getRoleObject(istiocsrNamespace, roleNamespace string, reso return role } -func (r *Reconciler) createOrApplyRoleBindings(istiocsr *v1alpha1.IstioCSR, serviceAccount string, resourceLabels map[string]string, istioCSRCreateRecon bool) error { +func (r *Reconciler) createOrApplyRoleBindings(istiocsr *v1alpha1.IstioCSR, serviceAccount string, resourceLabels map[string]string) error { desired := r.getRoleBindingObject(serviceAccount, istiocsr.GetNamespace(), istiocsr.Spec.IstioCSRConfig.Istio.Namespace, resourceLabels) - - roleBindingName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling rolebinding resource", "name", roleBindingName) - fetched := &rbacv1.RoleBinding{} - exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), fetched) - if err != nil { - return common.FromClientError(err, "failed to check %s rolebinding resource already exists", roleBindingName) - } - - if exist { - if istioCSRCreateRecon { - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s rolebinding resource already exists, maybe from previous installation", roleBindingName) - } - if hasObjectChanged(desired, fetched) { - r.log.V(1).Info("rolebinding has been modified, updating to desired state", "name", roleBindingName) - if err := r.UpdateWithRetry(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to update %s rolebinding resource", roleBindingName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "rolebinding resource %s reconciled back to desired state", roleBindingName) - } else { - r.log.V(4).Info("rolebinding resource already exists and is in expected state", "name", roleBindingName) - } - } - - if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to create %s rolebinding resource", roleBindingName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "rolebinding resource %s created", roleBindingName) - } - - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, desired, &rbacv1.RoleBinding{}, fieldOwner, + func(d, e *rbacv1.RoleBinding) bool { return hasObjectChanged(d, e) }, + ) } func (r *Reconciler) getRoleBindingObject(serviceAccount, istiocsrNamespace, roleNamespace string, resourceLabels map[string]string) *rbacv1.RoleBinding { @@ -335,40 +277,11 @@ func (r *Reconciler) getRoleBindingObject(serviceAccount, istiocsrNamespace, rol return roleBinding } -func (r *Reconciler) createOrApplyRoleForLeases(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string, istioCSRCreateRecon bool) error { +func (r *Reconciler) createOrApplyRoleForLeases(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error { desired := r.getRoleForLeasesObject(istiocsr.GetNamespace(), istiocsr.Spec.IstioCSRConfig.Istio.Namespace, resourceLabels) - - roleName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling role for lease resource", "name", roleName) - fetched := &rbacv1.Role{} - exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), fetched) - if err != nil { - return common.FromClientError(err, "failed to check %s role resource already exists", roleName) - } - - if exist { - if istioCSRCreateRecon { - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s role resource already exists, maybe from previous installation", roleName) - } - if hasObjectChanged(desired, fetched) { - r.log.V(1).Info("role has been modified, updating to desired state", "name", roleName) - if err := r.UpdateWithRetry(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to update %s role resource", roleName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "role resource %s reconciled back to desired state", roleName) - } else { - r.log.V(4).Info("role resource already exists and is in expected state", "name", roleName) - } - } - - if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to create %s role resource", roleName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "role resource %s created", roleName) - } - - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, desired, &rbacv1.Role{}, fieldOwner, + func(d, e *rbacv1.Role) bool { return hasObjectChanged(d, e) }, + ) } func (r *Reconciler) getRoleForLeasesObject(istiocsrNamespace, roleNamespace string, resourceLabels map[string]string) *rbacv1.Role { @@ -378,40 +291,11 @@ func (r *Reconciler) getRoleForLeasesObject(istiocsrNamespace, roleNamespace str return role } -func (r *Reconciler) createOrApplyRoleBindingForLeases(istiocsr *v1alpha1.IstioCSR, serviceAccount string, resourceLabels map[string]string, istioCSRCreateRecon bool) error { +func (r *Reconciler) createOrApplyRoleBindingForLeases(istiocsr *v1alpha1.IstioCSR, serviceAccount string, resourceLabels map[string]string) error { desired := r.getRoleBindingForLeasesObject(serviceAccount, istiocsr.GetNamespace(), istiocsr.Spec.IstioCSRConfig.Istio.Namespace, resourceLabels) - - roleBindingName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling rolebinding for lease resource", "name", roleBindingName) - fetched := &rbacv1.RoleBinding{} - exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), fetched) - if err != nil { - return common.FromClientError(err, "failed to check %s rolebinding resource already exists", roleBindingName) - } - - if exist { - if istioCSRCreateRecon { - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s rolebinding resource already exists, maybe from previous installation", roleBindingName) - } - if hasObjectChanged(desired, fetched) { - r.log.V(1).Info("rolebinding has been modified, updating to desired state", "name", roleBindingName) - if err := r.UpdateWithRetry(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to update %s rolebinding resource", roleBindingName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "rolebinding resource %s reconciled back to desired state", roleBindingName) - } else { - r.log.V(4).Info("rolebinding resource already exists and is in expected state", "name", roleBindingName) - } - } - - if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to create %s rolebinding resource", roleBindingName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "rolebinding resource %s created", roleBindingName) - } - - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, desired, &rbacv1.RoleBinding{}, fieldOwner, + func(d, e *rbacv1.RoleBinding) bool { return hasObjectChanged(d, e) }, + ) } func (r *Reconciler) getRoleBindingForLeasesObject(serviceAccount, istiocsrNamespace, roleNamespace string, resourceLabels map[string]string) *rbacv1.RoleBinding { diff --git a/pkg/controller/istiocsr/rbacs_test.go b/pkg/controller/istiocsr/rbacs_test.go index c2706e78a..6db12a5fb 100644 --- a/pkg/controller/istiocsr/rbacs_test.go +++ b/pkg/controller/istiocsr/rbacs_test.go @@ -67,7 +67,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return true, nil }) }, - wantErr: `failed to check istio-test-ns/cert-manager-istio-csr role resource already exists: test client error`, + wantErr: `failed to check if Role "istio-test-ns/cert-manager-istio-csr" exists: test client error`, }, { name: "rolebindings reconciliation fails while checking if exists", @@ -80,7 +80,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return true, nil }) }, - wantErr: `failed to check istio-test-ns/cert-manager-istio-csr rolebinding resource already exists: test client error`, + wantErr: `failed to check if RoleBinding "istio-test-ns/cert-manager-istio-csr" exists: test client error`, }, { name: "role-leases reconciliation fails while checking if exists", @@ -95,7 +95,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return true, nil }) }, - wantErr: `failed to check istio-test-ns/cert-manager-istio-csr-leases role resource already exists: test client error`, + wantErr: `failed to check if Role "istio-test-ns/cert-manager-istio-csr-leases" exists: test client error`, }, { name: "rolebindings-leases reconciliation fails while checking if exists", @@ -110,7 +110,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return true, nil }) }, - wantErr: `failed to check istio-test-ns/cert-manager-istio-csr-leases rolebinding resource already exists: test client error`, + wantErr: `failed to check if RoleBinding "istio-test-ns/cert-manager-istio-csr-leases" exists: test client error`, }, { name: "clusterrolebindings reconciliation fails while listing existing resources", @@ -453,7 +453,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { wantErr: `failed to create clusterrole resource: test client error`, }, { - name: "role reconciliation updating to desired state fails", + name: "role reconciliation applying to desired state fails", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, object client.Object) (bool, error) { switch o := object.(type) { @@ -464,7 +464,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return true, nil }) - m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { switch obj.(type) { case *rbacv1.Role: return errTestClient @@ -472,7 +472,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to update istio-test-ns/cert-manager-istio-csr role resource: test client error`, + wantErr: `failed to apply Role "istio-test-ns/cert-manager-istio-csr": test client error`, }, { name: "role reconciliation creation fails", @@ -484,7 +484,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return true, nil }) - m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { switch obj.(type) { case *rbacv1.Role: return errTestClient @@ -492,10 +492,10 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to create istio-test-ns/cert-manager-istio-csr role resource: test client error`, + wantErr: `failed to apply Role "istio-test-ns/cert-manager-istio-csr": test client error`, }, { - name: "role-leases reconciliation updating to desired state fails", + name: "role-leases reconciliation applying to desired state fails", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, object client.Object) (bool, error) { switch o := object.(type) { @@ -508,7 +508,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return true, nil }) - m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { switch obj.(type) { case *rbacv1.Role: if strings.HasSuffix(obj.GetName(), "-leases") { @@ -518,7 +518,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to update istio-test-ns/cert-manager-istio-csr-leases role resource: test client error`, + wantErr: `failed to apply Role "istio-test-ns/cert-manager-istio-csr-leases": test client error`, }, { name: "role-leases reconciliation creation fails", @@ -532,7 +532,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return true, nil }) - m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { switch obj.(type) { case *rbacv1.Role: if strings.HasSuffix(obj.GetName(), "-leases") { @@ -542,10 +542,10 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to create istio-test-ns/cert-manager-istio-csr-leases role resource: test client error`, + wantErr: `failed to apply Role "istio-test-ns/cert-manager-istio-csr-leases": test client error`, }, { - name: "rolebindings reconciliation updating to desired state fails", + name: "rolebindings reconciliation applying to desired state fails", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, object client.Object) (bool, error) { switch o := object.(type) { @@ -556,7 +556,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return true, nil }) - m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { switch obj.(type) { case *rbacv1.RoleBinding: return errTestClient @@ -564,7 +564,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to update istio-test-ns/cert-manager-istio-csr rolebinding resource: test client error`, + wantErr: `failed to apply RoleBinding "istio-test-ns/cert-manager-istio-csr": test client error`, }, { name: "rolebindings reconciliation creation fails", @@ -576,7 +576,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return true, nil }) - m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { switch obj.(type) { case *rbacv1.RoleBinding: return errTestClient @@ -584,10 +584,10 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to create istio-test-ns/cert-manager-istio-csr rolebinding resource: test client error`, + wantErr: `failed to apply RoleBinding "istio-test-ns/cert-manager-istio-csr": test client error`, }, { - name: "rolebinding-leases reconciliation updating to desired state fails", + name: "rolebinding-leases reconciliation applying to desired state fails", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, object client.Object) (bool, error) { switch o := object.(type) { @@ -600,7 +600,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return true, nil }) - m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { switch obj.(type) { case *rbacv1.RoleBinding: if strings.HasSuffix(obj.GetName(), "-leases") { @@ -610,7 +610,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to update istio-test-ns/cert-manager-istio-csr-leases rolebinding resource: test client error`, + wantErr: `failed to apply RoleBinding "istio-test-ns/cert-manager-istio-csr-leases": test client error`, }, { name: "rolebinding-leases reconciliation creation fails", @@ -624,7 +624,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { } return true, nil }) - m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { switch obj.(type) { case *rbacv1.RoleBinding: if strings.HasSuffix(obj.GetName(), "-leases") { @@ -634,7 +634,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to create istio-test-ns/cert-manager-istio-csr-leases rolebinding resource: test client error`, + wantErr: `failed to apply RoleBinding "istio-test-ns/cert-manager-istio-csr-leases": test client error`, }, } diff --git a/pkg/controller/istiocsr/serviceaccounts.go b/pkg/controller/istiocsr/serviceaccounts.go index 6930dc66b..e9c20cdb3 100644 --- a/pkg/controller/istiocsr/serviceaccounts.go +++ b/pkg/controller/istiocsr/serviceaccounts.go @@ -1,51 +1,24 @@ package istiocsr import ( - "fmt" - corev1 "k8s.io/api/core/v1" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/controller/common" "github.com/openshift/cert-manager-operator/pkg/operator/assets" ) -func (r *Reconciler) createOrApplyServiceAccounts(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string, istioCSRCreateRecon bool) error { +func (r *Reconciler) createOrApplyServiceAccounts(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error { desired := r.getServiceAccountObject(istiocsr, resourceLabels) - serviceAccountName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling serviceaccount resource", "name", serviceAccountName) - fetched := &corev1.ServiceAccount{} - exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), fetched) - if err != nil { - return common.FromClientError(err, "failed to check %s serviceaccount resource already exists", serviceAccountName) - } - - if exist { - if istioCSRCreateRecon { - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s serviceaccount resource already exists, maybe from previous installation", serviceAccountName) - } - if hasObjectChanged(desired, fetched) { - r.log.V(1).Info("serviceaccount has been modified, updating to desired state", "name", serviceAccountName) - if err := r.UpdateWithRetry(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to update %s serviceaccount resource", serviceAccountName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "serviceaccount resource %s reconciled back to desired state", serviceAccountName) - } else { - r.log.V(4).Info("serviceaccount resource already exists and is in expected state", "name", serviceAccountName) - } - } - - if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to create %s serviceaccount resource", serviceAccountName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "serviceaccount resource %s created", serviceAccountName) + if err := common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, desired, &corev1.ServiceAccount{}, fieldOwner, + func(d, e *corev1.ServiceAccount) bool { return hasObjectChanged(d, e) }, + ); err != nil { + return err } if err := r.updateServiceAccountNameInStatus(istiocsr, desired); err != nil { - return common.FromClientError(err, "failed to update %s/%s istiocsr status with %s serviceaccount resource name", istiocsr.GetNamespace(), istiocsr.GetName(), serviceAccountName) + return common.FromClientError(err, "failed to update %s/%s istiocsr status with %s serviceaccount resource name", istiocsr.GetNamespace(), istiocsr.GetName(), desired.GetName()) } return nil } diff --git a/pkg/controller/istiocsr/serviceaccounts_test.go b/pkg/controller/istiocsr/serviceaccounts_test.go index 82e0f9c4a..66c9cbfb5 100644 --- a/pkg/controller/istiocsr/serviceaccounts_test.go +++ b/pkg/controller/istiocsr/serviceaccounts_test.go @@ -2,13 +2,10 @@ package istiocsr import ( "context" - "strings" "testing" - "time" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" @@ -22,12 +19,10 @@ const ( func TestCreateOrApplyServiceAccounts(t *testing.T) { tests := []struct { - name string - preReq func(*Reconciler, *fakes.FakeCtrlClient) - istioCSRCreateRecon bool - wantErr string - assertEvents func(t *testing.T, r *Reconciler) - assertCalls func(t *testing.T, mock *fakes.FakeCtrlClient) + name string + preReq func(*Reconciler, *fakes.FakeCtrlClient) + wantErr string + assertCalls func(t *testing.T, mock *fakes.FakeCtrlClient) }{ { name: "serviceaccount reconciliation successful", @@ -53,7 +48,7 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { return false, nil }) }, - wantErr: `failed to check istiocsr-test-ns/cert-manager-istio-csr serviceaccount resource already exists: test client error`, + wantErr: `failed to check if ServiceAccount "istiocsr-test-ns/cert-manager-istio-csr" exists: test client error`, }, { name: "serviceaccount reconciliation fails while updating status", @@ -66,35 +61,10 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { return nil }) }, - wantErr: `failed to update istiocsr-test-ns/istiocsr-test-resource istiocsr status with istiocsr-test-ns/cert-manager-istio-csr serviceaccount resource name: failed to update status for "istiocsr-test-ns/istiocsr-test-resource": failed to update istiocsr.openshift.operator.io "istiocsr-test-ns/istiocsr-test-resource" status: test client error`, + wantErr: `failed to update istiocsr-test-ns/istiocsr-test-resource istiocsr status with cert-manager-istio-csr serviceaccount resource name: failed to update status for "istiocsr-test-ns/istiocsr-test-resource": failed to update istiocsr.openshift.operator.io "istiocsr-test-ns/istiocsr-test-resource" status: test client error`, }, { - name: "serviceaccount already exists with istioCSRCreateRecon true records ResourceAlreadyExists event", - istioCSRCreateRecon: true, - preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { - m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { - switch o := obj.(type) { - case *corev1.ServiceAccount: - serviceaccount := testServiceAccount() - serviceaccount.DeepCopyInto(o) - } - return true, nil - }) - }, - assertEvents: func(t *testing.T, r *Reconciler) { - rec := r.eventRecorder.(*record.FakeRecorder) - select { - case evt := <-rec.Events: - if !strings.Contains(evt, "ResourceAlreadyExists") || !strings.Contains(evt, "serviceaccount resource already exists") { - t.Errorf("createOrApplyServiceAccounts() event: %q, want ResourceAlreadyExists and serviceaccount already exists", evt) - } - case <-time.After(time.Second): - t.Error("expected ResourceAlreadyExists event but none received") - } - }, - }, - { - name: "serviceaccount exists but modified is updated and reconciled", + name: "serviceaccount exists but modified is applied and reconciled", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { switch o := obj.(type) { @@ -105,27 +75,15 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { } return true, nil }) - m.UpdateWithRetryReturns(nil) - }, - assertEvents: func(t *testing.T, r *Reconciler) { - rec := r.eventRecorder.(*record.FakeRecorder) - select { - case evt := <-rec.Events: - if !strings.Contains(evt, "Reconciled") || !strings.Contains(evt, "reconciled back to desired state") { - t.Errorf("createOrApplyServiceAccounts() event: %q, want Reconciled and reconciled back to desired state", evt) - } - case <-time.After(time.Second): - t.Error("expected Reconciled event but none received") - } }, assertCalls: func(t *testing.T, mock *fakes.FakeCtrlClient) { - if mock.UpdateWithRetryCallCount() != 1 { - t.Errorf("createOrApplyServiceAccounts() UpdateWithRetry call count: %d, want 1", mock.UpdateWithRetryCallCount()) + if mock.PatchCallCount() != 1 { + t.Errorf("createOrApplyServiceAccounts() Patch call count: %d, want 1", mock.PatchCallCount()) } }, }, { - name: "serviceaccount reconciliation fails while updating modified serviceaccount", + name: "serviceaccount reconciliation fails while applying modified serviceaccount", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { switch o := obj.(type) { @@ -136,15 +94,9 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { } return true, nil }) - m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, _ ...client.UpdateOption) error { - switch obj.(type) { - case *corev1.ServiceAccount: - return errTestClient - } - return nil - }) + m.PatchReturns(errTestClient) }, - wantErr: `failed to update istiocsr-test-ns/cert-manager-istio-csr serviceaccount resource: test client error`, + wantErr: `failed to apply ServiceAccount "istiocsr-test-ns/cert-manager-istio-csr": test client error`, }, { name: "serviceaccount created when it does not exist", @@ -158,8 +110,8 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { }) }, assertCalls: func(t *testing.T, mock *fakes.FakeCtrlClient) { - if mock.CreateCallCount() != 1 { - t.Errorf("createOrApplyServiceAccounts() Create call count: %d, want 1", mock.CreateCallCount()) + if mock.PatchCallCount() != 1 { + t.Errorf("createOrApplyServiceAccounts() Patch call count: %d, want 1", mock.PatchCallCount()) } }, }, @@ -174,7 +126,7 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { } r.CtrlClient = mock istiocsr := testIstioCSR() - err := r.createOrApplyServiceAccounts(istiocsr, controllerDefaultResourceLabels, tt.istioCSRCreateRecon) + err := r.createOrApplyServiceAccounts(istiocsr, controllerDefaultResourceLabels) if (tt.wantErr != "" || err != nil) && (err == nil || err.Error() != tt.wantErr) { t.Errorf("createOrApplyServiceAccounts() err: %v, wantErr: %v", err, tt.wantErr) } @@ -183,9 +135,6 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { t.Errorf("createOrApplyServiceAccounts() got: %v, want: %s", istiocsr.Status.ServiceAccount, serviceAccountName) } } - if tt.assertEvents != nil { - tt.assertEvents(t, r) - } if tt.assertCalls != nil { tt.assertCalls(t, mock) } diff --git a/pkg/controller/istiocsr/services.go b/pkg/controller/istiocsr/services.go index bc49f01f9..a318e6dcb 100644 --- a/pkg/controller/istiocsr/services.go +++ b/pkg/controller/istiocsr/services.go @@ -4,7 +4,6 @@ import ( "fmt" corev1 "k8s.io/api/core/v1" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/controller/common" @@ -12,13 +11,12 @@ import ( ) const ( - // grpcServicePortName is the name found for the GRPC service in the static manifest. grpcServicePortName = "web" ) -func (r *Reconciler) createOrApplyServices(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string, istioCSRCreateRecon bool) error { +func (r *Reconciler) createOrApplyServices(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error { service := r.getServiceObject(istiocsr, resourceLabels) - if err := r.createOrApplyService(istiocsr, service, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyService(istiocsr, service); err != nil { return err } if err := r.updateGRPCEndpointInStatus(istiocsr, service); err != nil { @@ -26,43 +24,16 @@ func (r *Reconciler) createOrApplyServices(istiocsr *v1alpha1.IstioCSR, resource } metricsService := r.getMetricsServiceObject(istiocsr, resourceLabels) - if err := r.createOrApplyService(istiocsr, metricsService, istioCSRCreateRecon); err != nil { + if err := r.createOrApplyService(istiocsr, metricsService); err != nil { return err } return nil } -func (r *Reconciler) createOrApplyService(istiocsr *v1alpha1.IstioCSR, svc *corev1.Service, istioCSRCreateRecon bool) error { - serviceName := fmt.Sprintf("%s/%s", svc.GetNamespace(), svc.GetName()) - r.log.V(4).Info("reconciling service resource", "name", serviceName) - fetched := &corev1.Service{} - exist, err := r.Exists(r.ctx, client.ObjectKeyFromObject(svc), fetched) - if err != nil { - return common.FromClientError(err, "failed to check %s service resource already exists", serviceName) - } - - if exist { - if istioCSRCreateRecon { - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s service resource already exists, maybe from previous installation", serviceName) - } - if hasObjectChanged(svc, fetched) { - r.log.V(1).Info("service has been modified, updating to desired state", "name", serviceName) - if err := r.UpdateWithRetry(r.ctx, svc); err != nil { - return common.FromClientError(err, "failed to update %s service resource", serviceName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "service resource %s reconciled back to desired state", serviceName) - } else { - r.log.V(4).Info("service resource already exists and is in expected state", "name", serviceName) - } - } - - if !exist { - if err := r.Create(r.ctx, svc); err != nil { - return common.FromClientError(err, "failed to create %s service resource", serviceName) - } - r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "service resource %s created", serviceName) - } - return nil +func (r *Reconciler) createOrApplyService(istiocsr *v1alpha1.IstioCSR, svc *corev1.Service) error { + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, istiocsr, svc, &corev1.Service{}, fieldOwner, + func(d, e *corev1.Service) bool { return hasObjectChanged(d, e) }, + ) } func (r *Reconciler) getServiceObject(istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) *corev1.Service { diff --git a/pkg/controller/istiocsr/services_test.go b/pkg/controller/istiocsr/services_test.go index 708a116a4..7af59fffd 100644 --- a/pkg/controller/istiocsr/services_test.go +++ b/pkg/controller/istiocsr/services_test.go @@ -50,18 +50,12 @@ func TestCreateOrApplyServices(t *testing.T) { return false, nil }) }, - wantErr: `failed to check istiocsr-test-ns/cert-manager-istio-csr service resource already exists: test client error`, + wantErr: `failed to check if Service "istiocsr-test-ns/cert-manager-istio-csr" exists: test client error`, }, { - name: "service reconciliation fails while updating to desired state", + name: "service reconciliation fails while applying to desired state", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { - m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, option ...client.UpdateOption) error { - switch obj.(type) { - case *corev1.Service: - return errTestClient - } - return nil - }) + m.PatchReturns(errTestClient) m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { switch o := obj.(type) { case *corev1.Service: @@ -73,20 +67,14 @@ func TestCreateOrApplyServices(t *testing.T) { return false, nil }) }, - wantErr: `failed to update istiocsr-test-ns/cert-manager-istio-csr service resource: test client error`, + wantErr: `failed to apply Service "istiocsr-test-ns/cert-manager-istio-csr": test client error`, }, { name: "service reconciliation fails while creating", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { - m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { - switch obj.(type) { - case *corev1.Service: - return errTestClient - } - return nil - }) + m.PatchReturns(errTestClient) }, - wantErr: `failed to create istiocsr-test-ns/cert-manager-istio-csr service resource: test client error`, + wantErr: `failed to apply Service "istiocsr-test-ns/cert-manager-istio-csr": test client error`, }, { name: "service reconciliation when server config is not empty", @@ -111,7 +99,7 @@ func TestCreateOrApplyServices(t *testing.T) { if tt.updateIstioCSR != nil { tt.updateIstioCSR(istiocsr) } - err := r.createOrApplyServices(istiocsr, controllerDefaultResourceLabels, false) + err := r.createOrApplyServices(istiocsr, controllerDefaultResourceLabels) if (tt.wantErr != "" || err != nil) && (err == nil || err.Error() != tt.wantErr) { t.Errorf("createOrApplyServices() err: %v, wantErr: %v", err, tt.wantErr) } diff --git a/pkg/controller/trustmanager/certificates.go b/pkg/controller/trustmanager/certificates.go index be60843e7..00e653baf 100644 --- a/pkg/controller/trustmanager/certificates.go +++ b/pkg/controller/trustmanager/certificates.go @@ -5,9 +5,7 @@ import ( "reflect" "slices" - corev1 "k8s.io/api/core/v1" "k8s.io/utils/ptr" - "sigs.k8s.io/controller-runtime/pkg/client" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" certmanagermetav1 "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" @@ -17,29 +15,9 @@ import ( "github.com/openshift/cert-manager-operator/pkg/operator/assets" ) -// createOrApplyIssuer reconciles the self-signed Issuer used for trust-manager's webhook TLS. func (r *Reconciler) createOrApplyIssuer(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := getIssuerObject(resourceLabels, resourceAnnotations) - resourceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling issuer resource", "name", resourceName) - - existing := &certmanagerv1.Issuer{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if issuer %q exists", resourceName) - } - if exists && !issuerModified(desired, existing) { - r.log.V(4).Info("issuer resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("issuer resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply issuer %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "issuer resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &certmanagerv1.Issuer{}, fieldOwner, issuerModified) } func getIssuerObject(resourceLabels, resourceAnnotations map[string]string) *certmanagerv1.Issuer { @@ -51,29 +29,9 @@ func getIssuerObject(resourceLabels, resourceAnnotations map[string]string) *cer return issuer } -// createOrApplyCertificate reconciles the Certificate used for trust-manager's webhook TLS. func (r *Reconciler) createOrApplyCertificate(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := getCertificateObject(resourceLabels, resourceAnnotations) - resourceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling certificate resource", "name", resourceName) - - existing := &certmanagerv1.Certificate{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if certificate %q exists", resourceName) - } - if exists && !certificateModified(desired, existing) { - r.log.V(4).Info("certificate resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("certificate resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply certificate %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "certificate resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &certmanagerv1.Certificate{}, fieldOwner, certificateModified) } func getCertificateObject(resourceLabels, resourceAnnotations map[string]string) *certmanagerv1.Certificate { @@ -96,15 +54,11 @@ func getCertificateObject(resourceLabels, resourceAnnotations map[string]string) return certificate } -// issuerModified compares only the fields we manage via SSA. func issuerModified(desired, existing *certmanagerv1.Issuer) bool { return managedMetadataModified(desired, existing) || !reflect.DeepEqual(desired.Spec, existing.Spec) } -// certificateModified compares only the fields we manage via SSA. -// We compare individual spec fields rather than the full Spec because -// cert-manager's webhook may default fields we don't set (e.g. Duration). func certificateModified(desired, existing *certmanagerv1.Certificate) bool { if managedMetadataModified(desired, existing) { return true diff --git a/pkg/controller/trustmanager/certificates_test.go b/pkg/controller/trustmanager/certificates_test.go index 122773ed8..df956c0ad 100644 --- a/pkg/controller/trustmanager/certificates_test.go +++ b/pkg/controller/trustmanager/certificates_test.go @@ -233,7 +233,7 @@ func TestIssuerReconciliation(t *testing.T) { return false, errTestClient }) }, - wantErr: "failed to check if issuer", + wantErr: "failed to check if Issuer", wantExistsCount: 1, wantPatchCount: 0, }, @@ -247,7 +247,7 @@ func TestIssuerReconciliation(t *testing.T) { return errTestClient }) }, - wantErr: "failed to apply issuer", + wantErr: "failed to apply Issuer", wantExistsCount: 1, wantPatchCount: 1, }, @@ -376,7 +376,7 @@ func TestCertificateReconciliation(t *testing.T) { return false, errTestClient }) }, - wantErr: "failed to check if certificate", + wantErr: "failed to check if Certificate", wantExistsCount: 1, wantPatchCount: 0, }, @@ -390,7 +390,7 @@ func TestCertificateReconciliation(t *testing.T) { return errTestClient }) }, - wantErr: "failed to apply certificate", + wantErr: "failed to apply Certificate", wantExistsCount: 1, wantPatchCount: 1, }, diff --git a/pkg/controller/trustmanager/controller_test.go b/pkg/controller/trustmanager/controller_test.go index 3efd817a0..cfb13f30b 100644 --- a/pkg/controller/trustmanager/controller_test.go +++ b/pkg/controller/trustmanager/controller_test.go @@ -251,7 +251,7 @@ func TestProcessReconcileRequest(t *testing.T) { Reason: v1alpha1.ReasonInProgress, }, }, - wantErr: "failed to check if serviceaccount", + wantErr: "failed to check if ServiceAccount", }, { name: "trust namespace does not exist sets degraded true", diff --git a/pkg/controller/trustmanager/deployments.go b/pkg/controller/trustmanager/deployments.go index da96689d4..56e8bf02e 100644 --- a/pkg/controller/trustmanager/deployments.go +++ b/pkg/controller/trustmanager/deployments.go @@ -10,7 +10,6 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/utils/ptr" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/controller/common" @@ -23,26 +22,7 @@ func (r *Reconciler) createOrApplyDeployment(trustManager *v1alpha1.TrustManager return err } - deploymentName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling deployment resource", "name", deploymentName) - - existing := &appsv1.Deployment{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if deployment %q exists", deploymentName) - } - if exists && !deploymentModified(desired, existing) { - r.log.V(4).Info("deployment resource exists and is in desired state", "name", deploymentName) - return nil - } - - r.log.V(2).Info("deployment resource has been modified, updating to desired state", "name", deploymentName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply deployment %q", deploymentName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "deployment resource %s applied", deploymentName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &appsv1.Deployment{}, fieldOwner, deploymentModified) } func (r *Reconciler) getDeploymentObject(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string, caBundleHash string) (*appsv1.Deployment, error) { diff --git a/pkg/controller/trustmanager/deployments_test.go b/pkg/controller/trustmanager/deployments_test.go index 8e86c456d..34f96b5c1 100644 --- a/pkg/controller/trustmanager/deployments_test.go +++ b/pkg/controller/trustmanager/deployments_test.go @@ -593,7 +593,7 @@ func TestDeploymentReconciliation(t *testing.T) { return false, errTestClient }) }, - wantErr: "failed to check if deployment", + wantErr: "failed to check if Deployment", wantExistsCount: 1, wantPatchCount: 0, }, @@ -608,7 +608,7 @@ func TestDeploymentReconciliation(t *testing.T) { return errTestClient }) }, - wantErr: "failed to apply deployment", + wantErr: "failed to apply Deployment", wantExistsCount: 1, wantPatchCount: 1, }, diff --git a/pkg/controller/trustmanager/rbacs.go b/pkg/controller/trustmanager/rbacs.go index d8c3bcc04..4ff13a32c 100644 --- a/pkg/controller/trustmanager/rbacs.go +++ b/pkg/controller/trustmanager/rbacs.go @@ -1,13 +1,10 @@ package trustmanager import ( - "fmt" "reflect" "slices" - corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/controller/common" @@ -52,26 +49,7 @@ func (r *Reconciler) createOrApplyRBACResources(trustManager *v1alpha1.TrustMana func (r *Reconciler) createOrApplyClusterRole(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := getClusterRoleObject(trustManager.Spec.TrustManagerConfig.SecretTargets, resourceLabels, resourceAnnotations) - resourceName := desired.GetName() - r.log.V(4).Info("reconciling clusterrole resource", "name", resourceName) - - existing := &rbacv1.ClusterRole{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if clusterrole %q exists", resourceName) - } - if exists && !clusterRoleModified(desired, existing) { - r.log.V(4).Info("clusterrole resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("clusterrole resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply clusterrole %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "clusterrole resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &rbacv1.ClusterRole{}, fieldOwner, clusterRoleModified) } func getClusterRoleObject(secretTargets v1alpha1.SecretTargetsConfig, resourceLabels, resourceAnnotations map[string]string) *rbacv1.ClusterRole { @@ -83,9 +61,6 @@ func getClusterRoleObject(secretTargets v1alpha1.SecretTargetsConfig, resourceLa return clusterRole } -// appendSecretTargetRules adds cluster-wide secret read and scoped write rules -// to the ClusterRole when the secretTargets policy is Custom. The authorizedSecrets -// list is sorted to ensure deterministic rule ordering for comparison. func appendSecretTargetRules(clusterRole *rbacv1.ClusterRole, secretTargets v1alpha1.SecretTargetsConfig) { if !secretTargetsEnabled(secretTargets) { return @@ -112,26 +87,7 @@ func appendSecretTargetRules(clusterRole *rbacv1.ClusterRole, secretTargets v1al func (r *Reconciler) createOrApplyClusterRoleBinding(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := getClusterRoleBindingObject(resourceLabels, resourceAnnotations) - resourceName := desired.GetName() - r.log.V(4).Info("reconciling clusterrolebinding resource", "name", resourceName) - - existing := &rbacv1.ClusterRoleBinding{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if clusterrolebinding %q exists", resourceName) - } - if exists && !clusterRoleBindingModified(desired, existing) { - r.log.V(4).Info("clusterrolebinding resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("clusterrolebinding resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply clusterrolebinding %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "clusterrolebinding resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &rbacv1.ClusterRoleBinding{}, fieldOwner, clusterRoleBindingModified) } func getClusterRoleBindingObject(resourceLabels, resourceAnnotations map[string]string) *rbacv1.ClusterRoleBinding { @@ -144,30 +100,11 @@ func getClusterRoleBindingObject(resourceLabels, resourceAnnotations map[string] return clusterRoleBinding } -// Role for trust namespace (secrets access) +// Role for trust namespace func (r *Reconciler) createOrApplyTrustNamespaceRole(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string, trustNamespace string) error { desired := getTrustNamespaceRoleObject(resourceLabels, resourceAnnotations, trustNamespace) - resourceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling role resource for trust namespace", "name", resourceName) - - existing := &rbacv1.Role{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if role %q exists", resourceName) - } - if exists && !roleModified(desired, existing) { - r.log.V(4).Info("role resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("role resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply role %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "role resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &rbacv1.Role{}, fieldOwner, roleModified) } func getTrustNamespaceRoleObject(resourceLabels, resourceAnnotations map[string]string, trustNamespace string) *rbacv1.Role { @@ -179,30 +116,11 @@ func getTrustNamespaceRoleObject(resourceLabels, resourceAnnotations map[string] return role } -// RoleBinding for trust namespace (secrets access) +// RoleBinding for trust namespace func (r *Reconciler) createOrApplyTrustNamespaceRoleBinding(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string, trustNamespace string) error { desired := getTrustNamespaceRoleBindingObject(resourceLabels, resourceAnnotations, trustNamespace) - resourceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling rolebinding resource for trust namespace", "name", resourceName) - - existing := &rbacv1.RoleBinding{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if rolebinding %q exists", resourceName) - } - if exists && !roleBindingModified(desired, existing) { - r.log.V(4).Info("rolebinding resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("rolebinding resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply rolebinding %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "rolebinding resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &rbacv1.RoleBinding{}, fieldOwner, roleBindingModified) } func getTrustNamespaceRoleBindingObject(resourceLabels, resourceAnnotations map[string]string, trustNamespace string) *rbacv1.RoleBinding { @@ -216,30 +134,11 @@ func getTrustNamespaceRoleBindingObject(resourceLabels, resourceAnnotations map[ return roleBinding } -// Leader election Role (in operand namespace) +// Leader election Role func (r *Reconciler) createOrApplyLeaderElectionRole(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := getLeaderElectionRoleObject(resourceLabels, resourceAnnotations) - resourceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling leader election role resource", "name", resourceName) - - existing := &rbacv1.Role{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if leader election role %q exists", resourceName) - } - if exists && !roleModified(desired, existing) { - r.log.V(4).Info("leader election role resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("leader election role resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply leader election role %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "leader election role resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &rbacv1.Role{}, fieldOwner, roleModified) } func getLeaderElectionRoleObject(resourceLabels, resourceAnnotations map[string]string) *rbacv1.Role { @@ -251,30 +150,11 @@ func getLeaderElectionRoleObject(resourceLabels, resourceAnnotations map[string] return role } -// Leader election RoleBinding (in operand namespace) +// Leader election RoleBinding func (r *Reconciler) createOrApplyLeaderElectionRoleBinding(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := getLeaderElectionRoleBindingObject(resourceLabels, resourceAnnotations) - resourceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling leader election rolebinding resource", "name", resourceName) - - existing := &rbacv1.RoleBinding{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if leader election rolebinding %q exists", resourceName) - } - if exists && !roleBindingModified(desired, existing) { - r.log.V(4).Info("leader election rolebinding resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("leader election rolebinding resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply leader election rolebinding %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "leader election rolebinding resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &rbacv1.RoleBinding{}, fieldOwner, roleBindingModified) } func getLeaderElectionRoleBindingObject(resourceLabels, resourceAnnotations map[string]string) *rbacv1.RoleBinding { @@ -288,7 +168,6 @@ func getLeaderElectionRoleBindingObject(resourceLabels, resourceAnnotations map[ return roleBinding } -// updateBindingSubjects sets the ServiceAccount name and namespace on RBAC binding subjects. func updateBindingSubjects(subjects []rbacv1.Subject, serviceAccountName, namespace string) { for i := range subjects { if subjects[i].Kind == roleBindingSubjectKind { @@ -298,26 +177,22 @@ func updateBindingSubjects(subjects []rbacv1.Subject, serviceAccountName, namesp } } -// clusterRoleModified compares only the fields we manage via SSA. func clusterRoleModified(desired, existing *rbacv1.ClusterRole) bool { return managedMetadataModified(desired, existing) || !reflect.DeepEqual(desired.Rules, existing.Rules) } -// clusterRoleBindingModified compares only the fields we manage via SSA. func clusterRoleBindingModified(desired, existing *rbacv1.ClusterRoleBinding) bool { return managedMetadataModified(desired, existing) || !reflect.DeepEqual(desired.RoleRef, existing.RoleRef) || !reflect.DeepEqual(desired.Subjects, existing.Subjects) } -// roleModified compares only the fields we manage via SSA. func roleModified(desired, existing *rbacv1.Role) bool { return managedMetadataModified(desired, existing) || !reflect.DeepEqual(desired.Rules, existing.Rules) } -// roleBindingModified compares only the fields we manage via SSA. func roleBindingModified(desired, existing *rbacv1.RoleBinding) bool { return managedMetadataModified(desired, existing) || !reflect.DeepEqual(desired.RoleRef, existing.RoleRef) || diff --git a/pkg/controller/trustmanager/rbacs_test.go b/pkg/controller/trustmanager/rbacs_test.go index 634bffc30..83b65ad9f 100644 --- a/pkg/controller/trustmanager/rbacs_test.go +++ b/pkg/controller/trustmanager/rbacs_test.go @@ -486,7 +486,7 @@ func TestRBACReconciliation(t *testing.T) { return false, errTestClient }) }, - wantErr: "failed to check if clusterrole", + wantErr: "failed to check if ClusterRole", wantExistsCount: 1, wantPatchCount: 0, }, @@ -503,7 +503,7 @@ func TestRBACReconciliation(t *testing.T) { return nil }) }, - wantErr: "failed to apply clusterrole", + wantErr: "failed to apply ClusterRole", wantExistsCount: 1, wantPatchCount: 1, }, @@ -520,7 +520,7 @@ func TestRBACReconciliation(t *testing.T) { return nil }) }, - wantErr: "failed to apply clusterrolebinding", + wantErr: "failed to apply ClusterRoleBinding", wantExistsCount: 2, wantPatchCount: 2, }, @@ -537,7 +537,7 @@ func TestRBACReconciliation(t *testing.T) { return nil }) }, - wantErr: "failed to apply role", + wantErr: "failed to apply Role", wantExistsCount: 3, wantPatchCount: 3, }, @@ -554,7 +554,7 @@ func TestRBACReconciliation(t *testing.T) { return nil }) }, - wantErr: "failed to apply rolebinding", + wantErr: "failed to apply RoleBinding", wantExistsCount: 4, wantPatchCount: 4, }, diff --git a/pkg/controller/trustmanager/serviceaccounts.go b/pkg/controller/trustmanager/serviceaccounts.go index 844e6420f..c4b4a4a3a 100644 --- a/pkg/controller/trustmanager/serviceaccounts.go +++ b/pkg/controller/trustmanager/serviceaccounts.go @@ -1,11 +1,8 @@ package trustmanager import ( - "fmt" - corev1 "k8s.io/api/core/v1" "k8s.io/utils/ptr" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/controller/common" @@ -14,29 +11,9 @@ import ( func (r *Reconciler) createOrApplyServiceAccounts(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := r.getServiceAccountObject(resourceLabels, resourceAnnotations) - serviceAccountName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling serviceaccount resource", "name", serviceAccountName) - - existing := &corev1.ServiceAccount{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if serviceaccount %q exists", serviceAccountName) - } - if exists && !serviceAccountModified(desired, existing) { - r.log.V(4).Info("serviceaccount resource exists and is in desired state", "name", serviceAccountName) - return nil - } - - r.log.V(2).Info("serviceaccount resource has been modified, updating to desired state", "name", serviceAccountName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply serviceaccount %q", serviceAccountName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "serviceaccount resource %s applied", serviceAccountName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &corev1.ServiceAccount{}, fieldOwner, serviceAccountModified) } -// serviceAccountModified compares only the fields we manage via SSA. func serviceAccountModified(desired, existing *corev1.ServiceAccount) bool { return managedMetadataModified(desired, existing) || !ptr.Equal(desired.AutomountServiceAccountToken, existing.AutomountServiceAccountToken) diff --git a/pkg/controller/trustmanager/serviceaccounts_test.go b/pkg/controller/trustmanager/serviceaccounts_test.go index 70c119582..0b179c455 100644 --- a/pkg/controller/trustmanager/serviceaccounts_test.go +++ b/pkg/controller/trustmanager/serviceaccounts_test.go @@ -149,7 +149,7 @@ func TestServiceAccountReconciliation(t *testing.T) { return false, errTestClient }) }, - wantErr: "failed to check if serviceaccount", + wantErr: "failed to check if ServiceAccount", wantExistsCount: 1, wantPatchCount: 0, }, @@ -163,7 +163,7 @@ func TestServiceAccountReconciliation(t *testing.T) { return errTestClient }) }, - wantErr: "failed to apply serviceaccount", + wantErr: "failed to apply ServiceAccount", wantExistsCount: 1, wantPatchCount: 1, }, diff --git a/pkg/controller/trustmanager/services.go b/pkg/controller/trustmanager/services.go index 85c28294e..7ab98f84a 100644 --- a/pkg/controller/trustmanager/services.go +++ b/pkg/controller/trustmanager/services.go @@ -1,12 +1,10 @@ package trustmanager import ( - "fmt" "maps" "reflect" corev1 "k8s.io/api/core/v1" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/controller/common" @@ -24,29 +22,9 @@ func (r *Reconciler) createOrApplyServices(trustManager *v1alpha1.TrustManager, } func (r *Reconciler) createOrApplyService(trustManager *v1alpha1.TrustManager, desired *corev1.Service) error { - serviceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) - r.log.V(4).Info("reconciling service resource", "name", serviceName) - - existing := &corev1.Service{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if service %q exists", serviceName) - } - if exists && !serviceModified(desired, existing) { - r.log.V(4).Info("service resource exists and is in desired state", "name", serviceName) - return nil - } - - r.log.V(2).Info("service resource has been modified, updating to desired state", "name", serviceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply service %q", serviceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "service resource %s applied", serviceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &corev1.Service{}, fieldOwner, serviceModified) } -// serviceModified compares only the fields we manage via SSA. func serviceModified(desired, existing *corev1.Service) bool { if managedMetadataModified(desired, existing) { return true diff --git a/pkg/controller/trustmanager/services_test.go b/pkg/controller/trustmanager/services_test.go index 4c7432370..79acf86b8 100644 --- a/pkg/controller/trustmanager/services_test.go +++ b/pkg/controller/trustmanager/services_test.go @@ -209,7 +209,7 @@ func TestServiceReconciliation(t *testing.T) { return false, errTestClient }) }, - wantErr: "failed to check if service", + wantErr: "failed to check if Service", wantExistsCount: 1, wantPatchCount: 0, }, @@ -223,7 +223,7 @@ func TestServiceReconciliation(t *testing.T) { return errTestClient }) }, - wantErr: "failed to apply service", + wantErr: "failed to apply Service", wantExistsCount: 1, wantPatchCount: 1, }, @@ -242,7 +242,7 @@ func TestServiceReconciliation(t *testing.T) { return nil }) }, - wantErr: "failed to apply service", + wantErr: "failed to apply Service", wantExistsCount: 2, wantPatchCount: 2, }, diff --git a/pkg/controller/trustmanager/webhooks.go b/pkg/controller/trustmanager/webhooks.go index 51e85cd7f..011d914aa 100644 --- a/pkg/controller/trustmanager/webhooks.go +++ b/pkg/controller/trustmanager/webhooks.go @@ -6,9 +6,7 @@ import ( "reflect" "slices" - corev1 "k8s.io/api/core/v1" "k8s.io/utils/ptr" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/controller/common" @@ -19,26 +17,7 @@ import ( func (r *Reconciler) createOrApplyValidatingWebhookConfiguration(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { desired := getValidatingWebhookConfigObject(resourceLabels, resourceAnnotations) - resourceName := desired.GetName() - r.log.V(4).Info("reconciling validatingwebhookconfiguration resource", "name", resourceName) - - existing := &admissionregistrationv1.ValidatingWebhookConfiguration{} - exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - return common.FromClientError(err, "failed to check if validatingwebhookconfiguration %q exists", resourceName) - } - if exists && !webhookConfigModified(desired, existing) { - r.log.V(4).Info("validatingwebhookconfiguration resource exists and is in desired state", "name", resourceName) - return nil - } - - r.log.V(2).Info("validatingwebhookconfiguration resource has been modified, updating to desired state", "name", resourceName) - if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { - return common.FromClientError(err, "failed to apply validatingwebhookconfiguration %q", resourceName) - } - - r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "validatingwebhookconfiguration resource %s applied", resourceName) - return nil + return common.ApplyResource(r.ctx, r.CtrlClient, r.log, r.eventRecorder, trustManager, desired, &admissionregistrationv1.ValidatingWebhookConfiguration{}, fieldOwner, webhookConfigModified) } func getValidatingWebhookConfigObject(resourceLabels, resourceAnnotations map[string]string) *admissionregistrationv1.ValidatingWebhookConfiguration { @@ -52,7 +31,6 @@ func getValidatingWebhookConfigObject(resourceLabels, resourceAnnotations map[st return webhookConfig } -// updateWebhookClientConfig sets the webhook clientConfig service name and namespace. func updateWebhookClientConfig(webhookConfig *admissionregistrationv1.ValidatingWebhookConfiguration) { for i := range webhookConfig.Webhooks { if webhookConfig.Webhooks[i].ClientConfig.Service != nil { @@ -62,9 +40,6 @@ func updateWebhookClientConfig(webhookConfig *admissionregistrationv1.Validating } } -// updateWebhookAnnotations merges user-provided annotations with the required -// cert-manager CA injection annotation. The CA injection annotation references -// the Certificate resource by namespace/name and is constructed dynamically. func updateWebhookAnnotations(webhookConfig *admissionregistrationv1.ValidatingWebhookConfiguration, resourceAnnotations map[string]string) { mergedAnnotations := make(map[string]string, len(resourceAnnotations)+1) maps.Copy(mergedAnnotations, resourceAnnotations) @@ -72,9 +47,6 @@ func updateWebhookAnnotations(webhookConfig *admissionregistrationv1.ValidatingW webhookConfig.SetAnnotations(mergedAnnotations) } -// webhookConfigModified compares only the fields we manage via SSA. -// Individual webhook fields are compared explicitly because the API server -// defaults fields like matchPolicy, namespaceSelector, and objectSelector. func webhookConfigModified(desired, existing *admissionregistrationv1.ValidatingWebhookConfiguration) bool { if managedMetadataModified(desired, existing) { return true diff --git a/pkg/controller/trustmanager/webhooks_test.go b/pkg/controller/trustmanager/webhooks_test.go index 17841c685..39a38b469 100644 --- a/pkg/controller/trustmanager/webhooks_test.go +++ b/pkg/controller/trustmanager/webhooks_test.go @@ -204,7 +204,7 @@ func TestValidatingWebhookConfigReconciliation(t *testing.T) { return false, errTestClient }) }, - wantErr: "failed to check if validatingwebhookconfiguration", + wantErr: "failed to check if ValidatingWebhookConfiguration", wantExistsCount: 1, wantPatchCount: 0, }, @@ -218,7 +218,7 @@ func TestValidatingWebhookConfigReconciliation(t *testing.T) { return errTestClient }) }, - wantErr: "failed to apply validatingwebhookconfiguration", + wantErr: "failed to apply ValidatingWebhookConfiguration", wantExistsCount: 1, wantPatchCount: 1, },