Feat/Adding support for PushSecret using HashiCorp Vault KV v1 (#2879)

* feat: init pushsecret support for vault kv1

Signed-off-by: talasulin <tal.asulin@appsflyer.comn>

* feat: update delete secret to support vault kv1

Signed-off-by: talasulin <tal.asulin@appsflyer.comn>

* test: adding unit tests for deletesecret for vault v1 coverage

Signed-off-by: talasulin <tal.asulin@appsflyer.comn>

* docs: adding a note for describing the potential risk of using kv1 with pushsecret

Signed-off-by: talasulin <tal.asulin@appsflyer.comn>

* feat: removing white spaces

Signed-off-by: talasulin <tal.asulin@appsflyer.comn>

* feat: removing white spaces

Signed-off-by: talasulin <tal.asulin@appsflyer.comn>

* chore: reverting buildMetadataPath changes as they are not called from v1 logic

Signed-off-by: talasulin <tal.asulin@appsflyer.comn>

* feat: add custom metadata to vault v1 secrets

Signed-off-by: talasulin <tal.asulin@appsflyer.comn>

* docs: adjusting documentation for supporting vault kv v1

Signed-off-by: talasulin <tal.asulin@appsflyer.comn>

* docs: adjusting documentation for supporting vault kv v1

Signed-off-by: talasulin <tal.asulin@appsflyer.comn>

* Update docs/provider/hashicorp-vault.md

Co-authored-by: Gustavo Fernandes de Carvalho <gusfcarvalho@gmail.com>
Signed-off-by: Tal Asulin <tallin900@gmail.com>
Signed-off-by: talasulin <tal.asulin@appsflyer.comn>

---------

Signed-off-by: talasulin <tal.asulin@appsflyer.comn>
Signed-off-by: Tal Asulin <tallin900@gmail.com>
Co-authored-by: talasulin <tal.asulin@appsflyer.comn>
Co-authored-by: tal-asulin <tal-asulin@users.noreply.github.com>
Co-authored-by: Gustavo Fernandes de Carvalho <gusfcarvalho@gmail.com>

docs/provider/hashicorp-vault.md

@@ -78,13 +78,13 @@ spec:
   # metadataPolicy to fetch all the labels in JSON format
   - secretKey: tags
     remoteRef:
-      metadataPolicy: Fetch 
+      metadataPolicy: Fetch
       key: foo
 
   # metadataPolicy to fetch a specific label (dev) from the source secret
   - secretKey: developer
     remoteRef:
-      metadataPolicy: Fetch 
+      metadataPolicy: Fetch
       key: foo
       property: dev
 
@@ -402,6 +402,10 @@ This approach assumes that appropriate IRSA setup is done controller's pod (i.e.
 Vault supports PushSecret features which allow you to sync a given Kubernetes secret key into a Hashicorp vault secret. To do so, it is expected that the secret key is a valid JSON object or that the `property` attribute has been specified under the `remoteRef`.
 To use PushSecret, you need to give `create`, `read` and `update` permissions to the path where you want to push secrets for both `data` and `metadata` of the secret. Use it with care!
 
+!!! note
+     Since Vault KV v1 API is not supported with storing secrets metadata, PushSecret will add a `custom_metadata` map to each secret in Vault that he will manage. It means pushing secret keys named `custom_metadata` is not supported with Vault KV v1.
+
+
 Here is an example of how to set up `PushSecret`:
 
 ```yaml

pkg/provider/vault/vault.go

@@ -446,35 +446,44 @@ func (v *client) DeleteSecret(ctx context.Context, remoteRef esv1beta1.PushSecre
 	if err != nil {
 		return err
 	}
+	metadata, err := v.readSecretMetadata(ctx, remoteRef.GetRemoteKey())
+	if err != nil {
+		return err
+	}
+	manager, ok := metadata["managed-by"]
+	if !ok || manager != "external-secrets" {
+		return nil
+	}
 	// If Push for a Property, we need to delete the property and update the secret
 	if remoteRef.GetProperty() != "" {
 		delete(secretVal, remoteRef.GetProperty())
+		// If the only key left in the remote secret is the reference of the metadata.
+		if v.store.Version == esv1beta1.VaultKVStoreV1 && len(secretVal) == 1 {
+			delete(secretVal, "custom_metadata")
+		}
 		if len(secretVal) > 0 {
-			secretToPush := map[string]interface{}{
-				"data": secretVal,
+			secretToPush := secretVal
+			if v.store.Version == esv1beta1.VaultKVStoreV2 {
+				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
-	}
-	manager, ok := metadata["managed-by"]
-	if !ok || manager != "external-secrets" {
-		return nil
-	}
 	_, err = v.logical.DeleteWithContext(ctx, path)
 	metrics.ObserveAPICall(constants.ProviderHCVault, constants.CallHCVaultDeleteSecret, err)
 	if err != nil {
 		return fmt.Errorf("could not delete secret %v: %w", remoteRef.GetRemoteKey(), err)
 	}
-	_, err = v.logical.DeleteWithContext(ctx, metaPath)
-	metrics.ObserveAPICall(constants.ProviderHCVault, constants.CallHCVaultDeleteSecret, err)
-	if err != nil {
-		return fmt.Errorf("could not delete secret metadata %v: %w", remoteRef.GetRemoteKey(), err)
+	if v.store.Version == esv1beta1.VaultKVStoreV2 {
+		_, err = v.logical.DeleteWithContext(ctx, metaPath)
+		metrics.ObserveAPICall(constants.ProviderHCVault, constants.CallHCVaultDeleteSecret, err)
+		if err != nil {
+			return fmt.Errorf("could not delete secret metadata %v: %w", remoteRef.GetRemoteKey(), err)
+		}
 	}
 	return nil
 }
@@ -510,6 +519,10 @@ func (v *client) PushSecret(ctx context.Context, secret *corev1.Secret, data esv
 			return fmt.Errorf("secret not managed by external-secrets")
 		}
 	}
+	// Remove the metadata map to check the reconcile difference
+	if v.store.Version == esv1beta1.VaultKVStoreV1 {
+		delete(vaultSecret, "custom_metadata")
+	}
 	buf := &bytes.Buffer{}
 	enc := json.NewEncoder(buf)
 	enc.SetEscapeHTML(false)
@@ -547,16 +560,26 @@ func (v *client) PushSecret(ctx context.Context, secret *corev1.Secret, data esv
 			return fmt.Errorf("error unmarshalling vault secret: %w", err)
 		}
 	}
-	secretToPush := map[string]interface{}{
-		"data": secretVal,
+	secretToPush := secretVal
+	// Adding custom_metadata to the secret for KV v1
+	if v.store.Version == esv1beta1.VaultKVStoreV1 {
+		secretToPush["custom_metadata"] = label["custom_metadata"]
+	}
+	if v.store.Version == esv1beta1.VaultKVStoreV2 {
+		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 {
-		return err
+	// Secret metadata should be pushed separately only for KV2
+	if v.store.Version == esv1beta1.VaultKVStoreV2 {
+		_, err = v.logical.WriteWithContext(ctx, metaPath, label)
+		metrics.ObserveAPICall(constants.ProviderHCVault, constants.CallHCVaultWriteSecretData, err)
+		if err != nil {
+			return err
+		}
 	}
 	// Otherwise, create or update the version.
 	_, err = v.logical.WriteWithContext(ctx, path, secretToPush)
@@ -859,14 +882,18 @@ func (v *client) Validate() (esv1beta1.ValidationResult, error) {
 
 func (v *client) buildMetadataPath(path string) (string, error) {
 	var url string
-	if v.store.Path == nil && !strings.Contains(path, "data") {
-		return "", fmt.Errorf(errPathInvalid)
-	}
-	if v.store.Path == nil {
-		path = strings.Replace(path, "data", "metadata", 1)
-		url = path
-	} else {
-		url = fmt.Sprintf("%s/metadata/%s", *v.store.Path, path)
+	if v.store.Version == esv1beta1.VaultKVStoreV1 {
+		url = fmt.Sprintf("%s/%s", *v.store.Path, path)
+	} else { // KV v2 is used
+		if v.store.Path == nil && !strings.Contains(path, "data") {
+			return "", fmt.Errorf(errPathInvalid)
+		}
+		if v.store.Path == nil {
+			path = strings.Replace(path, "data", "metadata", 1)
+			url = path
+		} else {
+			url = fmt.Sprintf("%s/metadata/%s", *v.store.Path, path)
+		}
 	}
 	return url, nil
 }

pkg/provider/vault/vault_test.go

@@ -1685,7 +1685,21 @@ func TestDeleteSecret(t *testing.T) {
 		want   want
 		value  []byte
 	}{
-		"DeleteSecretNoOp": {
+		"DeleteSecretNoOpKV1": {
+			reason: "No secret is because it does not exist",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV1).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(nil, nil),
+					WriteWithContextFn:        fake.ExpectWriteWithContextNoCall(),
+					DeleteWithContextFn:       fake.ExpectDeleteWithContextNoCall(),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"DeleteSecretNoOpKV2": {
 			reason: "No secret is because it does not exist",
 			args: args{
 				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
@@ -1699,7 +1713,21 @@ func TestDeleteSecret(t *testing.T) {
 				err: nil,
 			},
 		},
-		"DeleteSecretFailIfError": {
+		"DeleteSecretFailIfErrorKV1": {
+			reason: "No secret is because it does not exist",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV1).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"),
+			},
+		},
+		"DeleteSecretFailIfErrorKV2": {
 			reason: "No secret is because it does not exist",
 			args: args{
 				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
@@ -1713,7 +1741,26 @@ func TestDeleteSecret(t *testing.T) {
 				err: fmt.Errorf("failed to read"),
 			},
 		},
-		"DeleteSecretNotManaged": {
+		"DeleteSecretNotManagedKV1": {
+			reason: "No secret is because it does not exist",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV1).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(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,
+			},
+		},
+		"DeleteSecretNotManagedKV2": {
 			reason: "No secret is because it does not exist",
 			args: args{
 				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
@@ -1734,7 +1781,26 @@ func TestDeleteSecret(t *testing.T) {
 				err: nil,
 			},
 		},
-		"DeleteSecretSuccess": {
+		"DeleteSecretSuccessKV1": {
+			reason: "No secret is because it does not exist",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV1).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(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,
+			},
+		},
+		"DeleteSecretSuccessKV2": {
 			reason: "No secret is because it does not exist",
 			args: args{
 				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
@@ -1755,7 +1821,26 @@ func TestDeleteSecret(t *testing.T) {
 				err: nil,
 			},
 		},
-		"DeleteSecretError": {
+		"DeleteSecretErrorKV1": {
+			reason: "No secret is because it does not exist",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV1).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(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"),
+			},
+		},
+		"DeleteSecretErrorKV2": {
 			reason: "No secret is because it does not exist",
 			args: args{
 				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
@@ -1776,7 +1861,32 @@ func TestDeleteSecret(t *testing.T) {
 				err: fmt.Errorf("failed to delete"),
 			},
 		},
-		"DeleteSecretUpdateProperty": {
+		"DeleteSecretUpdatePropertyKV1": {
+			reason: "Secret should only be updated if Property is set",
+			ref:    &testingfake.PushSecretData{RemoteKey: "secret", Property: "fake-key"},
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV1).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(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{}{
+						"foo": "bar",
+						"custom_metadata": map[string]interface{}{
+							"managed-by": "external-secrets",
+						}}),
+					DeleteWithContextFn: fake.ExpectDeleteWithContextNoCall(),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"DeleteSecretUpdatePropertyKV2": {
 			reason: "Secret should only be updated if Property is set",
 			ref:    &testingfake.PushSecretData{RemoteKey: "secret", Property: "fake-key"},
 			args: args{
@@ -1799,7 +1909,27 @@ func TestDeleteSecret(t *testing.T) {
 				err: nil,
 			},
 		},
-		"DeleteSecretIfNoOtherProperties": {
+		"DeleteSecretIfNoOtherPropertiesKV1": {
+			reason: "Secret should only be deleted if no other properties are set",
+			ref:    &testingfake.PushSecretData{RemoteKey: "secret", Property: "foo"},
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV1).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(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,
+			},
+		},
+		"DeleteSecretIfNoOtherPropertiesKV2": {
 			reason: "Secret should only be deleted if no other properties are set",
 			ref:    &testingfake.PushSecretData{RemoteKey: "secret", Property: "foo"},
 			args: args{
@@ -1851,7 +1981,6 @@ func TestDeleteSecret(t *testing.T) {
 func TestPushSecret(t *testing.T) {
 	secretKey := "secret-key"
 	noPermission := errors.New("no permission")
-
 	type args struct {
 		store    *esv1beta1.VaultProvider
 		vLogical util.Logical
@@ -1867,7 +1996,20 @@ func TestPushSecret(t *testing.T) {
 		data   *testingfake.PushSecretData
 		value  []byte
 	}{
-		"SetSecret": {
+		"SetSecretKV1": {
+			reason: "secret is successfully set, with no existing vault secret",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV1).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(nil, nil),
+					WriteWithContextFn:        fake.NewWriteWithContextFn(nil, nil),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"SetSecretKV2": {
 			reason: "secret is successfully set, with no existing vault secret",
 			args: args{
 				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
@@ -1880,8 +2022,20 @@ func TestPushSecret(t *testing.T) {
 				err: nil,
 			},
 		},
-
-		"SetSecretWithWriteError": {
+		"SetSecretWithWriteErrorKV1": {
+			reason: "secret cannot be pushed if write fails",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV1).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(nil, nil),
+					WriteWithContextFn:        fake.NewWriteWithContextFn(nil, noPermission),
+				},
+			},
+			want: want{
+				err: noPermission,
+			},
+		},
+		"SetSecretWithWriteErrorKV2": {
 			reason: "secret cannot be pushed if write fails",
 			args: args{
 				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
@@ -1894,8 +2048,24 @@ func TestPushSecret(t *testing.T) {
 				err: noPermission,
 			},
 		},
-
-		"SetSecretEqualsPushSecret": {
+		"SetSecretEqualsPushSecretV1": {
+			reason: "vault secret kv equals secret to push kv",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV1).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
+						"fake-key": "fake-value",
+						"custom_metadata": map[string]interface{}{
+							"managed-by": "external-secrets",
+						},
+					}, nil),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"SetSecretEqualsPushSecretV2": {
 			reason: "vault secret kv equals secret to push kv",
 			args: args{
 				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
@@ -1914,7 +2084,33 @@ func TestPushSecret(t *testing.T) {
 				err: nil,
 			},
 		},
-		"PushSecretProperty": {
+		"PushSecretPropertyKV1": {
+			reason: "push secret with property adds the property",
+			value:  []byte("fake-value"),
+			data:   &testingfake.PushSecretData{SecretKey: secretKey, RemoteKey: "secret", Property: "foo"},
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV1).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
+						"fake-key": "fake-value",
+						"custom_metadata": map[string]interface{}{
+							"managed-by": "external-secrets",
+						},
+					}, nil),
+					WriteWithContextFn: fake.ExpectWriteWithContextValue(map[string]interface{}{
+						"fake-key": "fake-value",
+						"custom_metadata": map[string]string{
+							"managed-by": "external-secrets",
+						},
+						"foo": "fake-value",
+					}),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"PushSecretPropertyKV2": {
 			reason: "push secret with property adds the property",
 			value:  []byte("fake-value"),
 			data:   &testingfake.PushSecretData{SecretKey: secretKey, RemoteKey: "secret", Property: "foo"},
@@ -1936,7 +2132,32 @@ func TestPushSecret(t *testing.T) {
 				err: nil,
 			},
 		},
-		"PushSecretUpdateProperty": {
+		"PushSecretUpdatePropertyKV1": {
+			reason: "push secret with property only updates the property",
+			value:  []byte("new-value"),
+			data:   &testingfake.PushSecretData{SecretKey: secretKey, RemoteKey: "secret", Property: "foo"},
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV1).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
+						"foo": "fake-value",
+						"custom_metadata": map[string]interface{}{
+							"managed-by": "external-secrets",
+						},
+					}, nil),
+					WriteWithContextFn: fake.ExpectWriteWithContextValue(map[string]interface{}{
+						"foo": "new-value",
+						"custom_metadata": map[string]string{
+							"managed-by": "external-secrets",
+						},
+					}),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"PushSecretUpdatePropertyKV2": {
 			reason: "push secret with property only updates the property",
 			value:  []byte("new-value"),
 			data:   &testingfake.PushSecretData{SecretKey: secretKey, RemoteKey: "secret", Property: "foo"},
@@ -1958,7 +2179,27 @@ func TestPushSecret(t *testing.T) {
 				err: nil,
 			},
 		},
-		"PushSecretPropertyNoUpdate": {
+		"PushSecretPropertyNoUpdateKV1": {
+			reason: "push secret with property only updates the property",
+			value:  []byte("fake-value"),
+			data:   &testingfake.PushSecretData{SecretKey: secretKey, RemoteKey: "secret", Property: "foo"},
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV1).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
+						"foo": "fake-value",
+						"custom_metadata": map[string]interface{}{
+							"managed-by": "external-secrets",
+						},
+					}, nil),
+					WriteWithContextFn: fake.ExpectWriteWithContextNoCall(),
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"PushSecretPropertyNoUpdateKV2": {
 			reason: "push secret with property only updates the property",
 			value:  []byte("fake-value"),
 			data:   &testingfake.PushSecretData{SecretKey: secretKey, RemoteKey: "secret", Property: "foo"},
@@ -1980,8 +2221,19 @@ func TestPushSecret(t *testing.T) {
 				err: nil,
 			},
 		},
-
-		"SetSecretErrorReadingSecret": {
+		"SetSecretErrorReadingSecretKV1": {
+			reason: "error occurs if secret cannot be read",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV1).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(nil, noPermission),
+				},
+			},
+			want: want{
+				err: fmt.Errorf(errReadSecret, noPermission),
+			},
+		},
+		"SetSecretErrorReadingSecretKV2": {
 			reason: "error occurs if secret cannot be read",
 			args: args{
 				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,
@@ -1993,8 +2245,24 @@ func TestPushSecret(t *testing.T) {
 				err: fmt.Errorf(errReadSecret, noPermission),
 			},
 		},
-
-		"SetSecretNotManagedByESO": {
+		"SetSecretNotManagedByESOV1": {
+			reason: "a secret not managed by ESO cannot be updated",
+			args: args{
+				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV1).Spec.Provider.Vault,
+				vLogical: &fake.Logical{
+					ReadWithDataWithContextFn: fake.NewReadWithContextFn(map[string]interface{}{
+						"fake-key": "fake-value2",
+						"custom_metadata": map[string]interface{}{
+							"managed-by": "not-external-secrets",
+						},
+					}, nil),
+				},
+			},
+			want: want{
+				err: errors.New("secret not managed by external-secrets"),
+			},
+		},
+		"SetSecretNotManagedByESOV2": {
 			reason: "a secret not managed by ESO cannot be updated",
 			args: args{
 				store: makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV2).Spec.Provider.Vault,