GRPC-native healthcheck config option#383
Conversation
|
@baranov1ch in general LGTM, thank you for your contribution! If you are going to work on envoyproxy/envoy#369 can you please comment there so that we don't duplicate effort? Also this PR will need DCO/docs fixed. Please see failing tests and contribution guide. |
api/health_check.proto
Outdated
| // for details. | ||
| message GrpcHealthCheck { | ||
| // Name of the service to perform check on. | ||
| string service = 1; |
There was a problem hiding this comment.
Envoy uses the term cluster for gRPC. Shall we be consistent with it?
There was a problem hiding this comment.
My idea was that this is what will be sent in service field in HealthCheckRequest:
https://github.com/grpc/grpc/blob/master/src/proto/grpc/health/v1/health.proto#L21. Would calling it cluster cause more confusion about how this maps on gRPC health protocol?
api/health_check.proto
Outdated
| // See `GRPC doc <https://github.com/grpc/grpc/blob/master/doc/health-checking.md>` | ||
| // for details. | ||
| message GrpcHealthCheck { | ||
| // Name of the service to perform check on. |
There was a problem hiding this comment.
Name => The name.
Please give an example for each field whenever possible.
| message GrpcHealthCheck { | ||
| // Name of the service to perform check on. | ||
| string service = 1; | ||
| } |
There was a problem hiding this comment.
How do we send a health check request to a gRPC service? What the request looks like? What is the response would be? How does the client knows whether to use mTLS to send the request or something else?
There was a problem hiding this comment.
I'm not sure if I understand the question correctly. Standard gRPC health protocol will be used, it was chosen here. As for mTLS, I don't think gRPC healthcheck differs from usual HTTP one - all transport layer security stuff is defined in tls_context of the cluster being health-checked.
There was a problem hiding this comment.
Yup exactly. Cluster connection options are used.
5548a9c to
2031e7a
Compare
htuch
left a comment
There was a problem hiding this comment.
Great, thanks for making this happening.
api/health_check.proto
Outdated
| // See `GRPC doc <https://github.com/grpc/grpc/blob/master/doc/health-checking.md>` | ||
| // for details. | ||
| message GrpcHealthCheck { | ||
| // The name of the service to perform check on. |
There was a problem hiding this comment.
If we go with service terminology, maybe be extra specific and clearly state something like: The semantics of service names are provided in https://github.com/grpc/grpc/blob/master/doc/health-checking.md. If we go with cluster terminology, please provide a similar mapping.
If we think of upstreams as always being part of an Envoy mesh, then cluster ~= service, and it would be good to stick to have consistent terminology as per @wora comment. OTOH, whenever we impedance match with the gRPC world, we end up having to bridge the cluster/service gap (I'm doing this right now when integrating with the Google gRPC library implementation). In general, endpoints might be Google gRPC and not Envoy, at which point it makes more sense to use the "service name" terminology.
There was a problem hiding this comment.
Renamed to service_name. I've also added more elaborated comment on it and its connection with gRPC health protocol.
api/health_check.proto
Outdated
| } | ||
|
|
||
| // [#not-implemented-hide:] grpc.health.v1.Health-based healthcheck. | ||
| // See `GRPC doc <https://github.com/grpc/grpc/blob/master/doc/health-checking.md>` |
There was a problem hiding this comment.
I think you need a trailing underscore for this to render as an external link. Please validate the docs output for this by temporarily deleting the #not-implemented-hide and viewing docs in a browser.
There was a problem hiding this comment.
Thanks, fixed. I've looked through uncommented html, now looks fine:)
2031e7a to
8a0e31f
Compare
Part of the work on envoyproxy/envoy#369 Signed-off-by: Alexey Baranov <me@kotiki.cc>
8a0e31f to
9349999
Compare
|
Thanks. This looks very nice. In general, the proto spec should be self contained. It is what developers would read and write code against. We should not force developers to know things not mentioned by the proto files. |
Part of the work on
envoyproxy/envoy#369