health check: add optional alternative health check port#597
health check: add optional alternative health check port#597mattklein123 merged 7 commits intoenvoyproxy:masterfrom
Conversation
This provides an alternative health check port, if set it allows an upstream host to have different health check address port. Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
envoy/api/v2/endpoint/endpoint.proto
Outdated
| // By default the health check address port of an upstream host is the same as the host's serving | ||
| // address port. This provides an alternative health check port. If set it allows an upstream host | ||
| // to have different health check address port. The health checker should honor this. | ||
| uint32 health_check_port = 2; |
There was a problem hiding this comment.
I think we might eventually want something closer to
, but for now this is OK (we can always upgrade to aoneof without breaking wire compat).
envoy/api/v2/endpoint/endpoint.proto
Outdated
| // | ||
| // By default the health check address port of an upstream host is the same as the host's serving | ||
| // address port. This provides an alternative health check port. If set it allows an upstream host | ||
| // to have different health check address port. The health checker should honor this. |
There was a problem hiding this comment.
You can drop the last sentence, seems superfluous.
envoy/api/v2/endpoint/endpoint.proto
Outdated
|
|
||
| // [#not-implemented-hide:] Optional alternative health check port. | ||
| // | ||
| // By default the health check address port of an upstream host is the same as the host's serving |
There was a problem hiding this comment.
Presumably we're using uint32 rather than google.protobuf.UInt32Value because we can't possibly have a valid health check port of 0, since that doesn't make sense in TCP.
envoy/api/v2/endpoint/endpoint.proto
Outdated
| // The upstream host address. | ||
| core.Address address = 1; | ||
|
|
||
| // [#not-implemented-hide:] Optional alternative health check port. |
There was a problem hiding this comment.
How general is this? I'm wondering what other parameters might be configurable at the per-endpoint level, e.g. might we want to health check using TLS and a different cert, etc? Not really advocating to make this more complicated, but would like to make sure we have covered the potential endpoint health check specializations at least at the design level.
There was a problem hiding this comment.
Probably this thread from @codesuki on #envoy-users could be related too?
There was a problem hiding this comment.
+1 at least from the proto perspective. Can you maybe add a new message such as HealthCheckConfig (or something like that), and then put the port override in there? Then we can add other things in the future if needed.
From a doc perspective, we should make it clear that health checking must be configured at the cluster level for these options to take any effect, etc.
| @@ -18,7 +18,15 @@ option (gogoproto.equal_all) = true; | |||
|
|
|||
| // Upstream host identifier. | |||
| message Endpoint { | |||
There was a problem hiding this comment.
Do we really need each endpoint to have its own alternative health check port, or can this just be specified at cluster level?
There was a problem hiding this comment.
Some related discussions:
- Allow healthcheck on different port for non-EDS cluster types envoy#439
- Allow healthchecks to be performed on another port envoy#560
From above threads, I perceived that providing an alternative health check port per endpoint is preferred.
There was a problem hiding this comment.
I think per-host is likely required due to potentially dynamic ports on the service side. In the cluster general situation it's more verbose, but I don't think we are optimizing for lack of verbosity here so I think it's OK?
https://github.com/envoyproxy/data-plane-api/pull/597/files#r178582159 Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
dio
changed the title
| // as the host's serving address port. This provides an alternative health | ||
| // check port. If set it allows an upstream host to have different | ||
| // health check address port. | ||
| uint32 port_value = 1; |
There was a problem hiding this comment.
I assume 0 means no alt port? If so can we document that?
There was a problem hiding this comment.
Try to address this in 75662e3. Please let me know if that one is either good enough or need improvement. Thanks.
|
LGTM modulo comment from @mattklein123 . |
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
3 participants
This provides an alternative health check port, if set it allows an
upstream host to have different health check address port.
Ref: envoyproxy/envoy#439
Signed-off-by: Dhi Aurrahman dio@rockybars.com