feat(fake): deprecate ValueMap to use Value instead (external-secrets…

…#2884)
cloudant · Dec 1, 2023 · 3599384 · 3599384
1 parent 2441ad5
commit 3599384
Show file tree

Hide file tree

Showing 12 changed files with 135 additions and 16 deletions.
diff --git a/.golangci.yaml b/.golangci.yaml
@@ -113,6 +113,12 @@ issues:
       linters:
         - goheader
 
+    # excluding deprecation check introduced on purpose in #2884
+    - path: pkg/provider/fake/fake.go
+      text: 'SA1019: data.ValueMap is deprecated: ValueMap is deprecated and is intended to be removed in the future, use the `value` field instead.'
+    - path: pkg/provider/fake/fake_test.go
+      text: 'SA1019: data.ValueMap is deprecated: ValueMap is deprecated and is intended to be removed in the future, use the `value` field instead.'
+
   # Maximum issues count per one linter. Set to 0 to disable. Default is 50.
   max-per-linter: 0
 

diff --git a/apis/externalsecrets/v1beta1/secretstore_fake_types.go b/apis/externalsecrets/v1beta1/secretstore_fake_types.go
@@ -20,8 +20,9 @@ type FakeProvider struct {
 }
 
 type FakeProviderData struct {
-	Key      string            `json:"key"`
-	Value    string            `json:"value,omitempty"`
+	Key   string `json:"key"`
+	Value string `json:"value,omitempty"`
+	// Deprecated: ValueMap is deprecated and is intended to be removed in the future, use the `value` field instead.
 	ValueMap map[string]string `json:"valueMap,omitempty"`
 	Version  string            `json:"version,omitempty"`
 }
diff --git a/config/crds/bases/external-secrets.io_clustersecretstores.yaml b/config/crds/bases/external-secrets.io_clustersecretstores.yaml
@@ -2571,6 +2571,9 @@ spec:
                             valueMap:
                               additionalProperties:
                                 type: string
+                              description: 'Deprecated: ValueMap is deprecated and
+                                is intended to be removed in the future, use the `value`
+                                field instead.'
                               type: object
                             version:
                               type: string

diff --git a/config/crds/bases/external-secrets.io_secretstores.yaml b/config/crds/bases/external-secrets.io_secretstores.yaml
@@ -2571,6 +2571,9 @@ spec:
                             valueMap:
                               additionalProperties:
                                 type: string
+                              description: 'Deprecated: ValueMap is deprecated and
+                                is intended to be removed in the future, use the `value`
+                                field instead.'
                               type: object
                             version:
                               type: string

diff --git a/deploy/crds/bundle.yaml b/deploy/crds/bundle.yaml
@@ -2402,6 +2402,7 @@ spec:
                               valueMap:
                                 additionalProperties:
                                   type: string
+                                description: 'Deprecated: ValueMap is deprecated and is intended to be removed in the future, use the `value` field instead.'
                                 type: object
                               version:
                                 type: string
@@ -6344,6 +6345,7 @@ spec:
                               valueMap:
                                 additionalProperties:
                                   type: string
+                                description: 'Deprecated: ValueMap is deprecated and is intended to be removed in the future, use the `value` field instead.'
                                 type: object
                               version:
                                 type: string

diff --git a/docs/api/spec.md b/docs/api/spec.md
@@ -3496,6 +3496,7 @@ map[string]string
 </em>
 </td>
 <td>
+<p>Deprecated: ValueMap is deprecated and is intended to be removed in the future, use the <code>value</code> field instead.</p>
 </td>
 </tr>
 <tr>

diff --git a/docs/provider/fake.md b/docs/provider/fake.md
@@ -2,13 +2,14 @@ We provide a `fake` implementation to help with testing. This provider returns s
 To use the `fake` provider simply create a `SecretStore` or `ClusterSecretStore` and configure it like in the following example:
 
 !!! note inline end
-    The provider returns static data configured in `value` or `valueMap`. You can define a `version`, too. If set the `remoteRef` from an ExternalSecret must match otherwise no value is returned.
+    The provider returns static data configured in `value`. You can define a `version`, too. If set the `remoteRef` from an ExternalSecret must match otherwise no value is returned.
 
 ```yaml
 {% include 'fake-provider-store.yaml' %}
 ```
 
-Please note that `value` is intended for exclusive use with `data` and `valueMap` for `dataFrom`.
+Please note that `value` is intended for exclusive use with `data` for `dataFrom`. You can use the `data` to set a `JSON` compliant value to be used as `dataFrom`.
+
 Here is an example `ExternalSecret` that displays this behavior:
 
 !!! warning inline end

diff --git a/docs/snippets/fake-provider-es.yaml b/docs/snippets/fake-provider-es.yaml
@@ -17,3 +17,4 @@ spec:
   dataFrom:
   - extract:
       key: /foo/baz
+      version: v1
diff --git a/docs/snippets/fake-provider-secret.yaml b/docs/snippets/fake-provider-secret.yaml
@@ -5,5 +5,4 @@ metadata:
   namespace: default
 data:
   foo_bar: SEVMTE8x # HELLO1  (via data)
-  foo: ZXhhbXBsZQ== # example (via dataFrom)
-  other: dGhpbmc=   # thing   (via dataFrom)
+  john: ZG9l #doe (via dataFrom)
diff --git a/docs/snippets/fake-provider-store.yaml b/docs/snippets/fake-provider-store.yaml
@@ -13,8 +13,5 @@ spec:
         value: "HELLO2"
         version: "v2"
       - key: "/foo/baz"
-        valueMap:
-          foo: example
-          other: thing
-
-
+        value: '{"john": "doe"}'
+        version: "v1"
diff --git a/pkg/provider/fake/fake.go b/pkg/provider/fake/fake.go
@@ -16,6 +16,7 @@ package fake
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"strings"
 
@@ -182,12 +183,40 @@ func (p *Provider) GetSecret(_ context.Context, ref esv1beta1.ExternalSecretData
 }
 
 // GetSecretMap returns multiple k/v pairs from the provider.
-func (p *Provider) GetSecretMap(_ context.Context, ref esv1beta1.ExternalSecretDataRemoteRef) (map[string][]byte, error) {
-	data, ok := p.config[mapKey(ref.Key, ref.Version)]
-	if !ok || data.Version != ref.Version || data.ValueMap == nil {
+func (p *Provider) GetSecretMap(ctx context.Context, ref esv1beta1.ExternalSecretDataRemoteRef) (map[string][]byte, error) {
+	ddata, ok := p.config[mapKey(ref.Key, ref.Version)]
+	if !ok || ddata.Version != ref.Version {
 		return nil, esv1beta1.NoSecretErr
 	}
-	return convertMap(data.ValueMap), nil
+
+	// Due to backward compatibility valueMap will still be returned for now
+	if ddata.ValueMap != nil {
+		return convertMap(ddata.ValueMap), nil
+	}
+
+	data, err := p.GetSecret(ctx, ref)
+	if err != nil {
+		return nil, err
+	}
+
+	secretData := make(map[string][]byte)
+	kv := make(map[string]json.RawMessage)
+	err = json.Unmarshal(data, &kv)
+	if err != nil {
+		return nil, fmt.Errorf("unable to unmarshal secret: %w", err)
+	}
+
+	for k, v := range kv {
+		var strVal string
+		err = json.Unmarshal(v, &strVal)
+		if err == nil {
+			secretData[k] = []byte(strVal)
+		} else {
+			secretData[k] = v
+		}
+	}
+
+	return secretData, nil
 }
 
 func convertMap(in map[string]string) map[string][]byte {

diff --git a/pkg/provider/fake/fake_test.go b/pkg/provider/fake/fake_test.go
@@ -413,9 +413,85 @@ func TestGetSecretMap(t *testing.T) {
 			},
 			expErr: esv1beta1.NoSecretErr.Error(),
 		},
+		{
+			name: "get correct map from multiple versions by using Value only",
+			input: []esv1beta1.FakeProviderData{
+				{
+					Key:     "/bar",
+					Version: "v1",
+					Value:   `{"john":"doe"}`,
+				},
+			},
+			request: esv1beta1.ExternalSecretDataRemoteRef{
+				Key:     "/bar",
+				Version: "v1",
+			},
+			expValue: map[string][]byte{
+				"john": []byte("doe"),
+			},
+		},
+		{
+			name: "get correct maps from multiple versions by using Value only",
+			input: []esv1beta1.FakeProviderData{
+				{
+					Key:     "/bar",
+					Version: "v3",
+					Value:   `{"john":"doe", "foo": "bar"}`,
+				},
+			},
+			request: esv1beta1.ExternalSecretDataRemoteRef{
+				Key:     "/bar",
+				Version: "v3",
+			},
+			expValue: map[string][]byte{
+				"john": []byte("doe"),
+				"foo":  []byte("bar"),
+			},
+		},
+		{
+			name: "invalid marshal",
+			input: []esv1beta1.FakeProviderData{
+				{
+					Key:     "/bar",
+					Version: "v3",
+					Value:   `---------`,
+				},
+			},
+			request: esv1beta1.ExternalSecretDataRemoteRef{
+				Key:     "/bar",
+				Version: "v3",
+			},
+			expErr: "unable to unmarshal secret: invalid character '-' in numeric literal",
+		},
+		{
+			name: "get correct value from ValueMap due to retrocompatibility",
+			input: []esv1beta1.FakeProviderData{
+				{
+					Key:     "/foo/bar",
+					Version: "v3",
+					ValueMap: map[string]string{
+						"john": "doe",
+						"baz":  "bang",
+					},
+				},
+			},
+			request: esv1beta1.ExternalSecretDataRemoteRef{
+				Key:     "/foo/bar",
+				Version: "v3",
+			},
+			expValue: map[string][]byte{
+				"john": []byte("doe"),
+				"baz":  []byte("bang"),
+			},
+		},
 		{
 			name: "get correct value from multiple versions",
 			input: []esv1beta1.FakeProviderData{
+				{
+					Key:     "john",
+					Value:   "doe",
+					Version: "v2",
+				},
 				{
 					Key: "junk",
 					ValueMap: map[string]string{
@@ -467,7 +543,7 @@ func TestGetSecretMap(t *testing.T) {
 			gomega.Expect(err).ToNot(gomega.HaveOccurred())
 			out, err := cl.GetSecretMap(context.Background(), row.request)
 			if row.expErr != "" {
-				gomega.Expect(err).To(gomega.MatchError(row.expErr))
+				gomega.Expect(err).To(gomega.MatchError(gomega.ContainSubstring(row.expErr)))
 			} else {
 				gomega.Expect(err).ToNot(gomega.HaveOccurred())
 			}