:sparkles: Implements template MergePolicy. Fixes a few template merging bugs (#2115)

Signed-off-by: Gustavo Carvalho <gusfcarvalho@gmail.com>

apis/externalsecrets/v1beta1/externalsecret_types.go

@@ -95,13 +95,21 @@ type ExternalSecretTemplate struct {
 	EngineVersion TemplateEngineVersion `json:"engineVersion,omitempty"`
 	// +optional
 	Metadata ExternalSecretTemplateMetadata `json:"metadata,omitempty"`
-
+	// +kubebuilder:default="Replace"
+	MergePolicy TemplateMergePolicy `json:"mergePolicy,omitempty"`
 	// +optional
 	Data map[string]string `json:"data,omitempty"`
 	// +optional
 	TemplateFrom []TemplateFrom `json:"templateFrom,omitempty"`
 }
 
+type TemplateMergePolicy string
+
+const (
+	MergePolicyReplace TemplateMergePolicy = "Replace"
+	MergePolicyMerge   TemplateMergePolicy = "Merge"
+)
+
 type TemplateEngineVersion string
 
 const (

config/crds/bases/external-secrets.io_clusterexternalsecrets.yaml

@@ -352,6 +352,9 @@ spec:
                           engineVersion:
                             default: v2
                             type: string
+                          mergePolicy:
+                            default: Replace
+                            type: string
                           metadata:
                             description: ExternalSecretTemplateMetadata defines metadata
                               fields for the Secret blueprint.

config/crds/bases/external-secrets.io_externalsecrets.yaml

@@ -585,6 +585,9 @@ spec:
                       engineVersion:
                         default: v2
                         type: string
+                      mergePolicy:
+                        default: Replace
+                        type: string
                       metadata:
                         description: ExternalSecretTemplateMetadata defines metadata
                           fields for the Secret blueprint.

deploy/crds/bundle.yaml

@@ -293,6 +293,9 @@ spec:
                             engineVersion:
                               default: v2
                               type: string
+                            mergePolicy:
+                              default: Replace
+                              type: string
                             metadata:
                               description: ExternalSecretTemplateMetadata defines metadata fields for the Secret blueprint.
                               properties:
@@ -3550,6 +3553,9 @@ spec:
                         engineVersion:
                           default: v2
                           type: string
+                        mergePolicy:
+                          default: Replace
+                          type: string
                         metadata:
                           description: ExternalSecretTemplateMetadata defines metadata fields for the Secret blueprint.
                           properties:

docs/api/spec.md

@@ -2708,6 +2708,18 @@ ExternalSecretTemplateMetadata
 </tr>
 <tr>
 <td>
+<code>mergePolicy</code></br>
+<em>
+<a href="#external-secrets.io/v1beta1.TemplateMergePolicy">
+TemplateMergePolicy
+</a>
+</em>
+</td>
+<td>
+</td>
+</tr>
+<tr>
+<td>
 <code>data</code></br>
 <em>
 map[string]string
@@ -5091,6 +5103,27 @@ string
 </tr>
 </tbody>
 </table>
+<h3 id="external-secrets.io/v1beta1.TemplateMergePolicy">TemplateMergePolicy
+(<code>string</code> alias)</p></h3>
+<p>
+(<em>Appears on:</em>
+<a href="#external-secrets.io/v1beta1.ExternalSecretTemplate">ExternalSecretTemplate</a>)
+</p>
+<p>
+</p>
+<table>
+<thead>
+<tr>
+<th>Value</th>
+<th>Description</th>
+</tr>
+</thead>
+<tbody><tr><td><p>&#34;Merge&#34;</p></td>
+<td></td>
+</tr><tr><td><p>&#34;Replace&#34;</p></td>
+<td></td>
+</tr></tbody>
+</table>
 <h3 id="external-secrets.io/v1beta1.TemplateRef">TemplateRef
 </h3>
 <p>

docs/snippets/merge-template-v2-external-secret.yaml

@@ -0,0 +1,22 @@
+{% raw %}
+apiVersion: external-secrets.io/v1beta1
+kind: ExternalSecret
+metadata:
+  name: template
+spec:
+  # ...
+  target:
+    template:
+      mergePolicy: Merge
+      engineVersion: v2
+      data:
+        name: admin
+        password: "{{ .password | b64dec }}" # Overwrites the password from the data call and use this output
+  data:
+  - secretKey: password
+    remoteRef:
+      key: /credentials/password
+  - secretKey: username # Preserves the username in the templated Secret
+    remoteRef:
+      key: /credentials/username
+{% endraw %}

pkg/controllers/externalsecret/externalsecret_controller.go

@@ -242,23 +242,24 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		if secret.Data == nil {
 			secret.Data = make(map[string][]byte)
 		}
+		// diff existing keys
+		keys, err := getManagedKeys(&existingSecret, externalSecret.Name)
+		if err != nil {
+			return err
+		}
+		// Sanitize data map for any updates on the ES
+		for _, key := range keys {
+			if dataMap[key] == nil {
+				secret.Data[key] = nil
+				// Sanitizing any templated / updated keys
+				delete(secret.Data, key)
+			}
+		}
 		err = r.applyTemplate(ctx, &externalSecret, secret, dataMap)
 		if err != nil {
 			return fmt.Errorf(errApplyTemplate, err)
 		}
 
-		// diff existing keys
-		if externalSecret.Spec.Target.DeletionPolicy == esv1beta1.DeletionPolicyMerge {
-			keys, err := getManagedKeys(&existingSecret, externalSecret.Name)
-			if err != nil {
-				return err
-			}
-			for _, key := range keys {
-				if dataMap[key] == nil {
-					secret.Data[key] = nil
-				}
-			}
-		}
 		return nil
 	}
 
@@ -270,7 +271,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		log.V(1).Info("secret creation skipped due to creationPolicy=None")
 		err = nil
 	default:
-		_, err = ctrl.CreateOrUpdate(ctx, r.Client, secret, mutationFunc)
+		err = createOrUpdate(ctx, r.Client, secret, mutationFunc, externalSecret.Name)
 	}
 
 	if err != nil {
@@ -299,6 +300,38 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	}, nil
 }
 
+func createOrUpdate(ctx context.Context, c client.Client, obj client.Object, f func() error, fieldOwner string) error {
+	fqdn := fmt.Sprintf(fieldOwnerTemplate, fieldOwner)
+	key := client.ObjectKeyFromObject(obj)
+	if err := c.Get(ctx, key, obj); err != nil {
+		if !apierrors.IsNotFound(err) {
+			return err
+		}
+		if err := f(); err != nil {
+			return err
+		}
+		// Setting Field Owner even for CreationPolicy==Create
+		if err := c.Create(ctx, obj, client.FieldOwner(fqdn)); err != nil {
+			return err
+		}
+		return nil
+	}
+
+	existing := obj.DeepCopyObject()
+	if err := f(); err != nil {
+		return err
+	}
+
+	if equality.Semantic.DeepEqual(existing, obj) {
+		return nil
+	}
+
+	if err := c.Update(ctx, obj, client.FieldOwner(fqdn)); err != nil {
+		return err
+	}
+	return nil
+}
+
 func patchSecret(ctx context.Context, c client.Client, scheme *runtime.Scheme, secret *v1.Secret, mutationFunc func() error, fieldOwner string) error {
 	fqdn := fmt.Sprintf(fieldOwnerTemplate, fieldOwner)
 	err := c.Get(ctx, client.ObjectKeyFromObject(secret), secret.DeepCopy())
@@ -331,7 +364,8 @@ func patchSecret(ctx context.Context, c client.Client, scheme *runtime.Scheme, s
 	if equality.Semantic.DeepEqual(existing, secret) {
 		return nil
 	}
-
+	// Cleaning up Managed fields manually as to keep patch coherence
+	secret.ObjectMeta.ManagedFields = 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(fqdn), client.ForceOwnership)
@@ -357,7 +391,7 @@ func getManagedKeys(secret *v1.Secret, fieldOwner string) ([]string, error) {
 		if dataFields == nil {
 			continue
 		}
-		df, ok := dataFields.(map[string]string)
+		df, ok := dataFields.(map[string]interface{})
 		if !ok {
 			continue
 		}

pkg/controllers/externalsecret/externalsecret_controller_template.go

@@ -155,6 +155,12 @@ func (r *Reconciler) applyTemplate(ctx context.Context, es *esv1beta1.ExternalSe
 		secret.Annotations[esv1beta1.AnnotationDataHash] = utils.ObjectHash(secret.Data)
 		return nil
 	}
+	// Merge Policy should merge secrets
+	if es.Spec.Target.Template.MergePolicy == esv1beta1.MergePolicyMerge {
+		for k, v := range dataMap {
+			secret.Data[k] = v
+		}
+	}
 	execute, err := template.EngineForVersion(es.Spec.Target.Template.EngineVersion)
 	if err != nil {
 		return err

pkg/controllers/externalsecret/externalsecret_controller_test.go

@@ -1654,6 +1654,55 @@ var _ = Describe("ExternalSecret controller", func() {
 		}
 	}
 
+	// When we update the template, remaining keys should not be preserved
+	templateShouldRewrite := func(tc *testCase) {
+		const secretVal = "someValue"
+		fakeProvider.WithGetSecret([]byte(secretVal), nil)
+		tc.externalSecret.Spec.RefreshInterval = &metav1.Duration{Duration: time.Minute * 10}
+		tc.externalSecret.Spec.Target.Template = &esv1beta1.ExternalSecretTemplate{
+			Data: map[string]string{
+				"key": `{{.targetProperty}}-foo`,
+			},
+		}
+		tc.checkSecret = func(es *esv1beta1.ExternalSecret, secret *v1.Secret) {
+			Expect(secret.Data["key"]).To(Equal([]byte("someValue-foo")))
+			newEs := es.DeepCopy()
+			newEs.Spec.Target.Template.Data = map[string]string{
+				"new": "foo",
+			}
+			Expect(k8sClient.Patch(context.Background(), newEs, client.MergeFrom(es))).To(Succeed())
+
+			var refreshedSecret v1.Secret
+			secretLookupKey := types.NamespacedName{
+				Name:      ExternalSecretTargetSecretName,
+				Namespace: ExternalSecretNamespace,
+			}
+			Eventually(func() bool {
+				err := k8sClient.Get(context.Background(), secretLookupKey, &refreshedSecret)
+				if err != nil {
+					return false
+				}
+				_, ok := refreshedSecret.Data["key"]
+				return !ok && bytes.Equal(refreshedSecret.Data["new"], []byte("foo"))
+			}, timeout, interval).Should(BeTrue())
+		}
+	}
+	// When we update the template, remaining keys should not be preserved
+	templateShouldMerge := func(tc *testCase) {
+		const secretVal = "someValue"
+		fakeProvider.WithGetSecret([]byte(secretVal), nil)
+		tc.externalSecret.Spec.RefreshInterval = &metav1.Duration{Duration: time.Minute * 10}
+		tc.externalSecret.Spec.Target.Template = &esv1beta1.ExternalSecretTemplate{
+			MergePolicy: esv1beta1.MergePolicyMerge,
+			Data: map[string]string{
+				"key": `{{.targetProperty}}-foo`,
+			},
+		}
+		tc.checkSecret = func(es *esv1beta1.ExternalSecret, secret *v1.Secret) {
+			Expect(secret.Data["key"]).To(Equal([]byte("someValue-foo")))
+			Expect(string(secret.Data[targetProp])).To(Equal(secretVal))
+		}
+	}
 	useClusterSecretStore := func(tc *testCase) {
 		tc.secretStore = &esv1beta1.ClusterSecretStore{
 			ObjectMeta: metav1.ObjectMeta{
@@ -1917,6 +1966,8 @@ var _ = Describe("ExternalSecret controller", func() {
 		Entry("should sync template with correct value precedence", syncWithTemplatePrecedence),
 		Entry("should sync template from keys and values", syncTemplateFromKeysAndValues),
 		Entry("should sync template from literal", syncTemplateFromLiteral),
+		Entry("should update template if ExternalSecret is updated", templateShouldRewrite),
+		Entry("should keep data with templates if MergePolicy=Merge", templateShouldMerge),
 		Entry("should refresh secret from template", refreshWithTemplate),
 		Entry("should be able to use only metadata from template", onlyMetadataFromTemplate),
 		Entry("should refresh secret value when provider secret changes", refreshSecretValue),

pkg/provider/vault/vault.go

@@ -484,6 +484,9 @@ func (v *client) findSecretsFromTags(ctx context.Context, candidates []string, t
 		}
 		if match {
 			secret, err := v.GetSecret(ctx, esv1beta1.ExternalSecretDataRemoteRef{Key: name})
+			if errors.Is(err, esv1beta1.NoSecretError{}) {
+				continue
+			}
 			if err != nil {
 				return nil, err
 			}
@@ -505,6 +508,9 @@ func (v *client) findSecretsFromName(ctx context.Context, candidates []string, r
 		ok := matcher.MatchName(name)
 		if ok {
 			secret, err := v.GetSecret(ctx, esv1beta1.ExternalSecretDataRemoteRef{Key: name})
+			if errors.Is(err, esv1beta1.NoSecretError{}) {
+				continue
+			}
 			if err != nil {
 				return nil, err
 			}
@@ -867,7 +873,7 @@ func (v *client) readSecret(ctx context.Context, path, version string) (map[stri
 			return nil, errors.New(errDataField)
 		}
 		if dataInt == nil {
-			return nil, nil
+			return nil, esv1beta1.NoSecretError{}
 		}
 		secretData, ok = dataInt.(map[string]interface{})
 		if !ok {