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, },