fix: introducing support for conversion strategy for PushSecret. (#3292)

* fix: introducing support for conversion strategy for PushSecret.

Signed-off-by: Rodrigo Fior Kuntzer <rodrigo@miro.com>

* fix: unit tests code quality.

Signed-off-by: Rodrigo Fior Kuntzer <rodrigo@miro.com>

---------

Signed-off-by: Rodrigo Fior Kuntzer <rodrigo@miro.com>

apis/externalsecrets/v1alpha1/pushsecret_types.go

@@ -57,6 +57,14 @@ const (
 	PushSecretDeletionPolicyNone   PushSecretDeletionPolicy = "None"
 )
 
+// +kubebuilder:validation:Enum=None;ReverseUnicode
+type PushSecretConversionStrategy string
+
+const (
+	PushSecretConversionNone           PushSecretConversionStrategy = "None"
+	PushSecretConversionReverseUnicode PushSecretConversionStrategy = "ReverseUnicode"
+)
+
 // PushSecretSpec configures the behavior of the PushSecret.
 type PushSecretSpec struct {
 	// The Interval to which External Secrets will try to push a secret definition
@@ -121,6 +129,10 @@ type PushSecretData struct {
 	// The structure of metadata is provider specific, please look it up in the provider documentation.
 	// +optional
 	Metadata *apiextensionsv1.JSON `json:"metadata,omitempty"`
+	// +optional
+	// Used to define a conversion Strategy for the secret keys
+	// +kubebuilder:default="None"
+	ConversionStrategy PushSecretConversionStrategy `json:"conversionStrategy,omitempty"`
 }
 
 func (d PushSecretData) GetMetadata() *apiextensionsv1.JSON {

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

@@ -50,6 +50,14 @@ spec:
                 description: Secret Data that should be pushed to providers
                 items:
                   properties:
+                    conversionStrategy:
+                      default: None
+                      description: Used to define a conversion Strategy for the secret
+                        keys
+                      enum:
+                      - None
+                      - ReverseUnicode
+                      type: string
                     match:
                       description: Match a given Secret Key to be pushed to the provider.
                       properties:
@@ -315,6 +323,14 @@ spec:
                 additionalProperties:
                   additionalProperties:
                     properties:
+                      conversionStrategy:
+                        default: None
+                        description: Used to define a conversion Strategy for the
+                          secret keys
+                        enum:
+                        - None
+                        - ReverseUnicode
+                        type: string
                       match:
                         description: Match a given Secret Key to be pushed to the
                           provider.

deploy/crds/bundle.yaml

@@ -5547,6 +5547,13 @@ spec:
                   description: Secret Data that should be pushed to providers
                   items:
                     properties:
+                      conversionStrategy:
+                        default: None
+                        description: Used to define a conversion Strategy for the secret keys
+                        enum:
+                          - None
+                          - ReverseUnicode
+                        type: string
                       match:
                         description: Match a given Secret Key to be pushed to the provider.
                         properties:
@@ -5802,6 +5809,13 @@ spec:
                   additionalProperties:
                     additionalProperties:
                       properties:
+                        conversionStrategy:
+                          default: None
+                          description: Used to define a conversion Strategy for the secret keys
+                          enum:
+                            - None
+                            - ReverseUnicode
+                          type: string
                         match:
                           description: Match a given Secret Key to be pushed to the provider.
                           properties:

docs/guides/pushsecrets.md

@@ -40,3 +40,6 @@ This will _marshal_ the entire secret data and push it into this single property
 
 !!! warning inline
     This should _ONLY_ be done if the secret data is marshal-able. Values like, binary data cannot be marshaled and will result in error or invalid secret data.
+
+### Key conversion strategy
+You can also set `data[*].conversionStrategy: ReverseUnicode` to reverse the invalid character replaced by the `conversionStrategy: Unicode` configuration in the `ExternalSecret` object as [documented here](../guides/getallsecrets/#avoiding-name-conflicts).

docs/snippets/full-pushsecret.yaml

@@ -29,7 +29,8 @@ spec:
           items:
             - key: config.yml
   data:
-    - match:
+    - conversionStrategy: None # Also supports the ReverseUnicode strategy
+      match:
         secretKey: best-pokemon # Source Kubernetes secret key to be pushed
         remoteRef:
           remoteKey: my-first-parameter # Remote reference (where the secret is going to be pushed)

pkg/controllers/pushsecret/pushsecret_controller.go

@@ -38,6 +38,7 @@ import (
 	"github.com/external-secrets/external-secrets/pkg/controllers/pushsecret/psmetrics"
 	"github.com/external-secrets/external-secrets/pkg/controllers/secretstore"
 	"github.com/external-secrets/external-secrets/pkg/provider/util/locks"
+	"github.com/external-secrets/external-secrets/pkg/utils"
 )
 
 const (
@@ -47,6 +48,7 @@ const (
 	errGetClusterSecretStore = "could not get ClusterSecretStore %q, %w"
 	errSetSecretFailed       = "could not write remote ref %v to target secretstore %v: %v"
 	errFailedSetSecret       = "set secret failed: %v"
+	errConvert               = "could not apply conversion strategy to keys: %v"
 	pushSecretFinalizer      = "pushsecret.externalsecrets.io/finalizer"
 )
 
@@ -289,11 +291,17 @@ func (r *Reconciler) handlePushSecretDataForStore(ctx context.Context, ps esapi.
 		Name: storeName,
 		Kind: refKind,
 	}
+	originalSecretData := secret.Data
 	secretClient, err := mgr.Get(ctx, storeRef, ps.GetNamespace(), nil)
 	if err != nil {
 		return out, fmt.Errorf("could not get secrets client for store %v: %w", storeName, err)
 	}
 	for _, data := range ps.Spec.Data {
+		secretData, err := utils.ReverseKeys(data.ConversionStrategy, originalSecretData)
+		if err != nil {
+			return nil, fmt.Errorf(errConvert, err)
+		}
+		secret.Data = secretData
 		key := data.GetSecretKey()
 		if !secretKeyExists(key, secret) {
 			return out, fmt.Errorf("secret key %v does not exist", key)

pkg/controllers/pushsecret/pushsecret_controller_test.go

@@ -637,6 +637,64 @@ var _ = Describe("ExternalSecret controller", func() {
 			return true
 		}
 	}
+	// if conversion strategy is defined, revert the keys based on the strategy.
+	syncSuccessfullyWithConversionStrategy := func(tc *testCase) {
+		fakeProvider.SetSecretFn = func() error {
+			return nil
+		}
+		tc.pushsecret = &v1alpha1.PushSecret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      PushSecretName,
+				Namespace: PushSecretNamespace,
+			},
+			Spec: v1alpha1.PushSecretSpec{
+				SecretStoreRefs: []v1alpha1.PushSecretStoreRef{
+					{
+						Name: PushSecretStore,
+						Kind: "SecretStore",
+					},
+				},
+				Selector: v1alpha1.PushSecretSelector{
+					Secret: v1alpha1.PushSecretSecret{
+						Name: SecretName,
+					},
+				},
+				Data: []v1alpha1.PushSecretData{
+					{
+						ConversionStrategy: v1alpha1.PushSecretConversionReverseUnicode,
+						Match: v1alpha1.PushSecretMatch{
+							SecretKey: "some-array[0].entity",
+							RemoteRef: v1alpha1.PushSecretRemoteRef{
+								RemoteKey: "path/to/key",
+							},
+						},
+					},
+				},
+			},
+		}
+		tc.secret = &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      SecretName,
+				Namespace: PushSecretNamespace,
+			},
+			Data: map[string][]byte{
+				"some-array_U005b_0_U005d_.entity": []byte("value"),
+			},
+		}
+		tc.assert = func(ps *v1alpha1.PushSecret, secret *v1.Secret) bool {
+			Eventually(func() bool {
+				By("checking if Provider value got updated")
+				secretValue := secret.Data["some-array_U005b_0_U005d_.entity"]
+				providerValue, ok := fakeProvider.SetSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
+				if !ok {
+					return false
+				}
+				got := providerValue.Value
+				return bytes.Equal(got, secretValue)
+			}, time.Second*10, time.Second).Should(BeTrue())
+			return true
+		}
+	}
 	// if target Secret name is not specified it should use the ExternalSecret name.
 	syncMatchingLabels := func(tc *testCase) {
 		fakeProvider.SetSecretFn = func() error {
@@ -937,6 +995,7 @@ var _ = Describe("ExternalSecret controller", func() {
 		Entry("should update the PushSecret status correctly if UpdatePolicy=IfNotExists", updateIfNotExistsSyncStatus),
 		Entry("should fail if secret existence cannot be verified if UpdatePolicy=IfNotExists", updateIfNotExistsSyncFailed),
 		Entry("should sync with template", syncSuccessfullyWithTemplate),
+		Entry("should sync with conversion strategy", syncSuccessfullyWithConversionStrategy),
 		Entry("should delete if DeletionPolicy=Delete", syncAndDeleteSuccessfully),
 		Entry("should track deletion tasks if Delete fails", failDelete),
 		Entry("should track deleted stores if Delete fails", failDeleteStore),

pkg/utils/utils.go

@@ -33,6 +33,7 @@ import (
 
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 
+	esv1alpha1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1alpha1"
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
 	esmeta "github.com/external-secrets/external-secrets/apis/meta/v1"
 	"github.com/external-secrets/external-secrets/pkg/template/v2"
@@ -45,6 +46,7 @@ const (
 
 var (
 	errKeyNotFound = errors.New("key not found")
+	unicodeRegex   = regexp.MustCompile(`_U([0-9a-fA-F]{4,5})_`)
 )
 
 // JSONMarshal takes an interface and returns a new escaped and encoded byte slice.
@@ -237,6 +239,48 @@ func convert(strategy esv1beta1.ExternalSecretConversionStrategy, str string) st
 	return strings.Join(newName, "")
 }
 
+// ReverseKeys reverses a secret map into a valid key map as expected by push secrets.
+// Replaces the unicode encoded representation characters back to the actual unicode character depending on convert strategy.
+func ReverseKeys(strategy esv1alpha1.PushSecretConversionStrategy, in map[string][]byte) (map[string][]byte, error) {
+	out := make(map[string][]byte, len(in))
+	for k, v := range in {
+		key := reverse(strategy, k)
+		if _, exists := out[key]; exists {
+			return nil, fmt.Errorf("secret name collision during conversion: %s", key)
+		}
+		out[key] = v
+	}
+	return out, nil
+}
+
+func reverse(strategy esv1alpha1.PushSecretConversionStrategy, str string) string {
+	switch strategy {
+	case esv1alpha1.PushSecretConversionReverseUnicode:
+		matches := unicodeRegex.FindAllStringSubmatchIndex(str, -1)
+
+		for i := len(matches) - 1; i >= 0; i-- {
+			match := matches[i]
+			start := match[0]
+			end := match[1]
+			unicodeHex := str[match[2]:match[3]]
+
+			unicodeInt, err := strconv.ParseInt(unicodeHex, 16, 32)
+			if err != nil {
+				continue // Skip invalid unicode representations
+			}
+
+			unicodeChar := fmt.Sprintf("%c", unicodeInt)
+			str = str[:start] + unicodeChar + str[end:]
+		}
+
+		return str
+	case esv1alpha1.PushSecretConversionNone:
+		return str
+	default:
+		return str
+	}
+}
+
 // MergeStringMap performs a deep clone from src to dest.
 func MergeStringMap(dest, src map[string]string) {
 	for k, v := range src {

pkg/utils/utils_test.go

@@ -24,13 +24,17 @@ import (
 	v1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 
+	esv1alpha1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1alpha1"
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
 )
 
 const (
-	base64DecodedValue    string = "foo%_?bar"
-	base64EncodedValue    string = "Zm9vJV8/YmFy"
-	base64URLEncodedValue string = "Zm9vJV8_YmFy"
+	base64DecodedValue         string = "foo%_?bar"
+	base64EncodedValue         string = "Zm9vJV8/YmFy"
+	base64URLEncodedValue      string = "Zm9vJV8_YmFy"
+	keyWithEmojis              string = "😀foo😁bar😂baz😈bing"
+	keyWithInvalidChars        string = "some-array[0].entity"
+	keyWithEncodedInvalidChars string = "some-array_U005b_0_U005d_.entity"
 )
 
 func TestObjectHash(t *testing.T) {
@@ -212,7 +216,7 @@ func TestConvertKeys(t *testing.T) {
 			args: args{
 				strategy: esv1beta1.ExternalSecretConversionUnicode,
 				in: map[string][]byte{
-					"😀foo😁bar😂baz😈bing": []byte(`noop`),
+					keyWithEmojis: []byte(`noop`),
 				},
 			},
 			want: map[string][]byte{
@@ -234,6 +238,77 @@ func TestConvertKeys(t *testing.T) {
 	}
 }
 
+func TestReverseKeys(t *testing.T) {
+	type args struct {
+		encodingStrategy esv1beta1.ExternalSecretConversionStrategy
+		decodingStrategy esv1alpha1.PushSecretConversionStrategy
+		in               map[string][]byte
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    map[string][]byte
+		wantErr bool
+	}{
+		{
+			name: "encoding and decoding strategy are selecting Unicode conversion and reverse unicode, so the in and want should match, this test covers Unicode characters beyond the Basic Multilingual Plane (BMP)",
+			args: args{
+				encodingStrategy: esv1beta1.ExternalSecretConversionUnicode,
+				decodingStrategy: esv1alpha1.PushSecretConversionReverseUnicode,
+				in: map[string][]byte{
+					keyWithEmojis: []byte(`noop`),
+				},
+			},
+			want: map[string][]byte{
+				keyWithEmojis: []byte(`noop`),
+			},
+		},
+		{
+			name: "encoding and decoding strategy are selecting Unicode conversion and reverse unicode, so the in and want should match, this test covers Unicode characters in the Basic Multilingual Plane (BMP)",
+			args: args{
+				encodingStrategy: esv1beta1.ExternalSecretConversionUnicode,
+				decodingStrategy: esv1alpha1.PushSecretConversionReverseUnicode,
+				in: map[string][]byte{
+					keyWithInvalidChars: []byte(`noop`),
+				},
+			},
+			want: map[string][]byte{
+				keyWithInvalidChars: []byte(`noop`),
+			},
+		},
+		{
+			name: "the encoding strategy is selecting Unicode conversion, but the decoding strategy is none, so we want an encoded representation of the content",
+			args: args{
+				encodingStrategy: esv1beta1.ExternalSecretConversionUnicode,
+				decodingStrategy: esv1alpha1.PushSecretConversionNone,
+				in: map[string][]byte{
+					keyWithInvalidChars: []byte(`noop`),
+				},
+			},
+			want: map[string][]byte{
+				keyWithEncodedInvalidChars: []byte(`noop`),
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := ConvertKeys(tt.args.encodingStrategy, tt.args.in)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("ConvertKeys() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			got, err = ReverseKeys(tt.args.decodingStrategy, got)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("ReverseKeys() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("ReverseKeys() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
 func TestDecode(t *testing.T) {
 	type args struct {
 		strategy esv1beta1.ExternalSecretDecodingStrategy
@@ -542,6 +617,50 @@ func TestRewrite(t *testing.T) {
 	}
 }
 
+func TestReverse(t *testing.T) {
+	type args struct {
+		strategy esv1alpha1.PushSecretConversionStrategy
+		in       string
+	}
+	tests := []struct {
+		name string
+		args args
+		want string
+	}{
+		{
+			name: "do not change the key when using the None strategy",
+			args: args{
+				strategy: esv1alpha1.PushSecretConversionNone,
+				in:       keyWithEncodedInvalidChars,
+			},
+			want: keyWithEncodedInvalidChars,
+		},
+		{
+			name: "reverse an unicode encoded key",
+			args: args{
+				strategy: esv1alpha1.PushSecretConversionReverseUnicode,
+				in:       keyWithEncodedInvalidChars,
+			},
+			want: keyWithInvalidChars,
+		},
+		{
+			name: "do not attempt to decode an invalid unicode representation",
+			args: args{
+				strategy: esv1alpha1.PushSecretConversionReverseUnicode,
+				in:       "_U0xxx_x_U005b_",
+			},
+			want: "_U0xxx_x[",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := reverse(tt.args.strategy, tt.args.in); got != tt.want {
+				t.Errorf("reverse() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
 func TestFetchValueFromMetadata(t *testing.T) {
 	type args struct {
 		key  string