psFried
commented
2 years ago There's at least one possible omission in this PR, though I'd like to defer doing anything about it until after this PR gets deployed. From this comment in the Gazette repo:
Note this matcher will send an initial empty SETTINGS frame to the client, as gRPC clients delay the first request until the HTTP/2 handshake has completed.
I don't know whether that will pose a problem for gRPC clients or not. But I also don't know whether it would have any effect if we did send an empty settings frame, given that there's currently an L7 load balancer in front of this. My rough plan is to deploy this and then test whether gRPC end-to-end trough the LB. If the SETTINGS thing is really an issue, then it seems like it would essentially require us to use an L4 LB instead.
Multiplexing of these protocols was broken in a few ways. One was that REST requests that were sent over HTTP2 were never handled due to not matching any CMux handler. Another issue was that load balancers would maintain persistent connections to the server which would be re-used for both REST and gRPC calls. This was incompatible with CMux, which determines the handler for a given connection only once, when the connection is established.
This commit implements the approach described in: https://ahmet.im/blog/grpc-http-mux-go/ It invokes either the grpc or http handler for each request, based on the Content-Type header.
Additionally, the tests were modified to use TLS with a self-signed certificate in order to more closely mimic real-world usage.