consul/connect: interpolate connect block #9586
Conversation
shoenig
force-pushed
the
f-connect-interp
branch
from
131ba73 to
80723e1
Compare
|
TODO
|
This PR enables job submitters to use interpolation in the connect block of jobs making use of consul connect. Before, only the name of the connect service would be interpolated, and only for a few select identifiers related to the job itself (#6853). Now, all connect fields can be interpolated using the full spectrum of runtime parameters. Note that the service name is interpolated at job-submission time, and cannot make use of values known only at runtime. Fixes #7221
shoenig
force-pushed
the
f-connect-interp
branch
from
80723e1 to
4d0e745
Compare
client/taskenv/services.go
Outdated
| } | ||
|
|
||
| service.Name = taskEnv.ReplaceEnv(service.Name) | ||
| service.PortLabel = taskEnv.ReplaceEnv(service.PortLabel) | ||
| service.Tags = taskEnv.ParseAndReplace(service.Tags) | ||
| service.CanaryTags = taskEnv.ParseAndReplace(service.CanaryTags) | ||
| service.Meta = interpolateMapStringString(taskEnv, service.Meta) | ||
| service.CanaryMeta = interpolateMapStringString(taskEnv, service.CanaryMeta) | ||
| service.Connect = interpolateConnect(taskEnv, service.Connect) |
There was a problem hiding this comment.
Somewhat related because we're potentially setting these to nil and not empty maps, none of these three fields is Canonicalized. Should we be doing that and/or should we be assigning an empty map here for safety?
There was a problem hiding this comment.
The canonicalized form of empty maps is nil (at least in services + consul)
nomad/nomad/structs/services.go
Line 174 in a480eed
Copy also make use of the helper.Copy* functions which also reduce to nil if there are no elements
https://github.com/hashicorp/nomad/blob/master/helper/funcs.go#L282
client/taskenv/services.go
Outdated
| } | ||
|
|
||
| // make one copy and interpolate in-place on that | ||
| modified := orig.Copy() |
There was a problem hiding this comment.
The caller copies Service... shouldn't we already have a copy of the structs.ConsulConnect too?
There was a problem hiding this comment.
Good catch! Indeed there's no reason to create an extra copy here.
| for k, v := range proxy.EnvoyGatewayBindAddresses { | ||
| m[taskEnv.ReplaceEnv(k)] = &structs.ConsulGatewayBindAddress{ | ||
| Address: taskEnv.ReplaceEnv(v.Address), | ||
| Port: v.Port, |
There was a problem hiding this comment.
It looks like you interpolate the sidecar port below. Is there a reason we're not can't for this port?
There was a problem hiding this comment.
The ConsulGatewayBindAddress field is an int, and there's (currently?) no way to interpolate int values
https://github.com/hashicorp/nomad/blob/master/nomad/structs/services.go#L1291
Co-authored-by: Tim Gross <[email protected]>
|
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. |
This PR enables job submitters to use interpolation in the connect
block of jobs making use of consul connect. Before, only the name of
the connect service would be interpolated, and only for a few select
identifiers related to the job itself (#6853). Now, all connect fields
can be interpolated using the full spectrum of runtime parameters.
Note that the service name is interpolated at job-submission time,
and cannot make use of values known only at runtime.
Fixes #7221