add http1-safe max_connection_duration for http connection manager

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

@@ -447,16 +447,14 @@ 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 set to true, :ref:`common_http_protocol_options.max_connection_duration
+  // <envoy_v3_api_field_config.core.v3.HttpProtocolOptions.max_connection_duration>` will not
+  // trigger Envoy to close HTTP1 connections. Instead, Envoy will only add the Connection:close
+  // response header to all responses until the client chooses 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).
+  // 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;

changelogs/current.yaml

@@ -15,7 +15,7 @@ behavior_changes:
     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
+- area: stats scoped_rds
   change: |
     Added new tag extraction so that scoped rds stats have their :ref:'scope_route_config_name
     <envoy_v3_api_msg_config/route/v3/scoped_route>' and stat prefix extracted.

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)

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);
@@ -728,6 +738,7 @@ void ConnectionManagerImpl::onConnectionDurationTimeout() {
       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();
@@ -1788,8 +1799,7 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ResponseHeaderMap& heade
   // point, the cached route should never be updated or refreshed.
   blockRouteCache();
 
-  if ((connection_manager_.drain_state_ != DrainState::NotDraining ||
-       connection_manager_.soft_drain_http1_) &&
+  if (connection_manager_.drain_state_ != DrainState::NotDraining &&
       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/server/admin/admin.h

@@ -148,9 +148,7 @@ 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_;
-  }
+  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 {}; }
@@ -481,7 +479,6 @@ 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_test.cc

@@ -24,9 +24,7 @@ namespace Envoy {
 namespace Http {
 
 TEST_F(HttpConnectionManagerImplTest, HeaderOnlyRequestAndResponse) {
-  SetupOpts setup_opts;
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setTracing(false));
 
   // Store the basic request encoder during filter chain setup.
   std::shared_ptr<MockStreamDecoderFilter> filter(new NiceMock<MockStreamDecoderFilter>());
@@ -96,9 +94,7 @@ TEST_F(HttpConnectionManagerImplTest, HeaderOnlyRequestAndResponse) {
 // Similar to HeaderOnlyRequestAndResponse but uses newStreamHandle and has
 // lifetime checks.
 TEST_F(HttpConnectionManagerImplTest, HandleLifetime) {
-  SetupOpts setup_opts;
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setTracing(false));
   Http::RequestDecoderHandlePtr decoder_handle;
 
   // Store the basic request encoder during filter chain setup.
@@ -175,9 +171,7 @@ TEST_F(HttpConnectionManagerImplTest, HandleLifetime) {
 }
 
 TEST_F(HttpConnectionManagerImplTest, HeaderOnlyRequestAndResponseWithEarlyHeaderMutation) {
-  SetupOpts setup_opts;
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setTracing(false));
 
   auto mock_early_header_mutation_1 = std::make_unique<NiceMock<Http::MockEarlyHeaderMutation>>();
   auto mock_early_header_mutation_2 = std::make_unique<NiceMock<Http::MockEarlyHeaderMutation>>();
@@ -268,9 +262,7 @@ TEST_F(HttpConnectionManagerImplTest, HeaderOnlyRequestAndResponseWithEarlyHeade
 
 TEST_F(HttpConnectionManagerImplTest, 1xxResponse) {
   proxy_100_continue_ = true;
-  SetupOpts setup_opts;
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setTracing(false));
 
   // Store the basic request encoder during filter chain setup.
   std::shared_ptr<MockStreamDecoderFilter> filter(new NiceMock<MockStreamDecoderFilter>());
@@ -328,9 +320,7 @@ TEST_F(HttpConnectionManagerImplTest, 1xxResponse) {
 
 TEST_F(HttpConnectionManagerImplTest, 1xxResponseWithEncoderFiltersProxyingDisabled) {
   proxy_100_continue_ = false;
-  SetupOpts setup_opts;
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setTracing(false));
   setUpEncoderAndDecoder(false, false);
   sendRequestHeadersAndData();
 
@@ -357,9 +347,7 @@ TEST_F(HttpConnectionManagerImplTest, 1xxResponseWithEncoderFiltersProxyingDisab
 
 TEST_F(HttpConnectionManagerImplTest, 1xxResponseWithEncoderFilters) {
   proxy_100_continue_ = true;
-  SetupOpts setup_opts;
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setTracing(false));
   setUpEncoderAndDecoder(false, false);
   sendRequestHeadersAndData();
 
@@ -385,9 +373,7 @@ TEST_F(HttpConnectionManagerImplTest, 1xxResponseWithEncoderFilters) {
 
 TEST_F(HttpConnectionManagerImplTest, PauseResume1xx) {
   proxy_100_continue_ = true;
-  SetupOpts setup_opts;
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setTracing(false));
   setUpEncoderAndDecoder(false, false);
   sendRequestHeadersAndData();
 
@@ -421,9 +407,7 @@ TEST_F(HttpConnectionManagerImplTest, PauseResume1xx) {
 // Regression test for https://github.com/envoyproxy/envoy/issues/10923.
 TEST_F(HttpConnectionManagerImplTest, 1xxResponseWithDecoderPause) {
   proxy_100_continue_ = true;
-  SetupOpts setup_opts;
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setTracing(false));
 
   std::shared_ptr<MockStreamDecoderFilter> filter(new NiceMock<MockStreamDecoderFilter>());
 
@@ -490,10 +474,7 @@ TEST_F(HttpConnectionManagerImplTest, 1xxResponseWithDecoderPause) {
 
 // When create new stream, the stream info will be populated from the connection.
 TEST_F(HttpConnectionManagerImplTest, PopulateStreamInfo) {
-  SetupOpts setup_opts;
-  setup_opts.ssl = true;
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setSsl(true).setTracing(false));
 
   // Set up the codec.
   Buffer::OwnedImpl fake_input("input");
@@ -517,10 +498,7 @@ TEST_F(HttpConnectionManagerImplTest, PopulateStreamInfo) {
 
 // By default, Envoy will set the server header to the server name, here "custom-value"
 TEST_F(HttpConnectionManagerImplTest, ServerHeaderOverwritten) {
-  SetupOpts setup_opts;
-  setup_opts.server_name = "custom-value";
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setServerName("custom-value").setTracing(false));
   setUpEncoderAndDecoder(false, false);
 
   sendRequestHeadersAndData();
@@ -534,10 +512,7 @@ TEST_F(HttpConnectionManagerImplTest, ServerHeaderOverwritten) {
 // When configured APPEND_IF_ABSENT if the server header is present it will be retained.
 TEST_F(HttpConnectionManagerImplTest, ServerHeaderAppendPresent) {
   server_transformation_ = HttpConnectionManagerProto::APPEND_IF_ABSENT;
-  SetupOpts setup_opts;
-  setup_opts.server_name = "custom-value";
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setServerName("custom-value").setTracing(false));
   setUpEncoderAndDecoder(false, false);
 
   sendRequestHeadersAndData();
@@ -551,10 +526,7 @@ TEST_F(HttpConnectionManagerImplTest, ServerHeaderAppendPresent) {
 // When configured APPEND_IF_ABSENT if the server header is absent the server name will be set.
 TEST_F(HttpConnectionManagerImplTest, ServerHeaderAppendAbsent) {
   server_transformation_ = HttpConnectionManagerProto::APPEND_IF_ABSENT;
-  SetupOpts setup_opts;
-  setup_opts.server_name = "custom-value";
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setServerName("custom-value").setTracing(false));
   setUpEncoderAndDecoder(false, false);
 
   sendRequestHeadersAndData();
@@ -568,10 +540,7 @@ TEST_F(HttpConnectionManagerImplTest, ServerHeaderAppendAbsent) {
 // When configured PASS_THROUGH, the server name will pass through.
 TEST_F(HttpConnectionManagerImplTest, ServerHeaderPassthroughPresent) {
   server_transformation_ = HttpConnectionManagerProto::PASS_THROUGH;
-  SetupOpts setup_opts;
-  setup_opts.server_name = "custom-value";
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setServerName("custom-value").setTracing(false));
   setUpEncoderAndDecoder(false, false);
 
   sendRequestHeadersAndData();
@@ -585,10 +554,7 @@ TEST_F(HttpConnectionManagerImplTest, ServerHeaderPassthroughPresent) {
 // When configured PASS_THROUGH, the server header will not be added if absent.
 TEST_F(HttpConnectionManagerImplTest, ServerHeaderPassthroughAbsent) {
   server_transformation_ = HttpConnectionManagerProto::PASS_THROUGH;
-  SetupOpts setup_opts;
-  setup_opts.server_name = "custom-value";
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setServerName("custom-value").setTracing(false));
   setUpEncoderAndDecoder(false, false);
 
   sendRequestHeadersAndData();
@@ -2664,9 +2630,7 @@ TEST_F(HttpConnectionManagerImplTest, TestPeriodicAccessLogging) {
   request_headers_timeout_ = std::chrono::milliseconds(0);
   max_stream_duration_ = std::nullopt;
 
-  SetupOpts setup_opts;
-  setup_opts.server_name = "server-opts";
-  setup(setup_opts);
+  setup(SetupOpts().setServerName("server-opts"));
 
   std::shared_ptr<MockStreamDecoderFilter> filter(new NiceMock<MockStreamDecoderFilter>());
   std::shared_ptr<AccessLog::MockInstance> handler(new NiceMock<AccessLog::MockInstance>());
@@ -2825,9 +2789,7 @@ TEST_F(StreamErrorOnInvalidHttpMessageTest, ConnectionOpenIfCodecStreamErrorIsTr
 }
 
 TEST_F(HttpConnectionManagerImplTest, TestAccessLogSsl) {
-  SetupOpts setup_opts;
-  setup_opts.ssl = true;
-  setup(setup_opts);
+  setup(SetupOpts().setSsl(true));
 
   std::shared_ptr<MockStreamDecoderFilter> filter(new NiceMock<MockStreamDecoderFilter>());
   std::shared_ptr<AccessLog::MockInstance> handler(new NiceMock<AccessLog::MockInstance>());
@@ -4095,9 +4057,7 @@ TEST_F(HttpConnectionManagerImplTest, RejectWebSocketOnNonWebSocketRoute) {
 
 // Make sure for upgrades, we do not append Connection: Close when draining.
 TEST_F(HttpConnectionManagerImplTest, FooUpgradeDrainClose) {
-  SetupOpts setup_opts;
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setTracing(false));
 
   // Store the basic request encoder during filter chain setup.
   auto* filter = new MockStreamFilter();
@@ -4163,9 +4123,7 @@ TEST_F(HttpConnectionManagerImplTest, FooUpgradeDrainClose) {
 
 // Make sure CONNECT requests hit the upgrade filter path.
 TEST_F(HttpConnectionManagerImplTest, ConnectAsUpgrade) {
-  SetupOpts setup_opts;
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setTracing(false));
 
   EXPECT_CALL(filter_factory_, createUpgradeFilterChain("CONNECT", _, _, _))
       .WillRepeatedly(Return(true));
@@ -4189,9 +4147,7 @@ TEST_F(HttpConnectionManagerImplTest, ConnectAsUpgrade) {
 }
 
 TEST_F(HttpConnectionManagerImplTest, ConnectWithEmptyPath) {
-  SetupOpts setup_opts;
-  setup_opts.tracing = false;
-  setup(setup_opts);
+  setup(SetupOpts().setTracing(false));
 
   EXPECT_CALL(filter_factory_, createUpgradeFilterChain("CONNECT", _, _, _))
       .WillRepeatedly(Return(true));
@@ -4310,9 +4266,7 @@ TEST_F(HttpConnectionManagerImplTest,
 }
 
 TEST_F(HttpConnectionManagerImplTest, DrainClose) {
-  SetupOpts setup_opts;
-  setup_opts.ssl = true;
-  setup(setup_opts);
+  setup(SetupOpts().setSsl(true));
 
   MockStreamDecoderFilter* filter = new NiceMock<MockStreamDecoderFilter>();
   EXPECT_CALL(filter_factory_, createFilterChain(_))
@@ -4368,10 +4322,7 @@ TEST_F(HttpConnectionManagerImplTest, DrainClose) {
 class ProxyStatusTest : public HttpConnectionManagerImplTest {
 public:
   void initialize() {
-    SetupOpts setup_opts;
-    setup_opts.server_name = servername_;
-    setup_opts.tracing = false;
-    setup(setup_opts);
+    setup(SetupOpts().setServerName(servername_).setTracing(false));
     setUpEncoderAndDecoder(/*request_with_data_and_trailers=*/false,
                            /*decode_headers_stop_all=*/false);
     sendRequestHeadersAndData();