diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 07c2978141fba..56c03dc2d02a9 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -22,6 +22,7 @@ Removed Config or Runtime * compression: removed ``envoy.reloadable_features.enable_compression_without_content_length_header`` runtime guard and legacy code paths. * http: removed ``envoy.reloadable_features.dont_add_content_length_for_bodiless_requests deprecation`` and legacy code paths. +* http: removed ``envoy.reloadable_features.http2_skip_encoding_empty_trailers`` and legacy code paths. Envoy will always encode empty trailers by sending empty data with ``end_stream`` true (instead of sending empty trailers) for HTTP/2. * http: removed ``envoy.reloadable_features.improved_stream_limit_handling`` and legacy code paths. * http: removed ``envoy.reloadable_features.return_502_for_upstream_protocol_errors``. Envoy will always return 502 code upon encountering upstream protocol error. * http: removed ``envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure`` and legacy code paths. diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index da7092dc7bf6a..239198a6bb20b 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -276,8 +276,7 @@ void ConnectionImpl::StreamImpl::encodeTrailersBase(const HeaderMap& trailers) { // waiting on window updates. We need to save the trailers so that we can emit them later. // However, for empty trailers, we don't need to to save the trailers. ASSERT(!pending_trailers_to_encode_); - const bool skip_encoding_empty_trailers = - trailers.empty() && parent_.skip_encoding_empty_trailers_; + const bool skip_encoding_empty_trailers = trailers.empty(); if (!skip_encoding_empty_trailers) { pending_trailers_to_encode_ = cloneTrailers(trailers); onLocalEndStream(); @@ -401,8 +400,7 @@ void ConnectionImpl::StreamImpl::saveHeader(HeaderString&& name, HeaderString&& void ConnectionImpl::StreamImpl::submitTrailers(const HeaderMap& trailers) { ASSERT(local_end_stream_); - const bool skip_encoding_empty_trailers = - trailers.empty() && parent_.skip_encoding_empty_trailers_; + const bool skip_encoding_empty_trailers = trailers.empty(); if (skip_encoding_empty_trailers) { ENVOY_CONN_LOG(debug, "skipping submitting trailers", parent_.connection_); @@ -617,8 +615,6 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat stream_error_on_invalid_http_messaging_( http2_options.override_stream_error_on_invalid_http_message().value()), protocol_constraints_(stats, http2_options), - skip_encoding_empty_trailers_(Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.http2_skip_encoding_empty_trailers")), skip_dispatching_frames_for_closed_connection_(Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.skip_dispatching_frames_for_closed_connection")), dispatching_(false), raised_goaway_(false), random_(random_generator), @@ -1516,8 +1512,7 @@ void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const { os << spaces << "Http2::ConnectionImpl " << this << DUMP_MEMBER(max_headers_kb_) << DUMP_MEMBER(max_headers_count_) << DUMP_MEMBER(per_stream_buffer_limit_) << DUMP_MEMBER(allow_metadata_) << DUMP_MEMBER(stream_error_on_invalid_http_messaging_) - << DUMP_MEMBER(is_outbound_flood_monitored_control_frame_) - << DUMP_MEMBER(skip_encoding_empty_trailers_) << DUMP_MEMBER(dispatching_) + << DUMP_MEMBER(is_outbound_flood_monitored_control_frame_) << DUMP_MEMBER(dispatching_) << DUMP_MEMBER(raised_goaway_) << DUMP_MEMBER(pending_deferred_reset_streams_.size()) << '\n'; // Dump the protocol constraints diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 9b3363e740b9e..91759dcb06c2a 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -522,12 +522,6 @@ class ConnectionImpl : public virtual Connection, // nghttp2 library will keep calling this callback to write the rest of the frame. ssize_t onSend(const uint8_t* data, size_t length); - // Some browsers (e.g. WebKit-based browsers: https://bugs.webkit.org/show_bug.cgi?id=210108) have - // a problem with processing empty trailers (END_STREAM | END_HEADERS with zero length HEADERS) of - // an HTTP/2 response as reported here: https://github.com/envoyproxy/envoy/issues/10514. This is - // controlled by "envoy.reloadable_features.http2_skip_encoding_empty_trailers" runtime feature - // flag. - const bool skip_encoding_empty_trailers_; const bool skip_dispatching_frames_for_closed_connection_; // dumpState helper method. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 2ad1a5373aa4d..330a587e2a6dd 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -69,7 +69,6 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.health_check.graceful_goaway_handling", "envoy.reloadable_features.health_check.immediate_failure_exclude_from_cluster", "envoy.reloadable_features.http2_consume_stream_refused_errors", - "envoy.reloadable_features.http2_skip_encoding_empty_trailers", "envoy.reloadable_features.http_ext_authz_do_not_skip_direct_response_and_redirect", "envoy.reloadable_features.http_reject_path_with_fragment", "envoy.reloadable_features.http_strip_fragment_from_path_unsafe_if_disabled", diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 51c460a4d2f36..50d9b50802b3b 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -705,8 +705,6 @@ TEST_P(Http2CodecImplTest, TrailingHeaders) { // When having empty trailers, codec submits empty buffer and end_stream instead. TEST_P(Http2CodecImplTest, IgnoreTrailingEmptyHeaders) { TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.http2_skip_encoding_empty_trailers", "true"}}); initialize(); @@ -732,35 +730,6 @@ TEST_P(Http2CodecImplTest, IgnoreTrailingEmptyHeaders) { response_encoder_->encodeTrailers(TestResponseTrailerMapImpl{}); } -// When having empty trailers and "envoy.reloadable_features.http2_skip_encoding_empty_trailers" is -// turned off, codec submits empty trailers. -TEST_P(Http2CodecImplTest, SubmitTrailingEmptyHeaders) { - TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.http2_skip_encoding_empty_trailers", "false"}}); - - initialize(); - - TestRequestHeaderMapImpl request_headers; - HttpTestUtility::addDefaultHeaders(request_headers); - EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok()); - EXPECT_CALL(request_decoder_, decodeData(_, false)); - Buffer::OwnedImpl hello("hello"); - request_encoder_->encodeData(hello, false); - EXPECT_CALL(request_decoder_, decodeTrailers_(_)); - request_encoder_->encodeTrailers(TestRequestTrailerMapImpl{}); - - TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - EXPECT_CALL(response_decoder_, decodeHeaders_(_, false)); - response_encoder_->encodeHeaders(response_headers, false); - EXPECT_CALL(response_decoder_, decodeData(_, false)); - Buffer::OwnedImpl world("world"); - response_encoder_->encodeData(world, false); - EXPECT_CALL(response_decoder_, decodeTrailers_(_)); - response_encoder_->encodeTrailers(TestResponseTrailerMapImpl{}); -} - TEST_P(Http2CodecImplTest, TrailingHeadersLargeClientBody) { initialize(); @@ -1040,7 +1009,7 @@ TEST_P(Http2CodecImplTest, DumpsStreamlessConnectionWithoutAllocatingMemory) { "max_headers_kb_: 60, max_headers_count_: 100, " "per_stream_buffer_limit_: 268435456, allow_metadata_: 0, " "stream_error_on_invalid_http_messaging_: 0, is_outbound_flood_monitored_control_frame_: " - "0, skip_encoding_empty_trailers_: 1, dispatching_: 0, raised_goaway_: 0, " + "0, dispatching_: 0, raised_goaway_: 0, " "pending_deferred_reset_streams_.size(): 0\n" " &protocol_constraints_: \n" " ProtocolConstraints")); diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc index 235168207001c..2d8bdf6214c87 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc @@ -13,11 +13,9 @@ constexpr absl::string_view text{"application/grpc-web-text"}; constexpr absl::string_view binary{"application/grpc-web"}; constexpr uint64_t MAX_BUFFERED_PLAINTEXT_LENGTH = 16384; -using SkipEncodingEmptyTrailers = bool; using ContentType = std::string; using Accept = std::string; -using TestParams = std::tuple; +using TestParams = std::tuple; class GrpcWebFilterIntegrationTest : public testing::TestWithParam, public HttpIntegrationTest { @@ -33,21 +31,12 @@ class GrpcWebFilterIntegrationTest : public testing::TestWithParam, void initialize() override { if (downstream_protocol_ == Http::CodecType::HTTP1) { config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); - } else { - skipEncodingEmptyTrailers(http2_skip_encoding_empty_trailers_); } - setUpstreamProtocol(Http::CodecType::HTTP2); HttpIntegrationTest::initialize(); } - void skipEncodingEmptyTrailers(SkipEncodingEmptyTrailers http2_skip_encoding_empty_trailers) { - config_helper_.addRuntimeOverride( - "envoy.reloadable_features.http2_skip_encoding_empty_trailers", - http2_skip_encoding_empty_trailers ? "true" : "false"); - } - void setLocalReplyConfig(const std::string& yaml) { envoy::extensions::filters::network::http_connection_manager::v3::LocalReplyConfig local_reply_config; @@ -134,27 +123,23 @@ class GrpcWebFilterIntegrationTest : public testing::TestWithParam, static std::string testParamsToString(const testing::TestParamInfo params) { return fmt::format( - "{}_{}_{}_{}_{}", + "{}_{}_{}_{}", TestUtility::ipTestParamsToString(testing::TestParamInfo( std::get<0>(params.param), params.index)), std::get<1>(params.param) == Http::CodecType::HTTP2 ? "Http2" : "Http", - std::get<2>(params.param) ? "SkipEncodingEmptyTrailers" : "SubmitEncodingEmptyTrailers", - std::get<3>(params.param) == text ? "SendText" : "SendBinary", - std::get<4>(params.param) == text ? "AcceptText" : "AcceptBinary"); + std::get<2>(params.param) == text ? "SendText" : "SendBinary", + std::get<3>(params.param) == text ? "AcceptText" : "AcceptBinary"); } const Envoy::Http::CodecType downstream_protocol_{std::get<1>(GetParam())}; - const bool http2_skip_encoding_empty_trailers_{std::get<2>(GetParam())}; - const ContentType content_type_{std::get<3>(GetParam())}; - const Accept accept_{std::get<4>(GetParam())}; + const ContentType content_type_{std::get<2>(GetParam())}; + const Accept accept_{std::get<3>(GetParam())}; }; INSTANTIATE_TEST_SUITE_P( Params, GrpcWebFilterIntegrationTest, testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), testing::Values(Http::CodecType::HTTP1, Http::CodecType::HTTP2), - testing::Values(SkipEncodingEmptyTrailers{true}, - SkipEncodingEmptyTrailers{false}), testing::Values(ContentType{text}, ContentType{binary}), testing::Values(Accept{text}, Accept{binary})), GrpcWebFilterIntegrationTest::testParamsToString); @@ -206,15 +191,9 @@ TEST_P(GrpcWebFilterIntegrationTest, GrpcWebTrailersNotDuplicated) { } if (downstream_protocol_ == Http::CodecType::HTTP2) { - if (http2_skip_encoding_empty_trailers_) { - // When the downstream protocol is HTTP/2 and the feature-flag to skip encoding empty trailers - // is turned on, expect that the trailers are included in the response-body. - EXPECT_EQ(nullptr, response->trailers()); - } else { - // Otherwise, we send empty trailers. - ASSERT_NE(nullptr, response->trailers()); - EXPECT_TRUE(response->trailers()->empty()); - } + // When the downstream protocol is HTTP/2 expect that the trailers are included in the + // response-body. + EXPECT_EQ(nullptr, response->trailers()); } }