[Feature] Configure nomad cluster to use a Consul Namespace [Consul Enterprise]

[fredrikhgrelland]

Currently Nomad has the option of setting a global Vault namespace configuration
This PR brings feature parity, by allowing a global option to be set for Consul namespaces in the consul configuration

[tgross]

Hi @fredrikhgrelland! Thanks for this PR -- this is a great start!

Consul is a bit more deeply integrated into Nomad than Vault is, so there are several other places that will need to have this configuration:

• template hook
• service hook
• Connect Native hook
• script check hook
• the task runner's Consul client
• the server's Consul client
• (maybe more... you'd want to search for imports of "github.com/hashicorp/consul" and "github.com/hashicorp/consul-template")

This is a pretty big feature to take on, and the Nomad maintainers would need to follow up with multi-namespace support for the ENT product just as we've done with Vault in 0.12.2. But it's also not on our short-term roadmap, so if you're willing to tackle this, we'd love to have the contribution and we'd be happy to help review and guide you through it!

[fredrikhgrelland]

Thank you for the review.

I expected that it was a little bit more involved...
Multi-namespace support is great, but for our use-case we need nomad to be simply scoped to a consul namespace in the same way as vault currently is in OSS.

I'll review your comments and will get back to you :)

[fredrikhgrelland]

Hi @tgross.

I have looked through the places where you suggest there needs to be changes, but I have a hard time figuring out what it is you think needs changing. The way I read this code is that nomad will leverage the consul api configuration of nomad. Is this not true? In order for nomad to honor a different consul namepace than the default I have added the a namespace field to the consul config stanza and plumbed this to the conusl api. in my tests, this was enough for nomad to work with consul correctly for all registrations/de-registrations/checks of both nomad it self and jobs with services. To me it seems that it "just works" since it is all based off of the general config. The only place I found where nomad is not working off of the API is consul connect envoy bootstrap, which is why I changed that as well.

Please have a second look, and if I am mistaken, maybe you can show one example where changes is needed so I can get at it?

[tgross]

Basically the idea is looking for the places where we're converting from Nomad's internal representation of Consul configuration (ConsulConfig) to the actual Consul structs outside of the ApiConfig method you've change here.

Two areas I found:

• in the configuration for the template stanza (where we call into consul-template), the Consul config needs to be explicitly passed through in newRunnerConfig
• the connect hook passes the Consul struct into newConsulTransportConfig but that function doesn't grab the namespace field (ref envoybootstrap_hook.go#L36-L46)

Upon a bit more review I think we're good on the server and client's own clients. And elsewhere we're using the "Consul Service API" interface wrapper and using that, so those should be ok:

• service hook does just take the Consul Service API client as part of serviceHookConfig and uses it directly
• script_check hook takes the Consul Service API client as part of scriptCheckHookConfig and uses it directly.

[fredrikhgrelland]

@tgross ,

Sorry for the delay. I finally found enough time to dive in and review properly.
I opened a separate PR #8988 for consul-template hooks, as it requires a new version. That commit, bf8c8c6, is in this PR as well.

I tried to find any more places where config needs to be converted, but I have not found any more.
The way I understand it is that we need to convert ConsulConfig into a library native format where needed?
This PR has it covered for envoy (consul connect) and consul-template. Is there any more?

[tgross]

I tried to find any more places where config needs to be converted, but I have not found any more.
The way I understand it is that we need to convert ConsulConfig into a library native format where needed?
This PR has it covered for envoy (consul connect) and consul-template. Is there any more?

I think you've got them all. The #8988 PR needs a couple fixes. I'm going to tag-in one of my colleagues who's been working on the Consul integration a lot in the last few months just to double-check I haven't missed anything glaring. But I think we'll be good-to-go here.

[fredrikhgrelland]

@tgross I have updated this PR with changes requested in #8988 and updated changelog.

[shoenig]

Thanks for working on this @fredrikhgrelland , it is truly appreciated.

Getting this to work with Consul ACLs is a little funny but possible, and I think we'll want to add some documentation since it's not quite obvious.

Consul does not allow Namespace'd ACL policies to make use of agent permissions, of which Nomad requires agent:read. Because of this, Nomad itself will need a token generated in Consul's default namespace. That token would then be created from a policy with agent:read as well as a namespace block with the other relevant permissions for running Nomad in the intended namespace. Something like,

agent_prefix "" {
  policy = "read"
}

namespace "nomad-ns" {
  acl = "write"

  key_prefix "" {
    policy = "write"
  }

  node_prefix "" {
    policy = "read"
  }

  service_prefix "" {
    policy = "write"
  }
}

Can probably have the docs live under a heading at the bottom of https://www.nomadproject.io/docs/configuration/consul

[tgross]

I'll follow-up with that documentation suggestion in a separate PR so that we're not gating this one on some in-flight documentation infra changes.

[github-actions]

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.