fix(externalsecret): infinite reconcile loop with Merge secret (#2525)

* fix(externalsecret): infinite reconcile loop with Merge secret

Signed-off-by: Alexandre Gaudreault <alexandre.gaudreault@logmein.com>

* code review

Signed-off-by: Alexandre Gaudreault <alexandre.gaudreault@logmein.com>

* lint

Signed-off-by: Alexandre Gaudreault <alexandre.gaudreault@logmein.com>

* add unit tests

Signed-off-by: Alexandre Gaudreault <alexandre.gaudreault@logmein.com>

* lint

Signed-off-by: Alexandre Gaudreault <alexandre.gaudreault@logmein.com>

* Use objectHash instead of value

Signed-off-by: Alexandre Gaudreault <alexandre.gaudreault@logmein.com>

---------

Signed-off-by: Alexandre Gaudreault <alexandre.gaudreault@logmein.com>

ADOPTERS.md

@@ -7,6 +7,7 @@
 - [DaangnPay](https://www.daangnpay.com/)
 - [Epidemic Sound](https://www.epidemicsound.com/)
 - [Form3](https://www.form3.tech/)
+- [GoTo](https://www.goto.com/)
 - [Heureka Group](https://heureka.group)
 - [K8S Website Infra](https://k8s.io/)
 - [Mercedes-Benz Tech Innovation](https://www.mercedes-benz-techinnovation.com/)

docs/contributing/devguide.md

@@ -77,7 +77,7 @@ export IMAGE=$(make docker.imagename)
 make docker.build
 
 # Load docker image into local kind cluster
-kind load docker-image $IMAGE:$TAG -n external-secrets
+kind load docker-image $IMAGE:$TAG --name external-secrets
 
 # (Optional) Pull the image from GitHub Repo to copy into kind
 # docker pull ghcr.io/external-secrets/external-secrets:v0.8.2

pkg/controllers/externalsecret/externalsecret_controller.go

@@ -281,6 +281,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 			secret.Labels[esv1beta1.LabelOwner] = lblValue
 		}
 
+		secret.Annotations[esv1beta1.AnnotationDataHash] = r.computeDataHashAnnotation(&existingSecret, secret)
+
 		return nil
 	}
 
@@ -599,6 +601,18 @@ func isSecretValid(existingSecret v1.Secret) bool {
 	return true
 }
 
+// computeDataHashAnnotation generate a hash of the secret data combining the old key with the new keys to add or override.
+func (r *Reconciler) computeDataHashAnnotation(existing, secret *v1.Secret) string {
+	data := make(map[string][]byte)
+	for k, v := range existing.Data {
+		data[k] = v
+	}
+	for k, v := range secret.Data {
+		data[k] = v
+	}
+	return utils.ObjectHash(data)
+}
+
 // SetupWithManager returns a new controller builder that will be started by the provided Manager.
 func (r *Reconciler) SetupWithManager(mgr ctrl.Manager, opts controller.Options) error {
 	r.recorder = mgr.GetEventRecorderFor("external-secrets")

pkg/controllers/externalsecret/externalsecret_controller_template.go

@@ -152,7 +152,6 @@ func (r *Reconciler) applyTemplate(ctx context.Context, es *esv1beta1.ExternalSe
 	// no template: copy data and return
 	if es.Spec.Target.Template == nil {
 		secret.Data = dataMap
-		secret.Annotations[esv1beta1.AnnotationDataHash] = utils.ObjectHash(secret.Data)
 		return nil
 	}
 	// Merge Policy should merge secrets
@@ -198,7 +197,6 @@ func (r *Reconciler) applyTemplate(ctx context.Context, es *esv1beta1.ExternalSe
 	if len(es.Spec.Target.Template.Data) == 0 && len(es.Spec.Target.Template.TemplateFrom) == 0 {
 		secret.Data = dataMap
 	}
-	secret.Annotations[esv1beta1.AnnotationDataHash] = utils.ObjectHash(secret.Data)
 	return nil
 }
 

pkg/controllers/externalsecret/externalsecret_controller_test.go

@@ -39,6 +39,7 @@ import (
 	"github.com/external-secrets/external-secrets/pkg/controllers/externalsecret/esmetrics"
 	ctrlmetrics "github.com/external-secrets/external-secrets/pkg/controllers/metrics"
 	"github.com/external-secrets/external-secrets/pkg/provider/testing/fake"
+	"github.com/external-secrets/external-secrets/pkg/utils"
 )
 
 var (
@@ -77,6 +78,10 @@ type testCase struct {
 type testTweaks func(*testCase)
 
 var _ = Describe("Kind=secret existence logic", func() {
+	validData := map[string][]byte{
+		"foo": []byte("value1"),
+		"bar": []byte("value2"),
+	}
 	type testCase struct {
 		Name           string
 		Input          v1.Secret
@@ -126,13 +131,10 @@ var _ = Describe("Kind=secret existence logic", func() {
 				ObjectMeta: metav1.ObjectMeta{
 					UID: "xxx",
 					Annotations: map[string]string{
-						esv1beta1.AnnotationDataHash: "caa0155759a6a9b3b6ada5a6883ee2bb",
+						esv1beta1.AnnotationDataHash: utils.ObjectHash(validData),
 					},
 				},
-				Data: map[string][]byte{
-					"foo": []byte("value1"),
-					"bar": []byte("value2"),
-				},
+				Data: validData,
 			},
 			ExpectedOutput: true,
 		},
@@ -210,10 +212,14 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 		},
 	)
 
-	const secretVal = "some-value"
-	const targetProp = "targetProperty"
-	const remoteKey = "barz"
-	const remoteProperty = "bang"
+	const (
+		secretVal      = "some-value"
+		targetProp     = "targetProperty"
+		remoteKey      = "barz"
+		remoteProperty = "bang"
+		existingKey    = "pre-existing-key"
+		existingVal    = "pre-existing-value"
+	)
 
 	makeDefaultTestcase := func() *testCase {
 		return &testCase{
@@ -352,8 +358,6 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 	// metadata.managedFields with the correct owner should be added to the secret
 	mergeWithSecret := func(tc *testCase) {
 		const secretVal = "someValue"
-		const existingKey = "pre-existing-key"
-		existingVal := "pre-existing-value"
 		tc.externalSecret.Spec.Target.CreationPolicy = esv1beta1.CreatePolicyMerge
 
 		// create secret beforehand
@@ -393,8 +397,6 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 
 	// should not update if no changes
 	mergeWithSecretNoChange := func(tc *testCase) {
-		const existingKey = "pre-existing-key"
-		existingVal := "someValue"
 		tc.externalSecret.Spec.Target.CreationPolicy = esv1beta1.CreatePolicyMerge
 
 		// create secret beforehand
@@ -461,7 +463,6 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 		const secretVal = "someValue"
 		// this should confict
 		const existingKey = targetProp
-		existingVal := "pre-existing-value"
 		tc.externalSecret.Spec.Target.CreationPolicy = esv1beta1.CreatePolicyMerge
 
 		// create secret beforehand
@@ -1260,8 +1261,6 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 	// if provider secret gets deleted only the managed field should get deleted
 	deleteSecretPolicyMerge := func(tc *testCase) {
 		const secretVal = "someValue"
-		const existingKey = "some-existing-key"
-		existingVal := "some-existing-value"
 		tc.externalSecret.Spec.RefreshInterval = &metav1.Duration{Duration: time.Second}
 		tc.externalSecret.Spec.Target.CreationPolicy = esv1beta1.CreatePolicyMerge
 		tc.externalSecret.Spec.Target.DeletionPolicy = esv1beta1.DeletionPolicyMerge
@@ -1736,7 +1735,36 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 		const secretVal = "someValue"
 		fakeProvider.WithGetSecret([]byte(secretVal), nil)
 		tc.checkSecret = func(es *esv1beta1.ExternalSecret, secret *v1.Secret) {
-			Expect(secret.Annotations[esv1beta1.AnnotationDataHash]).To(Equal("9d30b95ca81e156f9454b5ef3bfcc6ee"))
+			expectedHash := utils.ObjectHash(map[string][]byte{
+				targetProp: []byte(secretVal),
+			})
+			Expect(secret.Annotations[esv1beta1.AnnotationDataHash]).To(Equal(expectedHash))
+		}
+	}
+
+	// Checks that secret annotation has been written based on the all the data for merge keys
+	checkMergeSecretDataHashAnnotation := func(tc *testCase) {
+		const secretVal = "someValue"
+		tc.externalSecret.Spec.Target.CreationPolicy = esv1beta1.CreatePolicyMerge
+
+		// create secret beforehand
+		Expect(k8sClient.Create(context.Background(), &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      ExternalSecretTargetSecretName,
+				Namespace: ExternalSecretNamespace,
+			},
+			Data: map[string][]byte{
+				existingKey: []byte(existingVal),
+			},
+		}, client.FieldOwner(FakeManager))).To(Succeed())
+
+		fakeProvider.WithGetSecret([]byte(secretVal), nil)
+		tc.checkSecret = func(es *esv1beta1.ExternalSecret, secret *v1.Secret) {
+			expectedHash := utils.ObjectHash(map[string][]byte{
+				existingKey: []byte(existingVal),
+				targetProp:  []byte(secretVal),
+			})
+			Expect(secret.Annotations[esv1beta1.AnnotationDataHash]).To(Equal(expectedHash))
 		}
 	}
 
@@ -2069,6 +2097,7 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 		},
 		Entry("should recreate deleted secret", checkDeletion),
 		Entry("should create proper hash annotation for the external secret", checkSecretDataHashAnnotation),
+		Entry("should create proper hash annotation for the external secret with creationPolicy=Merge", checkMergeSecretDataHashAnnotation),
 		Entry("es deletes orphaned secrets", deleteOrphanedSecrets),
 		Entry("should refresh when the hash annotation doesn't correspond to secret data", checkSecretDataHashAnnotationChange),
 		Entry("should use external secret name if target secret name isn't defined", syncWithoutTargetName),