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

consul: support admin partitions #19665

Merged
merged 4 commits into from Jan 10, 2024
Merged

consul: support admin partitions #19665

merged 4 commits into from Jan 10, 2024

Conversation

tgross
Copy link
Member

@tgross tgross commented Jan 8, 2024

Add support for Consul Enterprise admin partitions. We added fingerprinting in #19485. This PR adds a consul.partition field. The expectation is that most users will create a mapping of Nomad node pool to Consul admin partition. But we'll also create an implicit constraint for the fingerprinted value.

Fixes: #13139

Add support for Consul Enterprise admin partitions. We added fingerprinting in
#19485. This PR adds a `consul.partition`
field. The expectation is that most users will create a mapping of Nomad node
pool to Consul admin partition. But we'll also create an implicit constraint for
the fingerprinted value.

Fixes: #13139
Copy link
Member

@gulducat gulducat left a comment

Choose a reason for hiding this comment

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

An e2e test for this might be neat, and I think there's a little unit test gap (unless I'm missing something), but otherwise LGTM!

nomad/job_endpoint_hook_consul_ce_test.go Show resolved Hide resolved
@tgross
Copy link
Member Author

tgross commented Jan 9, 2024

An e2e test for this might be neat

Yeah, that's somewhat complicated in that we can only support that in Consul ENT, which we don't have for all tests. But adding the partition where it's available would be nice. Will probably do that in a follow-up PR once I've got the ENT PR done.

Copy link
Member

@Juanadelacuesta Juanadelacuesta left a comment

Choose a reason for hiding this comment

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

I would find a way to move the empty verifications into a function, to make the code cleaner, but that is nitpicking. It looks good

Copy link
Member

@jrasell jrasell left a comment

Choose a reason for hiding this comment

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

LGTM; exceptionally minor inline comment.

nomad/job_endpoint_hook_consul_ce.go Outdated Show resolved Hide resolved
@tgross tgross merged commit d3e5cae into main Jan 10, 2024
21 checks passed
@tgross tgross deleted the consul-admin-partition-jobspec branch January 10, 2024 15:41
@tgross tgross added the backport/1.7.x backport to 1.7.x release line label Jan 10, 2024
nvanthao pushed a commit to nvanthao/nomad that referenced this pull request Mar 1, 2024
Add support for Consul Enterprise admin partitions. We added fingerprinting in
hashicorp#19485. This PR adds a `consul.partition`
field. The expectation is that most users will create a mapping of Nomad node
pool to Consul admin partition. But we'll also create an implicit constraint for
the fingerprinted value.

Fixes: hashicorp#13139
nvanthao pushed a commit to nvanthao/nomad that referenced this pull request Mar 1, 2024
Add support for Consul Enterprise admin partitions. We added fingerprinting in
hashicorp#19485. This PR adds a `consul.partition`
field. The expectation is that most users will create a mapping of Nomad node
pool to Consul admin partition. But we'll also create an implicit constraint for
the fingerprinted value.

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

Successfully merging this pull request may close these issues.

consul: support for admin partitions
4 participants