Use Patch instead of Update for finalizer operations

Refactor all controllers to use Patch() instead of Update()
when adding or removing finalizers to improve performance, and to avoid
removing non-cached fields erroneously.

This is necesary because we no longer cache the last-applied-configuration
annotation, so when we add/remove the finalizers, we are removing that field
from the metadata. This causes issues with clients when they don't see that
annotation (e.g. apply the same ClusterExtension twice).

Update ClusterCatalog and ClusterExtension controllers to use Patch-based
funalizer management (ClusterExtensionRevision already uses Patch())

Signed-off-by: Todd Short <[email protected]>

Author: tmshort

api/v1/helpers.go

@@ -0,0 +1,19 @@
+package v1
+
+// GetSpec returns the Spec field of the ClusterExtension.
+// This method is provided to satisfy generic interfaces that require a GetSpec() method.
+func (c *ClusterExtension) GetSpec() ClusterExtensionSpec {
+	return c.Spec
+}
+
+// GetSpec returns the Spec field of the ClusterCatalog.
+// This method is provided to satisfy generic interfaces that require a GetSpec() method.
+func (c *ClusterCatalog) GetSpec() ClusterCatalogSpec {
+	return c.Spec
+}
+
+// GetSpec returns the Spec field of the ClusterExtensionRevision.
+// This method is provided to satisfy generic interfaces that require a GetSpec() method.
+func (c *ClusterExtensionRevision) GetSpec() ClusterExtensionRevisionSpec {
+	return c.Spec
+}

internal/catalogd/controllers/core/clustercatalog_controller.go

@@ -41,6 +41,7 @@ import (
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	"github.com/operator-framework/operator-controller/internal/catalogd/storage"
 	imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image"
+	k8sutil "github.com/operator-framework/operator-controller/internal/shared/util/k8s"
 )
 
 const (
@@ -107,16 +108,16 @@ func (r *ClusterCatalogReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	// Do checks before any Update()s, as Update() may modify the resource structure!
 	updateStatus := !equality.Semantic.DeepEqual(existingCatsrc.Status, reconciledCatsrc.Status)
 	updateFinalizers := !equality.Semantic.DeepEqual(existingCatsrc.Finalizers, reconciledCatsrc.Finalizers)
-	unexpectedFieldsChanged := checkForUnexpectedFieldChange(existingCatsrc, *reconciledCatsrc)
+	unexpectedFieldsChanged := k8sutil.CheckForUnexpectedFieldChange(&existingCatsrc, reconciledCatsrc)
 
 	if unexpectedFieldsChanged {
 		panic("spec or metadata changed by reconciler")
 	}
 
 	// Save the finalizers off to the side. If we update the status, the reconciledCatsrc will be updated
 	// to contain the new state of the ClusterCatalog, which contains the status update, but (critically)
-	// does not contain the finalizers. After the status update, we need to re-add the finalizers to the
-	// reconciledCatsrc before updating the object.
+	// does not contain the finalizers. After the status update, we will use the saved finalizers in the
+	// CreateOrPatch()
 	finalizers := reconciledCatsrc.Finalizers
 
 	if updateStatus {
@@ -125,10 +126,12 @@ func (r *ClusterCatalogReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		}
 	}
 
-	reconciledCatsrc.Finalizers = finalizers
-
 	if updateFinalizers {
-		if err := r.Update(ctx, reconciledCatsrc); err != nil {
+		// Use CreateOrPatch to update finalizers on the server
+		if _, err := controllerutil.CreateOrPatch(ctx, r.Client, reconciledCatsrc, func() error {
+			reconciledCatsrc.Finalizers = finalizers
+			return nil
+		}); err != nil {
 			reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating finalizers: %v", err))
 		}
 	}
@@ -415,13 +418,6 @@ func (r *ClusterCatalogReconciler) needsPoll(lastSuccessfulPoll time.Time, catal
 	return nextPoll.Before(time.Now())
 }
 
-// Compare resources - ignoring status & metadata.finalizers
-func checkForUnexpectedFieldChange(a, b ocv1.ClusterCatalog) bool {
-	a.Status, b.Status = ocv1.ClusterCatalogStatus{}, ocv1.ClusterCatalogStatus{}
-	a.Finalizers, b.Finalizers = []string{}, []string{}
-	return !equality.Semantic.DeepEqual(a, b)
-}
-
 type finalizerFunc func(ctx context.Context, obj client.Object) (crfinalizer.Result, error)
 
 func (f finalizerFunc) Finalize(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {

internal/operator-controller/controllers/clusterextension_controller.go

@@ -35,6 +35,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	crcontroller "sigs.k8s.io/controller-runtime/pkg/controller"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	crhandler "sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/log"
@@ -48,6 +49,7 @@ import (
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/conditionsets"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/labels"
+	k8sutil "github.com/operator-framework/operator-controller/internal/shared/util/k8s"
 )
 
 const (
@@ -135,25 +137,28 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	updateFinalizers := !equality.Semantic.DeepEqual(existingExt.Finalizers, reconciledExt.Finalizers)
 
 	// If any unexpected fields have changed, panic before updating the resource
-	unexpectedFieldsChanged := checkForUnexpectedClusterExtensionFieldChange(*existingExt, *reconciledExt)
+	unexpectedFieldsChanged := k8sutil.CheckForUnexpectedFieldChange(existingExt, reconciledExt)
 	if unexpectedFieldsChanged {
 		panic("spec or metadata changed by reconciler")
 	}
 
 	// Save the finalizers off to the side. If we update the status, the reconciledExt will be updated
 	// to contain the new state of the ClusterExtension, which contains the status update, but (critically)
-	// does not contain the finalizers. After the status update, we need to re-add the finalizers to the
-	// reconciledExt before updating the object.
+	// does not contain the finalizers. After the status update, we will use the saved finalizers in the
+	// CreateOrPatch()
 	finalizers := reconciledExt.Finalizers
 	if updateStatus {
 		if err := r.Client.Status().Update(ctx, reconciledExt); err != nil {
 			reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating status: %v", err))
 		}
 	}
-	reconciledExt.Finalizers = finalizers
 
 	if updateFinalizers {
-		if err := r.Update(ctx, reconciledExt); err != nil {
+		// Use CreateOrPatch to update finalizers on the server
+		if _, err := controllerutil.CreateOrPatch(ctx, r.Client, reconciledExt, func() error {
+			reconciledExt.Finalizers = finalizers
+			return nil
+		}); err != nil {
 			reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating finalizers: %v", err))
 		}
 	}
@@ -179,13 +184,6 @@ func ensureAllConditionsWithReason(ext *ocv1.ClusterExtension, reason v1alpha1.C
 	}
 }
 
-// Compare resources - ignoring status & metadata.finalizers
-func checkForUnexpectedClusterExtensionFieldChange(a, b ocv1.ClusterExtension) bool {
-	a.Status, b.Status = ocv1.ClusterExtensionStatus{}, ocv1.ClusterExtensionStatus{}
-	a.Finalizers, b.Finalizers = []string{}, []string{}
-	return !equality.Semantic.DeepEqual(a, b)
-}
-
 // SetDeprecationStatus will set the appropriate deprecation statuses for a ClusterExtension
 // based on the provided bundle
 func SetDeprecationStatus(ext *ocv1.ClusterExtension, bundleName string, deprecation *declcfg.Deprecation) {

internal/shared/util/k8s/k8s.go

@@ -0,0 +1,41 @@
+package k8s
+
+import (
+	"k8s.io/apimachinery/pkg/api/equality"
+)
+
+// ObjectWithSpec represents any Kubernetes custom resource that embeds metav1.ObjectMeta
+// and has a Spec field. The type parameter T should be the Spec type.
+type ObjectWithSpec[T any] interface {
+	GetAnnotations() map[string]string
+	GetLabels() map[string]string
+	GetSpec() T
+}
+
+// CheckForUnexpectedFieldChange compares two Kubernetes objects and returns true
+// if their annotations, labels, or spec have changed. This is useful for detecting
+// unexpected modifications during reconciliation.
+//
+// The function compares:
+//   - Annotations (via GetAnnotations)
+//   - Labels (via GetLabels)
+//   - Spec (via GetSpec, using semantic equality)
+//
+// Status and finalizers are intentionally not compared, as these are expected
+// to change during reconciliation.
+//
+// Type parameters:
+//   - T: The Spec type for the object (e.g., ClusterExtensionSpec, ClusterCatalogSpec)
+//   - O: The object type that implements ObjectWithSpec[T]
+//
+// This function works with any object that implements the ObjectWithSpec interface,
+// which requires GetAnnotations(), GetLabels(), and GetSpec() methods.
+func CheckForUnexpectedFieldChange[T any, O ObjectWithSpec[T]](a, b O) bool {
+	if !equality.Semantic.DeepEqual(a.GetAnnotations(), b.GetAnnotations()) {
+		return true
+	}
+	if !equality.Semantic.DeepEqual(a.GetLabels(), b.GetLabels()) {
+		return true
+	}
+	return !equality.Semantic.DeepEqual(a.GetSpec(), b.GetSpec())
+}