http: fixing connection close behavior

docs/root/version_history/current.rst

@@ -22,6 +22,7 @@ Changes
 * health checks: allow configuring health check transport sockets by specifying :ref:`transport socket match criteria <envoy_v3_api_field_config.core.v3.HealthCheck.transport_socket_match_criteria>`.
 * http: added :ref:`stripping port from host header <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.strip_matching_host_port>` support.
 * http: fixed a bug where in some cases slash was moved from path to query string when :ref:`merging of adjacent slashes<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.merge_slashes>` is enabled.
+* http: fixed several bugs with applying correct connection close behavior across the http connection manager, health checker, and connection pool. This behavior may be temporarily reverted by setting runtime feature `envoy.reloadable_features.fix_connection_close` to false.
 * http: fixed a bug where the upgrade header was not cleared on responses to non-upgrade requests.
   Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.fix_upgrade_response` to false.
 * http: remove legacy connection pool code and their runtime features: `envoy.reloadable_features.new_http1_connection_pool_behavior` and

source/common/http/conn_manager_impl.cc

@@ -766,15 +766,26 @@ uint32_t ConnectionManagerImpl::ActiveStream::localPort() {
 // can't route select properly without full headers), checking state required to
 // serve error responses (connection close, head requests, etc), and
 // modifications which may themselves affect route selection.
-//
-// TODO(alyssawilk) all the calls here should be audited for order priority,
-// e.g. many early returns do not currently handle connection: close properly.
 void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& headers,
                                                         bool end_stream) {
   ScopeTrackerScopeState scope(this,
                                connection_manager_.read_callbacks_->connection().dispatcher());
   request_headers_ = std::move(headers);
 
+  // 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_);
+  }
+  if (request_headers_->Method() && Http::Headers::get().MethodValues.Head ==
+                                        request_headers_->Method()->value().getStringView()) {
+    state_.is_head_request_ = true;
+  }
+
   if (HeaderUtility::isConnect(*request_headers_) && !request_headers_->Path() &&
       !Runtime::runtimeFeatureEnabled("envoy.reloadable_features.stop_faking_paths")) {
     request_headers_->setPath("/");
@@ -793,10 +804,6 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
     snapped_route_config_ = connection_manager_.config_.routeConfigProvider()->config();
   }
 
-  if (Http::Headers::get().MethodValues.Head ==
-      request_headers_->Method()->value().getStringView()) {
-    state_.is_head_request_ = true;
-  }
   ENVOY_STREAM_LOG(debug, "request headers complete (end_stream={}):\n{}", *this, end_stream,
                    *request_headers_);
 
@@ -834,7 +841,6 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
                                                         connection_manager_.stats_.scope_);
 
   // Make sure we are getting a codec version we support.
-  Protocol protocol = connection_manager_.codec_->protocol();
   if (protocol == Protocol::Http10) {
     // Assume this is HTTP/1.0. This is fine for HTTP/0.9 but this code will also affect any
     // requests with non-standard version numbers (0.9, 1.3), basically anything which is not
@@ -847,7 +853,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
       sendLocalReply(false, Code::UpgradeRequired, "", nullptr, state_.is_head_request_,
                      absl::nullopt, StreamInfo::ResponseCodeDetails::get().LowVersion);
       return;
-    } else {
+    } 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;
@@ -857,23 +863,22 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
         state_.saw_connection_close_ = false;
       }
     }
-  }
-
-  if (!request_headers_->Host()) {
-    if ((protocol == Protocol::Http10) &&
+    if (!request_headers_->Host() &&
         !connection_manager_.config_.http1Settings().default_host_for_http_10_.empty()) {
       // Add a default host if configured to do so.
       request_headers_->setHost(
           connection_manager_.config_.http1Settings().default_host_for_http_10_);
-    } else {
-      // Require host header. For HTTP/1.1 Host has already been translated to :authority.
-      sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "",
-                     nullptr, state_.is_head_request_, absl::nullopt,
-                     StreamInfo::ResponseCodeDetails::get().MissingHost);
-      return;
     }
   }
 
+  if (!request_headers_->Host()) {
+    // Require host header. For HTTP/1.1 Host has already been translated to :authority.
+    sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "",
+                   nullptr, state_.is_head_request_, absl::nullopt,
+                   StreamInfo::ResponseCodeDetails::get().MissingHost);
+    return;
+  }
+
   // Verify header sanity checks which should have been performed by the codec.
   ASSERT(HeaderUtility::requestHeadersValid(*request_headers_).has_value() == false);
 
@@ -909,15 +914,15 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
   ConnectionManagerUtility::maybeNormalizeHost(*request_headers_, connection_manager_.config_,
                                                localPort());
 
-  if (protocol == Protocol::Http11 && request_headers_->Connection() &&
+  if (!fixed_connection_close && protocol == Protocol::Http11 && request_headers_->Connection() &&
       absl::EqualsIgnoreCase(request_headers_->Connection()->value().getStringView(),
                              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 (protocol < Protocol::Http2 && !state_.saw_connection_close_ &&
+  if (!fixed_connection_close && protocol < Protocol::Http2 && !state_.saw_connection_close_ &&
       request_headers_->ProxyConnection() &&
       absl::EqualsIgnoreCase(request_headers_->ProxyConnection()->value().getStringView(),
                              Http::Headers::get().ConnectionValues.Close)) {

source/common/http/header_utility.cc

@@ -223,5 +223,33 @@ HeaderUtility::requestHeadersValid(const RequestHeaderMap& headers) {
   return absl::nullopt;
 }
 
+bool HeaderUtility::shouldCloseConnection(Http::Protocol protocol,
+                                          const RequestOrResponseHeaderMap& headers) {
+  // HTTP/1.0 defaults to single-use connections. Make sure the connection will be closed unless
+  // Keep-Alive is present.
+  if (protocol == Protocol::Http10 &&
+      (!headers.Connection() ||
+       !Envoy::StringUtil::caseFindToken(headers.Connection()->value().getStringView(), ",",
+                                         Http::Headers::get().ConnectionValues.KeepAlive))) {
+    return true;
+  }
+
+  if (protocol == Protocol::Http11 && headers.Connection() &&
+      Envoy::StringUtil::caseFindToken(headers.Connection()->value().getStringView(), ",",
+                                       Http::Headers::get().ConnectionValues.Close)) {
+    return 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 (protocol < Protocol::Http2 && headers.ProxyConnection() &&
+      Envoy::StringUtil::caseFindToken(headers.ProxyConnection()->value().getStringView(), ",",
+                                       Http::Headers::get().ConnectionValues.Close)) {
+    return true;
+  }
+  return false;
+}
+
 } // namespace Http
 } // namespace Envoy

source/common/http/header_utility.h

@@ -5,6 +5,7 @@
 #include "envoy/common/regex.h"
 #include "envoy/config/route/v3/route_components.pb.h"
 #include "envoy/http/header_map.h"
+#include "envoy/http/protocol.h"
 #include "envoy/json/json_object.h"
 #include "envoy/type/v3/range.pb.h"
 
@@ -136,6 +137,16 @@ class HeaderUtility {
   static absl::optional<std::reference_wrapper<const absl::string_view>>
   requestHeadersValid(const RequestHeaderMap& headers);
 
+  /**
+   * Determines if the response should be framed by Connection: Close based on protocol
+   * and headers.
+   * @param protocol the protocol of the request
+   * @param headers the request or response headers
+   * @return if the response should be framed by Connection: Close
+   */
+  static bool shouldCloseConnection(Http::Protocol protocol,
+                                    const RequestOrResponseHeaderMap& headers);
+
   /**
    * @brief Remove the port part from host/authority header if it is equal to provided port
    */

source/common/http/http1/conn_pool.cc

@@ -12,6 +12,7 @@
 
 #include "common/http/codec_client.h"
 #include "common/http/codes.h"
+#include "common/http/header_utility.h"
 #include "common/http/headers.h"
 #include "common/runtime/runtime_features.h"
 
@@ -81,23 +82,30 @@ ConnPoolImpl::StreamWrapper::~StreamWrapper() {
 void ConnPoolImpl::StreamWrapper::onEncodeComplete() { encode_complete_ = true; }
 
 void ConnPoolImpl::StreamWrapper::decodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream) {
-  // If Connection: close OR
-  //    Http/1.0 and not Connection: keep-alive OR
-  //    Proxy-Connection: close
-  if ((headers->Connection() &&
-       (absl::EqualsIgnoreCase(headers->Connection()->value().getStringView(),
-                               Headers::get().ConnectionValues.Close))) ||
-      (parent_.codec_client_->protocol() == Protocol::Http10 &&
-       (!headers->Connection() ||
-        !absl::EqualsIgnoreCase(headers->Connection()->value().getStringView(),
-                                Headers::get().ConnectionValues.KeepAlive))) ||
-      (headers->ProxyConnection() &&
-       (absl::EqualsIgnoreCase(headers->ProxyConnection()->value().getStringView(),
-                               Headers::get().ConnectionValues.Close)))) {
-    parent_.parent_.host_->cluster().stats().upstream_cx_close_notify_.inc();
-    close_connection_ = true;
+  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 ((headers->Connection() &&
+         (absl::EqualsIgnoreCase(headers->Connection()->value().getStringView(),
+                                 Headers::get().ConnectionValues.Close))) ||
+        (parent_.codec_client_->protocol() == Protocol::Http10 &&
+         (!headers->Connection() ||
+          !absl::EqualsIgnoreCase(headers->Connection()->value().getStringView(),
+                                  Headers::get().ConnectionValues.KeepAlive))) ||
+        (headers->ProxyConnection() &&
+         (absl::EqualsIgnoreCase(headers->ProxyConnection()->value().getStringView(),
+                                 Headers::get().ConnectionValues.Close)))) {
+      parent_.parent_.host_->cluster().stats().upstream_cx_close_notify_.inc();
+      close_connection_ = true;
+    }
   }
-
   ResponseDecoderWrapper::decodeHeaders(std::move(headers), end_stream);
 }
 

source/common/runtime/runtime_features.cc

@@ -62,6 +62,7 @@ constexpr const char* runtime_features[] = {
     "envoy.deprecated_features.allow_deprecated_extension_names",
     "envoy.reloadable_features.ext_authz_http_service_enable_case_sensitive_string_matcher",
     "envoy.reloadable_features.fix_upgrade_response",
+    "envoy.reloadable_features.fixed_connection_close",
     "envoy.reloadable_features.listener_in_place_filterchain_update",
     "envoy.reloadable_features.stop_faking_paths",
 };

source/common/upstream/health_checker_impl.cc

@@ -16,8 +16,10 @@
 #include "common/config/well_known_names.h"
 #include "common/grpc/common.h"
 #include "common/http/header_map_impl.h"
+#include "common/http/header_utility.h"
 #include "common/network/address_impl.h"
 #include "common/router/router.h"
+#include "common/runtime/runtime_features.h"
 #include "common/runtime/runtime_impl.h"
 #include "common/upstream/host_utility.h"
 
@@ -344,6 +346,14 @@ bool HttpHealthCheckerImpl::HttpActiveHealthCheckSession::shouldClose() const {
     return false;
   }
 
+  if (!parent_.reuse_connection_) {
+    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(),
@@ -362,10 +372,6 @@ bool HttpHealthCheckerImpl::HttpActiveHealthCheckSession::shouldClose() const {
     }
   }
 
-  if (!parent_.reuse_connection_) {
-    return true;
-  }
-
   return false;
 }