grpc-web: suppress content-length header from grpc-web requests #2946
Conversation
negator
changed the title
|
Curious, can you explain why this is a grpc specific issue and not an H2 issue in general? I just want to make sure we're not doing too narrow of a fix. |
|
You’re right, it’s not a grpc specific issue. It could occur anytime an incorrect content-length header is passed to a secondary envoy backend over h2. However this fix is necessary regardless of any other general fix, since browsers (currently) know nothing about h2 data framing and the content-length header is almost always going to be incorrect. |
|
Removing the content length header may be necessary but it'd be great if we can find one place to do it for all of HTTP/2 rather than have each filter do their own clearing. Offhand I think the best place for this is in the HTTP/2 codec, but I'd like to wait for Matt to weigh in. |
|
I don’t think removing it in all cases is the right thing to do, since it could lead to surprising behavior for clients that expect it to be there. But I do think we’d want to perform validation in the h2 codec, or elsewhere, prior to invoking an h2 backend. But I’d defer to you. |
There was a problem hiding this comment.
On the general topic of this change, this problem is actually not specific to h2. It's really specific to all HTTP versions and any filter which fundamentally alters body during processing. I.e., the same issue would occur over HTTP/1.1.
I also don't think removing in all cases is the right thing to do. It really comes down to filters that do this type of modification have to do one of two things:
- Strip content length and switch to chunk encoding. Envoy essentially does this transparently. In the HTTP/1.1 codec if no content-length header is seen, it will start chunk encoding. HTTP/2 is inherently chunk encoding at the binary level and content-length is really a hint.
- Write a new content-length that will match the final body size.
We could consider somehow checking/warning/enforcing this at the Envoy level so at least you would know at the local level if you were doing something wrong...
| @@ -46,6 +46,7 @@ Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, | |||
| } | |||
| is_grpc_web_request_ = true; | |||
|
|
|||
| headers.removeContentLength(); | |||
There was a problem hiding this comment.
Please add a comment as to why this is being removed.
There was a problem hiding this comment.
Switching to chunked transfer encoding is what nginx does in this situation, fwiw.
#2 always runs into the the problem of buffering the entire body to calculate the length, unless you can emit it as a trailer. Maybe feasible for h2, but not 1.1.
There was a problem hiding this comment.
@sayrer in grpc-web context the upstream must be http2, there is no chunked encoding, hence removing content length is enough.
|
Ah see I misunderstood the problem - I thought upstream was choking on content length being present not because the gRPC stack was changing the payload and not updating the header. In that case yeah, it should be a fix to the filter and we can add codec-level validation later on. |
or at nghttp2 level, since it does enforce (RST_STREAM with PROTOCOL_ERROR) this for incoming request, so it can raise at least a warning for outgoing request too. |
Ensure the content-length header field is not passed through from web requests, since upstream `grpc/http2` services may reject it if the value does not conform to spec: https://http2.github.io/http2-spec/#malformed. Namely the value must equal the sum of the DATA frames sizes in bytes. Signed-off-by: Naveen Gattu <[email protected]>
Signed-off-by: Naveen Gattu <[email protected]>
negator
changed the title
grpc-web: suppress content-length header from web requests
Description:
When using envoy in a tiered deployment, with a grpc-web filter on the downstream envoy, requests to upstream envoys fail over http2 when a content-length header is present, since the underlying nghttp2 library strictly adheres to the http2 spec. This header is usually passed up from browsers, but reflects the total http1.1 payload size, not the total DATA frame sizes.
Risk Level: Low
Testing:
A simple unit test was added to verify that content-length headers are suppressed.
Release Notes:
N/A
Fixes #2942