add http1-safe max_connection_duration for http connection manager (e…

…nvoyproxy#35209)

This PR adds an option to change the http connection manager's draining
behavior for max_connection_duration for http1. The new behavior is that
envoy will wait indefinitely for one more request, add connection:close
to the response headers, then close the connection once the stream ends.

This is to avoid a networking race condition which results in the client
not receiving a response to their last request if they send it right
when envoy is closing the connection.

Fixes envoyproxy#34356 (check for context)

---------

Signed-off-by: antoniovleonti <[email protected]>
Signed-off-by: Martin Duke <[email protected]>

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

@@ -37,7 +37,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
 // HTTP connection manager :ref:`configuration overview <config_http_conn_man>`.
 // [#extension: envoy.filters.network.http_connection_manager]
 
-// [#next-free-field: 58]
+// [#next-free-field: 59]
 message HttpConnectionManager {
   option (udpa.annotations.versioning).previous_message_type =
       "envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager";
@@ -447,6 +447,21 @@ message HttpConnectionManager {
   config.core.v3.HttpProtocolOptions common_http_protocol_options = 35
       [(udpa.annotations.security).configure_for_untrusted_downstream = true];
 
+  // If set to true, Envoy will not start a drain timer for downstream HTTP1 connections after
+  // :ref:`common_http_protocol_options.max_connection_duration
+  // <envoy_v3_api_field_config.core.v3.HttpProtocolOptions.max_connection_duration>` passes.
+  // Instead, Envoy will wait for the next downstream request, add connection:close to the response
+  // headers, then close the connection after the stream ends.
+  //
+  // This behavior is compliant with `RFC 9112 section 9.6 <https://www.rfc-editor.org/rfc/rfc9112#name-tear-down>`_
+  //
+  // If set to false, ``max_connection_duration`` will cause Envoy to enter the normal drain
+  // sequence for HTTP1 with Envoy eventually closing the connection (once there are no active
+  // streams).
+  //
+  // Has no effect if ``max_connection_duration`` is unset. Defaults to false.
+  bool http1_safe_max_connection_duration = 58;
+
   // Additional HTTP/1 settings that are passed to the HTTP/1 codec.
   // [#comment:TODO: The following fields are ignored when the
   // :ref:`header validation configuration <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.typed_header_validation_config>`

changelogs/current.yaml

@@ -6,6 +6,15 @@ behavior_changes:
   change: |
     Removed support for (long deprecated) opentracing.  See `issue 27401
     <https://github.com/envoyproxy/envoy/issues/27401>`_ for details.
+- area: http
+  change: |
+    Added HTTP1-safe option for :ref:`max_connection_duration
+    <envoy_v3_api_field_config.core.v3.HttpProtocolOptions.max_connection_duration>` in
+    HttpConnectionManager. When enabled, ``max_connection_duration`` will only drain downstream
+    HTTP1 connections by adding the Connection:close response header; it will never cause the
+    HttpConnectionManager to close the connection itself.  Defaults to off ("unsafe" -- check
+    \#34356) and is configurable via :ref:`http1_safe_max_connection_duration
+    <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.http1_safe_max_connection_duration>`.
 - area: stats scoped_rds
   change: |
     Added new tag extraction so that scoped rds stats have their :ref:'scope_route_config_name

docs/root/configuration/http/http_conn_man/stats.rst

@@ -26,6 +26,7 @@ statistics:
    ``downstream_cx_ssl_active``, Gauge, Total active TLS connections
    ``downstream_cx_http1_active``, Gauge, Total active HTTP/1.1 connections
    ``downstream_cx_upgrades_active``, Gauge, Total active upgraded connections. These are also counted as active http1/http2 connections.
+   ``downstream_cx_http1_soft_drain``, Gauge, Total active HTTP/1.x connections waiting for another downstream request to safely close the connection.
    ``downstream_cx_http2_active``, Gauge, Total active HTTP/2 connections
    ``downstream_cx_http3_active``, Gauge, Total active HTTP/3 connections
    ``downstream_cx_protocol_error``, Counter, Total protocol errors

source/common/http/conn_manager_config.h

@@ -83,6 +83,7 @@ namespace Http {
   GAUGE(downstream_cx_ssl_active, Accumulate)                                                      \
   GAUGE(downstream_cx_tx_bytes_buffered, Accumulate)                                               \
   GAUGE(downstream_cx_upgrades_active, Accumulate)                                                 \
+  GAUGE(downstream_cx_http1_soft_drain, Accumulate)                                                \
   GAUGE(downstream_rq_active, Accumulate)                                                          \
   HISTOGRAM(downstream_cx_length_ms, Milliseconds)                                                 \
   HISTOGRAM(downstream_rq_time, Milliseconds)
@@ -290,6 +291,12 @@ class ConnectionManagerConfig {
    */
   virtual absl::optional<std::chrono::milliseconds> maxConnectionDuration() const PURE;
 
+  /**
+   * @return whether maxConnectionDuration allows HTTP1 clients to choose when to close connection
+   *         (rather than Envoy closing the connection itself when there are no active streams).
+   */
+  virtual bool http1SafeMaxConnectionDuration() const PURE;
+
   /**
    * @return maximum request headers size the connection manager will accept.
    */

source/common/http/conn_manager_impl.cc

@@ -213,6 +213,10 @@ ConnectionManagerImpl::~ConnectionManagerImpl() {
     }
   }
 
+  if (soft_drain_http1_) {
+    stats_.named_.downstream_cx_http1_soft_drain_.dec();
+  }
+
   conn_length_->complete();
   user_agent_.completeConnectionLength(*conn_length_);
 }
@@ -413,6 +417,12 @@ RequestDecoder& ConnectionManagerImpl::newStream(ResponseEncoder& response_encod
     stats_.named_.downstream_cx_max_requests_reached_.inc();
   }
 
+  if (soft_drain_http1_) {
+    new_stream->filter_manager_.streamInfo().setShouldDrainConnectionUponCompletion(true);
+    // Prevent erroneous debug log of closing due to incoming connection close header.
+    drain_state_ = DrainState::Closing;
+  }
+
   new_stream->state_.is_internally_created_ = is_internally_created;
   new_stream->response_encoder_ = &response_encoder;
   new_stream->response_encoder_->getStream().addCallbacks(*new_stream);
@@ -724,7 +734,15 @@ void ConnectionManagerImpl::onConnectionDurationTimeout() {
                       StreamInfo::CoreResponseFlag::DurationTimeout,
                       StreamInfo::ResponseCodeDetails::get().DurationTimeout);
   } else if (drain_state_ == DrainState::NotDraining) {
-    startDrainSequence();
+    if (config_->http1SafeMaxConnectionDuration() && codec_->protocol() < Protocol::Http2) {
+      ENVOY_CONN_LOG(debug,
+                     "HTTP1-safe max connection duration is configured -- skipping drain sequence.",
+                     read_callbacks_->connection());
+      stats_.named_.downstream_cx_http1_soft_drain_.inc();
+      soft_drain_http1_ = true;
+    } else {
+      startDrainSequence();
+    }
   }
 }
 

source/common/http/conn_manager_impl.h

@@ -606,6 +606,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
   // A connection duration timer. Armed during handling new connection if enabled in config.
   Event::TimerPtr connection_duration_timer_;
   Event::TimerPtr drain_timer_;
+  // When set to true, add Connection:close response header to nudge downstream client to reconnect.
+  bool soft_drain_http1_{false};
   Random::RandomGenerator& random_generator_;
   Runtime::Loader& runtime_;
   const LocalInfo::LocalInfo& local_info_;

source/extensions/filters/network/http_connection_manager/config.cc

@@ -375,6 +375,7 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
       idle_timeout_(PROTOBUF_GET_OPTIONAL_MS(config.common_http_protocol_options(), idle_timeout)),
       max_connection_duration_(
           PROTOBUF_GET_OPTIONAL_MS(config.common_http_protocol_options(), max_connection_duration)),
+      http1_safe_max_connection_duration_(config.http1_safe_max_connection_duration()),
       max_stream_duration_(
           PROTOBUF_GET_OPTIONAL_MS(config.common_http_protocol_options(), max_stream_duration)),
       stream_idle_timeout_(

source/extensions/filters/network/http_connection_manager/config.h

@@ -180,6 +180,9 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   absl::optional<std::chrono::milliseconds> maxConnectionDuration() const override {
     return max_connection_duration_;
   }
+  bool http1SafeMaxConnectionDuration() const override {
+    return http1_safe_max_connection_duration_;
+  }
   std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; }
   std::chrono::milliseconds requestTimeout() const override { return request_timeout_; }
   std::chrono::milliseconds requestHeadersTimeout() const override {
@@ -324,6 +327,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   const uint32_t max_request_headers_count_;
   absl::optional<std::chrono::milliseconds> idle_timeout_;
   absl::optional<std::chrono::milliseconds> max_connection_duration_;
+  const bool http1_safe_max_connection_duration_;
   absl::optional<std::chrono::milliseconds> max_stream_duration_;
   std::chrono::milliseconds stream_idle_timeout_;
   std::chrono::milliseconds request_timeout_;

source/server/admin/admin.h

@@ -148,6 +148,7 @@ class AdminImpl : public Admin,
   absl::optional<std::chrono::milliseconds> maxConnectionDuration() const override {
     return max_connection_duration_;
   }
+  bool http1SafeMaxConnectionDuration() const override { return false; }
   uint32_t maxRequestHeadersKb() const override { return max_request_headers_kb_; }
   uint32_t maxRequestHeadersCount() const override { return max_request_headers_count_; }
   std::chrono::milliseconds streamIdleTimeout() const override { return {}; }

test/common/http/conn_manager_impl_fuzz_test.cc

@@ -153,6 +153,9 @@ class FuzzConfig : public ConnectionManagerConfig {
   absl::optional<std::chrono::milliseconds> maxConnectionDuration() const override {
     return max_connection_duration_;
   }
+  bool http1SafeMaxConnectionDuration() const override {
+    return http1_safe_max_connection_duration_;
+  }
   absl::optional<std::chrono::milliseconds> maxStreamDuration() const override {
     return max_stream_duration_;
   }
@@ -276,6 +279,7 @@ class FuzzConfig : public ConnectionManagerConfig {
   uint32_t max_request_headers_count_{Http::DEFAULT_MAX_HEADERS_COUNT};
   absl::optional<std::chrono::milliseconds> idle_timeout_;
   absl::optional<std::chrono::milliseconds> max_connection_duration_;
+  bool http1_safe_max_connection_duration_{false};
   absl::optional<std::chrono::milliseconds> max_stream_duration_;
   std::chrono::milliseconds stream_idle_timeout_{};
   std::chrono::milliseconds request_timeout_{};