Fix Secret metadata reconciliation to preserve cert-controller-managed TLS data

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

Author: chiragkyal

pkg/controller/external_secrets/secret.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	operatorv1alpha1 "github.com/openshift/external-secrets-operator/api/v1alpha1"
@@ -33,20 +34,32 @@ func (r *Reconciler) createOrApplySecret(esc *operatorv1alpha1.ExternalSecretsCo
 	}
 
 	if exist && common.ObjectMetadataModified(desired, fetched, &resourceMetadata) {
-		r.log.V(1).Info("secret has been modified, updating to desired state", "name", secretName)
-		common.RemoveObsoleteAnnotations(desired, resourceMetadata)
-		if err := r.UpdateWithRetry(r.ctx, desired); err != nil {
-			return common.FromClientError(err, "failed to update %s secret resource", secretName)
+		r.log.V(1).Info("secret has been modified, patching metadata to desired state", "name", secretName)
+		if err := r.patchResourceMetadata(desired, resourceMetadata); err != nil {
+			return common.FromClientError(err, "failed to patch %s secret resource metadata", secretName)
 		}
 		r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "secret resource %s reconciled back to desired state", secretName)
 	} else {
 		r.log.V(4).Info("secret resource already exists and is in expected state", "name", secretName)
 	}
 
 	if !exist {
-		if err := r.createWithFallback(desired, resourceMetadata, secretName, esc); err != nil {
-			return err
+		// NOTE: This Secret cannot use the generic createWithFallback helper because
+		// its Data field is managed by the external-secrets cert-controller, which injects
+		// TLS content at runtime. On AlreadyExists we use a MergePatch that touches only
+		// metadata, leaving cert-controller-managed TLS certificates untouched.
+		if err := r.Create(r.ctx, desired); err != nil {
+			if !apierrors.IsAlreadyExists(err) {
+				return common.FromClientError(err, "failed to create %s secret resource", secretName)
+			}
+			r.log.V(1).Info("secret exists on API server but absent from label-filtered cache, patching metadata", "name", secretName)
+			if patchErr := r.patchResourceMetadata(desired, resourceMetadata); patchErr != nil {
+				return common.FromClientError(patchErr, "failed to patch %s secret resource metadata", secretName)
+			}
+			r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "secret resource %s restored to desired state", secretName)
+			return nil
 		}
+		r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "secret resource %s created", secretName)
 	}
 	return nil
 }

pkg/controller/external_secrets/secret_test.go

@@ -6,6 +6,8 @@ import (
 	"testing"
 
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -87,7 +89,7 @@ func TestCreateOrApplySecret(t *testing.T) {
 			wantErr: fmt.Sprintf("failed to check %s/%s secret resource already exists: %s", commontest.TestExternalSecretsNamespace, testValidateSecretResourceName, commontest.ErrTestClient),
 		},
 		{
-			name: "reconciliation of secret fails while restoring to expected state",
+			name: "reconciliation of secret fails while patching metadata to expected state",
 			preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) {
 				m.GetCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) error {
 					if o, ok := obj.(*corev1.Secret); ok {
@@ -104,14 +106,52 @@ func TestCreateOrApplySecret(t *testing.T) {
 					}
 					return true, nil
 				})
-				m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
+				m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
 					if _, ok := obj.(*corev1.Secret); ok {
 						return commontest.ErrTestClient
 					}
 					return nil
 				})
 			},
-			wantErr: fmt.Sprintf("failed to update %s/%s secret resource: %s", commontest.TestExternalSecretsNamespace, testValidateSecretResourceName, commontest.ErrTestClient),
+			wantErr: fmt.Sprintf("failed to patch %s/%s secret resource metadata: %s", commontest.TestExternalSecretsNamespace, testValidateSecretResourceName, commontest.ErrTestClient),
+		},
+		{
+			// Regression test: when the managed label is removed from the Secret, the object
+			// falls out of the label-filtered cache. cached Exists() returns false, Create()
+			// fails with AlreadyExists, and the controller must patch only the metadata,
+			// leaving cert-controller-injected TLS data untouched.
+			name: "Create returns AlreadyExists (label-filtered cache miss): patches metadata to restore labels",
+			preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) {
+				m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) {
+					return false, nil
+				})
+				m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
+					return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "secrets"}, testValidateSecretResourceName)
+				})
+				m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
+					if _, ok := obj.(*corev1.Secret); ok {
+						if obj.GetLabels()["app"] != "external-secrets" {
+							t.Errorf("expected app=external-secrets label in patch target, got %v", obj.GetLabels())
+						}
+					}
+					return nil
+				})
+			},
+		},
+		{
+			name: "Patch fails after AlreadyExists from Create",
+			preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) {
+				m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) {
+					return false, nil
+				})
+				m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
+					return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "secrets"}, testValidateSecretResourceName)
+				})
+				m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
+					return commontest.ErrTestClient
+				})
+			},
+			wantErr: fmt.Sprintf("failed to patch %s/%s secret resource metadata: %s", commontest.TestExternalSecretsNamespace, testValidateSecretResourceName, commontest.ErrTestClient),
 		},
 		{
 			name: "reconciliation of secret which already exists in expected state",
@@ -155,7 +195,7 @@ func TestCreateOrApplySecret(t *testing.T) {
 					return nil
 				})
 			},
-			wantErr: fmt.Sprintf("failed to create Secret %s/%s: %s", commontest.TestExternalSecretsNamespace, testValidateSecretResourceName, commontest.ErrTestClient),
+			wantErr: fmt.Sprintf("failed to create %s/%s secret resource: %s", commontest.TestExternalSecretsNamespace, testValidateSecretResourceName, commontest.ErrTestClient),
 		},
 		{
 			name: "successful secret creation",