1.27 Backport H/2 header discard fix#33302
Merged
phlax merged 5 commits intoenvoyproxy:release/v1.27from Apr 4, 2024
Merged
Conversation
|
Hi @SeanKilleen, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
SeanKilleen
force-pushed
the
1.27-backport-h2-authority-fix
branch
from
7cdab56 to
49571b9
Compare
yanavlasov
previously requested changes
Apr 3, 2024
Contributor
yanavlasov
left a comment
yanavlasov
left a comment
There was a problem hiding this comment.
Thanks for doing this backport.
/wait
Discard the Host header if the :authority header was received to bring Envoy into compliance with https://www.rfc-editor.org/rfc/rfc9113#section-8.3.1 This behavioral change can be reverted by setting runtime flag envoy.reloadable_features.http2_discard_host_header to false. --------- Signed-off-by: Yan Avlasov <yavlasov@google.com> Signed-off-by: Sean Killeen <SeanKilleen@gmail.com>
Signed-off-by: Sean Killeen <SeanKilleen@gmail.com>
Signed-off-by: Sean Killeen <SeanKilleen@gmail.com>
Depended upon by test Signed-off-by: Sean Killeen <SeanKilleen@gmail.com>
Signed-off-by: Sean Killeen <SeanKilleen@gmail.com>
SeanKilleen
force-pushed
the
1.27-backport-h2-authority-fix
branch
from
984b254 to
7925a5a
Compare
phlax
approved these changes
Apr 4, 2024
Member
phlax
left a comment
phlax
left a comment
There was a problem hiding this comment.
added tests dont seem to break anything so ill land this to include in the release
lgtm, thanks again
Comment on lines
+2132
to
+2164
| TEST_P(Http2FrameIntegrationTest, HostDifferentFromAuthority) { | ||
| beginSession(); | ||
|
|
||
| uint32_t request_idx = 0; | ||
| auto request = Http2Frame::makeRequest(Http2Frame::makeClientStreamId(request_idx), | ||
| "one.example.com", "/path", {{"host", "two.example.com"}}); | ||
| sendFrame(request); | ||
|
|
||
| waitForNextUpstreamRequest(); | ||
| EXPECT_EQ(upstream_request_->headers().getHostValue(), "one.example.com"); | ||
| upstream_request_->encodeHeaders(default_response_headers_, true); | ||
| auto frame = readFrame(); | ||
| EXPECT_EQ(Http2Frame::Type::Headers, frame.type()); | ||
| EXPECT_EQ(Http2Frame::ResponseStatus::Ok, frame.responseStatus()); | ||
| tcp_client_->close(); | ||
| } | ||
|
|
||
| TEST_P(Http2FrameIntegrationTest, HostSameAsAuthority) { | ||
| beginSession(); | ||
|
|
||
| uint32_t request_idx = 0; | ||
| auto request = Http2Frame::makeRequest(Http2Frame::makeClientStreamId(request_idx), | ||
| "one.example.com", "/path", {{"host", "one.example.com"}}); | ||
| sendFrame(request); | ||
|
|
||
| waitForNextUpstreamRequest(); | ||
| EXPECT_EQ(upstream_request_->headers().getHostValue(), "one.example.com"); | ||
| upstream_request_->encodeHeaders(default_response_headers_, true); | ||
| auto frame = readFrame(); | ||
| EXPECT_EQ(Http2Frame::Type::Headers, frame.type()); | ||
| EXPECT_EQ(Http2Frame::ResponseStatus::Ok, frame.responseStatus()); | ||
| tcp_client_->close(); | ||
| } |
Member
There was a problem hiding this comment.
i think these 2 tests have been added in this pr - i guess because the original pr made alterations to them
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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:
Discard the Host header if the :authority header was received to bring Envoy into compliance with https://www.rfc-editor.org/rfc/rfc9113#section-8.3.1
This behavioral change can be reverted by setting runtime flag envoy.reloadable_features.http2_discard_host_header to false.
Risk Level: Low
Testing: Unit Tests
Docs Changes: N/A
Release Notes: Yes
Platform Specific Features: N/A
Runtime guard: envoy.reloadable_features.http2_discard_host_header
Back-ports #30005, supporting #31118