From b5bc27399d723806bab993895e64f9295c863016 Mon Sep 17 00:00:00 2001 From: Raven Black Date: Wed, 4 Sep 2024 16:26:00 -0400 Subject: [PATCH] Make http3 codec not encode empty trailers block (#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 --- changelogs/current.yaml | 6 +++ .../common/quic/envoy_quic_server_stream.cc | 9 +++++ source/common/runtime/runtime_features.cc | 1 + test/integration/BUILD | 1 + test/integration/filters/BUILD | 15 ++++++++ .../filters/remove_response_trailers.cc | 38 +++++++++++++++++++ test/integration/protocol_integration_test.cc | 29 ++++++++++++++ 7 files changed, 99 insertions(+) create mode 100644 test/integration/filters/remove_response_trailers.cc 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) {