diff --git a/pkg/controllers/externalsecret/externalsecret_controller.go b/pkg/controllers/externalsecret/externalsecret_controller.go index a08801378..464272642 100644 --- a/pkg/controllers/externalsecret/externalsecret_controller.go +++ b/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 { + switch externalSecret.Spec.Target.CreationPolicy { //nolint case esv1beta1.CreatePolicyMerge: err = patchSecret(ctx, r.Client, r.Scheme, secret, mutationFunc, externalSecret.Name) if err == nil { diff --git a/pkg/provider/vault/fake/vault.go b/pkg/provider/vault/fake/vault.go index 3a1722a89..78c841543 100644 --- a/pkg/provider/vault/fake/vault.go +++ b/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{ - Data: secret, - } - return vault, err + return &vault.Secret{Data: secret}, 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, diff --git a/pkg/provider/vault/vault.go b/pkg/provider/vault/vault.go index 7dcf44f39..bb7f9651e 100644 --- a/pkg/provider/vault/vault.go +++ b/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 { diff --git a/pkg/provider/vault/vault_test.go b/pkg/provider/vault/vault_test.go index 1161acba3..a536cdda1 100644 --- a/pkg/provider/vault/vault_test.go +++ b/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)) {