-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
xds: Add passive health check config for upstreams (aka envoy outlier detection) #7713
Conversation
I think the naming seems reasonable to me.
|
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.
Looks good to me.
It should just work with all the various Proxy config merging but it would be good when going through the integration tests to ensure that setting these in various places trickles down properly.
proxy-defaults/global
config entryservice-defaults/<service name>
- In the individual proxy configuration:
Upstream.Config
.
fb7df09
to
88b0196
Compare
88b0196
to
a08f0a2
Compare
I added some passive_health_check config to one of the integration tests. The |
a08f0a2
to
0861a27
Compare
Tests are passing! This is ready for a full review now. |
0861a27
to
043d4d1
Compare
@@ -277,6 +277,17 @@ definition](/docs/connect/registration/service-registration) or | |||
since HTTP/2 has many requests per connection. For this configuration to be | |||
respected, a L7 protocol must be defined in the `protocol` field. | |||
|
|||
- `passive_health_check` - Settings for the load balancer, used to remove |
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.
- `passive_health_check` - Settings for the load balancer, used to remove | |
- `passive_health_check` - Settings for the proxy, used to remove |
nit, but I would use the word proxy instead of load balancer so as not to introduce another noun into the mix
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.
A couple of small comments, but nothing blocking or big, good work! Just worked when I tested it out. 👍
Fix decoding of time.Duration types.
043d4d1
to
5655d7f
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.
Closes #6832
Adds support for configuring two of the many outlier detection settings which provide passive health checking. These two configuration options are approximately the two options supported by HAProxy and NGinx.
The new configuration is being added to
proxy.upstreams[].config
. Envoy has a number of settings (enabled bysplit_external_local_origin_errors
) that change the behaviour for local vs remote errors. I think if we were to configure these passive health check settings from the proxy config of the destination service (instead of the upstream config of the source service), it may be difficult to conceptualize "local".I am opening this PR with a couple of open TODOs to get feedback on the naming and location of the config (
UpstreamConfig
).TODO