-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Support disabling TCP checks for connect sidecar services #10531
Conversation
23ffa84
to
96fe87c
Compare
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.
LGTM! Just the two suggestions
command/agent/consul/connect.go
Outdated
}, | ||
} | ||
} else { | ||
// insert a NOOP check to avoid Consul inserting the default |
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 comment is a bit misleading - the aliasing here is functional to consul service discovery - e.g. if the tcp check exists and is failing, queries for the aliased service will not return results including this sidecar (i.e. when using connect upstreams).
command/agent/consul/connect.go
Outdated
Address: cMapping.HostIP, | ||
Proxy: proxy, | ||
Checks: api.AgentServiceChecks{ | ||
var checks api.AgentServiceChecks |
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.
nitpick: since we're building a slice that will always include the alias, i'd golf this down to
checks := api.ServiceChecks {{
// .. <alias check>
}}
if !css.DisableDefaultCheck {
checks = append(checks, <tcp check>)
}
@@ -550,6 +550,7 @@ func parseSidecarService(o *ast.ObjectItem) (*api.ConsulSidecarService, error) { | |||
"port", | |||
"proxy", | |||
"tags", | |||
"disable_default_tcp_check", |
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.
backporting to HCL1 isn't required, but also doesn't hurt anything
Co-authored-by: Seth Hoenig <shoenig@hashicorp.com>
Curses, the test is order-dependent. Will fix. |
@notnoop I tried this yesterday and adding |
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. |
Alternative to #10524 - that focuses on disabling the default TCP check for sidecar services. It solves the immediate needs without blocking future improvements in the area.
Fixes #9773 .