Fix the k8s double encoding problem (#2760)

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

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

pkg/provider/kubernetes/client.go

@@ -20,14 +20,13 @@ import (
 	"encoding/json"
 	"fmt"
 	"strings"
-	"unicode/utf8"
 
 	"github.com/tidwall/gjson"
 	v1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	labels "k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/labels"
 
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
 	"github.com/external-secrets/external-secrets/pkg/constants"
@@ -46,64 +45,33 @@ func (c *Client) GetSecret(ctx context.Context, ref esv1beta1.ExternalSecretData
 	if err != nil {
 		return nil, err
 	}
-	serializedSecret, err := serializeSecret(secret, ref)
-	if err != nil {
-		return nil, err
-	}
 
 	// if property is not defined, we will return the json-serialized secret
 	if ref.Property == "" {
-		return serializedSecret, nil
-	}
-
-	jsonStr := string(serializedSecret)
-	// We need to search if a given key with a . exists before using gjson operations.
-	idx := strings.Index(ref.Property, ".")
-	if idx > -1 {
-		refProperty := strings.ReplaceAll(ref.Property, ".", "\\.")
-		val := gjson.Get(jsonStr, refProperty)
-		if val.Exists() {
-			return []byte(val.Str), nil
-		}
-	}
-	val := gjson.Get(jsonStr, ref.Property)
-	if !val.Exists() {
-		return nil, fmt.Errorf("property %s does not exist in key %s", ref.Property, ref.Key)
-	}
-	return []byte(val.String()), nil
-}
-
-// serializeSecret serializes a secret map[string][]byte into a flat []byte.
-func serializeSecret(secret *v1.Secret, ref esv1beta1.ExternalSecretDataRemoteRef) ([]byte, error) {
-	// metadata is treated differently, because it
-	// contains nested maps which can be queried with `ref.Property`
 		if ref.MetadataPolicy == esv1beta1.ExternalSecretMetadataPolicyFetch {
-		values, err := getSecretMetadata(secret)
+			m := map[string]map[string]string{}
+			m[metaLabels] = secret.Labels
+			m[metaAnnotations] = secret.Annotations
+
+			j, err := jsonMarshal(m)
 			if err != nil {
 				return nil, err
 			}
-		data := make(map[string]json.RawMessage, len(values))
+			return j, nil
-		for k, v := range values {
-			data[k] = encodeBinaryData(v)
-		}
-		return jsonMarshal(data)
 		}
 
-	strMap := make(map[string]string)
+		m := map[string]string{}
-	for k, v := range secret.Data {
+		for key, val := range secret.Data {
-		strMap[k] = string(encodeBinaryData(v))
+			m[key] = string(val)
 		}
-	return jsonMarshal(strMap)
+		j, err := jsonMarshal(m)
+		if err != nil {
+			return nil, err
+		}
+		return j, nil
 	}
 
-// encode binary data encodes non UTF-8 data
+	return getSecret(secret, ref)
-// as base64. This is needed to support proper json serialization.
-// if binary data would not be encoded, it would be utf-8 escaped: `\uffed`.
-func encodeBinaryData(input []byte) []byte {
-	if utf8.Valid(input) {
-		return input
-	}
-	return []byte(base64.StdEncoding.EncodeToString(input))
 }
 
 func jsonMarshal(t interface{}) ([]byte, error) {
@@ -372,3 +340,87 @@ func (c *Client) updateProperty(ctx context.Context, extSecret *v1.Secret, remot
 	metrics.ObserveAPICall(constants.ProviderKubernetes, constants.CallKubernetesUpdateSecret, uErr)
 	return uErr
 }
+
+func getSecret(secret *v1.Secret, ref esv1beta1.ExternalSecretDataRemoteRef) ([]byte, error) {
+	if ref.MetadataPolicy == esv1beta1.ExternalSecretMetadataPolicyFetch {
+		s, found, err := getFromSecretMetadata(secret, ref)
+		if err != nil {
+			return nil, err
+		}
+
+		if !found {
+			return nil, fmt.Errorf("property %s does not exist in metadata of secret %q", ref.Property, ref.Key)
+		}
+
+		return s, nil
+	}
+
+	s, found := getFromSecretData(secret, ref)
+	if !found {
+		return nil, fmt.Errorf("property %s does not exist in data of secret %q", ref.Property, ref.Key)
+	}
+
+	return s, nil
+}
+
+func getFromSecretData(secret *v1.Secret, ref esv1beta1.ExternalSecretDataRemoteRef) ([]byte, bool) {
+	// Check if a property with "." exists first such as file.png
+	v, ok := secret.Data[ref.Property]
+	if ok {
+		return v, true
+	}
+
+	idx := strings.Index(ref.Property, ".")
+	if idx == -1 || idx == 0 || idx == len(ref.Property)-1 {
+		return nil, false
+	}
+
+	v, ok = secret.Data[ref.Property[:idx]]
+	if !ok {
+		return nil, false
+	}
+
+	val := gjson.Get(string(v), ref.Property[idx+1:])
+	if !val.Exists() {
+		return nil, false
+	}
+
+	return []byte(val.String()), true
+}
+
+func getFromSecretMetadata(secret *v1.Secret, ref esv1beta1.ExternalSecretDataRemoteRef) ([]byte, bool, error) {
+	path := strings.Split(ref.Property, ".")
+
+	var metadata map[string]string
+	switch path[0] {
+	case metaLabels:
+		metadata = secret.Labels
+	case metaAnnotations:
+		metadata = secret.Annotations
+	default:
+		return nil, false, nil
+	}
+
+	if len(path) == 1 {
+		j, err := jsonMarshal(metadata)
+		if err != nil {
+			return nil, false, err
+		}
+		return j, true, nil
+	}
+
+	v, ok := metadata[path[1]]
+	if !ok {
+		return nil, false, nil
+	}
+	if len(path) == 2 {
+		return []byte(v), true, nil
+	}
+
+	val := gjson.Get(v, strings.Join(path[2:], ""))
+	if !val.Exists() {
+		return nil, false, nil
+	}
+
+	return []byte(val.String()), true, nil
+}

pkg/provider/kubernetes/client_test.go

@@ -15,9 +15,9 @@ package kubernetes
 
 import (
 	"context"
-	"encoding/base64"
 	"errors"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -102,92 +102,23 @@ func (fk *fakeClient) Update(_ context.Context, secret *v1.Secret, _ metav1.Upda
 var binaryTestData = []byte{0x00, 0xff, 0x00, 0xff, 0xac, 0xab, 0x28, 0x21}
 
 func TestGetSecret(t *testing.T) {
-	type fields struct {
-		Client       KClient
-		ReviewClient RClient
-		Namespace    string
-	}
 	tests := []struct {
-		name   string
+		desc      string
-		fields fields
+		secrets   map[string]*v1.Secret
+		clientErr error
 		ref       esv1beta1.ExternalSecretDataRemoteRef
-
 		want      []byte
-		wantErr bool
+		wantErr   string
 	}{
 		{
-			name: "secretNotFound",
+			desc: "secret data with correct property",
-			fields: fields{
+			secrets: map[string]*v1.Secret{
-				Client: &fakeClient{
-					t: t,
-					secretMap: map[string]*v1.Secret{
 				"mysec": {
 					Data: map[string][]byte{
 						"token": []byte(`foobar`),
 					},
 				},
 			},
-					err: apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "Secret"}, "secret"),
-				},
-				Namespace: "default",
-			},
-			ref: esv1beta1.ExternalSecretDataRemoteRef{
-				Key:      "mysec",
-				Property: "token",
-			},
-			wantErr: true,
-		},
-		{
-			name: "err GetSecretMap",
-			fields: fields{
-				Client: &fakeClient{
-					t:         t,
-					secretMap: map[string]*v1.Secret{},
-				},
-				Namespace: "default",
-			},
-			ref: esv1beta1.ExternalSecretDataRemoteRef{
-				Key:      "mysec",
-				Property: "token",
-			},
-			wantErr: true,
-		},
-		{
-			name: "wrong property",
-			fields: fields{
-				Client: &fakeClient{
-					t: t,
-					secretMap: map[string]*v1.Secret{
-						"mysec": {
-							Data: map[string][]byte{
-								"token": []byte(`foobar`),
-							},
-						},
-					},
-				},
-				Namespace: "default",
-			},
-			ref: esv1beta1.ExternalSecretDataRemoteRef{
-				Key:      "mysec",
-				Property: "not-the-token",
-			},
-			wantErr: true,
-		},
-		{
-			name: "successful case",
-			fields: fields{
-				Client: &fakeClient{
-					t: t,
-					secretMap: map[string]*v1.Secret{
-						"mysec": {
-							Data: map[string][]byte{
-								"token": []byte(`foobar`),
-							},
-						},
-					},
-				},
-				Namespace: "default",
-			},
 			ref: esv1beta1.ExternalSecretDataRemoteRef{
 				Key:      "mysec",
 				Property: "token",
@@ -195,31 +126,53 @@ func TestGetSecret(t *testing.T) {
 			want: []byte(`foobar`),
 		},
 		{
-			name: "successful case with html chars",
+			desc: "secret data with multi level property",
-			fields: fields{
+			secrets: map[string]*v1.Secret{
-				Client: &fakeClient{
+				"mysec": {
-					t: t,
+					Data: map[string][]byte{
-					secretMap: map[string]*v1.Secret{
+						"foo": []byte(`{"huga":{"bar":"val"}}`),
+					},
+				},
+			},
+			ref: esv1beta1.ExternalSecretDataRemoteRef{
+				Key:      "mysec",
+				Property: "foo.huga.bar",
+			},
+			want: []byte(`val`),
+		},
+		{
+			desc: "secret data with property containing .",
+			secrets: map[string]*v1.Secret{
+				"mysec": {
+					Data: map[string][]byte{
+						"foo.png": []byte(`correct`),
+						"foo":     []byte(`{"png":"wrong"}`),
+					},
+				},
+			},
+			ref: esv1beta1.ExternalSecretDataRemoteRef{
+				Key:      "mysec",
+				Property: "foo.png",
+			},
+			want: []byte(`correct`),
+		},
+		{
+			desc: "secret data contains html characters",
+			secrets: map[string]*v1.Secret{
 				"mysec": {
 					Data: map[string][]byte{
 						"html": []byte(`<foobar>`),
 					},
 				},
 			},
-				},
-				Namespace: "default",
-			},
 			ref: esv1beta1.ExternalSecretDataRemoteRef{
 				Key: "mysec",
 			},
 			want: []byte(`{"html":"<foobar>"}`),
 		},
 		{
-			name: "successful case metadata with html special chars and without property",
+			desc: "secret metadata contains html characters",
-			fields: fields{
+			secrets: map[string]*v1.Secret{
-				Client: &fakeClient{
-					t: t,
-					secretMap: map[string]*v1.Secret{
 				"mysec": {
 					ObjectMeta: metav1.ObjectMeta{
 						Annotations: map[string]string{"date": "today"},
@@ -227,9 +180,6 @@ func TestGetSecret(t *testing.T) {
 					},
 				},
 			},
-				},
-				Namespace: "default",
-			},
 			ref: esv1beta1.ExternalSecretDataRemoteRef{
 				MetadataPolicy: esv1beta1.ExternalSecretMetadataPolicyFetch,
 				Key:            "mysec",
@@ -237,52 +187,37 @@ func TestGetSecret(t *testing.T) {
 			want: []byte(`{"annotations":{"date":"today"},"labels":{"dev":"<seb>"}}`),
 		},
 		{
-			name: "successful case with binary data",
+			desc: "secret data contains binary",
-			fields: fields{
+			secrets: map[string]*v1.Secret{
-				Client: &fakeClient{
-					t: t,
-					secretMap: map[string]*v1.Secret{
 				"mysec": {
 					Data: map[string][]byte{
 						"bindata": binaryTestData,
 					},
 				},
 			},
-				},
-				Namespace: "default",
-			},
 			ref: esv1beta1.ExternalSecretDataRemoteRef{
 				Key:      "mysec",
 				Property: "bindata",
 			},
-			want: []byte(base64.StdEncoding.EncodeToString(binaryTestData)),
+			want: binaryTestData,
 		},
 		{
-			name: "successful case without property",
+			desc: "secret data without property",
-			fields: fields{
+			secrets: map[string]*v1.Secret{
-				Client: &fakeClient{
-					t: t,
-					secretMap: map[string]*v1.Secret{
 				"mysec": {
 					Data: map[string][]byte{
 						"token": []byte(`foobar`),
 					},
 				},
 			},
-				},
-				Namespace: "default",
-			},
 			ref: esv1beta1.ExternalSecretDataRemoteRef{
 				Key: "mysec",
 			},
 			want: []byte(`{"token":"foobar"}`),
 		},
 		{
-			name: "successful case metadata without property",
+			desc: "secret metadata without property",
-			fields: fields{
+			secrets: map[string]*v1.Secret{
-				Client: &fakeClient{
-					t: t,
-					secretMap: map[string]*v1.Secret{
 				"mysec": {
 					ObjectMeta: metav1.ObjectMeta{
 						Annotations: map[string]string{"date": "today"},
@@ -290,9 +225,6 @@ func TestGetSecret(t *testing.T) {
 					},
 				},
 			},
-				},
-				Namespace: "default",
-			},
 			ref: esv1beta1.ExternalSecretDataRemoteRef{
 				MetadataPolicy: esv1beta1.ExternalSecretMetadataPolicyFetch,
 				Key:            "mysec",
@@ -300,11 +232,8 @@ func TestGetSecret(t *testing.T) {
 			want: []byte(`{"annotations":{"date":"today"},"labels":{"dev":"seb"}}`),
 		},
 		{
-			name: "successful case metadata with single property",
+			desc: "secret metadata with single level property",
-			fields: fields{
+			secrets: map[string]*v1.Secret{
-				Client: &fakeClient{
-					t: t,
-					secretMap: map[string]*v1.Secret{
 				"mysec": {
 					ObjectMeta: metav1.ObjectMeta{
 						Annotations: map[string]string{"date": "today"},
@@ -312,9 +241,6 @@ func TestGetSecret(t *testing.T) {
 					},
 				},
 			},
-				},
-				Namespace: "default",
-			},
 			ref: esv1beta1.ExternalSecretDataRemoteRef{
 				MetadataPolicy: esv1beta1.ExternalSecretMetadataPolicyFetch,
 				Key:            "mysec",
@@ -323,11 +249,8 @@ func TestGetSecret(t *testing.T) {
 			want: []byte(`{"dev":"seb"}`),
 		},
 		{
-			name: "successful case metadata with multiple properties",
+			desc: "secret metadata with multiple level property",
-			fields: fields{
+			secrets: map[string]*v1.Secret{
-				Client: &fakeClient{
-					t: t,
-					secretMap: map[string]*v1.Secret{
 				"mysec": {
 					ObjectMeta: metav1.ObjectMeta{
 						Annotations: map[string]string{"date": "today"},
@@ -335,9 +258,6 @@ func TestGetSecret(t *testing.T) {
 					},
 				},
 			},
-				},
-				Namespace: "default",
-			},
 			ref: esv1beta1.ExternalSecretDataRemoteRef{
 				MetadataPolicy: esv1beta1.ExternalSecretMetadataPolicyFetch,
 				Key:            "mysec",
@@ -346,11 +266,32 @@ func TestGetSecret(t *testing.T) {
 			want: []byte(`seb`),
 		},
 		{
-			name: "error case metadata with wrong property",
+			desc:      "secret is not found",
-			fields: fields{
+			clientErr: apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "Secret"}, "secret"),
-				Client: &fakeClient{
+			ref: esv1beta1.ExternalSecretDataRemoteRef{
-					t: t,
+				Key:      "mysec",
-					secretMap: map[string]*v1.Secret{
+				Property: "token",
+			},
+			wantErr: `Secret "secret" not found`,
+		},
+		{
+			desc: "secret data with wrong property",
+			secrets: map[string]*v1.Secret{
+				"mysec": {
+					Data: map[string][]byte{
+						"token": []byte(`foobar`),
+					},
+				},
+			},
+			ref: esv1beta1.ExternalSecretDataRemoteRef{
+				Key:      "mysec",
+				Property: "not-the-token",
+			},
+			wantErr: "property not-the-token does not exist in data of secret",
+		},
+		{
+			desc: "secret metadata with wrong property",
+			secrets: map[string]*v1.Secret{
 				"mysec": {
 					ObjectMeta: metav1.ObjectMeta{
 						Annotations: map[string]string{"date": "today"},
@@ -358,31 +299,39 @@ func TestGetSecret(t *testing.T) {
 					},
 				},
 			},
-				},
-				Namespace: "default",
-			},
 			ref: esv1beta1.ExternalSecretDataRemoteRef{
 				MetadataPolicy: esv1beta1.ExternalSecretMetadataPolicyFetch,
 				Key:            "mysec",
 				Property:       "foo",
 			},
-			wantErr: true,
+			wantErr: "property foo does not exist in metadata of secret",
 		},
 	}
 	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
+		t.Run(tt.desc, func(t *testing.T) {
 			p := &Client{
-				userSecretClient: tt.fields.Client,
+				userSecretClient: &fakeClient{t: t, secretMap: tt.secrets, err: tt.clientErr},
-				userReviewClient: tt.fields.ReviewClient,
+				namespace:        "default",
-				namespace:        tt.fields.Namespace,
 			}
 			got, err := p.GetSecret(context.Background(), tt.ref)
-			if (err != nil) != tt.wantErr {
+			if err != nil {
-				t.Errorf("ProviderKubernetes.GetSecret() error = %v, wantErr %v", err, tt.wantErr)
+				if tt.wantErr == "" {
+					t.Fatalf("failed to call GetSecret: %v", err)
+				}
+
+				if !strings.Contains(err.Error(), tt.wantErr) {
+					t.Fatalf("received an unexpected error: %q should have contained %q", err.Error(), tt.wantErr)
+				}
+
 				return
 			}
+
+			if tt.wantErr != "" {
+				t.Fatalf("expected to receive an error but got nil")
+			}
+
 			if !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("ProviderKubernetes.GetSecret() = %s, want %s", got, tt.want)
+				t.Fatalf("received an unexpected secret: got: %s, want %s", got, tt.want)
 			}
 		})
 	}