Update the ExternalSecret status even when data is empty (#2927)

https://github.com/external-secrets/external-secrets/issues/2874

Signed-off-by: shuheiktgw <s-kitagawa@mercari.com>
Co-authored-by: Moritz Johner <moolen@users.noreply.github.com>

pkg/controllers/externalsecret/externalsecret_controller.go

@@ -230,21 +230,20 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 				return ctrl.Result{}, err
 			}
 
-			err = r.Delete(ctx, secret)
-			if err != nil && !apierrors.IsNotFound(err) {
+			if err := r.Delete(ctx, secret); err != nil && !apierrors.IsNotFound(err) {
 				r.markAsFailed(log, errDeleteSecret, err, &externalSecret, syncCallsError.With(resourceLabels))
+				return ctrl.Result{}, err
 			}
 
 			conditionSynced := NewExternalSecretCondition(esv1beta1.ExternalSecretReady, v1.ConditionTrue, esv1beta1.ConditionReasonSecretDeleted, "secret deleted due to DeletionPolicy")
 			SetExternalSecretCondition(&externalSecret, *conditionSynced)
-			return ctrl.Result{RequeueAfter: refreshInt}, err
-
-		case esv1beta1.DeletionPolicyMerge:
-			// noop, handled below
-
+			return ctrl.Result{RequeueAfter: refreshInt}, nil
 		// In case provider secrets don't exist the kubernetes secret will be kept as-is.
 		case esv1beta1.DeletionPolicyRetain:
+			r.markAsDone(&externalSecret, start, log)
 			return ctrl.Result{RequeueAfter: refreshInt}, nil
+		// noop, handled below
+		case esv1beta1.DeletionPolicyMerge:
 		}
 	}
 

pkg/controllers/externalsecret/externalsecret_controller_test.go

@@ -22,6 +22,8 @@ import (
 	"strconv"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/prometheus/client_golang/prometheus"
@@ -599,7 +601,6 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 	}
 
 	deleteOrphanedSecrets := func(tc *testCase) {
-
 		tc.checkSecret = func(es *esv1beta1.ExternalSecret, secret *v1.Secret) {
 			cleanEs := es.DeepCopy()
 			oldSecret := v1.Secret{}
@@ -628,6 +629,7 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 			}, time.Second*10, time.Millisecond*200).Should(BeTrue())
 		}
 	}
+
 	ignoreMismatchControllerForGeneratorRef := func(tc *testCase) {
 		const secretKey = "somekey"
 		const secretVal = "someValue"
@@ -957,6 +959,7 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 			Expect(string(secret.Data["map-foo-value-sec"])).To(Equal(BarValue))
 		}
 	}
+
 	syncTemplateFromLiteral := func(tc *testCase) {
 		tplDataVal := "{{ .targetKey }}-literal: {{ .targetValue }}"
 		tplAnnotationsVal := "{{ .targetKey }}-annotations: {{ .targetValue }}"
@@ -1243,7 +1246,7 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 		}
 	}
 
-	deleteSecretPolicy := func(tc *testCase) {
+	deletionPolicyDelete := func(tc *testCase) {
 		expVal := []byte("1234")
 		// set initial value
 		fakeProvider.WithGetAllSecrets(map[string][]byte{
@@ -1291,7 +1294,7 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 		}
 	}
 
-	deleteSecretPolicyRetain := func(tc *testCase) {
+	deletionPolicyRetain := func(tc *testCase) {
 		expVal := []byte("1234")
 		// set initial value
 		fakeProvider.WithGetAllSecrets(map[string][]byte{
@@ -1321,14 +1324,56 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 			Consistently(func() bool {
 				By("checking that secret has not been deleted")
 				err := k8sClient.Get(context.Background(), secretLookupKey, sec)
-				return apierrors.IsNotFound(err) && bytes.Equal(sec.Data["foo"], expVal)
-			}, time.Second*10, time.Second).Should(BeFalse())
+				if err != nil {
+					GinkgoLogr.Error(err, "failed getting a secret")
+					return false
+				}
+				if got := sec.Data["foo"]; !bytes.Equal(got, expVal) {
+					GinkgoLogr.Info("received an unexpected secret value", "got", got, "expected", expVal)
+					return false
+				}
+				return true
+			}, time.Second*10, time.Second).Should(BeTrue())
+		}
+	}
+
+	deletionPolicyRetainEmptyData := func(tc *testCase) {
+		// set initial value
+		fakeProvider.WithGetAllSecrets(make(map[string][]byte), nil)
+		tc.externalSecret.Spec.Data = make([]esv1beta1.ExternalSecretData, 0)
+		tc.externalSecret.Spec.DataFrom = []esv1beta1.ExternalSecretDataFromRemoteRef{
+			{
+				Find: &esv1beta1.ExternalSecretFind{
+					Tags: map[string]string{
+						"non-existing-key": "non-existing-value",
+					},
+				},
+			},
+		}
+		tc.externalSecret.Spec.Target.DeletionPolicy = esv1beta1.DeletionPolicyRetain
+		tc.externalSecret.Spec.RefreshInterval = &metav1.Duration{Duration: time.Second}
+		tc.checkCondition = func(es *esv1beta1.ExternalSecret) bool {
+			expected := []esv1beta1.ExternalSecretStatusCondition{
+				{
+					Type:    esv1beta1.ExternalSecretReady,
+					Status:  v1.ConditionTrue,
+					Reason:  esv1beta1.ConditionReasonSecretSynced,
+					Message: "Secret was synced",
+				},
+			}
+
+			opts := cmpopts.IgnoreFields(esv1beta1.ExternalSecretStatusCondition{}, "LastTransitionTime")
+			if diff := cmp.Diff(expected, es.Status.Conditions, opts); diff != "" {
+				GinkgoLogr.Info("(-got, +want)\n%s", "diff", diff)
+				return false
+			}
+			return true
 		}
 	}
 
 	// merge with existing secret using creationPolicy=Merge
 	// if provider secret gets deleted only the managed field should get deleted
-	deleteSecretPolicyMerge := func(tc *testCase) {
+	deletionPolicyMerge := func(tc *testCase) {
 		const secretVal = "someValue"
 		tc.externalSecret.Spec.RefreshInterval = &metav1.Duration{Duration: time.Second}
 		tc.externalSecret.Spec.Target.CreationPolicy = esv1beta1.CreatePolicyMerge
@@ -1483,6 +1528,7 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 		}
 
 	}
+
 	// with rewrite keys from dataFrom
 	// should error if keys are not compliant
 	invalidFindKeysErrCondition := func(tc *testCase) {
@@ -2210,9 +2256,10 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 		Entry("should set an error condition when store provider constructor fails", storeConstructErrCondition),
 		Entry("should not process store with mismatching controller field", ignoreMismatchController),
 		Entry("should not process cluster secret store when it is disabled", ignoreClusterSecretStoreWhenDisabled),
-		Entry("should eventually delete target secret with deletionPolicy=Delete", deleteSecretPolicy),
-		Entry("should not delete target secret with deletionPolicy=Retain", deleteSecretPolicyRetain),
-		Entry("should not delete pre-existing secret with deletionPolicy=Merge", deleteSecretPolicyMerge),
+		Entry("should eventually delete target secret with deletionPolicy=Delete", deletionPolicyDelete),
+		Entry("should not delete target secret with deletionPolicy=Retain", deletionPolicyRetain),
+		Entry("should update the status properly even if the deletionPolicy is Retain and the data is empty", deletionPolicyRetainEmptyData),
+		Entry("should not delete pre-existing secret with deletionPolicy=Merge", deletionPolicyMerge),
 		Entry("secret is created when there are no conditions for the cluster secret store", useClusterSecretStore, noConditionsSecretCreated),
 		Entry("secret is not created when the condition for the cluster secret store states a different namespace single string condition", useClusterSecretStore, noSecretCreatedWhenNamespaceDoesntMatchStringCondition),
 		Entry("secret is not created when the condition for the cluster secret store states a different namespace single string condition with multiple names", useClusterSecretStore, noSecretCreatedWhenNamespaceDoesntMatchStringConditionWithMultipleNames),