fixup! Use Patch instead of Update for finalizer operations

Signed-off-by: Todd Short <[email protected]>

Author: tmshort

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,16 +174,16 @@ 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
 		updateStatusNotServing(&catalog.Status, catalog.GetGeneration())
 		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)
 }

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

internal/operator-controller/controllers/clusterextensionrevision_controller.go

@@ -37,6 +37,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,
@@ -140,7 +141,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,
@@ -375,7 +376,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

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