Mount flag syntax to mitigate confusion from KV-v2 path discrepancies

[digivava]

Summary

This alternative syntax for the vault kv commands hopes to relieve some of the confusion caused by the fact that the current way to refer to a secret in the KV CLI looks like the path to the secret but isn't.

The old syntax (vault kv get secret/foo) will still work, but now users will also be able to do vault kv get -mount=secret foo with the same result. This is intended to be the way we will present the command in our Getting Started guides and other docs going forward.

Background

The vault kv commands, often one of the first commands that users of Vault get to experience, currently accept only a “shortened” form of the secret path (secret/foo), and automatically append /data behind the scenes when working with KV-V2 (secret/data/foo).

This behavior was originally included so that the migrations from Vault KV-V1 (where the path was secret/foo) to KV-V2 (where the path is secret/data/foo) would be easier. However, with most developers now starting with KV-V2 due to its inclusion in the dev server and Getting Started guide, the initial usefulness of this behavior has turned into a confusing pain point, because everywhere else (the policy HCL file, client libraries, the API path accessed directly), users need to manually include /data/ in the path.

This often results in one of two scenarios:

1. A user runs vault kv put secret/foo and is then confused why their policies and API calls referencing the same path don't work.

or

2. A user runs vault kv put secret/data/foo to match their policy doc, and is then confused by subsequent 404 errors because they actually created a secret at path secret/data/data/foo thanks to the automatic appending.

[averche]

This looks great so far, left a few comments, mostly style, so feel free to ignore them.

[averche]

LGTM 👍 Added a small suggestion for refactoring which could make this code cleaner. If it doesn't fit, feel free to ignore.

[averche]

I've been thinking how to refactor this a bit to reduce the code duplication. This is what I have so far, not sure if it will work, so feel free to ignore:

func splitPaths(...) (mountPath, partialPath, fullPath string) {
	if c.flagMount != "" {
		// In this case, this arg is the secret path (e.g. "foo").
		partialPath = sanitizePath(args[0])
		mountPath = sanitizePath(c.flagMount)
		fullPath = path.Join(mountPath, partialPath)
	} else {
		// In this case, this arg is a path-like combination of mountPath/secretPath.
		// (e.g. "secret/foo")
		partialPath = sanitizePath(args[0])
		mountPath = ""
                fullPath = partialPath
	}

	return mountPath, partialPath, fullPath 
}

Then in the function code:

      mountPath, partialPath, fullPath := splitPaths(...)
      
      mountPath, v2, err := isKVv2(path, client)
      if err != nil {
           c.UI.Error(err.Error())
           return 2
       }

[digivava]

Thanks for thinking about it! Unfortunately, the "fullPath" isn't so simple. fullPath is only equal to partialPath when it's KV v1, so we need the isKVv2 call to happen first to determine fullPath. Because of that I tried squishing the isKVv2 call into something like the splitPaths function before, but that failed me when it came to some of the other KV commands, which require fullPath to come from a different kind of logic. (Fun fact: There's more than just /data and /metadata.... see KV Delete, Destroy, and Undelete!)

I think I will go with what I've got for now and if something flexible suddenly comes to mind we can make a new PR for that.