Skip to content
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

identity: version check multiple and implicit identities #18926

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

tgross
Copy link
Member

@tgross tgross commented Oct 31, 2023

Job submitters cannot set multiple identities prior to Nomad 1.7, and cluster administrators should not set the identity configurations for their consul and vault configuration blocks until all servers have been upgraded. Validate these cases during job submission so as to prevent state store corruption when jobs are submitting in the middle of a cluster upgrade.

Job submitters cannot set multiple identities prior to Nomad 1.7, and cluster
administrators should not set the identity configurations for their `consul` and
`vault` configuration blocks until all servers have been upgraded. Validate
these cases during job submission so as to prevent state store corruption when
jobs are submitting in the middle of a cluster upgrade.
Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think this is a great way to protect against truly wacky behavior in mixed version settings.

An implicit constraint on the client being >= 1.7.0 would be useful too.

Comment on lines +413 to +417
if v.srv == nil || v.srv.serf == nil {
return true // handle tests w/o real servers safely
}
return ServersMeetMinimumVersion(
v.srv.Members(), v.srv.Region(), minVersionMultiIdentities, true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice if ServersMeetMinimumVersion was a struct that wrapped up Members() and Region() and only provided a simple Meets(version, checkFailed) interface for us to mock/stub out in tests.

nbd though, nothing to block this work on.

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've opened #18931 to follow-up on that when I have a few minutes.

nomad/job_endpoint_hooks.go Outdated Show resolved Hide resolved
@tgross tgross force-pushed the identitites-min-version-check branch from 67502f6 to 91be7e3 Compare October 31, 2023 17:24
@tgross
Copy link
Member Author

tgross commented Oct 31, 2023

An implicit constraint on the client being >= 1.7.0 would be useful too.

Agreed. I'll follow-up with that in a separate PR.

@tgross tgross merged commit 01d050c into main Oct 31, 2023
25 of 26 checks passed
@tgross tgross deleted the identitites-min-version-check branch October 31, 2023 17:57
@tgross
Copy link
Member Author

tgross commented Oct 31, 2023

Follow-up for the client version check in #18932

nvanthao pushed a commit to nvanthao/nomad that referenced this pull request Mar 1, 2024
…8926)

Job submitters cannot set multiple identities prior to Nomad 1.7, and cluster
administrators should not set the identity configurations for their `consul` and
`vault` configuration blocks until all servers have been upgraded. Validate
these cases during job submission so as to prevent state store corruption when
jobs are submitting in the middle of a cluster upgrade.
nvanthao pushed a commit to nvanthao/nomad that referenced this pull request Mar 1, 2024
…8926)

Job submitters cannot set multiple identities prior to Nomad 1.7, and cluster
administrators should not set the identity configurations for their `consul` and
`vault` configuration blocks until all servers have been upgraded. Validate
these cases during job submission so as to prevent state store corruption when
jobs are submitting in the middle of a cluster upgrade.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants