Merge pull request #617 from marcingy/issue-221

Optimise patching so as changes only happen with something changes

pkg/controllers/externalsecret/externalsecret_controller.go

@@ -22,6 +22,7 @@ import (
 	"github.com/go-logr/logr"
 	"github.com/prometheus/client_golang/prometheus"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -263,10 +264,13 @@ func patchSecret(ctx context.Context, c client.Client, scheme *runtime.Scheme, s
 	if err != nil {
 		return fmt.Errorf(errPolicyMergeGetSecret, secret.Name, err)
 	}
+	existing := secret.DeepCopyObject()
+
 	err = mutationFunc()
 	if err != nil {
 		return fmt.Errorf(errPolicyMergeMutate, secret.Name, err)
 	}
+
 	// GVK is missing in the Secret, see:
 	// https://github.com/kubernetes-sigs/controller-runtime/issues/526
 	// https://github.com/kubernetes-sigs/controller-runtime/issues/1517
@@ -279,6 +283,11 @@ func patchSecret(ctx context.Context, c client.Client, scheme *runtime.Scheme, s
 	if !unversioned && len(gvks) == 1 {
 		secret.SetGroupVersionKind(gvks[0])
 	}
+
+	if equality.Semantic.DeepEqual(existing, secret) {
+		return nil
+	}
+
 	// we're not able to resolve conflicts so we force ownership
 	// see: https://kubernetes.io/docs/reference/using-api/server-side-apply/#using-server-side-apply-in-a-controller
 	err = c.Patch(ctx, secret, client.Apply, client.FieldOwner("external-secrets"), client.ForceOwnership)

pkg/controllers/externalsecret/externalsecret_controller_test.go

@@ -318,6 +318,47 @@ var _ = Describe("ExternalSecret controller", func() {
 		}
 	}
 
+	// should not update if no changes
+	mergeWithSecretNoChange := func(tc *testCase) {
+		const existingKey = "pre-existing-key"
+		existingVal := "someValue"
+		tc.externalSecret.Spec.Target.CreationPolicy = esv1alpha1.Merge
+
+		// 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())
+
+		tc.checkSecret = func(es *esv1alpha1.ExternalSecret, secret *v1.Secret) {
+			oldResourceVersion := secret.ResourceVersion
+
+			cleanSecret := secret.DeepCopy()
+			Expect(k8sClient.Patch(context.Background(), secret, client.MergeFrom(cleanSecret))).To(Succeed())
+
+			newSecret := &v1.Secret{}
+
+			Eventually(func() bool {
+				secretLookupKey := types.NamespacedName{
+					Name:      ExternalSecretTargetSecretName,
+					Namespace: ExternalSecretNamespace,
+				}
+
+				err := k8sClient.Get(context.Background(), secretLookupKey, newSecret)
+				if err != nil {
+					return false
+				}
+				return oldResourceVersion == newSecret.ResourceVersion
+			}, timeout, interval).Should(Equal(true))
+
+		}
+	}
+
 	// should not merge with secret if it doesn't exist
 	mergeWithSecretErr := func(tc *testCase) {
 		const secretVal = "someValue"
@@ -925,6 +966,7 @@ var _ = Describe("ExternalSecret controller", func() {
 		Entry("should merge with existing secret using creationPolicy=Merge", mergeWithSecret),
 		Entry("should error if secret doesn't exist when using creationPolicy=Merge", mergeWithSecretErr),
 		Entry("should not resolve conflicts with creationPolicy=Merge", mergeWithConflict),
+		Entry("should not update unchanged secret using creationPolicy=Merge", mergeWithSecretNoChange),
 		Entry("should sync with template", syncWithTemplate),
 		Entry("should sync template with correct value precedence", syncWithTemplatePrecedence),
 		Entry("should refresh secret from template", refreshWithTemplate),