Merge pull request #147 from envoyproxy/master

router: Add support for per-route configurable maximum number of inte…

api/envoy/api/v2/route/route_components.proto

@@ -518,7 +518,7 @@ message CorsPolicy {
   core.RuntimeFractionalPercent shadow_enabled = 10;
 }
 
-// [#next-free-field: 31]
+// [#next-free-field: 32]
 message RouteAction {
   enum ClusterNotFoundResponseCode {
     // HTTP status code - 503 Service Unavailable.
@@ -875,6 +875,22 @@ message RouteAction {
 
   InternalRedirectAction internal_redirect_action = 26;
 
+  // An internal redirect is handled, iff the number of previous internal redirects that a
+  // downstream request has encountered is lower than this value, and
+  // :ref:`internal_redirect_action <envoy_api_field_route.RouteAction.internal_redirect_action>`
+  // is set to :ref:`HANDLE_INTERNAL_REDIRECT
+  // <envoy_api_enum_value_route.RouteAction.InternalRedirectAction.HANDLE_INTERNAL_REDIRECT>`
+  // In the case where a downstream request is bounced among multiple routes by internal redirect,
+  // the first route that hits this threshold, or has
+  // :ref:`internal_redirect_action <envoy_api_field_route.RouteAction.internal_redirect_action>`
+  // set to
+  // :ref:`PASS_THROUGH_INTERNAL_REDIRECT
+  // <envoy_api_enum_value_route.RouteAction.InternalRedirectAction.PASS_THROUGH_INTERNAL_REDIRECT>`
+  // will pass the redirect back to downstream.
+  //
+  // If not specified, at most one redirect will be followed.
+  google.protobuf.UInt32Value max_internal_redirects = 31;
+
   // Indicates that the route has a hedge policy. Note that if this is set,
   // it'll take precedence over the virtual host level hedge policy entirely
   // (e.g.: policies are not merged, most internal one becomes the enforced policy).

api/envoy/config/route/v3alpha/route_components.proto

@@ -487,7 +487,7 @@ message CorsPolicy {
   core.v3alpha.RuntimeFractionalPercent shadow_enabled = 10;
 }
 
-// [#next-free-field: 31]
+// [#next-free-field: 32]
 message RouteAction {
   option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.route.RouteAction";
 
@@ -853,6 +853,23 @@ message RouteAction {
 
   InternalRedirectAction internal_redirect_action = 26;
 
+  // An internal redirect is handled, iff the number of previous internal redirects that a
+  // downstream request has encountered is lower than this value, and
+  // :ref:`internal_redirect_action
+  // <envoy_api_field_config.route.v3alpha.RouteAction.internal_redirect_action>` is set to
+  // :ref:`HANDLE_INTERNAL_REDIRECT
+  // <envoy_api_enum_value_config.route.v3alpha.RouteAction.InternalRedirectAction.HANDLE_INTERNAL_REDIRECT>`
+  // In the case where a downstream request is bounced among multiple routes by internal redirect,
+  // the first route that hits this threshold, or has
+  // :ref:`internal_redirect_action
+  // <envoy_api_field_config.route.v3alpha.RouteAction.internal_redirect_action>` set to
+  // :ref:`PASS_THROUGH_INTERNAL_REDIRECT
+  // <envoy_api_enum_value_config.route.v3alpha.RouteAction.InternalRedirectAction.PASS_THROUGH_INTERNAL_REDIRECT>`
+  // will pass the redirect back to downstream.
+  //
+  // If not specified, at most one redirect will be followed.
+  google.protobuf.UInt32Value max_internal_redirects = 31;
+
   // Indicates that the route has a hedge policy. Note that if this is set,
   // it'll take precedence over the virtual host level hedge policy entirely
   // (e.g.: policies are not merged, most internal one becomes the enforced policy).

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

@@ -136,8 +136,9 @@ Envoy supports handling 302 redirects internally, that is capturing a 302 redire
 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.
 
-Internal redirects are configured via the ref:`redirect action
-<envoy_api_field_route.RouteAction.redirect_action>` field in
+Internal redirects are configured via the ref:`internal redirect action
+<envoy_api_field_route.RouteAction.internal_redirect_action>` field and
+`max internal redirects <envoy_api_field_route.RouteAction.max_internal_redirects>` field in
 route configuration. When redirect handling is on, any 302 response from upstream is
 subject to the redirect being handled by Envoy.
 
@@ -147,10 +148,21 @@ For a redirect to be handled successfully it must pass the following checks:
 2. Have a *location* header with a valid, fully qualified URL matching the scheme of the original request.
 3. The request must have been fully processed by Envoy.
 4. The request must not have a body.
-5. The request must have not been previously redirected, as determined by the presence of an x-envoy-original-url header.
+5. The number of previously handled internal redirect within a given downstream request does not exceed
+   `max internal redirects <envoy_api_field_route.RouteAction.max_internal_redirects>` of the route
+   that the request or redirected request is hitting.
 
 Any failure will result in redirect being passed downstream instead.
 
+Since a redirected request may be bounced between different routes, any route in the chain of redirects that
+
+1. does not have internal redirect enabled
+2. or has a `max internal redirects
+   <envoy_api_field_route.RouteAction.max_internal_redirects>`
+   smaller or equal to the redirect chain length when the redirect chain hits it
+
+will cause the redirect to be passed downstream.
+
 Once the redirect has passed these checks, the request headers which were shipped to the original
 upstream will be modified by:
 

docs/root/intro/version_history.rst

@@ -39,6 +39,7 @@ Version history
 * router: added support for percentage-based :ref:`retry budgets <envoy_api_field_cluster.CircuitBreakers.Thresholds.retry_budget>`
 * router: allow using a :ref:`query parameter <envoy_api_field_route.RouteAction.HashPolicy.query_parameter>` for HTTP consistent hashing.
 * router: exposed DOWNSTREAM_REMOTE_ADDRESS as custom HTTP request/response headers.
+* router: added support for :ref:`max_internal_redirects <envoy_api_field_route.RouteAction.max_internal_redirects>` for configurable maximum internal redirect hops.
 * router: skip the Location header when the response code is not a 201 or a 3xx.
 * router: added :ref:`auto_sni <envoy_api_field_core.UpstreamHttpProtocolOptions.auto_sni>` to support setting SNI to transport socket for new upstream connections based on the downstream HTTP host/authority header.
 * server: added the :option:`--disable-extensions` CLI option, to disable extensions at startup.

include/envoy/router/router.h

@@ -774,6 +774,12 @@ class RouteEntry : public ResponseEntry {
    */
   virtual InternalRedirectAction internalRedirectAction() const PURE;
 
+  /**
+   * @returns the threshold of number of previously handled internal redirects, for this route to
+   * stop handle internal redirects.
+   */
+  virtual uint32_t maxInternalRedirects() const PURE;
+
   /**
    * @return std::string& the name of the route.
    */

include/envoy/stream_info/BUILD

@@ -32,3 +32,11 @@ envoy_cc_library(
     external_deps = ["abseil_optional"],
     deps = ["//source/common/protobuf"],
 )
+
+envoy_cc_library(
+    name = "uint32_accessor_interface",
+    hdrs = ["uint32_accessor.h"],
+    deps = [
+        ":filter_state_interface",
+    ],
+)

include/envoy/stream_info/uint32_accessor.h

@@ -0,0 +1,26 @@
+#pragma once
+
+#include "envoy/common/pure.h"
+#include "envoy/stream_info/filter_state.h"
+
+namespace Envoy {
+namespace StreamInfo {
+
+/**
+ * A FilterState object that tracks a single uint32_t value.
+ */
+class UInt32Accessor : public FilterState::Object {
+public:
+  /**
+   * Increments the tracked value by 1.
+   */
+  virtual void increment() PURE;
+
+  /**
+   * @return the tracked value.
+   */
+  virtual uint32_t value() const PURE;
+};
+
+} // namespace StreamInfo
+} // namespace Envoy

source/common/http/async_client_impl.h

@@ -272,6 +272,7 @@ class AsyncStreamImpl : public AsyncClient::Stream,
     Router::InternalRedirectAction internalRedirectAction() const override {
       return Router::InternalRedirectAction::PassThrough;
     }
+    uint32_t maxInternalRedirects() const override { return 1; }
     const std::string& routeName() const override { return route_name_; }
     std::unique_ptr<const HashPolicyImpl> hash_policy_;
     static const NullHedgePolicy hedge_policy_;

source/common/router/BUILD

@@ -291,6 +291,7 @@ envoy_cc_library(
         "//source/common/network:application_protocol_lib",
         "//source/common/network:transport_socket_options_lib",
         "//source/common/stream_info:stream_info_lib",
+        "//source/common/stream_info:uint32_accessor_lib",
         "//source/common/tracing:http_tracer_lib",
         "//source/common/upstream:load_balancer_lib",
         "@envoy_api//envoy/extensions/filters/http/router/v3alpha:pkg_cc_proto",

source/common/router/config_impl.cc

@@ -286,7 +286,9 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost,
                           route.hidden_envoy_deprecated_per_filter_config(), factory_context,
                           validator),
       route_name_(route.name()), time_source_(factory_context.dispatcher().timeSource()),
-      internal_redirect_action_(convertInternalRedirectAction(route.route())) {
+      internal_redirect_action_(convertInternalRedirectAction(route.route())),
+      max_internal_redirects_(
+          PROTOBUF_GET_WRAPPED_OR_DEFAULT(route.route(), max_internal_redirects, 1)) {
   if (route.route().has_metadata_match()) {
     const auto filter_it = route.route().metadata_match().filter_metadata().find(
         Envoy::Config::MetadataFilters::get().ENVOY_LB);

source/common/router/config_impl.h

@@ -450,6 +450,7 @@ class RouteEntryImplBase : public RouteEntry,
   InternalRedirectAction internalRedirectAction() const override {
     return internal_redirect_action_;
   }
+  uint32_t maxInternalRedirects() const override { return max_internal_redirects_; }
 
   // Router::DirectResponseEntry
   std::string newPath(const Http::HeaderMap& headers) const override;
@@ -566,6 +567,7 @@ class RouteEntryImplBase : public RouteEntry,
     InternalRedirectAction internalRedirectAction() const override {
       return parent_->internalRedirectAction();
     }
+    uint32_t maxInternalRedirects() const override { return parent_->maxInternalRedirects(); }
 
     // Router::Route
     const DirectResponseEntry* directResponseEntry() const override { return nullptr; }
@@ -711,6 +713,7 @@ class RouteEntryImplBase : public RouteEntry,
   const std::string route_name_;
   TimeSource& time_source_;
   InternalRedirectAction internal_redirect_action_;
+  uint32_t max_internal_redirects_{1};
 };
 
 /**

source/common/router/router.cc

@@ -32,13 +32,16 @@
 #include "common/router/debug_config.h"
 #include "common/router/retry_state_impl.h"
 #include "common/runtime/runtime_impl.h"
+#include "common/stream_info/uint32_accessor_impl.h"
 #include "common/tracing/http_tracer_impl.h"
 
 #include "extensions/filters/http/well_known_names.h"
 
 namespace Envoy {
 namespace Router {
 namespace {
+constexpr char NumInternalRedirectsFilterStateName[] = "num_internal_redirects";
+
 uint32_t getLength(const Buffer::Instance* instance) { return instance ? instance->length() : 0; }
 
 bool schemeIsHttp(const Http::HeaderMap& downstream_headers,
@@ -55,12 +58,10 @@ bool schemeIsHttp(const Http::HeaderMap& downstream_headers,
 }
 
 bool convertRequestHeadersForInternalRedirect(Http::HeaderMap& downstream_headers,
+                                              StreamInfo::FilterState& filter_state,
+                                              uint32_t max_internal_redirects,
                                               const Http::HeaderEntry& internal_redirect,
                                               const Network::Connection& connection) {
-  // Envoy does not currently support multiple rounds of redirects.
-  if (downstream_headers.EnvoyOriginalUrl()) {
-    return false;
-  }
   // Make sure the redirect response contains a URL to redirect to.
   if (internal_redirect.value().getStringView().length() == 0) {
     return false;
@@ -71,12 +72,28 @@ bool convertRequestHeadersForInternalRedirect(Http::HeaderMap& downstream_header
     return false;
   }
 
+  // Don't allow serving TLS responses over plaintext.
   bool scheme_is_http = schemeIsHttp(downstream_headers, connection);
   if (scheme_is_http && absolute_url.scheme() == Http::Headers::get().SchemeValues.Https) {
-    // Don't allow serving TLS responses over plaintext.
     return false;
   }
 
+  // Make sure that performing the redirect won't result in exceeding the configured number of
+  // redirects allowed for this route.
+  if (!filter_state.hasData<StreamInfo::UInt32Accessor>(NumInternalRedirectsFilterStateName)) {
+    filter_state.setData(NumInternalRedirectsFilterStateName,
+                         std::make_shared<StreamInfo::UInt32AccessorImpl>(0),
+                         StreamInfo::FilterState::StateType::Mutable,
+                         StreamInfo::FilterState::LifeSpan::DownstreamRequest);
+  }
+  StreamInfo::UInt32Accessor& num_internal_redirect =
+      filter_state.getDataMutable<StreamInfo::UInt32Accessor>(NumInternalRedirectsFilterStateName);
+
+  if (num_internal_redirect.value() >= max_internal_redirects) {
+    return false;
+  }
+  num_internal_redirect.increment();
+
   // Preserve the original request URL for the second pass.
   downstream_headers.setEnvoyOriginalUrl(
       absl::StrCat(scheme_is_http ? Http::Headers::get().SchemeValues.Http
@@ -1359,11 +1376,14 @@ bool Filter::setupRedirect(const Http::HeaderMap& headers, UpstreamRequest& upst
   attempting_internal_redirect_with_complete_stream_ =
       upstream_request.upstream_timing_.last_upstream_rx_byte_received_ && downstream_end_stream_;
 
+  StreamInfo::FilterState& filter_state = callbacks_->streamInfo().filterState();
+
   // As with setupRetry, redirects are not supported for streaming requests yet.
   if (downstream_end_stream_ &&
       !callbacks_->decodingBuffer() && // Redirects with body not yet supported.
       location != nullptr &&
-      convertRequestHeadersForInternalRedirect(*downstream_headers_, *location,
+      convertRequestHeadersForInternalRedirect(*downstream_headers_, filter_state,
+                                               route_entry_->maxInternalRedirects(), *location,
                                                *callbacks_->connection()) &&
       callbacks_->recreateStream()) {
     cluster_->stats().upstream_internal_redirect_succeeded_total_.inc();

source/common/stream_info/BUILD

@@ -39,3 +39,11 @@ envoy_cc_library(
         "//include/envoy/stream_info:stream_info_interface",
     ],
 )
+
+envoy_cc_library(
+    name = "uint32_accessor_lib",
+    hdrs = ["uint32_accessor_impl.h"],
+    deps = [
+        "//include/envoy/stream_info:uint32_accessor_interface",
+    ],
+)

source/common/stream_info/uint32_accessor_impl.h

@@ -0,0 +1,31 @@
+#pragma once
+
+#include "envoy/stream_info/uint32_accessor.h"
+
+namespace Envoy {
+namespace StreamInfo {
+
+/*
+ * A FilterState object that tracks a single uint32_t value.
+ */
+class UInt32AccessorImpl : public UInt32Accessor {
+public:
+  UInt32AccessorImpl(uint32_t value) : value_(value) {}
+
+  // From FilterState::Object
+  ProtobufTypes::MessagePtr serializeAsProto() const override {
+    auto message = std::make_unique<ProtobufWkt::UInt32Value>();
+    message->set_value(value_);
+    return message;
+  }
+
+  // From UInt32Accessor.
+  void increment() override { value_++; }
+  uint32_t value() const override { return value_; }
+
+private:
+  uint32_t value_;
+};
+
+} // namespace StreamInfo
+} // namespace Envoy