From a530331d35fc5c179a4d8483c8bae963a72c8cd5 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Wed, 12 Nov 2025 10:15:50 -0500 Subject: [PATCH 1/7] Use SSA instead of Update for finalizer operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactor all controllers to use server-side-apply 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() 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 Signed-off-by: Todd Short --- cmd/operator-controller/main.go | 48 +++--- .../core/clustercatalog_controller.go | 104 ++++++------- .../core/clustercatalog_controller_test.go | 27 +++- .../clusterextension_controller.go | 86 +++++++---- .../clusterextension_controller_test.go | 12 +- .../clusterextensionrevision_controller.go | 61 ++------ ...lusterextensionrevision_controller_test.go | 8 +- .../controllers/suite_test.go | 3 +- .../finalizers/finalizers.go | 14 -- internal/shared/util/finalizer/finalizer.go | 137 ++++++++++++++++++ 10 files changed, 292 insertions(+), 208 deletions(-) delete mode 100644 internal/operator-controller/finalizers/finalizers.go create mode 100644 internal/shared/util/finalizer/finalizer.go diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 20d55bc3cf..cd1c999f1a 100644 --- a/cmd/operator-controller/main.go +++ b/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 diff --git a/internal/catalogd/controllers/core/clustercatalog_controller.go b/internal/catalogd/controllers/core/clustercatalog_controller.go index e968db7b97..1481914dd3 100644 --- a/internal/catalogd/controllers/core/clustercatalog_controller.go +++ b/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) { diff --git a/internal/catalogd/controllers/core/clustercatalog_controller_test.go b/internal/catalogd/controllers/core/clustercatalog_controller_test.go index 76efafa228..6464c47e80 100644 --- a/internal/catalogd/controllers/core/clustercatalog_controller_test.go +++ b/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) diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index 7bcedde656..04751f012c 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -37,8 +37,8 @@ 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" - crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer" crhandler "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -54,6 +54,7 @@ import ( "github.com/operator-framework/operator-controller/internal/operator-controller/conditionsets" "github.com/operator-framework/operator-controller/internal/operator-controller/labels" "github.com/operator-framework/operator-controller/internal/operator-controller/resolve" + finalizerutil "github.com/operator-framework/operator-controller/internal/shared/util/finalizer" imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image" ) @@ -73,9 +74,12 @@ type ClusterExtensionReconciler struct { StorageMigrator StorageMigrator Applier Applier RevisionStatesGetter RevisionStatesGetter - Finalizers crfinalizer.Finalizers + FinalizerHandlers map[string]FinalizerHandler } +// FinalizerHandler defines a function that handles cleanup for a specific finalizer +type FinalizerHandler func(context.Context, *ocv1.ClusterExtension) error + type StorageMigrator interface { Migrate(context.Context, *ocv1.ClusterExtension, map[string]string) error } @@ -110,7 +114,6 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req // Do checks before any Update()s, as Update() may modify the resource structure! updateStatus := !equality.Semantic.DeepEqual(existingExt.Status, reconciledExt.Status) - updateFinalizers := !equality.Semantic.DeepEqual(existingExt.Finalizers, reconciledExt.Finalizers) // If any unexpected fields have changed, panic before updating the resource unexpectedFieldsChanged := checkForUnexpectedClusterExtensionFieldChange(*existingExt, *reconciledExt) @@ -118,23 +121,11 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req 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. - 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 { - reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating finalizers: %v", err)) - } - } return res, reconcileErr } @@ -157,10 +148,19 @@ func ensureAllConditionsWithReason(ext *ocv1.ClusterExtension, reason v1alpha1.C } } -// Compare resources - ignoring status & metadata.finalizers +// Compare resources - ignoring status, metadata.finalizers, metadata.resourceVersion, and metadata.managedFields func checkForUnexpectedClusterExtensionFieldChange(a, b ocv1.ClusterExtension) bool { a.Status, b.Status = ocv1.ClusterExtensionStatus{}, ocv1.ClusterExtensionStatus{} a.Finalizers, b.Finalizers = []string{}, []string{} + a.ResourceVersion, b.ResourceVersion = "", "" + a.ManagedFields, b.ManagedFields = nil, nil + // 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") + } + if b.Annotations != nil { + delete(b.Annotations, "kubectl.kubernetes.io/last-applied-configuration") + } return !equality.Semantic.DeepEqual(a, b) } @@ -179,28 +179,52 @@ func checkForUnexpectedClusterExtensionFieldChange(a, b ocv1.ClusterExtension) b 4.2 Generating a chart from k8s objects. 4.3 Store the release on cluster. */ + +func (r *ClusterExtensionReconciler) handleDeletion(ctx context.Context, ext *ocv1.ClusterExtension) (ctrl.Result, error) { + // Run cleanup for each registered finalizer and collect finalizers to remove + finalizersToRemove := make([]string, 0, len(r.FinalizerHandlers)) + for finalizerKey, handler := range r.FinalizerHandlers { + if !controllerutil.ContainsFinalizer(ext, finalizerKey) { + continue + } + if err := handler(ctx, ext); err != nil { + setStatusProgressing(ext, err) + return ctrl.Result{}, err + } + finalizersToRemove = append(finalizersToRemove, finalizerKey) + } + // Remove all finalizers in a single patch operation + if len(finalizersToRemove) > 0 { + if err := finalizerutil.RemoveFinalizer(ctx, r.Client, ext, finalizersToRemove...); err != nil { + setStatusProgressing(ext, err) + return ctrl.Result{}, fmt.Errorf("error removing finalizers: %v", err) + } + } + // All finalizers removed, nothing more to do + return ctrl.Result{}, nil +} + //nolint:unparam func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.ClusterExtension) (ctrl.Result, error) { l := log.FromContext(ctx) l.Info("handling finalizers") - finalizeResult, err := r.Finalizers.Finalize(ctx, ext) - if err != nil { - setStatusProgressing(ext, err) - 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 - return ctrl.Result{}, nil - } + // Handle deletion if ext.GetDeletionTimestamp() != nil { - // If we've gotten here, that means the cluster extension is being deleted, we've handled all of - // _our_ finalizers (above), but the cluster extension is still present in the cluster, likely - // because there are _other_ finalizers that other controllers need to handle, (e.g. the orphan - // deletion finalizer). - return ctrl.Result{}, nil + return r.handleDeletion(ctx, ext) + } + + // Ensure all finalizers are present + finalizers := make([]string, 0, len(r.FinalizerHandlers)) + for finalizerKey := range r.FinalizerHandlers { + finalizers = append(finalizers, finalizerKey) + } + if len(finalizers) > 0 { + if _, err := finalizerutil.EnsureFinalizer(ctx, r.Client, ext, finalizers...); err != nil { + setStatusProgressing(ext, err) + return ctrl.Result{}, fmt.Errorf("error ensuring finalizers: %v", err) + } } objLbls := map[string]string{ diff --git a/internal/operator-controller/controllers/clusterextension_controller_test.go b/internal/operator-controller/controllers/clusterextension_controller_test.go index 437f62dcec..36c91748ff 100644 --- a/internal/operator-controller/controllers/clusterextension_controller_test.go +++ b/internal/operator-controller/controllers/clusterextension_controller_test.go @@ -21,7 +21,6 @@ import ( "k8s.io/apimachinery/pkg/util/rand" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer" "sigs.k8s.io/controller-runtime/pkg/reconcile" helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client" @@ -31,7 +30,6 @@ import ( "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" "github.com/operator-framework/operator-controller/internal/operator-controller/conditionsets" "github.com/operator-framework/operator-controller/internal/operator-controller/controllers" - "github.com/operator-framework/operator-controller/internal/operator-controller/finalizers" "github.com/operator-framework/operator-controller/internal/operator-controller/labels" "github.com/operator-framework/operator-controller/internal/operator-controller/resolve" imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image" @@ -782,11 +780,11 @@ func TestClusterExtensionDeleteFinalizerFails(t *testing.T) { }, }, } - err = reconciler.Finalizers.Register(fakeFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { - return crfinalizer.Result{}, errors.New(finalizersMessage) - })) - - require.NoError(t, err) + reconciler.FinalizerHandlers = map[string]controllers.FinalizerHandler{ + fakeFinalizer: func(ctx context.Context, ext *ocv1.ClusterExtension) error { + return errors.New(finalizersMessage) + }, + } // Reconcile twice to simulate installing the ClusterExtension and loading in the finalizers res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller.go b/internal/operator-controller/controllers/clusterextensionrevision_controller.go index 7e98faa654..c4685c6a4c 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller.go @@ -4,7 +4,6 @@ package controllers import ( "context" - "encoding/json" "errors" "fmt" "strings" @@ -27,13 +26,13 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/source" ocv1 "github.com/operator-framework/operator-controller/api/v1" + finalizerutil "github.com/operator-framework/operator-controller/internal/shared/util/finalizer" ) const ( @@ -110,6 +109,13 @@ func checkForUnexpectedClusterExtensionRevisionFieldChange(a, b ocv1.ClusterExte a.Finalizers, b.Finalizers = []string{}, []string{} 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") + } + if b.Annotations != nil { + delete(b.Annotations, "kubectl.kubernetes.io/last-applied-configuration") + } return !equality.Semantic.DeepEqual(a.Spec, b.Spec) } @@ -125,7 +131,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev // // Reconcile // - if err := c.ensureFinalizer(ctx, rev, clusterExtensionRevisionTeardownFinalizer); err != nil { + if _, err := finalizerutil.EnsureFinalizer(ctx, c.Client, rev, clusterExtensionRevisionTeardownFinalizer); err != nil { meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{ Type: ocv1.ClusterExtensionRevisionTypeAvailable, Status: metav1.ConditionFalse, @@ -354,7 +360,7 @@ func (c *ClusterExtensionRevisionReconciler) teardown(ctx context.Context, rev * return ctrl.Result{}, nil } - if err := c.removeFinalizer(ctx, rev, clusterExtensionRevisionTeardownFinalizer); err != nil { + if err := finalizerutil.RemoveFinalizer(ctx, c.Client, rev, clusterExtensionRevisionTeardownFinalizer); err != nil { return ctrl.Result{}, fmt.Errorf("error removing teardown finalizer: %v", err) } return ctrl.Result{}, nil @@ -393,53 +399,6 @@ func (c *ClusterExtensionRevisionReconciler) establishWatch( return c.TrackingCache.Watch(ctx, rev, gvks) } -func (c *ClusterExtensionRevisionReconciler) ensureFinalizer( - ctx context.Context, obj client.Object, finalizer string, -) error { - if controllerutil.ContainsFinalizer(obj, finalizer) { - return nil - } - - controllerutil.AddFinalizer(obj, finalizer) - patch := map[string]any{ - "metadata": map[string]any{ - "resourceVersion": obj.GetResourceVersion(), - "finalizers": obj.GetFinalizers(), - }, - } - patchJSON, err := json.Marshal(patch) - if err != nil { - return fmt.Errorf("marshalling patch to remove finalizer: %w", err) - } - if err := c.Client.Patch(ctx, obj, client.RawPatch(types.MergePatchType, patchJSON)); err != nil { - return fmt.Errorf("adding finalizer: %w", err) - } - return nil -} - -func (c *ClusterExtensionRevisionReconciler) removeFinalizer(ctx context.Context, obj client.Object, finalizer string) error { - if !controllerutil.ContainsFinalizer(obj, finalizer) { - return nil - } - - controllerutil.RemoveFinalizer(obj, finalizer) - - patch := map[string]any{ - "metadata": map[string]any{ - "resourceVersion": obj.GetResourceVersion(), - "finalizers": obj.GetFinalizers(), - }, - } - patchJSON, err := json.Marshal(patch) - if err != nil { - return fmt.Errorf("marshalling patch to remove finalizer: %w", err) - } - if err := c.Client.Patch(ctx, obj, client.RawPatch(types.MergePatchType, patchJSON)); err != nil { - return fmt.Errorf("removing finalizer: %w", err) - } - return nil -} - func toBoxcutterRevision(rev *ocv1.ClusterExtensionRevision) (*boxcutter.Revision, []boxcutter.RevisionReconcileOption, []client.Object) { previous := make([]client.Object, 0, len(rev.Spec.Previous)) for _, specPrevious := range rev.Spec.Previous { diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go b/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go index 873a6cc748..d4295a2da0 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go @@ -8,7 +8,6 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -506,13 +505,14 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) { } }, validate: func(t *testing.T, c client.Client) { - t.Log("cluster revision is deleted") + t.Log("finalizer is removed from cluster revision") rev := &ocv1.ClusterExtensionRevision{} err := c.Get(t.Context(), client.ObjectKey{ Name: clusterExtensionRevisionName, }, rev) - require.Error(t, err) - require.True(t, errors.IsNotFound(err)) + require.NoError(t, err) + require.NotContains(t, rev.Finalizers, "olm.operatorframework.io/teardown") + require.NotNil(t, rev.DeletionTimestamp) }, }, { diff --git a/internal/operator-controller/controllers/suite_test.go b/internal/operator-controller/controllers/suite_test.go index 02d5382371..e8e82425ce 100644 --- a/internal/operator-controller/controllers/suite_test.go +++ b/internal/operator-controller/controllers/suite_test.go @@ -28,7 +28,6 @@ import ( utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" - crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer" ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/controllers" @@ -84,7 +83,7 @@ func newClientAndReconciler(t *testing.T) (client.Client, *controllers.ClusterEx RevisionStatesGetter: &MockRevisionStatesGetter{ RevisionStates: &controllers.RevisionStates{}, }, - Finalizers: crfinalizer.NewFinalizers(), + FinalizerHandlers: map[string]controllers.FinalizerHandler{}, } return cl, reconciler } diff --git a/internal/operator-controller/finalizers/finalizers.go b/internal/operator-controller/finalizers/finalizers.go deleted file mode 100644 index b04635a9e7..0000000000 --- a/internal/operator-controller/finalizers/finalizers.go +++ /dev/null @@ -1,14 +0,0 @@ -package finalizers - -import ( - "context" - - "sigs.k8s.io/controller-runtime/pkg/client" - crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer" -) - -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) -} diff --git a/internal/shared/util/finalizer/finalizer.go b/internal/shared/util/finalizer/finalizer.go new file mode 100644 index 0000000000..2a95dfb71b --- /dev/null +++ b/internal/shared/util/finalizer/finalizer.go @@ -0,0 +1,137 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package finalizer + +import ( + "context" + "fmt" + "slices" + "sort" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/util/sets" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +// EnsureFinalizer adds one or more finalizers to the object using server-side apply. +// If all finalizers already exist, this is a no-op and returns (false, nil). +// Returns (true, nil) if any finalizers were added. +// Note: This function will update the passed object with the server response. +func EnsureFinalizer(ctx context.Context, c client.Client, obj client.Object, finalizers ...string) (bool, error) { + if len(finalizers) == 0 { + return false, nil + } + + // Determine which finalizers need to be added + var toAdd []string + for _, finalizer := range finalizers { + if !controllerutil.ContainsFinalizer(obj, finalizer) { + toAdd = append(toAdd, finalizer) + } + } + + if len(toAdd) == 0 { + return false, nil + } + + // Sort the finalizers to add for consistent ordering + sort.Strings(toAdd) + + // Create a copy of the finalizers list to avoid mutating the object + currentFinalizers := obj.GetFinalizers() + newFinalizers := make([]string, len(currentFinalizers), len(currentFinalizers)+len(toAdd)) + copy(newFinalizers, currentFinalizers) + newFinalizers = append(newFinalizers, toAdd...) + + // Get the GVK for this object type + gvk, err := apiutil.GVKForObject(obj, c.Scheme()) + if err != nil { + return false, fmt.Errorf("getting object kind: %w", err) + } + + // Create an unstructured object for server-side apply + u := &unstructured.Unstructured{} + u.SetGroupVersionKind(gvk) + u.SetName(obj.GetName()) + u.SetNamespace(obj.GetNamespace()) + u.SetFinalizers(newFinalizers) + + // Use server-side apply to update finalizers + if err := c.Patch(ctx, u, client.Apply, client.ForceOwnership, client.FieldOwner("finalizer-controller")); err != nil { + return false, fmt.Errorf("adding finalizer: %w", err) + } + + // Update the passed object with the new finalizers + obj.SetFinalizers(newFinalizers) + obj.SetResourceVersion(u.GetResourceVersion()) + + return true, nil +} + +// RemoveFinalizer removes one or more finalizers from the object using server-side apply. +// If none of the finalizers exist, this is a no-op. +func RemoveFinalizer(ctx context.Context, c client.Client, obj client.Object, finalizers ...string) error { + if len(finalizers) == 0 { + return nil + } + + // Create a set of finalizers to remove for efficient lookup + toRemove := sets.New(finalizers...) + hasAny := false + for _, finalizer := range finalizers { + if controllerutil.ContainsFinalizer(obj, finalizer) { + hasAny = true + } + } + + if !hasAny { + return nil + } + + // Create a copy of the finalizers list and remove the specified finalizers + currentFinalizers := obj.GetFinalizers() + newFinalizers := slices.Clone(currentFinalizers) + newFinalizers = slices.DeleteFunc(newFinalizers, func(f string) bool { + return toRemove.Has(f) + }) + + // Get the GVK for this object type + gvk, err := apiutil.GVKForObject(obj, c.Scheme()) + if err != nil { + return fmt.Errorf("getting object kind: %w", err) + } + + // Create an unstructured object for server-side apply + u := &unstructured.Unstructured{} + u.SetGroupVersionKind(gvk) + u.SetName(obj.GetName()) + u.SetNamespace(obj.GetNamespace()) + u.SetFinalizers(newFinalizers) + + // Use server-side apply to update finalizers + if err := c.Patch(ctx, u, client.Apply, client.ForceOwnership, client.FieldOwner("finalizer-controller")); err != nil { + return fmt.Errorf("removing finalizer: %w", err) + } + + // Update the passed object with the new finalizers + obj.SetFinalizers(newFinalizers) + obj.SetResourceVersion(u.GetResourceVersion()) + + return nil +} From a404f0148aaba7546e32a81bb0d9c5baa62bd121 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Fri, 14 Nov 2025 11:24:34 -0500 Subject: [PATCH 2/7] fixup! Use SSA instead of Update for finalizer operations Signed-off-by: Todd Short --- .../core/clustercatalog_controller.go | 30 +++++++++---------- .../clusterextension_controller.go | 7 +++-- .../clusterextensionrevision_controller.go | 5 ++-- internal/shared/util/finalizer/finalizer.go | 12 ++++---- 4 files changed, 27 insertions(+), 27 deletions(-) diff --git a/internal/catalogd/controllers/core/clustercatalog_controller.go b/internal/catalogd/controllers/core/clustercatalog_controller.go index 1481914dd3..956fce6711 100644 --- a/internal/catalogd/controllers/core/clustercatalog_controller.go +++ b/internal/catalogd/controllers/core/clustercatalog_controller.go @@ -44,10 +44,12 @@ import ( ) const ( - fbcDeletionFinalizer = "olm.operatorframework.io/delete-server-cache" + ClusterCatalogFinalizerOwner = "olm.operatorframework.io/clustercatalog-controller" + fbcDeletionFinalizer = "olm.operatorframework.io/delete-server-cache" // CatalogSources are polled if PollInterval is mentioned, in intervals of wait.Jitter(pollDuration, maxFactor) // wait.Jitter returns a time.Duration between pollDuration and pollDuration + maxFactor * pollDuration. - requeueJitterMaxFactor = 0.01 + requeueJitterMaxFactor = 0.01 + lastAppliedConfiguration = "kubectl.kubernetes.io/last-applied-configuration" ) // ClusterCatalogReconciler reconciles a Catalog object @@ -150,20 +152,16 @@ 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. - if err := finalizerutil.RemoveFinalizer(ctx, r.Client, catalog, fbcDeletionFinalizer); err != nil { + if err := finalizerutil.RemoveFinalizers(ctx, ClusterCatalogFinalizerOwner, 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 } @@ -176,7 +174,7 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *ocv1. 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 { + if err := finalizerutil.RemoveFinalizers(ctx, ClusterCatalogFinalizerOwner, 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 @@ -184,8 +182,8 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *ocv1. return ctrl.Result{}, nil } - // Ensure finalizer is present - finalizerAdded, err := finalizerutil.EnsureFinalizer(ctx, r.Client, catalog, fbcDeletionFinalizer) + // Add finalizer + finalizerAdded, err := finalizerutil.AddFinalizers(ctx, ClusterCatalogFinalizerOwner, r.Client, catalog, fbcDeletionFinalizer) if err != nil { return ctrl.Result{}, fmt.Errorf("error ensuring finalizer: %v", err) } @@ -417,10 +415,10 @@ func checkForUnexpectedFieldChange(a, b ocv1.ClusterCatalog) bool { 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") + delete(a.Annotations, lastAppliedConfiguration) } if b.Annotations != nil { - delete(b.Annotations, "kubectl.kubernetes.io/last-applied-configuration") + delete(b.Annotations, lastAppliedConfiguration) } return !equality.Semantic.DeepEqual(a, b) } diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index 04751f012c..51cc63392e 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -61,6 +61,7 @@ import ( const ( ClusterExtensionCleanupUnpackCacheFinalizer = "olm.operatorframework.io/cleanup-unpack-cache" ClusterExtensionCleanupContentManagerCacheFinalizer = "olm.operatorframework.io/cleanup-contentmanager-cache" + ClusterExtensionFinalizerOwner = "olm.operatorframework.io/clusterextension-controller" ) // ClusterExtensionReconciler reconciles a ClusterExtension object @@ -195,7 +196,7 @@ func (r *ClusterExtensionReconciler) handleDeletion(ctx context.Context, ext *oc } // Remove all finalizers in a single patch operation if len(finalizersToRemove) > 0 { - if err := finalizerutil.RemoveFinalizer(ctx, r.Client, ext, finalizersToRemove...); err != nil { + if err := finalizerutil.RemoveFinalizers(ctx, ClusterExtensionFinalizerOwner, r.Client, ext, finalizersToRemove...); err != nil { setStatusProgressing(ext, err) return ctrl.Result{}, fmt.Errorf("error removing finalizers: %v", err) } @@ -215,13 +216,13 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl return r.handleDeletion(ctx, ext) } - // Ensure all finalizers are present + // Add all finalizers finalizers := make([]string, 0, len(r.FinalizerHandlers)) for finalizerKey := range r.FinalizerHandlers { finalizers = append(finalizers, finalizerKey) } if len(finalizers) > 0 { - if _, err := finalizerutil.EnsureFinalizer(ctx, r.Client, ext, finalizers...); err != nil { + if _, err := finalizerutil.AddFinalizers(ctx, ClusterExtensionFinalizerOwner, r.Client, ext, finalizers...); err != nil { setStatusProgressing(ext, err) return ctrl.Result{}, fmt.Errorf("error ensuring finalizers: %v", err) } diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller.go b/internal/operator-controller/controllers/clusterextensionrevision_controller.go index c4685c6a4c..789c65b313 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller.go @@ -38,6 +38,7 @@ import ( const ( ClusterExtensionRevisionOwnerLabel = "olm.operatorframework.io/owner" clusterExtensionRevisionTeardownFinalizer = "olm.operatorframework.io/teardown" + CluserExtensionRevisionFinalizerOwner = "olm.operatorframework.io/clusterextensionrevision-controller" ) // ClusterExtensionRevisionReconciler actions individual snapshots of ClusterExtensions, @@ -131,7 +132,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev // // Reconcile // - if _, err := finalizerutil.EnsureFinalizer(ctx, c.Client, rev, clusterExtensionRevisionTeardownFinalizer); err != nil { + if _, err := finalizerutil.AddFinalizers(ctx, CluserExtensionRevisionFinalizerOwner, c.Client, rev, clusterExtensionRevisionTeardownFinalizer); err != nil { meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{ Type: ocv1.ClusterExtensionRevisionTypeAvailable, Status: metav1.ConditionFalse, @@ -360,7 +361,7 @@ func (c *ClusterExtensionRevisionReconciler) teardown(ctx context.Context, rev * return ctrl.Result{}, nil } - if err := finalizerutil.RemoveFinalizer(ctx, c.Client, rev, clusterExtensionRevisionTeardownFinalizer); err != nil { + if err := finalizerutil.RemoveFinalizers(ctx, CluserExtensionRevisionFinalizerOwner, c.Client, rev, clusterExtensionRevisionTeardownFinalizer); err != nil { return ctrl.Result{}, fmt.Errorf("error removing teardown finalizer: %v", err) } return ctrl.Result{}, nil diff --git a/internal/shared/util/finalizer/finalizer.go b/internal/shared/util/finalizer/finalizer.go index 2a95dfb71b..4151d6784c 100644 --- a/internal/shared/util/finalizer/finalizer.go +++ b/internal/shared/util/finalizer/finalizer.go @@ -29,11 +29,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) -// EnsureFinalizer adds one or more finalizers to the object using server-side apply. +// AddFinalizers adds one or more finalizers to the object using server-side apply. // If all finalizers already exist, this is a no-op and returns (false, nil). // Returns (true, nil) if any finalizers were added. // Note: This function will update the passed object with the server response. -func EnsureFinalizer(ctx context.Context, c client.Client, obj client.Object, finalizers ...string) (bool, error) { +func AddFinalizers(ctx context.Context, owner string, c client.Client, obj client.Object, finalizers ...string) (bool, error) { if len(finalizers) == 0 { return false, nil } @@ -73,7 +73,7 @@ func EnsureFinalizer(ctx context.Context, c client.Client, obj client.Object, fi u.SetFinalizers(newFinalizers) // Use server-side apply to update finalizers - if err := c.Patch(ctx, u, client.Apply, client.ForceOwnership, client.FieldOwner("finalizer-controller")); err != nil { + if err := c.Patch(ctx, u, client.Apply, client.ForceOwnership, client.FieldOwner(owner)); err != nil { return false, fmt.Errorf("adding finalizer: %w", err) } @@ -84,9 +84,9 @@ func EnsureFinalizer(ctx context.Context, c client.Client, obj client.Object, fi return true, nil } -// RemoveFinalizer removes one or more finalizers from the object using server-side apply. +// RemoveFinalizers removes one or more finalizers from the object using server-side apply. // If none of the finalizers exist, this is a no-op. -func RemoveFinalizer(ctx context.Context, c client.Client, obj client.Object, finalizers ...string) error { +func RemoveFinalizers(ctx context.Context, owner string, c client.Client, obj client.Object, finalizers ...string) error { if len(finalizers) == 0 { return nil } @@ -125,7 +125,7 @@ func RemoveFinalizer(ctx context.Context, c client.Client, obj client.Object, fi u.SetFinalizers(newFinalizers) // Use server-side apply to update finalizers - if err := c.Patch(ctx, u, client.Apply, client.ForceOwnership, client.FieldOwner("finalizer-controller")); err != nil { + if err := c.Patch(ctx, u, client.Apply, client.ForceOwnership, client.FieldOwner(owner)); err != nil { return fmt.Errorf("removing finalizer: %w", err) } From 5b91efd69d7273f4effc01be9ffc015c57c8743a Mon Sep 17 00:00:00 2001 From: Todd Short Date: Fri, 14 Nov 2025 13:43:17 -0500 Subject: [PATCH 3/7] fixup! Use SSA instead of Update for finalizer operations Signed-off-by: Todd Short --- .../core/clustercatalog_controller.go | 24 +++++++------------ .../clusterextension_controller.go | 21 +++++++--------- .../clusterextensionrevision_controller.go | 24 +++++++------------ 3 files changed, 25 insertions(+), 44 deletions(-) diff --git a/internal/catalogd/controllers/core/clustercatalog_controller.go b/internal/catalogd/controllers/core/clustercatalog_controller.go index 956fce6711..1e212e0114 100644 --- a/internal/catalogd/controllers/core/clustercatalog_controller.go +++ b/internal/catalogd/controllers/core/clustercatalog_controller.go @@ -48,8 +48,7 @@ const ( fbcDeletionFinalizer = "olm.operatorframework.io/delete-server-cache" // CatalogSources are polled if PollInterval is mentioned, in intervals of wait.Jitter(pollDuration, maxFactor) // wait.Jitter returns a time.Duration between pollDuration and pollDuration + maxFactor * pollDuration. - requeueJitterMaxFactor = 0.01 - lastAppliedConfiguration = "kubectl.kubernetes.io/last-applied-configuration" + requeueJitterMaxFactor = 0.01 ) // ClusterCatalogReconciler reconciles a Catalog object @@ -106,7 +105,7 @@ 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) - unexpectedFieldsChanged := checkForUnexpectedFieldChange(existingCatsrc, *reconciledCatsrc) + unexpectedFieldsChanged := checkForUnexpectedFieldChange(&existingCatsrc, reconciledCatsrc) if unexpectedFieldsChanged { panic("spec or metadata changed by reconciler") @@ -407,20 +406,15 @@ 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{} - 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, lastAppliedConfiguration) +// Compare resources - Annotations/Labels/Spec +func checkForUnexpectedFieldChange(a, b *ocv1.ClusterCatalog) bool { + if !equality.Semantic.DeepEqual(a.ObjectMeta.Annotations, b.ObjectMeta.Annotations) { + return true } - if b.Annotations != nil { - delete(b.Annotations, lastAppliedConfiguration) + if !equality.Semantic.DeepEqual(a.ObjectMeta.Labels, b.ObjectMeta.Labels) { + return true } - return !equality.Semantic.DeepEqual(a, b) + return !equality.Semantic.DeepEqual(a.Spec, b.Spec) } func (r *ClusterCatalogReconciler) deleteStoredCatalog(catalogName string) { diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index 51cc63392e..b2b3c99690 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -117,7 +117,7 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req updateStatus := !equality.Semantic.DeepEqual(existingExt.Status, reconciledExt.Status) // If any unexpected fields have changed, panic before updating the resource - unexpectedFieldsChanged := checkForUnexpectedClusterExtensionFieldChange(*existingExt, *reconciledExt) + unexpectedFieldsChanged := checkForUnexpectedClusterExtensionFieldChange(existingExt, reconciledExt) if unexpectedFieldsChanged { panic("spec or metadata changed by reconciler") } @@ -149,20 +149,15 @@ func ensureAllConditionsWithReason(ext *ocv1.ClusterExtension, reason v1alpha1.C } } -// Compare resources - ignoring status, metadata.finalizers, metadata.resourceVersion, and metadata.managedFields -func checkForUnexpectedClusterExtensionFieldChange(a, b ocv1.ClusterExtension) bool { - a.Status, b.Status = ocv1.ClusterExtensionStatus{}, ocv1.ClusterExtensionStatus{} - a.Finalizers, b.Finalizers = []string{}, []string{} - a.ResourceVersion, b.ResourceVersion = "", "" - a.ManagedFields, b.ManagedFields = nil, nil - // 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") +// Compare resources - Annotations/Labels/Spec +func checkForUnexpectedClusterExtensionFieldChange(a, b *ocv1.ClusterExtension) bool { + if !equality.Semantic.DeepEqual(a.ObjectMeta.Annotations, b.ObjectMeta.Annotations) { + return true } - if b.Annotations != nil { - delete(b.Annotations, "kubectl.kubernetes.io/last-applied-configuration") + if !equality.Semantic.DeepEqual(a.ObjectMeta.Labels, b.ObjectMeta.Labels) { + return true } - return !equality.Semantic.DeepEqual(a, b) + return !equality.Semantic.DeepEqual(a.Spec, b.Spec) } // Helper function to do the actual reconcile diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller.go b/internal/operator-controller/controllers/clusterextensionrevision_controller.go index 789c65b313..dfc3fa60e4 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller.go @@ -83,7 +83,7 @@ func (c *ClusterExtensionRevisionReconciler) Reconcile(ctx context.Context, req // Do checks before any Update()s, as Update() may modify the resource structure! updateStatus := !equality.Semantic.DeepEqual(existingRev.Status, reconciledRev.Status) - unexpectedFieldsChanged := checkForUnexpectedClusterExtensionRevisionFieldChange(*existingRev, *reconciledRev) + unexpectedFieldsChanged := checkForUnexpectedClusterExtensionRevisionFieldChange(existingRev, reconciledRev) if unexpectedFieldsChanged { panic("spec or metadata changed by reconciler") } @@ -101,21 +101,13 @@ func (c *ClusterExtensionRevisionReconciler) Reconcile(ctx context.Context, req return res, reconcileErr } -// Compare resources - ignoring status & metadata.finalizers -func checkForUnexpectedClusterExtensionRevisionFieldChange(a, b ocv1.ClusterExtensionRevision) bool { - a.Status, b.Status = ocv1.ClusterExtensionRevisionStatus{}, ocv1.ClusterExtensionRevisionStatus{} - - // when finalizers are updated during reconcile, we expect finalizers, managedFields, and resourceVersion - // to be updated, so we ignore changes in these fields. - a.Finalizers, b.Finalizers = []string{}, []string{} - 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") - } - if b.Annotations != nil { - delete(b.Annotations, "kubectl.kubernetes.io/last-applied-configuration") +// Compare resources - Annotations/Labels/Spec +func checkForUnexpectedClusterExtensionRevisionFieldChange(a, b *ocv1.ClusterExtensionRevision) bool { + if !equality.Semantic.DeepEqual(a.ObjectMeta.Annotations, b.ObjectMeta.Annotations) { + return true + } + if !equality.Semantic.DeepEqual(a.ObjectMeta.Labels, b.ObjectMeta.Labels) { + return true } return !equality.Semantic.DeepEqual(a.Spec, b.Spec) } From 1f64a3ed74bfafbafe2487be087b6abcfdf15e20 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Fri, 14 Nov 2025 14:21:44 -0500 Subject: [PATCH 4/7] fixup! Use SSA instead of Update for finalizer operations Signed-off-by: Todd Short --- .../controllers/core/clustercatalog_controller_test.go | 4 ++-- internal/shared/util/finalizer/finalizer.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/catalogd/controllers/core/clustercatalog_controller_test.go b/internal/catalogd/controllers/core/clustercatalog_controller_test.go index 6464c47e80..c618b32c60 100644 --- a/internal/catalogd/controllers/core/clustercatalog_controller_test.go +++ b/internal/catalogd/controllers/core/clustercatalog_controller_test.go @@ -384,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{ @@ -665,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{ diff --git a/internal/shared/util/finalizer/finalizer.go b/internal/shared/util/finalizer/finalizer.go index 4151d6784c..18039da0dd 100644 --- a/internal/shared/util/finalizer/finalizer.go +++ b/internal/shared/util/finalizer/finalizer.go @@ -78,7 +78,7 @@ func AddFinalizers(ctx context.Context, owner string, c client.Client, obj clien } // Update the passed object with the new finalizers - obj.SetFinalizers(newFinalizers) + obj.SetFinalizers(u.GetFinalizers()) obj.SetResourceVersion(u.GetResourceVersion()) return true, nil @@ -130,7 +130,7 @@ func RemoveFinalizers(ctx context.Context, owner string, c client.Client, obj cl } // Update the passed object with the new finalizers - obj.SetFinalizers(newFinalizers) + obj.SetFinalizers(u.GetFinalizers()) obj.SetResourceVersion(u.GetResourceVersion()) return nil From 0495a08ce4cbe81852d0c95096c508ef345b24a6 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Fri, 14 Nov 2025 17:03:48 -0500 Subject: [PATCH 5/7] fixup! Use SSA instead of Update for finalizer operations Signed-off-by: Todd Short --- .../catalogd/controllers/core/clustercatalog_controller.go | 4 ++-- .../controllers/clusterextension_controller.go | 4 ++-- .../controllers/clusterextensionrevision_controller.go | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/catalogd/controllers/core/clustercatalog_controller.go b/internal/catalogd/controllers/core/clustercatalog_controller.go index 1e212e0114..c1ee2fc82b 100644 --- a/internal/catalogd/controllers/core/clustercatalog_controller.go +++ b/internal/catalogd/controllers/core/clustercatalog_controller.go @@ -408,10 +408,10 @@ func (r *ClusterCatalogReconciler) needsPoll(lastSuccessfulPoll time.Time, catal // Compare resources - Annotations/Labels/Spec func checkForUnexpectedFieldChange(a, b *ocv1.ClusterCatalog) bool { - if !equality.Semantic.DeepEqual(a.ObjectMeta.Annotations, b.ObjectMeta.Annotations) { + if !equality.Semantic.DeepEqual(a.Annotations, b.Annotations) { return true } - if !equality.Semantic.DeepEqual(a.ObjectMeta.Labels, b.ObjectMeta.Labels) { + if !equality.Semantic.DeepEqual(a.Labels, b.Labels) { return true } return !equality.Semantic.DeepEqual(a.Spec, b.Spec) diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index b2b3c99690..21df0f332e 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -151,10 +151,10 @@ func ensureAllConditionsWithReason(ext *ocv1.ClusterExtension, reason v1alpha1.C // Compare resources - Annotations/Labels/Spec func checkForUnexpectedClusterExtensionFieldChange(a, b *ocv1.ClusterExtension) bool { - if !equality.Semantic.DeepEqual(a.ObjectMeta.Annotations, b.ObjectMeta.Annotations) { + if !equality.Semantic.DeepEqual(a.Annotations, b.Annotations) { return true } - if !equality.Semantic.DeepEqual(a.ObjectMeta.Labels, b.ObjectMeta.Labels) { + if !equality.Semantic.DeepEqual(a.Labels, b.Labels) { return true } return !equality.Semantic.DeepEqual(a.Spec, b.Spec) diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller.go b/internal/operator-controller/controllers/clusterextensionrevision_controller.go index dfc3fa60e4..14452b7ce4 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller.go @@ -103,10 +103,10 @@ func (c *ClusterExtensionRevisionReconciler) Reconcile(ctx context.Context, req // Compare resources - Annotations/Labels/Spec func checkForUnexpectedClusterExtensionRevisionFieldChange(a, b *ocv1.ClusterExtensionRevision) bool { - if !equality.Semantic.DeepEqual(a.ObjectMeta.Annotations, b.ObjectMeta.Annotations) { + if !equality.Semantic.DeepEqual(a.Annotations, b.Annotations) { return true } - if !equality.Semantic.DeepEqual(a.ObjectMeta.Labels, b.ObjectMeta.Labels) { + if !equality.Semantic.DeepEqual(a.Labels, b.Labels) { return true } return !equality.Semantic.DeepEqual(a.Spec, b.Spec) From dcb26e90fc2cb3725f804653121bad30456f028f Mon Sep 17 00:00:00 2001 From: Todd Short Date: Sun, 16 Nov 2025 09:45:03 -0500 Subject: [PATCH 6/7] fixup! Use SSA instead of Update for finalizer operations Signed-off-by: Todd Short --- .../core/clustercatalog_controller.go | 10 +- .../clusterextension_controller.go | 16 +-- .../clusterextension_controller_test.go | 3 +- .../clusterextensionrevision_controller.go | 8 +- internal/shared/util/finalizer/finalizer.go | 112 ++++++------------ 5 files changed, 54 insertions(+), 95 deletions(-) diff --git a/internal/catalogd/controllers/core/clustercatalog_controller.go b/internal/catalogd/controllers/core/clustercatalog_controller.go index c1ee2fc82b..f9bf811f7b 100644 --- a/internal/catalogd/controllers/core/clustercatalog_controller.go +++ b/internal/catalogd/controllers/core/clustercatalog_controller.go @@ -44,8 +44,8 @@ import ( ) const ( - ClusterCatalogFinalizerOwner = "olm.operatorframework.io/clustercatalog-controller" - fbcDeletionFinalizer = "olm.operatorframework.io/delete-server-cache" + clusterCatalogFinalizerOwner = finalizerutil.FinalizerPrefix + "clustercatalog-controller" + fbcDeletionFinalizer = finalizerutil.FinalizerPrefix + "delete-server-cache" // CatalogSources are polled if PollInterval is mentioned, in intervals of wait.Jitter(pollDuration, maxFactor) // wait.Jitter returns a time.Duration between pollDuration and pollDuration + maxFactor * pollDuration. requeueJitterMaxFactor = 0.01 @@ -157,7 +157,7 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *ocv1. // 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. - if err := finalizerutil.RemoveFinalizers(ctx, ClusterCatalogFinalizerOwner, r.Client, catalog, fbcDeletionFinalizer); err != nil { + if _, err := finalizerutil.UpdateFinalizers(ctx, clusterCatalogFinalizerOwner, r.Client, catalog); err != nil { return ctrl.Result{}, fmt.Errorf("error removing finalizer: %v", err) } @@ -173,7 +173,7 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *ocv1. if err := r.deleteCatalogCache(ctx, catalog); err != nil { return ctrl.Result{}, fmt.Errorf("finalizer %q failed: %w", fbcDeletionFinalizer, err) } - if err := finalizerutil.RemoveFinalizers(ctx, ClusterCatalogFinalizerOwner, r.Client, catalog, fbcDeletionFinalizer); err != nil { + if _, err := finalizerutil.UpdateFinalizers(ctx, clusterCatalogFinalizerOwner, r.Client, catalog); err != nil { return ctrl.Result{}, fmt.Errorf("error removing finalizer: %v", err) } // Update status to reflect that catalog is no longer serving @@ -182,7 +182,7 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *ocv1. } // Add finalizer - finalizerAdded, err := finalizerutil.AddFinalizers(ctx, ClusterCatalogFinalizerOwner, r.Client, catalog, fbcDeletionFinalizer) + finalizerAdded, err := finalizerutil.UpdateFinalizers(ctx, clusterCatalogFinalizerOwner, r.Client, catalog, fbcDeletionFinalizer) if err != nil { return ctrl.Result{}, fmt.Errorf("error ensuring finalizer: %v", err) } diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index 21df0f332e..fb85d09ece 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -59,9 +59,9 @@ import ( ) const ( - ClusterExtensionCleanupUnpackCacheFinalizer = "olm.operatorframework.io/cleanup-unpack-cache" - ClusterExtensionCleanupContentManagerCacheFinalizer = "olm.operatorframework.io/cleanup-contentmanager-cache" - ClusterExtensionFinalizerOwner = "olm.operatorframework.io/clusterextension-controller" + ClusterExtensionCleanupUnpackCacheFinalizer = finalizerutil.FinalizerPrefix + "cleanup-unpack-cache" + ClusterExtensionCleanupContentManagerCacheFinalizer = finalizerutil.FinalizerPrefix + "cleanup-contentmanager-cache" + clusterExtensionFinalizerOwner = finalizerutil.FinalizerPrefix + "clusterextension-controller" ) // ClusterExtensionReconciler reconciles a ClusterExtension object @@ -178,7 +178,7 @@ func checkForUnexpectedClusterExtensionFieldChange(a, b *ocv1.ClusterExtension) func (r *ClusterExtensionReconciler) handleDeletion(ctx context.Context, ext *ocv1.ClusterExtension) (ctrl.Result, error) { // Run cleanup for each registered finalizer and collect finalizers to remove - finalizersToRemove := make([]string, 0, len(r.FinalizerHandlers)) + removeFinalizers := false for finalizerKey, handler := range r.FinalizerHandlers { if !controllerutil.ContainsFinalizer(ext, finalizerKey) { continue @@ -187,11 +187,11 @@ func (r *ClusterExtensionReconciler) handleDeletion(ctx context.Context, ext *oc setStatusProgressing(ext, err) return ctrl.Result{}, err } - finalizersToRemove = append(finalizersToRemove, finalizerKey) + removeFinalizers = true } // Remove all finalizers in a single patch operation - if len(finalizersToRemove) > 0 { - if err := finalizerutil.RemoveFinalizers(ctx, ClusterExtensionFinalizerOwner, r.Client, ext, finalizersToRemove...); err != nil { + if removeFinalizers { + if _, err := finalizerutil.UpdateFinalizers(ctx, clusterExtensionFinalizerOwner, r.Client, ext); err != nil { setStatusProgressing(ext, err) return ctrl.Result{}, fmt.Errorf("error removing finalizers: %v", err) } @@ -217,7 +217,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl finalizers = append(finalizers, finalizerKey) } if len(finalizers) > 0 { - if _, err := finalizerutil.AddFinalizers(ctx, ClusterExtensionFinalizerOwner, r.Client, ext, finalizers...); err != nil { + if _, err := finalizerutil.UpdateFinalizers(ctx, clusterExtensionFinalizerOwner, r.Client, ext, finalizers...); err != nil { setStatusProgressing(ext, err) return ctrl.Result{}, fmt.Errorf("error ensuring finalizers: %v", err) } diff --git a/internal/operator-controller/controllers/clusterextension_controller_test.go b/internal/operator-controller/controllers/clusterextension_controller_test.go index 36c91748ff..5403f46fc9 100644 --- a/internal/operator-controller/controllers/clusterextension_controller_test.go +++ b/internal/operator-controller/controllers/clusterextension_controller_test.go @@ -32,6 +32,7 @@ import ( "github.com/operator-framework/operator-controller/internal/operator-controller/controllers" "github.com/operator-framework/operator-controller/internal/operator-controller/labels" "github.com/operator-framework/operator-controller/internal/operator-controller/resolve" + finalizerutil "github.com/operator-framework/operator-controller/internal/shared/util/finalizer" imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image" ) @@ -767,7 +768,7 @@ func TestClusterExtensionDeleteFinalizerFails(t *testing.T) { Image: "quay.io/operatorhubio/prometheus@fake1.0.0", }, &v, nil, nil }) - fakeFinalizer := "fake.testfinalizer.io" + fakeFinalizer := finalizerutil.FinalizerPrefix + "fake.testfinalizer" finalizersMessage := "still have finalizers" reconciler.Applier = &MockApplier{ installCompleted: true, diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller.go b/internal/operator-controller/controllers/clusterextensionrevision_controller.go index 14452b7ce4..dfbdcd83ef 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller.go @@ -37,8 +37,8 @@ import ( const ( ClusterExtensionRevisionOwnerLabel = "olm.operatorframework.io/owner" - clusterExtensionRevisionTeardownFinalizer = "olm.operatorframework.io/teardown" - CluserExtensionRevisionFinalizerOwner = "olm.operatorframework.io/clusterextensionrevision-controller" + clusterExtensionRevisionTeardownFinalizer = finalizerutil.FinalizerPrefix + "teardown" + cluserExtensionRevisionFinalizerOwner = finalizerutil.FinalizerPrefix + "clusterextensionrevision-controller" ) // ClusterExtensionRevisionReconciler actions individual snapshots of ClusterExtensions, @@ -124,7 +124,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev // // Reconcile // - if _, err := finalizerutil.AddFinalizers(ctx, CluserExtensionRevisionFinalizerOwner, c.Client, rev, clusterExtensionRevisionTeardownFinalizer); err != nil { + if _, err := finalizerutil.UpdateFinalizers(ctx, cluserExtensionRevisionFinalizerOwner, c.Client, rev, clusterExtensionRevisionTeardownFinalizer); err != nil { meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{ Type: ocv1.ClusterExtensionRevisionTypeAvailable, Status: metav1.ConditionFalse, @@ -353,7 +353,7 @@ func (c *ClusterExtensionRevisionReconciler) teardown(ctx context.Context, rev * return ctrl.Result{}, nil } - if err := finalizerutil.RemoveFinalizers(ctx, CluserExtensionRevisionFinalizerOwner, c.Client, rev, clusterExtensionRevisionTeardownFinalizer); err != nil { + if _, err := finalizerutil.UpdateFinalizers(ctx, cluserExtensionRevisionFinalizerOwner, c.Client, rev); err != nil { return ctrl.Result{}, fmt.Errorf("error removing teardown finalizer: %v", err) } return ctrl.Result{}, nil diff --git a/internal/shared/util/finalizer/finalizer.go b/internal/shared/util/finalizer/finalizer.go index 18039da0dd..457399c868 100644 --- a/internal/shared/util/finalizer/finalizer.go +++ b/internal/shared/util/finalizer/finalizer.go @@ -21,100 +21,58 @@ import ( "fmt" "slices" "sort" + "strings" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) -// AddFinalizers adds one or more finalizers to the object using server-side apply. -// If all finalizers already exist, this is a no-op and returns (false, nil). -// Returns (true, nil) if any finalizers were added. +const ( + FinalizerPrefix = "olm.operatorframework.io/" +) + +// UpdateFinalizers sets the finalizers on an object to exactly the provided list using server-side apply. +// If no finalizers are supplied, all finalizers will be removed from the object. +// If one finalizer is supplied, all other finalizers will be removed and only the supplied one will remain. +// Returns (true, nil) if the finalizers were changed, (false, nil) if they were already set to the desired value. // Note: This function will update the passed object with the server response. -func AddFinalizers(ctx context.Context, owner string, c client.Client, obj client.Object, finalizers ...string) (bool, error) { - if len(finalizers) == 0 { - return false, nil +func UpdateFinalizers(ctx context.Context, owner string, c client.Client, obj client.Object, finalizers ...string) (bool, error) { + // Sort the desired finalizers for consistent ordering + newFinalizers := slices.Clone(finalizers) + if newFinalizers == nil { + newFinalizers = []string{} } - - // Determine which finalizers need to be added - var toAdd []string - for _, finalizer := range finalizers { - if !controllerutil.ContainsFinalizer(obj, finalizer) { - toAdd = append(toAdd, finalizer) + // Possibly overkill, but it will ensure our finalizers use the proper prefix + for _, s := range newFinalizers { + if !strings.HasPrefix(s, FinalizerPrefix) { + panic(fmt.Sprintf("finalizer does not have %q prefix: %q", FinalizerPrefix, s)) } } + sort.Strings(newFinalizers) - if len(toAdd) == 0 { - return false, nil - } - - // Sort the finalizers to add for consistent ordering - sort.Strings(toAdd) - - // Create a copy of the finalizers list to avoid mutating the object + // Check if the current finalizers already match the desired state + // Remove any non-"olm.operatorframework.io" finalizers (ones we don't manage) from the list currentFinalizers := obj.GetFinalizers() - newFinalizers := make([]string, len(currentFinalizers), len(currentFinalizers)+len(toAdd)) - copy(newFinalizers, currentFinalizers) - newFinalizers = append(newFinalizers, toAdd...) - - // Get the GVK for this object type - gvk, err := apiutil.GVKForObject(obj, c.Scheme()) - if err != nil { - return false, fmt.Errorf("getting object kind: %w", err) - } - - // Create an unstructured object for server-side apply - u := &unstructured.Unstructured{} - u.SetGroupVersionKind(gvk) - u.SetName(obj.GetName()) - u.SetNamespace(obj.GetNamespace()) - u.SetFinalizers(newFinalizers) - - // Use server-side apply to update finalizers - if err := c.Patch(ctx, u, client.Apply, client.ForceOwnership, client.FieldOwner(owner)); err != nil { - return false, fmt.Errorf("adding finalizer: %w", err) - } - - // Update the passed object with the new finalizers - obj.SetFinalizers(u.GetFinalizers()) - obj.SetResourceVersion(u.GetResourceVersion()) - - return true, nil -} - -// RemoveFinalizers removes one or more finalizers from the object using server-side apply. -// If none of the finalizers exist, this is a no-op. -func RemoveFinalizers(ctx context.Context, owner string, c client.Client, obj client.Object, finalizers ...string) error { - if len(finalizers) == 0 { - return nil - } - - // Create a set of finalizers to remove for efficient lookup - toRemove := sets.New(finalizers...) - hasAny := false - for _, finalizer := range finalizers { - if controllerutil.ContainsFinalizer(obj, finalizer) { - hasAny = true - } + currentSorted := slices.Clone(currentFinalizers) + currentSorted = slices.DeleteFunc(currentSorted, func(f string) bool { + return !strings.HasPrefix(f, FinalizerPrefix) + }) + if currentSorted == nil { + currentSorted = []string{} } + sort.Strings(currentSorted) - if !hasAny { - return nil + // With only "olm.operatorframework.io" finalizers, other controller's finalizers + // won't interfere in this check + if slices.Equal(currentSorted, newFinalizers) { + return false, nil } - // Create a copy of the finalizers list and remove the specified finalizers - currentFinalizers := obj.GetFinalizers() - newFinalizers := slices.Clone(currentFinalizers) - newFinalizers = slices.DeleteFunc(newFinalizers, func(f string) bool { - return toRemove.Has(f) - }) - // Get the GVK for this object type gvk, err := apiutil.GVKForObject(obj, c.Scheme()) if err != nil { - return fmt.Errorf("getting object kind: %w", err) + return false, fmt.Errorf("getting object kind: %w", err) } // Create an unstructured object for server-side apply @@ -126,12 +84,12 @@ func RemoveFinalizers(ctx context.Context, owner string, c client.Client, obj cl // Use server-side apply to update finalizers if err := c.Patch(ctx, u, client.Apply, client.ForceOwnership, client.FieldOwner(owner)); err != nil { - return fmt.Errorf("removing finalizer: %w", err) + return false, fmt.Errorf("updating finalizers: %w", err) } // Update the passed object with the new finalizers obj.SetFinalizers(u.GetFinalizers()) obj.SetResourceVersion(u.GetResourceVersion()) - return nil + return true, nil } From 9dea2351669254b0c95d34775d8fdbd3a286abe5 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Sun, 16 Nov 2025 10:56:39 -0500 Subject: [PATCH 7/7] fixup! fixup! Use SSA instead of Update for finalizer operations Signed-off-by: Todd Short --- .../catalogd/controllers/core/clustercatalog_controller.go | 6 +++--- .../controllers/clusterextension_controller.go | 4 ++-- .../controllers/clusterextensionrevision_controller.go | 4 ++-- internal/shared/util/finalizer/finalizer.go | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/catalogd/controllers/core/clustercatalog_controller.go b/internal/catalogd/controllers/core/clustercatalog_controller.go index f9bf811f7b..41440ccb49 100644 --- a/internal/catalogd/controllers/core/clustercatalog_controller.go +++ b/internal/catalogd/controllers/core/clustercatalog_controller.go @@ -157,7 +157,7 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *ocv1. // 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. - if _, err := finalizerutil.UpdateFinalizers(ctx, clusterCatalogFinalizerOwner, r.Client, catalog); err != nil { + if _, err := finalizerutil.EnsureFinalizers(ctx, clusterCatalogFinalizerOwner, r.Client, catalog); err != nil { return ctrl.Result{}, fmt.Errorf("error removing finalizer: %v", err) } @@ -173,7 +173,7 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *ocv1. if err := r.deleteCatalogCache(ctx, catalog); err != nil { return ctrl.Result{}, fmt.Errorf("finalizer %q failed: %w", fbcDeletionFinalizer, err) } - if _, err := finalizerutil.UpdateFinalizers(ctx, clusterCatalogFinalizerOwner, r.Client, catalog); err != nil { + if _, err := finalizerutil.EnsureFinalizers(ctx, clusterCatalogFinalizerOwner, r.Client, catalog); err != nil { return ctrl.Result{}, fmt.Errorf("error removing finalizer: %v", err) } // Update status to reflect that catalog is no longer serving @@ -182,7 +182,7 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *ocv1. } // Add finalizer - finalizerAdded, err := finalizerutil.UpdateFinalizers(ctx, clusterCatalogFinalizerOwner, r.Client, catalog, fbcDeletionFinalizer) + finalizerAdded, err := finalizerutil.EnsureFinalizers(ctx, clusterCatalogFinalizerOwner, r.Client, catalog, fbcDeletionFinalizer) if err != nil { return ctrl.Result{}, fmt.Errorf("error ensuring finalizer: %v", err) } diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index fb85d09ece..f37dad4a81 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -191,7 +191,7 @@ func (r *ClusterExtensionReconciler) handleDeletion(ctx context.Context, ext *oc } // Remove all finalizers in a single patch operation if removeFinalizers { - if _, err := finalizerutil.UpdateFinalizers(ctx, clusterExtensionFinalizerOwner, r.Client, ext); err != nil { + if _, err := finalizerutil.EnsureFinalizers(ctx, clusterExtensionFinalizerOwner, r.Client, ext); err != nil { setStatusProgressing(ext, err) return ctrl.Result{}, fmt.Errorf("error removing finalizers: %v", err) } @@ -217,7 +217,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl finalizers = append(finalizers, finalizerKey) } if len(finalizers) > 0 { - if _, err := finalizerutil.UpdateFinalizers(ctx, clusterExtensionFinalizerOwner, r.Client, ext, finalizers...); err != nil { + if _, err := finalizerutil.EnsureFinalizers(ctx, clusterExtensionFinalizerOwner, r.Client, ext, finalizers...); err != nil { setStatusProgressing(ext, err) return ctrl.Result{}, fmt.Errorf("error ensuring finalizers: %v", err) } diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller.go b/internal/operator-controller/controllers/clusterextensionrevision_controller.go index dfbdcd83ef..c8c379ff05 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller.go @@ -124,7 +124,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev // // Reconcile // - if _, err := finalizerutil.UpdateFinalizers(ctx, cluserExtensionRevisionFinalizerOwner, c.Client, rev, clusterExtensionRevisionTeardownFinalizer); err != nil { + if _, err := finalizerutil.EnsureFinalizers(ctx, cluserExtensionRevisionFinalizerOwner, c.Client, rev, clusterExtensionRevisionTeardownFinalizer); err != nil { meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{ Type: ocv1.ClusterExtensionRevisionTypeAvailable, Status: metav1.ConditionFalse, @@ -353,7 +353,7 @@ func (c *ClusterExtensionRevisionReconciler) teardown(ctx context.Context, rev * return ctrl.Result{}, nil } - if _, err := finalizerutil.UpdateFinalizers(ctx, cluserExtensionRevisionFinalizerOwner, c.Client, rev); err != nil { + if _, err := finalizerutil.EnsureFinalizers(ctx, cluserExtensionRevisionFinalizerOwner, c.Client, rev); err != nil { return ctrl.Result{}, fmt.Errorf("error removing teardown finalizer: %v", err) } return ctrl.Result{}, nil diff --git a/internal/shared/util/finalizer/finalizer.go b/internal/shared/util/finalizer/finalizer.go index 457399c868..401fbf985c 100644 --- a/internal/shared/util/finalizer/finalizer.go +++ b/internal/shared/util/finalizer/finalizer.go @@ -32,12 +32,12 @@ const ( FinalizerPrefix = "olm.operatorframework.io/" ) -// UpdateFinalizers sets the finalizers on an object to exactly the provided list using server-side apply. +// EnsureFinalizers sets the finalizers on an object to exactly the provided list using server-side apply. // If no finalizers are supplied, all finalizers will be removed from the object. // If one finalizer is supplied, all other finalizers will be removed and only the supplied one will remain. // Returns (true, nil) if the finalizers were changed, (false, nil) if they were already set to the desired value. // Note: This function will update the passed object with the server response. -func UpdateFinalizers(ctx context.Context, owner string, c client.Client, obj client.Object, finalizers ...string) (bool, error) { +func EnsureFinalizers(ctx context.Context, owner string, c client.Client, obj client.Object, finalizers ...string) (bool, error) { // Sort the desired finalizers for consistent ordering newFinalizers := slices.Clone(finalizers) if newFinalizers == nil {