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. Create shared finalizer utilities
to eliminate code duplication across controllers.

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

- Add shared finalizer.EnsureFinalizer() utilities
- Update ClusterCatalog, ClusterExtension, and ClusterExtensionRevision
  controllers to use Patch-based finalizer management
- Maintain early return behavior after adding finalizers on create
- Remove unused internal/operator-controller/finalizers package
- Update all unit tests to match new behavior

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Signed-off-by: Todd Short <[email protected]>

Author: tmshort

cmd/operator-controller/main.go

@@ -51,7 +51,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/certwatcher"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	crcontroller "sigs.k8s.io/controller-runtime/pkg/controller"
-	crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer"
 	"sigs.k8s.io/controller-runtime/pkg/healthz"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
@@ -70,7 +69,6 @@ import (
 	"github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/controllers"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/features"
-	"github.com/operator-framework/operator-controller/internal/operator-controller/finalizers"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/resolve"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/preflights/crdupgradesafety"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render"
@@ -388,12 +386,11 @@ func run() error {
 		},
 	}
 
-	clusterExtensionFinalizers := crfinalizer.NewFinalizers()
-	if err := clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupUnpackCacheFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
-		return crfinalizer.Result{}, imageCache.Delete(ctx, obj.GetName())
-	})); err != nil {
-		setupLog.Error(err, "unable to register finalizer", "finalizerKey", controllers.ClusterExtensionCleanupUnpackCacheFinalizer)
-		return err
+	// Set up finalizer handlers for ClusterExtension
+	clusterExtensionFinalizerHandlers := map[string]controllers.FinalizerHandler{
+		controllers.ClusterExtensionCleanupUnpackCacheFinalizer: func(ctx context.Context, ext *ocv1.ClusterExtension) error {
+			return imageCache.Delete(ctx, ext.GetName())
+		},
 	}
 
 	cl := mgr.GetClient()
@@ -440,11 +437,11 @@ func run() error {
 	}
 
 	ceReconciler := &controllers.ClusterExtensionReconciler{
-		Client:      cl,
-		Resolver:    resolver,
-		ImageCache:  imageCache,
-		ImagePuller: imagePuller,
-		Finalizers:  clusterExtensionFinalizers,
+		Client:            cl,
+		Resolver:          resolver,
+		ImageCache:        imageCache,
+		ImagePuller:       imagePuller,
+		FinalizerHandlers: clusterExtensionFinalizerHandlers,
 	}
 	ceController, err := ceReconciler.SetupWithManager(mgr, ctrlBuilderOpts...)
 	if err != nil {
@@ -461,9 +458,9 @@ func run() error {
 	}
 
 	if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) {
-		err = setupBoxcutter(mgr, ceReconciler, preflights, clusterExtensionFinalizers, regv1ManifestProvider)
+		err = setupBoxcutter(mgr, ceReconciler, preflights, regv1ManifestProvider)
 	} else {
-		err = setupHelm(mgr, ceReconciler, preflights, ceController, clusterExtensionFinalizers, regv1ManifestProvider)
+		err = setupHelm(mgr, ceReconciler, preflights, ceController, regv1ManifestProvider)
 	}
 	if err != nil {
 		setupLog.Error(err, "unable to setup lifecycler")
@@ -528,7 +525,6 @@ func setupBoxcutter(
 	mgr manager.Manager,
 	ceReconciler *controllers.ClusterExtensionReconciler,
 	preflights []applier.Preflight,
-	clusterExtensionFinalizers crfinalizer.Registerer,
 	regv1ManifestProvider applier.ManifestProvider,
 ) error {
 	coreClient, err := corev1client.NewForConfig(mgr.GetConfig())
@@ -557,13 +553,9 @@ func setupBoxcutter(
 	// This finalizer was added by the Helm applier for ClusterExtensions created
 	// before BoxcutterRuntime was enabled. Boxcutter doesn't use contentmanager,
 	// so we just need to acknowledge the finalizer to allow deletion to proceed.
-	err = clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupContentManagerCacheFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
+	ceReconciler.FinalizerHandlers[controllers.ClusterExtensionCleanupContentManagerCacheFinalizer] = func(ctx context.Context, ext *ocv1.ClusterExtension) error {
 		// No-op: Boxcutter doesn't use contentmanager, so no cleanup is needed
-		return crfinalizer.Result{}, nil
-	}))
-	if err != nil {
-		setupLog.Error(err, "unable to register content manager cleanup finalizer for boxcutter")
-		return err
+		return nil
 	}
 
 	// TODO: add support for preflight checks
@@ -636,7 +628,6 @@ func setupHelm(
 	ceReconciler *controllers.ClusterExtensionReconciler,
 	preflights []applier.Preflight,
 	ceController crcontroller.Controller,
-	clusterExtensionFinalizers crfinalizer.Registerer,
 	regv1ManifestProvider applier.ManifestProvider,
 ) error {
 	coreClient, err := corev1client.NewForConfig(mgr.GetConfig())
@@ -675,14 +666,9 @@ func setupHelm(
 	}
 
 	cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper())
-	err = clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupContentManagerCacheFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
-		ext := obj.(*ocv1.ClusterExtension)
-		err := cm.Delete(ext)
-		return crfinalizer.Result{}, err
-	}))
-	if err != nil {
-		setupLog.Error(err, "unable to register content manager cleanup finalizer")
-		return err
+	// Register the content manager cleanup finalizer handler
+	ceReconciler.FinalizerHandlers[controllers.ClusterExtensionCleanupContentManagerCacheFinalizer] = func(ctx context.Context, ext *ocv1.ClusterExtension) error {
+		return cm.Delete(ext)
 	}
 
 	// now initialize the helmApplier, assigning the potentially nil preAuth

internal/catalogd/controllers/core/clustercatalog_controller.go

@@ -34,12 +34,12 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
-	crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	"github.com/operator-framework/operator-controller/internal/catalogd/storage"
+	finalizerutil "github.com/operator-framework/operator-controller/internal/shared/util/finalizer"
 	imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image"
 )
 
@@ -59,8 +59,6 @@ type ClusterCatalogReconciler struct {
 
 	Storage storage.Instance
 
-	finalizers crfinalizer.Finalizers
-
 	// TODO: The below storedCatalogs fields are used for a quick a hack that helps
 	//    us correctly populate a ClusterCatalog's status. The fact that we need
 	//    these is indicative of a larger problem with the design of one or both
@@ -106,33 +104,18 @@ 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)
 
 	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.
-	finalizers := reconciledCatsrc.Finalizers
-
 	if updateStatus {
 		if err := r.Client.Status().Update(ctx, reconciledCatsrc); err != nil {
 			reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating status: %v", err))
 		}
 	}
 
-	reconciledCatsrc.Finalizers = finalizers
-
-	if updateFinalizers {
-		if err := r.Update(ctx, reconciledCatsrc); err != nil {
-			reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating finalizers: %v", err))
-		}
-	}
-
 	return res, reconcileErr
 }
 
@@ -142,10 +125,6 @@ func (r *ClusterCatalogReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	defer r.storedCatalogsMu.Unlock()
 	r.storedCatalogs = make(map[string]storedCatalogData)
 
-	if err := r.setupFinalizers(); err != nil {
-		return fmt.Errorf("failed to setup finalizers: %v", err)
-	}
-
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&ocv1.ClusterCatalog{}).
 		Named("catalogd-clustercatalog-controller").
@@ -171,32 +150,47 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *ocv1.
 			return ctrl.Result{}, err
 		}
 
-		// Set status.conditions[type=Progressing] to False as we are done with
-		// all that needs to be done with the catalog
-		updateStatusProgressingUserSpecifiedUnavailable(&catalog.Status, catalog.GetGeneration())
-
 		// Remove the fbcDeletionFinalizer as we do not want a finalizer attached to the catalog
 		// when it is disabled. Because the finalizer serves no purpose now.
-		controllerutil.RemoveFinalizer(catalog, fbcDeletionFinalizer)
+		if err := finalizerutil.RemoveFinalizer(ctx, r.Client, catalog, fbcDeletionFinalizer); err != nil {
+			return ctrl.Result{}, fmt.Errorf("error removing finalizer: %v", err)
+		}
+
+		// Set status.conditions[type=Progressing] to True as we are done with
+		// all that needs to be done with the catalog
+		updateStatusProgressingUserSpecifiedUnavailable(&catalog.Status, catalog.GetGeneration())
+		// Clear URLs, ResolvedSource, and LastUnpacked since catalog is unavailable
+		catalog.Status.ResolvedSource = nil
+		catalog.Status.URLs = nil
+		catalog.Status.LastUnpacked = nil
 
 		return ctrl.Result{}, nil
 	}
 
-	finalizeResult, err := r.finalizers.Finalize(ctx, catalog)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-	if finalizeResult.Updated || finalizeResult.StatusUpdated {
-		// On create: make sure the finalizer is applied before we do anything
-		// On delete: make sure we do nothing after the finalizer is removed
+	// Handle deletion
+	if catalog.GetDeletionTimestamp() != nil {
+		if !controllerutil.ContainsFinalizer(catalog, fbcDeletionFinalizer) {
+			// All finalizers removed, nothing more to do
+			return ctrl.Result{}, nil
+		}
+		if err := r.deleteCatalogCache(ctx, catalog); err != nil {
+			return ctrl.Result{}, fmt.Errorf("finalizer %q failed: %w", fbcDeletionFinalizer, err)
+		}
+		if err := finalizerutil.RemoveFinalizer(ctx, r.Client, catalog, fbcDeletionFinalizer); err != nil {
+			return ctrl.Result{}, fmt.Errorf("error removing finalizer: %v", err)
+		}
+		// Update status to reflect that catalog is no longer serving
+		updateStatusNotServing(&catalog.Status, catalog.GetGeneration())
 		return ctrl.Result{}, nil
 	}
 
-	if catalog.GetDeletionTimestamp() != nil {
-		// If we've gotten here, that means the cluster catalog is being deleted, we've handled all of
-		// _our_ finalizers (above), but the cluster catalog is still present in the cluster, likely
-		// because there are _other_ finalizers that other controllers need to handle, (e.g. the orphan
-		// deletion finalizer).
+	// Ensure finalizer is present
+	finalizerAdded, err := finalizerutil.EnsureFinalizer(ctx, r.Client, catalog, fbcDeletionFinalizer)
+	if err != nil {
+		return ctrl.Result{}, fmt.Errorf("error ensuring finalizer: %v", err)
+	}
+	// On create: make sure the finalizer is applied before we do anything else
+	if finalizerAdded {
 		return ctrl.Result{}, nil
 	}
 
@@ -419,30 +413,16 @@ func (r *ClusterCatalogReconciler) needsPoll(lastSuccessfulPoll time.Time, catal
 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) {
-	return f(ctx, obj)
-}
-
-func (r *ClusterCatalogReconciler) setupFinalizers() error {
-	f := crfinalizer.NewFinalizers()
-	err := f.Register(fbcDeletionFinalizer, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
-		catalog, ok := obj.(*ocv1.ClusterCatalog)
-		if !ok {
-			panic("could not convert object to clusterCatalog")
-		}
-		err := r.deleteCatalogCache(ctx, catalog)
-		return crfinalizer.Result{StatusUpdated: true}, err
-	}))
-	if err != nil {
-		return err
+	a.ManagedFields, b.ManagedFields = nil, nil
+	a.ResourceVersion, b.ResourceVersion = "", ""
+	// Remove kubectl's last-applied-configuration annotation which may be added by the API server
+	if a.Annotations != nil {
+		delete(a.Annotations, "kubectl.kubernetes.io/last-applied-configuration")
 	}
-	r.finalizers = f
-	return nil
+	if b.Annotations != nil {
+		delete(b.Annotations, "kubectl.kubernetes.io/last-applied-configuration")
+	}
+	return !equality.Semantic.DeepEqual(a, b)
 }
 
 func (r *ClusterCatalogReconciler) deleteStoredCatalog(catalogName string) {

internal/catalogd/controllers/core/clustercatalog_controller_test.go

@@ -17,8 +17,10 @@ import (
 	"go.podman.io/image/v5/docker/reference"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
@@ -304,6 +306,7 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 			name: "storage finalizer not set, storage finalizer gets set",
 			puller: &imageutil.MockPuller{
 				ImageFS: &fstest.MapFS{},
+				Ref:     mustRef(t, "my.org/someimage@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"),
 			},
 			store: &MockStore{},
 			catalog: &ocv1.ClusterCatalog{
@@ -332,6 +335,8 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 						},
 					},
 				},
+				// Status remains empty because controller returns early after adding finalizer
+				Status: ocv1.ClusterCatalogStatus{},
 			},
 		},
 		{
@@ -802,8 +807,12 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
+			scheme := runtime.NewScheme()
+			require.NoError(t, ocv1.AddToScheme(scheme))
+			cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.catalog).WithStatusSubresource(tt.catalog).Build()
+
 			reconciler := &ClusterCatalogReconciler{
-				Client:         nil,
+				Client:         cl,
 				ImagePuller:    tt.puller,
 				ImageCache:     tt.cache,
 				Storage:        tt.store,
@@ -812,7 +821,6 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 			if reconciler.ImageCache == nil {
 				reconciler.ImageCache = &imageutil.MockCache{}
 			}
-			require.NoError(t, reconciler.setupFinalizers())
 			ctx := context.Background()
 
 			res, err := reconciler.reconcile(ctx, tt.catalog)
@@ -826,6 +834,7 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 			}
 			diff := cmp.Diff(tt.expectedCatalog, tt.catalog,
 				cmpopts.IgnoreFields(metav1.Condition{}, "Message", "LastTransitionTime"),
+				cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion"),
 				cmpopts.SortSlices(func(a, b metav1.Condition) bool { return a.Type < b.Type }))
 			assert.Empty(t, diff, "comparing the expected Catalog")
 		})
@@ -909,8 +918,12 @@ func TestPollingRequeue(t *testing.T) {
 				URLs:         &ocv1.ClusterCatalogURLs{Base: "URL"},
 				LastUnpacked: ptr.To(metav1.NewTime(time.Now().Truncate(time.Second))),
 			}
+			scheme := runtime.NewScheme()
+			require.NoError(t, ocv1.AddToScheme(scheme))
+			cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.catalog).WithStatusSubresource(tc.catalog).Build()
+
 			reconciler := &ClusterCatalogReconciler{
-				Client: nil,
+				Client: cl,
 				ImagePuller: &imageutil.MockPuller{
 					ImageFS: &fstest.MapFS{},
 					Ref:     ref,
@@ -924,7 +937,6 @@ func TestPollingRequeue(t *testing.T) {
 					},
 				},
 			}
-			require.NoError(t, reconciler.setupFinalizers())
 			res, _ := reconciler.reconcile(context.Background(), tc.catalog)
 			assert.InDelta(t, tc.expectedRequeueAfter, res.RequeueAfter, 2*requeueJitterMaxFactor*float64(tc.expectedRequeueAfter))
 		})
@@ -1136,13 +1148,16 @@ func TestPollingReconcilerUnpack(t *testing.T) {
 			if scd == nil {
 				scd = map[string]storedCatalogData{}
 			}
+			scheme := runtime.NewScheme()
+			require.NoError(t, ocv1.AddToScheme(scheme))
+			cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.catalog).WithStatusSubresource(tc.catalog).Build()
+
 			reconciler := &ClusterCatalogReconciler{
-				Client:         nil,
+				Client:         cl,
 				ImagePuller:    &imageutil.MockPuller{Error: errors.New("mockpuller error")},
 				Storage:        &MockStore{},
 				storedCatalogs: scd,
 			}
-			require.NoError(t, reconciler.setupFinalizers())
 			_, err := reconciler.reconcile(context.Background(), tc.catalog)
 			if tc.expectedUnpackRun {
 				assert.Error(t, err)