diff --git a/cmd/catalogd/main.go b/cmd/catalogd/main.go index 36f7b16752..af2463e2c6 100644 --- a/cmd/catalogd/main.go +++ b/cmd/catalogd/main.go @@ -59,6 +59,7 @@ import ( "github.com/operator-framework/operator-controller/internal/catalogd/storage" "github.com/operator-framework/operator-controller/internal/catalogd/webhook" sharedcontrollers "github.com/operator-framework/operator-controller/internal/shared/controllers" + cacheutil "github.com/operator-framework/operator-controller/internal/shared/util/cache" fsutil "github.com/operator-framework/operator-controller/internal/shared/util/fs" httputil "github.com/operator-framework/operator-controller/internal/shared/util/http" imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image" @@ -254,6 +255,8 @@ func run(ctx context.Context) error { cacheOptions := crcache.Options{ ByObject: map[client.Object]crcache.ByObject{}, + // Memory optimization: strip managed fields and large annotations from cached objects + DefaultTransform: cacheutil.StripManagedFieldsAndAnnotations(), } saKey, err := sautil.GetServiceAccount() diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 7425c7b66b..19c30c6e39 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" @@ -78,6 +76,7 @@ import ( "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render/registryv1" "github.com/operator-framework/operator-controller/internal/operator-controller/scheme" sharedcontrollers "github.com/operator-framework/operator-controller/internal/shared/controllers" + cacheutil "github.com/operator-framework/operator-controller/internal/shared/util/cache" fsutil "github.com/operator-framework/operator-controller/internal/shared/util/fs" httputil "github.com/operator-framework/operator-controller/internal/shared/util/http" imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image" @@ -232,6 +231,8 @@ func run() error { cfg.systemNamespace: {LabelSelector: k8slabels.Everything()}, }, DefaultLabelSelector: k8slabels.Nothing(), + // Memory optimization: strip managed fields and large annotations from cached objects + DefaultTransform: cacheutil.StripAnnotations(), } if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) { @@ -388,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() @@ -440,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 { @@ -461,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") @@ -528,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()) @@ -557,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 @@ -636,7 +631,6 @@ func setupHelm( ceReconciler *controllers.ClusterExtensionReconciler, preflights []applier.Preflight, ceController crcontroller.Controller, - clusterExtensionFinalizers crfinalizer.Registerer, regv1ManifestProvider applier.ManifestProvider, ) error { coreClient, err := corev1client.NewForConfig(mgr.GetConfig()) @@ -675,14 +669,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..41440ccb49 100644 --- a/internal/catalogd/controllers/core/clustercatalog_controller.go +++ b/internal/catalogd/controllers/core/clustercatalog_controller.go @@ -34,17 +34,18 @@ 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" ) const ( - 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 @@ -59,8 +60,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 +105,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) + 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 +126,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"). @@ -177,26 +157,37 @@ 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. - controllerutil.RemoveFinalizer(catalog, fbcDeletionFinalizer) + if _, err := finalizerutil.EnsureFinalizers(ctx, clusterCatalogFinalizerOwner, r.Client, catalog); err != nil { + return ctrl.Result{}, fmt.Errorf("error removing finalizer: %v", err) + } 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.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 + 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). + // Add finalizer + finalizerAdded, err := finalizerutil.EnsureFinalizers(ctx, clusterCatalogFinalizerOwner, 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 } @@ -415,34 +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{} - 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 +// Compare resources - Annotations/Labels/Spec +func checkForUnexpectedFieldChange(a, b *ocv1.ClusterCatalog) bool { + if !equality.Semantic.DeepEqual(a.Annotations, b.Annotations) { + return true } - r.finalizers = f - return nil + if !equality.Semantic.DeepEqual(a.Labels, b.Labels) { + return true + } + return !equality.Semantic.DeepEqual(a.Spec, b.Spec) } 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..c618b32c60 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{}, }, }, { @@ -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) diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 6abcd0c43b..3895b49dfa 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -27,6 +27,7 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/labels" + "github.com/operator-framework/operator-controller/internal/shared/util/cache" ) const ( @@ -66,6 +67,9 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease( maps.Copy(labels, objectLabels) obj.SetLabels(labels) + // Memory optimization: strip large annotations + // Note: ApplyStripTransform never returns an error in practice + _ = cache.ApplyStripAnnotationsTransform(&obj) sanitizedUnstructured(ctx, &obj) objs = append(objs, ocv1.ClusterExtensionRevisionObject{ @@ -117,6 +121,10 @@ func (r *SimpleRevisionGenerator) GenerateRevision( unstr := unstructured.Unstructured{Object: unstrObj} unstr.SetGroupVersionKind(gvk) + // Memory optimization: strip large annotations + if err := cache.ApplyStripAnnotationsTransform(&unstr); err != nil { + return nil, err + } sanitizedUnstructured(ctx, &unstr) objs = append(objs, ocv1.ClusterExtensionRevisionObject{ diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index d8d9d8de0e..ed126f04e0 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,12 +54,14 @@ 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" ) const ( - ClusterExtensionCleanupUnpackCacheFinalizer = "olm.operatorframework.io/cleanup-unpack-cache" - ClusterExtensionCleanupContentManagerCacheFinalizer = "olm.operatorframework.io/cleanup-contentmanager-cache" + ClusterExtensionCleanupUnpackCacheFinalizer = finalizerutil.FinalizerPrefix + "cleanup-unpack-cache" + ClusterExtensionCleanupContentManagerCacheFinalizer = finalizerutil.FinalizerPrefix + "cleanup-contentmanager-cache" + clusterExtensionFinalizerOwner = finalizerutil.FinalizerPrefix + "clusterextension-controller" ) // ClusterExtensionReconciler reconciles a ClusterExtension object @@ -73,9 +75,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,31 +115,18 @@ 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) + unexpectedFieldsChanged := checkForUnexpectedClusterExtensionFieldChange(existingExt, reconciledExt) if unexpectedFieldsChanged { panic("spec or metadata changed by reconciler") } - // Save the finalizers off to the side. If we update the status, the reconciledExt will be updated - // to contain the new state of the ClusterExtension, which contains the status update, but (critically) - // does not contain the finalizers. After the status update, we need to re-add the finalizers to the - // reconciledExt before updating the object. - 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,11 +149,15 @@ func ensureAllConditionsWithReason(ext *ocv1.ClusterExtension, reason v1alpha1.C } } -// Compare resources - ignoring status & metadata.finalizers -func checkForUnexpectedClusterExtensionFieldChange(a, b ocv1.ClusterExtension) bool { - a.Status, b.Status = ocv1.ClusterExtensionStatus{}, ocv1.ClusterExtensionStatus{} - a.Finalizers, b.Finalizers = []string{}, []string{} - return !equality.Semantic.DeepEqual(a, b) +// Compare resources - Annotations/Labels/Spec +func checkForUnexpectedClusterExtensionFieldChange(a, b *ocv1.ClusterExtension) bool { + if !equality.Semantic.DeepEqual(a.Annotations, b.Annotations) { + return true + } + if !equality.Semantic.DeepEqual(a.Labels, b.Labels) { + return true + } + return !equality.Semantic.DeepEqual(a.Spec, b.Spec) } // Helper function to do the actual reconcile @@ -179,28 +175,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 + removeFinalizers := false + 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 + } + removeFinalizers = true + } + // Remove all finalizers in a single patch operation + if removeFinalizers { + if _, err := finalizerutil.EnsureFinalizers(ctx, clusterExtensionFinalizerOwner, r.Client, ext); 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) + } + + // 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.EnsureFinalizers(ctx, clusterExtensionFinalizerOwner, 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 20761aec91..2e255c202d 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,9 +30,9 @@ 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" + finalizerutil "github.com/operator-framework/operator-controller/internal/shared/util/finalizer" imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image" ) @@ -770,7 +769,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, @@ -783,11 +782,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 ec035eee78..261fc291cd 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" @@ -26,7 +25,6 @@ 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" @@ -34,10 +32,12 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/labels" + finalizerutil "github.com/operator-framework/operator-controller/internal/shared/util/finalizer" ) const ( - clusterExtensionRevisionTeardownFinalizer = "olm.operatorframework.io/teardown" + clusterExtensionRevisionTeardownFinalizer = finalizerutil.FinalizerPrefix + "teardown" + cluserExtensionRevisionFinalizerOwner = finalizerutil.FinalizerPrefix + "clusterextensionrevision-controller" ) // ClusterExtensionRevisionReconciler actions individual snapshots of ClusterExtensions, @@ -82,7 +82,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") } @@ -100,15 +100,14 @@ 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 = "", "" +// Compare resources - Annotations/Labels/Spec +func checkForUnexpectedClusterExtensionRevisionFieldChange(a, b *ocv1.ClusterExtensionRevision) bool { + if !equality.Semantic.DeepEqual(a.Annotations, b.Annotations) { + return true + } + if !equality.Semantic.DeepEqual(a.Labels, b.Labels) { + return true + } return !equality.Semantic.DeepEqual(a.Spec, b.Spec) } @@ -134,7 +133,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev // // Reconcile // - if err := c.ensureFinalizer(ctx, 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, @@ -369,7 +368,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.EnsureFinalizers(ctx, cluserExtensionRevisionFinalizerOwner, c.Client, rev); err != nil { return ctrl.Result{}, fmt.Errorf("error removing teardown finalizer: %v", err) } return ctrl.Result{}, nil @@ -408,53 +407,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 -} - // listPreviousRevisions returns active revisions belonging to the same ClusterExtension with lower revision numbers. // Filters out the current revision, archived revisions, deleting revisions, and revisions with equal or higher numbers. func (c *ClusterExtensionRevisionReconciler) listPreviousRevisions(ctx context.Context, rev *ocv1.ClusterExtensionRevision) ([]*ocv1.ClusterExtensionRevision, error) { diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go b/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go index e88051537b..34d589f7ca 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" @@ -499,13 +498,16 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) { } }, validate: func(t *testing.T, c client.Client) { - t.Log("cluster revision is deleted") + t.Log("revision is deleted after finalizer removal") rev := &ocv1.ClusterExtensionRevision{} err := c.Get(t.Context(), client.ObjectKey{ Name: clusterExtensionRevisionName, }, rev) + // When DeletionTimestamp is set and the last finalizer is removed, + // Kubernetes deletes the object immediately. The fake client simulates + // this behavior, so we expect a NotFound error. require.Error(t, err) - require.True(t, errors.IsNotFound(err)) + require.NoError(t, client.IgnoreNotFound(err), "expected NotFound error, got: %v", err) }, }, { 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/cache/transform.go b/internal/shared/util/cache/transform.go new file mode 100644 index 0000000000..50a5530395 --- /dev/null +++ b/internal/shared/util/cache/transform.go @@ -0,0 +1,91 @@ +package cache + +import ( + "maps" + + toolscache "k8s.io/client-go/tools/cache" + crcache "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// stripAnnotations removes memory-heavy annotations that aren't needed for controller operations. +func stripAnnotations(obj interface{}) (interface{}, error) { + if metaObj, ok := obj.(client.Object); ok { + // Remove the last-applied-configuration annotation which can be very large + // Clone the annotations map to avoid modifying shared references + annotations := metaObj.GetAnnotations() + if annotations != nil { + annotations = maps.Clone(annotations) + delete(annotations, "kubectl.kubernetes.io/last-applied-configuration") + if len(annotations) == 0 { + metaObj.SetAnnotations(nil) + } else { + metaObj.SetAnnotations(annotations) + } + } + } + return obj, nil +} + +// StripManagedFieldsAndAnnotations returns a cache transform function that removes +// memory-heavy fields that aren't needed for controller operations. +// This significantly reduces memory usage in informer caches by removing: +// - Managed fields (can be several KB per object) +// - kubectl.kubernetes.io/last-applied-configuration annotation (can be very large) +// +// Use this function as a DefaultTransform in controller-runtime cache.Options +// to reduce memory overhead across all cached objects. +// +// Example: +// +// cacheOptions := cache.Options{ +// DefaultTransform: cacheutil.StripManagedFieldsAndAnnotations(), +// } +func StripManagedFieldsAndAnnotations() toolscache.TransformFunc { + // Use controller-runtime's built-in TransformStripManagedFields and compose it + // with our custom annotation stripping transform + managedFieldsTransform := crcache.TransformStripManagedFields() + + return func(obj interface{}) (interface{}, error) { + // First strip managed fields using controller-runtime's transform + obj, err := managedFieldsTransform(obj) + if err != nil { + return obj, err + } + + // Then strip the large annotations + return stripAnnotations(obj) + } +} + +// StripAnnotations returns a cache transform function that removes +// memory-heavy annotation fields that aren't needed for controller operations. +// This significantly reduces memory usage in informer caches by removing: +// - kubectl.kubernetes.io/last-applied-configuration annotation (can be very large) +// +// Use this function as a DefaultTransform in controller-runtime cache.Options +// to reduce memory overhead across all cached objects. +// +// Example: +// +// cacheOptions := cache.Options{ +// DefaultTransform: cacheutil.StripAnnotations(), +// } +func StripAnnotations() toolscache.TransformFunc { + return func(obj interface{}) (interface{}, error) { + // Strip the large annotations + return stripAnnotations(obj) + } +} + +// ApplyStripAnnotationsTransform applies the strip transform directly to an object. +// This is a convenience function for cases where you need to strip fields +// from an object outside of the cache transform context. +// +// Note: This function never returns an error in practice, but returns error +// to satisfy the TransformFunc interface. +func ApplyStripAnnotationsTransform(obj client.Object) error { + transform := StripAnnotations() + _, err := transform(obj) + return err +} diff --git a/internal/shared/util/finalizer/finalizer.go b/internal/shared/util/finalizer/finalizer.go new file mode 100644 index 0000000000..b0b4205a69 --- /dev/null +++ b/internal/shared/util/finalizer/finalizer.go @@ -0,0 +1,101 @@ +/* +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" + "encoding/json" + "fmt" + "slices" + "sort" + "strings" + + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + FinalizerPrefix = "olm.operatorframework.io/" +) + +// EnsureFinalizers sets the FinalizerPrefix finalizers an object using Patch (vs. Update) +// If no finalizers are supplied, all FinalizerPrefix finalizers will be removed from the object. +// If any finalizers are supplied, all other FinalizerPrefix finalizers will be removed and only the supplied ones 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 ONLY the finalizers field of the passed object, not other metadata fields. +func EnsureFinalizers(ctx context.Context, owner string, c client.Client, obj client.Object, finalizers ...string) (bool, error) { + newFinalizers := slices.Clone(finalizers) + if newFinalizers == nil { + newFinalizers = []string{} + } + // 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)) + } + } + + // Add any other, non-FinalizerPrefix, finalizers to the newFinalizer list + currentFinalizers := obj.GetFinalizers() + for _, f := range currentFinalizers { + if !strings.HasPrefix(f, FinalizerPrefix) { + newFinalizers = append(newFinalizers, f) + } + } + // Sort the desired finalizers for consistent ordering + sort.Strings(newFinalizers) + + // Check if the current finalizers already match the desired state (newFinalizers) + currentSorted := slices.Clone(currentFinalizers) + if currentSorted == nil { + currentSorted = []string{} + } + sort.Strings(currentSorted) + + // Compare the current list with the desired newFinalizers + if slices.Equal(currentSorted, newFinalizers) { + return false, nil + } + + patch := map[string]any{ + "metadata": map[string]any{ + "finalizers": newFinalizers, + }, + } + + patchJSON, err := json.Marshal(patch) + if err != nil { + return false, fmt.Errorf("marshalling patch to ensure finalizers: %w", err) + } + + // Create a copy to use for patching. We patch the copy to avoid having the server's + // patch response overwrite the original object with potentially changed metadata fields + // (like annotations) that the caller didn't intend to modify. + objCopy := obj.DeepCopyObject().(client.Object) + + // Use patch to update finalizers on the server + if err := c.Patch(ctx, objCopy, client.RawPatch(types.MergePatchType, patchJSON)); err != nil { + return false, fmt.Errorf("updating finalizers: %w", err) + } + + // Update the finalizers and resource version in the original object to reflect the change. + // The resource version must be updated to avoid conflicts with subsequent operations. + obj.SetFinalizers(objCopy.GetFinalizers()) + obj.SetResourceVersion(objCopy.GetResourceVersion()) + + return true, nil +} diff --git a/internal/shared/util/finalizer/finalizer_test.go b/internal/shared/util/finalizer/finalizer_test.go new file mode 100644 index 0000000000..1ec397f1a8 --- /dev/null +++ b/internal/shared/util/finalizer/finalizer_test.go @@ -0,0 +1,415 @@ +/* +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_test + +import ( + "context" + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + "github.com/operator-framework/operator-controller/internal/shared/util/finalizer" +) + +func TestEnsureFinalizers(t *testing.T) { + const ( + testOwner = "test-owner" + testNS = "test-namespace" + testName = "test-configmap" + ) + + tests := []struct { + name string + initialFinalizers []string + newFinalizers []string + wantChanged bool + wantFinalizers []string + wantErr bool + }{ + { + name: "add single finalizer to empty object", + initialFinalizers: nil, + newFinalizers: []string{finalizer.FinalizerPrefix + "test"}, + wantChanged: true, + wantFinalizers: []string{finalizer.FinalizerPrefix + "test"}, + }, + { + name: "add multiple finalizers to empty object", + initialFinalizers: nil, + newFinalizers: []string{finalizer.FinalizerPrefix + "test1", finalizer.FinalizerPrefix + "test2"}, + wantChanged: true, + wantFinalizers: []string{finalizer.FinalizerPrefix + "test1", finalizer.FinalizerPrefix + "test2"}, + }, + { + name: "finalizers already match - no change", + initialFinalizers: []string{finalizer.FinalizerPrefix + "test"}, + newFinalizers: []string{finalizer.FinalizerPrefix + "test"}, + wantChanged: false, + wantFinalizers: []string{finalizer.FinalizerPrefix + "test"}, + }, + { + name: "finalizers already match (multiple) - no change", + initialFinalizers: []string{finalizer.FinalizerPrefix + "test1", finalizer.FinalizerPrefix + "test2"}, + newFinalizers: []string{finalizer.FinalizerPrefix + "test1", finalizer.FinalizerPrefix + "test2"}, + wantChanged: false, + wantFinalizers: []string{finalizer.FinalizerPrefix + "test1", finalizer.FinalizerPrefix + "test2"}, + }, + { + name: "finalizers match but different order - no change", + initialFinalizers: []string{finalizer.FinalizerPrefix + "test2", finalizer.FinalizerPrefix + "test1"}, + newFinalizers: []string{finalizer.FinalizerPrefix + "test1", finalizer.FinalizerPrefix + "test2"}, + wantChanged: false, + // When no change happens, the object is not patched, so finalizers remain in their original order + wantFinalizers: []string{finalizer.FinalizerPrefix + "test2", finalizer.FinalizerPrefix + "test1"}, + }, + { + name: "remove all FinalizerPrefix finalizers", + initialFinalizers: []string{finalizer.FinalizerPrefix + "test1", finalizer.FinalizerPrefix + "test2"}, + newFinalizers: []string{}, + wantChanged: true, + wantFinalizers: []string{}, + }, + { + name: "remove all FinalizerPrefix finalizers with nil", + initialFinalizers: []string{finalizer.FinalizerPrefix + "test"}, + newFinalizers: nil, + wantChanged: true, + wantFinalizers: []string{}, + }, + { + name: "preserve non-FinalizerPrefix finalizers", + initialFinalizers: []string{"other.io/finalizer", finalizer.FinalizerPrefix + "test"}, + newFinalizers: []string{}, + wantChanged: true, + wantFinalizers: []string{"other.io/finalizer"}, + }, + { + name: "preserve non-FinalizerPrefix finalizers and add new ones", + initialFinalizers: []string{"other.io/finalizer"}, + newFinalizers: []string{finalizer.FinalizerPrefix + "test"}, + wantChanged: true, + wantFinalizers: []string{finalizer.FinalizerPrefix + "test", "other.io/finalizer"}, + }, + { + name: "replace FinalizerPrefix finalizers while preserving others", + initialFinalizers: []string{"other.io/finalizer", finalizer.FinalizerPrefix + "old"}, + newFinalizers: []string{finalizer.FinalizerPrefix + "new"}, + wantChanged: true, + wantFinalizers: []string{finalizer.FinalizerPrefix + "new", "other.io/finalizer"}, + }, + { + name: "multiple non-FinalizerPrefix finalizers preserved and sorted", + initialFinalizers: []string{"z.io/finalizer", "a.io/finalizer", finalizer.FinalizerPrefix + "test"}, + newFinalizers: []string{finalizer.FinalizerPrefix + "new"}, + wantChanged: true, + wantFinalizers: []string{"a.io/finalizer", finalizer.FinalizerPrefix + "new", "z.io/finalizer"}, + }, + { + name: "no change when object already has correct finalizers with others", + initialFinalizers: []string{finalizer.FinalizerPrefix + "test", "other.io/finalizer"}, + newFinalizers: []string{finalizer.FinalizerPrefix + "test"}, + wantChanged: false, + wantFinalizers: []string{finalizer.FinalizerPrefix + "test", "other.io/finalizer"}, + }, + { + name: "update existing FinalizerPrefix finalizer", + initialFinalizers: []string{finalizer.FinalizerPrefix + "old"}, + newFinalizers: []string{finalizer.FinalizerPrefix + "new"}, + wantChanged: true, + wantFinalizers: []string{finalizer.FinalizerPrefix + "new"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: testName, + Namespace: testNS, + Finalizers: tt.initialFinalizers, + ResourceVersion: "1", + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(cm). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + // Parse the patch to validate structure + patchBytes, err := patch.Data(obj) + require.NoError(t, err) + + var patchMap map[string]interface{} + require.NoError(t, json.Unmarshal(patchBytes, &patchMap)) + + // Verify patch structure + metadata, ok := patchMap["metadata"].(map[string]interface{}) + require.True(t, ok, "patch should contain metadata") + + finalizers, ok := metadata["finalizers"] + require.True(t, ok, "patch metadata should contain finalizers") + + // Update the object with new finalizers and increment resource version + // to simulate what the API server does + if finalizerSlice, ok := finalizers.([]interface{}); ok { + stringFinalizers := make([]string, len(finalizerSlice)) + for i, f := range finalizerSlice { + stringFinalizers[i] = f.(string) + } + obj.SetFinalizers(stringFinalizers) + // Simulate API server incrementing resourceVersion + obj.SetResourceVersion("2") + } + + return nil + }, + }). + Build() + + ctx := context.Background() + changed, err := finalizer.EnsureFinalizers(ctx, testOwner, fakeClient, cm, tt.newFinalizers...) + + if tt.wantErr { + require.Error(t, err) + return + } + + require.NoError(t, err) + assert.Equal(t, tt.wantChanged, changed, "unexpected changed value") + assert.Equal(t, tt.wantFinalizers, cm.GetFinalizers(), "unexpected finalizers") + }) + } +} + +func TestEnsureFinalizers_PanicOnInvalidPrefix(t *testing.T) { + tests := []struct { + name string + finalizers []string + wantPanic bool + }{ + { + name: "valid finalizer with correct prefix", + finalizers: []string{finalizer.FinalizerPrefix + "test"}, + wantPanic: false, + }, + { + name: "invalid finalizer without prefix", + finalizers: []string{"test"}, + wantPanic: true, + }, + { + name: "invalid finalizer with wrong prefix", + finalizers: []string{"other.io/test"}, + wantPanic: true, + }, + { + name: "mix of valid and invalid finalizers", + finalizers: []string{finalizer.FinalizerPrefix + "test", "invalid"}, + wantPanic: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "test", + ResourceVersion: "1", + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(cm). + Build() + + test := func() { + _, _ = finalizer.EnsureFinalizers(context.Background(), "test", fakeClient, cm, tt.finalizers...) + } + + if tt.wantPanic { + require.Panics(t, test) + } else { + require.NotPanics(t, test) + } + }) + } +} + +func TestEnsureFinalizers_FinalizersSorting(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "test", + Finalizers: []string{}, + ResourceVersion: "1", + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(cm). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + patchBytes, err := patch.Data(obj) + require.NoError(t, err) + + var patchMap map[string]interface{} + require.NoError(t, json.Unmarshal(patchBytes, &patchMap)) + + metadata := patchMap["metadata"].(map[string]interface{}) + finalizers := metadata["finalizers"] + + if finalizerSlice, ok := finalizers.([]interface{}); ok { + stringFinalizers := make([]string, len(finalizerSlice)) + for i, f := range finalizerSlice { + stringFinalizers[i] = f.(string) + } + obj.SetFinalizers(stringFinalizers) + } + + return nil + }, + }). + Build() + + ctx := context.Background() + + // Add finalizers in unsorted order + unsortedFinalizers := []string{ + finalizer.FinalizerPrefix + "zebra", + finalizer.FinalizerPrefix + "apple", + finalizer.FinalizerPrefix + "banana", + } + + changed, err := finalizer.EnsureFinalizers(ctx, "test", fakeClient, cm, unsortedFinalizers...) + require.NoError(t, err) + assert.True(t, changed) + + // Verify finalizers are sorted + expectedFinalizers := []string{ + finalizer.FinalizerPrefix + "apple", + finalizer.FinalizerPrefix + "banana", + finalizer.FinalizerPrefix + "zebra", + } + assert.Equal(t, expectedFinalizers, cm.GetFinalizers()) +} + +func TestEnsureFinalizers_EmptyInitialFinalizers(t *testing.T) { + tests := []struct { + name string + initialFinalizers []string + newFinalizers []string + wantChanged bool + }{ + { + name: "nil to empty slice", + initialFinalizers: nil, + newFinalizers: []string{}, + wantChanged: false, + }, + { + name: "empty slice to nil", + initialFinalizers: []string{}, + newFinalizers: nil, + wantChanged: false, + }, + { + name: "empty slice to empty slice", + initialFinalizers: []string{}, + newFinalizers: []string{}, + wantChanged: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "test", + Finalizers: tt.initialFinalizers, + ResourceVersion: "1", + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(cm). + Build() + + ctx := context.Background() + changed, err := finalizer.EnsureFinalizers(ctx, "test", fakeClient, cm, tt.newFinalizers...) + + require.NoError(t, err) + assert.Equal(t, tt.wantChanged, changed) + }) + } +} + +func TestEnsureFinalizers_PatchError(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "test", + ResourceVersion: "1", + }, + } + + // Create a client that will fail on patch + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(cm). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + return assert.AnError + }, + }). + Build() + + ctx := context.Background() + changed, err := finalizer.EnsureFinalizers(ctx, "test", fakeClient, cm, finalizer.FinalizerPrefix+"test") + + require.Error(t, err) + assert.False(t, changed) + assert.Contains(t, err.Error(), "updating finalizers") +}