Stop deleting all the Secret metadata (#2900)

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

pkg/controllers/externalsecret/externalsecret_controller.go

@@ -259,7 +259,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 			secret.Data = make(map[string][]byte)
 		}
 		// diff existing keys
-		keys, err := getManagedKeys(&existingSecret, externalSecret.Name)
+		keys, err := getManagedDataKeys(&existingSecret, externalSecret.Name)
 		if err != nil {
 			return err
 		}
@@ -447,7 +447,29 @@ func patchSecret(ctx context.Context, c client.Client, scheme *runtime.Scheme, s
 	return nil
 }
 
-func getManagedKeys(secret *v1.Secret, fieldOwner string) ([]string, error) {
+func getManagedDataKeys(secret *v1.Secret, fieldOwner string) ([]string, error) {
+	return getManagedFieldKeys(secret, fieldOwner, func(fields map[string]interface{}) []string {
+		dataFields := fields["f:data"]
+		if dataFields == nil {
+			return nil
+		}
+		df, ok := dataFields.(map[string]interface{})
+		if !ok {
+			return nil
+		}
+		var keys []string
+		for k := range df {
+			keys = append(keys, k)
+		}
+		return keys
+	})
+}
+
+func getManagedFieldKeys(
+	secret *v1.Secret,
+	fieldOwner string,
+	process func(fields map[string]interface{}) []string,
+) ([]string, error) {
 	fqdn := fmt.Sprintf(fieldOwnerTemplate, fieldOwner)
 	var keys []string
 	for _, v := range secret.ObjectMeta.ManagedFields {
@@ -459,19 +481,11 @@ func getManagedKeys(secret *v1.Secret, fieldOwner string) ([]string, error) {
 		if err != nil {
 			return nil, fmt.Errorf("error unmarshaling managed fields: %w", err)
 		}
-		dataFields := fields["f:data"]
-		if dataFields == nil {
-			continue
-		}
-		df, ok := dataFields.(map[string]interface{})
-		if !ok {
-			continue
-		}
-		for k := range df {
-			if k == "." {
+		for _, key := range process(fields) {
+			if key == "." {
 				continue
 			}
-			keys = append(keys, strings.TrimPrefix(k, "f:"))
+			keys = append(keys, strings.TrimPrefix(key, "f:"))
 		}
 	}
 	return keys, nil

pkg/controllers/externalsecret/externalsecret_controller_template.go

@@ -147,7 +147,9 @@ 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 {
-	setMetadata(secret, es)
+	if err := setMetadata(secret, es); err != nil {
+		return err
+	}
 
 	// no template: copy data and return
 	if es.Spec.Target.Template == nil {
@@ -201,17 +203,91 @@ func (r *Reconciler) applyTemplate(ctx context.Context, es *esv1beta1.ExternalSe
 }
 
 // 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)
-		return
-	}
-	// if template is defined: use those labels/annotations
-	secret.Type = externalSecret.Spec.Target.Template.Type
-	utils.MergeStringMap(secret.ObjectMeta.Labels, externalSecret.Spec.Target.Template.Metadata.Labels)
-	utils.MergeStringMap(secret.ObjectMeta.Annotations, externalSecret.Spec.Target.Template.Metadata.Annotations)
+func setMetadata(secret *v1.Secret, es *esv1beta1.ExternalSecret) error {
+	if secret.Labels == nil {
+		secret.Labels = make(map[string]string)
+	}
+	if secret.Annotations == nil {
+		secret.Annotations = make(map[string]string)
+	}
+	// Clean up Labels and Annotations added by the operator
+	// so that it won't leave outdated ones
+	labelKeys, err := getManagedLabelKeys(secret, es.Name)
+	if err != nil {
+		return err
+	}
+	for _, key := range labelKeys {
+		delete(secret.ObjectMeta.Labels, key)
+	}
+
+	annotationKeys, err := getManagedAnnotationKeys(secret, es.Name)
+	if err != nil {
+		return err
+	}
+	for _, key := range annotationKeys {
+		delete(secret.ObjectMeta.Annotations, key)
+	}
+
+	if es.Spec.Target.Template == nil {
+		utils.MergeStringMap(secret.ObjectMeta.Labels, es.ObjectMeta.Labels)
+		utils.MergeStringMap(secret.ObjectMeta.Annotations, es.ObjectMeta.Annotations)
+		return nil
+	}
+
+	secret.Type = es.Spec.Target.Template.Type
+	utils.MergeStringMap(secret.ObjectMeta.Labels, es.Spec.Target.Template.Metadata.Labels)
+	utils.MergeStringMap(secret.ObjectMeta.Annotations, es.Spec.Target.Template.Metadata.Annotations)
+	return nil
+}
+
+func getManagedAnnotationKeys(secret *v1.Secret, fieldOwner string) ([]string, error) {
+	return getManagedFieldKeys(secret, fieldOwner, func(fields map[string]interface{}) []string {
+		metadataFields, exists := fields["f:metadata"]
+		if !exists {
+			return nil
+		}
+		mf, ok := metadataFields.(map[string]interface{})
+		if !ok {
+			return nil
+		}
+		annotationFields, exists := mf["f:annotations"]
+		if !exists {
+			return nil
+		}
+		af, ok := annotationFields.(map[string]interface{})
+		if !ok {
+			return nil
+		}
+		var keys []string
+		for k := range af {
+			keys = append(keys, k)
+		}
+		return keys
+	})
+}
+
+func getManagedLabelKeys(secret *v1.Secret, fieldOwner string) ([]string, error) {
+	return getManagedFieldKeys(secret, fieldOwner, func(fields map[string]interface{}) []string {
+		metadataFields, exists := fields["f:metadata"]
+		if !exists {
+			return nil
+		}
+		mf, ok := metadataFields.(map[string]interface{})
+		if !ok {
+			return nil
+		}
+		labelFields, exists := mf["f:labels"]
+		if !exists {
+			return nil
+		}
+		lf, ok := labelFields.(map[string]interface{})
+		if !ok {
+			return nil
+		}
+		var keys []string
+		for k := range lf {
+			keys = append(keys, k)
+		}
+		return keys
+	})
 }

pkg/controllers/externalsecret/externalsecret_controller_test.go

@@ -314,30 +314,85 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 	// labels and annotations from the Kind=ExternalSecret
 	// should be copied over to the Kind=Secret
 	syncLabelsAnnotations := func(tc *testCase) {
-		const secretVal = "someValue"
 		tc.externalSecret.ObjectMeta.Labels = map[string]string{
-			"fooobar": "bazz",
+			"label-key": "label-value",
 		}
 		tc.externalSecret.ObjectMeta.Annotations = map[string]string{
-			"hihihih": "hehehe",
+			"annotation-key": "annotation-value",
 		}
 		fakeProvider.WithGetSecret([]byte(secretVal), nil)
+
 		tc.checkSecret = func(es *esv1beta1.ExternalSecret, secret *v1.Secret) {
-			// check value
-			Expect(string(secret.Data[targetProp])).To(Equal(secretVal))
+			Expect(secret.ObjectMeta.Labels).To(HaveKeyWithValue("label-key", "label-value"))
+			Expect(secret.ObjectMeta.Annotations).To(HaveKeyWithValue("annotation-key", "annotation-value"))
 
-			// 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))
-			}
-			// ownerRef must not not be set!
+			// ownerRef must not be set!
 			Expect(ctest.HasOwnerRef(secret.ObjectMeta, "ExternalSecret", ExternalSecretName)).To(BeTrue())
 		}
 	}
 
+	// labels and annotations from the ExternalSecret
+	// should be merged to the Secret if exists
+	mergeLabelsAnnotations := func(tc *testCase) {
+		tc.externalSecret.ObjectMeta.Labels = map[string]string{
+			"label-key": "label-value",
+		}
+		tc.externalSecret.ObjectMeta.Annotations = map[string]string{
+			"annotation-key": "annotation-value",
+		}
+		fakeProvider.WithGetSecret([]byte(secretVal), nil)
+		// Create a secret owned by another entity to test if the pre-existing metadata is preserved
+		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",
+				},
+			},
+		}, client.FieldOwner(FakeManager))).To(Succeed())
+
+		tc.checkSecret = func(es *esv1beta1.ExternalSecret, secret *v1.Secret) {
+			Expect(secret.ObjectMeta.Labels).To(HaveKeyWithValue("label-key", "label-value"))
+			Expect(secret.ObjectMeta.Labels).To(HaveKeyWithValue("existing-label-key", "existing-label-value"))
+			Expect(secret.ObjectMeta.Annotations).To(HaveKeyWithValue("annotation-key", "annotation-value"))
+			Expect(secret.ObjectMeta.Annotations).To(HaveKeyWithValue("existing-annotation-key", "existing-annotation-value"))
+		}
+	}
+
+	removeOutdatedLabelsAnnotations := func(tc *testCase) {
+		tc.externalSecret.ObjectMeta.Labels = map[string]string{
+			"label-key": "label-value",
+		}
+		tc.externalSecret.ObjectMeta.Annotations = map[string]string{
+			"annotation-key": "annotation-value",
+		}
+		fakeProvider.WithGetSecret([]byte(secretVal), nil)
+		// Create a secret owned by the operator to test if the outdated pre-existing metadata is removed
+		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",
+				},
+			},
+		}, client.FieldOwner(ExternalSecretFQDN))).To(Succeed())
+
+		tc.checkSecret = func(es *esv1beta1.ExternalSecret, secret *v1.Secret) {
+			Expect(secret.ObjectMeta.Labels).To(HaveKeyWithValue("label-key", "label-value"))
+			Expect(secret.ObjectMeta.Labels).NotTo(HaveKeyWithValue("existing-label-key", "existing-label-value"))
+			Expect(secret.ObjectMeta.Annotations).To(HaveKeyWithValue("annotation-key", "annotation-value"))
+			Expect(secret.ObjectMeta.Annotations).NotTo(HaveKeyWithValue("existing-annotation-key", "existing-annotation-value"))
+		}
+	}
+
 	checkPrometheusCounters := func(tc *testCase) {
 		const secretVal = "someValue"
 		fakeProvider.WithGetSecret([]byte(secretVal), nil)
@@ -2118,7 +2173,9 @@ var _ = Describe("ExternalSecret controller", Serial, func() {
 		Entry("should sync to target secrets with naming bigger than 63 characters", syncBigNames),
 		Entry("should expose the secret as a provisioned service binding secret", syncBindingSecret),
 		Entry("should not expose a provisioned service when no secret is synced", skipBindingSecret),
-		Entry("should set the condition eventually", syncLabelsAnnotations),
+		Entry("should set labels and annotations from the ExternalSecret", syncLabelsAnnotations),
+		Entry("should merge labels and annotations to the ones owned by other entity", mergeLabelsAnnotations),
+		Entry("should removed outdated labels and annotations", removeOutdatedLabelsAnnotations),
 		Entry("should set prometheus counters", checkPrometheusCounters),
 		Entry("should merge with existing secret using creationPolicy=Merge", mergeWithSecret),
 		Entry("should error if secret doesn't exist when using creationPolicy=Merge", mergeWithSecretErr),