Skip to content
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

fix(server): handle gRPC-Web requests over HTTP2 #3352

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

maxbrunet
Copy link
Member

@maxbrunet maxbrunet commented Jun 23, 2023

After much testing, I found out the following:

HTTP1.1 works: ✔️

$ curl --http1.1 127.0.0.1:7070/api/grpc.health.v1.Health/Check \
  -H 'Content-Type: application/grpc-web-text' \
  -H 'Accept: application/grpc-web-text' \
  -d 'AAAAAAA='
AAAAAAIIAQ==gAAAABBncnBjLXN0YXR1czogMA0K

H2C (HTTP2 upgraded from HTTP1.1) works: ✔️

$ curl --http2 127.0.0.1:7070/api/grpc.health.v1.Health/Check \
  -H 'Content-Type: application/grpc-web-text' \
  -H 'Accept: application/grpc-web-text' \
  -d 'AAAAAAA='
AAAAAAIIAQ==gAAAABBncnBjLXN0YXR1czogMA0K

HTTP2 (without upgrade from HTTP1.1) does not work: ❌

$ curl --http2-prior-knowledge 127.0.0.1:7070/api/grpc.health.v1.Health/Check \
  -H 'Content-Type: application/grpc-web-text' \
  -H 'Accept: application/grpc-web-text' \
  -d 'AAAAAAA='
invalid gRPC request content-type "application/grpc-web-text"

This patch fixes it: ✔️

$ curl --http2-prior-knowledge 127.0.0.1:7070/api/grpc.health.v1.Health/Check \
  -H 'Content-Type: application/grpc-web-text' \
  -H 'Accept: application/grpc-web-text' \
  -d 'AAAAAAA='
AAAAAAIIAQ==gAAAABBncnBjLXN0YXR1czogMA0K

gRPC-Web requests can use HTTP2, and most modern browsers actually use it if the host supports it.

This latest case happens if the reverse proxy (e.g. Envoy) forwards HTTP2 to HTTP2. Until now, we had been using NGINX in the demo which downgrades the request to HTTP1.1, so the issue had not been surfaced.

This should unblock advanced use cases with mixed client protocols (HTTP1.1/HTTP2/H2C, gRPC/gRPC-Web).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants