diff --git a/changelogs/current.yaml b/changelogs/current.yaml index f7fa938c4040b..5019a19f60aba 100644 --- a/changelogs/current.yaml +++ b/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 diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 6a9bd3c0d446f..a9e4a70cc25dc 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/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); diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 2ef124d5c9bf4..60669e8d41d4b 100644 --- a/source/common/runtime/runtime_features.cc +++ b/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); diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index 15a6e46eccc49..bcd5d6cc17ebf 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/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 diff --git a/test/integration/redirect_integration_test.cc b/test/integration/redirect_integration_test.cc index db289c321775f..51537b20bbcf0 100644 --- a/test/integration/redirect_integration_test.cc +++ b/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; }