client: split identity_hook across allocrunner and taskrunner #18431
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pkazmierczak
changed the title
pkazmierczak
force-pushed
the
f-consul-wi-consul_hook
branch
from
78a575c to
b959429
Compare
tgross
reviewed
Sep 14, 2023
tgross
reviewed
Sep 20, 2023
client/widmgr/widmgr.go
Outdated
| } | ||
|
|
||
| renewNow := false | ||
| minExp := time.Now().Add(30 * time.Hour) // set high default expiration |
There was a problem hiding this comment.
I like the idea of being able to bail out of the renew loop entirely. That implies downstream consumers will need to either Get or Watch based on whether they have a TTL, but I think that's ok.
tgross
approved these changes
Sep 21, 2023
There was a problem hiding this comment.
@pkazmierczak I've left some minor remaining comments but the only one that seems potentially blocking are the ones around shutdown and double-closing the channels. Good-to-merge once that's fixed.
lgfa29
added a commit
that referenced
this pull request
Sep 27, 2023
The initial intention behind the `vault.use_identity` configuration was to indicate to Nomad servers that they would need to sign a workload identities for allocs with a `vault` block. But in order to support identity renewal, #18262 and #18431 moved the token signing logic to the alloc runner since a new token needs to be signed prior to the TTL expiring. So #18343 implemented `use_identity` as a flag to indicate that the workload identity JWT flow should be used when deriving Vault tokens for tasks. But this configuration value is set on servers so it is not available to clients at the time of token derivation, making its meaning not clear: a job may end up using the identity-based flow even when `use_identity` is `false`. The only reliable signal available to clients at token derivation time is the presence of an `identity` block for Vault, and this is already configured with the `vault.default_identity` configuration block, making `vault.use_identity` redundant. This commit removes the `vault.use_identity` configuration and simplifies the logic on when an implicit Vault identity is injected into tasks.
lgfa29
added a commit
that referenced
this pull request
Sep 27, 2023
The initial intention behind the `vault.use_identity` configuration was to indicate to Nomad servers that they would need to sign a workload identities for allocs with a `vault` block. But in order to support identity renewal, #18262 and #18431 moved the token signing logic to the alloc runner since a new token needs to be signed prior to the TTL expiring. So #18343 implemented `use_identity` as a flag to indicate that the workload identity JWT flow should be used when deriving Vault tokens for tasks. But this configuration value is set on servers so it is not available to clients at the time of token derivation, making its meaning not clear: a job may end up using the identity-based flow even when `use_identity` is `false`. The only reliable signal available to clients at token derivation time is the presence of an `identity` block for Vault, and this is already configured with the `vault.default_identity` configuration block, making `vault.use_identity` redundant. This commit removes the `vault.use_identity` configuration and simplifies the logic on when an implicit Vault identity is injected into tasks.
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR splits
identity_hookbetween theallocrunnerandtaskrunner. Theallocrunner-level part of the hook signs each task identity, and thetaskrunner-level part picks it up and stores secrets for each task.This code revamps the
WIDMgr, which is now split into 2 interfaces:IdentityManagerwhich manages renewals of signatures and handles sending updates to subscribers viaWatchmethod, andIdentitySignerwhich only does the signing.This work is necessary for having a unified Consul login workflow that comes with the new Consul integration. A new,
allocrunner-levelconsul_hookwill now be the only hook doing Consul authentication.Ref: https://github.com/hashicorp/team-nomad/issues/404