connect: canonicalize before adding sidecar #6855
Conversation
schmichael
force-pushed
the
b-interp-connect-task
branch
from
781d01f to
512826d
Compare
| jobCanonicalizer{}, | ||
| jobConnectHook{}, |
There was a problem hiding this comment.
Note to reviewers: this is one of those fun things that looks super minor but is actually the key to the entire PR.
Previously we were canonicalizing after adding Connect tasks.
This change makes it so we canonicalize before adding Connect tasks.
This solves the interpolation issue linked from the PR, but is also generally safer as just about everywhere in nomad/ we touch Job structs we expect them to be canonicalized. So this should make future work in mutators much more predictable.
The downside is that any mutations must canonicalize themselves. The conservative version of this approach would just canonicalize twice: once at the beginning and once at the end. Since there is so little mutator code today I went with this minimal approach where the connect hook canonicalizes anything it adds.
| return fmt.Errorf("No Connect services in task group with Connect proxy (%q)", serviceName) | ||
| } else { | ||
| return fmt.Errorf("Connect proxy service name (%q) not found in Connect services from task group: %s", serviceName, names) | ||
| } |
There was a problem hiding this comment.
Sorry for the code churn here. The previous error message was just extremely unhelpful. It might as well have been a hardcoded UUID because it only really made sense once you grepped the code for it.
The new errors are still pretty weird because it's a pretty weird condition that is probably due to a code error. The errors at least include a little more context.
Fixes #6853 Canonicalize jobs first before adding any sidecars. This fixes a bug where sidecar tasks were added without interpolated names and broke validation. Sidecar tasks must be canonicalized independently. Also adds a group network to the mock connect job because it wasn't a valid connect job before!
Also make Connect related fixes more consistent in the changelog. I suspect users won't care if a Connect related fix is in the server's admission controller or in the client's groupservice hook or somewhere else, so I think grouping them by `consul/connect:` makes the most sense.
schmichael
force-pushed
the
b-interp-connect-task
branch
from
512826d to
4f718f7
Compare
|
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. |
Fixes #6853
Canonicalize jobs first before adding any sidecars. This fixes a bug
where sidecar tasks were added without interpolated names and broke
validation. Sidecar tasks must be canonicalized independently.
Also adds a group network to the mock connect job because it wasn't a
valid connect job before!