From 17f836b33b7a6a4d60e578e9eca9616805a40000 Mon Sep 17 00:00:00 2001 From: Yao Zengzeng Date: Tue, 30 Nov 2021 03:45:48 +0800 Subject: [PATCH] http2: drain only once when reached max_requests_per_connection (#19078) Commit Message: drain only once when reached max_requests_per_connection Additional Description: fixes #19045 Risk Level: low Testing: unit test Docs Changes: n/a Signed-off-by: YaoZengzeng --- docs/root/version_history/current.rst | 1 + source/common/http/conn_manager_impl.cc | 2 +- test/common/http/conn_manager_impl_test_2.cc | 29 +++++++++++++++++++ .../common/http/conn_manager_impl_test_base.h | 5 ++-- 4 files changed, 34 insertions(+), 3 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 75fe04ac22552..0df1db889c439 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -16,6 +16,7 @@ Minor Behavior Changes `envoy.reloadable_features.http_strip_fragment_from_path_unsafe_if_disabled`. This runtime guard must only be set to false when existing non-compliant traffic relies on #fragment in URI. When this option is enabled, Envoy request authorization extensions may be bypassed. This override and its associated behavior will be decommissioned after the standard deprecation period. +* http: when envoy run out of ``max_requests_per_connection``, it will send an HTTP/2 "shutdown nofitication" (GOAWAY frame with max stream ID) and go to a default grace period of 5000 milliseconds (5 seconds) if ``drain_timeout`` is not specified. During this grace period, envoy will continue to accept new streams. After the grace period, a final GOAWAY is sent and envoy will start refusing new streams. However before bugfix, during the grace period, every time a new stream is received, old envoy will always send a "shutdown notification" and restart drain again which actually causes the grace period to be extended and is no longer equal to ``drain_timeout``. Bug Fixes --------- diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index cd5d04cd8497a..1106531eb22da 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -261,7 +261,7 @@ RequestDecoder& ConnectionManagerImpl::newStream(ResponseEncoder& response_encod new_stream->state_.saw_connection_close_ = true; // Prevent erroneous debug log of closing due to incoming connection close header. drain_state_ = DrainState::Closing; - } else { + } else if (drain_state_ == DrainState::NotDraining) { startDrainSequence(); } ENVOY_CONN_LOG(debug, "max requests per connection reached", read_callbacks_->connection()); diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index 76495244f5087..f4e193ece31d9 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -348,6 +348,35 @@ TEST_F(HttpConnectionManagerImplTest, ConnectionDurationNoCodec) { EXPECT_EQ(1U, stats_.named_.downstream_cx_max_duration_reached_.value()); } +// Regression test for https://github.com/envoyproxy/envoy/issues/19045 +TEST_F(HttpConnectionManagerImplTest, MaxRequests) { + max_requests_per_connection_ = 1; + codec_->protocol_ = Protocol::Http2; + setup(false, ""); + + Event::MockTimer* drain_timer = setUpTimer(); + EXPECT_CALL(*drain_timer, enableTimer(_, _)); + + EXPECT_CALL(*codec_, dispatch(_)).WillRepeatedly(Invoke([&](Buffer::Instance&) -> Http::Status { + conn_manager_->newStream(response_encoder_); + return Http::okStatus(); + })); + + EXPECT_CALL(*codec_, goAway()); + EXPECT_CALL(*codec_, shutdownNotice()); + EXPECT_CALL(*drain_timer, disableTimer()); + + // Kick off two requests. + Buffer::OwnedImpl fake_input("hello"); + conn_manager_->onData(fake_input, false); + conn_manager_->onData(fake_input, false); + drain_timer->invokeCallback(); + + EXPECT_EQ(2U, stats_.named_.downstream_cx_max_requests_reached_.value()); + + conn_manager_->onEvent(Network::ConnectionEvent::RemoteClose); +} + TEST_F(HttpConnectionManagerImplTest, ConnectionDuration) { max_connection_duration_ = (std::chrono::milliseconds(10)); Event::MockTimer* connection_duration_timer = setUpTimer(); diff --git a/test/common/http/conn_manager_impl_test_base.h b/test/common/http/conn_manager_impl_test_base.h index aad4e49a64d21..5df7fe3386a78 100644 --- a/test/common/http/conn_manager_impl_test_base.h +++ b/test/common/http/conn_manager_impl_test_base.h @@ -147,7 +147,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan pathWithEscapedSlashesAction() const override { return path_with_escaped_slashes_action_; } - uint64_t maxRequestsPerConnection() const override { return 0; } + uint64_t maxRequestsPerConnection() const override { return max_requests_per_connection_; } Envoy::Event::SimulatedTimeSystem test_time_; NiceMock route_config_provider_; @@ -177,6 +177,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan absl::optional user_agent_; uint32_t max_request_headers_kb_{Http::DEFAULT_MAX_REQUEST_HEADERS_KB}; uint32_t max_request_headers_count_{Http::DEFAULT_MAX_HEADERS_COUNT}; + uint64_t max_requests_per_connection_{}; absl::optional idle_timeout_; absl::optional max_connection_duration_; std::chrono::milliseconds stream_idle_timeout_{}; @@ -224,7 +225,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan PathWithEscapedSlashesAction path_with_escaped_slashes_action_{ envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager:: KEEP_UNCHANGED}; -}; + }; } // namespace Http } // namespace Envoy