🌱 Use Patch instead of Update for finalizer operations (#2328)

* 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 <tshort@redhat.com>

* Revert "Restore last-applied-config annotation in cache (#2338)"

This reverts commit 39cbdbe.

---------

Signed-off-by: Todd Short <tshort@redhat.com>

Author: tmshort

cmd/catalogd/main.go

@@ -59,6 +59,7 @@ import (
 	"github.com/operator-framework/operator-controller/internal/catalogd/storage"
 	"github.com/operator-framework/operator-controller/internal/catalogd/webhook"
 	sharedcontrollers "github.com/operator-framework/operator-controller/internal/shared/controllers"
+	cacheutil "github.com/operator-framework/operator-controller/internal/shared/util/cache"
 	fsutil "github.com/operator-framework/operator-controller/internal/shared/util/fs"
 	httputil "github.com/operator-framework/operator-controller/internal/shared/util/http"
 	imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image"
@@ -254,6 +255,8 @@ func run(ctx context.Context) error {
 
 	cacheOptions := crcache.Options{
 		ByObject: map[client.Object]crcache.ByObject{},
+		// Memory optimization: strip managed fields and large annotations from cached objects
+		DefaultTransform: cacheutil.StripManagedFieldsAndAnnotations(),
 	}
 
 	saKey, err := sautil.GetServiceAccount()

cmd/operator-controller/main.go

@@ -78,6 +78,7 @@ import (
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render/registryv1"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/scheme"
 	sharedcontrollers "github.com/operator-framework/operator-controller/internal/shared/controllers"
+	cacheutil "github.com/operator-framework/operator-controller/internal/shared/util/cache"
 	fsutil "github.com/operator-framework/operator-controller/internal/shared/util/fs"
 	httputil "github.com/operator-framework/operator-controller/internal/shared/util/http"
 	imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image"
@@ -257,6 +258,8 @@ func run() error {
 			cfg.systemNamespace: {LabelSelector: k8slabels.Everything()},
 		},
 		DefaultLabelSelector: k8slabels.Nothing(),
+		// Memory optimization: strip managed fields and large annotations from cached objects
+		DefaultTransform: cacheutil.StripAnnotations(),
 	}
 
 	if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) {

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/applier/boxcutter.go

@@ -27,6 +27,7 @@ import (
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/labels"
+	"github.com/operator-framework/operator-controller/internal/shared/util/cache"
 )
 
 const (
@@ -66,6 +67,9 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease(
 		maps.Copy(labels, objectLabels)
 		obj.SetLabels(labels)
 
+		// Memory optimization: strip large annotations
+		// Note: ApplyStripTransform never returns an error in practice
+		_ = cache.ApplyStripAnnotationsTransform(&obj)
 		sanitizedUnstructured(ctx, &obj)
 
 		objs = append(objs, ocv1.ClusterExtensionRevisionObject{
@@ -117,6 +121,10 @@ func (r *SimpleRevisionGenerator) GenerateRevision(
 		unstr := unstructured.Unstructured{Object: unstrObj}
 		unstr.SetGroupVersionKind(gvk)
 
+		// Memory optimization: strip large annotations
+		if err := cache.ApplyStripAnnotationsTransform(&unstr); err != nil {
+			return nil, err
+		}
 		sanitizedUnstructured(ctx, &unstr)
 
 		objs = append(objs, ocv1.ClusterExtensionRevisionObject{

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/cache/transform.go

@@ -0,0 +1,91 @@
+package cache
+
+import (
+	"maps"
+
+	toolscache "k8s.io/client-go/tools/cache"
+	crcache "sigs.k8s.io/controller-runtime/pkg/cache"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+// stripAnnotations removes memory-heavy annotations that aren't needed for controller operations.
+func stripAnnotations(obj interface{}) (interface{}, error) {
+	if metaObj, ok := obj.(client.Object); ok {
+		// Remove the last-applied-configuration annotation which can be very large
+		// Clone the annotations map to avoid modifying shared references
+		annotations := metaObj.GetAnnotations()
+		if annotations != nil {
+			annotations = maps.Clone(annotations)
+			delete(annotations, "kubectl.kubernetes.io/last-applied-configuration")
+			if len(annotations) == 0 {
+				metaObj.SetAnnotations(nil)
+			} else {
+				metaObj.SetAnnotations(annotations)
+			}
+		}
+	}
+	return obj, nil
+}
+
+// StripManagedFieldsAndAnnotations returns a cache transform function that removes
+// memory-heavy fields that aren't needed for controller operations.
+// This significantly reduces memory usage in informer caches by removing:
+// - Managed fields (can be several KB per object)
+// - kubectl.kubernetes.io/last-applied-configuration annotation (can be very large)
+//
+// Use this function as a DefaultTransform in controller-runtime cache.Options
+// to reduce memory overhead across all cached objects.
+//
+// Example:
+//
+//	cacheOptions := cache.Options{
+//	    DefaultTransform: cacheutil.StripManagedFieldsAndAnnotations(),
+//	}
+func StripManagedFieldsAndAnnotations() toolscache.TransformFunc {
+	// Use controller-runtime's built-in TransformStripManagedFields and compose it
+	// with our custom annotation stripping transform
+	managedFieldsTransform := crcache.TransformStripManagedFields()
+
+	return func(obj interface{}) (interface{}, error) {
+		// First strip managed fields using controller-runtime's transform
+		obj, err := managedFieldsTransform(obj)
+		if err != nil {
+			return obj, err
+		}
+
+		// Then strip the large annotations
+		return stripAnnotations(obj)
+	}
+}
+
+// StripAnnotations returns a cache transform function that removes
+// memory-heavy annotation fields that aren't needed for controller operations.
+// This significantly reduces memory usage in informer caches by removing:
+// - kubectl.kubernetes.io/last-applied-configuration annotation (can be very large)
+//
+// Use this function as a DefaultTransform in controller-runtime cache.Options
+// to reduce memory overhead across all cached objects.
+//
+// Example:
+//
+//	cacheOptions := cache.Options{
+//	    DefaultTransform: cacheutil.StripAnnotations(),
+//	}
+func StripAnnotations() toolscache.TransformFunc {
+	return func(obj interface{}) (interface{}, error) {
+		// Strip the large annotations
+		return stripAnnotations(obj)
+	}
+}
+
+// ApplyStripAnnotationsTransform applies the strip transform directly to an object.
+// This is a convenience function for cases where you need to strip fields
+// from an object outside of the cache transform context.
+//
+// Note: This function never returns an error in practice, but returns error
+// to satisfy the TransformFunc interface.
+func ApplyStripAnnotationsTransform(obj client.Object) error {
+	transform := StripAnnotations()
+	_, err := transform(obj)
+	return err
+}

internal/shared/util/k8s/k8s.go

@@ -0,0 +1,54 @@
+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.
+//
+// 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(a, b metav1.Object) 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())
+}