Use Patch instead of Update for finalizer operations

Refactor all controllers to use client.Patch() instead of Update()
when adding or removing finalizers to reduce conflicts and improve
performance. 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() and RemoveFinalizer() 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"
@@ -391,12 +389,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()
@@ -443,11 +440,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 {
@@ -464,9 +461,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")
@@ -531,7 +528,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())
@@ -560,13 +556,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
@@ -638,7 +630,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())
@@ -677,14 +668,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
 	}
 
@@ -422,29 +416,6 @@ func checkForUnexpectedFieldChange(a, b ocv1.ClusterCatalog) bool {
 	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
-	}
-	r.finalizers = f
-	return nil
-}
-
 func (r *ClusterCatalogReconciler) deleteStoredCatalog(catalogName string) {
 	r.storedCatalogsMu.Lock()
 	defer r.storedCatalogsMu.Unlock()

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{},
 			},
 		},
 		{
@@ -379,7 +384,7 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 			expectedCatalog: &ocv1.ClusterCatalog{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:              "catalog",
-					Finalizers:        []string{},
+					Finalizers:        nil,
 					DeletionTimestamp: &metav1.Time{Time: time.Date(2023, time.October, 10, 4, 19, 0, 0, time.UTC)},
 				},
 				Spec: ocv1.ClusterCatalogSpec{
@@ -660,7 +665,7 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 			expectedCatalog: &ocv1.ClusterCatalog{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:       "catalog",
-					Finalizers: []string{},
+					Finalizers: nil,
 				},
 				Spec: ocv1.ClusterCatalogSpec{
 					Source: ocv1.CatalogSource{
@@ -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)