:bug: Implements new buildPath logic (#1636)

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

pkg/provider/vault/vault.go

@@ -693,31 +693,79 @@ func (v *client) buildMetadataPath(path string) (string, error) {
 	}
 	return url, nil
 }
+
+/*
+	 buildPath is a helper method to build the vault equivalent path
+		 from ExternalSecrets and SecretStore manifests. the path build logic
+		 varies depending on the SecretStore KV version:
+		 Example inputs/outputs:
+		 # simple build:
+		 kv version == "v2":
+			provider_path: "secret/path"
+			input: "foo"
+			output: "secret/path/data/foo" # provider_path and data are prepended
+		 kv version == "v1":
+			provider_path: "secret/path"
+			input: "foo"
+			output: "secret/path/foo" # provider_path is prepended
+		 # inheriting paths:
+		 kv version == "v2":
+			provider_path: "secret/path"
+			input: "secret/path/foo"
+			output: "secret/path/data/foo" #data is prepended
+		 kv version == "v2":
+			provider_path: "secret/path"
+			input: "secret/path/data/foo"
+			output: "secret/path/data/foo" #noop
+		 kv version == "v1":
+			provider_path: "secret/path"
+			input: "secret/path/foo"
+			output: "secret/path/foo" #noop
+		 # provider path not defined:
+		 kv version == "v2":
+			provider_path: nil
+			input: "secret/path/foo"
+			output: "secret/data/path/foo" # data is prepended to secret/
+		 kv version == "v2":
+			provider_path: nil
+			input: "secret/path/data/foo"
+			output: "secret/path/data/foo" #noop
+		 kv version == "v1":
+			provider_path: nil
+			input: "secret/path/foo"
+			output: "secret/path/foo" #noop
+*/
 func (v *client) buildPath(path string) string {
 	optionalMount := v.store.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 == esv1beta1.VaultKVStoreV2 {
-		// Add the required `data` part of the URL for the v2 API
-		if len(origPath) < 2 || origPath[1] != "data" {
-			newPath = append(newPath, "data")
+	out := path
+	// if optionalMount is Set, remove it from path if its there
+	if optionalMount != nil {
+		cut := *optionalMount + "/"
+		if strings.HasPrefix(out, cut) {
+			// This current logic induces a bug when the actual secret resides on same path names as the mount path.
+			_, out, _ = strings.Cut(out, cut)
+			// if data succeeds optionalMount on v2 store, we should remove it as well
+			if strings.HasPrefix(out, "data/") && v.store.Version == esv1beta1.VaultKVStoreV2 {
+				_, out, _ = strings.Cut(out, "data/")
+			}
 		}
-	}
-	newPath = append(newPath, origPath[cursor:]...)
-	returnPath := strings.Join(newPath, "/")
-
-	return returnPath
+		buildPath := strings.Split(out, "/")
+		buildMount := strings.Split(*optionalMount, "/")
+		if v.store.Version == esv1beta1.VaultKVStoreV2 {
+			buildMount = append(buildMount, "data")
+		}
+		buildMount = append(buildMount, buildPath...)
+		out = strings.Join(buildMount, "/")
+		return out
+	}
+	if !strings.Contains(out, "/data/") && v.store.Version == esv1beta1.VaultKVStoreV2 {
+		buildPath := strings.Split(out, "/")
+		buildMount := []string{buildPath[0], "data"}
+		buildMount = append(buildMount, buildPath[1:]...)
+		out = strings.Join(buildMount, "/")
+		return out
+	}
+	return out
 }
 
 func (v *client) readSecret(ctx context.Context, path, version string) (map[string]interface{}, error) {

pkg/provider/vault/vault_test.go

@@ -1140,10 +1140,13 @@ func TestGetAllSecrets(t *testing.T) {
 func TestGetSecretPath(t *testing.T) {
 	storeV2 := makeValidSecretStore()
 	storeV2NoPath := storeV2.DeepCopy()
+	multiPath := "secret/path"
+	storeV2.Spec.Provider.Vault.Path = &multiPath
 	storeV2NoPath.Spec.Provider.Vault.Path = nil
 
 	storeV1 := makeValidSecretStoreWithVersion(esv1beta1.VaultKVStoreV1)
 	storeV1NoPath := storeV1.DeepCopy()
+	storeV1.Spec.Provider.Vault.Path = &multiPath
 	storeV1NoPath.Spec.Provider.Vault.Path = nil
 
 	type args struct {
@@ -1156,39 +1159,47 @@ func TestGetSecretPath(t *testing.T) {
 		args   args
 	}{
 		"PathWithoutFormatV2": {
-			reason: "Data needs to be found in path",
+			reason: "path should compose with mount point if set",
 			args: args{
 				store:    storeV2.Spec.Provider.Vault,
-				path:     "secret/test",
-				expected: "secret/data/test",
+				path:     "secret/path/data/test",
+				expected: "secret/path/data/test",
 			},
 		},
-		"PathWithDataV2": {
-			reason: "Data needs to be found only once in path",
+		"PathWithoutFormatV2_NoData": {
+			reason: "path should compose with mount point if set without data",
 			args: args{
 				store:    storeV2.Spec.Provider.Vault,
-				path:     "secret/data/test",
-				expected: "secret/data/test",
+				path:     "secret/path/test",
+				expected: "secret/path/data/test",
 			},
 		},
 		"PathWithoutFormatV2_NoPath": {
-			reason: "Data needs to be found in path and correct mountpoint is set",
+			reason: "if no mountpoint and no data available, needs to be set in second element",
 			args: args{
 				store:    storeV2NoPath.Spec.Provider.Vault,
-				path:     "secret/test",
-				expected: "secret/data/test",
+				path:     "secret/test/big/path",
+				expected: "secret/data/test/big/path",
+			},
+		},
+		"PathWithoutFormatV2_NoPathWithData": {
+			reason: "if data is available, should respect order",
+			args: args{
+				store:    storeV2NoPath.Spec.Provider.Vault,
+				path:     "secret/test/data/not/the/first/and/data/twice",
+				expected: "secret/test/data/not/the/first/and/data/twice",
 			},
 		},
 		"PathWithoutFormatV1": {
-			reason: "Data needs to be found in path",
+			reason: "v1 mountpoint should be added but not enforce 'data'",
 			args: args{
 				store:    storeV1.Spec.Provider.Vault,
-				path:     "secret/test",
-				expected: "secret/test",
+				path:     "secret/path/test",
+				expected: "secret/path/test",
 			},
 		},
 		"PathWithoutFormatV1_NoPath": {
-			reason: "Data needs to be found in path and correct mountpoint is set",
+			reason: "Should not append any path information if v1 with no mountpoint",
 			args: args{
 				store:    storeV1NoPath.Spec.Provider.Vault,
 				path:     "secret/test",
@@ -1200,7 +1211,7 @@ func TestGetSecretPath(t *testing.T) {
 			args: args{
 				store:    storeV2.Spec.Provider.Vault,
 				path:     "test",
-				expected: "secret/data/test",
+				expected: "secret/path/data/test",
 			},
 		},
 		"WithoutPathButMountpointV1": {
@@ -1208,7 +1219,7 @@ func TestGetSecretPath(t *testing.T) {
 			args: args{
 				store:    storeV1.Spec.Provider.Vault,
 				path:     "test",
-				expected: "secret/test",
+				expected: "secret/path/test",
 			},
 		},
 	}