Skip to content

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

Open
wants to merge 2 commits into
base: f-NMD-763-identity
Choose a base branch
from

Conversation

jrasell
Copy link
Member

@jrasell jrasell commented Jul 8, 2025

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 If this PR changes user-facing behavior, please generate and add a
    changelog entry using the make cl command.
  • Testing Please add tests to cover any new functionality or to demonstrate bug fixes and
    ensure regressions will be caught.
  • Documentation If the change impacts user-facing functionality such as the CLI, API, UI,
    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

  • Backport Labels Please add the correct backport labels as described by the internal
    backporting document.
  • Commit Type Ensure the correct merge method is selected which should be "squash and merge"
    in the majority of situations. The main exceptions are long-lived feature branches or merges where
    history should be preserved.
  • Enterprise PRs If this is an enterprise only PR, please add any required changelog entry
    within the public repository.

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.
@jrasell jrasell self-assigned this Jul 8, 2025
@jrasell jrasell marked this pull request as ready for review July 8, 2025 09:31
@jrasell jrasell requested review from a team as code owners July 8, 2025 09:31
Copy link
Member

@tgross tgross left a 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!)

// satisfies the NodeIdentityHandler interface and calls SetNodeIdentityToken if
// it does.
func assertAndSetNodeIdentityToken(impl any, token string) {
if handler, ok := impl.(NodeIdentityHandler); ok {
Copy link
Member

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)

Copy link
Member

@mismithhisler mismithhisler Jul 9, 2025

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?

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants