Let ManagedField handle metadata (#2705)

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

Signed-off-by: shuheiktgw <s-kitagawa@mercari.com>

pkg/controllers/commontest/common.go

@@ -19,6 +19,7 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -61,11 +62,14 @@ func HasOwnerRef(meta metav1.ObjectMeta, kind, name string) bool {
 	return false
 }
 
-func HasFieldOwnership(meta metav1.ObjectMeta, mgr, rawFields string) bool {
+func HasFieldOwnership(meta metav1.ObjectMeta, mgr, expected string) string {
 	for _, ref := range meta.ManagedFields {
-		if ref.Manager == mgr && string(ref.FieldsV1.Raw) == rawFields {
-			return true
+		if ref.Manager == mgr {
+			if diff := cmp.Diff(string(ref.FieldsV1.Raw), expected); diff != "" {
+				return fmt.Sprintf("(-got, +want)\n%s", diff)
+			}
+			return ""
 		}
 	}
-	return false
+	return fmt.Sprintf("No managed fields managed by %s", mgr)
 }

pkg/controllers/externalsecret/externalsecret_controller.go

@@ -292,7 +292,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return nil
 	}
 
-	switch externalSecret.Spec.Target.CreationPolicy { //nolint
+	switch externalSecret.Spec.Target.CreationPolicy { //nolint:exhaustive
 	case esv1beta1.CreatePolicyMerge:
 		err = patchSecret(ctx, r.Client, r.Scheme, secret, mutationFunc, externalSecret.Name)
 		if err == nil {

pkg/controllers/externalsecret/externalsecret_controller_template.go

@@ -26,7 +26,7 @@ import (
 	// Loading registered providers.
 	_ "github.com/external-secrets/external-secrets/pkg/provider/register"
 	"github.com/external-secrets/external-secrets/pkg/template"
-	utils "github.com/external-secrets/external-secrets/pkg/utils"
+	"github.com/external-secrets/external-secrets/pkg/utils"
 )
 
 type Parser struct {
@@ -147,7 +147,7 @@ func (p *Parser) MergeMap(tplMap map[string]string, target esv1beta1.TemplateTar
 // * template.templateFrom
 // * secret via es.data or es.dataFrom.
 func (r *Reconciler) applyTemplate(ctx context.Context, es *esv1beta1.ExternalSecret, secret *v1.Secret, dataMap map[string][]byte) error {
-	mergeMetadata(secret, es)
+	setMetadata(secret, es)
 
 	// no template: copy data and return
 	if es.Spec.Target.Template == nil {
@@ -187,7 +187,7 @@ func (r *Reconciler) applyTemplate(ctx context.Context, es *esv1beta1.ExternalSe
 	if err != nil {
 		return fmt.Errorf(errExecTpl, err)
 	}
-	// get template data for labels
+	// get template data for annotations
 	err = p.MergeMap(es.Spec.Target.Template.Metadata.Annotations, esv1beta1.TemplateTargetAnnotations)
 	if err != nil {
 		return fmt.Errorf(errExecTpl, err)
@@ -200,15 +200,11 @@ func (r *Reconciler) applyTemplate(ctx context.Context, es *esv1beta1.ExternalSe
 	return nil
 }
 
-// we do not want to force-override the label/annotations
-// and only copy the necessary key/value pairs.
-func mergeMetadata(secret *v1.Secret, externalSecret *esv1beta1.ExternalSecret) {
-	if secret.ObjectMeta.Labels == nil {
-		secret.ObjectMeta.Labels = make(map[string]string)
-	}
-	if secret.ObjectMeta.Annotations == nil {
-		secret.ObjectMeta.Annotations = make(map[string]string)
-	}
+// setMetadata sets Labels and Annotations to the given secret.
+func setMetadata(secret *v1.Secret, externalSecret *esv1beta1.ExternalSecret) {
+	// It is safe to override the metadata since the Server-Side Apply merges those fields if necessary
+	secret.ObjectMeta.Labels = make(map[string]string)
+	secret.ObjectMeta.Annotations = make(map[string]string)
 	if externalSecret.Spec.Target.Template == nil {
 		utils.MergeStringMap(secret.ObjectMeta.Labels, externalSecret.ObjectMeta.Labels)
 		utils.MergeStringMap(secret.ObjectMeta.Annotations, externalSecret.ObjectMeta.Annotations)

pkg/controllers/externalsecret/externalsecret_controller_test.go

@@ -359,12 +359,24 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 	mergeWithSecret := func(tc *testCase) {
 		const secretVal = "someValue"
 		tc.externalSecret.Spec.Target.CreationPolicy = esv1beta1.CreatePolicyMerge
+		tc.externalSecret.Labels = map[string]string{
+			"es-label-key": "es-label-value",
+		}
+		tc.externalSecret.Annotations = map[string]string{
+			"es-annotation-key": "es-annotation-value",
+		}
 
 		// create secret beforehand
 		Expect(k8sClient.Create(context.Background(), &v1.Secret{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      ExternalSecretTargetSecretName,
 				Namespace: ExternalSecretNamespace,
+				Labels: map[string]string{
+					"existing-label-key": "existing-label-value",
+				},
+				Annotations: map[string]string{
+					"existing-annotation-key": "existing-annotation-value",
+				},
 			},
 			Data: map[string][]byte{
 				existingKey: []byte(existingVal),
@@ -377,21 +389,23 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 			Expect(string(secret.Data[existingKey])).To(Equal(existingVal))
 			Expect(string(secret.Data[targetProp])).To(Equal(secretVal))
 
-			// check labels & annotations
-			for k, v := range es.ObjectMeta.Labels {
-				Expect(secret.ObjectMeta.Labels).To(HaveKeyWithValue(k, v))
-			}
-			for k, v := range es.ObjectMeta.Annotations {
-				Expect(secret.ObjectMeta.Annotations).To(HaveKeyWithValue(k, v))
-			}
+			Expect(secret.ObjectMeta.Labels).To(HaveLen(2))
+			Expect(secret.ObjectMeta.Labels).To(HaveKeyWithValue("existing-label-key", "existing-label-value"))
+			Expect(secret.ObjectMeta.Labels).To(HaveKeyWithValue("es-label-key", "es-label-value"))
+
+			Expect(secret.ObjectMeta.Annotations).To(HaveLen(3))
+			Expect(secret.ObjectMeta.Annotations).To(HaveKeyWithValue("existing-annotation-key", "existing-annotation-value"))
+			Expect(secret.ObjectMeta.Annotations).To(HaveKeyWithValue("es-annotation-key", "es-annotation-value"))
+			Expect(secret.ObjectMeta.Annotations).To(HaveKey(esv1beta1.AnnotationDataHash))
+
 			Expect(ctest.HasOwnerRef(secret.ObjectMeta, "ExternalSecret", ExternalSecretFQDN)).To(BeFalse())
 			Expect(secret.ObjectMeta.ManagedFields).To(HaveLen(2))
 			Expect(ctest.HasFieldOwnership(
 				secret.ObjectMeta,
 				ExternalSecretFQDN,
-				fmt.Sprintf("{\"f:data\":{\"f:targetProperty\":{}},\"f:immutable\":{},\"f:metadata\":{\"f:annotations\":{\"f:%s\":{}}}}", esv1beta1.AnnotationDataHash)),
-			).To(BeTrue())
-			Expect(ctest.HasFieldOwnership(secret.ObjectMeta, FakeManager, "{\"f:data\":{\".\":{},\"f:pre-existing-key\":{}},\"f:type\":{}}")).To(BeTrue())
+				fmt.Sprintf(`{"f:data":{"f:targetProperty":{}},"f:immutable":{},"f:metadata":{"f:annotations":{"f:es-annotation-key":{},"f:%s":{}},"f:labels":{"f:es-label-key":{}}}}`, esv1beta1.AnnotationDataHash)),
+			).To(BeEmpty())
+			Expect(ctest.HasFieldOwnership(secret.ObjectMeta, FakeManager, `{"f:data":{".":{},"f:pre-existing-key":{}},"f:metadata":{"f:annotations":{".":{},"f:existing-annotation-key":{}},"f:labels":{".":{},"f:existing-label-key":{}}},"f:type":{}}`)).To(BeEmpty())
 		}
 	}
 
@@ -488,7 +502,7 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 				secret.ObjectMeta,
 				ExternalSecretFQDN,
 				fmt.Sprintf("{\"f:data\":{\"f:targetProperty\":{}},\"f:immutable\":{},\"f:metadata\":{\"f:annotations\":{\"f:%s\":{}}}}", esv1beta1.AnnotationDataHash)),
-			).To(BeTrue())
+			).To(BeEmpty())
 		}
 	}