Use Patch instead of Update for finalizer operations

Refactor all controllers to use Patch() instead of Update()
when adding or removing finalizers to improve performance, and to avoid
removing non-cached fields erroneously.

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

Update ClusterCatalog and ClusterExtension controllers to use Patch-based
funalizer management (ClusterExtensionRevision already uses Patch())

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

Author: tmshort

internal/catalogd/controllers/core/clustercatalog_controller.go

@@ -18,6 +18,7 @@ package core
 
 import (
 	"context" // #nosec
+	"encoding/json"
 	"errors"
 	"fmt"
 	"slices"
@@ -28,6 +29,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/utils/ptr"
@@ -115,8 +117,8 @@ func (r *ClusterCatalogReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 
 	// 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.
+	// does not contain the finalizers. After the status update, we will use the saved finalizers in the
+	// Patch()
 	finalizers := reconciledCatsrc.Finalizers
 
 	if updateStatus {
@@ -125,10 +127,17 @@ func (r *ClusterCatalogReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		}
 	}
 
-	reconciledCatsrc.Finalizers = finalizers
-
 	if updateFinalizers {
-		if err := r.Update(ctx, reconciledCatsrc); err != nil {
+		// Use patch to update finalizers on the server
+		patch := map[string]any{
+			"metadata": map[string]any{
+				"finalizers": finalizers,
+			},
+		}
+		patchJSON, err := json.Marshal(patch)
+		if err != nil {
+			reconcileErr = errors.Join(reconcileErr, fmt.Errorf("marshalling patch to ensure finalizers: %w", err))
+		} else if err := r.Patch(ctx, reconciledCatsrc, client.RawPatch(types.MergePatchType, patchJSON)); err != nil {
 			reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating finalizers: %v", err))
 		}
 	}
@@ -415,11 +424,15 @@ func (r *ClusterCatalogReconciler) needsPoll(lastSuccessfulPoll time.Time, catal
 	return nextPoll.Before(time.Now())
 }
 
-// Compare resources - ignoring status & metadata.finalizers
+// Compare resources - annotations, labels and spec
 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)
+	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)
 }
 
 type finalizerFunc func(ctx context.Context, obj client.Object) (crfinalizer.Result, error)

internal/operator-controller/controllers/clusterextension_controller.go

@@ -18,6 +18,7 @@ package controllers
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io/fs"
@@ -142,18 +143,26 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req
 
 	// 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.
+	// does not contain the finalizers. After the status update, we will use the saved finalizers in the
+	// Patch()
 	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 {
+		// Use patch to update finalizers on the server
+		patch := map[string]any{
+			"metadata": map[string]any{
+				"finalizers": finalizers,
+			},
+		}
+		patchJSON, err := json.Marshal(patch)
+		if err != nil {
+			reconcileErr = errors.Join(reconcileErr, fmt.Errorf("marshalling patch to ensure finalizers: %w", err))
+		} else if err := r.Patch(ctx, reconciledExt, client.RawPatch(types.MergePatchType, patchJSON)); err != nil {
 			reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating finalizers: %v", err))
 		}
 	}
@@ -179,11 +188,15 @@ func ensureAllConditionsWithReason(ext *ocv1.ClusterExtension, reason v1alpha1.C
 	}
 }
 
-// Compare resources - ignoring status & metadata.finalizers
+// Compare resources - annotations, labels and spec
 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)
+	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)
 }
 
 // SetDeprecationStatus will set the appropriate deprecation statuses for a ClusterExtension