Make http3 codec not encode empty trailers block (envoyproxy#35907)

Commit Message: Make http3 codec not encode empty trailers block
Additional Description: http3 and `grpc_web_filter` combined resulted in
an incompatibility with Chrome - a grpc web request received a response
with an empty trailers block, which Chrome treats as a network error.
The http2 codec had a special case in it for empty trailers, which was
not mimicked in the http3 codec. This PR makes the http3 codec similar
to the http2 codec in this respect, and adds an integration test
ensuring that this behavior is in all downstream codecs.
Risk Level: Small. It is a behavior change, but from an already
malfunctioning state in a rare edge case.
Testing: Added integration test which fails before the change and passes
after.
Docs Changes: n/a
Release Notes: Yes
Platform Specific Features: n/a

---------

Signed-off-by: Raven Black <[email protected]>

changelogs/current.yaml

@@ -113,6 +113,12 @@ bug_fixes:
   change: |
     RBAC will now allow stat prefixes configured in per-route config to override the base config's
     stat prefix.
+- area: http3
+  change: |
+    Fixed a bug where an empty trailers block could be sent. This would occur if a filter removed
+    the last trailer - a likely occurrence with the ``grpc_web_filter``. This change makes HTTP/3 codec
+    behave the same way HTTP/2 codec does, converting an empty trailers block to no trailers.
+    This behavior can be reverted by setting the runtime guard ``envoy.reloadable_features.http3_remove_empty_trailers`` to ``false``.
 - area: http
   change: |
     Fixed a bug where an incomplete request (missing body or trailers) may be proxied to the upstream when the limit on

source/common/quic/envoy_quic_server_stream.cc

@@ -92,6 +92,15 @@ void EnvoyQuicServerStream::encodeHeaders(const Http::ResponseHeaderMap& headers
 }
 
 void EnvoyQuicServerStream::encodeTrailers(const Http::ResponseTrailerMap& trailers) {
+  if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http3_remove_empty_trailers")) {
+    if (trailers.empty()) {
+      ENVOY_STREAM_LOG(debug, "skipping submitting empty trailers", *this);
+      // Instead of submitting empty trailers, we send empty data with end_stream=true instead.
+      Buffer::OwnedImpl empty_buffer;
+      encodeData(empty_buffer, true);
+      return;
+    }
+  }
   ENVOY_STREAM_LOG(debug, "encodeTrailers: {}.", *this, trailers);
   encodeTrailersImpl(envoyHeadersToHttp2HeaderBlock(trailers));
 }

source/common/runtime/runtime_features.cc

@@ -59,6 +59,7 @@ RUNTIME_GUARD(envoy_reloadable_features_http2_use_oghttp2);
 RUNTIME_GUARD(envoy_reloadable_features_http2_use_visitor_for_data);
 RUNTIME_GUARD(envoy_reloadable_features_http2_validate_authority_with_quiche);
 RUNTIME_GUARD(envoy_reloadable_features_http3_happy_eyeballs);
+RUNTIME_GUARD(envoy_reloadable_features_http3_remove_empty_trailers);
 RUNTIME_GUARD(envoy_reloadable_features_http_filter_avoid_reentrant_local_reply);
 // Delay deprecation and decommission until UHV is enabled.
 RUNTIME_GUARD(envoy_reloadable_features_http_reject_path_with_fragment);

test/integration/BUILD

@@ -817,6 +817,7 @@ envoy_cc_test_library(
         "//test/integration/filters:metadata_control_filter_lib",
         "//test/integration/filters:random_pause_filter_lib",
         "//test/integration/filters:remove_response_headers_lib",
+        "//test/integration/filters:remove_response_trailers_lib",
         "//test/integration/filters:stop_iteration_headers_inject_body",
         "//test/test_common:logging_lib",
         "//test/test_common:threadsafe_singleton_injector_lib",

test/integration/filters/BUILD

@@ -871,6 +871,21 @@ envoy_cc_test_library(
     ],
 )
 
+envoy_cc_test_library(
+    name = "remove_response_trailers_lib",
+    srcs = [
+        "remove_response_trailers.cc",
+    ],
+    deps = [
+        ":common_lib",
+        "//envoy/http:filter_interface",
+        "//envoy/registry",
+        "//envoy/server:filter_config_interface",
+        "//source/extensions/filters/http/common:pass_through_filter_lib",
+        "//test/extensions/filters/http/common:empty_http_filter_config_lib",
+    ],
+)
+
 envoy_cc_test_library(
     name = "repick_cluster_filter_lib",
     srcs = [

test/integration/filters/remove_response_trailers.cc

@@ -0,0 +1,38 @@
+#include "envoy/registry/registry.h"
+#include "envoy/server/filter_config.h"
+
+#include "source/extensions/filters/http/common/pass_through_filter.h"
+
+#include "test/extensions/filters/http/common/empty_http_filter_config.h"
+#include "test/integration/filters/common.h"
+
+namespace Envoy {
+
+// Registers a filter which removes all trailers, leaving an empty trailers block.
+// This resembles the behavior of grpc_web_filter, so we can verify that the codecs
+// do the right thing when that happens.
+class RemoveResponseTrailersFilter : public Http::PassThroughFilter {
+public:
+  constexpr static char name[] = "remove-response-trailers-filter";
+  Http::FilterTrailersStatus encodeTrailers(Http::ResponseTrailerMap& trailers) override {
+    std::vector<std::string> keys;
+    trailers.iterate([&keys](const Http::HeaderEntry& trailer) -> Http::HeaderMap::Iterate {
+      keys.push_back(std::string(trailer.key().getStringView()));
+      return Http::HeaderMap::Iterate::Continue;
+    });
+    for (auto& k : keys) {
+      const Http::LowerCaseString lower_key{k};
+      trailers.remove(lower_key);
+    }
+    return Http::FilterTrailersStatus::Continue;
+  }
+};
+
+static Registry::RegisterFactory<SimpleFilterConfig<RemoveResponseTrailersFilter>,
+                                 Server::Configuration::NamedHttpFilterConfigFactory>
+    register_;
+static Registry::RegisterFactory<SimpleFilterConfig<RemoveResponseTrailersFilter>,
+                                 Server::Configuration::UpstreamHttpFilterConfigFactory>
+    register_upstream_;
+
+} // namespace Envoy

test/integration/protocol_integration_test.cc

@@ -1718,6 +1718,35 @@ TEST_P(ProtocolIntegrationTest, MaxStreamDurationWithRetryPolicyWhenRetryUpstrea
   EXPECT_EQ("408", response->headers().getStatusValue());
 }
 
+// Verify that empty trailers are not sent as trailers.
+TEST_P(DownstreamProtocolIntegrationTest, EmptyTrailersAreNotEncoded) {
+  config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1());
+  config_helper_.addConfigModifier(setEnableUpstreamTrailersHttp1());
+  config_helper_.prependFilter(R"EOF(
+name: remove-response-trailers-filter
+)EOF");
+  initialize();
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  auto encoder_decoder =
+      codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "GET"},
+                                                                 {":path", "/test/long/url"},
+                                                                 {":scheme", "http"},
+                                                                 {":authority", "sni.lyft.com"}});
+  request_encoder_ = &encoder_decoder.first;
+  auto response = std::move(encoder_decoder.second);
+  codec_client_->sendData(*request_encoder_, 1, true);
+  waitForNextUpstreamRequest();
+
+  upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, false);
+  upstream_request_->encodeData("b", false);
+  Http::TestResponseTrailerMapImpl removed_trailers{{"some-trailer", "removed-by-filter"}};
+  upstream_request_->encodeTrailers(removed_trailers);
+  ASSERT_TRUE(response->waitForEndStream());
+  EXPECT_TRUE(response->complete());
+  EXPECT_EQ("200", response->headers().getStatusValue());
+  EXPECT_THAT(response->trailers(), testing::IsNull());
+}
+
 // Verify that headers with underscores in their names are dropped from client requests
 // but remain in upstream responses.
 TEST_P(ProtocolIntegrationTest, HeadersWithUnderscoresDropped) {