Address review comments

Signed-off-by: chiragkyal <ckyal@redhat.com>

Author: chiragkyal

pkg/controller/external_secrets/certificate_test.go

@@ -268,7 +268,7 @@ func TestCreateOrApplyCertificates(t *testing.T) {
 				esc.Spec.ControllerConfig.CertProvider.CertManager.IssuerRef.Name = testIssuerName
 			},
 			recon:   false,
-			wantErr: fmt.Sprintf("failed to create %s/%s: %s", commontest.TestExternalSecretsNamespace, testValidateCertificateResourceName, commontest.ErrTestClient),
+			wantErr: fmt.Sprintf("failed to create Certificate %s/%s: %s", commontest.TestExternalSecretsNamespace, testValidateCertificateResourceName, commontest.ErrTestClient),
 		},
 		{
 			name: "successful webhook certificate creation",

pkg/controller/external_secrets/configmap.go

@@ -1,15 +1,18 @@
 package external_secrets
 
 import (
+	"encoding/json"
 	"fmt"
 	"maps"
 
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	operatorv1alpha1 "github.com/openshift/external-secrets-operator/api/v1alpha1"
+	operatorclient "github.com/openshift/external-secrets-operator/pkg/controller/client"
 	"github.com/openshift/external-secrets-operator/pkg/controller/common"
 )
 
@@ -59,24 +62,17 @@ func (r *Reconciler) ensureTrustedCABundleConfigMap(esc *operatorv1alpha1.Extern
 
 	if !exist {
 		// NOTE: This ConfigMap cannot use the generic createWithFallback helper because
-		// the Data field is managed by CNO (not by this operator). A plain UpdateWithRetry
-		// with our empty-Data desired object would wipe the CA certificates CNO injected.
-		// Instead, on AlreadyExists we must fetch the real object first, preserve its Data,
-		// then update only labels/annotations.
+		// the Data/BinaryData fields are managed by CNO (not by this operator). On
+		// AlreadyExists we use a MergePatch that touches only metadata, leaving
+		// CNO-injected CA certificates untouched.
 		if err := r.Create(r.ctx, desiredConfigMap); err != nil {
 			if !apierrors.IsAlreadyExists(err) {
 				return common.FromClientError(err, "failed to create %s trusted CA bundle ConfigMap resource", configMapName)
 			}
-			r.log.V(1).Info("trusted CA bundle ConfigMap exists on API server but absent from label-filtered cache, restoring desired state", "name", configMapName)
-			actualConfigMap := &corev1.ConfigMap{}
-			if _, fetchErr := r.UncachedClient.Exists(r.ctx, client.ObjectKeyFromObject(desiredConfigMap), actualConfigMap); fetchErr != nil {
-				return common.FromClientError(fetchErr, "failed to fetch existing %s trusted CA bundle ConfigMap for restoration", configMapName)
-			}
-			desiredConfigMap.Data = actualConfigMap.Data
-			desiredConfigMap.BinaryData = actualConfigMap.BinaryData
+			r.log.V(1).Info("trusted CA bundle ConfigMap exists on API server but absent from label-filtered cache, patching metadata", "name", configMapName)
 			common.RemoveObsoleteAnnotations(desiredConfigMap, resourceMetadata)
-			if updateErr := r.UncachedClient.UpdateWithRetry(r.ctx, desiredConfigMap); updateErr != nil {
-				return common.FromClientError(updateErr, "failed to restore %s trusted CA bundle ConfigMap to desired state", configMapName)
+			if patchErr := r.patchConfigMapMetadata(desiredConfigMap, r.UncachedClient); patchErr != nil {
+				return common.FromClientError(patchErr, "failed to patch %s trusted CA bundle ConfigMap metadata", configMapName)
 			}
 			r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "trusted CA bundle ConfigMap resource %s restored to desired state", configMapName)
 			return nil
@@ -85,17 +81,13 @@ func (r *Reconciler) ensureTrustedCABundleConfigMap(esc *operatorv1alpha1.Extern
 		return nil
 	}
 
-	// ConfigMap exists, ensure it has the correct labels and annotations.
-	// Do not update the data of the ConfigMap since it is managed by CNO.
-	// MergeFetchedAnnotations preserves external annotations (e.g., CNO's openshift.io/owning-component).
+	// ConfigMap exists in cache — ensure it has the correct labels and annotations.
+	// Use a metadata-only patch so CNO-managed Data/BinaryData are never touched.
 	if exist && common.ObjectMetadataModified(desiredConfigMap, existingConfigMap, &resourceMetadata) {
-		r.log.V(1).Info("trusted CA bundle ConfigMap has been modified, updating to desired state", "name", configMapName)
-		// Preserve data from existing ConfigMap (managed by CNO)
-		desiredConfigMap.Data = existingConfigMap.Data
-		desiredConfigMap.BinaryData = existingConfigMap.BinaryData
+		r.log.V(1).Info("trusted CA bundle ConfigMap has been modified, patching metadata to desired state", "name", configMapName)
 		common.RemoveObsoleteAnnotations(desiredConfigMap, resourceMetadata)
-		if err := r.UpdateWithRetry(r.ctx, desiredConfigMap); err != nil {
-			return common.FromClientError(err, "failed to update %s trusted CA bundle ConfigMap resource", configMapName)
+		if err := r.patchConfigMapMetadata(desiredConfigMap, r.CtrlClient); err != nil {
+			return common.FromClientError(err, "failed to patch %s trusted CA bundle ConfigMap metadata", configMapName)
 		}
 		r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "trusted CA bundle ConfigMap resource %s reconciled back to desired state", configMapName)
 	} else {
@@ -105,6 +97,25 @@ func (r *Reconciler) ensureTrustedCABundleConfigMap(esc *operatorv1alpha1.Extern
 	return nil
 }
 
+// patchConfigMapMetadata sends a MergePatch that sets only labels and annotations
+// on the ConfigMap, leaving all other fields (especially Data/BinaryData managed
+// by CNO) untouched. The caller chooses the client: the cached client when the
+// object is visible in the informer cache, or the uncached client when it has
+// fallen out of the label-filtered cache.
+func (r *Reconciler) patchConfigMapMetadata(desired *corev1.ConfigMap, patchClient operatorclient.CtrlClient) error {
+	patch := map[string]interface{}{
+		"metadata": map[string]interface{}{
+			"labels":      desired.Labels,
+			"annotations": desired.Annotations,
+		},
+	}
+	patchBytes, err := json.Marshal(patch)
+	if err != nil {
+		return fmt.Errorf("failed to marshal metadata patch: %w", err)
+	}
+	return patchClient.Patch(r.ctx, desired, client.RawPatch(types.MergePatchType, patchBytes))
+}
+
 // getTrustedCABundleLabels merges resource labels with the injection label.
 func getTrustedCABundleLabels(resourceLabels map[string]string) map[string]string {
 	labels := make(map[string]string)

pkg/controller/external_secrets/configmap_test.go

@@ -36,7 +36,7 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) {
 		preReq           func(r *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient)
 		wantErr          string
 		wantCreate       bool
-		wantUpdate       bool
+		wantPatch        bool
 		noProxy          bool
 	}{
 		{
@@ -79,10 +79,9 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) {
 					return true, nil
 				})
 			},
-			wantUpdate: false,
 		},
 		{
-			name:             "ConfigMap exists with wrong labels, update triggered",
+			name:             "ConfigMap exists with wrong labels, patch triggered",
 			resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
 			preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) {
 				cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) {
@@ -97,73 +96,58 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) {
 					existing.DeepCopyInto(obj.(*corev1.ConfigMap))
 					return true, nil
 				})
-				cached.UpdateWithRetryCalls(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error {
+				cached.PatchCalls(func(_ context.Context, obj client.Object, patch client.Patch, _ ...client.PatchOption) error {
 					cm, ok := obj.(*corev1.ConfigMap)
 					if !ok {
 						t.Errorf("expected ConfigMap, got %T", obj)
 					}
 					if cm.Labels[trustedCABundleInjectLabel] != "true" {
-						t.Errorf("expected inject label restored, got %q", cm.Labels[trustedCABundleInjectLabel])
+						t.Errorf("expected inject label in patch target, got %q", cm.Labels[trustedCABundleInjectLabel])
 					}
-					if cm.Data["ca-bundle.crt"] != "cert-data" {
-						t.Errorf("CNO-managed data should be preserved, got %v", cm.Data)
+					if patch.Type() != types.MergePatchType {
+						t.Errorf("expected MergePatch, got %s", patch.Type())
 					}
 					return nil
 				})
 			},
-			wantUpdate: true,
+			wantPatch: true,
 		},
 		{
 			// Regression test for: labels updating with app: external-secrets of cm
 			// external-secrets-trusted-ca-bundle will block the reconcile in the http_proxy env.
 			//
 			// When the managed label (app=external-secrets) is removed from the ConfigMap,
 			// the object falls out of the label-filtered cache. The cached Exists() returns
-			// false, Create() fails with AlreadyExists, and the controller must use the
-			// uncached client to fetch and restore the correct labels instead of returning
-			// an error that blocks reconciliation.
-			name:             "Create returns AlreadyExists (label-filtered cache miss): uncached client restores labels",
+			// false, Create() fails with AlreadyExists, and the controller must patch only
+			// the metadata (labels/annotations) via the uncached client, leaving
+			// CNO-managed Data/BinaryData untouched.
+			name:             "Create returns AlreadyExists (label-filtered cache miss): uncached client patches metadata",
 			resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
 			preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) {
-				// Cached client doesn't see the ConfigMap because its label was changed.
 				cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) {
 					return false, nil
 				})
 				cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error {
 					return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, trustedCABundleConfigMapName)
 				})
-				// Uncached client fetches the real object from the API server.
-				uncached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) {
-					existing := &corev1.ConfigMap{
-						ObjectMeta: metav1.ObjectMeta{
-							Name:            trustedCABundleConfigMapName,
-							Namespace:       commontest.TestExternalSecretsNamespace,
-							ResourceVersion: "1000",
-							Labels:          map[string]string{"app": "update-secrets"},
-						},
-						Data: cnoData,
-					}
-					existing.DeepCopyInto(obj.(*corev1.ConfigMap))
-					return true, nil
-				})
-				uncached.UpdateWithRetryCalls(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error {
+				uncached.PatchCalls(func(_ context.Context, obj client.Object, patch client.Patch, _ ...client.PatchOption) error {
 					cm, ok := obj.(*corev1.ConfigMap)
 					if !ok {
 						t.Errorf("expected ConfigMap, got %T", obj)
 					}
 					if cm.Labels["app"] != "external-secrets" {
-						t.Errorf("expected app=external-secrets label restored, got %q", cm.Labels["app"])
+						t.Errorf("expected app=external-secrets label in patch target, got %q", cm.Labels["app"])
 					}
 					if cm.Labels[trustedCABundleInjectLabel] != "true" {
-						t.Errorf("expected inject label restored, got %q", cm.Labels[trustedCABundleInjectLabel])
+						t.Errorf("expected inject label in patch target, got %q", cm.Labels[trustedCABundleInjectLabel])
 					}
-					if cm.Data["ca-bundle.crt"] != "cert-data" {
-						t.Errorf("CNO-managed data should be preserved, got %v", cm.Data)
+					if patch.Type() != types.MergePatchType {
+						t.Errorf("expected MergePatch, got %s", patch.Type())
 					}
 					return nil
 				})
 			},
-			wantUpdate: true,
+			wantPatch: true,
 		},
 		{
 			name:             "proxy not configured: ConfigMap reconcile skipped",
@@ -194,23 +178,7 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) {
 			wantErr: "failed to create external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap resource: test client error",
 		},
 		{
-			name:             "uncached Exists fails after AlreadyExists from Create",
-			resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
-			preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) {
-				cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) {
-					return false, nil
-				})
-				cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error {
-					return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, trustedCABundleConfigMapName)
-				})
-				uncached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) {
-					return false, commontest.ErrTestClient
-				})
-			},
-			wantErr: "failed to fetch existing external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap for restoration: test client error",
-		},
-		{
-			name:             "uncached UpdateWithRetry fails after AlreadyExists from Create",
+			name:             "uncached Patch fails after AlreadyExists from Create",
 			resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
 			preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) {
 				cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) {
@@ -219,26 +187,14 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) {
 				cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error {
 					return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, trustedCABundleConfigMapName)
 				})
-				uncached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) {
-					existing := &corev1.ConfigMap{
-						ObjectMeta: metav1.ObjectMeta{
-							Name:            trustedCABundleConfigMapName,
-							Namespace:       commontest.TestExternalSecretsNamespace,
-							ResourceVersion: "1000",
-							Labels:          map[string]string{"app": "update-secrets"},
-						},
-					}
-					existing.DeepCopyInto(obj.(*corev1.ConfigMap))
-					return true, nil
-				})
-				uncached.UpdateWithRetryCalls(func(_ context.Context, _ client.Object, _ ...client.UpdateOption) error {
+				uncached.PatchCalls(func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error {
 					return commontest.ErrTestClient
 				})
 			},
-			wantErr: "failed to restore external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap to desired state: test client error",
+			wantErr: "failed to patch external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap metadata: test client error",
 		},
 		{
-			name:             "cached UpdateWithRetry fails when ConfigMap has wrong labels",
+			name:             "cached Patch fails when ConfigMap has wrong labels",
 			resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
 			preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) {
 				cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) {
@@ -252,11 +208,11 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) {
 					existing.DeepCopyInto(obj.(*corev1.ConfigMap))
 					return true, nil
 				})
-				cached.UpdateWithRetryCalls(func(_ context.Context, _ client.Object, _ ...client.UpdateOption) error {
+				cached.PatchCalls(func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error {
 					return commontest.ErrTestClient
 				})
 			},
-			wantErr: "failed to update external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap resource: test client error",
+			wantErr: "failed to patch external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap metadata: test client error",
 		},
 	}
 
@@ -293,11 +249,11 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) {
 			if tt.wantCreate && cached.CreateCallCount() == 0 {
 				t.Error("expected Create to be called, but it wasn't")
 			}
-			if tt.wantUpdate && cached.UpdateWithRetryCallCount() == 0 && uncached.UpdateWithRetryCallCount() == 0 {
-				t.Error("expected UpdateWithRetry to be called (on cached or uncached client), but it wasn't")
+			if tt.wantPatch && cached.PatchCallCount() == 0 && uncached.PatchCallCount() == 0 {
+				t.Error("expected Patch to be called (on cached or uncached client), but it wasn't")
 			}
-			if !tt.wantUpdate && (cached.UpdateWithRetryCallCount() > 0 || uncached.UpdateWithRetryCallCount() > 0) {
-				t.Error("expected no UpdateWithRetry call, but one was made")
+			if !tt.wantPatch && (cached.PatchCallCount() > 0 || uncached.PatchCallCount() > 0) {
+				t.Error("expected no Patch call, but one was made")
 			}
 		})
 	}

pkg/controller/external_secrets/controller.go

@@ -270,20 +270,40 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
 		return object.GetLabels() != nil && object.GetLabels()[requestEnqueueLabelKey] == requestEnqueueLabelValue
 	}
 
+	logIgnored := func(obj client.Object) {
+		r.log.V(4).Info("object not of interest, ignoring reconcile event", "object", fmt.Sprintf("%T", obj), "name", obj.GetName(), "namespace", obj.GetNamespace())
+	}
+
 	// On updates, check both old and new objects so that events where the managed
 	// label is removed externally still trigger reconciliation and label restoration.
 	managedResources := predicate.Funcs{
 		CreateFunc: func(e event.CreateEvent) bool {
-			return isManagedResource(e.Object)
+			if !isManagedResource(e.Object) {
+				logIgnored(e.Object)
+				return false
+			}
+			return true
 		},
 		UpdateFunc: func(e event.UpdateEvent) bool {
-			return isManagedResource(e.ObjectOld) || isManagedResource(e.ObjectNew)
+			if !isManagedResource(e.ObjectOld) && !isManagedResource(e.ObjectNew) {
+				logIgnored(e.ObjectNew)
+				return false
+			}
+			return true
 		},
 		DeleteFunc: func(e event.DeleteEvent) bool {
-			return isManagedResource(e.Object)
+			if !isManagedResource(e.Object) {
+				logIgnored(e.Object)
+				return false
+			}
+			return true
 		},
 		GenericFunc: func(e event.GenericEvent) bool {
-			return isManagedResource(e.Object)
+			if !isManagedResource(e.Object) {
+				logIgnored(e.Object)
+				return false
+			}
+			return true
 		},
 	}
 

pkg/controller/external_secrets/networkpolicy_test.go

@@ -146,7 +146,7 @@ func TestCreateOrApplyStaticNetworkPolicies(t *testing.T) {
 					return nil
 				})
 			},
-			wantErr: "failed to create external-secrets/eso-sys-deny-all-traffic: test client error",
+			wantErr: "failed to create NetworkPolicy external-secrets/eso-sys-deny-all-traffic: test client error",
 		},
 		{
 			name: "network policy exists check fails",
@@ -344,7 +344,7 @@ func TestCreateOrApplyCustomNetworkPolicies(t *testing.T) {
 					},
 				}
 			},
-			wantErr: "failed to create external-secrets/eso-user-test-fail-policy: test client error",
+			wantErr: "failed to create NetworkPolicy external-secrets/eso-user-test-fail-policy: test client error",
 		},
 		{
 			name: "custom network policy updated successfully",