CM-1040: Extract shared ApplyResource helper and migrate istiocsr to SSA

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

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) {

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

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"

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,49 +85,59 @@ 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
 					}
 					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)
 					}
 					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)
 					}
 					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`,
 		},
 	}
 

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
 	}