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

Both istiocsr and trustmanager had 8-10 nearly identical createOrApply
methods with duplicated reconciliation boilerplate. trustmanager used
Server-Side Apply while istiocsr used Create/UpdateWithRetry.

Add a generic common.ApplyResource[T]() helper that handles the common
Exists/drift-check/Patch pattern with a pluggable hasChanged callback.
The helper derives the Kubernetes kind from the type parameter for
clear error messages and uses client.ObjectKeyFromObject for consistent
resource name formatting.

Migrate all trustmanager methods and istiocsr simple methods to use it.
ClusterRole/ClusterRoleBinding methods in istiocsr are kept as-is since
they use GenerateName with List fallback and Delete for immutable RoleRef
changes.

The istioCSRCreateRecon warning events are dropped from the migrated
methods since SSA is inherently idempotent.

Author: sebrandon1

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
 	}