-
Notifications
You must be signed in to change notification settings - Fork 2k
client: Handle identities from servers and use for RPC auth. #26218
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
base: f-NMD-763-identity
Are you sure you want to change the base?
Conversation
Nomad servers, if upgraded, can return node identities as part of the register and update/heartbeat response objects. The Nomad client will now handle this and store it as appropriate within its memory and statedb. The client will now use any stored identity for RPC authentication with a fallback to the secretID. Ths supports upgrades paths where the Nomad clients are updated before the Nomad servers.
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!
Seeing how this is playing out in the client is making me realize we're probably going to want a version guard in the ShouldGenerateNodeIdentity
because of a pathological case where a client and the leader are updated but a follower is not yet updated. In that scenario, you could have the client Node.Register
RPC get forwarded to the leader and receive an identity, which it then starts using on every request. But all the node's stale requests will go to the follower, which doesn't understand the identity. Kind of a corner case and one that can only be hit if you're doing your upgrade incorrectly. But worth thinking about whether we want to try to account for that in the client or the RPC handler (certainly fine to do in a separate PR!)
client/identity.go
Outdated
// satisfies the NodeIdentityHandler interface and calls SetNodeIdentityToken if | ||
// it does. | ||
func assertAndSetNodeIdentityToken(impl any, token string) { | ||
if handler, ok := impl.(NodeIdentityHandler); ok { |
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.
We're doing a runtime check of something that's a programmer's error: the set of types where we'll call SetNodeIdentityToken
is known at compile time. If we hit the !ok
path then we're silently failing to set the identity.
If we can't figure out a way to get the compiler to see that in the production code, we can make a type assertion like this in a test and it'll get checked when the tests compile:
var _ NodeIdentityHandler = (*nsd.ServiceRegistrationHandler)(nil)
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.
Out of curiosity, is there a reason why impl cannot be the NodeIdentityHandler
interface type?
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 added a change within e475522 using nil as a number of tests setup a thin client which can cause panics. The test file already had type assertion tests as you describe.
Out of curiosity, is there a reason why impl cannot be the NodeIdentityHandler interface type?
Technically the client is passing another interface implementation to the function and we assert it satisfies the additional interface. If we change the function signature, the caller would need to do the assertion otherwise Go's type system wouldn't allow the direct pass.
6c895a0
to
f18da52
Compare
f18da52
to
e475522
Compare
Nomad servers, if upgraded, can return node identities as part of the register and update/heartbeat response objects. The Nomad client will now handle this and store it as appropriate within its memory and statedb.
The client will now use any stored identity for RPC authentication with a fallback to the secretID. Ths supports upgrades paths where the Nomad clients are updated before the Nomad servers.
Links
internal jira: https://hashicorp.atlassian.net/browse/NMD-763
internal design doc: https://docs.google.com/document/d/1MYjlFlOAmGHmWGC3VsrIMUL_VSgKwjLAq6GUymDY38M/edit?tab=t.0
Contributor Checklist
changelog entry using the
make cl
command.ensure regressions will be caught.
and job configuration, please update the Nomad website documentation to reflect this. Refer to
the website README for docs guidelines. Please also consider whether the
change requires notes within the upgrade guide.
Reviewer Checklist
backporting document.
in the majority of situations. The main exceptions are long-lived feature branches or merges where
history should be preserved.
within the public repository.