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

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,57 @@
+package k8s
+
+import (
+	"reflect"
+
+	"k8s.io/apimachinery/pkg/api/equality"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+// 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 (using reflection to access the Spec field, with semantic equality)
+//
+// Status and finalizers are intentionally not compared, as these are expected
+// to change during reconciliation.
+//
+// Type parameter:
+//   - O: The object type that implements metav1.Object and has a Spec field
+//
+// This function uses reflection to access the Spec field, so no explicit GetSpec()
+// method is required. The objects must have a field named "Spec".
+func CheckForUnexpectedFieldChange[O metav1.Object](a, b O) bool {
+	if !equality.Semantic.DeepEqual(a.GetAnnotations(), b.GetAnnotations()) {
+		return true
+	}
+	if !equality.Semantic.DeepEqual(a.GetLabels(), b.GetLabels()) {
+		return true
+	}
+
+	// Use reflection to access the Spec field
+	aVal := reflect.ValueOf(a)
+	bVal := reflect.ValueOf(b)
+
+	// Handle pointer types
+	if aVal.Kind() == reflect.Ptr {
+		aVal = aVal.Elem()
+	}
+	if bVal.Kind() == reflect.Ptr {
+		bVal = bVal.Elem()
+	}
+
+	// Get the Spec field from both objects
+	aSpec := aVal.FieldByName("Spec")
+	bSpec := bVal.FieldByName("Spec")
+
+	// If either Spec field is invalid, return false (no change detected)
+	if !aSpec.IsValid() || !bSpec.IsValid() {
+		return false
+	}
+
+	return !equality.Semantic.DeepEqual(aSpec.Interface(), bSpec.Interface())
+}