http: deprecate `envoy.reloadable_features.http1_connection_close_hea…

…der_in_redirect` (envoyproxy#35603)

Signed-off-by: Dan Zhang <[email protected]>
Co-authored-by: Dan Zhang <[email protected]>

changelogs/current.yaml

@@ -85,6 +85,10 @@ removed_config_or_runtime:
   change: |
     Removed runtime flag ``envoy.reloadable_features.abort_filter_chain_on_stream_reset`` and legacy
     code path.
+- area: http
+  change: |
+    Removed runtime flag ``envoy.reloadable_features.http1_connection_close_header_in_redirect`` and
+    legacy code paths.
 - area: grpc reverse bridge
   change: |
     Removed ``envoy.reloadable_features.grpc_http1_reverse_bridge_handle_empty_response`` runtime

source/common/http/conn_manager_impl.cc

@@ -1152,17 +1152,11 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapSharedPt
   // Both shouldDrainConnectionUponCompletion() and is_head_request_ affect local replies: set them
   // as early as possible.
   const Protocol protocol = connection_manager_.codec_->protocol();
-  if (Runtime::runtimeFeatureEnabled(
-          "envoy.reloadable_features.http1_connection_close_header_in_redirect")) {
-    if (HeaderUtility::shouldCloseConnection(protocol, *request_headers_)) {
-      // Only mark the connection to be closed if the request indicates so. The connection might
-      // already be marked so before this step, in which case if shouldCloseConnection() returns
-      // false, the stream info value shouldn't be overridden.
-      filter_manager_.streamInfo().setShouldDrainConnectionUponCompletion(true);
-    }
-  } else {
-    filter_manager_.streamInfo().setShouldDrainConnectionUponCompletion(
-        HeaderUtility::shouldCloseConnection(protocol, *request_headers_));
+  if (HeaderUtility::shouldCloseConnection(protocol, *request_headers_)) {
+    // Only mark the connection to be closed if the request indicates so. The connection might
+    // already be marked so before this step, in which case if shouldCloseConnection() returns
+    // false, the stream info value shouldn't be overridden.
+    filter_manager_.streamInfo().setShouldDrainConnectionUponCompletion(true);
   }
 
   filter_manager_.streamInfo().protocol(protocol);

source/common/runtime/runtime_features.cc

@@ -50,7 +50,6 @@ RUNTIME_GUARD(envoy_reloadable_features_gcp_authn_use_fixed_url);
 RUNTIME_GUARD(envoy_reloadable_features_grpc_side_stream_flow_control);
 RUNTIME_GUARD(envoy_reloadable_features_http1_balsa_delay_reset);
 RUNTIME_GUARD(envoy_reloadable_features_http1_balsa_disallow_lone_cr_in_chunk_extension);
-RUNTIME_GUARD(envoy_reloadable_features_http1_connection_close_header_in_redirect);
 // Ignore the automated "remove this flag" issue: we should keep this for 1 year.
 RUNTIME_GUARD(envoy_reloadable_features_http1_use_balsa_parser);
 RUNTIME_GUARD(envoy_reloadable_features_http2_discard_host_header);

source/common/stream_info/stream_info_impl.h

@@ -377,10 +377,7 @@ struct StreamInfoImpl : public StreamInfo {
     downstream_transport_failure_reason_ = std::string(info.downstreamTransportFailureReason());
     bytes_retransmitted_ = info.bytesRetransmitted();
     packets_retransmitted_ = info.packetsRetransmitted();
-    if (Runtime::runtimeFeatureEnabled(
-            "envoy.reloadable_features.http1_connection_close_header_in_redirect")) {
-      should_drain_connection_ = info.shouldDrainConnectionUponCompletion();
-    }
+    should_drain_connection_ = info.shouldDrainConnectionUponCompletion();
   }
 
   // This function is used to copy over every field exposed in the StreamInfo interface, with a

test/integration/redirect_integration_test.cc

@@ -208,68 +208,9 @@ TEST_P(RedirectIntegrationTest, BasicInternalRedirect) {
   EXPECT_THAT(waitForAccessLog(access_log_name_, 1), HasSubstr("200 via_upstream -"));
 }
 
-// Test the buggy behavior where Envoy doesn't respond to "Connection: close" header in requests
-// which get redirected.
-// TODO(danzh) remove the test once the runtime guard is deprecated.
-TEST_P(RedirectIntegrationTest, ConnectionCloseHeaderDroppedInInternalRedirect) {
-  config_helper_.addRuntimeOverride(
-      "envoy.reloadable_features.http1_connection_close_header_in_redirect", "false");
-
-  if (downstreamProtocol() != Envoy::Http::CodecClient::Type::HTTP1) {
-    return;
-  }
-  useAccessLog("%RESPONSE_FLAGS% %RESPONSE_CODE% %RESPONSE_CODE_DETAILS% %RESP(test-header)%");
-  // Validate that header sanitization is only called once.
-  config_helper_.addConfigModifier(
-      [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
-             hcm) {
-        // The delay should be ignored in light of "Connection: close".
-        hcm.mutable_delayed_close_timeout()->set_seconds(1);
-        hcm.set_via("via_value");
-        hcm.mutable_common_http_protocol_options()
-            ->mutable_max_requests_per_connection()
-            ->set_value(10);
-      });
-  initialize();
-
-  codec_client_ = makeHttpConnection(lookupPort("http"));
-
-  default_request_headers_.setHost("handle.internal.redirect");
-  default_request_headers_.setConnection("close");
-  IntegrationStreamDecoderPtr response =
-      codec_client_->makeHeaderOnlyRequest(default_request_headers_);
-
-  waitForNextUpstreamRequest();
-
-  upstream_request_->encodeHeaders(redirect_response_, true);
-  EXPECT_THAT(waitForAccessLog(access_log_name_, 0),
-              HasSubstr("302 internal_redirect test-header-value"));
-
-  waitForNextUpstreamRequest();
-  ASSERT(upstream_request_->headers().EnvoyOriginalUrl() != nullptr);
-  EXPECT_EQ("http://handle.internal.redirect/test/long/url",
-            upstream_request_->headers().getEnvoyOriginalUrlValue());
-  EXPECT_EQ("/new/url", upstream_request_->headers().getPathValue());
-  EXPECT_EQ("authority2", upstream_request_->headers().getHostValue());
-  EXPECT_EQ("via_value", upstream_request_->headers().getViaValue());
-
-  upstream_request_->encodeHeaders(default_response_headers_, true);
-
-  ASSERT_TRUE(response->waitForEndStream());
-  ASSERT_TRUE(response->complete());
-  EXPECT_EQ("200", response->headers().getStatusValue());
-  EXPECT_TRUE(response->headers().get(Envoy::Http::Headers::get().Connection).empty());
-
-  // Envoy won't close the connection with out the fix.
-  ASSERT_FALSE(codec_client_->waitForDisconnect(std::chrono::milliseconds(2000)));
-}
-
 // Test that Envoy should correctly respond to "Connection: close" header in requests which get
 // redirected by echoing "Connection: close" in response and closing the connection immediately.
 TEST_P(RedirectIntegrationTest, ConnectionCloseHeaderHonoredInInternalRedirect) {
-  config_helper_.addRuntimeOverride(
-      "envoy.reloadable_features.http1_connection_close_header_in_redirect", "true");
-
   if (downstreamProtocol() != Envoy::Http::CodecClient::Type::HTTP1) {
     return;
   }