-
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
consul: provide CONSUL_HTTP_TOKEN
env var to tasks
#20519
Conversation
When available, we provide an environment variable `CONSUL_TOKEN` to tasks, but this isn't the environment variable expected by the Consul CLI. Job specifications like deploying an API Gateway become noticeably nicer if we can instead provide the expected env var.
@@ -79,7 +79,8 @@ func (h *consulHook) Prestart(ctx context.Context, req *interfaces.TaskPrestartR | |||
} | |||
|
|||
env := map[string]string{ | |||
"CONSUL_TOKEN": token.SecretID, |
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.
note for reviewers: as I mention in the docs update below, we're leaving this old variable for backwards compatibility.
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.
just the one compatibility / escape hatch question, otherwise LGTM!
@@ -79,7 +79,8 @@ func (h *consulHook) Prestart(ctx context.Context, req *interfaces.TaskPrestartR | |||
} | |||
|
|||
env := map[string]string{ | |||
"CONSUL_TOKEN": token.SecretID, | |||
"CONSUL_TOKEN": token.SecretID, | |||
"CONSUL_HTTP_TOKEN": token.SecretID, |
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.
trying to remember the order of operations -- I hope this could be overridden by the user with an explicit env{}
block, or a template{}
, and/or... other?
basically I think it's good to start adding this, but if folks are already setting it on their own to something that would be different from this, or if they want to unset it for whatever reason, the former should continue working, and the latter should be available.
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.
Having a template
block when Nomad is configured to use Workload Identity with Consul is what causes us to hit this code path in the first place. But the template hook runs after this hook so it will definitely override it if you've decided to, for example, fetch a time-limited Consul token from Vault.
This will overwrite a hard-coded env
block field, but there's no recursive environment interpolation there, so it has to be a literal token string. Once you've got WI set up (again, that's what triggers this code path), there's no reason to do that and it's incredibly insecure to boot.
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.
(Also note that even if you could unset the env var, the token is written to /secrets
, so it's not like this is a security issue where a job submitter can hide the token from the task.)
When available, we provide an environment variable
CONSUL_TOKEN
to tasks, but this isn't the environment variable expected by the Consul CLI. Job specifications like deploying an API Gateway become noticeably nicer if we can instead provide the expected env var.