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

Update all unit tests to match new behavior

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"
@@ -124,11 +126,20 @@ func (r *ClusterCatalogReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 			reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating status: %v", err))
 		}
 	}
-
-	reconciledCatsrc.Finalizers = finalizers
+	// finalizers are used in patch below
+	// 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 +426,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"
@@ -150,10 +151,20 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req
 			reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating status: %v", err))
 		}
 	}
-	reconciledExt.Finalizers = finalizers
+	// finalizers are used in the patch below
+	//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 +190,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