quic: enabling some more tests (#15381)

Enabling (most of the) the downstream tests for quic upstream for more coverage.
Enabling cookie tests given Dan's fix
Fixing a couple of tests with filters assuming headers-come-with-fin
Defaulting header size to Envoy defaults.

Risk Level: low (quic / test code)
Testing: yes
Docs Changes: n/a
Release Notes: n/a
part of #14829

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

source/extensions/quic_listeners/quiche/envoy_quic_client_session.cc

@@ -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());

source/extensions/quic_listeners/quiche/envoy_quic_server_session.cc

@@ -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());

test/extensions/quic_listeners/quiche/integration/quic_protocol_integration_test.cc

@@ -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})),
+                         HttpProtocolIntegrationTest::protocolTestParamsToString);
 
 // This will run with HTTP/1 and HTTP/2 downstream, and HTTP/3 upstream.
 INSTANTIATE_TEST_SUITE_P(Protocols, ProtocolIntegrationTest,

test/integration/filters/add_body_filter.cc

@@ -13,7 +13,7 @@
 
 namespace Envoy {
 
-// A test filter that inserts body to a header only request/response.
+// A test filter that adds body data to a request/response without body payload.
 class AddBodyStreamFilter : public Http::PassThroughFilter {
 public:
   constexpr static char name[] = "add-body-filter";
@@ -24,11 +24,22 @@ class AddBodyStreamFilter : public Http::PassThroughFilter {
       Buffer::OwnedImpl body("body");
       headers.setContentLength(body.length());
       decoder_callbacks_->addDecodedData(body, false);
+    } else {
+      headers.removeContentLength();
     }
 
     return Http::FilterHeadersStatus::Continue;
   }
 
+  Http::FilterDataStatus encodeData(Buffer::Instance& data, bool end_stream) override {
+    // Ensure that encodeData is only called for HTTP/3 (where protocol is set at the
+    // connection level). In HTTP/3 the FIN arrives separately so we will get
+    // encodeData() with an empty body.
+    ASSERT(end_stream == false || decoder_callbacks_->connection()->streamInfo().protocol());
+    data.add("body");
+    return Http::FilterDataStatus::Continue;
+  }
+
   Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap& headers,
                                           bool end_stream) override {
     if (end_stream) {

test/integration/filters/continue_headers_only_inject_body_filter.cc

@@ -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.
+  // 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>,

test/integration/protocol_integration_test.cc

@@ -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,12 @@ 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) {
+    // TODO(alyssawilk) http3 stats.
+    Stats::CounterSharedPtr counter =
+        TestUtility::findCounter(stats, "cluster.cluster_0.upstream_rq_tx_reset");
+    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 +514,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 +692,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 +737,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;
   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 +810,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 +1017,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 +1047,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 +1279,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 +1302,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 +1479,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 +1490,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 +1506,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 +1534,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 +1567,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 +1642,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 +1863,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 +1908,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 +1939,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 +2169,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 +2184,7 @@ TEST_P(DownstreamProtocolIntegrationTest, Test100AndDisconnect) {
 }
 
 TEST_P(DownstreamProtocolIntegrationTest, Test100AndDisconnectLegacy) {
+  EXCLUDE_UPSTREAM_HTTP3;
   config_helper_.addRuntimeOverride("envoy.reloadable_features.allow_500_after_100", "false");
 
   initialize();