-
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
Custom Connect Sidecar Checks #10524
Conversation
Proxy: proxy, | ||
Checks: api.AgentServiceChecks{ | ||
var checks []*api.AgentServiceCheck | ||
if len(css.Checks) == 0 { |
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.
With this change, there is no way to actually disable sidecar checks. The job author must specify a check
or we insert the default TCP ones. This seems a bit reasonable.
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.
This is also the only option if registering with Consul directly; this approach is just following suite
//if c.PortLabel != "" { | ||
// mErr.Errors = append(mErr.Errors, fmt.Errorf("Check %s invalid: port is automatically assigned", c.Name)) | ||
// continue | ||
//} |
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.
Not sure if we should allow setting port labels here; as the sidecar service doesn't specify ports.
} else { | ||
checks = make([]*api.AgentServiceCheck, len(css.Checks)) | ||
for i, c := range css.Checks { | ||
check, err := createCheckReg(serviceId, "", c, cMapping.HostIP, cMapping.Value, "") |
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.
Not sure if I'm doing this correctly, and what host/port should be used. When I tried, I can successfully set a custom TCP
check; but cannot seem to get an HTTP
to pass. The default envoyproxy seems to reject HTTP connection?!
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.
I sort of imagined being able to create checks that somehow made use of NOMAD_ENVOY_ADMIN_ADDR_<service>
, where envoy provides some default paths one might query https://www.envoyproxy.io/docs/envoy/latest/operations/admin to represent the envoy's health.
The only way that will work is if the Consul agent is able to make a connection to that ip:port, which is going to be 127.0.0.1:1900x
inside the network namespace - which means Nomad would need to inject a new dynamic port mapping to that envoy admin port.
@@ -120,7 +117,25 @@ func connectSidecarRegistration(serviceId string, css *structs.ConsulSidecarServ | |||
Name: "Connect Sidecar Aliasing " + serviceId, | |||
AliasService: serviceId, | |||
}, |
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.
Unclear what the value of this Aliasing service id - do we need to insert it even when checks are overriden?
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.
The alias basically means "if you request health for service X, send the request to me instead" - which is useful for connect, where requests must go through the sidecar proxy. I think yes, this should always be there
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.
@notnoop this is definitely along the lines of the original suggestion - being able to specify custom checks on the sidecar service.
Taking into consideration the original ask of #9773 and the amount of complexity in enabling custom sidecar checks that actually do something, I now wonder if it would be better to just add a kind of disable_default_tcp_check
option. Otherwise, to have envoy be able to respond to these custom checks, we're going to need to basically hack-up the expose
machinery, making it possible for envoy creates an exposed listener for itself. Re-reading that issue, nobody is really asking for that - they just want the TCP checks to stop.
What are your thoughts? The simpler option is divergent from Consul, but the checks
option is going to be more work.
Thinking out-loud, there might also be an in-between approach, where the sidecar_service.checks
definition is a minimal subset of a normal check, where Nomad owns all of the fields except maybe path
and tags
- making so that we just need to automatically manage an injected port mapping and the expose listener.
Proxy: proxy, | ||
Checks: api.AgentServiceChecks{ | ||
var checks []*api.AgentServiceCheck | ||
if len(css.Checks) == 0 { |
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.
This is also the only option if registering with Consul directly; this approach is just following suite
} else { | ||
checks = make([]*api.AgentServiceCheck, len(css.Checks)) | ||
for i, c := range css.Checks { | ||
check, err := createCheckReg(serviceId, "", c, cMapping.HostIP, cMapping.Value, "") |
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.
I sort of imagined being able to create checks that somehow made use of NOMAD_ENVOY_ADMIN_ADDR_<service>
, where envoy provides some default paths one might query https://www.envoyproxy.io/docs/envoy/latest/operations/admin to represent the envoy's health.
The only way that will work is if the Consul agent is able to make a connection to that ip:port, which is going to be 127.0.0.1:1900x
inside the network namespace - which means Nomad would need to inject a new dynamic port mapping to that envoy admin port.
That sounds reasonable given my experience too, indeed - it's kinda hard to specify a check that targets the envoy directly without it being actually for the backend service. I'll have another PR ready this afternoon. |
Superseded by #10531 . Can resurrect to handle the extra complexity if the demand is there. |
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 is WIP PR to ensure I'm in the right path before committing to this approach. This is all very foreign to me.
This PR adds supports for custom checks for connect's
sidecar_service
, in a similar fashion to normal services. When specified, it overrides the hardcoded "Connect Sidecar Listening" TCP check inserted by Nomad.A sample connect stanza might be:
I have few comments/questions that are inlined.
Related to #9773