router: fix a bug where internal redirect will hang up request or unexpected redirect (#44154) (#44304)

Commit Message: router: fix a bug where internal redirect will hang up
request or unexpected redirect (#44154)
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API
Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

Signed-off-by: wbpcode <wbphub@gmail.com>

Author: wbpcode

changelogs/current.yaml

@@ -17,6 +17,10 @@ bug_fixes:
     Fixed a bug where dynamic module filter may result in a incomplete body being sent to upstream
     or downstream when some filters before or after the dynamic module filter in the chain
     buffered the body and the dynamic module filter did not.
+- area: http
+  change: |
+    Fixed a bug where the internal redirect logic may hang up a request when the request buffer is
+    overflowed.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

source/common/router/router.cc

@@ -992,38 +992,34 @@ Http::FilterDataStatus Filter::decodeData(Buffer::Instance& data, bool end_strea
 
   const bool retry_enabled = retry_state_ && retry_state_->enabled();
   const bool redirect_enabled = route_entry_ && route_entry_->internalRedirectPolicy().enabled();
-  const bool is_redirect_only = redirect_enabled && !retry_enabled;
   const uint64_t effective_buffer_limit = calculateEffectiveBufferLimit();
 
-  bool buffering = retry_enabled || redirect_enabled;
+  bool buffering = (retry_enabled || redirect_enabled) && (!request_buffer_overflowed_);
 
   // Check if we would exceed buffer limits, regardless of current buffering state
   // This ensures error details are set even if retry state was cleared due to upstream reset.
   const bool would_exceed_buffer =
       (getLength(callbacks_->decodingBuffer()) + data.length() > effective_buffer_limit);
 
-  // Handle retry buffer overflow, excluding redirect-only scenarios.
-  // For redirect scenarios, buffer overflow should only affect redirect processing, not initial
-  // request.
-  if (would_exceed_buffer && retry_enabled && !is_redirect_only && !request_buffer_overflowed_) {
+  // Handle buffer overflow.
+  if (buffering && would_exceed_buffer) {
     ENVOY_LOG(debug,
               "The request payload has at least {} bytes data which exceeds buffer limit {}. "
               "Giving up on buffering.",
               getLength(callbacks_->decodingBuffer()) + data.length(), effective_buffer_limit);
-
     cluster_->trafficStats()->retry_or_shadow_abandoned_.inc();
     retry_state_.reset();
-    ENVOY_LOG(debug, "retry or shadow overflow: retry_state_ reset, buffering set to false");
+    ENVOY_LOG(debug, "retry or redirect buffer overflow: skipping buffering");
     buffering = false;
     active_shadow_policies_.clear();
+    request_buffer_overflowed_ = true;
 
     // Only send local reply and cleanup if we're in a retry waiting state (no active upstream
     // requests). If there are active upstream requests, let the normal upstream failure handling
     // take precedence.
     if (upstream_requests_.empty()) {
-      request_buffer_overflowed_ = true;
-      ENVOY_LOG(debug,
-                "retry or shadow overflow: No upstream requests, resetting and calling cleanup()");
+      ENVOY_LOG(debug, "retry or redirect buffer overflow: No upstream requests, resetting and "
+                       "calling cleanup()");
       resetAll();
       cleanup();
       callbacks_->streamInfo().setResponseCodeDetails(
@@ -1034,25 +1030,12 @@ Http::FilterDataStatus Filter::decodeData(Buffer::Instance& data, bool end_strea
           StreamInfo::ResponseCodeDetails::get().RequestPayloadExceededRetryBufferLimit);
       return Http::FilterDataStatus::StopIterationNoBuffer;
     } else {
-      ENVOY_LOG(debug, "retry or shadow overflow: Upstream requests exist, deferring to normal "
-                       "upstream failure handling");
+      ENVOY_LOG(debug,
+                "retry or redirect buffer overflow: Upstream requests exist, deferring to normal "
+                "upstream failure handling");
     }
   }
 
-  // Handle redirect-only buffer overflow when retry/shadow is not active.
-  // For redirect scenarios, buffer overflow should only affect redirect processing, not initial
-  // request.
-  if (would_exceed_buffer && is_redirect_only && !request_buffer_overflowed_) {
-    ENVOY_LOG(debug,
-              "The request payload has at least {} bytes data which exceeds buffer limit {}. "
-              "Marking request as buffer overflowed to cancel internal redirects.",
-              getLength(callbacks_->decodingBuffer()) + data.length(), effective_buffer_limit);
-
-    // Set the flag to cancel internal redirect processing, but allow the request to proceed
-    // normally.
-    request_buffer_overflowed_ = true;
-  }
-
   for (auto* shadow_stream : shadow_streams_) {
     if (end_stream) {
       shadow_stream->removeDestructorCallback();
@@ -1444,13 +1427,6 @@ void Filter::onUpstreamAbort(Http::Code code, StreamInfo::CoreResponseFlag respo
   // Otherwise just reset the ongoing response.
   callbacks_->streamInfo().setResponseFlag(response_flags);
 
-  // Check if buffer overflow occurred and override error details accordingly
-  if (request_buffer_overflowed_) {
-    code = Http::Code::InsufficientStorage;
-    body = "exceeded request buffer limit while retrying upstream";
-    details = StreamInfo::ResponseCodeDetails::get().RequestPayloadExceededRetryBufferLimit;
-  }
-
   // This will destroy any created retry timers.
   cleanup();
   // sendLocalReply may instead reset the stream if downstream_response_started_ is true.
@@ -2008,8 +1984,7 @@ bool Filter::setupRedirect(const Http::ResponseHeaderMap& headers) {
   const uint64_t status_code = Http::Utility::getResponseStatus(headers);
 
   // Redirects are not supported for streaming requests yet.
-  if (downstream_end_stream_ && (!request_buffer_overflowed_ || !callbacks_->decodingBuffer()) &&
-      location != nullptr &&
+  if (downstream_end_stream_ && (!request_buffer_overflowed_) && location != nullptr &&
       convertRequestHeadersForInternalRedirect(*downstream_headers_, headers, *location,
                                                status_code) &&
       callbacks_->recreateStream(&headers)) {

test/common/router/router_test.cc

@@ -3240,45 +3240,6 @@ TEST_F(RouterTest, RetryRequestDuringBodyBufferLimitExceeded) {
   EXPECT_CALL(callbacks_, decodingBuffer()).WillRepeatedly(Return(&decoding_buffer));
   EXPECT_CALL(callbacks_, addDecodedData(_, true))
       .WillRepeatedly(Invoke([&](Buffer::Instance& data, bool) { decoding_buffer.move(data); }));
-  EXPECT_CALL(callbacks_.route_->route_entry_, requestBodyBufferLimit()).WillOnce(Return(10));
-
-  NiceMock<Http::MockRequestEncoder> encoder1;
-  Http::ResponseDecoder* response_decoder = nullptr;
-  expectNewStreamWithImmediateEncoder(encoder1, &response_decoder, Http::Protocol::Http10);
-
-  Http::TestRequestHeaderMapImpl headers{
-      {"x-envoy-retry-on", "5xx"}, {"x-envoy-internal", "true"}, {"myheader", "present"}};
-  HttpTestUtility::addDefaultHeaders(headers);
-  router_->decodeHeaders(headers, false);
-  const std::string body1("body1");
-  Buffer::OwnedImpl buf1(body1);
-  EXPECT_CALL(*router_->retry_state_, enabled()).Times(2).WillRepeatedly(Return(true));
-  router_->decodeData(buf1, false);
-
-  router_->retry_state_->expectResetRetry();
-  encoder1.stream_.resetStream(Http::StreamResetReason::RemoteReset);
-
-  // Send additional 15 bytes - total 55 bytes, which should exceed request body buffer limit (50).
-  const std::string body2(15, 'y');
-  Buffer::OwnedImpl buf2(body2);
-  router_->decodeData(buf2, false);
-
-  EXPECT_EQ(callbacks_.details(), "request_payload_exceeded_retry_buffer_limit");
-  EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_
-                    .counter("retry_or_shadow_abandoned")
-                    .value());
-  EXPECT_TRUE(verifyHostUpstreamStats(0, 1));
-}
-
-// Test that router uses request_body_buffer_limit when configured instead of
-// per_request_buffer_limit.
-TEST_F(RouterTest, RequestBodyBufferLimitExceeded) {
-  Buffer::OwnedImpl decoding_buffer;
-  EXPECT_CALL(callbacks_, decodingBuffer()).WillRepeatedly(Return(&decoding_buffer));
-  EXPECT_CALL(callbacks_, addDecodedData(_, true))
-      .WillRepeatedly(Invoke([&](Buffer::Instance& data, bool) { decoding_buffer.move(data); }));
-  // Configure a large request body buffer limit (50 bytes) but small request buffer limit (10
-  // bytes).
   EXPECT_CALL(callbacks_.route_->route_entry_, requestBodyBufferLimit()).WillOnce(Return(50));
 
   NiceMock<Http::MockRequestEncoder> encoder1;
@@ -3290,7 +3251,7 @@ TEST_F(RouterTest, RequestBodyBufferLimitExceeded) {
   HttpTestUtility::addDefaultHeaders(headers);
   router_->decodeHeaders(headers, false);
 
-  // Send 40 bytes - should be within request body buffer limit (50) but exceeds retry limit (10).
+  // Send 40 bytes - should be within request body buffer limit (50).
   const std::string body1(40, 'x');
   Buffer::OwnedImpl buf1(body1);
   EXPECT_CALL(*router_->retry_state_, enabled()).Times(2).WillRepeatedly(Return(true));
@@ -3319,8 +3280,7 @@ TEST_F(RouterTest, BufferLimitLogicCase1RequestBodyBufferLimitSet) {
   EXPECT_CALL(callbacks_, addDecodedData(_, true))
       .WillRepeatedly(Invoke([&](Buffer::Instance& data, bool) { decoding_buffer.move(data); }));
 
-  // Case 1: request_body_buffer_limit=60, per_request_buffer_limit_bytes=20
-  // Should use request_body_buffer_limit = 60
+  // Use route level buffer limit.
   EXPECT_CALL(callbacks_.route_->route_entry_, requestBodyBufferLimit()).WillRepeatedly(Return(60));
 
   NiceMock<Http::MockRequestEncoder> encoder1;
@@ -3353,8 +3313,7 @@ TEST_F(RouterTest, BufferLimitLogicCase1RequestBodyBufferLimitSet) {
   EXPECT_TRUE(verifyHostUpstreamStats(0, 1));
 }
 
-// When per_request_buffer_limit_bytes is set but request_body_buffer_limit is not set,
-// we should use min(per_request_buffer_limit_bytes, per_connection_buffer_limit_bytes).
+// Route level request buffer limit will override the connection buffer limit.
 TEST_F(RouterTest, BufferLimitLogicCase2PerRequestSetRequestBodyNotSet) {
   Buffer::OwnedImpl decoding_buffer;
   EXPECT_CALL(callbacks_, decodingBuffer()).WillRepeatedly(Return(&decoding_buffer));
@@ -3363,8 +3322,8 @@ TEST_F(RouterTest, BufferLimitLogicCase2PerRequestSetRequestBodyNotSet) {
   // Set up the connection buffer limit mock to return 40 as expected
   EXPECT_CALL(callbacks_, bufferLimit()).WillRepeatedly(Return(40));
 
-  // Case 2: per_request_buffer_limit_bytes=20, request_body_buffer_limit=not set
-  // Should use min(20, connection_buffer_limit) = 20 (since connection limit is default 40)
+  // Route level request buffer limit is set to 20, which should override connection buffer limit
+  // of 40.
   EXPECT_CALL(callbacks_.route_->route_entry_, requestBodyBufferLimit()).WillRepeatedly(Return(20));
 
   NiceMock<Http::MockRequestEncoder> encoder1;
@@ -3397,51 +3356,6 @@ TEST_F(RouterTest, BufferLimitLogicCase2PerRequestSetRequestBodyNotSet) {
   EXPECT_TRUE(verifyHostUpstreamStats(0, 1));
 }
 
-// Test that when connection limit is smaller than per_request limit,
-// we use min(per_request_buffer_limit_bytes, per_connection_buffer_limit_bytes) = connection limit.
-TEST_F(RouterTest, BufferLimitLogicCase2ConnectionLimitSmaller) {
-  Buffer::OwnedImpl decoding_buffer;
-  EXPECT_CALL(callbacks_, decodingBuffer()).WillRepeatedly(Return(&decoding_buffer));
-  EXPECT_CALL(callbacks_, addDecodedData(_, true))
-      .WillRepeatedly(Invoke([&](Buffer::Instance& data, bool) { decoding_buffer.move(data); }));
-  // Set up the connection buffer limit mock to return 40 as expected
-  EXPECT_CALL(callbacks_, bufferLimit()).WillRepeatedly(Return(40));
-
-  // Case 2: per_request_buffer_limit_bytes=50, request_body_buffer_limit=not set
-  // Should use min(50, connection_limit) = min(50, 40) = 40
-  // With consolidated approach, the effective limit should be 40
-  EXPECT_CALL(callbacks_.route_->route_entry_, requestBodyBufferLimit()).WillRepeatedly(Return(40));
-
-  NiceMock<Http::MockRequestEncoder> encoder1;
-  Http::ResponseDecoder* response_decoder = nullptr;
-  expectNewStreamWithImmediateEncoder(encoder1, &response_decoder, Http::Protocol::Http10);
-
-  Http::TestRequestHeaderMapImpl headers{{"x-envoy-retry-on", "5xx"}, {"x-envoy-internal", "true"}};
-  HttpTestUtility::addDefaultHeaders(headers);
-  router_->decodeHeaders(headers, false);
-
-  // Send initial data (35 bytes)
-  const std::string body1(35, 'x');
-  Buffer::OwnedImpl buf1(body1);
-  EXPECT_CALL(*router_->retry_state_, enabled()).Times(2).WillRepeatedly(Return(true));
-  router_->decodeData(buf1, false);
-
-  // Simulate upstream failure to trigger retry logic
-  router_->retry_state_->expectResetRetry();
-  encoder1.stream_.resetStream(Http::StreamResetReason::RemoteReset);
-
-  // Send additional data (10 bytes) - total 45 bytes should exceed connection limit of 40
-  const std::string body2(10, 'y');
-  Buffer::OwnedImpl buf2(body2);
-  router_->decodeData(buf2, false);
-
-  EXPECT_EQ(callbacks_.details(), "request_payload_exceeded_retry_buffer_limit");
-  EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_
-                    .counter("retry_or_shadow_abandoned")
-                    .value());
-  EXPECT_TRUE(verifyHostUpstreamStats(0, 1));
-}
-
 // Test that when neither fields are set we use per_connection_buffer_limit_bytes.
 TEST_F(RouterTest, BufferLimitLogicCase3NeitherFieldSet) {
   Buffer::OwnedImpl decoding_buffer;
@@ -4097,9 +4011,54 @@ TEST_F(RouterTest, NoRetryWithBodyLimit) {
   EXPECT_CALL(callbacks_, addDecodedData(_, _)).Times(0);
   Buffer::OwnedImpl body("t");
   router_->decodeData(body, false);
+  Buffer::OwnedImpl body2("t");
+  // Ensure subsequent chunks after overflow are also not buffered.
+  router_->decodeData(body2, false);
   EXPECT_EQ(1U,
             callbacks_.route_->virtual_host_->virtual_cluster_.stats().upstream_rq_total_.value());
 
+  EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_
+                    .counter("retry_or_shadow_abandoned")
+                    .value());
+
+  Http::ResponseHeaderMapPtr response_headers(
+      new Http::TestResponseHeaderMapImpl{{":status", "200"}});
+  response_decoder->decodeHeaders(std::move(response_headers), true);
+}
+
+TEST_F(RouterTest, EnableRedirectAndRetryButNoRetryWithBodyLimit) {
+  TestScopedRuntime scoped_runtime;
+  scoped_runtime.mergeValues(
+      {{"envoy.reloadable_features.allow_multiplexed_upstream_half_close", "false"}});
+
+  recreateFilter();
+  NiceMock<Http::MockRequestEncoder> encoder1;
+  Http::ResponseDecoder* response_decoder = nullptr;
+  expectNewStreamWithImmediateEncoder(encoder1, &response_decoder, Http::Protocol::Http10);
+
+  // Enable redirects also. This feature will not be used but will affect buffer logic.
+  enableRedirects(3);
+
+  // Set a per route body limit which disallows any buffering.
+  EXPECT_CALL(callbacks_.route_->route_entry_, requestBodyBufferLimit()).WillOnce(Return(0));
+  Http::TestRequestHeaderMapImpl headers{{"x-envoy-retry-on", "5xx"}, {"x-envoy-internal", "true"}};
+  HttpTestUtility::addDefaultHeaders(headers);
+  router_->decodeHeaders(headers, false);
+  // Unlike RetryUpstreamReset above the data won't be buffered as the body exceeds the buffer limit
+  EXPECT_CALL(*router_->retry_state_, enabled()).WillOnce(Return(true));
+  EXPECT_CALL(callbacks_, addDecodedData(_, _)).Times(0);
+  Buffer::OwnedImpl body("t");
+  router_->decodeData(body, false);
+  Buffer::OwnedImpl body2("t");
+  // Ensure subsequent chunks after overflow are also not buffered.
+  router_->decodeData(body2, false);
+  EXPECT_EQ(1U,
+            callbacks_.route_->virtual_host_->virtual_cluster_.stats().upstream_rq_total_.value());
+
+  EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_
+                    .counter("retry_or_shadow_abandoned")
+                    .value());
+
   Http::ResponseHeaderMapPtr response_headers(
       new Http::TestResponseHeaderMapImpl{{":status", "200"}});
   response_decoder->decodeHeaders(std::move(response_headers), true);
@@ -5067,6 +5026,41 @@ TEST_F(RouterTest, InternalRedirectAcceptedWithRequestBody) {
                    ->value());
 }
 
+TEST_F(RouterTest, InternalRedirectWithRequestBodyBufferOverflow) {
+  EXPECT_CALL(callbacks_.route_->route_entry_, requestBodyBufferLimit()).WillOnce(Return(10));
+
+  enableRedirects();
+  sendRequest(false);
+
+  EXPECT_CALL(callbacks_.dispatcher_, createTimer_);
+
+  Buffer::InstancePtr body_data(new Buffer::OwnedImpl("random_fake_data"));
+  // We will not buffer the body data because it exceeds the buffer limit. But note the initial
+  // request is still valid.
+  EXPECT_CALL(callbacks_, addDecodedData(_, _)).Times(0);
+  EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, router_->decodeData(*body_data, true));
+
+  // No redirect because buffer overflowed.
+  EXPECT_CALL(callbacks_.route_->route_entry_.internal_redirect_policy_, responseHeadersToCopy())
+      .Times(0);
+  EXPECT_CALL(callbacks_.downstream_callbacks_, clearRouteCache()).Times(0);
+  EXPECT_CALL(callbacks_, recreateStream(_)).Times(0);
+
+  response_decoder_->decodeHeaders(std::move(redirect_headers_), false);
+  Buffer::OwnedImpl response_data("1234567890");
+  response_decoder_->decodeData(response_data, false);
+
+  router_->onDestroy();
+  EXPECT_FALSE(callbacks_.streamInfo().filterState()->hasDataWithName("num_internal_redirects"));
+
+  EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_
+                    .counter("retry_or_shadow_abandoned")
+                    .value());
+  EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_
+                    .counter("upstream_internal_redirect_failed_total")
+                    .value());
+}
+
 TEST_F(RouterTest, CrossSchemeRedirectRejectedByPolicy) {
   enableRedirects();
   sendRequest();