-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
connect: canonicalize before adding sidecar #6855
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5552,6 +5552,8 @@ func (t *Task) Validate(ephemeralDisk *EphemeralDisk, jobType string, tgServices | |
if t.Leader { | ||
mErr.Errors = append(mErr.Errors, fmt.Errorf("Connect proxy task must not have leader set")) | ||
} | ||
|
||
// Ensure the proxy task has a corresponding service entry | ||
serviceErr := ValidateConnectProxyService(t.Kind.Value(), tgServices) | ||
if serviceErr != nil { | ||
mErr.Errors = append(mErr.Errors, serviceErr) | ||
|
@@ -5746,15 +5748,28 @@ const ConnectProxyPrefix = "connect-proxy" | |
// valid Connect config. | ||
func ValidateConnectProxyService(serviceName string, tgServices []*Service) error { | ||
found := false | ||
names := make([]string, 0, len(tgServices)) | ||
for _, svc := range tgServices { | ||
if svc.Name == serviceName && svc.Connect != nil && svc.Connect.SidecarService != nil { | ||
if svc.Connect == nil || svc.Connect.SidecarService == nil { | ||
continue | ||
} | ||
|
||
if svc.Name == serviceName { | ||
found = true | ||
break | ||
} | ||
|
||
// Build up list of mismatched Connect service names for error | ||
// reporting. | ||
names = append(names, svc.Name) | ||
} | ||
|
||
if !found { | ||
return fmt.Errorf("Connect proxy service name not found in services from task group") | ||
if len(names) == 0 { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
return nil | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.