-
Notifications
You must be signed in to change notification settings - Fork 2k
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
vault: load default config for tasks without vault #19439
Conversation
It is often expected that a task that needs access to Vault defines a `vault` block to specify the Vault policy to use to derive a token. But in some scenarios, like when the Nomad client is connected to a local Vault agent that is responsible for authn/authz, the task is not required to defined a `vault` block. In these situations, the `default` Vault cluster should be used to render the template.
5cef72f
to
bd0626e
Compare
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.
LGTM!
}() | ||
|
||
var gotRequest bool | ||
LOOP: |
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.
Because the send on reqCh
is blocking, at first I thought you could probably select over reqCh
and ctx
without this loop and check the error afterwards, and avoid the label/break/goto. But this is a nicely elegant way of avoiding blocking 3 whole seconds for the context timeout on error. 👍
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.
Yeah, this ensure we exit as soon as Prestart()
finishes. I think there's a theoretical issue where a test case reads a request from another test case, because reqCh
is shared while each test case has its own consumer. But since this is a test, and each test case doesn't run in parallel, I think it's fine.
It appears there's one small regression in this. Our configuration is spread across multiple files which are merged together using the Nomad configuration merge behavior
In Vault 1.6.x and up through Vault 1.7.1, the above behavior was working properly. With the introduction of Vault 1.7.2, I believe the merge behavior has changed because of the later file not specifying a Vault address such that a default address is used which then overwrites/hide the value specified in the earlier file. |
It is often expected that a task that needs access to Vault defines a
vault
block to specify the Vault policy to use to derive a token.But in some scenarios, like when the Nomad client is connected to a local Vault agent that is responsible for authn/authz, the task is not required to defined a
vault
block.In these situations, the
default
Vault cluster should be used to render the template.Closes #19380