http: Internal redirects with request body

docs/root/intro/arch_overview/http/http_connection_management.rst

@@ -153,7 +153,9 @@ Internal redirects
 
 Envoy supports handling 3xx redirects internally, that is capturing a configurable 3xx redirect
 response, synthesizing a new request, sending it to the upstream specified by the new route match,
-and returning the redirected response as the response to the original request.
+and returning the redirected response as the response to the original request. The headers and body
+of the original request will be sent in the redirect to the new location. Trailers are not yet
+supported.
 
 Internal redirects are configured via the :ref:`internal redirect policy
 <envoy_v3_api_field_config.route.v3.RouteAction.internal_redirect_policy>` field in route configuration.
@@ -162,14 +164,20 @@ When redirect handling is on, any 3xx response from upstream, that matches
 <envoy_v3_api_field_config.route.v3.InternalRedirectPolicy.redirect_response_codes>`
 is subject to the redirect being handled by Envoy.
 
+If Envoy is configured to internally redirect HTTP 303 and receives an HTTP 303 response, it will
+dispatch the redirect with a bodiless HTTP GET if the original request was not a GET or HEAD
+request. Otherwise, Envoy will preserve the original HTTP method. For more information, see `RFC
+7231 Section 6.4.4 <https://tools.ietf.org/html/rfc7231#section-6.4.4>`_.
+
 For a redirect to be handled successfully it must pass the following checks:
 
 1. Have a response code matching one of :ref:`redirect_response_codes
    <envoy_v3_api_field_config.route.v3.InternalRedirectPolicy.redirect_response_codes>`, which is
    either 302 (by default), or a set of 3xx codes (301, 302, 303, 307, 308).
 2. Have a *location* header with a valid, fully qualified URL.
 3. The request must have been fully processed by Envoy.
-4. The request must not have a body.
+4. The request must be smaller than the :ref:`per_request_buffer_limit_bytes
+   <envoy_v3_api_field_config.route.v3.Route.per_request_buffer_limit_bytes>` limit.
 5. :ref:`allow_cross_scheme_redirect
    <envoy_v3_api_field_config.route.v3.InternalRedirectPolicy.allow_cross_scheme_redirect>` is true (default to false),
    or the scheme of the downstream request and the *location* header are the same.

docs/root/version_history/current.rst

@@ -47,6 +47,7 @@ Minor Behavior Changes
   whether receiving `x-envoy-immediate-health-check-fail` will cause exclusion or not. Thus,
   depending on the Envoy deployment, the feature flag may need to be flipped on both downstream
   and upstream instances, depending on the reason.
+* http: added support for internal redirects with bodies. This behavior can be disabled temporarily by setting `envoy.reloadable_features.internal_redirects_with_body` to false.
 * http: allow to use path canonicalizer from `googleurl <https://quiche.googlesource.com/googleurl>`_
   instead of `//source/common/chromium_url`. The new path canonicalizer is enabled by default. To
   revert to the legacy path canonicalizer, enable the runtime flag

source/common/http/conn_manager_impl.cc

@@ -101,7 +101,9 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config,
           overload_state_.getState(Server::OverloadActionNames::get().StopAcceptingRequests)),
       overload_disable_keepalive_ref_(
           overload_state_.getState(Server::OverloadActionNames::get().DisableHttpKeepAlive)),
-      time_source_(time_source) {}
+      time_source_(time_source),
+      enable_internal_redirects_with_body_(Runtime::runtimeFeatureEnabled(
+          "envoy.reloadable_features.internal_redirects_with_body")) {}
 
 const ResponseHeaderMap& ConnectionManagerImpl::continueHeader() {
   static const auto headers = createHeaderMap<ResponseHeaderMapImpl>(
@@ -1613,6 +1615,14 @@ void ConnectionManagerImpl::ActiveStream::recreateStream(
   ResponseEncoder* response_encoder = response_encoder_;
   response_encoder_ = nullptr;
 
+  Buffer::InstancePtr request_data = std::make_unique<Buffer::OwnedImpl>();
+  const auto& buffered_request_data = filter_manager_.bufferedRequestData();
+  const bool proxy_body = connection_manager_.enable_internal_redirects_with_body_ &&
+                          buffered_request_data != nullptr && buffered_request_data->length() > 0;
+  if (proxy_body) {
+    request_data->move(*buffered_request_data);
+  }
+
   response_encoder->getStream().removeCallbacks(*this);
   // This functionally deletes the stream (via deferred delete) so do not
   // reference anything beyond this point.
@@ -1631,7 +1641,13 @@ void ConnectionManagerImpl::ActiveStream::recreateStream(
             filter_state->parent(), StreamInfo::FilterState::LifeSpan::FilterChain);
   }
 
-  new_stream.decodeHeaders(std::move(request_headers_), true);
+  new_stream.decodeHeaders(std::move(request_headers_), !proxy_body);
+  if (proxy_body) {
+    // This functionality is currently only used for internal redirects, which the router only
+    // allows if the full request has been read (end_stream = true) so we don't need to handle the
+    // case of upstream sending an early response mid-request.
+    new_stream.decodeData(*request_data, true);
+  }
 }
 
 Http1StreamEncoderOptionsOptRef ConnectionManagerImpl::ActiveStream::http1StreamEncoderOptions() {

source/common/http/conn_manager_impl.h

@@ -280,6 +280,10 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     Tracing::Config& tracingConfig() override;
     const ScopeTrackedObject& scope() override;
 
+    bool enableInternalRedirectsWithBody() const override {
+      return connection_manager_.enable_internal_redirects_with_body_;
+    }
+
     void traceRequest();
 
     // Updates the snapped_route_config_ (by reselecting scoped route configuration), if a scope is
@@ -445,6 +449,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
   const Server::OverloadActionState& overload_disable_keepalive_ref_;
   TimeSource& time_source_;
   bool remote_close_{};
+  bool enable_internal_redirects_with_body_{};
 };
 
 } // namespace Http

source/common/http/filter_manager.cc

@@ -1374,7 +1374,8 @@ bool ActiveStreamDecoderFilter::recreateStream(const ResponseHeaderMap* headers)
   // Because the filter's and the HCM view of if the stream has a body and if
   // the stream is complete may differ, re-check bytesReceived() to make sure
   // there was no body from the HCM's point of view.
-  if (!complete() || parent_.stream_info_.bytesReceived() != 0) {
+  if (!complete() ||
+      (!parent_.enableInternalRedirectsWithBody() && parent_.stream_info_.bytesReceived() != 0)) {
     return false;
   }
 

source/common/http/filter_manager.h

@@ -563,6 +563,11 @@ class FilterManagerCallbacks {
    * Returns the tracked scope to use for this stream.
    */
   virtual const ScopeTrackedObject& scope() PURE;
+
+  /**
+   * Returns whether internal redirects with request bodies is enabled.
+   */
+  virtual bool enableInternalRedirectsWithBody() const PURE;
 };
 
 /**
@@ -901,6 +906,12 @@ class FilterManager : public ScopeTrackedObject,
 
   uint64_t streamId() const { return stream_id_; }
 
+  Buffer::InstancePtr& bufferedRequestData() { return buffered_request_data_; }
+
+  bool enableInternalRedirectsWithBody() const {
+    return filter_manager_callbacks_.enableInternalRedirectsWithBody();
+  }
+
 private:
   // Indicates which filter to start the iteration with.
   enum class FilterIterationStartState { AlwaysStartFromNext, CanStartFromCurrent };

source/common/router/router.cc

@@ -625,6 +625,9 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
     }
   }
 
+  internal_redirects_with_body_enabled_ =
+      Runtime::runtimeFeatureEnabled("envoy.reloadable_features.internal_redirects_with_body");
+
   ENVOY_STREAM_LOG(debug, "router decoding headers:\n{}", *callbacks_, headers);
 
   // Hang onto the modify_headers function for later use in handling upstream responses.
@@ -687,14 +690,17 @@ Http::FilterDataStatus Filter::decodeData(Buffer::Instance& data, bool end_strea
   // a backoff timer.
   ASSERT(upstream_requests_.size() <= 1);
 
-  bool buffering = (retry_state_ && retry_state_->enabled()) || !active_shadow_policies_.empty();
+  bool buffering = (retry_state_ && retry_state_->enabled()) || !active_shadow_policies_.empty() ||
+                   (internal_redirects_with_body_enabled_ && route_entry_ &&
+                    route_entry_->internalRedirectPolicy().enabled());
   if (buffering &&
       getLength(callbacks_->decodingBuffer()) + data.length() > retry_shadow_buffer_limit_) {
     // The request is larger than we should buffer. Give up on the retry/shadow
     cluster_->stats().retry_or_shadow_abandoned_.inc();
     retry_state_.reset();
     buffering = false;
     active_shadow_policies_.clear();
+    request_buffer_overflowed_ = true;
 
     // If we had to abandon buffering and there's no request in progress, abort the request and
     // clean up. This happens if the initial upstream request failed, and we are currently waiting
@@ -1475,7 +1481,7 @@ bool Filter::setupRedirect(const Http::ResponseHeaderMap& headers,
   // destruction of this filter before the stream is marked as complete, and onDestroy will reset
   // the stream.
   //
-  // Normally when a stream is complete we signal this by resetting the upstream but this cam not
+  // Normally when a stream is complete we signal this by resetting the upstream but this cannot
   // be done in this case because if recreateStream fails, the "failure" path continues to call
   // code in onUpstreamHeaders which requires the upstream *not* be reset. To avoid onDestroy
   // performing a spurious stream reset in the case recreateStream() succeeds, we explicitly track
@@ -1484,11 +1490,14 @@ bool Filter::setupRedirect(const Http::ResponseHeaderMap& headers,
   attempting_internal_redirect_with_complete_stream_ =
       upstream_request.upstreamTiming().last_upstream_rx_byte_received_ && downstream_end_stream_;
 
+  const uint64_t status_code = Http::Utility::getResponseStatus(headers);
+
   // Redirects are not supported for streaming requests yet.
   if (downstream_end_stream_ &&
-      !callbacks_->decodingBuffer() && // Redirects with body not yet supported.
+      ((internal_redirects_with_body_enabled_ && !request_buffer_overflowed_) ||
+       !callbacks_->decodingBuffer()) &&
       location != nullptr &&
-      convertRequestHeadersForInternalRedirect(*downstream_headers_, *location) &&
+      convertRequestHeadersForInternalRedirect(*downstream_headers_, *location, status_code) &&
       callbacks_->recreateStream(&headers)) {
     cluster_->stats().upstream_internal_redirect_succeeded_total_.inc();
     return true;
@@ -1502,7 +1511,8 @@ bool Filter::setupRedirect(const Http::ResponseHeaderMap& headers,
 }
 
 bool Filter::convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream_headers,
-                                                      const Http::HeaderEntry& internal_redirect) {
+                                                      const Http::HeaderEntry& internal_redirect,
+                                                      uint64_t status_code) {
   if (!downstream_headers.Path()) {
     ENVOY_STREAM_LOG(trace, "no path in downstream_headers", *callbacks_);
     return false;
@@ -1581,6 +1591,15 @@ bool Filter::convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& do
     }
   }
 
+  // See https://tools.ietf.org/html/rfc7231#section-6.4.4.
+  if (status_code == enumToInt(Http::Code::SeeOther) &&
+      downstream_headers.getMethodValue() != Http::Headers::get().MethodValues.Get &&
+      downstream_headers.getMethodValue() != Http::Headers::get().MethodValues.Head) {
+    downstream_headers.setMethod(Http::Headers::get().MethodValues.Get);
+    downstream_headers.remove(Http::Headers::get().ContentLength);
+    callbacks_->modifyDecodingBuffer([](Buffer::Instance& data) { data.drain(data.length()); });
+  }
+
   num_internal_redirect.increment();
   restore_original_headers.cancel();
   // Preserve the original request URL for the second pass.

source/common/router/router.h

@@ -283,7 +283,8 @@ class Filter : Logger::Loggable<Logger::Id::router>,
       : config_(config), final_upstream_request_(nullptr),
         downstream_100_continue_headers_encoded_(false), downstream_response_started_(false),
         downstream_end_stream_(false), is_retry_(false),
-        attempting_internal_redirect_with_complete_stream_(false) {}
+        attempting_internal_redirect_with_complete_stream_(false),
+        request_buffer_overflowed_(false) {}
 
   ~Filter() override;
 
@@ -495,7 +496,8 @@ class Filter : Logger::Loggable<Logger::Id::router>,
   void sendNoHealthyUpstreamResponse();
   bool setupRedirect(const Http::ResponseHeaderMap& headers, UpstreamRequest& upstream_request);
   bool convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream_headers,
-                                                const Http::HeaderEntry& internal_redirect);
+                                                const Http::HeaderEntry& internal_redirect,
+                                                uint64_t status_code);
   void updateOutlierDetection(Upstream::Outlier::Result result, UpstreamRequest& upstream_request,
                               absl::optional<uint64_t> code);
   void doRetry();
@@ -539,6 +541,8 @@ class Filter : Logger::Loggable<Logger::Id::router>,
   bool is_retry_ : 1;
   bool include_attempt_count_in_request_ : 1;
   bool attempting_internal_redirect_with_complete_stream_ : 1;
+  bool request_buffer_overflowed_ : 1;
+  bool internal_redirects_with_body_enabled_ : 1;
   uint32_t attempt_count_{1};
   uint32_t pending_retries_{0};
 

source/common/runtime/runtime_features.cc

@@ -78,6 +78,7 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.http_upstream_wait_connect_response",
     "envoy.reloadable_features.http2_skip_encoding_empty_trailers",
     "envoy.reloadable_features.improved_stream_limit_handling",
+    "envoy.reloadable_features.internal_redirects_with_body",
     "envoy.reloadable_features.overload_manager_disable_keepalive_drain_http2",
     "envoy.reloadable_features.prefer_quic_kernel_bpf_packet_routing",
     "envoy.reloadable_features.preserve_downstream_scheme",

test/common/router/router_test.cc

@@ -4045,7 +4045,7 @@ TEST_F(RouterTest, RetryUpstreamGrpcCancelled) {
 
 // Verifies that the initial host is select with max host count of one, but during retries
 // RetryPolicy will be consulted.
-TEST_F(RouterTest, RetryRespsectsMaxHostSelectionCount) {
+TEST_F(RouterTest, RetryRespectsMaxHostSelectionCount) {
   router_.reject_all_hosts_ = true;
 
   NiceMock<Http::MockRequestEncoder> encoder1;
@@ -4259,7 +4259,6 @@ TEST_F(RouterTest, InternalRedirectRejectedWithInvalidLocation) {
 
 TEST_F(RouterTest, InternalRedirectRejectedWithoutCompleteRequest) {
   enableRedirects();
-
   sendRequest(false);
 
   EXPECT_CALL(callbacks_, recreateStream(_)).Times(0);
@@ -4275,7 +4274,6 @@ TEST_F(RouterTest, InternalRedirectRejectedWithoutCompleteRequest) {
 
 TEST_F(RouterTest, InternalRedirectRejectedWithoutLocation) {
   enableRedirects();
-
   sendRequest();
 
   redirect_headers_->removeLocation();
@@ -4290,9 +4288,12 @@ TEST_F(RouterTest, InternalRedirectRejectedWithoutLocation) {
                     .value());
 }
 
-TEST_F(RouterTest, InternalRedirectRejectedWithBody) {
-  enableRedirects();
+TEST_F(RouterTest, InternalRedirectRejectedWithRequestBodyDisabledLegacy) {
+  TestScopedRuntime scoped_runtime;
+  Runtime::LoaderSingleton::getExisting()->mergeValues(
+      {{"envoy.reloadable_features.internal_redirects_with_body", "false"}});
 
+  enableRedirects();
   sendRequest();
 
   Buffer::InstancePtr body_data(new Buffer::OwnedImpl("random_fake_data"));
@@ -4307,14 +4308,43 @@ TEST_F(RouterTest, InternalRedirectRejectedWithBody) {
                     .value());
 }
 
-TEST_F(RouterTest, CrossSchemeRedirectRejectedByPolicy) {
+TEST_F(RouterTest, InternalRedirectAcceptedWithRequestBody) {
   enableRedirects();
+  sendRequest(false);
+
+  EXPECT_CALL(callbacks_.dispatcher_, createTimer_);
+
+  Buffer::InstancePtr body_data(new Buffer::OwnedImpl("random_fake_data"));
+  EXPECT_CALL(callbacks_, addDecodedData(_, true));
+  EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, router_.decodeData(*body_data, true));
+
+  EXPECT_CALL(callbacks_, clearRouteCache());
+  EXPECT_CALL(callbacks_, recreateStream(_)).WillOnce(Return(true));
+
+  response_decoder_->decodeHeaders(std::move(redirect_headers_), false);
+  Buffer::OwnedImpl response_data("1234567890");
+  response_decoder_->decodeData(response_data, false);
+  EXPECT_EQ(0U, cm_.thread_local_cluster_.cluster_.info_->stats_store_
+                    .counter("upstream_internal_redirect_failed_total")
+                    .value());
+  EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_
+                    .counter("upstream_internal_redirect_succeeded_total")
+                    .value());
 
+  // In production, the HCM recreateStream would have called this.
+  router_.onDestroy();
+  EXPECT_EQ(1, callbacks_.streamInfo()
+                   .filterState()
+                   ->getDataMutable<StreamInfo::UInt32Accessor>("num_internal_redirects")
+                   .value());
+}
+
+TEST_F(RouterTest, CrossSchemeRedirectRejectedByPolicy) {
+  enableRedirects();
   sendRequest();
 
   redirect_headers_->setLocation("https://www.foo.com");
 
-  EXPECT_CALL(callbacks_, decodingBuffer());
   EXPECT_CALL(callbacks_, recreateStream(_)).Times(0);
 
   response_decoder_->decodeHeaders(std::move(redirect_headers_), true);
@@ -4326,14 +4356,12 @@ TEST_F(RouterTest, CrossSchemeRedirectRejectedByPolicy) {
 
 TEST_F(RouterTest, InternalRedirectRejectedByPredicate) {
   enableRedirects();
-
   sendRequest();
 
   redirect_headers_->setLocation("http://www.foo.com/some/path");
 
   auto mock_predicate = std::make_shared<NiceMock<MockInternalRedirectPredicate>>();
 
-  EXPECT_CALL(callbacks_, decodingBuffer());
   EXPECT_CALL(callbacks_, clearRouteCache());
   EXPECT_CALL(callbacks_.route_->route_entry_.internal_redirect_policy_, predicates())
       .WillOnce(Return(std::vector<InternalRedirectPredicateSharedPtr>({mock_predicate})));
@@ -4360,7 +4388,6 @@ TEST_F(RouterTest, HttpInternalRedirectSucceeded) {
   default_request_headers_.setForwardedProto("http");
   sendRequest();
 
-  EXPECT_CALL(callbacks_, decodingBuffer());
   EXPECT_CALL(callbacks_, clearRouteCache());
   EXPECT_CALL(callbacks_, recreateStream(_)).WillOnce(Return(true));
   response_decoder_->decodeHeaders(std::move(redirect_headers_), false);
@@ -4385,7 +4412,6 @@ TEST_F(RouterTest, HttpsInternalRedirectSucceeded) {
 
   redirect_headers_->setLocation("https://www.foo.com");
   EXPECT_CALL(connection_, ssl()).WillOnce(Return(ssl_connection));
-  EXPECT_CALL(callbacks_, decodingBuffer());
   EXPECT_CALL(callbacks_, clearRouteCache());
   EXPECT_CALL(callbacks_, recreateStream(_)).WillOnce(Return(true));
   response_decoder_->decodeHeaders(std::move(redirect_headers_), false);
@@ -4405,7 +4431,6 @@ TEST_F(RouterTest, CrossSchemeRedirectAllowedByPolicy) {
 
   redirect_headers_->setLocation("http://www.foo.com");
   EXPECT_CALL(connection_, ssl()).WillOnce(Return(ssl_connection));
-  EXPECT_CALL(callbacks_, decodingBuffer());
   EXPECT_CALL(callbacks_.route_->route_entry_.internal_redirect_policy_,
               isCrossSchemeRedirectAllowed())
       .WillOnce(Return(true));