✨ Adds PushSecret property compatibility with Hashicorp vault Provider (#2361)

* Adds PushSecret property compatibility with Hashicorp vault Provider

Increases Test Coverage for Hashicorp Vault provider
Signed-off-by: Gustavo Carvalho <gusfcarvalho@gmail.com>

* Fixing lint

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

* Fixing test property setup

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

---------

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

pkg/controllers/externalsecret/externalsecret_controller.go

@@ -279,8 +279,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return nil
 	}
 
-	//nolint
+	switch externalSecret.Spec.Target.CreationPolicy { //nolint
-	switch externalSecret.Spec.Target.CreationPolicy {
 	case esv1beta1.CreatePolicyMerge:
 		err = patchSecret(ctx, r.Client, r.Scheme, secret, mutationFunc, externalSecret.Name)
 		if err == nil {

pkg/provider/vault/fake/vault.go

@@ -16,6 +16,9 @@ package fake
 
 import (
 	"context"
+	"fmt"
+	"reflect"
+	"strings"
 
 	vault "github.com/hashicorp/vault/api"
 
@@ -82,13 +85,33 @@ func NewReadMetadataWithContextFn(secret map[string]interface{}, err error) Read
 
 func NewWriteWithContextFn(secret map[string]interface{}, err error) WriteWithContextFn {
 	return func(ctx context.Context, path string, data map[string]interface{}) (*vault.Secret, error) {
-		vault := &vault.Secret{
+		return &vault.Secret{Data: secret}, err
-			Data: secret,
-		}
-		return vault, err
 	}
 }
 
+func ExpectWriteWithContextValue(expected map[string]interface{}) WriteWithContextFn {
+	return func(ctx context.Context, path string, data map[string]interface{}) (*vault.Secret, error) {
+		if strings.Contains(path, "metadata") {
+			return &vault.Secret{Data: data}, nil
+		}
+		if !reflect.DeepEqual(expected, data) {
+			return nil, fmt.Errorf("expected: %v, got: %v", expected, data)
+		}
+		return &vault.Secret{Data: data}, nil
+	}
+}
+
+func ExpectWriteWithContextNoCall() WriteWithContextFn {
+	return func(_ context.Context, path string, data map[string]interface{}) (*vault.Secret, error) {
+		return nil, fmt.Errorf("fail")
+	}
+}
+
+func ExpectDeleteWithContextNoCall() DeleteWithContextFn {
+	return func(ctx context.Context, path string) (*vault.Secret, error) {
+		return nil, fmt.Errorf("fail")
+	}
+}
 func WriteChangingReadContext(secret map[string]interface{}, l Logical) WriteWithContextFn {
 	v := &vault.Secret{
 		Data: secret,

pkg/provider/vault/vault.go

@@ -413,7 +413,7 @@ func (v *client) DeleteSecret(ctx context.Context, remoteRef esv1beta1.PushRemot
 		return err
 	}
 	// Retrieve the secret map from vault and convert the secret value in string form.
-	_, err = v.readSecret(ctx, path, "")
+	secretVal, err := v.readSecret(ctx, path, "")
 	// If error is not of type secret not found, we should error
 	if err != nil && errors.Is(err, esv1beta1.NoSecretError{}) {
 		return nil
@@ -421,6 +421,18 @@ func (v *client) DeleteSecret(ctx context.Context, remoteRef esv1beta1.PushRemot
 	if err != nil {
 		return err
 	}
+	// If Push for a Property, we need to delete the property and update the secret
+	if remoteRef.GetProperty() != "" {
+		delete(secretVal, remoteRef.GetProperty())
+		if len(secretVal) > 0 {
+			secretToPush := map[string]interface{}{
+				"data": secretVal,
+			}
+			_, err = v.logical.WriteWithContext(ctx, path, secretToPush)
+			metrics.ObserveAPICall(constants.ProviderHCVault, constants.CallHCVaultDeleteSecret, err)
+			return err
+		}
+	}
 	metadata, err := v.readSecretMetadata(ctx, remoteRef.GetRemoteKey())
 	if err != nil {
 		return err
@@ -449,13 +461,6 @@ func (v *client) PushSecret(ctx context.Context, value []byte, remoteRef esv1bet
 		},
 	}
 	secretVal := make(map[string]interface{})
-	err := json.Unmarshal(value, &secretVal)
-	if err != nil {
-		return fmt.Errorf("failed to convert value to a valid JSON: %w", err)
-	}
-	secretToPush := map[string]interface{}{
-		"data": secretVal,
-	}
 	path := v.buildPath(remoteRef.GetRemoteKey())
 	metaPath, err := v.buildMetadataPath(remoteRef.GetRemoteKey())
 	if err != nil {
@@ -486,6 +491,35 @@ func (v *client) PushSecret(ctx context.Context, value []byte, remoteRef esv1bet
 	if bytes.Equal(vaultSecretValue, value) {
 		return nil
 	}
+	// If a Push of a property only, we should merge and add/update the property
+	if remoteRef.GetProperty() != "" {
+		if _, ok := vaultSecret[remoteRef.GetProperty()]; ok {
+			d := vaultSecret[remoteRef.GetProperty()].(string)
+			if err != nil {
+				return fmt.Errorf("error marshaling vault secret: %w", err)
+			}
+			// If the property has the same value, don't update the secret
+			if bytes.Equal([]byte(d), value) {
+				return nil
+			}
+		}
+		for k, v := range vaultSecret {
+			secretVal[k] = v
+		}
+		// Secret got from vault is already on map[string]string format
+		secretVal[remoteRef.GetProperty()] = string(value)
+	} else {
+		err = json.Unmarshal(value, &secretVal)
+		if err != nil {
+			return fmt.Errorf("error unmarshalling vault secret: %w", err)
+		}
+	}
+	secretToPush := map[string]interface{}{
+		"data": secretVal,
+	}
+	if err != nil {
+		return fmt.Errorf("failed to convert value to a valid JSON: %w", err)
+	}
 	_, err = v.logical.WriteWithContext(ctx, metaPath, label)
 	metrics.ObserveAPICall(constants.ProviderHCVault, constants.CallHCVaultWriteSecretData, err)
 	if err != nil {

pkg/provider/vault/vault_test.go

@@ -1570,7 +1570,8 @@ func TestValidateStore(t *testing.T) {
 }
 
 type fakeRef struct {
-	key string
+	key      string
+	property string
 }
 
 func (f fakeRef) GetRemoteKey() string {
@@ -1578,9 +1579,188 @@ func (f fakeRef) GetRemoteKey() string {
 }
 
 func (f fakeRef) GetProperty() string {
-	return ""
+	return f.property
 }
 
+func TestDeleteSecret(t *testing.T) {
+	type args struct {
+		store    *esv1beta1.VaultProvider
+		vLogical util.Logical
+	}
+
+	type want struct {
+		err error
+	}
+	tests := map[string]struct {
+		reason string
+		args   args
+		ref    *fakeRef
+		want   want
+		value  []byte
+	}{
+		"DeleteSecretNoOp": {
+			reason: "No secret is because it does not exist",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(nil, nil),
+					WriteWithContextFn:        fake.ExpectWriteWithContextNoCall(),
+					DeleteWithContextFn:       fake.ExpectDeleteWithContextNoCall(),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"DeleteSecretFailIfError": {
+			reason: "No secret is because it does not exist",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(nil, fmt.Errorf("failed to read")),
+					WriteWithContextFn:        fake.ExpectWriteWithContextNoCall(),
+					DeleteWithContextFn:       fake.ExpectDeleteWithContextNoCall(),
+				},
+			},
+			want: want{
+				err: fmt.Errorf("failed to read"),
+			},
+		},
+		"DeleteSecretNotManaged": {
+			reason: "No secret is because it does not exist",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
+						"data": map[string]interface{}{
+							"fake-key": "fake-value",
+						},
+						"custom_metadata": map[string]interface{}{
+							"managed-by": "another-secret-tool",
+						},
+					}, nil),
+					WriteWithContextFn:  fake.ExpectWriteWithContextNoCall(),
+					DeleteWithContextFn: fake.NewDeleteWithContextFn(nil, nil),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"DeleteSecretSuccess": {
+			reason: "No secret is because it does not exist",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
+						"data": map[string]interface{}{
+							"fake-key": "fake-value",
+						},
+						"custom_metadata": map[string]interface{}{
+							"managed-by": "external-secrets",
+						},
+					}, nil),
+					WriteWithContextFn:  fake.ExpectWriteWithContextNoCall(),
+					DeleteWithContextFn: fake.NewDeleteWithContextFn(nil, nil),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"DeleteSecretError": {
+			reason: "No secret is because it does not exist",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
+						"data": map[string]interface{}{
+							"fake-key": "fake-value",
+						},
+						"custom_metadata": map[string]interface{}{
+							"managed-by": "external-secrets",
+						},
+					}, nil),
+					WriteWithContextFn:  fake.ExpectWriteWithContextNoCall(),
+					DeleteWithContextFn: fake.NewDeleteWithContextFn(nil, fmt.Errorf("failed to delete")),
+				},
+			},
+			want: want{
+				err: fmt.Errorf("failed to delete"),
+			},
+		},
+		"DeleteSecretUpdateProperty": {
+			reason: "Secret should only be updated if Property is set",
+			ref:    &fakeRef{key: "secret", property: "fake-key"},
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
+						"data": map[string]interface{}{
+							"fake-key": "fake-value",
+							"foo":      "bar",
+						},
+						"custom_metadata": map[string]interface{}{
+							"managed-by": "external-secrets",
+						},
+					}, nil),
+					WriteWithContextFn:  fake.ExpectWriteWithContextValue(map[string]interface{}{"data": map[string]interface{}{"foo": "bar"}}),
+					DeleteWithContextFn: fake.ExpectDeleteWithContextNoCall(),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"DeleteSecretIfNoOtherProperties": {
+			reason: "Secret should only be deleted if no other properties are set",
+			ref:    &fakeRef{key: "secret", property: "foo"},
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
+						"data": map[string]interface{}{
+							"foo": "bar",
+						},
+						"custom_metadata": map[string]interface{}{
+							"managed-by": "external-secrets",
+						},
+					}, nil),
+					WriteWithContextFn:  fake.ExpectWriteWithContextNoCall(),
+					DeleteWithContextFn: fake.NewDeleteWithContextFn(nil, nil),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+	}
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			ref := fakeRef{key: "secret", property: ""}
+			if tc.ref != nil {
+				ref = *tc.ref
+			}
+			client := &client{
+				logical: tc.args.vLogical,
+				store:   tc.args.store,
+			}
+			err := client.DeleteSecret(context.Background(), ref)
+
+			// Error nil XOR tc.want.err nil
+			if ((err == nil) || (tc.want.err == nil)) && !((err == nil) && (tc.want.err == nil)) {
+				t.Errorf("\nTesting DeleteSecret:\nName: %v\nReason: %v\nWant error: %v\nGot error: %v", name, tc.reason, tc.want.err, err)
+			}
+
+			// if errors are the same type but their contents do not match
+			if err != nil && tc.want.err != nil {
+				if !strings.Contains(err.Error(), tc.want.err.Error()) {
+					t.Errorf("\nTesting DeleteSecret:\nName: %v\nReason: %v\nWant error: %v\nGot error got nil", name, tc.reason, tc.want.err)
+				}
+			}
+		})
+	}
+}
 func TestSetSecret(t *testing.T) {
 	noPermission := errors.New("no permission")
 
@@ -1596,6 +1776,8 @@ func TestSetSecret(t *testing.T) {
 		reason string
 		args   args
 		want   want
+		ref    *fakeRef
+		value  []byte
 	}{
 		"SetSecret": {
 			reason: "secret is successfully set, with no existing vault secret",
@@ -1644,6 +1826,72 @@ func TestSetSecret(t *testing.T) {
 				err: nil,
 			},
 		},
+		"PushSecretProperty": {
+			reason: "push secret with property adds the property",
+			value:  []byte("fake-value"),
+			ref:    &fakeRef{key: "secret", property: "foo"},
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
+						"data": map[string]interface{}{
+							"fake-key": "fake-value",
+						},
+						"custom_metadata": map[string]interface{}{
+							"managed-by": "external-secrets",
+						},
+					}, nil),
+					WriteWithContextFn: fake.ExpectWriteWithContextValue(map[string]interface{}{"data": map[string]interface{}{"fake-key": "fake-value", "foo": "fake-value"}}),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"PushSecretUpdateProperty": {
+			reason: "push secret with property only updates the property",
+			value:  []byte("new-value"),
+			ref:    &fakeRef{key: "secret", property: "foo"},
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
+						"data": map[string]interface{}{
+							"foo": "fake-value",
+						},
+						"custom_metadata": map[string]interface{}{
+							"managed-by": "external-secrets",
+						},
+					}, nil),
+					WriteWithContextFn: fake.ExpectWriteWithContextValue(map[string]interface{}{"data": map[string]interface{}{"foo": "new-value"}}),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"PushSecretPropertyNoUpdate": {
+			reason: "push secret with property only updates the property",
+			value:  []byte("fake-value"),
+			ref:    &fakeRef{key: "secret", property: "foo"},
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
+						"data": map[string]interface{}{
+							"foo": "fake-value",
+						},
+						"custom_metadata": map[string]interface{}{
+							"managed-by": "external-secrets",
+						},
+					}, nil),
+					WriteWithContextFn: fake.ExpectWriteWithContextNoCall(),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
 
 		"SetSecretErrorReadingSecret": {
 			reason: "error occurs if secret cannot be read",
@@ -1681,12 +1929,19 @@ func TestSetSecret(t *testing.T) {
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			ref := fakeRef{key: "secret"}
+			ref := fakeRef{key: "secret", property: ""}
+			if tc.ref != nil {
+				ref = *tc.ref
+			}
 			client := &client{
 				logical: tc.args.vLogical,
 				store:   tc.args.store,
 			}
-			err := client.PushSecret(context.Background(), []byte(`{"fake-key":"fake-value"}`), ref)
+			val := tc.value
+			if val == nil {
+				val = []byte(`{"fake-key":"fake-value"}`)
+			}
+			err := client.PushSecret(context.Background(), val, ref)
 
 			// Error nil XOR tc.want.err nil
 			if ((err == nil) || (tc.want.err == nil)) && !((err == nil) && (tc.want.err == nil)) {