Make http3 codec not encode empty trailers block

changelogs/current.yaml

@@ -104,6 +104,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

@@ -93,6 +93,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) {