Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix reading Vault KVv2 secrets metadata #1399

Merged
merged 2 commits into from
Jul 27, 2020

Conversation

findkim
Copy link
Contributor

@findkim findkim commented Jul 24, 2020

Fixes reading Vault KVv2 secrets metadata by not prepending /data/ path.

Test results before change
=== RUN   TestVaultReadQuery_Fetch_KVv2/read_metadata
    vault_read_test.go:407: 
        	Error Trace:	vault_read_test.go:407
        	Error:      	Received unexpected error:
        	            	no secret exists at read_fetch_v2/data/metadata/foo/bar
        	            	vault.read(read_fetch_v2/metadata/foo/bar)
        	            	github.com/hashicorp/consul-template/dependency.(*VaultReadQuery).Fetch
        	            		/Users/kngo/dev/hashicorp/consul-template/dependency/vault_read.go:80
        	            	github.com/hashicorp/consul-template/dependency.TestVaultReadQuery_Fetch_KVv2.func2
        	            		/Users/kngo/dev/hashicorp/consul-template/dependency/vault_read_test.go:406
        	            	testing.tRunner
        	            		/usr/local/go/src/testing/testing.go:1039
        	            	runtime.goexit
        	            		/usr/local/go/src/runtime/asm_amd64.s:1373
        	Test:       	TestVaultReadQuery_Fetch_KVv2/read_metadata

Resolves #1396

@findkim findkim added the bug label Jul 24, 2020
@findkim findkim added this to the 0.25.1 milestone Jul 24, 2020
@findkim findkim requested a review from a team July 24, 2020 17:44
The changes to fix KVv2 paths with datafoo as a subpath caused
the metadata path to break by prepending the /data/ path to it.
@eikenb
Copy link
Contributor

eikenb commented Jul 24, 2020

While this looks good I did come up with a suggestion. While looking at the code it bugged me a bit that "metadata" was hardcoded in there but I realized it wasn't that that which bothered me but rather that it was that apiPrefix's value wasn't. The addPrefixToVKVPath function is called exactly once, in vault_read.go, and passed with apiPrefix set to "data".

My suggestion is then to remove the apiPrefix argument and hardcode "data" in there. Also maybe even move it into vault_read.go, the only place it is used.

What do you think?

@findkim
Copy link
Contributor Author

findkim commented Jul 24, 2020

@eikenb I'm glad you brought that up! I'm been fighting with preserving the existing code/logic. The apiPrefix parameter makes the function more obscure to handle and read. I can update this now

addPrefixToVKVPath is only used once and had the "data" prefix
passed as a parameter which required additional logic obscuring the
underlying goal. This moves and rewords the function to read more
clear.
@findkim findkim requested a review from eikenb July 24, 2020 21:21
Copy link
Contributor

@eikenb eikenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great and I like the new name.

@eikenb eikenb merged commit b11fa80 into hashicorp:master Jul 27, 2020
@findkim findkim deleted the fix-vault-metadata branch July 27, 2020 19:42
@eikenb eikenb added hashicat-update-required Changes that need to be ported to hashicat hashicat-update-complete Completed porting changes to hashicat labels Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug hashicat-update-complete Completed porting changes to hashicat hashicat-update-required Changes that need to be ported to hashicat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to read Vault KVv2 secrets metadata
2 participants