consul: plumbing for specifying consul namespace in job/group #10235
Conversation
There was a problem hiding this comment.
This should change before 1.0 GA; instead we can fingerprint Consul oss/ent and set a boolean to do the Right Thing without leaning on Consul error response. I'll open a followup PR for that.
go.mod
Outdated
There was a problem hiding this comment.
For now, until Consul produces a new API version with this change.
nomad/consul.go
Outdated
There was a problem hiding this comment.
This left me with a belief that we need to overhaul how Nomad handles Consul tokens. In the past the Nomad agent token was used no matter who the job submitter or what the job contained. With Connect we introduced a Consul ACL token permissions check that the job submitter had equivalent Service Identity permissions, because such a token is generated and given to the task. With Namespaces, we need to check that the job submitter has necessary service:write and kv:read permissions within their namespace - and without extensive plumbing the only safe way to do that is to parse and check the HCL ourselves. Ideally we should be actually using the job submitter's Consul ACL token and letting Consul do the ACL checks - but to do that safely requires managing AccessorID's in the Nomad Client, wiring up some RPC's with Nomad Server to get the SecretID from Consul and passing it back to the Client, who can then submit the token to Consul without it ever being written to the Client's disk (the security hole @schmichael points out). That seems like a lot of work so it'll have to wait for another day.
nomad/structs/consul_oss.go
Outdated
There was a problem hiding this comment.
The implementation in ent is slightly different
command/job_init.bindata_assetfs.go
Outdated
| } | ||
|
|
||
| info := bindataFileInfo{name: "command/assets/connect-short.nomad", size: 997, mode: os.FileMode(436), modTime: time.Unix(1612560436, 0)} | ||
| info := bindataFileInfo{name: "command/assets/connect-short.nomad", size: 997, mode: os.FileMode(436), modTime: time.Unix(1616684356, 0)} |
There was a problem hiding this comment.
Should this file have been committed?
There was a problem hiding this comment.
Nope, reverted. Still not totally familiar with the generation workflow 😞
| hashMeta(h, s.CanaryMeta) | ||
| hashConnect(h, s.Connect) | ||
| hashString(h, s.OnUpdate) | ||
| hashString(h, s.Namespace) |
There was a problem hiding this comment.
I just spent like 15 trying to figure out why we have a hashStringIfNonEmpty function because I was worried about upgrades. But this is SHA1, so "" doesn't impact the hash. 🤷♂️
There was a problem hiding this comment.
Huh, TIL! We can probably remove hashStringIfNonEmpty then, maybe just doing a spot check first with some examples to make sure everything is the same before and after. For a rainy day, perhaps.
nomad/consul_test.go
Outdated
| } | ||
|
|
||
| func TestConsulACLsAPI_CheckSIPolicy(t *testing.T) { | ||
| // CheckPermissions(ctx context.Context, namespace string, usage *structs.ConsulUsage) error |
There was a problem hiding this comment.
| // CheckPermissions(ctx context.Context, namespace string, usage *structs.ConsulUsage) error |
nomad/consul_test.go
Outdated
| t.Parallel() | ||
|
|
||
| try := func(t *testing.T, service, token string, expErr string) { | ||
| equalError := func(t *testing.T, exp, err error) { |
There was a problem hiding this comment.
Nitpicky, but this isn't reused anywhere so it was probably more clear to inline it
| return false | ||
| } | ||
|
|
||
| // ConsulUsages returns a map from Consul namespace to things that will use Consul, |
This PR adds the common OSS changes for adding support for Consul Namespaces, which is going to be a Nomad Enterprise feature. There is no new functionality provided by this changeset and hopefully no new bugs.
Co-authored-by: Tim Gross <tgross@hashicorp.com>
|
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. |
2 participants
This PR adds the common changes for adding support for Consul Namespaces,
which is going to be a Nomad Enterprise feature. There is no new functionality
provided by this changeset and hopefully no new bugs.