-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
UHV: Fix response status checks #27125
Changes from 1 commit
a07abe6
21705a9
2b024c7
6940338
c12fc22
3524186
38d892e
98a495d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -524,14 +524,13 @@ void ConnectionImpl::StreamImpl::decodeData() { | |
|
||
void ConnectionImpl::ClientStreamImpl::decodeHeaders() { | ||
auto& headers = absl::get<ResponseHeaderMapPtr>(headers_or_trailers_); | ||
#ifndef ENVOY_ENABLE_UHV | ||
const uint64_t status = Http::Utility::getResponseStatus(*headers); | ||
|
||
#ifndef ENVOY_ENABLE_UHV | ||
// Extended CONNECT to H/1 upgrade transformation has moved to UHV | ||
if (!upgrade_type_.empty() && headers->Status()) { | ||
Http::Utility::transformUpgradeResponseFromH2toH1(*headers, upgrade_type_); | ||
} | ||
#endif | ||
|
||
// Non-informational headers are non-1xx OR 101-SwitchingProtocols, since 101 implies that further | ||
// proxying is on an upgrade path. | ||
|
@@ -544,6 +543,31 @@ void ConnectionImpl::ClientStreamImpl::decodeHeaders() { | |
} else { | ||
response_decoder_.decodeHeaders(std::move(headers), sendEndStream()); | ||
} | ||
#else | ||
// in UHV mode the :status header at this point can be malformed, as it is validated | ||
// later on in the response_decoder_.decodeHeaders() call. | ||
// Account for this here. | ||
absl::optional<uint64_t> status_opt = Http::Utility::getResponseStatusOrNullopt(*headers); | ||
if (status_opt.has_value()) { | ||
const uint64_t status = status_opt.value(); | ||
// Non-informational headers are non-1xx OR 101-SwitchingProtocols, since 101 implies that | ||
// further proxying is on an upgrade path. | ||
received_noninformational_headers_ = | ||
!CodeUtility::is1xx(status) || status == enumToInt(Http::Code::SwitchingProtocols); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why don't we have he same problem for the other codecs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Both balsa and EnvoyQuicClientStream validate status, so it does not reach UHV. I will work on fixing this. Balsa fix will need to wait, since Bence is OOO. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have moved the I think keeping status validation in H/1 parser for now is ok, as it just reduces visibility into protocol errors if status is malformed. When the plumbing for the parser error codes into reset reason strings is finished it will reduce this shortcoming. |
||
|
||
if (HeaderUtility::isSpecial1xx(*headers)) { | ||
ASSERT(!remote_end_stream_); | ||
response_decoder_.decode1xxHeaders(std::move(headers)); | ||
} else { | ||
response_decoder_.decodeHeaders(std::move(headers), sendEndStream()); | ||
} | ||
} else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Let's use early return to save a level of nesting:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
// In case the status is invalid or missing, the response_decoder_.decodeHeaders() will fail the | ||
// request | ||
response_decoder_.decodeHeaders(std::move(headers), sendEndStream()); | ||
} | ||
|
||
#endif | ||
} | ||
|
||
bool ConnectionImpl::StreamImpl::maybeDeferDecodeTrailers() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1860,13 +1860,6 @@ TEST_P(ProtocolIntegrationTest, 304WithBody) { | |
} | ||
|
||
TEST_P(ProtocolIntegrationTest, OverflowingResponseCode) { | ||
#ifdef ENVOY_ENABLE_UHV | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Woo hoo! |
||
// TODO(#24945): In UHV case this test breaks in the CONNECT/upgrade normalization code in H/2 | ||
// codec | ||
if (upstreamProtocol() != Http::CodecType::HTTP1) { | ||
return; | ||
} | ||
#endif | ||
useAccessLog("%RESPONSE_CODE_DETAILS%"); | ||
initialize(); | ||
|
||
|
@@ -1895,13 +1888,6 @@ TEST_P(ProtocolIntegrationTest, OverflowingResponseCode) { | |
} | ||
|
||
TEST_P(ProtocolIntegrationTest, OverflowingResponseCodeStreamError) { | ||
#ifdef ENVOY_ENABLE_UHV | ||
// TODO(#24945): In UHV case this test breaks in the CONNECT/upgrade normalization code in H/2 | ||
// codec | ||
if (upstreamProtocol() != Http::CodecType::HTTP1) { | ||
return; | ||
} | ||
#endif | ||
// For H/1 this test is equivalent to OverflowingResponseCode | ||
if (upstreamProtocol() == Http::CodecType::HTTP1) { | ||
return; | ||
|
@@ -1927,13 +1913,6 @@ TEST_P(ProtocolIntegrationTest, OverflowingResponseCodeStreamError) { | |
} | ||
|
||
TEST_P(ProtocolIntegrationTest, MissingStatus) { | ||
#ifdef ENVOY_ENABLE_UHV | ||
// TODO(#24945): In UHV case this test breaks in the CONNECT/upgrade normalization code in H/2 | ||
// codec | ||
if (upstreamProtocol() != Http::CodecType::HTTP1) { | ||
return; | ||
} | ||
#endif | ||
initialize(); | ||
|
||
// HTTP1, uses a defined protocol which doesn't split up messages into raw byte frames | ||
|
@@ -1973,13 +1952,6 @@ TEST_P(ProtocolIntegrationTest, MissingStatus) { | |
} | ||
|
||
TEST_P(ProtocolIntegrationTest, MissingStatusStreamError) { | ||
#ifdef ENVOY_ENABLE_UHV | ||
// TODO(#24945): In UHV case this test breaks in the CONNECT/upgrade normalization code in H/2 | ||
// codec | ||
if (upstreamProtocol() != Http::CodecType::HTTP1) { | ||
return; | ||
} | ||
#endif | ||
// For H/1 this test is equivalent to MissingStatus | ||
if (upstreamProtocol() == Http::CodecType::HTTP1) { | ||
return; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "in" => "In"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done