Skip to content

http3: turning up more tests #15853

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

Merged
alyssawilk merged 11 commits into from
Apr 13, 2021
Merged

http3: turning up more tests #15853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
53cd6cf
WIP
alyssawilk Apr 5, 2021
4022687
http3: turning up the multiplexed integration test
alyssawilk Apr 6, 2021
df4e7b6
clang
alyssawilk Apr 6, 2021
16b749b
Merge branch 'main' into mux2
alyssawilk Apr 8, 2021
c90dbe3
Merge branch 'main' into mux2
alyssawilk Apr 8, 2021
9eccbae
Merge branch 'main' into mux2
alyssawilk Apr 8, 2021
91ca778
comment
alyssawilk Apr 8, 2021
ec4b8ec
comments + merge fixes
alyssawilk Apr 8, 2021
923657f
more time?
alyssawilk Apr 8, 2021
a90445f
re-disabling giant tests
alyssawilk Apr 8, 2021
5a5f3f8
ffs
alyssawilk Apr 12, 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
1 change: 0 additions & 1 deletion source/common/quic/envoy_quic_server_stream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,6 @@ void EnvoyQuicServerStream::OnHeadersTooLarge() {

void EnvoyQuicServerStream::maybeDecodeTrailers() {
if (sequencer()->IsClosed() && !FinishedReadingTrailers()) {
ASSERT(!received_trailers().empty());
Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 Apr 6, 2021

Choose a reason for hiding this comment

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

some h2 protocol test sends empty trailer?

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Apr 6, 2021

Choose a reason for hiding this comment

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

yeah, the autonomous upstream sends empty trailers for H2/H3 by default

// Only decode trailers after finishing decoding body.
end_stream_decoded_ = true;
request_decoder_->decodeTrailers(
Expand Down
8 changes: 4 additions & 4 deletions test/integration/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,14 +335,14 @@ envoy_cc_test(
)

envoy_cc_test(
name = "http2_integration_test",
name = "multiplexed_integration_test",
srcs = [
"http2_integration_test.cc",
"http2_integration_test.h",
"multiplexed_integration_test.cc",
"multiplexed_integration_test.h",
],
shard_count = 4,
deps = [
":http_integration_lib",
":http_protocol_integration_lib",
"//source/common/buffer:buffer_lib",
"//source/common/http:header_map_lib",
"//source/extensions/filters/http/buffer:config",
Expand Down
9 changes: 9 additions & 0 deletions test/integration/http_protocol_integration.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,16 @@ std::vector<HttpProtocolTestParams> HttpProtocolIntegrationTest::getProtocolTest
for (auto ip_version : TestEnvironment::getIpVersionsForTest()) {
for (auto downstream_protocol : downstream_protocols) {
for (auto upstream_protocol : upstream_protocols) {
#ifdef ENVOY_ENABLE_QUIC
ret.push_back(HttpProtocolTestParams{ip_version, downstream_protocol, upstream_protocol});
#else
if (downstream_protocol == Http::CodecClient::Type::HTTP3 ||
upstream_protocol == FakeHttpConnection::Type::HTTP3) {
ENVOY_LOG_MISC(warn, "Skipping HTTP/3 as support is compiled out");
} else {
ret.push_back(HttpProtocolTestParams{ip_version, downstream_protocol, upstream_protocol});
}
#endif
}
}
}
Expand Down
62 changes: 48 additions & 14 deletions test/integration/http2_integration_test.cc → ...tegration/multiplexed_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#include "test/integration/http2_integration_test.h"
#include "test/integration/multiplexed_integration_test.h"

#include <algorithm>
#include <string>
Expand All @@ -24,46 +24,66 @@ using ::testing::MatchesRegex;

namespace Envoy {

// TODO(#2557) fix all the failures.
#define EXCLUDE_DOWNSTREAM_HTTP3 \
if (downstreamProtocol() == Http::CodecClient::Type::HTTP3) { \
return; \
}

INSTANTIATE_TEST_SUITE_P(IpVersions, Http2IntegrationTest,
testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
TestUtility::ipTestParamsToString);
testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams(
{Http::CodecClient::Type::HTTP2, Http::CodecClient::Type::HTTP3},
{FakeHttpConnection::Type::HTTP1})),
HttpProtocolIntegrationTest::protocolTestParamsToString);

TEST_P(Http2IntegrationTest, RouterRequestAndResponseWithBodyNoBuffer) {
testRouterRequestAndResponseWithBody(1024, 512, false, false);
}

TEST_P(Http2IntegrationTest, RouterRequestAndResponseWithGiantBodyNoBuffer) {
testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, false);
EXCLUDE_DOWNSTREAM_HTTP3; // sort out timeouts
testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, false, nullptr,
TSAN_TIMEOUT_FACTOR * TestUtility::DefaultTimeout);
Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 Apr 12, 2021

Choose a reason for hiding this comment

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

Do they simply timed out?
Mind adding TODO for me to re-enable it after #15865?

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Apr 13, 2021

Choose a reason for hiding this comment

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

Unsure. There's already a TODO to look into the excludes so I'd prefer to not do another push-and-ci cycle to update TODO if that's Ok?

Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 Apr 13, 2021

Choose a reason for hiding this comment

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

Fair enough. I'll keep in mind of these tests.

}

TEST_P(Http2IntegrationTest, FlowControlOnAndGiantBody) {
EXCLUDE_DOWNSTREAM_HTTP3;
config_helper_.setBufferLimits(1024, 1024); // Set buffer limits upstream and downstream.
testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, false);
testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, false, nullptr,
TSAN_TIMEOUT_FACTOR * TestUtility::DefaultTimeout);
}

TEST_P(Http2IntegrationTest, LargeFlowControlOnAndGiantBody) {
EXCLUDE_DOWNSTREAM_HTTP3; // sort out timeouts
config_helper_.setBufferLimits(128 * 1024,
128 * 1024); // Set buffer limits upstream and downstream.
testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, false);
testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, false, nullptr,
TSAN_TIMEOUT_FACTOR * TestUtility::DefaultTimeout);
}

TEST_P(Http2IntegrationTest, RouterRequestAndResponseWithBodyAndContentLengthNoBuffer) {
testRouterRequestAndResponseWithBody(1024, 512, false, true);
}

TEST_P(Http2IntegrationTest, RouterRequestAndResponseWithGiantBodyAndContentLengthNoBuffer) {
testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, true);
EXCLUDE_DOWNSTREAM_HTTP3; // sort out timeouts
testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, true, nullptr,
TSAN_TIMEOUT_FACTOR * TestUtility::DefaultTimeout);
}

TEST_P(Http2IntegrationTest, FlowControlOnAndGiantBodyWithContentLength) {
EXCLUDE_DOWNSTREAM_HTTP3;
config_helper_.setBufferLimits(1024, 1024); // Set buffer limits upstream and downstream.
testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, true);
testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, true, nullptr,
TSAN_TIMEOUT_FACTOR * TestUtility::DefaultTimeout);
}

TEST_P(Http2IntegrationTest, LargeFlowControlOnAndGiantBodyWithContentLength) {
EXCLUDE_DOWNSTREAM_HTTP3; // sort out timeouts
config_helper_.setBufferLimits(128 * 1024,
128 * 1024); // Set buffer limits upstream and downstream.
testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, true);
testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, true, nullptr,
TSAN_TIMEOUT_FACTOR * TestUtility::DefaultTimeout);
}

TEST_P(Http2IntegrationTest, RouterHeaderOnlyRequestAndResponseNoBuffer) {
Expand Down Expand Up @@ -102,6 +122,7 @@ TEST_P(Http2IntegrationTest, LargeRequestTrailersRejected) { testLargeRequestTra

// Verify downstream codec stream flush timeout.
TEST_P(Http2IntegrationTest, CodecStreamIdleTimeout) {
EXCLUDE_DOWNSTREAM_HTTP3;
config_helper_.setBufferLimits(1024, 1024);
config_helper_.addConfigModifier(
[&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
Expand All @@ -125,6 +146,7 @@ TEST_P(Http2IntegrationTest, CodecStreamIdleTimeout) {
}

TEST_P(Http2IntegrationTest, Http2DownstreamKeepalive) {
EXCLUDE_DOWNSTREAM_HTTP3;
constexpr uint64_t interval_ms = 1;
constexpr uint64_t timeout_ms = 250;
config_helper_.addConfigModifier(
Expand Down Expand Up @@ -848,6 +870,7 @@ TEST_P(Http2IntegrationTest, GrpcRetry) { testGrpcRetry(); }

// Verify the case where there is an HTTP/2 codec/protocol error with an active stream.
TEST_P(Http2IntegrationTest, CodecErrorAfterStreamStart) {
EXCLUDE_DOWNSTREAM_HTTP3;
initialize();
codec_client_ = makeHttpConnection(lookupPort("http"));

Expand All @@ -863,7 +886,11 @@ TEST_P(Http2IntegrationTest, CodecErrorAfterStreamStart) {
response->waitForReset();
}

TEST_P(Http2IntegrationTest, BadMagic) {
TEST_P(Http2IntegrationTest, Http2BadMagic) {
if (downstreamProtocol() == Http::CodecClient::Type::HTTP3) {
// The "magic" payload is an HTTP/2 specific thing.
return;
}
Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 Apr 7, 2021

Choose a reason for hiding this comment

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

EXCLUDE_DOWNSTREAM_HTTP3 means need to fix. This one doesn't?

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Apr 8, 2021

Choose a reason for hiding this comment

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

correct. I don't think H3 has the equivalent of the initial H2 magic, so we don't have to replicate this test for H3.
I'll add a better comment next I'm in that file.

Copy link
Copy Markdown
Contributor

@yanavlasov yanavlasov Apr 8, 2021

Choose a reason for hiding this comment

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

Or just rename the test BadHttp2Magic

initialize();
std::string response;
auto connection = createConnectionDriver(
Expand All @@ -876,6 +903,8 @@ TEST_P(Http2IntegrationTest, BadMagic) {
}

TEST_P(Http2IntegrationTest, BadFrame) {
EXCLUDE_DOWNSTREAM_HTTP3; // Needs HTTP/3 "bad frame" equivalent.

initialize();
std::string response;
auto connection = createConnectionDriver(
Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 Apr 6, 2021

Choose a reason for hiding this comment

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

RawConnectionDriver doesn't create a QUIC "connection".

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Apr 6, 2021

Choose a reason for hiding this comment

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

agreed, but I think we need a similar facility to send invalid packets for a quic connection to make sure it's handled gracefully.

Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 Apr 7, 2021

Choose a reason for hiding this comment

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

ACK

Expand All @@ -890,6 +919,7 @@ TEST_P(Http2IntegrationTest, BadFrame) {
// Send client headers, a GoAway and then a body and ensure the full request and
// response are received.
TEST_P(Http2IntegrationTest, GoAway) {
EXCLUDE_DOWNSTREAM_HTTP3; // QuicHttpClientConnectionImpl::goAway NOT_REACHED_GCOVR_EXCL_LINE
Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 Apr 6, 2021

Choose a reason for hiding this comment

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

QUICHE doesn't support client sending GOAWAY as there is no need. AFAIK Envoy doesn't support server push either, why Envoy needs this feature?

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Apr 6, 2021

Choose a reason for hiding this comment

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

That's fine. I think we should test that if the API is called we don't do anything harmful (it should be a no-op like it is for HTTP/1.1, rather than crashing) and we should also test to make sure if a client sends a goaway it is a no-op or treated as a protocol error and again doesn't crash.

Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 Apr 7, 2021

Choose a reason for hiding this comment

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

ACK

Copy link
Copy Markdown
Contributor

@yanavlasov yanavlasov Apr 8, 2021

Choose a reason for hiding this comment

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

It also tests that GOAWAY does not cause existing streams to terminate.

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Apr 8, 2021

Choose a reason for hiding this comment

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

For HTTP/2. For HTTP/3 given it's a protocol error to send goaway client -> server, I think it probably would cause connection termination.

Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 Apr 8, 2021

Choose a reason for hiding this comment

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

It's not a protocol error, but QUICHE client doesn't support it because QUICHE doesn't do server push. I can change NOT_REACHED_GCOVR_EXCL_LINE to log error and return. It won't affect out-going request.

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Apr 12, 2021

Choose a reason for hiding this comment

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

isn't it a protocol error if an H3 server gets a goaway? cc @DavidSchinazi so I don't make things up =P

Copy link
Copy Markdown
Contributor

@DavidSchinazi DavidSchinazi Apr 12, 2021

Choose a reason for hiding this comment

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

@alyssawilk that's no longer true in IETF QUIC. Client-to-server GOAWAY tells the server to stop using server push. https://quicwg.org/base-drafts/draft-ietf-quic-http.html#name-goaway

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Apr 12, 2021

Choose a reason for hiding this comment

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

Ahh, thanks, I was looking at the wrong version of the spec, and Dan is of course just more up to date than me :-P

config_helper_.addFilter(ConfigHelper::defaultHealthCheckFilter());
initialize();

Expand Down Expand Up @@ -1195,6 +1225,7 @@ TEST_P(Http2IntegrationTest, SimultaneousRequestWithBufferLimits) {

// Test downstream connection delayed close processing.
TEST_P(Http2IntegrationTest, DelayedCloseAfterBadFrame) {
EXCLUDE_DOWNSTREAM_HTTP3; // Needs HTTP/3 "bad frame" equivalent.
config_helper_.addConfigModifier(
[](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
hcm) { hcm.mutable_delayed_close_timeout()->set_nanos(1000 * 1000); });
Expand Down Expand Up @@ -1224,6 +1255,7 @@ TEST_P(Http2IntegrationTest, DelayedCloseAfterBadFrame) {

// Test disablement of delayed close processing on downstream connections.
TEST_P(Http2IntegrationTest, DelayedCloseDisabled) {
EXCLUDE_DOWNSTREAM_HTTP3; // Needs HTTP/3 "bad frame" equivalent.
config_helper_.addConfigModifier(
[](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
hcm) { hcm.mutable_delayed_close_timeout()->set_seconds(0); });
Expand Down Expand Up @@ -1359,12 +1391,14 @@ void Http2RingHashIntegrationTest::createUpstreams() {
}

INSTANTIATE_TEST_SUITE_P(IpVersions, Http2RingHashIntegrationTest,
testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
TestUtility::ipTestParamsToString);
testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams(
{Http::CodecClient::Type::HTTP2}, {FakeHttpConnection::Type::HTTP1})),
HttpProtocolIntegrationTest::protocolTestParamsToString);

INSTANTIATE_TEST_SUITE_P(IpVersions, Http2MetadataIntegrationTest,
testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
TestUtility::ipTestParamsToString);
testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams(
{Http::CodecClient::Type::HTTP2}, {FakeHttpConnection::Type::HTTP2})),
HttpProtocolIntegrationTest::protocolTestParamsToString);

void Http2RingHashIntegrationTest::sendMultipleRequests(
int request_bytes, Http::TestRequestHeaderMapImpl headers,
Expand Down
12 changes: 3 additions & 9 deletions test/integration/http2_integration_test.h → ...ntegration/multiplexed_integration_test.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,14 @@
#include "envoy/config/bootstrap/v3/bootstrap.pb.h"
#include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h"

#include "test/integration/http_integration.h"
#include "test/integration/http_protocol_integration.h"

#include "absl/synchronization/mutex.h"
#include "gtest/gtest.h"

namespace Envoy {
class Http2IntegrationTest : public testing::TestWithParam<Network::Address::IpVersion>,
public HttpIntegrationTest {
class Http2IntegrationTest : public HttpProtocolIntegrationTest {
public:
Http2IntegrationTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP2, GetParam()) {}

void SetUp() override { setDownstreamProtocol(Http::CodecClient::Type::HTTP2); }

void simultaneousRequest(int32_t request1_bytes, int32_t request2_bytes);

protected:
Expand Down Expand Up @@ -47,8 +42,7 @@ class Http2RingHashIntegrationTest : public Http2IntegrationTest {
class Http2MetadataIntegrationTest : public Http2IntegrationTest {
public:
void SetUp() override {
setDownstreamProtocol(Http::CodecClient::Type::HTTP2);
setUpstreamProtocol(FakeHttpConnection::Type::HTTP2);
HttpProtocolIntegrationTest::SetUp();
config_helper_.addConfigModifier(
[&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {
RELEASE_ASSERT(bootstrap.mutable_static_resources()->clusters_size() >= 1, "");
Expand Down