Fixes a bug in Envoy's HTTP/3-to-HTTP/1 proxying when a transfer-enco… (#39589)

Commit Message: QUIC Stream: Fixes a bug in Envoy's HTTP/3-to-HTTP/1
proxying when a "transfer-encoding" header is incorrectly appended.
Additional Description: Envoy, when proxying via hyperloop to HTTP/1
backend, incorrectly interprets the end of an HTTP/3 HEADERS frame
contained in a QUIC STREAM frame with the end_stream/fin bit set to
true. This causes the HTTP/1 codec to mistakenly add a
"transfer-encoding" header. This CL addresses this by inspecting the
data before encoding headers. For requests containing only headers, it
now explicitly signals the end of the stream with the "headers-only"
indicator, which is the correct way to denote this in HTTP/3 (unlike the
"fin" flag). This prevents the incorrect "transfer-encoding" header from
being added.
Risk Level: low, guarded by runtime guard
Testing: internally tested with google3 e2e tests
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
envoy_reloadable_features_quic_signal_headers_only_to_http1_backend
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API
Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

---------

Signed-off-by: Ting Pan <panting@google.com>

Author: fishpan1209

changelogs/current.yaml

@@ -74,6 +74,10 @@ bug_fixes:
   change: |
     Fixes a bug where the lifetime of the HttpConnectionManager's ActiveStream can be out of sync
     with the lifetime of the codec stream.
+- area: quic
+  change: |
+    Fixes a bug in Envoy's HTTP/3-to-HTTP/1 proxying when a "transfer-encoding" header is incorrectly appended.
+    Protected by runtime guard ``envoy.reloadable_features.quic_signal_headers_only_to_http1_backend``.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

source/common/quic/envoy_quic_server_stream.cc

@@ -164,7 +164,13 @@ void EnvoyQuicServerStream::OnInitialHeadersComplete(bool fin, size_t frame_len,
   }
 
   ENVOY_STREAM_LOG(debug, "Received headers: {}.", *this, header_list.DebugString());
-  if (fin) {
+  const bool headers_only = fin_received() && highest_received_byte_offset() == NumBytesConsumed();
+  const bool end_stream =
+      fin ||
+      (headers_only && Runtime::runtimeFeatureEnabled("envoy.reloadable_features.quic_signal_"
+                                                      "headers_only_to_http1_backend"));
+  ENVOY_STREAM_LOG(debug, "Headers_only: {}, end_stream: {}.", *this, headers_only, end_stream);
+  if (end_stream) {
     end_stream_decoded_ = true;
   }
   saw_regular_headers_ = false;
@@ -217,7 +223,7 @@ void EnvoyQuicServerStream::OnInitialHeadersComplete(bool fin, size_t frame_len,
 
   Http::RequestDecoder* decoder = request_decoder_->get().ptr();
   if (decoder != nullptr) {
-    decoder->decodeHeaders(std::move(headers), /*end_stream=*/fin);
+    decoder->decodeHeaders(std::move(headers), /*end_stream=*/end_stream);
   }
   ConsumeHeaderList();
 }

source/common/runtime/runtime_features.cc

@@ -77,6 +77,7 @@ RUNTIME_GUARD(envoy_reloadable_features_proxy_status_mapping_more_core_response_
 // Ignore the automated "remove this flag" issue: we should keep this for 1 year. Confirm with
 // @danzh2010 or @RyanTheOptimist before removing.
 RUNTIME_GUARD(envoy_reloadable_features_quic_send_server_preferred_address_to_all_clients);
+RUNTIME_GUARD(envoy_reloadable_features_quic_signal_headers_only_to_http1_backend);
 RUNTIME_GUARD(envoy_reloadable_features_quic_upstream_reads_fixed_number_packets);
 RUNTIME_GUARD(envoy_reloadable_features_quic_upstream_socket_use_address_cache_for_read);
 RUNTIME_GUARD(envoy_reloadable_features_report_load_with_rq_issued);

test/common/quic/envoy_quic_server_stream_test.cc

@@ -263,7 +263,7 @@ class EnvoyQuicServerStreamTest : public testing::Test {
 };
 
 TEST_F(EnvoyQuicServerStreamTest, GetRequestAndResponse) {
-  EXPECT_CALL(stream_decoder_, decodeHeaders_(_, /*end_stream=*/false))
+  EXPECT_CALL(stream_decoder_, decodeHeaders_(_, /*end_stream=*/true))
       .WillOnce(Invoke([this](const Http::RequestHeaderMapSharedPtr& headers, bool) {
         EXPECT_EQ(host_, headers->getHostValue());
         EXPECT_EQ("/", headers->getPathValue());
@@ -273,7 +273,6 @@ TEST_F(EnvoyQuicServerStreamTest, GetRequestAndResponse) {
         EXPECT_EQ("a=b; c=d",
                   headers->get(Http::Headers::get().Cookie)[0]->value().getStringView());
       }));
-  EXPECT_CALL(stream_decoder_, decodeData(BufferStringEqual(""), /*end_stream=*/true));
   quiche::HttpHeaderBlock spdy_headers;
   spdy_headers[":authority"] = host_;
   spdy_headers[":method"] = "GET";

test/integration/multiplexed_integration_test.cc

@@ -811,11 +811,6 @@ TEST_P(MetadataIntegrationTest, ConsumeAndInsertRequestMetadata) {
   // Verifies a headers metadata added.
   std::set<std::string> expected_metadata_keys = {"headers"};
   expected_metadata_keys.insert("metadata");
-  if (downstreamProtocol() == Http::CodecType::HTTP3) {
-    // HTTP/3 Sends "end stream" in an empty DATA frame which results in the test filter
-    // adding the "data" metadata header.
-    expected_metadata_keys.insert("data");
-  }
   verifyExpectedMetadata(upstream_request_->metadataMap(), expected_metadata_keys);
 
   // Sends a headers only request with metadata. An empty data frame carries end_stream.
@@ -927,11 +922,6 @@ void MetadataIntegrationTest::verifyHeadersOnlyTest() {
   // Verifies a headers metadata added.
   std::set<std::string> expected_metadata_keys = {"headers"};
   expected_metadata_keys.insert("metadata");
-  if (downstreamProtocol() == Http::CodecType::HTTP3) {
-    // HTTP/3 Sends "end stream" in an empty DATA frame which results in the test filter
-    // adding the "data" metadata header.
-    expected_metadata_keys.insert("data");
-  }
   verifyExpectedMetadata(upstream_request_->metadataMap(), expected_metadata_keys);
 
   // Verifies zero length data received, and end_stream is true.

test/integration/protocol_integration_test.cc

@@ -5742,4 +5742,33 @@ TEST_P(DownstreamProtocolIntegrationTest, DownstreamCxStats) {
   test_server_->waitForCounterGe("http.config_test.downstream_cx_tx_bytes_total", 512);
 }
 
+// When upstream protocol is HTTP1, an OPTIONS request with no body will not
+// append transfer-encoding chunked.
+TEST_P(DownstreamProtocolIntegrationTest, OptionsWithNoBodyNotChunked) {
+  // This test is only relevant to H1 upstream.
+  if (upstreamProtocol() != Http::CodecType::HTTP1) {
+    return;
+  }
+
+  initialize();
+  codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http")));
+
+  Http::TestRequestHeaderMapImpl request_headers{
+      {":method", "OPTIONS"},
+      {":path", "/foo"},
+      {":scheme", "http"},
+      {":authority", "host"},
+      {"access-control-request-method", "GET"},
+      {"origin", "test-origin"},
+  };
+  auto response = codec_client_->makeHeaderOnlyRequest(request_headers);
+  waitForNextUpstreamRequest();
+  EXPECT_EQ(upstream_request_->headers().TransferEncoding(), nullptr);
+  upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true);
+  ASSERT_TRUE(response->waitForEndStream());
+  EXPECT_TRUE(response->complete());
+  EXPECT_THAT(response->headers(), Http::HttpStatusIs("200"));
+  EXPECT_EQ(response->headers().TransferEncoding(), nullptr);
+}
+
 } // namespace Envoy