diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 102c5f4a908b..15d9555cb1d8 100644 --- a/changelogs/current.yaml +++ b/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 diff --git a/source/common/quic/envoy_quic_server_stream.cc b/source/common/quic/envoy_quic_server_stream.cc index 522c7209d4c8..331495df292b 100644 --- a/source/common/quic/envoy_quic_server_stream.cc +++ b/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)); } diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index e1cc4cec1bf7..7f0378342a27 100644 --- a/source/common/runtime/runtime_features.cc +++ b/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); diff --git a/test/integration/BUILD b/test/integration/BUILD index fd0687cab3a2..fdfa59a6cf62 100644 --- a/test/integration/BUILD +++ b/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", diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index 83459f3c5970..dfebb8f118cf 100644 --- a/test/integration/filters/BUILD +++ b/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 = [ diff --git a/test/integration/filters/remove_response_trailers.cc b/test/integration/filters/remove_response_trailers.cc new file mode 100644 index 000000000000..780d016b4fb4 --- /dev/null +++ b/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 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, + Server::Configuration::NamedHttpFilterConfigFactory> + register_; +static Registry::RegisterFactory, + Server::Configuration::UpstreamHttpFilterConfigFactory> + register_upstream_; + +} // namespace Envoy diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 1c063b008c70..dcc490cd6139 100644 --- a/test/integration/protocol_integration_test.cc +++ b/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) {