-
Notifications
You must be signed in to change notification settings - Fork 784
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
Ignore anonymous request failures checking kv status #1231
Conversation
When making unauthenticated secret lookup requests, ignore errors that result from checking the secret mount type. If it's a persistent error, the error on the actual lookup. This restores behavior to before hashicorp#1180 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test introduced in 9dc0e8d fails initially but passes after test:
$ git checkout 9dc0e8d738ecd466ad6761d580a41cbe463b1126
Previous HEAD position was ae80c54d Ignore anonymous request failures checking kv status
HEAD is now at 9dc0e8d7 add a test for anonymous access of pki ca cert
$ make test
==> Testing consul-template
ok github.com/hashicorp/consul-template 2.502s
ok github.com/hashicorp/consul-template/child 1.263s
ok github.com/hashicorp/consul-template/config 0.126s
2019/07/16 15:42:22 CONFIG JSON: {"node_name":"node-3696ae10-0ebe-f37b-234b-95ede3838c0c","node_id":"3696ae10-0ebe-f37b-234b-95ede3838c0c","performance":{"raft_multiplier":1},"bootstrap":true,"server":true,"data_dir":"/tmp/consul-test/consul360095124/data","segments":null,"disable_update_check":true,"log_level":"warn","bind_addr":"127.0.0.1","addresses":{},"ports":{"dns":35501,"http":35502,"https":35503,"serf_lan":35504,"serf_wan":35505,"server":35506},"acl_enforce_version_8":false,"connect":{"ca_config":{"cluster_id":"11111111-2222-3333-4444-555555555555"},"enabled":true,"proxy":{"allow_managed_api_registration":true}}}
--- FAIL: TestFileQuery_Fetch (0.01s)
--- FAIL: TestFileQuery_Fetch/fires_changes (0.00s)
Error Trace: file_test.go:184
Error: Not equal: "" (expected)
!= "goodbye" (actual)
--- FAIL: TestVaultReadQuery_Fetch_PKI_Anonymous (4.23s)
vault_read_test.go:450: vault.read(pki/cert/ca): vault.read(pki/cert/ca): Error making API request.
URL: GET http://127.0.0.1:57240/v1/sys/internal/ui/mounts/pki/cert/ca
Code: 500. Errors:
* missing client token
FAIL
FAIL github.com/hashicorp/consul-template/dependency 6.773s
ok github.com/hashicorp/consul-template/logging 0.035s
ok github.com/hashicorp/consul-template/manager 4.066s
ok github.com/hashicorp/consul-template/renderer 0.082s
ok github.com/hashicorp/consul-template/signals 0.029s
ok github.com/hashicorp/consul-template/template 0.096s
? github.com/hashicorp/consul-template/test [no test files]
? github.com/hashicorp/consul-template/version [no test files]
ok github.com/hashicorp/consul-template/watch 0.483s
make: *** [test] Error 1
With the fix
$ git checkout ae80c54d6e244f46581053dafa81d6edf9986787
Previous HEAD position was 9dc0e8d7 add a test for anonymous access of pki ca cert
HEAD is now at ae80c54d Ignore anonymous request failures checking kv status
$ make test
==> Testing consul-template
ok github.com/hashicorp/consul-template 2.164s
ok github.com/hashicorp/consul-template/child 1.180s
ok github.com/hashicorp/consul-template/config 0.114s
ok github.com/hashicorp/consul-template/dependency 6.349s
ok github.com/hashicorp/consul-template/logging 0.035s
ok github.com/hashicorp/consul-template/manager 4.319s
ok github.com/hashicorp/consul-template/renderer 0.099s
ok github.com/hashicorp/consul-template/signals 0.031s
ok github.com/hashicorp/consul-template/template 0.355s
? github.com/hashicorp/consul-template/test [no test files]
? github.com/hashicorp/consul-template/version [no test files]
ok github.com/hashicorp/consul-template/watch 0.987s
if client.Token() == "" { | ||
return "", false, nil | ||
} | ||
|
||
return "", false, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've considered always suppressing errors, so we fallback to pre-#1180 behavior, but was worried about transient errors. It will not be ideal if clients inconsistently wrap lookups as KVv2 and not, when hitting transient errors in this call; and debugging that will be very hard. Open for better suggestions.
Hey @notnoop, thanks for the PR. Running this test locally I found it failed every time with...
Thinking about it I realized it was because I had VAULT_TOKEN set as an environment variable as part of my setup for some other testing. The vault client sets the client's token automatically if it finds this environment variable even if the token in it is not used or valid. The documentation is pretty clear that setting the vault token is a... meaningful act and, as such, it conflicting with an alternative system isn't necessarily wrong. I just wanted to note this so you'd be aware and to be sure you had taken it into account. Thanks. |
@eikenb yes - I considered always suppressing errors from isKVv2 (or 403 errors specifically) and treating it like a kv1 lookup. I decided against it in this PR for fear of transient errors. Consider the case where consul-template process starts with lack of acl access to a specific secret (hence getting 403) but then granted access; the process would still access it as a KVv1 while newly launched processes will access as KVv2 secret. I can come with few similar edge casing scenarios. We can revamp how to do caching isKVv2 or being more finegrained in how to handle errors, but that's beyond the scope of our immediate need so I'd punt on it for another PR if the need arise. |
Great! Thanks @notnoop for addressing my comment so fast. I'll probably go ahead and merge this tomorrow so I can rework the test as I just completely re-worked the vault tests in #1230. But I should be able to rework that test quickly as I just finished with that one and know what needs to be done. |
To pick up the fix in hashicorp/consul-template#1231 .
To pick up the fix in hashicorp/consul-template#1231 .
When making unauthenticated secret lookup requests, ignore errors that
result from checking the secret mount type. If it's a persistent error,
the error on the actual lookup.
This restores behavior prior to #1180 .