Added runtime feature.

birenroy · birenroy · commit a07f2f243d11 · 2026-05-14T18:17:18.000-04:00
Signed-off-by: Biren Roy &lt;birenroy@google.com&gt;
diff --git 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
@@ -99,7 +99,7 @@ ServerConnectionPtr ConnectionManagerUtility::autoCreateCodec(
     Http2::CodecStats& stats = Http2::CodecStats::atomicGet(http2_codec_stats, scope);
     return std::make_unique<Http2::ServerConnectionImpl>(
         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<Http1::ServerConnectionImpl>(
diff --git 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<Http2Visitor>(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
@@ -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
@@ -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
@@ -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<QuicHttpServerConnectionFactory>(
diff --git 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<Http2::ServerConnectionImpl>(
         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<Http1::ServerConnectionImpl>(
diff --git 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
@@ -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([&times_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([&times_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<RequestEncoder*> request_encoders;
+  std::vector<ResponseEncoder*> 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
@@ -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
@@ -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<Network::MockConnection> mock_server_connection_;
   NiceMock<MockServerConnectionCallbacks> mock_server_callbacks_;
   NiceMock<Random::MockRandomGenerator> random_;
+  NiceMock<Server::MockOverloadManager> overload_manager_;
 
   ServerConnectionPtr server_;
 };
diff --git 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<http2::adapter::Http2Setting, 1> settings;
@@ -418,7 +419,8 @@ FakeHttpConnection::FakeHttpConnection(
     Http::Http2::CodecStats& stats = fake_upstream.http2CodecStats();
     codec_ = std::make_unique<TestHttp2ServerConnectionImpl>(
         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
@@ -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(
