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: enabling some more tests #15381

Merged
merged 9 commits into from
Mar 15, 2021
Merged

quic: enabling some more tests #15381

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
df50101
quic: enabling some more tests
alyssawilk Mar 9, 2021
aaddfd5
spelling
alyssawilk Mar 9, 2021
4cedd0f
todo
alyssawilk Mar 10, 2021
d5ccacc
more tests
alyssawilk Mar 10, 2021
e37e311
format
alyssawilk Mar 10, 2021
55b6e6a
comments
alyssawilk Mar 10, 2021
682b110
format
alyssawilk Mar 11, 2021
d202922
comments
alyssawilk Mar 11, 2021
5a593c0
TODO
alyssawilk Mar 11, 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
5 changes: 4 additions & 1 deletion source/extensions/quic_listeners/quiche/envoy_quic_client_session.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ EnvoyQuicClientSession::EnvoyQuicClientSession(
: QuicFilterManagerConnectionImpl(*connection, dispatcher, send_buffer_limit),
quic::QuicSpdyClientSession(config, supported_versions, connection.release(), server_id,
crypto_config, push_promise_index),
host_name_(server_id.host()) {}
host_name_(server_id.host()) {
// HTTP/3 header limits should be configurable, but for now hard-code to Envoy defaults.
set_max_inbound_header_list_size(Http::DEFAULT_MAX_REQUEST_HEADERS_KB * 1000);
}

EnvoyQuicClientSession::~EnvoyQuicClientSession() {
ASSERT(!connection()->connected());
Expand Down
5 changes: 4 additions & 1 deletion source/extensions/quic_listeners/quiche/envoy_quic_server_session.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ EnvoyQuicServerSession::EnvoyQuicServerSession(
: quic::QuicServerSessionBase(config, supported_versions, connection.get(), visitor, helper,
crypto_config, compressed_certs_cache),
QuicFilterManagerConnectionImpl(*connection, dispatcher, send_buffer_limit),
quic_connection_(std::move(connection)), listener_config_(listener_config) {}
quic_connection_(std::move(connection)), listener_config_(listener_config) {
// HTTP/3 header limits should be configurable, but for now hard-code to Envoy defaults.
set_max_inbound_header_list_size(Http::DEFAULT_MAX_REQUEST_HEADERS_KB * 1000);
}

EnvoyQuicServerSession::~EnvoyQuicServerSession() {
ASSERT(!quic_connection_->connected());
Expand Down
7 changes: 5 additions & 2 deletions test/extensions/quic_listeners/quiche/integration/quic_protocol_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@

namespace Envoy {

// We do not yet run QUIC downstream tests.
GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(DownstreamProtocolIntegrationTest);
// TODO(danzh) this should run with QUIC downstream instead of HTTP2.
INSTANTIATE_TEST_SUITE_P(Protocols, DownstreamProtocolIntegrationTest,
testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams(
{Http::CodecClient::Type::HTTP2}, {FakeHttpConnection::Type::HTTP3})),
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest to test HTTP and HTTP2 for downstream as well. I will add another test suite for downstream HTTP3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

those are tested in test/integration/instantiate_protocol_integration_test.cc so I think we're good?

Copy link
Contributor

Choose a reason for hiding this comment

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

I find a test for h3 downstream work for H1 but not H2 upstream. So worth to run both I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about I land this as-is, and after yours is, see if adding the extras nets anything new? I'm mildly skeptical it will and either way I think incremental change should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

HttpProtocolIntegrationTest::protocolTestParamsToString);

// This will run with HTTP/1 and HTTP/2 downstream, and HTTP/3 upstream.
INSTANTIATE_TEST_SUITE_P(Protocols, ProtocolIntegrationTest,
Expand Down
9 changes: 9 additions & 0 deletions test/integration/filters/add_body_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,20 @@ class AddBodyStreamFilter : public Http::PassThroughFilter {
Buffer::OwnedImpl body("body");
headers.setContentLength(body.length());
decoder_callbacks_->addDecodedData(body, false);
} else {
headers.removeContentLength();
Comment on lines +27 to +28
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we hit this if this is used with header only requests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

header only request doesn't mean the fin bit arrives with the headers, in the case of QUIC.
will comment.

}

return Http::FilterHeadersStatus::Continue;
}

Http::FilterDataStatus encodeData(Buffer::Instance& data, bool end_stream) override {
// For HTTP/3, there's no headers-only streams so the data will be added here.
ASSERT(end_stream == false || decoder_callbacks_->connection()->streamInfo().protocol());
Copy link
Contributor

Choose a reason for hiding this comment

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

What is decoder_callbacks_->connection()->streamInfo().protocol() for? comment?

data.add("body");
return Http::FilterDataStatus::Continue;
}

Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap& headers,
bool end_stream) override {
if (end_stream) {
Expand Down
33 changes: 29 additions & 4 deletions test/integration/filters/continue_headers_only_inject_body_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,42 @@ class ContinueHeadersOnlyInjectBodyFilter : public Http::PassThroughFilter {
return Http::FilterHeadersStatus::ContinueAndDontEndStream;
}

Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap& headers, bool) override {
Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap& headers,
bool end_stream) override {
headers.setContentLength(body_.length());
encoder_callbacks_->dispatcher().post([this]() -> void {
encoder_callbacks_->dispatcher().post([this, end_stream]() -> void {
posted_ = true;
Buffer::OwnedImpl buffer(body_);
encoder_callbacks_->injectEncodedDataToFilterChain(buffer, true);
encoder_callbacks_->injectEncodedDataToFilterChain(buffer, end_stream);
if (encoded_) {
encoder_callbacks_->continueEncoding();
}
});
return Http::FilterHeadersStatus::ContinueAndDontEndStream;
if (end_stream) {
return Http::FilterHeadersStatus::ContinueAndDontEndStream;
} else {
return Http::FilterHeadersStatus::Continue;
}
}

Http::FilterDataStatus encodeData(Buffer::Instance&, bool) override {
encoded_ = true;
if (!posted_) {
return Http::FilterDataStatus::StopIterationAndBuffer;
}
return Http::FilterDataStatus::Continue;
}

private:
constexpr static absl::string_view body_ = "body";
// For HTTP/3 upstream, the headers and fin will arrive separately.
Copy link
Contributor

Choose a reason for hiding this comment

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

so when we say that HTTP/3 doesn't have header only its because we'd at least expect to see a headers(end_stream=false) with an empty data(end_stream=true)? Seems not great that we have all these differences in callback usage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

largely these tests make sure you can go from
headers + fin
to
headers + body + fin.
for QUIC, we just insert the data at a different part of the pipeline. I still think it's important to make sure it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense! I think I'm just worried about filter authors having to be aware of all these variations in callback patterns, but this isn't something we need to address in this PR

// Make sure that the body is added, and then continue encoding occurs once.
// If encodeData hits before the post, encodeData will stop iteration to ensure
// the fin is not passed on, and when the post happens it will resume
// encoding.
// If the post happens first, encodeData can simply continue.
bool posted_{};
bool encoded_{};
};

static Registry::RegisterFactory<SimpleFilterConfig<ContinueHeadersOnlyInjectBodyFilter>,
Expand Down
48 changes: 36 additions & 12 deletions test/integration/protocol_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,6 @@ TEST_P(ProtocolIntegrationTest, AddBodyToRequestAndWaitForIt) {
}

TEST_P(ProtocolIntegrationTest, AddBodyToResponseAndWaitForIt) {
EXCLUDE_UPSTREAM_HTTP3;
// filters are prepended, so add them in reverse order
config_helper_.addFilter(R"EOF(
name: add-body-filter
Expand All @@ -235,7 +234,6 @@ TEST_P(ProtocolIntegrationTest, AddBodyToResponseAndWaitForIt) {
}

TEST_P(ProtocolIntegrationTest, ContinueHeadersOnlyInjectBodyFilter) {
EXCLUDE_UPSTREAM_HTTP3;
config_helper_.addFilter(R"EOF(
name: continue-headers-only-inject-body-filter
typed_config:
Expand Down Expand Up @@ -425,7 +423,6 @@ TEST_P(DownstreamProtocolIntegrationTest, MissingHeadersLocalReply) {

// Regression test for https://github.com/envoyproxy/envoy/issues/10270
TEST_P(ProtocolIntegrationTest, LongHeaderValueWithSpaces) {
EXCLUDE_UPSTREAM_HTTP3;
// Header with at least 20kb of spaces surrounded by non-whitespace characters to ensure that
// dispatching is split across 2 dispatch calls. This threshold comes from Envoy preferring 16KB
// reads, which the buffer rounds up to about 20KB when allocating slices in
Expand Down Expand Up @@ -463,7 +460,7 @@ TEST_P(ProtocolIntegrationTest, LongHeaderValueWithSpaces) {
}

TEST_P(ProtocolIntegrationTest, Retry) {
EXCLUDE_UPSTREAM_HTTP3;
EXCLUDE_UPSTREAM_HTTP3; // flakes with CHECK failed: (max_plaintext_size_) >= (PacketSize()).
initialize();
codec_client_ = makeHttpConnection(lookupPort("http"));
auto response = codec_client_->makeRequestWithBody(
Expand Down Expand Up @@ -501,6 +498,11 @@ TEST_P(ProtocolIntegrationTest, Retry) {
TestUtility::findCounter(stats, "cluster.cluster_0.http2.tx_reset");
ASSERT_NE(nullptr, counter);
EXPECT_EQ(1L, counter->value());
} else if (upstreamProtocol() == FakeHttpConnection::Type::HTTP3) {
Stats::CounterSharedPtr counter =
TestUtility::findCounter(stats, "cluster.cluster_0.upstream_rq_tx_reset");
Copy link
Contributor

Choose a reason for hiding this comment

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

do we not want a http3 stats scope to match the other branches here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think landing this will help a few tests over in #15424 so would prefer to fix in a follow-up. added TODO!

ASSERT_NE(nullptr, counter);
EXPECT_EQ(1L, counter->value());
} else {
Stats::CounterSharedPtr counter =
TestUtility::findCounter(stats, "cluster.cluster_0.http1.dropped_headers_with_underscores");
Expand All @@ -509,7 +511,7 @@ TEST_P(ProtocolIntegrationTest, Retry) {
}

TEST_P(ProtocolIntegrationTest, RetryStreaming) {
EXCLUDE_UPSTREAM_HTTP3;
EXCLUDE_UPSTREAM_HTTP3; // flakes with CHECK failed: (max_plaintext_size_) >= (PacketSize()).
initialize();
codec_client_ = makeHttpConnection(lookupPort("http"));
auto encoder_decoder =
Expand Down Expand Up @@ -687,6 +689,7 @@ TEST_P(ProtocolIntegrationTest, RetryStreamingCancelDueToBufferOverflow) {
// Tests that the x-envoy-attempt-count header is properly set on the upstream request and the
// downstream response, and updated after the request is retried.
TEST_P(DownstreamProtocolIntegrationTest, RetryAttemptCountHeader) {
EXCLUDE_UPSTREAM_HTTP3;
auto host = config_helper_.createVirtualHost("host", "/test_retry");
host.set_include_request_attempt_count(true);
host.set_include_attempt_count_in_response(true);
Expand Down Expand Up @@ -731,6 +734,7 @@ TEST_P(DownstreamProtocolIntegrationTest, RetryAttemptCountHeader) {
// The retry priority will always target P1, which would otherwise never be hit due to P0 being
// healthy.
TEST_P(DownstreamProtocolIntegrationTest, RetryPriority) {
EXCLUDE_UPSTREAM_HTTP3;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe include some indication of why these are failing under http/3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's a TODO to address all of these where I declare the macro so if I can avoid classifying for now that'd be helpful (I have WIP PRs for about 80% but I don't recall offhand which are which =P)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, sgtm!

const Upstream::HealthyLoad healthy_priority_load({0u, 100u});
const Upstream::DegradedLoad degraded_priority_load({0u, 100u});
NiceMock<Upstream::MockRetryPriority> retry_priority(healthy_priority_load,
Expand Down Expand Up @@ -803,6 +807,7 @@ TEST_P(DownstreamProtocolIntegrationTest, RetryPriority) {
// the same host. With a total of two upstream hosts, this should result in us continuously sending
// requests to the same host.
TEST_P(DownstreamProtocolIntegrationTest, RetryHostPredicateFilter) {
EXCLUDE_UPSTREAM_HTTP3;
TestHostPredicateFactory predicate_factory;
Registry::InjectFactory<Upstream::RetryHostPredicateFactory> inject_factory(predicate_factory);

Expand Down Expand Up @@ -1009,7 +1014,7 @@ TEST_P(ProtocolIntegrationTest, HittingEncoderFilterLimit) {
// connection error is not detected under these circumstances.
#if !defined(__APPLE__)
TEST_P(ProtocolIntegrationTest, 100ContinueAndClose) {
EXCLUDE_UPSTREAM_HTTP3; // TODO(#14829) 503
EXCLUDE_UPSTREAM_HTTP3;
testEnvoyHandling100Continue(false, "", true);
}
#endif
Expand Down Expand Up @@ -1039,7 +1044,7 @@ TEST_P(ProtocolIntegrationTest, EnvoyProxyingLateMultiple1xx) {
TEST_P(ProtocolIntegrationTest, TwoRequests) { testTwoRequests(); }

TEST_P(ProtocolIntegrationTest, TwoRequestsWithForcedBackup) {
EXCLUDE_UPSTREAM_HTTP3;
EXCLUDE_UPSTREAM_HTTP3; // Hits assert in switchStreamBlockState.
testTwoRequests(true);
}

Expand Down Expand Up @@ -1271,6 +1276,7 @@ TEST_P(ProtocolIntegrationTest, MissingStatus) {

// Validate that lots of tiny cookies doesn't cause a DoS (single cookie header).
TEST_P(DownstreamProtocolIntegrationTest, LargeCookieParsingConcatenated) {
EXCLUDE_UPSTREAM_HTTP3;
initialize();

codec_client_ = makeHttpConnection(lookupPort("http"));
Expand All @@ -1293,6 +1299,7 @@ TEST_P(DownstreamProtocolIntegrationTest, LargeCookieParsingConcatenated) {

// Validate that lots of tiny cookies doesn't cause a DoS (many cookie headers).
TEST_P(DownstreamProtocolIntegrationTest, LargeCookieParsingMany) {
EXCLUDE_UPSTREAM_HTTP3;
// Set header count limit to 2010.
uint32_t max_count = 2010;
config_helper_.addConfigModifier(
Expand Down Expand Up @@ -1469,6 +1476,7 @@ TEST_P(DownstreamProtocolIntegrationTest, LargeRequestUrlRejected) {
}

TEST_P(DownstreamProtocolIntegrationTest, LargeRequestUrlAccepted) {
EXCLUDE_UPSTREAM_HTTP3;
// Send one 95 kB URL with limit 96 kB headers.
testLargeRequestUrl(95, 96);
}
Expand All @@ -1479,6 +1487,7 @@ TEST_P(DownstreamProtocolIntegrationTest, LargeRequestHeadersRejected) {
}

TEST_P(DownstreamProtocolIntegrationTest, LargeRequestHeadersAccepted) {
EXCLUDE_UPSTREAM_HTTP3;
// Send one 95 kB header with limit 96 kB and 100 headers.
testLargeRequestHeaders(95, 1, 96, 100);
}
Expand Down Expand Up @@ -1521,6 +1530,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersRejected) {
}

TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersAccepted) {
EXCLUDE_UPSTREAM_HTTP3;
// Set header (and trailer) count limit to 200.
uint32_t max_count = 200;
config_helper_.addConfigModifier(
Expand Down Expand Up @@ -1553,23 +1563,27 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersAccepted) {
// This test uses an Http::HeaderMapImpl instead of an Http::TestHeaderMapImpl to avoid
// time-consuming byte size validations that will cause this test to timeout.
TEST_P(DownstreamProtocolIntegrationTest, ManyRequestHeadersTimeout) {
EXCLUDE_UPSTREAM_HTTP3;
// Set timeout for 5 seconds, and ensure that a request with 10k+ headers can be sent.
testManyRequestHeaders(std::chrono::milliseconds(5000));
}

TEST_P(DownstreamProtocolIntegrationTest, LargeRequestTrailersAccepted) {
EXCLUDE_UPSTREAM_HTTP3;
config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1());
testLargeRequestTrailers(60, 96);
}

TEST_P(DownstreamProtocolIntegrationTest, LargeRequestTrailersRejected) {
EXCLUDE_UPSTREAM_HTTP3; // TODO(danzh) QuicMemSliceImpl flake
config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1());
testLargeRequestTrailers(66, 60);
}

// This test uses an Http::HeaderMapImpl instead of an Http::TestHeaderMapImpl to avoid
// time-consuming byte size verification that will cause this test to timeout.
TEST_P(DownstreamProtocolIntegrationTest, ManyTrailerHeaders) {
EXCLUDE_UPSTREAM_HTTP3;
max_request_headers_kb_ = 96;
max_request_headers_count_ = 20005;

Expand Down Expand Up @@ -1624,7 +1638,6 @@ TEST_P(ProtocolIntegrationTest, LargeRequestMethod) {
// There will be no upstream connections for HTTP/1 downstream, we need to
// test the full mesh regardless.
testing_upstream_intentionally_ = true;
EXCLUDE_UPSTREAM_HTTP3; // TODO(#14829) Rejected with QUIC_STREAM_EXCESSIVE_LOAD
const std::string long_method = std::string(48 * 1024, 'a');
const Http::TestRequestHeaderMapImpl request_headers{{":method", long_method},
{":path", "/test/long/url"},
Expand Down Expand Up @@ -1846,7 +1859,12 @@ name: encode-headers-return-stop-all-filter

response->waitForEndStream();
ASSERT_TRUE(response->complete());
EXPECT_EQ(count_ * size_ + added_decoded_data_size_, response->body().size());
// Data is added in encodeData for all protocols, and encodeTrailers for HTTP/2 and above.
int times_added = (upstreamProtocol() == FakeHttpConnection::Type::HTTP1 ||
downstreamProtocol() == Http::CodecClient::Type::HTTP1)
? 1
: 2;
EXPECT_EQ(count_ * size_ + added_decoded_data_size_ * times_added, response->body().size());
}

// Tests encodeHeaders() returns StopAllIterationAndWatermark.
Expand Down Expand Up @@ -1886,13 +1904,17 @@ name: encode-headers-return-stop-all-filter

response->waitForEndStream();
ASSERT_TRUE(response->complete());
EXPECT_EQ(count_ * size_ + added_decoded_data_size_, response->body().size());
// Data is added in encodeData for all protocols, and encodeTrailers for HTTP/2 and above.
int times_added = (upstreamProtocol() == FakeHttpConnection::Type::HTTP1 ||
downstreamProtocol() == Http::CodecClient::Type::HTTP1)
? 1
: 2;
EXPECT_EQ(count_ * size_ + added_decoded_data_size_ * times_added, response->body().size());
}

// Per https://github.com/envoyproxy/envoy/issues/7488 make sure we don't
// combine set-cookie headers
TEST_P(ProtocolIntegrationTest, MultipleSetCookies) {
EXCLUDE_UPSTREAM_HTTP3; // Multiple cookies not yet supported.
initialize();

codec_client_ = makeHttpConnection(lookupPort("http"));
Expand All @@ -1913,7 +1935,7 @@ TEST_P(ProtocolIntegrationTest, MultipleSetCookies) {

// Resets the downstream stream immediately and verifies that we clean up everything.
TEST_P(ProtocolIntegrationTest, TestDownstreamResetIdleTimeout) {
EXCLUDE_UPSTREAM_HTTP3;
EXCLUDE_UPSTREAM_HTTP3; // TODO(danzh) QuicMemSliceImpl flake
useAccessLog("%RESPONSE_FLAGS% %RESPONSE_CODE_DETAILS%");
config_helper_.setDownstreamHttpIdleTimeout(std::chrono::milliseconds(100));

Expand Down Expand Up @@ -2143,6 +2165,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ConnectStreamRejection) {

// Regression test for https://github.com/envoyproxy/envoy/issues/12131
TEST_P(DownstreamProtocolIntegrationTest, Test100AndDisconnect) {
EXCLUDE_UPSTREAM_HTTP3;
initialize();
codec_client_ = makeHttpConnection(lookupPort("http"));
auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
Expand All @@ -2157,6 +2180,7 @@ TEST_P(DownstreamProtocolIntegrationTest, Test100AndDisconnect) {
}

TEST_P(DownstreamProtocolIntegrationTest, Test100AndDisconnectLegacy) {
EXCLUDE_UPSTREAM_HTTP3;
config_helper_.addRuntimeOverride("envoy.reloadable_features.allow_500_after_100", "false");

initialize();
Expand Down