From cba00d7f9b4eafaf372da4e986f6eb5d2ea71c69 Mon Sep 17 00:00:00 2001 From: Biren Roy Date: Wed, 29 Apr 2026 10:44:44 -0400 Subject: [PATCH 1/7] Moves HTTP/2 GOAWAY load shed point handling to the ConnectionManager. Signed-off-by: Biren Roy --- source/common/http/conn_manager_impl.cc | 25 +++++- source/common/http/conn_manager_impl.h | 2 + source/common/http/conn_manager_utility.cc | 2 +- source/common/http/http2/codec_impl.cc | 28 +----- source/common/http/http2/codec_impl.h | 6 +- .../network/http_connection_manager/config.cc | 3 +- test/common/http/codec_impl_fuzz_test.cc | 2 +- test/common/http/conn_manager_impl_test_3.cc | 18 ++++ test/common/http/http2/codec_impl_test.cc | 86 ------------------- test/common/http/http2/codec_impl_test_util.h | 2 +- .../http/http2/http2_connection_fuzz_test.cc | 4 +- test/integration/BUILD | 2 +- test/integration/fake_upstream.cc | 8 +- test/integration/overload_integration_test.cc | 16 ++-- 14 files changed, 66 insertions(+), 138 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 77e44fe6fae18..2257afd39bd05 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -133,6 +133,10 @@ ConnectionManagerImpl::ConnectionManagerImpl( overload_manager.getLoadShedPoint(Server::LoadShedPointName::get().HcmDecodeHeaders)), hcm_ondata_creating_codec_( overload_manager.getLoadShedPoint(Server::LoadShedPointName::get().HcmCodecCreation)), + should_send_go_away_on_dispatch_(overload_manager.getLoadShedPoint( + Server::LoadShedPointName::get().H2ServerGoAwayOnDispatch)), + should_send_go_away_and_close_on_dispatch_(overload_manager.getLoadShedPoint( + Server::LoadShedPointName::get().H2ServerGoAwayAndCloseOnDispatch)), overload_stop_accepting_requests_ref_( overload_state_.getState(Server::OverloadActionNames::get().StopAcceptingRequests)), overload_disable_keepalive_ref_( @@ -155,6 +159,13 @@ ConnectionManagerImpl::ConnectionManagerImpl( ENVOY_LOG_ONCE_IF(trace, hcm_ondata_creating_codec_ == nullptr, "LoadShedPoint envoy.load_shed_points.hcm_ondata_creating_codec is not found. " "Is it configured?"); + ENVOY_LOG_ONCE_IF(trace, should_send_go_away_on_dispatch_ == nullptr, + "LoadShedPoint envoy.load_shed_points.http2_server_go_away_on_dispatch is not " + "found. Is it configured?"); + ENVOY_LOG_ONCE_IF( + trace, should_send_go_away_and_close_on_dispatch_ == nullptr, + "LoadShedPoint envoy.load_shed_points.http2_server_go_away_and_close_on_dispatch is not " + "found. Is it configured?"); } const ResponseHeaderMap& ConnectionManagerImpl::continueHeader() { @@ -514,6 +525,18 @@ Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool createCodec(data); } + if (should_send_go_away_and_close_on_dispatch_ != nullptr && + should_send_go_away_and_close_on_dispatch_->shouldShedLoad()) { + sendGoAwayAndClose(/*graceful=*/false); + handleCodecOverloadError( + "Load shed point http2_server_go_away_and_close_on_dispatch triggered"); + return Network::FilterStatus::StopIteration; + } + if (should_send_go_away_on_dispatch_ != nullptr && + should_send_go_away_on_dispatch_->shouldShedLoad()) { + sendGoAwayAndClose(/*graceful=*/true); + } + bool redispatch; do { redispatch = false; @@ -794,7 +817,7 @@ void ConnectionManagerImpl::onDrainTimeout() { } void ConnectionManagerImpl::sendGoAwayAndClose(bool graceful) { - ENVOY_CONN_LOG(trace, "connection manager sendGoAwayAndClose was triggerred from filters.", + ENVOY_CONN_LOG(trace, "connection manager sendGoAwayAndClose was triggered.", read_callbacks_->connection()); if (go_away_sent_) { return; diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 0077c748672ad..f8e17265a1516 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -665,6 +665,8 @@ class ConnectionManagerImpl : Logger::Loggable, Server::ThreadLocalOverloadState& overload_state_; Server::LoadShedPoint* accept_new_http_stream_{nullptr}; Server::LoadShedPoint* hcm_ondata_creating_codec_{nullptr}; + Server::LoadShedPoint* should_send_go_away_on_dispatch_{nullptr}; + Server::LoadShedPoint* should_send_go_away_and_close_on_dispatch_{nullptr}; // References into the overload manager thread local state map. Using these lets us avoid a // map lookup in the hot path of processing each request. const Server::OverloadActionState& overload_stop_accepting_requests_ref_; diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index c99ac3b57d6e9..f31f8ecd08928 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -99,7 +99,7 @@ ServerConnectionPtr ConnectionManagerUtility::autoCreateCodec( Http2::CodecStats& stats = Http2::CodecStats::atomicGet(http2_codec_stats, scope); return std::make_unique( connection, callbacks, stats, random, http2_options, max_request_headers_kb, - max_request_headers_count, headers_with_underscores_action, overload_manager); + max_request_headers_count, headers_with_underscores_action); } else { Http1::CodecStats& stats = Http1::CodecStats::atomicGet(http1_codec_stats, scope); return std::make_unique( diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index e02141e75fa60..3e572f7582240 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -2371,22 +2371,10 @@ ServerConnectionImpl::ServerConnectionImpl( const envoy::config::core::v3::Http2ProtocolOptions& http2_options, const uint32_t max_request_headers_kb, const uint32_t max_request_headers_count, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction - headers_with_underscores_action, - Server::OverloadManager& overload_manager) + headers_with_underscores_action) : ConnectionImpl(connection, stats, random_generator, http2_options, max_request_headers_kb, max_request_headers_count), - callbacks_(callbacks), headers_with_underscores_action_(headers_with_underscores_action), - should_send_go_away_on_dispatch_(overload_manager.getLoadShedPoint( - Server::LoadShedPointName::get().H2ServerGoAwayOnDispatch)), - should_send_go_away_and_close_on_dispatch_(overload_manager.getLoadShedPoint( - Server::LoadShedPointName::get().H2ServerGoAwayAndCloseOnDispatch)) { - ENVOY_LOG_ONCE_IF(trace, should_send_go_away_on_dispatch_ == nullptr, - "LoadShedPoint envoy.load_shed_points.http2_server_go_away_on_dispatch is not " - "found. Is it configured?"); - ENVOY_LOG_ONCE_IF( - trace, should_send_go_away_and_close_on_dispatch_ == nullptr, - "LoadShedPoint envoy.load_shed_points.http2_server_go_away_and_close_on_dispatch is not " - "found. Is it configured?"); + callbacks_(callbacks), headers_with_underscores_action_(headers_with_underscores_action) { Http2Options h2_options(http2_options, max_request_headers_kb); auto direct_visitor = std::make_unique(this); @@ -2452,18 +2440,6 @@ int ServerConnectionImpl::onHeader(int32_t stream_id, HeaderString&& name, Heade Http::Status ServerConnectionImpl::dispatch(Buffer::Instance& data) { // Make sure downstream outbound queue was not flooded by the upstream frames. RETURN_IF_ERROR(protocol_constraints_.checkOutboundFrameLimits()); - if (should_send_go_away_and_close_on_dispatch_ != nullptr && - should_send_go_away_and_close_on_dispatch_->shouldShedLoad()) { - ConnectionImpl::goAway(); - sent_go_away_on_dispatch_ = true; - return envoyOverloadError( - "Load shed point http2_server_go_away_and_close_on_dispatch triggered"); - } - if (should_send_go_away_on_dispatch_ != nullptr && !sent_go_away_on_dispatch_ && - should_send_go_away_on_dispatch_->shouldShedLoad()) { - ConnectionImpl::goAway(); - sent_go_away_on_dispatch_ = true; - } return ConnectionImpl::dispatch(data); } diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 20a620efdc2d9..90e83159c0282 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -858,8 +858,7 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { const uint32_t max_request_headers_kb, const uint32_t max_request_headers_count, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction - headers_with_underscores_action, - Server::OverloadManager& overload_manager); + headers_with_underscores_action); private: // ConnectionImpl @@ -885,9 +884,6 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { // The action to take when a request header name contains underscore characters. envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action_; - Server::LoadShedPoint* should_send_go_away_on_dispatch_{nullptr}; - Server::LoadShedPoint* should_send_go_away_and_close_on_dispatch_{nullptr}; - bool sent_go_away_on_dispatch_{false}; }; } // namespace Http2 diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 099bee0355176..0cc7e0ac06904 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -776,8 +776,7 @@ Http::ServerConnectionPtr HttpConnectionManagerConfig::createCodec( connection, callbacks, Http::Http2::CodecStats::atomicGet(http2_codec_stats_, context_.scope()), context_.serverFactoryContext().api().randomGenerator(), http2_options_, - maxRequestHeadersKb(), maxRequestHeadersCount(), headersWithUnderscoresAction(), - overload_manager); + maxRequestHeadersKb(), maxRequestHeadersCount(), headersWithUnderscoresAction()); case CodecType::HTTP3: #ifdef ENVOY_ENABLE_QUIC return Config::Utility::getAndCheckFactoryByName( diff --git a/test/common/http/codec_impl_fuzz_test.cc b/test/common/http/codec_impl_fuzz_test.cc index 2ce49b16cb616..b73c493205110 100644 --- a/test/common/http/codec_impl_fuzz_test.cc +++ b/test/common/http/codec_impl_fuzz_test.cc @@ -622,7 +622,7 @@ void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersi server = std::make_unique( server_connection, server_callbacks, Http2::CodecStats::atomicGet(http2_stats, scope), random, server_http2_options, max_request_headers_kb, max_request_headers_count, - headers_with_underscores_action, overload_manager_); + headers_with_underscores_action); } else { const Http1Settings server_http1settings{fromHttp1Settings(input.h1_settings().server())}; server = std::make_unique( diff --git a/test/common/http/conn_manager_impl_test_3.cc b/test/common/http/conn_manager_impl_test_3.cc index fdd43cf61168a..06499c59e5d89 100644 --- a/test/common/http/conn_manager_impl_test_3.cc +++ b/test/common/http/conn_manager_impl_test_3.cc @@ -848,6 +848,12 @@ TEST_F(HttpConnectionManagerImplTest, CodecCreationLoadShedPointCanCloseConnecti EXPECT_CALL(overload_manager_, getLoadShedPoint(Server::LoadShedPointName::get().HcmDecodeHeaders)) .WillOnce(Return(nullptr)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().H2ServerGoAwayOnDispatch)) + .WillOnce(Return(nullptr)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().H2ServerGoAwayAndCloseOnDispatch)) + .WillOnce(Return(nullptr)); setup(); @@ -874,6 +880,12 @@ TEST_F(HttpConnectionManagerImplTest, CodecCreationLoadShedPointBypasscheck) { EXPECT_CALL(overload_manager_, getLoadShedPoint(Server::LoadShedPointName::get().HttpDownstreamFilterCheck)) .WillOnce(Return(nullptr)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().H2ServerGoAwayOnDispatch)) + .WillOnce(Return(nullptr)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().H2ServerGoAwayAndCloseOnDispatch)) + .WillOnce(Return(nullptr)); setup(); @@ -904,6 +916,12 @@ TEST_F(HttpConnectionManagerImplTest, DecodeHeaderLoadShedPointCanRejectNewStrea EXPECT_CALL(overload_manager_, getLoadShedPoint(Server::LoadShedPointName::get().HttpDownstreamFilterCheck)) .WillRepeatedly(Return(nullptr)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().H2ServerGoAwayOnDispatch)) + .WillOnce(Return(nullptr)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().H2ServerGoAwayAndCloseOnDispatch)) + .WillOnce(Return(nullptr)); setup(); setupFilterChain(1, 0); diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index e53b7872079a1..f717f4587b968 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -4596,92 +4596,6 @@ TEST_P(Http2CodecImplTest, ChunkProcessingShouldNotScheduleIfReadDisabled) { } } -TEST_P(Http2CodecImplTest, ServerDispatchLoadShedPointCanCauseServerToSendGoAway) { - initialize(); - ASSERT_EQ(0, server_stats_store_.counter("http2.goaway_sent").value()); - - TestRequestHeaderMapImpl request_headers; - HttpTestUtility::addDefaultHeaders(request_headers); - EXPECT_CALL(server_->server_go_away_on_dispatch, shouldShedLoad()).WillOnce(Return(true)); - EXPECT_CALL(client_callbacks_, onGoAway(_)); - - if (http2_implementation_ == Http2Impl::Oghttp2) { - EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); - } else { - // nghttp2 does not raise the headers to the decoder. - EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); - } - driveToCompletion(); - - EXPECT_EQ(1, server_stats_store_.counter("http2.goaway_sent").value()); -} - -TEST_P(Http2CodecImplTest, ServerDispatchLoadShedPointSendGoAwayAndClose) { - expect_buffered_data_on_teardown_ = true; - - initialize(); - ASSERT_EQ(0, server_stats_store_.counter("http2.goaway_sent").value()); - - TestRequestHeaderMapImpl request_headers; - HttpTestUtility::addDefaultHeaders(request_headers); - EXPECT_CALL(server_->server_go_away_and_close_on_dispatch, shouldShedLoad()) - .WillOnce(Return(true)); - EXPECT_CALL(client_callbacks_, onGoAway(_)); - - EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)).Times(0); - EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); - - driveToCompletion(); - - EXPECT_EQ(1, server_stats_store_.counter("http2.goaway_sent").value()); -} - -TEST_P(Http2CodecImplTest, ServerDispatchLoadShedPointsAreOnlyConsultedOncePerDispatch) { - initialize(); - - int times_shed_load_goaway_invoked = 0; - EXPECT_CALL(server_->server_go_away_on_dispatch, shouldShedLoad()) - .WillRepeatedly(Invoke([×_shed_load_goaway_invoked]() { - ++times_shed_load_goaway_invoked; - return false; - })); - int times_shed_load_goaway_and_close_invoked = 0; - EXPECT_CALL(server_->server_go_away_and_close_on_dispatch, shouldShedLoad()) - .WillRepeatedly(Invoke([×_shed_load_goaway_and_close_invoked]() { - ++times_shed_load_goaway_and_close_invoked; - return false; - })); - - // Drive new streams to be created within a single server dispatch. - const int num_streams_to_create = 20; - TestRequestHeaderMapImpl request_headers; - HttpTestUtility::addDefaultHeaders(request_headers); - - std::vector request_encoders; - std::vector response_encoders; - request_encoders.reserve(num_streams_to_create); - response_encoders.reserve(num_streams_to_create); - for (int i = 0; i < num_streams_to_create; ++i) { - request_encoders.push_back(&client_->newStream(response_decoder_)); - EXPECT_CALL(server_callbacks_, newStream(_, _)) - .WillRepeatedly(Invoke([&](ResponseEncoder& encoder, bool) -> RequestDecoder& { - response_encoders.push_back(&encoder); - encoder.getStream().addCallbacks(server_stream_callbacks_); - return request_decoder_; - })); - - EXPECT_TRUE(request_encoders[i]->encodeHeaders(request_headers, true).ok()); - } - - // All the newly created streams are queued in the connection buffer. - EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)).Times(num_streams_to_create); - driveToCompletion(); - EXPECT_EQ(1, times_shed_load_goaway_invoked); - EXPECT_EQ(1, times_shed_load_goaway_and_close_invoked); - EXPECT_EQ(0, server_stats_store_.counter("http2.goaway_sent").value()); -} - TEST_P(Http2CodecImplTest, CheckHeaderPaddedWhitespaceValidation) { // Per https://datatracker.ietf.org/doc/html/rfc9113#section-8.2.1, // leading & trailing whitespace characters in headers are not valid, but this is a new diff --git a/test/common/http/http2/codec_impl_test_util.h b/test/common/http/http2/codec_impl_test_util.h index b8573acd61a7c..02cfac65fa871 100644 --- a/test/common/http/http2/codec_impl_test_util.h +++ b/test/common/http/http2/codec_impl_test_util.h @@ -95,7 +95,7 @@ class TestServerConnectionImpl : public TestCodecStatsProvider, : TestCodecStatsProvider(scope), ServerConnectionImpl(connection, callbacks, http2CodecStats(), random, http2_options, max_request_headers_kb, max_request_headers_count, - headers_with_underscores_action, overload_manager_) {} + headers_with_underscores_action) {} http2::adapter::Http2Adapter* adapter() { return adapter_.get(); } using ServerConnectionImpl::getStream; diff --git a/test/common/http/http2/http2_connection_fuzz_test.cc b/test/common/http/http2/http2_connection_fuzz_test.cc index c9a8561538cc8..f20914e9b8538 100644 --- a/test/common/http/http2/http2_connection_fuzz_test.cc +++ b/test/common/http/http2/http2_connection_fuzz_test.cc @@ -10,7 +10,6 @@ #include "test/fuzz/fuzz_runner.h" #include "test/mocks/http/mocks.h" #include "test/mocks/network/mocks.h" -#include "test/mocks/server/overload_manager.h" #include "test/test_common/test_runtime.h" #include "absl/strings/string_view.h" @@ -193,7 +192,7 @@ class Http2Harness { mock_server_connection_, mock_server_callbacks_, Http2::CodecStats::atomicGet(http2_stats_, scope), random_, server_settings_, Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT, - envoy::config::core::v3::HttpProtocolOptions::ALLOW, overload_manager_); + envoy::config::core::v3::HttpProtocolOptions::ALLOW); Buffer::OwnedImpl payload; payload.add(Http2Frame::Preamble, 24); @@ -214,7 +213,6 @@ class Http2Harness { NiceMock mock_server_connection_; NiceMock mock_server_callbacks_; NiceMock random_; - NiceMock overload_manager_; ServerConnectionPtr server_; }; diff --git a/test/integration/BUILD b/test/integration/BUILD index f49f41eb67d93..438a2632fccdd 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1843,7 +1843,7 @@ envoy_cc_test( size = "large", srcs = ["overload_integration_test.cc"], rbe_pool = "6gig", - shard_count = 4, + shard_count = 8, tags = [ "cpu:3", ], diff --git a/test/integration/fake_upstream.cc b/test/integration/fake_upstream.cc index 72ec3fec08971..63f8bc76f6f21 100644 --- a/test/integration/fake_upstream.cc +++ b/test/integration/fake_upstream.cc @@ -366,11 +366,10 @@ class TestHttp2ServerConnectionImpl : public Http::Http2::ServerConnectionImpl { const envoy::config::core::v3::Http2ProtocolOptions& http2_options, const uint32_t max_request_headers_kb, const uint32_t max_request_headers_count, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction - headers_with_underscores_action, - Server::OverloadManager& overload_manager) + headers_with_underscores_action) : ServerConnectionImpl(connection, callbacks, stats, random_generator, http2_options, max_request_headers_kb, max_request_headers_count, - headers_with_underscores_action, overload_manager) {} + headers_with_underscores_action) {} void updateConcurrentStreams(uint32_t max_streams) { absl::InlinedVector settings; @@ -419,8 +418,7 @@ FakeHttpConnection::FakeHttpConnection( Http::Http2::CodecStats& stats = fake_upstream.http2CodecStats(); codec_ = std::make_unique( shared_connection_.connection(), *this, stats, random_, http2_options, - max_request_headers_kb, max_request_headers_count, headers_with_underscores_action, - overload_manager_); + max_request_headers_kb, max_request_headers_count, headers_with_underscores_action); } else { ASSERT(type == Http::CodecType::HTTP3); #ifdef ENVOY_ENABLE_QUIC diff --git a/test/integration/overload_integration_test.cc b/test/integration/overload_integration_test.cc index 6b45429ccd435..1e3d8c02ab4d5 100644 --- a/test/integration/overload_integration_test.cc +++ b/test/integration/overload_integration_test.cc @@ -1219,13 +1219,17 @@ TEST_P(LoadShedPointIntegrationTest, Http2ServerDispatchSendsGoAwayCompletingPen first_request_encoder.encodeData(first_request_body, true); ASSERT_TRUE(first_request_decoder->waitForEndStream()); + // This is the initial GOAWAY, with max stream ID. EXPECT_TRUE(codec_client_->sawGoAway()); + + // This waits for the final GOAWAY, with a real stream ID. test_server_->waitForCounterEq("http2.goaway_sent", 1); - // The GOAWAY gets submitted with the first created stream as the last stream - // that will be processed on this connection, so the second stream's frames - // are ignored. - EXPECT_FALSE(second_request_decoder->complete()); + // Because the load shed operation uses a two-phase GOAWAY, a request initiated before the drain + // timer fires will be processed as usual. + EXPECT_TRUE(second_request_decoder->complete()); + + ASSERT_TRUE(codec_client_->waitForDisconnect()); updateResource(0.80); test_server_->waitForGaugeEq( @@ -1265,9 +1269,9 @@ TEST_P(LoadShedPointIntegrationTest, Http2ServerDispatchSendsGoAwayAndClosesConn ASSERT_TRUE(codec_client_->waitForDisconnect()); EXPECT_TRUE(codec_client_->sawGoAway()); test_server_->waitForCounterEq("http2.goaway_sent", 1); - test_server_->waitForCounterEq("http.config_test.downstream_rq_overload_close", 1); - // The second request will not complete. + // The second request is ignored and will not complete, since the connection manager stops network + // filter iteration. EXPECT_FALSE(second_request_decoder->complete()); } From 945d950dc5b9e1b5b2720fd94886bbebe2685ceb Mon Sep 17 00:00:00 2001 From: Biren Roy Date: Tue, 5 May 2026 23:33:16 -0400 Subject: [PATCH 2/7] Added some test cases. Signed-off-by: Biren Roy --- test/common/http/conn_manager_impl_test_3.cc | 77 ++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/test/common/http/conn_manager_impl_test_3.cc b/test/common/http/conn_manager_impl_test_3.cc index 06499c59e5d89..3e1ede560f08d 100644 --- a/test/common/http/conn_manager_impl_test_3.cc +++ b/test/common/http/conn_manager_impl_test_3.cc @@ -956,6 +956,83 @@ TEST_F(HttpConnectionManagerImplTest, DecodeHeaderLoadShedPointCanRejectNewStrea ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true, "details"); } +TEST_F(HttpConnectionManagerImplTest, GoAwayLoadShedPoint) { + Server::MockLoadShedPoint goaway_point; + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().HcmDecodeHeaders)) + .WillOnce(Return(nullptr)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().HcmCodecCreation)) + .WillOnce(Return(nullptr)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().HttpDownstreamFilterCheck)) + .WillRepeatedly(Return(nullptr)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().H2ServerGoAwayOnDispatch)) + .WillOnce(Return(&goaway_point)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().H2ServerGoAwayAndCloseOnDispatch)) + .WillOnce(Return(nullptr)); + + setup(); + setupFilterChain(1, 0); + + EXPECT_CALL(goaway_point, shouldShedLoad()).WillOnce(Return(true)); + + EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) + .WillOnce(Invoke([](const ResponseHeaderMap& headers, bool) -> void { + EXPECT_EQ("200", headers.getStatusValue()); + })); + + // The client will see a warning GOAWAY, but no final GOAWAY. + EXPECT_CALL(*codec_, shutdownNotice()); + EXPECT_CALL(*codec_, goAway()).Times(0); + + startRequest(); + + // Clean up. + expectOnDestroy(); + + decoder_filters_[0]->callbacks_->streamInfo().setResponseCodeDetails(""); + decoder_filters_[0]->callbacks_->encodeHeaders( + ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true, "details"); +} + +TEST_F(HttpConnectionManagerImplTest, GoAwayAndCloseLoadShedPoint) { + Server::MockLoadShedPoint goaway_and_close_point; + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().HcmDecodeHeaders)) + .WillOnce(Return(nullptr)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().HcmCodecCreation)) + .WillOnce(Return(nullptr)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().HttpDownstreamFilterCheck)) + .WillRepeatedly(Return(nullptr)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().H2ServerGoAwayOnDispatch)) + .WillOnce(Return(nullptr)); + EXPECT_CALL(overload_manager_, + getLoadShedPoint(Server::LoadShedPointName::get().H2ServerGoAwayAndCloseOnDispatch)) + .WillOnce(Return(&goaway_and_close_point)); + + setup(); + + EXPECT_CALL(goaway_and_close_point, shouldShedLoad()).WillOnce(Return(true)); + + EXPECT_CALL(response_encoder_, encodeHeaders(_, _)).Times(0); + + // The client will see the warning GOAWAY and final GOAWAY immediately afterward. + EXPECT_CALL(*codec_, shutdownNotice()); + EXPECT_CALL(*codec_, goAway()); + + // Gives the connection manager an event to handle. + Buffer::OwnedImpl fake_input; + conn_manager_->onData(fake_input, false); + + // No cleanup because the filter chain is not even created. +} + TEST_F(HttpConnectionManagerImplTest, TestStopAllIterationAndBufferOnDecodingPathFirstFilter) { setup(SetupOpts().setTracing(false)); setUpEncoderAndDecoder(true, true); From 2cd26a92a29e786b93aa8eb50087f74237a6bf1f Mon Sep 17 00:00:00 2001 From: Biren Roy Date: Wed, 6 May 2026 09:40:56 -0400 Subject: [PATCH 3/7] Reduces test flakiness. Signed-off-by: Biren Roy --- test/integration/overload_integration_test.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/integration/overload_integration_test.cc b/test/integration/overload_integration_test.cc index 1e3d8c02ab4d5..5277326a19f6d 100644 --- a/test/integration/overload_integration_test.cc +++ b/test/integration/overload_integration_test.cc @@ -1191,6 +1191,11 @@ TEST_P(LoadShedPointIntegrationTest, Http2ServerDispatchSendsGoAwayCompletingPen if (downstreamProtocol() != Http::CodecClient::Type::HTTP2) { return; } + config_helper_.addConfigModifier( + [=](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + cm) -> void { + cm.mutable_drain_timeout()->MergeFrom(ProtobufUtil::TimeUtil::SecondsToDuration(7)); + }); autonomous_upstream_ = true; initializeOverloadManager( TestUtility::parseYaml(R"EOF( From c13545e56a5991fa5c2700ca63e1a664e4bbd7d7 Mon Sep 17 00:00:00 2001 From: Biren Roy Date: Wed, 6 May 2026 11:41:12 -0400 Subject: [PATCH 4/7] One last fix for test flakiness. Signed-off-by: Biren Roy --- test/integration/overload_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/overload_integration_test.cc b/test/integration/overload_integration_test.cc index 5277326a19f6d..4bbb69ae06491 100644 --- a/test/integration/overload_integration_test.cc +++ b/test/integration/overload_integration_test.cc @@ -1232,7 +1232,7 @@ TEST_P(LoadShedPointIntegrationTest, Http2ServerDispatchSendsGoAwayCompletingPen // Because the load shed operation uses a two-phase GOAWAY, a request initiated before the drain // timer fires will be processed as usual. - EXPECT_TRUE(second_request_decoder->complete()); + EXPECT_TRUE(second_request_decoder->waitForEndStream()); ASSERT_TRUE(codec_client_->waitForDisconnect()); From a07f2f243d11d7454b2e408a554493f1af26b4cc Mon Sep 17 00:00:00 2001 From: Biren Roy Date: Thu, 14 May 2026 18:17:18 -0400 Subject: [PATCH 5/7] Added runtime feature. Signed-off-by: Biren Roy --- source/common/http/conn_manager_impl.cc | 22 +++-- source/common/http/conn_manager_utility.cc | 2 +- source/common/http/http2/codec_impl.cc | 31 +++++- source/common/http/http2/codec_impl.h | 7 +- source/common/runtime/runtime_features.cc | 1 + .../network/http_connection_manager/config.cc | 3 +- test/common/http/codec_impl_fuzz_test.cc | 2 +- test/common/http/conn_manager_impl_test_3.cc | 8 ++ test/common/http/http2/codec_impl_test.cc | 96 +++++++++++++++++++ test/common/http/http2/codec_impl_test_util.h | 2 +- .../http/http2/http2_connection_fuzz_test.cc | 4 +- test/integration/fake_upstream.cc | 8 +- test/integration/overload_integration_test.cc | 15 ++- 13 files changed, 176 insertions(+), 25 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 2257afd39bd05..d6940a569b90d 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -525,16 +525,18 @@ Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool createCodec(data); } - if (should_send_go_away_and_close_on_dispatch_ != nullptr && - should_send_go_away_and_close_on_dispatch_->shouldShedLoad()) { - sendGoAwayAndClose(/*graceful=*/false); - handleCodecOverloadError( - "Load shed point http2_server_go_away_and_close_on_dispatch triggered"); - return Network::FilterStatus::StopIteration; - } - if (should_send_go_away_on_dispatch_ != nullptr && - should_send_go_away_on_dispatch_->shouldShedLoad()) { - sendGoAwayAndClose(/*graceful=*/true); + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http2_fix_goaway_loadshed_point")) { + if (should_send_go_away_and_close_on_dispatch_ != nullptr && + should_send_go_away_and_close_on_dispatch_->shouldShedLoad()) { + sendGoAwayAndClose(/*graceful=*/false); + handleCodecOverloadError( + "Load shed point http2_server_go_away_and_close_on_dispatch triggered"); + return Network::FilterStatus::StopIteration; + } + if (should_send_go_away_on_dispatch_ != nullptr && + should_send_go_away_on_dispatch_->shouldShedLoad()) { + sendGoAwayAndClose(/*graceful=*/true); + } } bool redispatch; diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index f31f8ecd08928..c99ac3b57d6e9 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -99,7 +99,7 @@ ServerConnectionPtr ConnectionManagerUtility::autoCreateCodec( Http2::CodecStats& stats = Http2::CodecStats::atomicGet(http2_codec_stats, scope); return std::make_unique( connection, callbacks, stats, random, http2_options, max_request_headers_kb, - max_request_headers_count, headers_with_underscores_action); + max_request_headers_count, headers_with_underscores_action, overload_manager); } else { Http1::CodecStats& stats = Http1::CodecStats::atomicGet(http1_codec_stats, scope); return std::make_unique( diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 3e572f7582240..f1e2005117638 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -2371,10 +2371,22 @@ ServerConnectionImpl::ServerConnectionImpl( const envoy::config::core::v3::Http2ProtocolOptions& http2_options, const uint32_t max_request_headers_kb, const uint32_t max_request_headers_count, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction - headers_with_underscores_action) + headers_with_underscores_action, + Server::OverloadManager& overload_manager) : ConnectionImpl(connection, stats, random_generator, http2_options, max_request_headers_kb, max_request_headers_count), - callbacks_(callbacks), headers_with_underscores_action_(headers_with_underscores_action) { + callbacks_(callbacks), headers_with_underscores_action_(headers_with_underscores_action), + should_send_go_away_on_dispatch_(overload_manager.getLoadShedPoint( + Server::LoadShedPointName::get().H2ServerGoAwayOnDispatch)), + should_send_go_away_and_close_on_dispatch_(overload_manager.getLoadShedPoint( + Server::LoadShedPointName::get().H2ServerGoAwayAndCloseOnDispatch)) { + ENVOY_LOG_ONCE_IF(trace, should_send_go_away_on_dispatch_ == nullptr, + "LoadShedPoint envoy.load_shed_points.http2_server_go_away_on_dispatch is not " + "found. Is it configured?"); + ENVOY_LOG_ONCE_IF( + trace, should_send_go_away_and_close_on_dispatch_ == nullptr, + "LoadShedPoint envoy.load_shed_points.http2_server_go_away_and_close_on_dispatch is not " + "found. Is it configured?"); Http2Options h2_options(http2_options, max_request_headers_kb); auto direct_visitor = std::make_unique(this); @@ -2440,6 +2452,21 @@ int ServerConnectionImpl::onHeader(int32_t stream_id, HeaderString&& name, Heade Http::Status ServerConnectionImpl::dispatch(Buffer::Instance& data) { // Make sure downstream outbound queue was not flooded by the upstream frames. RETURN_IF_ERROR(protocol_constraints_.checkOutboundFrameLimits()); + if (!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.http2_fix_goaway_loadshed_point")) { + if (should_send_go_away_and_close_on_dispatch_ != nullptr && + should_send_go_away_and_close_on_dispatch_->shouldShedLoad()) { + ConnectionImpl::goAway(); + sent_go_away_on_dispatch_ = true; + return envoyOverloadError( + "Load shed point http2_server_go_away_and_close_on_dispatch triggered"); + } + if (should_send_go_away_on_dispatch_ != nullptr && !sent_go_away_on_dispatch_ && + should_send_go_away_on_dispatch_->shouldShedLoad()) { + ConnectionImpl::goAway(); + sent_go_away_on_dispatch_ = true; + } + } return ConnectionImpl::dispatch(data); } diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 90e83159c0282..20288fb3c7e70 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -858,7 +858,8 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { const uint32_t max_request_headers_kb, const uint32_t max_request_headers_count, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction - headers_with_underscores_action); + headers_with_underscores_action, + Server::OverloadManager& overload_manager); private: // ConnectionImpl @@ -884,6 +885,10 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { // The action to take when a request header name contains underscore characters. envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action_; + // Remove when removing runtime feature `http2_fix_goaway_loadshed_point`. + Server::LoadShedPoint* should_send_go_away_on_dispatch_{nullptr}; + Server::LoadShedPoint* should_send_go_away_and_close_on_dispatch_{nullptr}; + bool sent_go_away_on_dispatch_{false}; }; } // namespace Http2 diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index cb461bd701b33..c609e90fb9f85 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -65,6 +65,7 @@ RUNTIME_GUARD(envoy_reloadable_features_health_check_after_cluster_warming); RUNTIME_GUARD(envoy_reloadable_features_hide_transport_failure_reason_in_response_body); RUNTIME_GUARD(envoy_reloadable_features_http1_close_connection_on_zombie_stream_complete); RUNTIME_GUARD(envoy_reloadable_features_http2_discard_host_header); +RUNTIME_GUARD(envoy_reloadable_features_http2_fix_goaway_loadshed_point); RUNTIME_GUARD(envoy_reloadable_features_http_async_client_retry_respect_buffer_limits); // Delay deprecation and decommission until UHV is enabled. RUNTIME_GUARD(envoy_reloadable_features_http_reject_path_with_fragment); diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 0cc7e0ac06904..099bee0355176 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -776,7 +776,8 @@ Http::ServerConnectionPtr HttpConnectionManagerConfig::createCodec( connection, callbacks, Http::Http2::CodecStats::atomicGet(http2_codec_stats_, context_.scope()), context_.serverFactoryContext().api().randomGenerator(), http2_options_, - maxRequestHeadersKb(), maxRequestHeadersCount(), headersWithUnderscoresAction()); + maxRequestHeadersKb(), maxRequestHeadersCount(), headersWithUnderscoresAction(), + overload_manager); case CodecType::HTTP3: #ifdef ENVOY_ENABLE_QUIC return Config::Utility::getAndCheckFactoryByName( diff --git a/test/common/http/codec_impl_fuzz_test.cc b/test/common/http/codec_impl_fuzz_test.cc index b73c493205110..2ce49b16cb616 100644 --- a/test/common/http/codec_impl_fuzz_test.cc +++ b/test/common/http/codec_impl_fuzz_test.cc @@ -622,7 +622,7 @@ void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersi server = std::make_unique( server_connection, server_callbacks, Http2::CodecStats::atomicGet(http2_stats, scope), random, server_http2_options, max_request_headers_kb, max_request_headers_count, - headers_with_underscores_action); + headers_with_underscores_action, overload_manager_); } else { const Http1Settings server_http1settings{fromHttp1Settings(input.h1_settings().server())}; server = std::make_unique( diff --git a/test/common/http/conn_manager_impl_test_3.cc b/test/common/http/conn_manager_impl_test_3.cc index 2c53b8d11bfe3..cbb50fa3838de 100644 --- a/test/common/http/conn_manager_impl_test_3.cc +++ b/test/common/http/conn_manager_impl_test_3.cc @@ -957,6 +957,10 @@ TEST_F(HttpConnectionManagerImplTest, DecodeHeaderLoadShedPointCanRejectNewStrea } TEST_F(HttpConnectionManagerImplTest, GoAwayLoadShedPoint) { + if (!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.http2_fix_goaway_loadshed_point")) { + GTEST_SKIP(); + } Server::MockLoadShedPoint goaway_point; EXPECT_CALL(overload_manager_, getLoadShedPoint(Server::LoadShedPointName::get().HcmDecodeHeaders)) @@ -999,6 +1003,10 @@ TEST_F(HttpConnectionManagerImplTest, GoAwayLoadShedPoint) { } TEST_F(HttpConnectionManagerImplTest, GoAwayAndCloseLoadShedPoint) { + if (!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.http2_fix_goaway_loadshed_point")) { + GTEST_SKIP(); + } Server::MockLoadShedPoint goaway_and_close_point; EXPECT_CALL(overload_manager_, getLoadShedPoint(Server::LoadShedPointName::get().HcmDecodeHeaders)) diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index c4ebdced54d58..64acf8ae25bbd 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -4591,6 +4591,102 @@ TEST_P(Http2CodecImplTest, ChunkProcessingShouldNotScheduleIfReadDisabled) { } } +TEST_P(Http2CodecImplTest, ServerDispatchLoadShedPointCanCauseServerToSendGoAway) { + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http2_fix_goaway_loadshed_point")) { + GTEST_SKIP(); + } + initialize(); + ASSERT_EQ(0, server_stats_store_.counter("http2.goaway_sent").value()); + + TestRequestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(server_->server_go_away_on_dispatch, shouldShedLoad()).WillOnce(Return(true)); + EXPECT_CALL(client_callbacks_, onGoAway(_)); + + if (http2_implementation_ == Http2Impl::Oghttp2) { + EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); + } else { + // nghttp2 does not raise the headers to the decoder. + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); + } + driveToCompletion(); + + EXPECT_EQ(1, server_stats_store_.counter("http2.goaway_sent").value()); +} + +TEST_P(Http2CodecImplTest, ServerDispatchLoadShedPointSendGoAwayAndClose) { + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http2_fix_goaway_loadshed_point")) { + GTEST_SKIP(); + } + expect_buffered_data_on_teardown_ = true; + + initialize(); + ASSERT_EQ(0, server_stats_store_.counter("http2.goaway_sent").value()); + + TestRequestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(server_->server_go_away_and_close_on_dispatch, shouldShedLoad()) + .WillOnce(Return(true)); + EXPECT_CALL(client_callbacks_, onGoAway(_)); + + EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)).Times(0); + EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok()); + + driveToCompletion(); + + EXPECT_EQ(1, server_stats_store_.counter("http2.goaway_sent").value()); +} + +TEST_P(Http2CodecImplTest, ServerDispatchLoadShedPointsAreOnlyConsultedOncePerDispatch) { + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http2_fix_goaway_loadshed_point")) { + GTEST_SKIP(); + } + + initialize(); + + int times_shed_load_goaway_invoked = 0; + EXPECT_CALL(server_->server_go_away_on_dispatch, shouldShedLoad()) + .WillRepeatedly(Invoke([×_shed_load_goaway_invoked]() { + ++times_shed_load_goaway_invoked; + return false; + })); + int times_shed_load_goaway_and_close_invoked = 0; + EXPECT_CALL(server_->server_go_away_and_close_on_dispatch, shouldShedLoad()) + .WillRepeatedly(Invoke([×_shed_load_goaway_and_close_invoked]() { + ++times_shed_load_goaway_and_close_invoked; + return false; + })); + + // Drive new streams to be created within a single server dispatch. + const int num_streams_to_create = 20; + TestRequestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + + std::vector request_encoders; + std::vector response_encoders; + request_encoders.reserve(num_streams_to_create); + response_encoders.reserve(num_streams_to_create); + for (int i = 0; i < num_streams_to_create; ++i) { + request_encoders.push_back(&client_->newStream(response_decoder_)); + EXPECT_CALL(server_callbacks_, newStream(_, _)) + .WillRepeatedly(Invoke([&](ResponseEncoder& encoder, bool) -> RequestDecoder& { + response_encoders.push_back(&encoder); + encoder.getStream().addCallbacks(server_stream_callbacks_); + return request_decoder_; + })); + + EXPECT_TRUE(request_encoders[i]->encodeHeaders(request_headers, true).ok()); + } + + // All the newly created streams are queued in the connection buffer. + EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)).Times(num_streams_to_create); + driveToCompletion(); + EXPECT_EQ(1, times_shed_load_goaway_invoked); + EXPECT_EQ(1, times_shed_load_goaway_and_close_invoked); + EXPECT_EQ(0, server_stats_store_.counter("http2.goaway_sent").value()); +} + TEST_P(Http2CodecImplTest, CheckHeaderPaddedWhitespaceValidation) { // Per https://datatracker.ietf.org/doc/html/rfc9113#section-8.2.1, // leading & trailing whitespace characters in headers are not valid, but this is a new diff --git a/test/common/http/http2/codec_impl_test_util.h b/test/common/http/http2/codec_impl_test_util.h index 02cfac65fa871..b8573acd61a7c 100644 --- a/test/common/http/http2/codec_impl_test_util.h +++ b/test/common/http/http2/codec_impl_test_util.h @@ -95,7 +95,7 @@ class TestServerConnectionImpl : public TestCodecStatsProvider, : TestCodecStatsProvider(scope), ServerConnectionImpl(connection, callbacks, http2CodecStats(), random, http2_options, max_request_headers_kb, max_request_headers_count, - headers_with_underscores_action) {} + headers_with_underscores_action, overload_manager_) {} http2::adapter::Http2Adapter* adapter() { return adapter_.get(); } using ServerConnectionImpl::getStream; diff --git a/test/common/http/http2/http2_connection_fuzz_test.cc b/test/common/http/http2/http2_connection_fuzz_test.cc index f20914e9b8538..c9a8561538cc8 100644 --- a/test/common/http/http2/http2_connection_fuzz_test.cc +++ b/test/common/http/http2/http2_connection_fuzz_test.cc @@ -10,6 +10,7 @@ #include "test/fuzz/fuzz_runner.h" #include "test/mocks/http/mocks.h" #include "test/mocks/network/mocks.h" +#include "test/mocks/server/overload_manager.h" #include "test/test_common/test_runtime.h" #include "absl/strings/string_view.h" @@ -192,7 +193,7 @@ class Http2Harness { mock_server_connection_, mock_server_callbacks_, Http2::CodecStats::atomicGet(http2_stats_, scope), random_, server_settings_, Http::DEFAULT_MAX_REQUEST_HEADERS_KB, Http::DEFAULT_MAX_HEADERS_COUNT, - envoy::config::core::v3::HttpProtocolOptions::ALLOW); + envoy::config::core::v3::HttpProtocolOptions::ALLOW, overload_manager_); Buffer::OwnedImpl payload; payload.add(Http2Frame::Preamble, 24); @@ -213,6 +214,7 @@ class Http2Harness { NiceMock mock_server_connection_; NiceMock mock_server_callbacks_; NiceMock random_; + NiceMock overload_manager_; ServerConnectionPtr server_; }; diff --git a/test/integration/fake_upstream.cc b/test/integration/fake_upstream.cc index 63f8bc76f6f21..72ec3fec08971 100644 --- a/test/integration/fake_upstream.cc +++ b/test/integration/fake_upstream.cc @@ -366,10 +366,11 @@ class TestHttp2ServerConnectionImpl : public Http::Http2::ServerConnectionImpl { const envoy::config::core::v3::Http2ProtocolOptions& http2_options, const uint32_t max_request_headers_kb, const uint32_t max_request_headers_count, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction - headers_with_underscores_action) + headers_with_underscores_action, + Server::OverloadManager& overload_manager) : ServerConnectionImpl(connection, callbacks, stats, random_generator, http2_options, max_request_headers_kb, max_request_headers_count, - headers_with_underscores_action) {} + headers_with_underscores_action, overload_manager) {} void updateConcurrentStreams(uint32_t max_streams) { absl::InlinedVector settings; @@ -418,7 +419,8 @@ FakeHttpConnection::FakeHttpConnection( Http::Http2::CodecStats& stats = fake_upstream.http2CodecStats(); codec_ = std::make_unique( shared_connection_.connection(), *this, stats, random_, http2_options, - max_request_headers_kb, max_request_headers_count, headers_with_underscores_action); + max_request_headers_kb, max_request_headers_count, headers_with_underscores_action, + overload_manager_); } else { ASSERT(type == Http::CodecType::HTTP3); #ifdef ENVOY_ENABLE_QUIC diff --git a/test/integration/overload_integration_test.cc b/test/integration/overload_integration_test.cc index 4efe26e3c7172..edd8574013898 100644 --- a/test/integration/overload_integration_test.cc +++ b/test/integration/overload_integration_test.cc @@ -1247,11 +1247,18 @@ TEST_P(LoadShedPointIntegrationTest, Http2ServerDispatchSendsGoAwayCompletingPen // This waits for the final GOAWAY, with a real stream ID. test_server_->waitForCounter("http2.goaway_sent", Eq(1)); - // Because the load shed operation uses a two-phase GOAWAY, a request initiated before the drain - // timer fires will be processed as usual. - EXPECT_TRUE(second_request_decoder->waitForEndStream()); + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http2_fix_goaway_loadshed_point")) { + // Because the load shed operation uses a two-phase GOAWAY, a request initiated before the drain + // timer fires will be processed as usual. + EXPECT_TRUE(second_request_decoder->waitForEndStream()); - ASSERT_TRUE(codec_client_->waitForDisconnect()); + ASSERT_TRUE(codec_client_->waitForDisconnect()); + } else { + // The GOAWAY gets submitted with the first created stream as the last stream + // that will be processed on this connection, so the second stream's frames + // are ignored. + EXPECT_FALSE(second_request_decoder->complete()); + } updateResource(0.80); test_server_->waitForGauge( From 677b32fc5a65b5282d61a3934c086a351022cc59 Mon Sep 17 00:00:00 2001 From: Biren Roy Date: Thu, 14 May 2026 18:59:49 -0400 Subject: [PATCH 6/7] Added a changelog entry. Signed-off-by: Biren Roy --- changelogs/current.yaml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 0fc3636aa24fb..8072da720a6fd 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -43,6 +43,11 @@ minor_behavior_changes: graceful close against the reset. This behavioral change can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.ssl_socket_report_connection_reset`` to ``false``. +- area: http2 + change: | + The GOAWAY load shed point is fixed to use a graceful two-phase shutdown sequence, to avoid + risk to client traffic. This behavioral change can be temporarily reverted by setting runtime + guard ``envoy.reloadable_features.http2_fix_goaway_loadshed_point`` to ``false``. bug_fixes: # *Changes expected to improve the state of the world and are unlikely to have negative effects* From 88aa8e3b015ee0218729d58173879e5ddd74ba4d Mon Sep 17 00:00:00 2001 From: Biren Roy Date: Fri, 15 May 2026 09:26:45 -0400 Subject: [PATCH 7/7] Trying to fix coverage. Signed-off-by: Biren Roy --- test/common/http/http2/codec_impl_test.cc | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 64acf8ae25bbd..657270b201f26 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -4592,9 +4592,7 @@ TEST_P(Http2CodecImplTest, ChunkProcessingShouldNotScheduleIfReadDisabled) { } TEST_P(Http2CodecImplTest, ServerDispatchLoadShedPointCanCauseServerToSendGoAway) { - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http2_fix_goaway_loadshed_point")) { - GTEST_SKIP(); - } + Runtime::maybeSetRuntimeGuard("envoy.reloadable_features.http2_fix_goaway_loadshed_point", false); initialize(); ASSERT_EQ(0, server_stats_store_.counter("http2.goaway_sent").value()); @@ -4616,9 +4614,7 @@ TEST_P(Http2CodecImplTest, ServerDispatchLoadShedPointCanCauseServerToSendGoAway } TEST_P(Http2CodecImplTest, ServerDispatchLoadShedPointSendGoAwayAndClose) { - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http2_fix_goaway_loadshed_point")) { - GTEST_SKIP(); - } + Runtime::maybeSetRuntimeGuard("envoy.reloadable_features.http2_fix_goaway_loadshed_point", false); expect_buffered_data_on_teardown_ = true; initialize(); @@ -4639,9 +4635,7 @@ TEST_P(Http2CodecImplTest, ServerDispatchLoadShedPointSendGoAwayAndClose) { } TEST_P(Http2CodecImplTest, ServerDispatchLoadShedPointsAreOnlyConsultedOncePerDispatch) { - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http2_fix_goaway_loadshed_point")) { - GTEST_SKIP(); - } + Runtime::maybeSetRuntimeGuard("envoy.reloadable_features.http2_fix_goaway_loadshed_point", false); initialize();