http: removing envoy.reloadable_features.fixed_connection_close (#14705)

Risk Level: Low (removing deprecated guarded code) Testing: n/a Docs Changes: n/a Release Notes: inline Fixes #14645 Signed-off-by: Alyssa Wilk <[email protected]>
envoyproxy · Jan 20, 2021 · 7fca50b · 7fca50b
1 parent 1fe95aa
commit 7fca50b
Show file tree

Hide file tree

Showing 8 changed files with 7 additions and 272 deletions.
diff --git 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`.
 * http: removed legacy sanitization path for upgrade response headers and runtime guard `envoy.reloadable_features.fix_upgrade_response`.
 

diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc
@@ -860,12 +860,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("/");
@@ -932,14 +927,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()) {
@@ -990,20 +977,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
@@ -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
@@ -66,7 +66,6 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.check_ocsp_policy",
     "envoy.reloadable_features.disable_tls_inspector_injection",
     "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
@@ -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
@@ -2980,77 +2980,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
@@ -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<MockResponseDecoder> outer_decoder;
-  ConnPoolCallbacks callbacks;
-  conn_pool_->expectClientCreate();
-  Http::ConnectionPool::Cancellable* handle = conn_pool_->newStream(outer_decoder, callbacks);
-
-  EXPECT_NE(nullptr, handle);
-
-  NiceMock<MockRequestEncoder> 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<MockResponseDecoder> outer_decoder;
-  ConnPoolCallbacks callbacks;
-  conn_pool_->expectClientCreate(Protocol::Http10);
-  Http::ConnectionPool::Cancellable* handle = conn_pool_->newStream(outer_decoder, callbacks);
-
-  EXPECT_NE(nullptr, handle);
-
-  NiceMock<MockRequestEncoder> 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
@@ -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_ = {