Made SecretStore path for Vault optional

  * Backwards compatible change
  * Added tests to check for a range of possible combinations for paths

apis/externalsecrets/v1alpha1/secretstore_vault_types.go

@@ -63,7 +63,8 @@ type VaultProvider struct {
 	// "secret". The v2 KV secret engine version specific "/data" path suffix
 	// for fetching secrets from Vault is optional and will be appended
 	// if not present in specified path.
-	Path string `json:"path"`
+	// +optional
+	Path *string `json:"path"`
 
 	// Version is the Vault KV secret engine version. This can be either "v1" or
 	// "v2". Version defaults to "v2".

e2e/suite/vault/provider.go

@@ -48,6 +48,10 @@ const (
 	kubernetesProviderName  = "kubernetes-provider"
 )
 
+var (
+	secretStorePath string = "secret"
+)
+
 func newVaultProvider(f *framework.Framework) *vaultProvider {
 	prov := &vaultProvider{
 		framework: f,
@@ -104,7 +108,7 @@ func makeStore(name, ns string, v *addon.Vault) *esv1alpha1.SecretStore {
 			Provider: &esv1alpha1.SecretStoreProvider{
 				Vault: &esv1alpha1.VaultProvider{
 					Version:  esv1alpha1.VaultKVStoreV2,
-					Path:     "secret",
+					Path:     &secretStorePath,
 					Server:   v.VaultURL,
 					CABundle: v.VaultServerCA,
 				},
@@ -215,8 +219,9 @@ func (s vaultProvider) CreateV1Store(v *addon.Vault, ns string) {
 	err := s.framework.CRClient.Create(context.Background(), vaultCreds)
 	Expect(err).ToNot(HaveOccurred())
 	secretStore := makeStore(kvv1ProviderName, ns, v)
+	secretV1StorePath := "secret_v1"
 	secretStore.Spec.Provider.Vault.Version = esv1alpha1.VaultKVStoreV1
-	secretStore.Spec.Provider.Vault.Path = "secret_v1"
+	secretStore.Spec.Provider.Vault.Path = &secretV1StorePath
 	secretStore.Spec.Provider.Vault.Auth = esv1alpha1.VaultAuth{
 		TokenSecretRef: &esmeta.SecretKeySelector{
 			Name: "v1-provider",

pkg/provider/vault/vault.go

@@ -176,19 +176,42 @@ func (v *client) Close(ctx context.Context) error {
 	return nil
 }
 
-func (v *client) readSecret(ctx context.Context, path, version string) (map[string][]byte, error) {
-	kvPath := v.store.Path
+func (v *client) buildPath(ctx context.Context, path string) string {
+	optionalMount := v.store.Path
+	returnPath := path
+
+	origPath := strings.Split(path, "/")
+	newPath := make([]string, 0)
+	cursor := 0
+
+	if optionalMount != nil && origPath[0] != *optionalMount {
+		// Default case before path was optional
+		// Ensure that the requested path includes the SecretStores paths as prefix
+		newPath = append(newPath, *optionalMount)
+	} else {
+		newPath = append(newPath, origPath[cursor])
+		cursor++
+	}
 
 	if v.store.Version == esv1alpha1.VaultKVStoreV2 {
-		if !strings.HasSuffix(kvPath, "/data") {
-			kvPath = fmt.Sprintf("%s/data", kvPath)
+		// Add the required `data` part of the URL for the v2 API
+		if len(origPath) < 2 || origPath[1] != "data" {
+			newPath = append(newPath, "data")
 		}
 	}
+	newPath = append(newPath, origPath[cursor:]...)
+	returnPath = strings.Join(newPath, "/")
+
+	return returnPath
+}
+
+func (v *client) readSecret(ctx context.Context, path, version string) (map[string][]byte, error) {
+	dataPath := v.buildPath(ctx, path)
 
 	// path formated according to vault docs for v1 and v2 API
 	// v1: https://www.vaultproject.io/api-docs/secret/kv/kv-v1#read-secret
 	// v2: https://www.vaultproject.io/api/secret/kv/kv-v2#read-secret-version
-	req := v.client.NewRequest(http.MethodGet, fmt.Sprintf("/v1/%s/%s", kvPath, path))
+	req := v.client.NewRequest(http.MethodGet, fmt.Sprintf("/v1/%s", dataPath))
 	if version != "" {
 		req.Params.Set("version", version)
 	}

pkg/provider/vault/vault_test.go

@@ -41,6 +41,10 @@ const (
 	secretDataString = "some-creds"
 )
 
+var (
+	secretStorePath string = "secret"
+)
+
 func makeValidSecretStoreWithVersion(v esv1alpha1.VaultKVStoreVersion) *esv1alpha1.SecretStore {
 	return &esv1alpha1.SecretStore{
 		ObjectMeta: metav1.ObjectMeta{
@@ -51,7 +55,7 @@ func makeValidSecretStoreWithVersion(v esv1alpha1.VaultKVStoreVersion) *esv1alph
 			Provider: &esv1alpha1.SecretStoreProvider{
 				Vault: &esv1alpha1.VaultProvider{
 					Server:  "vault.example.com",
-					Path:    "secret",
+					Path:    &secretStorePath,
 					Version: v,
 					Auth: esv1alpha1.VaultAuth{
 						Kubernetes: &esv1alpha1.VaultKubernetesAuth{
@@ -82,7 +86,7 @@ func makeValidSecretStoreWithCerts() *esv1alpha1.SecretStore {
 			Provider: &esv1alpha1.SecretStoreProvider{
 				Vault: &esv1alpha1.VaultProvider{
 					Server:  "vault.example.com",
-					Path:    "secret",
+					Path:    &secretStorePath,
 					Version: esv1alpha1.VaultKVStoreV2,
 					Auth: esv1alpha1.VaultAuth{
 						Cert: &esv1alpha1.VaultCertAuth{
@@ -674,3 +678,93 @@ func TestGetSecretMap(t *testing.T) {
 		})
 	}
 }
+
+func TestGetSecretPath(t *testing.T) {
+
+	storeV2 := makeValidSecretStore()
+	storeV2_NoPath := storeV2.DeepCopy()
+	storeV2_NoPath.Spec.Provider.Vault.Path = nil
+
+	storeV1 := makeValidSecretStoreWithVersion(esv1alpha1.VaultKVStoreV1)
+	storeV1_NoPath := storeV1.DeepCopy()
+	storeV1_NoPath.Spec.Provider.Vault.Path = nil
+
+	type args struct {
+		store    *esv1alpha1.VaultProvider
+		path     string
+		expected string
+	}
+	cases := map[string]struct {
+		reason string
+		args   args
+	}{
+		"PathWithoutFormatV2": {
+			reason: "Data needs to be found in path",
+			args: args{
+				store:    storeV2.Spec.Provider.Vault,
+				path:     "secret/test",
+				expected: "secret/data/test",
+			},
+		},
+		"PathWithDataV2": {
+			reason: "Data needs to be found only once in path",
+			args: args{
+				store:    storeV2.Spec.Provider.Vault,
+				path:     "secret/data/test",
+				expected: "secret/data/test",
+			},
+		},
+		"PathWithoutFormatV2_NoPath": {
+			reason: "Data needs to be found in path and correct mountpoint is set",
+			args: args{
+				store:    storeV2_NoPath.Spec.Provider.Vault,
+				path:     "secret/test",
+				expected: "secret/data/test",
+			},
+		},
+		"PathWithoutFormatV1": {
+			reason: "Data needs to be found in path",
+			args: args{
+				store:    storeV1.Spec.Provider.Vault,
+				path:     "secret/test",
+				expected: "secret/test",
+			},
+		},
+		"PathWithoutFormatV1_NoPath": {
+			reason: "Data needs to be found in path and correct mountpoint is set",
+			args: args{
+				store:    storeV1_NoPath.Spec.Provider.Vault,
+				path:     "secret/test",
+				expected: "secret/test",
+			},
+		},
+		"WithoutPathButMountpointV2": {
+			reason: "Mountpoint needs to be set in addition to data",
+			args: args{
+				store:    storeV2.Spec.Provider.Vault,
+				path:     "test",
+				expected: "secret/data/test",
+			},
+		},
+		"WithoutPathButMountpointV1": {
+			reason: "Mountpoint needs to be set in addition to data",
+			args: args{
+				store:    storeV1.Spec.Provider.Vault,
+				path:     "test",
+				expected: "secret/test",
+			},
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			vStore := &client{
+				store: tc.args.store,
+			}
+			want := vStore.buildPath(context.Background(), tc.args.path)
+			if diff := cmp.Diff(want, tc.args.expected); diff != "" {
+				t.Errorf("\n%s\nvault.buildPath(...): -want expected, +got error:\n%s", tc.reason, diff)
+			}
+		})
+	}
+}