Remove envoy.reloadable_features.http2_skip_encoding_empty_trailers

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.

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

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.

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",

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"));

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<Network::Address::IpVersion, Http::CodecType,
-                              SkipEncodingEmptyTrailers, ContentType, Accept>;
+using TestParams = std::tuple<Network::Address::IpVersion, Http::CodecType, ContentType, Accept>;
 
 class GrpcWebFilterIntegrationTest : public testing::TestWithParam<TestParams>,
                                      public HttpIntegrationTest {
@@ -33,21 +31,12 @@ class GrpcWebFilterIntegrationTest : public testing::TestWithParam<TestParams>,
   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<TestParams>,
 
   static std::string testParamsToString(const testing::TestParamInfo<TestParams> params) {
     return fmt::format(
-        "{}_{}_{}_{}_{}",
+        "{}_{}_{}_{}",
         TestUtility::ipTestParamsToString(testing::TestParamInfo<Network::Address::IpVersion>(
             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());
   }
 }