Fixes a bug in Envoy's HTTP/3-to-HTTP/1 proxying when a transfer-enco… #39589
+43
−14
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ding header is incorrectly appended Signed-off-by: Ting Pan <[email protected]>
Signed-off-by: Ting Pan <[email protected]>
|
/retest |
|
@RyanTheOptimist Hi Ryan, I restarted the pr here, added an integration test, please review again, thank you |
| // When downstream protocol is HTTP3 and upstream protocol is HTTP1, an OPTIONS | ||
| // request with no body will not append transfer-encoding chunked. | ||
| TEST_P(DownstreamProtocolIntegrationTest, OptionsWithNoBodyNotChunked) { | ||
| // This test is only relevant for H3 downstream to H1 upstream. |
There was a problem hiding this comment.
While this test was added specifically for HTTP/3, I think the logic applies to H2 and H1 as well, right? Does this test pass if we remove the H3 restriction here?
There was a problem hiding this comment.
They do pass, in that case, do you prefer to make it non-quic-specific test? in that case I'll move it to protocol_integration_test
There was a problem hiding this comment.
updated, please review again, thank you
Signed-off-by: Ting Pan <[email protected]>
|
/retest |
|
/retest |
Signed-off-by: Ting Pan <[email protected]>
RyanTheOptimist
approved these changes
May 30, 2025
Signed-off-by: Ting Pan <[email protected]>
RyanTheOptimist
approved these changes
May 30, 2025
|
thank you Ryan. trying rerun |
|
/retest |
|
@RyanTheOptimist all green now, ready for merge, thank you |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit Message: QUIC Stream: Fixes a bug in Envoy's HTTP/3-to-HTTP/1 proxying when a "transfer-encoding" header is incorrectly appended.
Additional Description: Envoy, when proxying via hyperloop to HTTP/1 backend, incorrectly interprets the end of an HTTP/3 HEADERS frame contained in a QUIC STREAM frame with the end_stream/fin bit set to true. This causes the HTTP/1 codec to mistakenly add a "transfer-encoding" header. This CL addresses this by inspecting the data before encoding headers. For requests containing only headers, it now explicitly signals the end of the stream with the "headers-only" indicator, which is the correct way to denote this in HTTP/3 (unlike the "fin" flag). This prevents the incorrect "transfer-encoding" header from being added.
Risk Level: low, guarded by runtime guard
Testing: internally tested with google3 e2e tests
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:] envoy_reloadable_features_quic_signal_headers_only_to_http1_backend
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]