diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index e0ac0e7de4a40..db8223715a43d 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -32,6 +32,7 @@ Removed Config or Runtime * access_logs: removed legacy unbounded access logs and runtime guard `envoy.reloadable_features.disallow_unbounded_access_logs`. * dynamic_forward_proxy: removed `envoy.reloadable_features.enable_dns_cache_circuit_breakers` and legacy code path. +* http: removed legacy connection close behavior and runtime guard `envoy.reloadable_features.fixed_connection_close`. * http: removed legacy HTTP/1.1 error reporting path and runtime guard `envoy.reloadable_features.early_errors_via_hcm`. New Features diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index d6f501985486e..730bf31c2e9c8 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -858,12 +858,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he // Both saw_connection_close_ and is_head_request_ affect local replies: set // them as early as possible. const Protocol protocol = connection_manager_.codec_->protocol(); - const bool fixed_connection_close = - Runtime::runtimeFeatureEnabled("envoy.reloadable_features.fixed_connection_close"); - if (fixed_connection_close) { - state_.saw_connection_close_ = - HeaderUtility::shouldCloseConnection(protocol, *request_headers_); - } + state_.saw_connection_close_ = HeaderUtility::shouldCloseConnection(protocol, *request_headers_); if (HeaderUtility::isConnect(*request_headers_) && !request_headers_->Path() && !Runtime::runtimeFeatureEnabled("envoy.reloadable_features.stop_faking_paths")) { request_headers_->setPath("/"); @@ -930,14 +925,6 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he sendLocalReply(false, Code::UpgradeRequired, "", nullptr, absl::nullopt, StreamInfo::ResponseCodeDetails::get().LowVersion); return; - } else if (!fixed_connection_close) { - // HTTP/1.0 defaults to single-use connections. Make sure the connection - // will be closed unless Keep-Alive is present. - state_.saw_connection_close_ = true; - if (absl::EqualsIgnoreCase(request_headers_->getConnectionValue(), - Http::Headers::get().ConnectionValues.KeepAlive)) { - state_.saw_connection_close_ = false; - } } if (!request_headers_->Host() && !connection_manager_.config_.http1Settings().default_host_for_http_10_.empty()) { @@ -988,20 +975,6 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he ConnectionManagerUtility::maybeNormalizeHost(*request_headers_, connection_manager_.config_, localPort()); - if (!fixed_connection_close && protocol == Protocol::Http11 && - absl::EqualsIgnoreCase(request_headers_->getConnectionValue(), - Http::Headers::get().ConnectionValues.Close)) { - state_.saw_connection_close_ = true; - } - // Note: Proxy-Connection is not a standard header, but is supported here - // since it is supported by http-parser the underlying parser for http - // requests. - if (!fixed_connection_close && protocol < Protocol::Http2 && !state_.saw_connection_close_ && - absl::EqualsIgnoreCase(request_headers_->getProxyConnectionValue(), - Http::Headers::get().ConnectionValues.Close)) { - state_.saw_connection_close_ = true; - } - if (!state_.is_internally_created_) { // Only sanitize headers on first pass. // Modify the downstream remote address depending on configuration and headers. filter_manager_.setDownstreamRemoteAddress(ConnectionManagerUtility::mutateRequestHeaders( diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index 0a070c28395d3..8a59c01005572 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -39,26 +39,10 @@ ActiveClient::StreamWrapper::~StreamWrapper() { void ActiveClient::StreamWrapper::onEncodeComplete() { encode_complete_ = true; } void ActiveClient::StreamWrapper::decodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream) { - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.fixed_connection_close")) { - close_connection_ = - HeaderUtility::shouldCloseConnection(parent_.codec_client_->protocol(), *headers); - if (close_connection_) { - parent_.parent().host()->cluster().stats().upstream_cx_close_notify_.inc(); - } - } else { - // If Connection: close OR - // Http/1.0 and not Connection: keep-alive OR - // Proxy-Connection: close - if ((absl::EqualsIgnoreCase(headers->getConnectionValue(), - Headers::get().ConnectionValues.Close)) || - (parent_.codec_client_->protocol() == Protocol::Http10 && - !absl::EqualsIgnoreCase(headers->getConnectionValue(), - Headers::get().ConnectionValues.KeepAlive)) || - (absl::EqualsIgnoreCase(headers->getProxyConnectionValue(), - Headers::get().ConnectionValues.Close))) { - parent_.parent().host()->cluster().stats().upstream_cx_close_notify_.inc(); - close_connection_ = true; - } + close_connection_ = + HeaderUtility::shouldCloseConnection(parent_.codec_client_->protocol(), *headers); + if (close_connection_) { + parent_.parent().host()->cluster().stats().upstream_cx_close_notify_.inc(); } ResponseDecoderWrapper::decodeHeaders(std::move(headers), end_stream); } diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index e39a80ad6df0f..77afcf99702f8 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -67,7 +67,6 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.disable_tls_inspector_injection", "envoy.reloadable_features.fix_upgrade_response", "envoy.reloadable_features.fix_wildcard_matching", - "envoy.reloadable_features.fixed_connection_close", "envoy.reloadable_features.hcm_stream_error_on_invalid_message", "envoy.reloadable_features.health_check.graceful_goaway_handling", "envoy.reloadable_features.http_default_alpn", diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index 71228947b8704..be41d9064d160 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -395,29 +395,7 @@ bool HttpHealthCheckerImpl::HttpActiveHealthCheckSession::shouldClose() const { return true; } - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.fixed_connection_close")) { - return Http::HeaderUtility::shouldCloseConnection(client_->protocol(), *response_headers_); - } - - if (response_headers_->Connection()) { - const bool close = - absl::EqualsIgnoreCase(response_headers_->Connection()->value().getStringView(), - Http::Headers::get().ConnectionValues.Close); - if (close) { - return true; - } - } - - if (response_headers_->ProxyConnection() && protocol_ < Http::Protocol::Http2) { - const bool close = - absl::EqualsIgnoreCase(response_headers_->ProxyConnection()->value().getStringView(), - Http::Headers::get().ConnectionValues.Close); - if (close) { - return true; - } - } - - return false; + return Http::HeaderUtility::shouldCloseConnection(client_->protocol(), *response_headers_); } void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onTimeout() { diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index cd9944e0d7ccb..e137af802ec2e 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -2978,77 +2978,6 @@ TEST_F(HttpConnectionManagerImplTest, Http10Rejected) { conn_manager_->onData(fake_input, false); } -TEST_F(HttpConnectionManagerImplTest, Http10ConnCloseLegacy) { - http1_settings_.accept_http_10_ = true; - TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.fixed_connection_close", "false"}}); - setup(false, ""); - EXPECT_CALL(*codec_, protocol()).Times(AnyNumber()).WillRepeatedly(Return(Protocol::Http10)); - EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status { - decoder_ = &conn_manager_->newStream(response_encoder_); - RequestHeaderMapPtr headers{ - new TestRequestHeaderMapImpl{{":authority", "host:80"}, {":method", "CONNECT"}}}; - decoder_->decodeHeaders(std::move(headers), true); - data.drain(4); - return Http::okStatus(); - })); - - EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { - EXPECT_EQ("close", headers.getConnectionValue()); - })); - - Buffer::OwnedImpl fake_input("1234"); - conn_manager_->onData(fake_input, false); -} - -TEST_F(HttpConnectionManagerImplTest, ProxyConnectLegacyClose) { - TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.fixed_connection_close", "false"}}); - setup(false, ""); - EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status { - decoder_ = &conn_manager_->newStream(response_encoder_); - RequestHeaderMapPtr headers{new TestRequestHeaderMapImpl{ - {":authority", "host:80"}, {":method", "CONNECT"}, {"proxy-connection", "close"}}}; - decoder_->decodeHeaders(std::move(headers), true); - data.drain(4); - return Http::okStatus(); - })); - - EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { - EXPECT_EQ("close", headers.getConnectionValue()); - })); - - Buffer::OwnedImpl fake_input("1234"); - conn_manager_->onData(fake_input, false); -} - -TEST_F(HttpConnectionManagerImplTest, ConnectLegacyClose) { - TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.fixed_connection_close", "false"}}); - setup(false, ""); - EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status { - decoder_ = &conn_manager_->newStream(response_encoder_); - RequestHeaderMapPtr headers{new TestRequestHeaderMapImpl{ - {":authority", "host"}, {":method", "CONNECT"}, {"connection", "close"}}}; - decoder_->decodeHeaders(std::move(headers), true); - data.drain(4); - return Http::okStatus(); - })); - - EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) - .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { - EXPECT_EQ("close", headers.getConnectionValue()); - })); - - Buffer::OwnedImpl fake_input("1234"); - conn_manager_->onData(fake_input, false); -} - TEST_F(HttpConnectionManagerImplTest, MaxStreamDurationCallbackNotCalledIfResetStreamValidly) { max_stream_duration_ = std::chrono::milliseconds(5000); setup(false, ""); diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index cd31d3ca441c3..2f9814b1b5e01 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -822,46 +822,6 @@ TEST_F(Http1ConnPoolImplTest, ProxyConnectionCloseHeader) { EXPECT_EQ(0U, cluster_->stats_.upstream_cx_destroy_with_active_rq_.value()); } -/** - * Test legacy behavior when upstream sends us 'proxy-connection: close' - */ -TEST_F(Http1ConnPoolImplTest, ProxyConnectionCloseHeaderLegacy) { - TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.fixed_connection_close", "false"}}); - InSequence s; - - // Request 1 should kick off a new connection. - NiceMock outer_decoder; - ConnPoolCallbacks callbacks; - conn_pool_->expectClientCreate(); - Http::ConnectionPool::Cancellable* handle = conn_pool_->newStream(outer_decoder, callbacks); - - EXPECT_NE(nullptr, handle); - - NiceMock request_encoder; - ResponseDecoder* inner_decoder; - EXPECT_CALL(*conn_pool_->test_clients_[0].codec_, newStream(_)) - .WillOnce(DoAll(SaveArgAddress(&inner_decoder), ReturnRef(request_encoder))); - EXPECT_CALL(callbacks.pool_ready_, ready()); - - conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); - EXPECT_TRUE( - callbacks.outer_encoder_ - ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) - .ok()); - - // Response with 'proxy-connection: close' which should cause the connection to go away, even if - // there are other tokens in that header. - EXPECT_CALL(*conn_pool_, onClientDestroy()); - ResponseHeaderMapPtr response_headers( - new TestResponseHeaderMapImpl{{":status", "200"}, {"Proxy-Connection", "Close"}}); - inner_decoder->decodeHeaders(std::move(response_headers), true); - dispatcher_.clearDeferredDeleteList(); - - EXPECT_EQ(0U, cluster_->stats_.upstream_cx_destroy_with_active_rq_.value()); -} - /** * Test when upstream is HTTP/1.0 and does not send 'connection: keep-alive' */ @@ -898,45 +858,6 @@ TEST_F(Http1ConnPoolImplTest, Http10NoConnectionKeepAlive) { EXPECT_EQ(0U, cluster_->stats_.upstream_cx_destroy_with_active_rq_.value()); } -/** - * Test legacy behavior when upstream is HTTP/1.0 and does not send 'connection: keep-alive' - */ -TEST_F(Http1ConnPoolImplTest, Http10NoConnectionKeepAliveLegacy) { - TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.fixed_connection_close", "false"}}); - InSequence s; - - // Request 1 should kick off a new connection. - NiceMock outer_decoder; - ConnPoolCallbacks callbacks; - conn_pool_->expectClientCreate(Protocol::Http10); - Http::ConnectionPool::Cancellable* handle = conn_pool_->newStream(outer_decoder, callbacks); - - EXPECT_NE(nullptr, handle); - - NiceMock request_encoder; - ResponseDecoder* inner_decoder; - EXPECT_CALL(*conn_pool_->test_clients_[0].codec_, newStream(_)) - .WillOnce(DoAll(SaveArgAddress(&inner_decoder), ReturnRef(request_encoder))); - EXPECT_CALL(callbacks.pool_ready_, ready()); - - conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); - EXPECT_TRUE( - callbacks.outer_encoder_ - ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) - .ok()); - - // Response without 'connection: keep-alive' which should cause the connection to go away. - EXPECT_CALL(*conn_pool_, onClientDestroy()); - ResponseHeaderMapPtr response_headers( - new TestResponseHeaderMapImpl{{":protocol", "HTTP/1.0"}, {":status", "200"}}); - inner_decoder->decodeHeaders(std::move(response_headers), true); - dispatcher_.clearDeferredDeleteList(); - - EXPECT_EQ(0U, cluster_->stats_.upstream_cx_destroy_with_active_rq_.value()); -} - /** * Test when we reach max requests per connection. */ diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index d42cc90578d22..79673e3ae47ca 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -2103,56 +2103,6 @@ TEST_F(HttpHealthCheckerImplTest, ProxyConnectionClose) { test_sessions_[0]->interval_timer_->invokeCallback(); } -TEST_F(HttpHealthCheckerImplTest, ConnectionCloseLegacy) { - TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.fixed_connection_close", "false"}}); - setupNoServiceValidationHC(); - EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Unchanged)); - - cluster_->prioritySet().getMockHostSet(0)->hosts_ = { - makeTestHost(cluster_->info_, "tcp://127.0.0.1:80", simTime())}; - expectSessionCreate(); - expectStreamCreate(0); - EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); - health_checker_->start(); - - EXPECT_CALL(*test_sessions_[0]->interval_timer_, enableTimer(_, _)); - EXPECT_CALL(*test_sessions_[0]->timeout_timer_, disableTimer()); - respond(0, "200", true); - EXPECT_EQ(Host::Health::Healthy, cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->health()); - - expectClientCreate(0); - expectStreamCreate(0); - EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); - test_sessions_[0]->interval_timer_->invokeCallback(); -} - -TEST_F(HttpHealthCheckerImplTest, ProxyConnectionCloseLegacy) { - TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.fixed_connection_close", "false"}}); - setupNoServiceValidationHC(); - EXPECT_CALL(*this, onHostStatus(_, HealthTransition::Unchanged)); - - cluster_->prioritySet().getMockHostSet(0)->hosts_ = { - makeTestHost(cluster_->info_, "tcp://127.0.0.1:80", simTime())}; - expectSessionCreate(); - expectStreamCreate(0); - EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); - health_checker_->start(); - - EXPECT_CALL(*test_sessions_[0]->interval_timer_, enableTimer(_, _)); - EXPECT_CALL(*test_sessions_[0]->timeout_timer_, disableTimer()); - respond(0, "200", false, true); - EXPECT_EQ(Host::Health::Healthy, cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->health()); - - expectClientCreate(0); - expectStreamCreate(0); - EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); - test_sessions_[0]->interval_timer_->invokeCallback(); -} - TEST_F(HttpHealthCheckerImplTest, HealthCheckIntervals) { setupHealthCheckIntervalOverridesHC(); cluster_->prioritySet().getMockHostSet(0)->hosts_ = {