add http1-safe max_connection_duration for http connection manager

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,20 @@ message HttpConnectionManager {
   config.core.v3.HttpProtocolOptions common_http_protocol_options = 35
       [(udpa.annotations.security).configure_for_untrusted_downstream = true];
 
+  // If :ref:`common_http_protocol_options.max_connection_duration
+  // <envoy_v3_api_field_config.core.v3.HttpProtocolOptions.max_connection_duration>` is
+  // set and this is set to true, the ``max_connection_duration`` setting
+  // will not cause Envoy to ever close HTTP1 connections (other settings are unaffected). Instead,
+  // Envoy will start adding the Connection:close response header to all responses and let the
+  // client choose when to close the connection.
+  //
+  // If ``max_connection_duration`` is set and this is 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

@@ -50,6 +50,15 @@ behavior_changes:
   change: |
     Datadog: Disabled remote configuration by default.
     To enable this feature, use the :ref:`remote_config <envoy_v3_api_field_config.trace.v3.DatadogConfig.remote_config>` field.
+- 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: local_ratelimit
   change: |
     Updated the token bucket implementation to use non-timer based token bucket. The tokens will be refilled when

source/common/http/conn_manager_config.h

@@ -290,6 +290,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

@@ -723,7 +723,14 @@ 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());
+      soft_drain_http1_ = true;
+    } else {
+      startDrainSequence();
+    }
   }
 }
 
@@ -1780,7 +1787,8 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ResponseHeaderMap& heade
   // point, the cached route should never be updated or refreshed.
   blockRouteCache();
 
-  if (connection_manager_.drain_state_ != DrainState::NotDraining &&
+  if ((connection_manager_.drain_state_ != DrainState::NotDraining ||
+       connection_manager_.soft_drain_http1_) &&
       connection_manager_.codec_->protocol() < Protocol::Http2) {
     // If the connection manager is draining send "Connection: Close" on HTTP/1.1 connections.
     // Do not do this for H2 (which drains via GOAWAY) or Upgrade or CONNECT (as the

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

@@ -179,6 +179,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 {
@@ -323,6 +326,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

@@ -147,6 +147,9 @@ class AdminImpl : public Admin,
   absl::optional<std::chrono::milliseconds> maxConnectionDuration() const override {
     return max_connection_duration_;
   }
+  bool http1SafeMaxConnectionDuration() const override {
+    return http1_safe_max_connection_duration_;
+  }
   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 {}; }
@@ -477,6 +480,7 @@ class AdminImpl : public Admin,
   const 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_;
   absl::optional<std::string> user_agent_;
   Http::SlowDateProviderImpl date_provider_;

test/common/http/conn_manager_impl_fuzz_test.cc

@@ -152,6 +152,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_;
   }
@@ -275,6 +278,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_{};