http: fixing connection close behavior

docs/root/version_history/current.rst

@@ -20,6 +20,7 @@ Changes
 * gzip filter: added option to set zlib's next output buffer size.
 * 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: 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 a bug where Connection: Close was not always sent correctly. 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

@@ -37,6 +37,7 @@
 #include "common/http/utility.h"
 #include "common/network/utility.h"
 #include "common/router/config_impl.h"
+#include "common/runtime/runtime_features.h"
 #include "common/runtime/runtime_impl.h"
 #include "common/stats/timespan_impl.h"
 
@@ -752,15 +753,26 @@ const Network::Connection* ConnectionManagerImpl::ActiveStream::connection() {
 // 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.
+  Protocol protocol = connection_manager_.codec_->protocol();
+  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 (Http::Headers::get().MethodValues.Head ==
+      request_headers_->Method()->value().getStringView()) {
+    state_.is_head_request_ = true;
+  }
+
   // TODO(alyssawilk) remove this synthetic path in a follow-up PR, including
   // auditing of empty path headers. We check for path because HTTP/2 connect requests may have a
   // path.
@@ -781,10 +793,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_);
 
@@ -822,7 +830,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
@@ -835,7 +842,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;
@@ -845,23 +852,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);
 
@@ -891,15 +897,15 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
     return;
   }
 
-  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

@@ -190,5 +190,33 @@ HeaderUtility::requestHeadersValid(const RequestHeaderMap& headers) {
   return absl::nullopt;
 }
 
+bool HeaderUtility::shouldCloseConnection(Http::Protocol protocol,
+                                          const RequestHeaderMap& request_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 &&
+      (!request_headers.Connection() ||
+       !Envoy::StringUtil::caseFindToken(request_headers.Connection()->value().getStringView(), ",",
+                                         Http::Headers::get().ConnectionValues.KeepAlive))) {
+    return true;
+  }
+
+  if (protocol == Protocol::Http11 && request_headers.Connection() &&
+      Envoy::StringUtil::caseFindToken(request_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 && request_headers.ProxyConnection() &&
+      Envoy::StringUtil::caseFindToken(request_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"
 
@@ -135,6 +136,15 @@ 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 request headers.
+   * @param protocol the protocol of the request
+   * @param headers the request headers
+   * @return if the response should be framed by Connection: Close
+   */
+  static bool shouldCloseConnection(Http::Protocol protocol, const RequestHeaderMap& headers);
 };
 } // namespace Http
 } // namespace Envoy

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",
 };
 

test/common/http/BUILD

@@ -230,6 +230,7 @@ envoy_cc_test(
         "//test/mocks/tracing:tracing_mocks",
         "//test/mocks/upstream:upstream_mocks",
         "//test/test_common:logging_lib",
+        "//test/test_common:test_runtime_lib",
         "//test/test_common:test_time_lib",
         "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto",
         "@envoy_api//envoy/type/tracing/v3:pkg_cc_proto",