-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
quic: enabling some more tests #15381
Changes from 7 commits
df50101
aaddfd5
4cedd0f
d5ccacc
e37e311
55b6e6a
682b110
d202922
5a593c0
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 |
---|---|---|
|
@@ -24,11 +24,21 @@ 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
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 would we hit this if this is used with header only requests? 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. header only request doesn't mean the fin bit arrives with the headers, in the case of QUIC. |
||
} | ||
|
||
return Http::FilterHeadersStatus::Continue; | ||
} | ||
|
||
Http::FilterDataStatus encodeData(Buffer::Instance& data, bool end_stream) override { | ||
// For HTTP/3, the only time the protocol is set at the connection level, there's no | ||
// headers-only streams so the data will be added here. | ||
ASSERT(end_stream == false || decoder_callbacks_->connection()->streamInfo().protocol()); | ||
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. What is 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. This still seems a bit confusing to me, when do we expect end_stream=false and when do we expect the protocol to be set? |
||
data.add("body"); | ||
return Http::FilterDataStatus::Continue; | ||
} | ||
|
||
Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap& headers, | ||
bool end_stream) override { | ||
if (end_stream) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
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. 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 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. largely these tests make sure you can go from 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. 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>, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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: | ||
|
@@ -338,6 +336,7 @@ TEST_P(ProtocolIntegrationTest, ResponseWithHostHeader) { | |
|
||
// Tests missing headers needed for H/1 codec first line. | ||
TEST_P(DownstreamProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) { | ||
EXCLUDE_UPSTREAM_HTTP3; // buffer bug. | ||
useAccessLog("%RESPONSE_CODE_DETAILS%"); | ||
config_helper_.addFilter("{ name: invalid-header-filter, typed_config: { \"@type\": " | ||
"type.googleapis.com/google.protobuf.Empty } }"); | ||
|
@@ -370,6 +369,7 @@ TEST_P(DownstreamProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) { | |
} | ||
|
||
TEST_P(DownstreamProtocolIntegrationTest, FaultyFilterWithConnect) { | ||
EXCLUDE_UPSTREAM_HTTP3; // buffer bug. | ||
// Faulty filter that removed host in a CONNECT request. | ||
config_helper_.addConfigModifier( | ||
[&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& | ||
|
@@ -425,7 +425,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 | ||
|
@@ -463,7 +462,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( | ||
|
@@ -501,6 +500,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"); | ||
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. do we not want a http3 stats scope to match the other branches here? 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 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"); | ||
|
@@ -509,7 +513,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 = | ||
|
@@ -687,6 +691,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); | ||
|
@@ -731,6 +736,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; | ||
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. maybe include some indication of why these are failing under http/3? 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. 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) 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. 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, | ||
|
@@ -803,6 +809,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); | ||
|
||
|
@@ -1009,7 +1016,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 | ||
|
@@ -1039,7 +1046,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); | ||
} | ||
|
||
|
@@ -1271,6 +1278,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")); | ||
|
@@ -1293,6 +1301,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( | ||
|
@@ -1469,6 +1478,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); | ||
} | ||
|
@@ -1479,6 +1489,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); | ||
} | ||
|
@@ -1494,6 +1505,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestHeadersAccepted) { | |
} | ||
|
||
TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersRejected) { | ||
EXCLUDE_UPSTREAM_HTTP3; // buffer bug. | ||
// Default header (and trailer) count limit is 100. | ||
config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); | ||
config_helper_.addConfigModifier(setEnableUpstreamTrailersHttp1()); | ||
|
@@ -1521,6 +1533,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( | ||
|
@@ -1553,23 +1566,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; | ||
|
||
|
@@ -1624,7 +1641,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"}, | ||
|
@@ -1846,7 +1862,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. | ||
|
@@ -1886,13 +1907,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")); | ||
|
@@ -1913,7 +1938,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)); | ||
|
||
|
@@ -2143,6 +2168,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_); | ||
|
@@ -2157,6 +2183,7 @@ TEST_P(DownstreamProtocolIntegrationTest, Test100AndDisconnect) { | |
} | ||
|
||
TEST_P(DownstreamProtocolIntegrationTest, Test100AndDisconnectLegacy) { | ||
EXCLUDE_UPSTREAM_HTTP3; | ||
config_helper_.addRuntimeOverride("envoy.reloadable_features.allow_500_after_100", "false"); | ||
|
||
initialize(); | ||
|
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.
suggest to test HTTP and HTTP2 for downstream as well. I will add another test suite for downstream HTTP3.
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.
those are tested in test/integration/instantiate_protocol_integration_test.cc so I think we're good?
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.
I find a test for h3 downstream work for H1 but not H2 upstream. So worth to run both I think.
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.
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.
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.
SGTM