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.

Sign up for GitHub

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

Jump to bottom

quic: turn up status tests #15648

Merged
merged 6 commits into from
Mar 30, 2021
Merged

quic: turn up status tests #15648

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
797adae
test: turn up status tests
alyssawilk Mar 24, 2021
cb4229f
spelling
alyssawilk Mar 24, 2021
1d31b56
update
alyssawilk Mar 24, 2021
fd5f90b
move assert
alyssawilk Mar 29, 2021
4df8532
Merge branch 'main' into quic_test
alyssawilk Mar 30, 2021
6edf18c
comment
alyssawilk Mar 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion source/common/http/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,10 +439,18 @@ std::string Utility::makeSetCookieValue(const std::string& key, const std::strin
}

uint64_t Utility::getResponseStatus(const ResponseHeaderMap& headers) {
auto status = Utility::getResponseStatusNoThrow(headers);
if (!status.has_value()) {
throw CodecClientException(":status must be specified and a valid unsigned long");
}
return status.value();
}

absl::optional<uint64_t> Utility::getResponseStatusNoThrow(const ResponseHeaderMap& headers) {
const HeaderEntry* header = headers.Status();
uint64_t response_code;
if (!header || !absl::SimpleAtoi(headers.getStatusValue(), &response_code)) {
throw CodecClientException(":status must be specified and a valid unsigned long");
return absl::nullopt;
}
return response_code;
}
Expand Down
7 changes: 7 additions & 0 deletions source/common/http/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,13 @@ std::string makeSetCookieValue(const std::string& key, const std::string& value,
*/
uint64_t getResponseStatus(const ResponseHeaderMap& headers);

/**
* Get the response status from the response headers.
* @param headers supplies the headers to get the status from.
* @return absl::optional<uint64_t> the response code or absl::nullopt if the headers are invalid.
*/
absl::optional<uint64_t> getResponseStatusNoThrow(const ResponseHeaderMap& headers);

/**
* Determine whether these headers are a valid Upgrade request or response.
* This function returns true if the following HTTP headers and values are present:
Expand Down
10 changes: 8 additions & 2 deletions source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,15 +151,21 @@ void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len,
return;
}
quic::QuicSpdyStream::OnInitialHeadersComplete(fin, frame_len, header_list);
ASSERT(headers_decompressed() && !header_list.empty());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really need to be moved down?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the new 11111111...111 test fails that assertion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it fail because of header_list is empty? If so, there is nothing to validate at all. I think an early reset makes sense. TBH I don't know where could it fail in QUICHE stack, maybe Qpack decoding? Ideally, QUICHE should reset the stream in that case.

getResponseStatusNoThrow() uses absl::SimpleAtoi() which doesn't seem to check the status code to be a 3 digit code though. Did I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this is testing for overflow. Envoy allows greater than 3 digit but not overflowing is important.


ENVOY_STREAM_LOG(debug, "Received headers: {}.", *this, header_list.DebugString());
if (fin) {
end_stream_decoded_ = true;
}
std::unique_ptr<Http::ResponseHeaderMapImpl> headers =
quicHeadersToEnvoyHeaders<Http::ResponseHeaderMapImpl>(header_list);
const uint64_t status = Http::Utility::getResponseStatus(*headers);
const absl::optional<uint64_t> optional_status =
Http::Utility::getResponseStatusNoThrow(*headers);
if (!optional_status.has_value()) {
Reset(quic::QUIC_BAD_APPLICATION_PAYLOAD);
return;
}
ASSERT(headers_decompressed() && !header_list.empty());
const uint64_t status = optional_status.value();
if (Http::CodeUtility::is1xx(status)) {
if (status == enumToInt(Http::Code::SwitchingProtocols)) {
// HTTP3 doesn't support the HTTP Upgrade mechanism or 101 (Switching Protocols) status code.
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/quic_listeners/quiche/envoy_quic_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ Http::StreamResetReason quicRstErrorToEnvoyLocalResetReason(quic::QuicRstStreamE
return Http::StreamResetReason::LocalRefusedStreamReset;
case quic::QUIC_STREAM_CONNECTION_ERROR:
return Http::StreamResetReason::ConnectionFailure;
case quic::QUIC_BAD_APPLICATION_PAYLOAD:
return Http::StreamResetReason::ProtocolError;
default:
return Http::StreamResetReason::LocalReset;
}
Expand Down
2 changes: 1 addition & 1 deletion test/extensions/quic_listeners/quiche/envoy_quic_client_stream_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ TEST_P(EnvoyQuicClientStreamTest, ResetUpon101SwitchProtocol) {
const auto result = quic_stream_->encodeHeaders(request_headers_, false);
EXPECT_TRUE(result.ok());

EXPECT_CALL(stream_callbacks_, onResetStream(Http::StreamResetReason::LocalReset, _));
EXPECT_CALL(stream_callbacks_, onResetStream(Http::StreamResetReason::ProtocolError, _));
// Receive several 10x headers, only the first 100 Continue header should be
// delivered.
if (quic::VersionUsesHttp3(quic_version_.transport_version)) {
Expand Down
75 changes: 39 additions & 36 deletions test/integration/protocol_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ TEST_P(ProtocolIntegrationTest, ResponseWithHostHeader) {

// Tests missing headers needed for H/1 codec first line.
TEST_P(DownstreamProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) {
EXCLUDE_UPSTREAM_HTTP3; // buffer bug.
EXCLUDE_UPSTREAM_HTTP3; // tsan flake?
useAccessLog("%RESPONSE_CODE_DETAILS%");
config_helper_.addFilter("{ name: invalid-header-filter, typed_config: { \"@type\": "
"type.googleapis.com/google.protobuf.Empty } }");
Expand Down Expand Up @@ -378,7 +378,7 @@ TEST_P(DownstreamProtocolIntegrationTest, FaultyFilterWithConnect) {
// TODO(danzh) re-enable after plumbing through http2 option
// "allow_connect".
EXCLUDE_DOWNSTREAM_HTTP3;
EXCLUDE_UPSTREAM_HTTP3; // buffer bug.
EXCLUDE_UPSTREAM_HTTP3; // use after free.
// Faulty filter that removed host in a CONNECT request.
config_helper_.addConfigModifier(
[&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
Expand Down Expand Up @@ -1030,7 +1030,7 @@ TEST_P(ProtocolIntegrationTest, HittingEncoderFilterLimit) {
// connection error is not detected under these circumstances.
#if !defined(__APPLE__)
TEST_P(ProtocolIntegrationTest, 100ContinueAndClose) {
EXCLUDE_UPSTREAM_HTTP3;
EXCLUDE_UPSTREAM_HTTP3; // CI fails with 503 in access logs.
testEnvoyHandling100Continue(false, "", true);
}
#endif
Expand Down Expand Up @@ -1060,7 +1060,8 @@ TEST_P(ProtocolIntegrationTest, EnvoyProxyingLateMultiple1xx) {
TEST_P(ProtocolIntegrationTest, TwoRequests) { testTwoRequests(); }

TEST_P(ProtocolIntegrationTest, TwoRequestsWithForcedBackup) {
EXCLUDE_UPSTREAM_HTTP3; // Hits assert in switchStreamBlockState.
// hits assert "Upper stream buffer limit is reached before response body is delivered."
EXCLUDE_UPSTREAM_HTTP3;
testTwoRequests(true);
}

Expand Down Expand Up @@ -1239,57 +1240,54 @@ TEST_P(ProtocolIntegrationTest, 304WithBody) {
}

TEST_P(ProtocolIntegrationTest, OverflowingResponseCode) {
EXCLUDE_UPSTREAM_HTTP3; // Protocol-specific framing.
useAccessLog("%RESPONSE_CODE_DETAILS%");
initialize();

FakeRawConnectionPtr fake_upstream_connection;
// HTTP1, uses a defined protocol which doesn't split up messages into raw byte frames
codec_client_ = makeHttpConnection(lookupPort("http"));
auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);

ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));
ASSERT(fake_upstream_connection != nullptr);

if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) {
// The http1 codec won't send illegal status codes so send raw HTTP/1.1
FakeRawConnectionPtr fake_upstream_connection;
ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));
ASSERT(fake_upstream_connection != nullptr);
ASSERT_TRUE(fake_upstream_connection->write(
"HTTP/1.1 11111111111111111111111111111111111111111111111111111111111111111 OK\r\n",
false));
ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
} else {
Http::Http2::Http2Frame::SettingsFlags settings_flags =
static_cast<Http::Http2::Http2Frame::SettingsFlags>(0);
Http2Frame setting_frame = Http::Http2::Http2Frame::makeEmptySettingsFrame(settings_flags);
// Ack settings
settings_flags = static_cast<Http::Http2::Http2Frame::SettingsFlags>(1); // ack
Http2Frame ack_frame = Http::Http2::Http2Frame::makeEmptySettingsFrame(settings_flags);
ASSERT(fake_upstream_connection->write(std::string(setting_frame))); // empty settings
ASSERT(fake_upstream_connection->write(std::string(ack_frame))); // ack setting
Http::Http2::Http2Frame overflowed_status = Http::Http2::Http2Frame::makeHeadersFrameWithStatus(
"11111111111111111111111111111111111111111111111111111111111111111", 0);
ASSERT_TRUE(fake_upstream_connection->write(std::string(overflowed_status)));
waitForNextUpstreamRequest();
default_response_headers_.setStatus(
"11111111111111111111111111111111111111111111111111111111111111111");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this invalid status code cause getResponseStatusNoThrow() to return nullopt to reset the stream?
I thought we will explicitly check status to be one of 1xx/2xx/3xx/4xx/5xx.

Copy link
Contributor Author

@alyssawilk alyssawilk Mar 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly what happens. We're checking for 1xx below, but because we explicitly call QuicSpdyStream::OnInitialHeadersComplete rather than quic::QuicSpdyClientStream::OnInitialHeadersComplete we were missing at lease some quiche validation checks.
Alternately given we're failing the assert it's possible somewhere in the quiche stack also validated. Instead of resetting on Envoy's invalid status code we could instead reset on (!headers_decompressed() || header_list.empty()) in case there's other quiche validation checks we're failing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quic::QuicSpdyClientStream::OnInitialHeadersComplete() will copy and consume the headers and unblock the stream. Since we want headers to be decoded before decoding any data, skipping QuicSpdyClientStream::OnInitialHeadersComplete() is an easier implementation.

If the ASSERT() earlier-on fails, I suspect even QuicSpdyClientStream::OnInitialHeadersComplete() would fail because of empty headers instead of invalid status code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, the assert fails for no status present. I can bump it back up for the test of "missing status header"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG. That means Envoy won't receive any overflow response status code from QUIC stream, is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!

upstream_request_->encodeHeaders(default_response_headers_, false);
if (upstreamProtocol() == FakeHttpConnection::Type::HTTP2) {
ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect());
} else {
// TODO(#14829) QUIC won't disconnect on invalid messaging.
ASSERT_TRUE(upstream_request_->waitForReset());
}
}

ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
response->waitForEndStream();
EXPECT_EQ("502", response->headers().getStatusValue());
EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("protocol_error"));
}

TEST_P(ProtocolIntegrationTest, MissingStatus) {
EXCLUDE_UPSTREAM_HTTP3; // Protocol-specific framing.
initialize();

FakeRawConnectionPtr fake_upstream_connection;
// HTTP1, uses a defined protocol which doesn't split up messages into raw byte frames
codec_client_ = makeHttpConnection(lookupPort("http"));
auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);

ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));
ASSERT(fake_upstream_connection != nullptr);

if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) {
FakeRawConnectionPtr fake_upstream_connection;
ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));
ASSERT(fake_upstream_connection != nullptr);
ASSERT_TRUE(fake_upstream_connection->write("HTTP/1.1 OK\r\n", false));
ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
} else {
} else if (upstreamProtocol() == FakeHttpConnection::Type::HTTP2) {
FakeRawConnectionPtr fake_upstream_connection;
ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));
Http::Http2::Http2Frame::SettingsFlags settings_flags =
static_cast<Http::Http2::Http2Frame::SettingsFlags>(0);
Http2Frame setting_frame = Http::Http2::Http2Frame::makeEmptySettingsFrame(settings_flags);
Expand All @@ -1300,9 +1298,14 @@ TEST_P(ProtocolIntegrationTest, MissingStatus) {
ASSERT(fake_upstream_connection->write(std::string(ack_frame))); // ack setting
Http::Http2::Http2Frame missing_status = Http::Http2::Http2Frame::makeHeadersFrameNoStatus(0);
ASSERT_TRUE(fake_upstream_connection->write(std::string(missing_status)));
ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
} else {
waitForNextUpstreamRequest();
default_response_headers_.removeStatus();
upstream_request_->encodeHeaders(default_response_headers_, false);
ASSERT_TRUE(upstream_request_->waitForReset());
}

ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect watiForDisconnect() here and above will be fixed by #15753. Mind adding a TODO here?

response->waitForEndStream();
EXPECT_EQ("502", response->headers().getStatusValue());
}
Expand All @@ -1311,7 +1314,7 @@ TEST_P(ProtocolIntegrationTest, MissingStatus) {
TEST_P(DownstreamProtocolIntegrationTest, LargeCookieParsingConcatenated) {
// TODO(danzh) re-enable this test after quic headers size become configurable.
EXCLUDE_DOWNSTREAM_HTTP3
EXCLUDE_UPSTREAM_HTTP3;
EXCLUDE_UPSTREAM_HTTP3; // QUIC_STREAM_EXCESSIVE_LOAD
initialize();

codec_client_ = makeHttpConnection(lookupPort("http"));
Expand All @@ -1336,7 +1339,7 @@ TEST_P(DownstreamProtocolIntegrationTest, LargeCookieParsingConcatenated) {
TEST_P(DownstreamProtocolIntegrationTest, LargeCookieParsingMany) {
// TODO(danzh) re-enable this test after quic headers size become configurable.
EXCLUDE_DOWNSTREAM_HTTP3
EXCLUDE_UPSTREAM_HTTP3;
EXCLUDE_UPSTREAM_HTTP3; // QUIC_STREAM_EXCESSIVE_LOAD
// Set header count limit to 2010.
uint32_t max_count = 2010;
config_helper_.addConfigModifier(
Expand Down Expand Up @@ -1549,7 +1552,7 @@ TEST_P(DownstreamProtocolIntegrationTest, LargeRequestUrlRejected) {
TEST_P(DownstreamProtocolIntegrationTest, LargeRequestUrlAccepted) {
// TODO(danzh) re-enable this test after quic headers size become configurable.
EXCLUDE_DOWNSTREAM_HTTP3
EXCLUDE_UPSTREAM_HTTP3;
EXCLUDE_UPSTREAM_HTTP3; // requires configurable header limits.
// Send one 95 kB URL with limit 96 kB headers.
testLargeRequestUrl(95, 96);
}
Expand All @@ -1561,7 +1564,7 @@ TEST_P(DownstreamProtocolIntegrationTest, LargeRequestHeadersRejected) {

TEST_P(DownstreamProtocolIntegrationTest, LargeRequestHeadersAccepted) {
EXCLUDE_DOWNSTREAM_HTTP3
EXCLUDE_UPSTREAM_HTTP3;
EXCLUDE_UPSTREAM_HTTP3; // requires configurable header limits.
// Send one 95 kB header with limit 96 kB and 100 headers.
testLargeRequestHeaders(95, 1, 96, 100);
}
Expand All @@ -1581,7 +1584,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestHeadersAccepted) {
TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersRejected) {
// QUICHE doesn't limit number of headers.
EXCLUDE_DOWNSTREAM_HTTP3
EXCLUDE_UPSTREAM_HTTP3; // buffer bug.
EXCLUDE_UPSTREAM_HTTP3; // CI asan use-after-free
// Default header (and trailer) count limit is 100.
config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1());
config_helper_.addConfigModifier(setEnableUpstreamTrailersHttp1());
Expand Down Expand Up @@ -1609,7 +1612,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersRejected) {
}

TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersAccepted) {
EXCLUDE_UPSTREAM_HTTP3;
EXCLUDE_UPSTREAM_HTTP3; // assert failure: validHeaderString
// Set header (and trailer) count limit to 200.
uint32_t max_count = 200;
config_helper_.addConfigModifier(
Expand Down