From 0f0916b0bb73eab36786ddb431814c42e2394e0c Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Mon, 20 Jul 2020 19:37:07 +1000 Subject: [PATCH 01/35] router: add new ratelimited retry backoff strategy Adds new retry back off strategy that uses feedback from response headers like Retry-After or X-RateLimit-Reset to decide the back off interval before retrying. Signed-off-by: Martin Matusiak --- .../config/route/v3/route_components.proto | 32 ++- .../route/v4alpha/route_components.proto | 35 ++- .../http/http_filters/router_filter.rst | 14 ++ .../cluster_manager/cluster_stats.rst | 2 + docs/root/version_history/current.rst | 4 + .../config/route/v3/route_components.proto | 32 ++- .../route/v4alpha/route_components.proto | 35 ++- include/envoy/http/header_map.h | 12 ++ include/envoy/router/router.h | 33 +++ include/envoy/upstream/upstream.h | 2 + source/common/common/backoff_strategy.cc | 17 +- source/common/common/backoff_strategy.h | 28 ++- source/common/config/datasource.cc | 4 +- source/common/config/grpc_stream.h | 4 +- source/common/config/utility.h | 3 +- source/common/http/async_client_impl.h | 7 + source/common/http/header_utility.h | 7 + source/common/http/headers.h | 4 + source/common/router/config_impl.cc | 11 + source/common/router/config_impl.h | 8 + source/common/router/retry_state_impl.cc | 125 ++++++++++- source/common/router/retry_state_impl.h | 20 +- source/common/router/router.cc | 20 +- source/common/router/router.h | 13 +- .../upstream/health_discovery_service.cc | 4 +- test/common/common/backoff_strategy_test.cc | 39 +++- test/common/config/utility_test.cc | 2 +- test/common/http/header_utility_test.cc | 31 +++ test/common/router/config_impl_test.cc | 82 +++++++ test/common/router/retry_state_impl_test.cc | 204 +++++++++++++++++- test/common/router/router_test.cc | 2 +- .../common/router/router_upstream_log_test.cc | 2 +- .../dns_cache_impl_test.cc | 2 +- test/mocks/router/mocks.h | 13 ++ 34 files changed, 789 insertions(+), 64 deletions(-) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index c35e210691c5..49df7fd13593 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1033,7 +1033,7 @@ message RouteAction { } // HTTP retry :ref:`architecture overview `. -// [#next-free-field: 11] +// [#next-free-field: 12] message RetryPolicy { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.route.RetryPolicy"; @@ -1087,6 +1087,25 @@ message RetryPolicy { google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}]; } + message RateLimitedRetryBackOff { + // Specifies the rate limited retry reset headers (like ``Retry-After`` or + // ``X-RateLimit-Reset``) to match against the response. If a request is + // being retried and the response contains one of these headers the value + // of the header will be used as the retry back off interval, with jitter added. + // + // The value of the header must be either: + // 1. An interval in seconds as an integer, eg. ``120``. + // 2. A unix timestamp as an integer, eg. ``1595320702``. The timestamp + // must be in the future relative to the current system time. + repeated HeaderMatcher reset_headers = 1; + + // Specifies the maximum rate limited retry back off interval that Envoy + // will allow. If a rate limited reset header contains an interval longer + // than this then the default exponential back off strategy will be used + // instead. + google.protobuf.Duration reset_max_interval = 2 [(validate.rules).duration = {gt {}}]; + } + // Specifies the conditions under which retry takes place. These are the same // conditions documented for :ref:`config_http_filters_router_x-envoy-retry-on` and // :ref:`config_http_filters_router_x-envoy-retry-grpc-on`. @@ -1130,13 +1149,22 @@ message RetryPolicy { // HTTP status codes that should trigger a retry in addition to those specified by retry_on. repeated uint32 retriable_status_codes = 7; - // Specifies parameters that control retry back off. This parameter is optional, in which case the + // Specifies parameters that control exponential retry back off. This parameter is optional, in which case the // default base interval is 25 milliseconds or, if set, the current value of the // `upstream.base_retry_backoff_ms` runtime parameter. The default maximum interval is 10 times // the base interval. The documentation for :ref:`config_http_filters_router_x-envoy-max-retries` // describes Envoy's back-off algorithm. RetryBackOff retry_back_off = 8; + // Specifies parameters that control a retry back off based on rate limited + // responses. When a request is rate limited by the server the server may + // include a header like ``Retry-After`` or ``X-RateLimit-Reset`` to provide + // feedback to the client on how long to wait before retrying. If configured, + // the rate limited retry back off strategy will be used instead of the + // default exponential back off strategy (configured using `retry_back_off`) + // whenever a response includes the matching headers. + RateLimitedRetryBackOff rate_limited_retry_back_off = 11; + // HTTP response headers that trigger a retry if present in the response. A retry will be // triggered if any of the header matches match the upstream response headers. // The field is only consulted if 'retriable-headers' retry policy is active. diff --git a/api/envoy/config/route/v4alpha/route_components.proto b/api/envoy/config/route/v4alpha/route_components.proto index f921ea506d99..ef08c64b129e 100644 --- a/api/envoy/config/route/v4alpha/route_components.proto +++ b/api/envoy/config/route/v4alpha/route_components.proto @@ -1011,7 +1011,7 @@ message RouteAction { } // HTTP retry :ref:`architecture overview `. -// [#next-free-field: 11] +// [#next-free-field: 12] message RetryPolicy { option (udpa.annotations.versioning).previous_message_type = "envoy.config.route.v3.RetryPolicy"; @@ -1065,6 +1065,28 @@ message RetryPolicy { google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}]; } + message RateLimitedRetryBackOff { + option (udpa.annotations.versioning).previous_message_type = + "envoy.config.route.v3.RetryPolicy.RateLimitedRetryBackOff"; + + // Specifies the rate limited retry reset headers (like ``Retry-After`` or + // ``X-RateLimit-Reset``) to match against the response. If a request is + // being retried and the response contains one of these headers the value + // of the header will be used as the retry back off interval, with jitter added. + // + // The value of the header must be either: + // 1. An interval in seconds as an integer, eg. ``120``. + // 2. A unix timestamp as an integer, eg. ``1595320702``. The timestamp + // must be in the future relative to the current system time. + repeated HeaderMatcher reset_headers = 1; + + // Specifies the maximum rate limited retry back off interval that Envoy + // will allow. If a rate limited reset header contains an interval longer + // than this then the default exponential back off strategy will be used + // instead. + google.protobuf.Duration reset_max_interval = 2 [(validate.rules).duration = {gt {}}]; + } + // Specifies the conditions under which retry takes place. These are the same // conditions documented for :ref:`config_http_filters_router_x-envoy-retry-on` and // :ref:`config_http_filters_router_x-envoy-retry-grpc-on`. @@ -1107,13 +1129,22 @@ message RetryPolicy { // HTTP status codes that should trigger a retry in addition to those specified by retry_on. repeated uint32 retriable_status_codes = 7; - // Specifies parameters that control retry back off. This parameter is optional, in which case the + // Specifies parameters that control exponential retry back off. This parameter is optional, in which case the // default base interval is 25 milliseconds or, if set, the current value of the // `upstream.base_retry_backoff_ms` runtime parameter. The default maximum interval is 10 times // the base interval. The documentation for :ref:`config_http_filters_router_x-envoy-max-retries` // describes Envoy's back-off algorithm. RetryBackOff retry_back_off = 8; + // Specifies parameters that control a retry back off based on rate limited + // responses. When a request is rate limited by the server the server may + // include a header like ``Retry-After`` or ``X-RateLimit-Reset`` to provide + // feedback to the client on how long to wait before retrying. If configured, + // the rate limited retry back off strategy will be used instead of the + // default exponential back off strategy (configured using `retry_back_off`) + // whenever a response includes the matching headers. + RateLimitedRetryBackOff rate_limited_retry_back_off = 11; + // HTTP response headers that trigger a retry if present in the response. A retry will be // triggered if any of the header matches match the upstream response headers. // The field is only consulted if 'retriable-headers' retry policy is active. diff --git a/docs/root/configuration/http/http_filters/router_filter.rst b/docs/root/configuration/http/http_filters/router_filter.rst index 4ca285e9eda7..b8a9e895a858 100644 --- a/docs/root/configuration/http/http_filters/router_filter.rst +++ b/docs/root/configuration/http/http_filters/router_filter.rst @@ -257,6 +257,18 @@ in flight. The value of the header should be "true" or "false", and is ignored if invalid. +x-envoy-ratelimited-reset-headers +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Setting this header will extend the list of rate limited reset headers specified in the :ref:`route configuration rate limited retry back off +`. + +x-envoy-ratelimited-reset-max-interval-ms +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Setting this header will override the rate limited reset max interval specified in the :ref:`route configuration rate limited retry back off +`. + .. _config_http_filters_router_x-envoy-decorator-operation: x-envoy-decorator-operation @@ -413,6 +425,8 @@ statistics: upstream_rq_<\*xx>, Counter, "Aggregate HTTP response codes (e.g., 2xx, 3xx, etc.)" upstream_rq_<\*>, Counter, "Specific HTTP response codes (e.g., 201, 302, etc.)" upstream_rq_retry, Counter, Total request retries + upstream_rq_retry_backoff_exponential, Counter, Total retries using the exponential backoff strategy + upstream_rq_retry_backoff_ratelimited, Counter, Total retries using the ratelimited backoff strategy upstream_rq_retry_limit_exceeded, Counter, Total requests not retried due to exceeding :ref:`the configured number of maximum retries ` upstream_rq_retry_overflow, Counter, Total requests not retried due to circuit breaking or exceeding the :ref:`retry budgets ` upstream_rq_retry_success, Counter, Total request retry successes diff --git a/docs/root/configuration/upstream/cluster_manager/cluster_stats.rst b/docs/root/configuration/upstream/cluster_manager/cluster_stats.rst index 5d956c28d2b3..e4ba160313e7 100644 --- a/docs/root/configuration/upstream/cluster_manager/cluster_stats.rst +++ b/docs/root/configuration/upstream/cluster_manager/cluster_stats.rst @@ -74,6 +74,8 @@ Every cluster has a statistics tree rooted at *cluster..* with the followi upstream_rq_rx_reset, Counter, Total requests that were reset remotely upstream_rq_tx_reset, Counter, Total requests that were reset locally upstream_rq_retry, Counter, Total request retries + upstream_rq_retry_backoff_exponential, Counter, Total retries using the exponential backoff strategy + upstream_rq_retry_backoff_ratelimited, Counter, Total retries using the ratelimited backoff strategy upstream_rq_retry_limit_exceeded, Counter, Total requests not retried due to exceeding :ref:`the configured number of maximum retries ` upstream_rq_retry_success, Counter, Total request retry successes upstream_rq_retry_overflow, Counter, Total requests not retried due to circuit breaking or exceeding the :ref:`retry budget ` diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index f5b6217a1757..61466687aa9d 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -67,6 +67,10 @@ New Features * stats: added optional histograms to :ref:`cluster stats ` that track headers and body sizes of requests and responses. * stats: allow configuring histogram buckets for stats sinks and admin endpoints that support it. +* router: added a new :ref:`rate limited retry back off + ` strategy that uses + headers like `Retry-After` or `X-RateLimit-Reset` to decide the back off + interval. * tap: added :ref:`generic body matcher` to scan http requests and responses for text or hex patterns. * tcp: switched the TCP connection pool to the new "shared" connection pool, sharing a common code base with HTTP and HTTP/2. Any unexpected behavioral changes can be temporarily reverted by setting `envoy.reloadable_features.new_tcp_connection_pool` to false. * watchdog: support randomizing the watchdog's kill timeout to prevent synchronized kills via a maximium jitter parameter :ref:`max_kill_timeout_jitter`. diff --git a/generated_api_shadow/envoy/config/route/v3/route_components.proto b/generated_api_shadow/envoy/config/route/v3/route_components.proto index f79f399d2140..8e32d9b21886 100644 --- a/generated_api_shadow/envoy/config/route/v3/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v3/route_components.proto @@ -1044,7 +1044,7 @@ message RouteAction { } // HTTP retry :ref:`architecture overview `. -// [#next-free-field: 11] +// [#next-free-field: 12] message RetryPolicy { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.route.RetryPolicy"; @@ -1094,6 +1094,25 @@ message RetryPolicy { google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}]; } + message RateLimitedRetryBackOff { + // Specifies the rate limited retry reset headers (like ``Retry-After`` or + // ``X-RateLimit-Reset``) to match against the response. If a request is + // being retried and the response contains one of these headers the value + // of the header will be used as the retry back off interval, with jitter added. + // + // The value of the header must be either: + // 1. An interval in seconds as an integer, eg. ``120``. + // 2. A unix timestamp as an integer, eg. ``1595320702``. The timestamp + // must be in the future relative to the current system time. + repeated HeaderMatcher reset_headers = 1; + + // Specifies the maximum rate limited retry back off interval that Envoy + // will allow. If a rate limited reset header contains an interval longer + // than this then the default exponential back off strategy will be used + // instead. + google.protobuf.Duration reset_max_interval = 2 [(validate.rules).duration = {gt {}}]; + } + // Specifies the conditions under which retry takes place. These are the same // conditions documented for :ref:`config_http_filters_router_x-envoy-retry-on` and // :ref:`config_http_filters_router_x-envoy-retry-grpc-on`. @@ -1137,13 +1156,22 @@ message RetryPolicy { // HTTP status codes that should trigger a retry in addition to those specified by retry_on. repeated uint32 retriable_status_codes = 7; - // Specifies parameters that control retry back off. This parameter is optional, in which case the + // Specifies parameters that control exponential retry back off. This parameter is optional, in which case the // default base interval is 25 milliseconds or, if set, the current value of the // `upstream.base_retry_backoff_ms` runtime parameter. The default maximum interval is 10 times // the base interval. The documentation for :ref:`config_http_filters_router_x-envoy-max-retries` // describes Envoy's back-off algorithm. RetryBackOff retry_back_off = 8; + // Specifies parameters that control a retry back off based on rate limited + // responses. When a request is rate limited by the server the server may + // include a header like ``Retry-After`` or ``X-RateLimit-Reset`` to provide + // feedback to the client on how long to wait before retrying. If configured, + // the rate limited retry back off strategy will be used instead of the + // default exponential back off strategy (configured using `retry_back_off`) + // whenever a response includes the matching headers. + RateLimitedRetryBackOff rate_limited_retry_back_off = 11; + // HTTP response headers that trigger a retry if present in the response. A retry will be // triggered if any of the header matches match the upstream response headers. // The field is only consulted if 'retriable-headers' retry policy is active. diff --git a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto index a8b6ae4459ce..7b7c98d07b7d 100644 --- a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto @@ -1039,7 +1039,7 @@ message RouteAction { } // HTTP retry :ref:`architecture overview `. -// [#next-free-field: 11] +// [#next-free-field: 12] message RetryPolicy { option (udpa.annotations.versioning).previous_message_type = "envoy.config.route.v3.RetryPolicy"; @@ -1093,6 +1093,28 @@ message RetryPolicy { google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}]; } + message RateLimitedRetryBackOff { + option (udpa.annotations.versioning).previous_message_type = + "envoy.config.route.v3.RetryPolicy.RateLimitedRetryBackOff"; + + // Specifies the rate limited retry reset headers (like ``Retry-After`` or + // ``X-RateLimit-Reset``) to match against the response. If a request is + // being retried and the response contains one of these headers the value + // of the header will be used as the retry back off interval, with jitter added. + // + // The value of the header must be either: + // 1. An interval in seconds as an integer, eg. ``120``. + // 2. A unix timestamp as an integer, eg. ``1595320702``. The timestamp + // must be in the future relative to the current system time. + repeated HeaderMatcher reset_headers = 1; + + // Specifies the maximum rate limited retry back off interval that Envoy + // will allow. If a rate limited reset header contains an interval longer + // than this then the default exponential back off strategy will be used + // instead. + google.protobuf.Duration reset_max_interval = 2 [(validate.rules).duration = {gt {}}]; + } + // Specifies the conditions under which retry takes place. These are the same // conditions documented for :ref:`config_http_filters_router_x-envoy-retry-on` and // :ref:`config_http_filters_router_x-envoy-retry-grpc-on`. @@ -1135,13 +1157,22 @@ message RetryPolicy { // HTTP status codes that should trigger a retry in addition to those specified by retry_on. repeated uint32 retriable_status_codes = 7; - // Specifies parameters that control retry back off. This parameter is optional, in which case the + // Specifies parameters that control exponential retry back off. This parameter is optional, in which case the // default base interval is 25 milliseconds or, if set, the current value of the // `upstream.base_retry_backoff_ms` runtime parameter. The default maximum interval is 10 times // the base interval. The documentation for :ref:`config_http_filters_router_x-envoy-max-retries` // describes Envoy's back-off algorithm. RetryBackOff retry_back_off = 8; + // Specifies parameters that control a retry back off based on rate limited + // responses. When a request is rate limited by the server the server may + // include a header like ``Retry-After`` or ``X-RateLimit-Reset`` to provide + // feedback to the client on how long to wait before retrying. If configured, + // the rate limited retry back off strategy will be used instead of the + // default exponential back off strategy (configured using `retry_back_off`) + // whenever a response includes the matching headers. + RateLimitedRetryBackOff rate_limited_retry_back_off = 11; + // HTTP response headers that trigger a retry if present in the response. A retry will be // triggered if any of the header matches match the upstream response headers. // The field is only consulted if 'retriable-headers' retry policy is active. diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index bc5e9338a2dc..22216aa12ea5 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -16,6 +16,7 @@ #include "absl/container/inlined_vector.h" #include "absl/strings/string_view.h" +#include "absl/types/optional.h" namespace Envoy { namespace Http { @@ -272,6 +273,8 @@ class HeaderEntry { HEADER_FUNC(EnvoyInternalRequest) \ HEADER_FUNC(EnvoyIpTags) \ HEADER_FUNC(EnvoyMaxRetries) \ + HEADER_FUNC(EnvoyRateLimitedResetHeaders) \ + HEADER_FUNC(EnvoyRateLimitedResetMaxIntervalMs) \ HEADER_FUNC(EnvoyRetryOn) \ HEADER_FUNC(EnvoyRetryGrpcOn) \ HEADER_FUNC(EnvoyRetriableStatusCodes) \ @@ -773,6 +776,15 @@ class HeaderMatcher { public: virtual ~HeaderMatcher() = default; + /** + * This getter exists so that once matchHeaders() has been called on a HeaderMap the concrete + * header in that map can be extracted by using this getter to discover its name. This is possible + * in all cases, except when invert_match_ is true, because in the latter case name_ would *not* + * reflect the matching header. + * @return the header name the matcher will match against. + */ + virtual const LowerCaseString* name() const PURE; + /** * Check whether header matcher matches any headers in a given HeaderMap. */ diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 7d37cf02cf32..71fcbdba6c68 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -235,6 +235,18 @@ class RetryPolicy { * @return absl::optional maximum retry interval */ virtual absl::optional maxInterval() const PURE; + + /** + * @return std::vector& list of response header + * matchers that will be attempted to extract a rate limited maximum retry interval. + */ + virtual const std::vector& rateLimitedResetHeaders() const PURE; + + /** + * @return absl::optional limit placed on a rate limited retry + * interval. + */ + virtual absl::optional rateLimitedResetMaxInterval() const PURE; }; /** @@ -294,6 +306,14 @@ class RetryState { */ virtual bool enabled() PURE; + /** + * Attempts to parse any matching rate limited reset headers (RFC 7231), either in the form of an + * interval directly, or in the form of a unix timestamp relative to the current system time. + * @return the interval if parsing was successful. + */ + virtual absl::optional + parseRateLimitedResetInterval(const Http::ResponseHeaderMap& response_headers) const PURE; + /** * Determine whether a request should be retried based on the response headers. * @param response_headers supplies the response headers. @@ -369,10 +389,21 @@ class RetryState { const Upstream::PrioritySet& priority_set, const Upstream::HealthyAndDegradedLoad& original_priority_load, const Upstream::RetryPriority::PriorityMappingFunc& priority_mapping_func) PURE; + /** * return how many times host selection should be reattempted during host selection. */ virtual uint32_t hostSelectionMaxAttempts() const PURE; + + /** + * @return the rate limited reset headers used to match against rate limited responses. + */ + virtual const std::vector& rateLimitedResetHeaders() const PURE; + + /** + * @return the upper limit placed on an interval parsed from a rate limited reset header. + */ + virtual std::chrono::milliseconds rateLimitedResetMaxInterval() const PURE; }; using RetryStatePtr = std::unique_ptr; @@ -417,6 +448,8 @@ using ShadowPolicyPtr = std::unique_ptr; */ #define ALL_VIRTUAL_CLUSTER_STATS(COUNTER) \ COUNTER(upstream_rq_retry) \ + COUNTER(upstream_rq_retry_backoff_exponential) \ + COUNTER(upstream_rq_retry_backoff_ratelimited) \ COUNTER(upstream_rq_retry_limit_exceeded) \ COUNTER(upstream_rq_retry_overflow) \ COUNTER(upstream_rq_retry_success) \ diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index ebdc1575eb8f..9330b0e8fdb5 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -577,6 +577,8 @@ class PrioritySet { COUNTER(upstream_rq_pending_total) \ COUNTER(upstream_rq_per_try_timeout) \ COUNTER(upstream_rq_retry) \ + COUNTER(upstream_rq_retry_backoff_exponential) \ + COUNTER(upstream_rq_retry_backoff_ratelimited) \ COUNTER(upstream_rq_retry_limit_exceeded) \ COUNTER(upstream_rq_retry_overflow) \ COUNTER(upstream_rq_retry_success) \ diff --git a/source/common/common/backoff_strategy.cc b/source/common/common/backoff_strategy.cc index c9b5b61b733b..4f08e4332fa6 100644 --- a/source/common/common/backoff_strategy.cc +++ b/source/common/common/backoff_strategy.cc @@ -2,15 +2,15 @@ namespace Envoy { -JitteredBackOffStrategy::JitteredBackOffStrategy(uint64_t base_interval, uint64_t max_interval, - Random::RandomGenerator& random) +JitteredExponentialBackOffStrategy::JitteredExponentialBackOffStrategy( + uint64_t base_interval, uint64_t max_interval, Random::RandomGenerator& random) : base_interval_(base_interval), max_interval_(max_interval), next_interval_(base_interval), random_(random) { ASSERT(base_interval_ > 0); ASSERT(base_interval_ <= max_interval_); } -uint64_t JitteredBackOffStrategy::nextBackOffMs() { +uint64_t JitteredExponentialBackOffStrategy::nextBackOffMs() { const uint64_t backoff = next_interval_; ASSERT(backoff > 0); // Set next_interval_ to max_interval_ if doubling the interval would exceed the max or overflow. @@ -22,7 +22,16 @@ uint64_t JitteredBackOffStrategy::nextBackOffMs() { return std::min(random_.random() % backoff, max_interval_); } -void JitteredBackOffStrategy::reset() { next_interval_ = base_interval_; } +void JitteredExponentialBackOffStrategy::reset() { next_interval_ = base_interval_; } + +JitteredLowerBoundBackOffStrategy::JitteredLowerBoundBackOffStrategy( + uint64_t min_interval, Random::RandomGenerator& random) + : min_interval_(min_interval), random_(random) {} + +uint64_t JitteredLowerBoundBackOffStrategy::nextBackOffMs() { + // random(min_interval_, 1.5 * min_interval_) + return (random_.random() % (min_interval_ >> 1)) + min_interval_; +} FixedBackOffStrategy::FixedBackOffStrategy(uint64_t interval_ms) : interval_ms_(interval_ms) { ASSERT(interval_ms_ > 0); diff --git a/source/common/common/backoff_strategy.h b/source/common/common/backoff_strategy.h index 2484f3e11b20..c1928632979a 100644 --- a/source/common/common/backoff_strategy.h +++ b/source/common/common/backoff_strategy.h @@ -13,7 +13,7 @@ namespace Envoy { /** * Implementation of BackOffStrategy that uses a fully jittered exponential backoff algorithm. */ -class JitteredBackOffStrategy : public BackOffStrategy { +class JitteredExponentialBackOffStrategy : public BackOffStrategy { public: /** @@ -23,8 +23,8 @@ class JitteredBackOffStrategy : public BackOffStrategy { * @param max_interval the cap on the next backoff value. * @param random the random generator. */ - JitteredBackOffStrategy(uint64_t base_interval, uint64_t max_interval, - Random::RandomGenerator& random); + JitteredExponentialBackOffStrategy(uint64_t base_interval, uint64_t max_interval, + Random::RandomGenerator& random); // BackOffStrategy methods uint64_t nextBackOffMs() override; @@ -37,6 +37,28 @@ class JitteredBackOffStrategy : public BackOffStrategy { Random::RandomGenerator& random_; }; +/** + * Implementation of BackOffStrategy that returns random values in the range + * [min_interval, 1.5 * min_interval). + */ +class JitteredLowerBoundBackOffStrategy : public BackOffStrategy { +public: + /** + * Constructs fully jittered backoff strategy. + * @param min_interval the lower bound on the next backoff value. + * @param random the random generator. + */ + JitteredLowerBoundBackOffStrategy(uint64_t min_interval, Random::RandomGenerator& random); + + // BackOffStrategy methods + uint64_t nextBackOffMs() override; + void reset() override {} + +private: + const uint64_t min_interval_; + Random::RandomGenerator& random_; +}; + /** * Implementation of BackOffStrategy that uses a fixed backoff. */ diff --git a/source/common/config/datasource.cc b/source/common/config/datasource.cc index d3e286d0b27a..776061a61be2 100644 --- a/source/common/config/datasource.cc +++ b/source/common/config/datasource.cc @@ -64,8 +64,8 @@ RemoteAsyncDataProvider::RemoteAsyncDataProvider( } } - backoff_strategy_ = - std::make_unique(base_interval_ms, max_interval_ms, random); + backoff_strategy_ = std::make_unique(base_interval_ms, + max_interval_ms, random); retry_timer_ = dispatcher.createTimer([this]() -> void { start(); }); manager.add(init_target_); diff --git a/source/common/config/grpc_stream.h b/source/common/config/grpc_stream.h index b922d8c09f0f..f6673402df4f 100644 --- a/source/common/config/grpc_stream.h +++ b/source/common/config/grpc_stream.h @@ -48,8 +48,8 @@ class GrpcStream : public Grpc::AsyncStreamCallbacks, // TODO(htuch): Make this configurable. static constexpr uint32_t RetryInitialDelayMs = 500; static constexpr uint32_t RetryMaxDelayMs = 30000; // Do not cross more than 30s - backoff_strategy_ = - std::make_unique(RetryInitialDelayMs, RetryMaxDelayMs, random_); + backoff_strategy_ = std::make_unique( + RetryInitialDelayMs, RetryMaxDelayMs, random_); } void establishNewStream() { diff --git a/source/common/config/utility.h b/source/common/config/utility.h index d19026386c53..fc8e4f0f7352 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -428,7 +428,8 @@ class Utility { throw EnvoyException("dns_failure_refresh_rate must have max_interval greater than " "or equal to the base_interval"); } - return std::make_unique(base_interval_ms, max_interval_ms, random); + return std::make_unique(base_interval_ms, max_interval_ms, + random); } return std::make_unique(dns_refresh_rate_ms); } diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index a4e2e7c86b84..c7cb930306a3 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -152,10 +152,17 @@ class AsyncStreamImpl : public AsyncClient::Stream, return absl::nullopt; } absl::optional maxInterval() const override { return absl::nullopt; } + const std::vector& rateLimitedResetHeaders() const override { + return ratelimited_reset_headers_; + } + absl::optional rateLimitedResetMaxInterval() const override { + return absl::nullopt; + } const std::vector retriable_status_codes_{}; const std::vector retriable_headers_{}; const std::vector retriable_request_headers_{}; + const std::vector ratelimited_reset_headers_{}; }; struct NullConfig : public Router::Config { diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index 22992f1927f9..7304286f5d4c 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -47,6 +47,13 @@ class HeaderUtility { const bool invert_match_; // HeaderMatcher + const LowerCaseString* name() const override { + if (invert_match_) { + return nullptr; + } + return &name_; + } + bool matchesHeaders(const HeaderMap& headers) const override { return HeaderUtility::matchHeaders(headers, *this); }; diff --git a/source/common/http/headers.h b/source/common/http/headers.h index 73f866f7b60f..dc4806985642 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -157,6 +157,10 @@ class HeaderValues { const LowerCaseString EnvoyOriginalPath{absl::StrCat(prefix(), "-original-path")}; const LowerCaseString EnvoyOverloaded{absl::StrCat(prefix(), "-overloaded")}; const LowerCaseString EnvoyRateLimited{absl::StrCat(prefix(), "-ratelimited")}; + const LowerCaseString EnvoyRateLimitedResetHeaders{ + absl::StrCat(prefix(), "-ratelimited-reset-headers")}; + const LowerCaseString EnvoyRateLimitedResetMaxIntervalMs{ + absl::StrCat(prefix(), "-ratelimited-reset-max-interval-ms")}; const LowerCaseString EnvoyRetryOn{absl::StrCat(prefix(), "-retry-on")}; const LowerCaseString EnvoyRetryGrpcOn{absl::StrCat(prefix(), "-retry-grpc-on")}; const LowerCaseString EnvoyRetriableStatusCodes{ diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 73a2de20e6dd..fc644a518eb0 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -120,6 +120,17 @@ RetryPolicyImpl::RetryPolicyImpl(const envoy::config::route::v3::RetryPolicy& re } } } + + if (retry_policy.has_rate_limited_retry_back_off()) { + ratelimited_reset_headers_ = Http::HeaderUtility::buildHeaderMatcherVector( + retry_policy.rate_limited_retry_back_off().reset_headers()); + + ratelimited_reset_max_interval_ = + PROTOBUF_GET_OPTIONAL_MS(retry_policy.rate_limited_retry_back_off(), reset_max_interval); + if (ratelimited_reset_max_interval_ && ((*ratelimited_reset_max_interval_).count()) < 1) { + ratelimited_reset_max_interval_ = std::chrono::milliseconds(1); + } + } } std::vector RetryPolicyImpl::retryHostPredicates() const { diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index a32d19fbe742..206c9e37aa41 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -272,6 +272,12 @@ class RetryPolicyImpl : public RetryPolicy { } absl::optional baseInterval() const override { return base_interval_; } absl::optional maxInterval() const override { return max_interval_; } + const std::vector& rateLimitedResetHeaders() const override { + return ratelimited_reset_headers_; + } + absl::optional rateLimitedResetMaxInterval() const override { + return ratelimited_reset_max_interval_; + } private: std::chrono::milliseconds per_try_timeout_{0}; @@ -294,6 +300,8 @@ class RetryPolicyImpl : public RetryPolicy { std::vector retriable_request_headers_; absl::optional base_interval_; absl::optional max_interval_; + std::vector ratelimited_reset_headers_{}; + absl::optional ratelimited_reset_max_interval_; ProtobufMessage::ValidationVisitor* validation_visitor_{}; }; diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc index 9912f041709e..a19355ac8513 100644 --- a/source/common/router/retry_state_impl.cc +++ b/source/common/router/retry_state_impl.cc @@ -38,14 +38,14 @@ RetryStatePtr RetryStateImpl::create(const RetryPolicy& route_policy, const Upstream::ClusterInfo& cluster, const VirtualCluster* vcluster, Runtime::Loader& runtime, Random::RandomGenerator& random, Event::Dispatcher& dispatcher, - Upstream::ResourcePriority priority) { + TimeSource& time_source, Upstream::ResourcePriority priority) { RetryStatePtr ret; // We short circuit here and do not bother with an allocation if there is no chance we will retry. if (request_headers.EnvoyRetryOn() || request_headers.EnvoyRetryGrpcOn() || route_policy.retryOn()) { ret.reset(new RetryStateImpl(route_policy, request_headers, cluster, vcluster, runtime, random, - dispatcher, priority)); + dispatcher, time_source, priority)); } // Consume all retry related headers to avoid them being propagated to the upstream @@ -66,14 +66,16 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, Http::RequestHeaderMap& request_headers, const Upstream::ClusterInfo& cluster, const VirtualCluster* vcluster, Runtime::Loader& runtime, Random::RandomGenerator& random, - Event::Dispatcher& dispatcher, Upstream::ResourcePriority priority) + Event::Dispatcher& dispatcher, TimeSource& time_source, + Upstream::ResourcePriority priority) : cluster_(cluster), vcluster_(vcluster), runtime_(runtime), random_(random), - dispatcher_(dispatcher), retry_on_(route_policy.retryOn()), + dispatcher_(dispatcher), time_source_(time_source), retry_on_(route_policy.retryOn()), retries_remaining_(route_policy.numRetries()), priority_(priority), retry_host_predicates_(route_policy.retryHostPredicates()), retry_priority_(route_policy.retryPriority()), retriable_status_codes_(route_policy.retriableStatusCodes()), - retriable_headers_(route_policy.retriableHeaders()) { + retriable_headers_(route_policy.retriableHeaders()), + ratelimited_reset_headers_(route_policy.rateLimitedResetHeaders()) { std::chrono::milliseconds base_interval( runtime_.snapshot().getInteger("upstream.base_retry_backoff_ms", 25)); @@ -88,8 +90,8 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, max_interval = *route_policy.maxInterval(); } - backoff_strategy_ = std::make_unique(base_interval.count(), - max_interval.count(), random_); + backoff_strategy_ = std::make_unique( + base_interval.count(), max_interval.count(), random_); host_selection_max_attempts_ = route_policy.hostSelectionMaxAttempts(); // Merge in the headers. @@ -147,6 +149,32 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, std::make_shared(header_matcher)); } } + + if (request_headers.EnvoyRateLimitedResetMaxIntervalMs()) { + const auto max_interval = + request_headers.EnvoyRateLimitedResetMaxIntervalMs()->value().getStringView(); + unsigned long out; + if (absl::SimpleAtoi(max_interval, &out)) { + ratelimited_reset_max_interval_ = std::chrono::milliseconds(out); + } + } else if (route_policy.rateLimitedResetMaxInterval().has_value()) { + ratelimited_reset_max_interval_ = route_policy.rateLimitedResetMaxInterval().value(); + } + + if (request_headers.EnvoyRateLimitedResetHeaders()) { + // Ratelimit reset headers in the configuration are specified via HeaderMatcher. + // Giving the same flexibility via request header would require the user + // to provide a HeaderMatcher serialized into a string. To avoid this extra + // complexity we only support name-only header matchers via request + // header. Anything more sophisticated needs to be provided via config. + for (const auto header_name : StringUtil::splitToken( + request_headers.EnvoyRateLimitedResetHeaders()->value().getStringView(), ",")) { + envoy::config::route::v3::HeaderMatcher header_matcher; + header_matcher.set_name(std::string(absl::StripAsciiWhitespace(header_name))); + ratelimited_reset_headers_.emplace_back( + std::make_shared(header_matcher)); + } + } } RetryStateImpl::~RetryStateImpl() { resetRetry(); } @@ -156,8 +184,29 @@ void RetryStateImpl::enableBackoffTimer() { retry_timer_ = dispatcher_.createTimer([this]() -> void { callback_(); }); } - // We use a fully jittered exponential backoff algorithm. - retry_timer_->enableTimer(std::chrono::milliseconds(backoff_strategy_->nextBackOffMs())); + if (ratelimited_backoff_strategy_ != nullptr) { + // If we have a backoff strategy based on rate limit feedback from the response we use it. + retry_timer_->enableTimer( + std::chrono::milliseconds(ratelimited_backoff_strategy_->nextBackOffMs())); + + // The strategy is only valid for the response that sent the ratelimit reset header and cannot + // be reused. + ratelimited_backoff_strategy_.reset(); + + cluster_.stats().upstream_rq_retry_backoff_ratelimited_.inc(); + if (vcluster_) { + vcluster_->stats().upstream_rq_retry_backoff_ratelimited_.inc(); + } + + } else { + // Otherwise we use a fully jittered exponential backoff algorithm. + retry_timer_->enableTimer(std::chrono::milliseconds(backoff_strategy_->nextBackOffMs())); + + cluster_.stats().upstream_rq_retry_backoff_exponential_.inc(); + if (vcluster_) { + vcluster_->stats().upstream_rq_retry_backoff_exponential_.inc(); + } + } } std::pair RetryStateImpl::parseRetryOn(absl::string_view config) { @@ -212,6 +261,50 @@ std::pair RetryStateImpl::parseRetryGrpcOn(absl::string_view ret return {ret, all_fields_valid}; } +absl::optional RetryStateImpl::parseRateLimitedResetInterval( + const Http::ResponseHeaderMap& response_headers) const { + bool parsed_value = false; + uint64_t num_seconds{}; + + for (const auto& reset_header : ratelimited_reset_headers_) { + if (reset_header->matchesHeaders(response_headers)) { + const Http::LowerCaseString* header_name = reset_header->name(); + + if (header_name != nullptr) { + const Http::HeaderEntry* entry = response_headers.get(*header_name); + + if (entry != nullptr) { + const auto& header_value = entry->value().getStringView(); + + // Try to parse the value of the header as an int storing the number of seconds + if (absl::SimpleAtoi(header_value, &num_seconds)) { + parsed_value = true; + } + } + } + } + } + + if (parsed_value) { + const auto time_now = time_source_.systemTime().time_since_epoch(); + uint64_t timestamp = std::chrono::duration_cast(time_now).count(); + + // Is the value big enough to be a unix timestamp rather than an interval? + if (num_seconds > timestamp) { + num_seconds = num_seconds - timestamp; + } + + const auto interval = std::chrono::milliseconds(num_seconds * 1000UL); + + // Is the interval value within our accepted range? + if (interval <= ratelimited_reset_max_interval_) { + return absl::optional(interval); + } + } + + return absl::nullopt; +} + void RetryStateImpl::resetRetry() { if (callback_) { cluster_.resourceManager(priority_).retries().dec(); @@ -273,7 +366,19 @@ RetryStatus RetryStateImpl::shouldRetry(bool would_retry, DoRetryCallback callba RetryStatus RetryStateImpl::shouldRetryHeaders(const Http::ResponseHeaderMap& response_headers, DoRetryCallback callback) { - return shouldRetry(wouldRetryFromHeaders(response_headers), callback); + const bool would_retry = wouldRetryFromHeaders(response_headers); + + // Yes, we will retry based on the headers - try to parse a rate limited reset interval from the + // response. + if (would_retry && ratelimited_reset_headers_.size() > 0) { + const auto backoff_interval = parseRateLimitedResetInterval(response_headers); + if (backoff_interval.has_value()) { + ratelimited_backoff_strategy_ = std::make_unique( + backoff_interval.value().count(), random_); + } + } + + return shouldRetry(would_retry, callback); } RetryStatus RetryStateImpl::shouldRetryReset(Http::StreamResetReason reset_reason, diff --git a/source/common/router/retry_state_impl.h b/source/common/router/retry_state_impl.h index 9b0a19a77911..72949aa63938 100644 --- a/source/common/router/retry_state_impl.h +++ b/source/common/router/retry_state_impl.h @@ -29,7 +29,8 @@ class RetryStateImpl : public RetryState { Http::RequestHeaderMap& request_headers, const Upstream::ClusterInfo& cluster, const VirtualCluster* vcluster, Runtime::Loader& runtime, Random::RandomGenerator& random, - Event::Dispatcher& dispatcher, Upstream::ResourcePriority priority); + Event::Dispatcher& dispatcher, TimeSource& time_source, + Upstream::ResourcePriority priority); ~RetryStateImpl() override; /** @@ -52,6 +53,8 @@ class RetryStateImpl : public RetryState { // Router::RetryState bool enabled() override { return retry_on_ != 0; } + absl::optional + parseRateLimitedResetInterval(const Http::ResponseHeaderMap& response_headers) const override; RetryStatus shouldRetryHeaders(const Http::ResponseHeaderMap& response_headers, DoRetryCallback callback) override; // Returns true if the retry policy would retry the passed headers. Does not @@ -88,11 +91,20 @@ class RetryStateImpl : public RetryState { uint32_t hostSelectionMaxAttempts() const override { return host_selection_max_attempts_; } + const std::vector& rateLimitedResetHeaders() const override { + return ratelimited_reset_headers_; + } + + std::chrono::milliseconds rateLimitedResetMaxInterval() const override { + return ratelimited_reset_max_interval_; + } + private: RetryStateImpl(const RetryPolicy& route_policy, Http::RequestHeaderMap& request_headers, const Upstream::ClusterInfo& cluster, const VirtualCluster* vcluster, Runtime::Loader& runtime, Random::RandomGenerator& random, - Event::Dispatcher& dispatcher, Upstream::ResourcePriority priority); + Event::Dispatcher& dispatcher, TimeSource& time_source, + Upstream::ResourcePriority priority); void enableBackoffTimer(); void resetRetry(); @@ -104,17 +116,21 @@ class RetryStateImpl : public RetryState { Runtime::Loader& runtime_; Random::RandomGenerator& random_; Event::Dispatcher& dispatcher_; + TimeSource& time_source_; uint32_t retry_on_{}; uint32_t retries_remaining_{}; DoRetryCallback callback_; Event::TimerPtr retry_timer_; Upstream::ResourcePriority priority_; BackOffStrategyPtr backoff_strategy_; + BackOffStrategyPtr ratelimited_backoff_strategy_{}; std::vector retry_host_predicates_; Upstream::RetryPrioritySharedPtr retry_priority_; uint32_t host_selection_max_attempts_; std::vector retriable_status_codes_; std::vector retriable_headers_; + std::vector ratelimited_reset_headers_{}; + std::chrono::milliseconds ratelimited_reset_max_interval_{300000}; }; } // namespace Router diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 88db133d6e2c..9e2e00128a4a 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -569,9 +569,9 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, // Ensure an http transport scheme is selected before continuing with decoding. ASSERT(headers.Scheme()); - retry_state_ = createRetryState(route_entry_->retryPolicy(), headers, *cluster_, - request_vcluster_, config_.runtime_, config_.random_, - callbacks_->dispatcher(), route_entry_->priority()); + retry_state_ = createRetryState( + route_entry_->retryPolicy(), headers, *cluster_, request_vcluster_, config_.runtime_, + config_.random_, callbacks_->dispatcher(), config_.timeSource(), route_entry_->priority()); // Determine which shadow policies to use. It's possible that we don't do any shadowing due to // runtime keys. @@ -1575,13 +1575,15 @@ uint32_t Filter::numRequestsAwaitingHeaders() { [](const auto& req) -> bool { return req->awaitingHeaders(); }); } -RetryStatePtr -ProdFilter::createRetryState(const RetryPolicy& policy, Http::RequestHeaderMap& request_headers, - const Upstream::ClusterInfo& cluster, const VirtualCluster* vcluster, - Runtime::Loader& runtime, Random::RandomGenerator& random, - Event::Dispatcher& dispatcher, Upstream::ResourcePriority priority) { +RetryStatePtr ProdFilter::createRetryState(const RetryPolicy& policy, + Http::RequestHeaderMap& request_headers, + const Upstream::ClusterInfo& cluster, + const VirtualCluster* vcluster, Runtime::Loader& runtime, + Random::RandomGenerator& random, + Event::Dispatcher& dispatcher, TimeSource& time_source, + Upstream::ResourcePriority priority) { return RetryStateImpl::create(policy, request_headers, cluster, vcluster, runtime, random, - dispatcher, priority); + dispatcher, time_source, priority); } } // namespace Router diff --git a/source/common/router/router.h b/source/common/router/router.h index 65ef129ccf3a..e1be0571e2b6 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -473,11 +473,13 @@ class Filter : Logger::Loggable, bool dropped); void chargeUpstreamAbort(Http::Code code, bool dropped, UpstreamRequest& upstream_request); void cleanup(); - virtual RetryStatePtr - createRetryState(const RetryPolicy& policy, Http::RequestHeaderMap& request_headers, - const Upstream::ClusterInfo& cluster, const VirtualCluster* vcluster, - Runtime::Loader& runtime, Random::RandomGenerator& random, - Event::Dispatcher& dispatcher, Upstream::ResourcePriority priority) PURE; + virtual RetryStatePtr createRetryState(const RetryPolicy& policy, + Http::RequestHeaderMap& request_headers, + const Upstream::ClusterInfo& cluster, + const VirtualCluster* vcluster, Runtime::Loader& runtime, + Random::RandomGenerator& random, + Event::Dispatcher& dispatcher, TimeSource& time_source, + Upstream::ResourcePriority priority) PURE; std::unique_ptr createConnPool(); UpstreamRequestPtr createUpstreamRequest(); @@ -567,6 +569,7 @@ class ProdFilter : public Filter { const Upstream::ClusterInfo& cluster, const VirtualCluster* vcluster, Runtime::Loader& runtime, Random::RandomGenerator& random, Event::Dispatcher& dispatcher, + TimeSource& time_source, Upstream::ResourcePriority priority) override; }; diff --git a/source/common/upstream/health_discovery_service.cc b/source/common/upstream/health_discovery_service.cc index 21fa74e34588..4d66e0f6c63f 100644 --- a/source/common/upstream/health_discovery_service.cc +++ b/source/common/upstream/health_discovery_service.cc @@ -50,8 +50,8 @@ HdsDelegate::HdsDelegate(Stats::Scope& scope, Grpc::RawAsyncClientPtr async_clie api_(api) { health_check_request_.mutable_health_check_request()->mutable_node()->MergeFrom( local_info_.node()); - backoff_strategy_ = std::make_unique(RetryInitialDelayMilliseconds, - RetryMaxDelayMilliseconds, random_); + backoff_strategy_ = std::make_unique( + RetryInitialDelayMilliseconds, RetryMaxDelayMilliseconds, random_); hds_retry_timer_ = dispatcher.createTimer([this]() -> void { establishNewStream(); }); hds_stream_response_timer_ = dispatcher.createTimer([this]() -> void { sendResponse(); }); diff --git a/test/common/common/backoff_strategy_test.cc b/test/common/common/backoff_strategy_test.cc index 20ffe937065c..bfc540234c7b 100644 --- a/test/common/common/backoff_strategy_test.cc +++ b/test/common/common/backoff_strategy_test.cc @@ -9,20 +9,20 @@ using testing::Return; namespace Envoy { -TEST(BackOffStrategyTest, JitteredBackOffBasicFlow) { +TEST(ExponentialBackOffStrategyTest, JitteredBackOffBasicFlow) { NiceMock random; ON_CALL(random, random()).WillByDefault(Return(27)); - JitteredBackOffStrategy jittered_back_off(25, 30, random); + JitteredExponentialBackOffStrategy jittered_back_off(25, 30, random); EXPECT_EQ(2, jittered_back_off.nextBackOffMs()); EXPECT_EQ(27, jittered_back_off.nextBackOffMs()); } -TEST(BackOffStrategyTest, JitteredBackOffBasicReset) { +TEST(ExponentialBackOffStrategyTest, JitteredBackOffBasicReset) { NiceMock random; ON_CALL(random, random()).WillByDefault(Return(27)); - JitteredBackOffStrategy jittered_back_off(25, 30, random); + JitteredExponentialBackOffStrategy jittered_back_off(25, 30, random); EXPECT_EQ(2, jittered_back_off.nextBackOffMs()); EXPECT_EQ(27, jittered_back_off.nextBackOffMs()); @@ -30,22 +30,23 @@ TEST(BackOffStrategyTest, JitteredBackOffBasicReset) { EXPECT_EQ(2, jittered_back_off.nextBackOffMs()); // Should start from start } -TEST(BackOffStrategyTest, JitteredBackOffDoesntOverflow) { +TEST(ExponentialBackOffStrategyTest, JitteredBackOffDoesntOverflow) { NiceMock random; ON_CALL(random, random()).WillByDefault(Return(std::numeric_limits::max() - 1)); - JitteredBackOffStrategy jittered_back_off(1, std::numeric_limits::max(), random); + JitteredExponentialBackOffStrategy jittered_back_off(1, std::numeric_limits::max(), + random); for (int iter = 0; iter < 100; ++iter) { EXPECT_GE(std::numeric_limits::max(), jittered_back_off.nextBackOffMs()); } EXPECT_EQ(std::numeric_limits::max() - 1, jittered_back_off.nextBackOffMs()); } -TEST(BackOffStrategyTest, JitteredBackOffWithMaxInterval) { +TEST(ExponentialBackOffStrategyTest, JitteredBackOffWithMaxInterval) { NiceMock random; ON_CALL(random, random()).WillByDefault(Return(9999)); - JitteredBackOffStrategy jittered_back_off(5, 100, random); + JitteredExponentialBackOffStrategy jittered_back_off(5, 100, random); EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); EXPECT_EQ(9, jittered_back_off.nextBackOffMs()); EXPECT_EQ(19, jittered_back_off.nextBackOffMs()); @@ -55,11 +56,11 @@ TEST(BackOffStrategyTest, JitteredBackOffWithMaxInterval) { EXPECT_EQ(99, jittered_back_off.nextBackOffMs()); } -TEST(BackOffStrategyTest, JitteredBackOffWithMaxIntervalReset) { +TEST(ExponentialBackOffStrategyTest, JitteredBackOffWithMaxIntervalReset) { NiceMock random; ON_CALL(random, random()).WillByDefault(Return(9999)); - JitteredBackOffStrategy jittered_back_off(5, 100, random); + JitteredExponentialBackOffStrategy jittered_back_off(5, 100, random); EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); EXPECT_EQ(9, jittered_back_off.nextBackOffMs()); EXPECT_EQ(19, jittered_back_off.nextBackOffMs()); @@ -78,7 +79,23 @@ TEST(BackOffStrategyTest, JitteredBackOffWithMaxIntervalReset) { EXPECT_EQ(99, jittered_back_off.nextBackOffMs()); } -TEST(BackOffStrategyTest, FixedBackOffBasicReset) { +TEST(LowerBoundBackOffStrategyTest, JitteredBackOffWithLowRandomValue) { + NiceMock random; + ON_CALL(random, random()).WillByDefault(Return(22)); + + JitteredLowerBoundBackOffStrategy jittered_lower_bound_back_off(500, random); + EXPECT_EQ(522, jittered_lower_bound_back_off.nextBackOffMs()); +} + +TEST(LowerBoundBackOffStrategyTest, JitteredBackOffWithHighRandomValue) { + NiceMock random; + ON_CALL(random, random()).WillByDefault(Return(9999)); + + JitteredLowerBoundBackOffStrategy jittered_lower_bound_back_off(500, random); + EXPECT_EQ(749, jittered_lower_bound_back_off.nextBackOffMs()); +} + +TEST(FixedBackOffStrategyTest, FixedBackOffBasicReset) { FixedBackOffStrategy fixed_back_off(30); EXPECT_EQ(30, fixed_back_off.nextBackOffMs()); EXPECT_EQ(30, fixed_back_off.nextBackOffMs()); diff --git a/test/common/config/utility_test.cc b/test/common/config/utility_test.cc index 23ab3e0b0235..d755486a872b 100644 --- a/test/common/config/utility_test.cc +++ b/test/common/config/utility_test.cc @@ -265,7 +265,7 @@ TEST(UtilityTest, PrepareDnsRefreshStrategy) { BackOffStrategyPtr strategy = Utility::prepareDnsRefreshStrategy(cluster, 5000, random); - EXPECT_NE(nullptr, dynamic_cast(strategy.get())); + EXPECT_NE(nullptr, dynamic_cast(strategy.get())); } { diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index dc8c831c650d..a88024a64a3b 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -499,6 +499,37 @@ invert_match: true EXPECT_FALSE(HeaderUtility::matchHeaders(unmatching_headers, header_data)); } +TEST(MatchHeadersTest, HeaderNameGetter) { + TestRequestHeaderMapImpl matching_headers{{"match-header", "value"}}; + const std::string yaml = R"EOF( +name: match-header + )EOF"; + + std::vector header_data; + header_data.push_back( + std::make_unique(parseHeaderMatcherFromYaml(yaml))); + EXPECT_TRUE(HeaderUtility::matchHeaders(matching_headers, header_data)); + + const LowerCaseString* header_name = header_data[0]->name(); + EXPECT_EQ("match-header", header_name->get()); +} + +TEST(MatchHeadersTest, HeaderNameGetterInverse) { + TestRequestHeaderMapImpl unmatching_headers{{"other-header", "value"}}; + const std::string yaml = R"EOF( +name: match-header +invert_match: true + )EOF"; + + std::vector header_data; + header_data.push_back( + std::make_unique(parseHeaderMatcherFromYaml(yaml))); + EXPECT_TRUE(HeaderUtility::matchHeaders(unmatching_headers, header_data)); + + const LowerCaseString* header_name = header_data[0]->name(); + EXPECT_EQ(nullptr, header_name); +} + TEST(HeaderIsValidTest, InvalidHeaderValuesAreRejected) { // ASCII values 1-31 are control characters (with the exception of ASCII // values 9, 10, and 13 which are a horizontal tab, line feed, and carriage diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 5d4ce7b56bc7..7c50e444425c 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -3553,6 +3553,88 @@ TEST_F(RouteMatcherTest, InvalidRetryBackOff) { "retry_policy.max_interval must greater than or equal to the base_interval"); } +TEST_F(RouteMatcherTest, RateLimitedRetryBackOff) { + const std::string yaml = R"EOF( +virtual_hosts: +- name: www + domains: + - www.lyft.com + routes: + - match: + prefix: "/no-backoff" + route: + cluster: www + - match: + prefix: "/empty-backoff" + route: + cluster: www + retry_policy: + rate_limited_retry_back_off: {} + - match: + prefix: "/sub-ms-interval" + route: + cluster: www + retry_policy: + rate_limited_retry_back_off: + reset_max_interval: 0.0001s # < 1 ms + - match: + prefix: "/typical-backoff" + route: + cluster: www + retry_policy: + rate_limited_retry_back_off: + reset_headers: + - name: Retry-After + - name: RateLimit-Reset + reset_max_interval: 0.050s + )EOF"; + + TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true); + + // has no ratelimit retry back off + EXPECT_EQ(absl::nullopt, config.route(genHeaders("www.lyft.com", "/no-backoff", "GET"), 0) + ->routeEntry() + ->retryPolicy() + .rateLimitedResetMaxInterval()); + EXPECT_EQ(0, config.route(genHeaders("www.lyft.com", "/no-backoff", "GET"), 0) + ->routeEntry() + ->retryPolicy() + .rateLimitedResetHeaders() + .size()); + + // has empty ratelimit retry back off + EXPECT_EQ(absl::nullopt, config.route(genHeaders("www.lyft.com", "/empty-backoff", "GET"), 0) + ->routeEntry() + ->retryPolicy() + .rateLimitedResetMaxInterval()); + EXPECT_EQ(0, config.route(genHeaders("www.lyft.com", "/empty-backoff", "GET"), 0) + ->routeEntry() + ->retryPolicy() + .rateLimitedResetHeaders() + .size()); + + // has sub millisecond interval + EXPECT_EQ(absl::optional(1), + config.route(genHeaders("www.lyft.com", "/sub-ms-interval", "GET"), 0) + ->routeEntry() + ->retryPolicy() + .rateLimitedResetMaxInterval()); + + // has a ratelimit retry back off + Http::TestRequestHeaderMapImpl headers = genHeaders("www.lyft.com", "/typical-backoff", "GET"); + const auto& retry_policy = config.route(headers, 0)->routeEntry()->retryPolicy(); + EXPECT_EQ(2, retry_policy.rateLimitedResetHeaders().size()); + + Http::TestResponseHeaderMapImpl expected_0{{"Retry-After", "not-used"}}; + Http::TestResponseHeaderMapImpl expected_1{{"RateLimit-Reset", "not-used"}}; + + EXPECT_TRUE(retry_policy.rateLimitedResetHeaders()[0]->matchesHeaders(expected_0)); + EXPECT_TRUE(retry_policy.rateLimitedResetHeaders()[1]->matchesHeaders(expected_1)); + + EXPECT_EQ(absl::optional(50), + retry_policy.rateLimitedResetMaxInterval()); +} + TEST_F(RouteMatcherTest, HedgeRouteLevel) { const std::string yaml = R"EOF( virtual_hosts: diff --git a/test/common/router/retry_state_impl_test.cc b/test/common/router/retry_state_impl_test.cc index 6f3d6441baaf..ba9584b5b2ba 100644 --- a/test/common/router/retry_state_impl_test.cc +++ b/test/common/router/retry_state_impl_test.cc @@ -42,7 +42,8 @@ class RouterRetryStateImplTest : public testing::Test { void setup(Http::RequestHeaderMap& request_headers) { state_ = RetryStateImpl::create(policy_, request_headers, cluster_, &virtual_cluster_, runtime_, - random_, dispatcher_, Upstream::ResourcePriority::Default); + random_, dispatcher_, test_time_.timeSystem(), + Upstream::ResourcePriority::Default); } void expectTimerCreateAndEnable() { @@ -123,6 +124,7 @@ class RouterRetryStateImplTest : public testing::Test { void TearDown() override { cleanupOutstandingResources(); } + Event::SimulatedTimeSystem test_time_; NiceMock policy_; NiceMock cluster_; TestVirtualCluster virtual_cluster_; @@ -925,6 +927,206 @@ TEST_F(RouterRetryStateImplTest, CustomBackOffIntervalDefaultMax) { retry_timer_->invokeCallback(); } +TEST_F(RouterRetryStateImplTest, ParseRateLimitResetMaxInterval) { + // No value set in via config nor headers so we get the default value + { + Http::TestRequestHeaderMapImpl request_headers{{"x-envoy-retry-on", "5xx"}}; + setup(request_headers); + EXPECT_TRUE(state_->enabled()); + + EXPECT_EQ(std::chrono::milliseconds(300000), state_->rateLimitedResetMaxInterval()); + } + + // The remaining cases expect the config to have a max_interval + policy_.ratelimited_reset_max_interval_ = absl::optional(3000); + + // Value set in config is used as fallback + { + Http::TestRequestHeaderMapImpl request_headers{{"x-envoy-retry-on", "5xx"}}; + setup(request_headers); + EXPECT_TRUE(state_->enabled()); + + EXPECT_EQ(std::chrono::milliseconds(3000), state_->rateLimitedResetMaxInterval()); + } + + // Value set in headers takes precedence over config + { + Http::TestRequestHeaderMapImpl request_headers{ + {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-max-interval-ms", "1000"}}; + setup(request_headers); + EXPECT_TRUE(state_->enabled()); + + EXPECT_EQ(std::chrono::milliseconds(1000), state_->rateLimitedResetMaxInterval()); + } +} + +TEST_F(RouterRetryStateImplTest, ParseRateLimitedResetHeaders) { + Protobuf::RepeatedPtrField matchers; + auto* matcher = matchers.Add(); + matcher->set_name("Retry-After"); + + policy_.ratelimited_reset_headers_ = Http::HeaderUtility::buildHeaderMatcherVector(matchers); + + Http::TestRequestHeaderMapImpl request_headers{ + {"x-envoy-retry-on", "5xx"}, + {"x-envoy-ratelimited-reset-headers", "RateLimit-Reset, X-RateLimit-Reset"}}; + setup(request_headers); + EXPECT_TRUE(state_->enabled()); + + Http::TestResponseHeaderMapImpl expected_0{{"Retry-After", "not-used"}}; + Http::TestResponseHeaderMapImpl expected_1{{"RateLimit-Reset", "not-used"}}; + Http::TestResponseHeaderMapImpl expected_2{{"X-RateLimit-Reset", "not-used"}}; + + EXPECT_TRUE(state_->rateLimitedResetHeaders()[0]->matchesHeaders(expected_0)); + EXPECT_TRUE(state_->rateLimitedResetHeaders()[1]->matchesHeaders(expected_1)); + EXPECT_TRUE(state_->rateLimitedResetHeaders()[2]->matchesHeaders(expected_2)); +} + +TEST_F(RouterRetryStateImplTest, ParseRateLimitedResetInterval) { + // Set a fixed system time to be used for all these tests + const time_t known_date_time = 1000000000; + test_time_.setSystemTime(std::chrono::system_clock::from_time_t(known_date_time)); + + // Failure case: The reset header name doesn't match + { + Http::TestRequestHeaderMapImpl request_headers{ + {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-headers", "RateLimit-Reset"}}; + setup(request_headers); + EXPECT_TRUE(state_->enabled()); + + Http::TestResponseHeaderMapImpl response_headers{{":status", "429"}, {"Retry-After", "2"}}; + EXPECT_EQ(absl::nullopt, state_->parseRateLimitedResetInterval(response_headers)); + } + + // Failure case: The reset header value is not a valid interval + { + Http::TestRequestHeaderMapImpl request_headers{ + {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-headers", "Retry-After"}}; + setup(request_headers); + EXPECT_TRUE(state_->enabled()); + + Http::TestResponseHeaderMapImpl response_headers{ + {":status", "429"}, {"Retry-After", "Fri, 17 Jul 2020 11:59:51 GMT"}}; + EXPECT_EQ(absl::nullopt, state_->parseRateLimitedResetInterval(response_headers)); + } + + // Failure case: The reset header value is a valid interval but out of range (5min+) + { + Http::TestRequestHeaderMapImpl request_headers{ + {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-headers", "Retry-After"}}; + setup(request_headers); + EXPECT_TRUE(state_->enabled()); + + Http::TestResponseHeaderMapImpl response_headers{{":status", "429"}, {"Retry-After", "301"}}; + EXPECT_EQ(absl::nullopt, state_->parseRateLimitedResetInterval(response_headers)); + } + + // The only reset header matches and the header value is in within range + { + Http::TestRequestHeaderMapImpl request_headers{ + {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-headers", "Retry-After"}}; + setup(request_headers); + EXPECT_TRUE(state_->enabled()); + + Http::TestResponseHeaderMapImpl response_headers{{":status", "429"}, {"Retry-After", "300"}}; + EXPECT_EQ(absl::optional(300000), + state_->parseRateLimitedResetInterval(response_headers)); + } + + // The second reset header matches and the header value is in within range + { + Http::TestRequestHeaderMapImpl request_headers{ + {"x-envoy-retry-on", "5xx"}, + {"x-envoy-ratelimited-reset-headers", "Retry-After, X-RateLimit-Reset"}}; + setup(request_headers); + EXPECT_TRUE(state_->enabled()); + + Http::TestResponseHeaderMapImpl response_headers{{":status", "429"}, + {"x-ratelimit-reset", "2"}}; + EXPECT_EQ(absl::optional(2000), + state_->parseRateLimitedResetInterval(response_headers)); + } + + // The header value is a timestamp before now() + { + Http::TestRequestHeaderMapImpl request_headers{ + {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-headers", "Retry-After"}}; + setup(request_headers); + EXPECT_TRUE(state_->enabled()); + + Http::TestResponseHeaderMapImpl response_headers{{":status", "429"}, + {"retry-after", "999999999"}}; + EXPECT_EQ(absl::nullopt, state_->parseRateLimitedResetInterval(response_headers)); + } + + // The header value is a timestamp after now() but out of range (5min+) + { + Http::TestRequestHeaderMapImpl request_headers{ + {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-headers", "Retry-After"}}; + setup(request_headers); + EXPECT_TRUE(state_->enabled()); + + Http::TestResponseHeaderMapImpl response_headers{{":status", "429"}, + {"retry-after", "1000000301"}}; + EXPECT_EQ(absl::nullopt, state_->parseRateLimitedResetInterval(response_headers)); + } + + // The header value is a timestamp after now() and within range (<=5min) + { + Http::TestRequestHeaderMapImpl request_headers{ + {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-headers", "Retry-After"}}; + setup(request_headers); + EXPECT_TRUE(state_->enabled()); + + Http::TestResponseHeaderMapImpl response_headers{{":status", "429"}, + {"retry-after", "1000000300"}}; + EXPECT_EQ(absl::optional(300000), + state_->parseRateLimitedResetInterval(response_headers)); + } +} + +TEST_F(RouterRetryStateImplTest, RateLimitedRetryBackoffStrategy) { + policy_.num_retries_ = 3; + + Http::TestRequestHeaderMapImpl request_headers{ + {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-headers", "Retry-After"}}; + setup(request_headers); + EXPECT_TRUE(state_->enabled()); + + retry_timer_ = new Event::MockTimer(&dispatcher_); + Http::TestResponseHeaderMapImpl response_headers_reset{{":status", "500"}, {"retry-after", "2"}}; + Http::TestResponseHeaderMapImpl response_headers_plain{{":status", "500"}}; + + // reset header present -> ratelimit backoff used + EXPECT_CALL(random_, random()).WillOnce(Return(190)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(2190), _)); + EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryHeaders(response_headers_reset, callback_)); + EXPECT_CALL(callback_ready_, ready()); + retry_timer_->invokeCallback(); + + // reset header present -> exponential backoff used + EXPECT_CALL(random_, random()).WillOnce(Return(190)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(15), _)); + EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryHeaders(response_headers_plain, callback_)); + EXPECT_CALL(callback_ready_, ready()); + retry_timer_->invokeCallback(); + + // reset header present -> ratelimit backoff used + EXPECT_CALL(random_, random()).WillOnce(Return(190)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(2190), _)); + EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryHeaders(response_headers_reset, callback_)); + EXPECT_CALL(callback_ready_, ready()); + retry_timer_->invokeCallback(); + + EXPECT_EQ(RetryStatus::NoRetryLimitExceeded, + state_->shouldRetryHeaders(response_headers_reset, callback_)); + + EXPECT_EQ(2UL, cluster_.stats().upstream_rq_retry_backoff_ratelimited_.value()); + EXPECT_EQ(1UL, cluster_.stats().upstream_rq_retry_backoff_exponential_.value()); + EXPECT_EQ(2UL, virtual_cluster_.stats().upstream_rq_retry_backoff_ratelimited_.value()); + EXPECT_EQ(1UL, virtual_cluster_.stats().upstream_rq_retry_backoff_exponential_.value()); +} + TEST_F(RouterRetryStateImplTest, HostSelectionAttempts) { policy_.host_selection_max_attempts_ = 2; policy_.retry_on_ = RetryPolicy::RETRY_ON_CONNECT_FAILURE; diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 0e829d87914e..6cc60aad3c01 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -71,7 +71,7 @@ class RouterTestFilter : public Filter { RetryStatePtr createRetryState(const RetryPolicy&, Http::RequestHeaderMap&, const Upstream::ClusterInfo&, const VirtualCluster*, Runtime::Loader&, Random::RandomGenerator&, Event::Dispatcher&, - Upstream::ResourcePriority) override { + TimeSource&, Upstream::ResourcePriority) override { EXPECT_EQ(nullptr, retry_state_); retry_state_ = new NiceMock(); if (reject_all_hosts_) { diff --git a/test/common/router/router_upstream_log_test.cc b/test/common/router/router_upstream_log_test.cc index 8662e760364a..42ed52b60d83 100644 --- a/test/common/router/router_upstream_log_test.cc +++ b/test/common/router/router_upstream_log_test.cc @@ -64,7 +64,7 @@ class TestFilter : public Filter { RetryStatePtr createRetryState(const RetryPolicy&, Http::RequestHeaderMap&, const Upstream::ClusterInfo&, const VirtualCluster*, Runtime::Loader&, Random::RandomGenerator&, Event::Dispatcher&, - Upstream::ResourcePriority) override { + TimeSource&, Upstream::ResourcePriority) override { EXPECT_EQ(nullptr, retry_state_); retry_state_ = new NiceMock(); return RetryStatePtr{retry_state_}; diff --git a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc index c12f94d4e99b..332f9e7651e8 100644 --- a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc +++ b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc @@ -757,7 +757,7 @@ TEST(UtilityTest, PrepareDnsRefreshStrategy) { BackOffStrategyPtr strategy = Config::Utility::prepareDnsRefreshStrategy< envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig>(dns_cache_config, 5000, random); - EXPECT_NE(nullptr, dynamic_cast(strategy.get())); + EXPECT_NE(nullptr, dynamic_cast(strategy.get())); } { diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 890a6ccef893..a79af202e548 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -119,6 +119,12 @@ class TestRetryPolicy : public RetryPolicy { absl::optional baseInterval() const override { return base_interval_; } absl::optional maxInterval() const override { return max_interval_; } + absl::optional rateLimitedResetMaxInterval() const override { + return ratelimited_reset_max_interval_; + } + const std::vector& rateLimitedResetHeaders() const override { + return ratelimited_reset_headers_; + } std::chrono::milliseconds per_try_timeout_{0}; uint32_t num_retries_{}; @@ -129,6 +135,8 @@ class TestRetryPolicy : public RetryPolicy { std::vector retriable_request_headers_; absl::optional base_interval_{}; absl::optional max_interval_{}; + std::vector ratelimited_reset_headers_; + absl::optional ratelimited_reset_max_interval_{}; }; class MockInternalRedirectPolicy : public InternalRedirectPolicy { @@ -157,6 +165,8 @@ class MockRetryState : public RetryState { void expectResetRetry(); MOCK_METHOD(bool, enabled, ()); + MOCK_METHOD(absl::optional, parseRateLimitedResetInterval, + (const Http::ResponseHeaderMap& response_headers), (const)); MOCK_METHOD(RetryStatus, shouldRetryHeaders, (const Http::ResponseHeaderMap& response_headers, DoRetryCallback callback)); MOCK_METHOD(bool, wouldRetryFromHeaders, (const Http::ResponseHeaderMap& response_headers)); @@ -169,6 +179,9 @@ class MockRetryState : public RetryState { (const Upstream::PrioritySet&, const Upstream::HealthyAndDegradedLoad&, const Upstream::RetryPriority::PriorityMappingFunc&)); MOCK_METHOD(uint32_t, hostSelectionMaxAttempts, (), (const)); + MOCK_METHOD(const std::vector&, rateLimitedResetHeaders, (), + (const)); + MOCK_METHOD(std::chrono::milliseconds, rateLimitedResetMaxInterval, (), (const)); DoRetryCallback callback_; }; From c1889365f09c440a8772b24e2cedd636a698e590 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Tue, 21 Jul 2020 20:48:43 +1000 Subject: [PATCH 02/35] Update docs to mention the default value. Signed-off-by: Martin Matusiak --- api/envoy/config/route/v3/route_components.proto | 2 +- api/envoy/config/route/v4alpha/route_components.proto | 2 +- .../envoy/config/route/v3/route_components.proto | 2 +- .../envoy/config/route/v4alpha/route_components.proto | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index 49df7fd13593..fd2fb49949c7 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1102,7 +1102,7 @@ message RetryPolicy { // Specifies the maximum rate limited retry back off interval that Envoy // will allow. If a rate limited reset header contains an interval longer // than this then the default exponential back off strategy will be used - // instead. + // instead. Defaults to 300 seconds. google.protobuf.Duration reset_max_interval = 2 [(validate.rules).duration = {gt {}}]; } diff --git a/api/envoy/config/route/v4alpha/route_components.proto b/api/envoy/config/route/v4alpha/route_components.proto index ef08c64b129e..08ffea41bd98 100644 --- a/api/envoy/config/route/v4alpha/route_components.proto +++ b/api/envoy/config/route/v4alpha/route_components.proto @@ -1083,7 +1083,7 @@ message RetryPolicy { // Specifies the maximum rate limited retry back off interval that Envoy // will allow. If a rate limited reset header contains an interval longer // than this then the default exponential back off strategy will be used - // instead. + // instead. Defaults to 300 seconds. google.protobuf.Duration reset_max_interval = 2 [(validate.rules).duration = {gt {}}]; } diff --git a/generated_api_shadow/envoy/config/route/v3/route_components.proto b/generated_api_shadow/envoy/config/route/v3/route_components.proto index 8e32d9b21886..bcca81f3a74e 100644 --- a/generated_api_shadow/envoy/config/route/v3/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v3/route_components.proto @@ -1109,7 +1109,7 @@ message RetryPolicy { // Specifies the maximum rate limited retry back off interval that Envoy // will allow. If a rate limited reset header contains an interval longer // than this then the default exponential back off strategy will be used - // instead. + // instead. Defaults to 300 seconds. google.protobuf.Duration reset_max_interval = 2 [(validate.rules).duration = {gt {}}]; } diff --git a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto index 7b7c98d07b7d..6e3222c11567 100644 --- a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto @@ -1111,7 +1111,7 @@ message RetryPolicy { // Specifies the maximum rate limited retry back off interval that Envoy // will allow. If a rate limited reset header contains an interval longer // than this then the default exponential back off strategy will be used - // instead. + // instead. Defaults to 300 seconds. google.protobuf.Duration reset_max_interval = 2 [(validate.rules).duration = {gt {}}]; } From dcc4e1456b015ef23967b4b1335ab7f36abf73ab Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Tue, 21 Jul 2020 20:49:33 +1000 Subject: [PATCH 03/35] Make the spell checker happy Signed-off-by: Martin Matusiak --- api/envoy/config/route/v3/route_components.proto | 4 ++-- api/envoy/config/route/v4alpha/route_components.proto | 4 ++-- .../envoy/config/route/v3/route_components.proto | 4 ++-- .../envoy/config/route/v4alpha/route_components.proto | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index fd2fb49949c7..b61090fe66ef 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1094,8 +1094,8 @@ message RetryPolicy { // of the header will be used as the retry back off interval, with jitter added. // // The value of the header must be either: - // 1. An interval in seconds as an integer, eg. ``120``. - // 2. A unix timestamp as an integer, eg. ``1595320702``. The timestamp + // 1. An interval in seconds as an integer, for example: ``120``. + // 2. A unix timestamp as an integer, for example: ``1595320702``. The timestamp // must be in the future relative to the current system time. repeated HeaderMatcher reset_headers = 1; diff --git a/api/envoy/config/route/v4alpha/route_components.proto b/api/envoy/config/route/v4alpha/route_components.proto index 08ffea41bd98..a2d980bcdf77 100644 --- a/api/envoy/config/route/v4alpha/route_components.proto +++ b/api/envoy/config/route/v4alpha/route_components.proto @@ -1075,8 +1075,8 @@ message RetryPolicy { // of the header will be used as the retry back off interval, with jitter added. // // The value of the header must be either: - // 1. An interval in seconds as an integer, eg. ``120``. - // 2. A unix timestamp as an integer, eg. ``1595320702``. The timestamp + // 1. An interval in seconds as an integer, for example: ``120``. + // 2. A unix timestamp as an integer, for example: ``1595320702``. The timestamp // must be in the future relative to the current system time. repeated HeaderMatcher reset_headers = 1; diff --git a/generated_api_shadow/envoy/config/route/v3/route_components.proto b/generated_api_shadow/envoy/config/route/v3/route_components.proto index bcca81f3a74e..43d85ebec3de 100644 --- a/generated_api_shadow/envoy/config/route/v3/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v3/route_components.proto @@ -1101,8 +1101,8 @@ message RetryPolicy { // of the header will be used as the retry back off interval, with jitter added. // // The value of the header must be either: - // 1. An interval in seconds as an integer, eg. ``120``. - // 2. A unix timestamp as an integer, eg. ``1595320702``. The timestamp + // 1. An interval in seconds as an integer, for example: ``120``. + // 2. A unix timestamp as an integer, for example: ``1595320702``. The timestamp // must be in the future relative to the current system time. repeated HeaderMatcher reset_headers = 1; diff --git a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto index 6e3222c11567..b20623a73a96 100644 --- a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto @@ -1103,8 +1103,8 @@ message RetryPolicy { // of the header will be used as the retry back off interval, with jitter added. // // The value of the header must be either: - // 1. An interval in seconds as an integer, eg. ``120``. - // 2. A unix timestamp as an integer, eg. ``1595320702``. The timestamp + // 1. An interval in seconds as an integer, for example: ``120``. + // 2. A unix timestamp as an integer, for example: ``1595320702``. The timestamp // must be in the future relative to the current system time. repeated HeaderMatcher reset_headers = 1; From 69088979ba452d6a1d28258f80a7a8744bcda114 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Wed, 22 Jul 2020 15:09:15 +1000 Subject: [PATCH 04/35] Update version history Signed-off-by: Martin Matusiak --- docs/root/version_history/current.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 61466687aa9d..77c3ff27b6bf 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -71,6 +71,8 @@ New Features ` strategy that uses headers like `Retry-After` or `X-RateLimit-Reset` to decide the back off interval. +* stats: added optional histograms to :ref:`cluster stats ` + that track headers and body sizes of requests and responses. * tap: added :ref:`generic body matcher` to scan http requests and responses for text or hex patterns. * tcp: switched the TCP connection pool to the new "shared" connection pool, sharing a common code base with HTTP and HTTP/2. Any unexpected behavioral changes can be temporarily reverted by setting `envoy.reloadable_features.new_tcp_connection_pool` to false. * watchdog: support randomizing the watchdog's kill timeout to prevent synchronized kills via a maximium jitter parameter :ref:`max_kill_timeout_jitter`. From 5cabd3cbda37f48b41eaa61698f5ed607b3f3a03 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Wed, 22 Jul 2020 15:28:14 +1000 Subject: [PATCH 05/35] Make sure the new headers are consumed Signed-off-by: Martin Matusiak --- source/common/router/retry_state_impl.cc | 2 ++ test/common/router/retry_state_impl_test.cc | 16 ++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc index a19355ac8513..1348b1bb530c 100644 --- a/source/common/router/retry_state_impl.cc +++ b/source/common/router/retry_state_impl.cc @@ -52,6 +52,8 @@ RetryStatePtr RetryStateImpl::create(const RetryPolicy& route_policy, request_headers.removeEnvoyRetryOn(); request_headers.removeEnvoyRetryGrpcOn(); request_headers.removeEnvoyMaxRetries(); + request_headers.removeEnvoyRateLimitedResetHeaders(); + request_headers.removeEnvoyRateLimitedResetMaxIntervalMs(); if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.consume_all_retry_headers")) { request_headers.removeEnvoyHedgeOnPerTryTimeout(); request_headers.removeEnvoyRetriableHeaderNames(); diff --git a/test/common/router/retry_state_impl_test.cc b/test/common/router/retry_state_impl_test.cc index ba9584b5b2ba..f138b36414fe 100644 --- a/test/common/router/retry_state_impl_test.cc +++ b/test/common/router/retry_state_impl_test.cc @@ -1346,6 +1346,8 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { Http::TestRequestHeaderMapImpl request_headers{ {"x-envoy-retry-on", "5xx,retriable-header-names,retriable-status-codes"}, {"x-envoy-retry-grpc-on", "resource-exhausted"}, + {"x-envoy-ratelimited-reset-headers", "Retry-After"}, + {"x-envoy-ratelimited-reset-max-interval-ms", "1000"}, {"x-envoy-retriable-header-names", "X-Upstream-Pushback"}, {"x-envoy-retriable-status-codes", "418,420"}, {"x-envoy-max-retries", "7"}, @@ -1358,6 +1360,8 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { EXPECT_FALSE(request_headers.has("x-envoy-retry-on")); EXPECT_FALSE(request_headers.has("x-envoy-retry-grpc-on")); EXPECT_FALSE(request_headers.has("x-envoy-max-retries")); + EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-headers")); + EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-max-interval-ms")); EXPECT_FALSE(request_headers.has("x-envoy-retriable-header-names")); EXPECT_FALSE(request_headers.has("x-envoy-retriable-status-codes")); EXPECT_FALSE(request_headers.has("x-envoy-hedge-on-per-try-timeout")); @@ -1367,6 +1371,8 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { // Make sure retry related headers are removed even if the policy is disabled. { Http::TestRequestHeaderMapImpl request_headers{ + {"x-envoy-ratelimited-reset-headers", "Retry-After"}, + {"x-envoy-ratelimited-reset-max-interval-ms", "1000"}, {"x-envoy-retriable-header-names", "X-Upstream-Pushback"}, {"x-envoy-retriable-status-codes", "418,420"}, {"x-envoy-max-retries", "7"}, @@ -1379,6 +1385,8 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { EXPECT_FALSE(request_headers.has("x-envoy-retry-on")); EXPECT_FALSE(request_headers.has("x-envoy-retry-grpc-on")); EXPECT_FALSE(request_headers.has("x-envoy-max-retries")); + EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-headers")); + EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-max-interval-ms")); EXPECT_FALSE(request_headers.has("x-envoy-retriable-header-names")); EXPECT_FALSE(request_headers.has("x-envoy-retriable-status-codes")); EXPECT_FALSE(request_headers.has("x-envoy-hedge-on-per-try-timeout")); @@ -1394,6 +1402,8 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { Http::TestRequestHeaderMapImpl request_headers{ {"x-envoy-retry-on", "5xx,retriable-header-names,retriable-status-codes"}, {"x-envoy-retry-grpc-on", "resource-exhausted"}, + {"x-envoy-ratelimited-reset-headers", "Retry-After"}, + {"x-envoy-ratelimited-reset-max-interval-ms", "1000"}, {"x-envoy-retriable-header-names", "X-Upstream-Pushback"}, {"x-envoy-retriable-status-codes", "418,420"}, {"x-envoy-max-retries", "7"}, @@ -1406,6 +1416,8 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { EXPECT_FALSE(request_headers.has("x-envoy-retry-on")); EXPECT_FALSE(request_headers.has("x-envoy-retry-grpc-on")); EXPECT_FALSE(request_headers.has("x-envoy-max-retries")); + EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-headers")); + EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-max-interval-ms")); EXPECT_TRUE(request_headers.has("x-envoy-retriable-header-names")); EXPECT_TRUE(request_headers.has("x-envoy-retriable-status-codes")); EXPECT_TRUE(request_headers.has("x-envoy-hedge-on-per-try-timeout")); @@ -1419,6 +1431,8 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { {{"envoy.reloadable_features.consume_all_retry_headers", "false"}}); Http::TestRequestHeaderMapImpl request_headers{ + {"x-envoy-ratelimited-reset-headers", "Retry-After"}, + {"x-envoy-ratelimited-reset-max-interval-ms", "1000"}, {"x-envoy-retriable-header-names", "X-Upstream-Pushback"}, {"x-envoy-retriable-status-codes", "418,420"}, {"x-envoy-max-retries", "7"}, @@ -1431,6 +1445,8 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { EXPECT_FALSE(request_headers.has("x-envoy-retry-on")); EXPECT_FALSE(request_headers.has("x-envoy-retry-grpc-on")); EXPECT_FALSE(request_headers.has("x-envoy-max-retries")); + EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-headers")); + EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-max-interval-ms")); EXPECT_TRUE(request_headers.has("x-envoy-retriable-header-names")); EXPECT_TRUE(request_headers.has("x-envoy-retriable-status-codes")); EXPECT_TRUE(request_headers.has("x-envoy-hedge-on-per-try-timeout")); From 902f17cdd43d07f6a313cef47cb4f8e0b7614a1b Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Wed, 22 Jul 2020 15:40:16 +1000 Subject: [PATCH 06/35] Remove unnecessary import Signed-off-by: Martin Matusiak --- include/envoy/http/header_map.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 22216aa12ea5..0866c34e9ae2 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -16,7 +16,6 @@ #include "absl/container/inlined_vector.h" #include "absl/strings/string_view.h" -#include "absl/types/optional.h" namespace Envoy { namespace Http { From 40e7b0ac3c91e5f54fe6f8e5213368087d755fed Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Wed, 22 Jul 2020 17:14:47 +1000 Subject: [PATCH 07/35] Add missing break statement Add missing break statement and add test documenting the behavior of using the first header that matches Signed-off-by: Martin Matusiak --- source/common/router/retry_state_impl.cc | 1 + test/common/router/retry_state_impl_test.cc | 14 ++++++++++++++ 2 files changed, 15 insertions(+) diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc index 1348b1bb530c..f47015a4005b 100644 --- a/source/common/router/retry_state_impl.cc +++ b/source/common/router/retry_state_impl.cc @@ -281,6 +281,7 @@ absl::optional RetryStateImpl::parseRateLimitedResetI // Try to parse the value of the header as an int storing the number of seconds if (absl::SimpleAtoi(header_value, &num_seconds)) { parsed_value = true; + break; } } } diff --git a/test/common/router/retry_state_impl_test.cc b/test/common/router/retry_state_impl_test.cc index f138b36414fe..27775717021c 100644 --- a/test/common/router/retry_state_impl_test.cc +++ b/test/common/router/retry_state_impl_test.cc @@ -1047,6 +1047,20 @@ TEST_F(RouterRetryStateImplTest, ParseRateLimitedResetInterval) { state_->parseRateLimitedResetInterval(response_headers)); } + // The second and third reset header match and the first of these two is used + { + Http::TestRequestHeaderMapImpl request_headers{ + {"x-envoy-retry-on", "5xx"}, + {"x-envoy-ratelimited-reset-headers", "Retry-After,X-RateLimit-Reset,X-Retry-After"}}; + setup(request_headers); + EXPECT_TRUE(state_->enabled()); + + Http::TestResponseHeaderMapImpl response_headers{ + {":status", "429"}, {"x-ratelimit-reset", "2"}, {"X-Retry-After", "3"}}; + EXPECT_EQ(absl::optional(2000), + state_->parseRateLimitedResetInterval(response_headers)); + } + // The header value is a timestamp before now() { Http::TestRequestHeaderMapImpl request_headers{ From c926c9cbd376d9e93dfcc06db2b1a1c092b3025c Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Wed, 22 Jul 2020 17:28:28 +1000 Subject: [PATCH 08/35] Document that first matching header is used Signed-off-by: Martin Matusiak --- api/envoy/config/route/v3/route_components.proto | 4 +++- api/envoy/config/route/v4alpha/route_components.proto | 4 +++- .../envoy/config/route/v3/route_components.proto | 4 +++- .../envoy/config/route/v4alpha/route_components.proto | 4 +++- 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index b61090fe66ef..1180c6bd0395 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1091,7 +1091,9 @@ message RetryPolicy { // Specifies the rate limited retry reset headers (like ``Retry-After`` or // ``X-RateLimit-Reset``) to match against the response. If a request is // being retried and the response contains one of these headers the value - // of the header will be used as the retry back off interval, with jitter added. + // of the header will be used as the retry back off interval, with jitter + // added. If multiple headers are specified they are tried in the order + // given and the first matching header in the response is used. // // The value of the header must be either: // 1. An interval in seconds as an integer, for example: ``120``. diff --git a/api/envoy/config/route/v4alpha/route_components.proto b/api/envoy/config/route/v4alpha/route_components.proto index a2d980bcdf77..e228c271640d 100644 --- a/api/envoy/config/route/v4alpha/route_components.proto +++ b/api/envoy/config/route/v4alpha/route_components.proto @@ -1072,7 +1072,9 @@ message RetryPolicy { // Specifies the rate limited retry reset headers (like ``Retry-After`` or // ``X-RateLimit-Reset``) to match against the response. If a request is // being retried and the response contains one of these headers the value - // of the header will be used as the retry back off interval, with jitter added. + // of the header will be used as the retry back off interval, with jitter + // added. If multiple headers are specified they are tried in the order + // given and the first matching header in the response is used. // // The value of the header must be either: // 1. An interval in seconds as an integer, for example: ``120``. diff --git a/generated_api_shadow/envoy/config/route/v3/route_components.proto b/generated_api_shadow/envoy/config/route/v3/route_components.proto index 43d85ebec3de..392f4dcc6884 100644 --- a/generated_api_shadow/envoy/config/route/v3/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v3/route_components.proto @@ -1098,7 +1098,9 @@ message RetryPolicy { // Specifies the rate limited retry reset headers (like ``Retry-After`` or // ``X-RateLimit-Reset``) to match against the response. If a request is // being retried and the response contains one of these headers the value - // of the header will be used as the retry back off interval, with jitter added. + // of the header will be used as the retry back off interval, with jitter + // added. If multiple headers are specified they are tried in the order + // given and the first matching header in the response is used. // // The value of the header must be either: // 1. An interval in seconds as an integer, for example: ``120``. diff --git a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto index b20623a73a96..8769968c772a 100644 --- a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto @@ -1100,7 +1100,9 @@ message RetryPolicy { // Specifies the rate limited retry reset headers (like ``Retry-After`` or // ``X-RateLimit-Reset``) to match against the response. If a request is // being retried and the response contains one of these headers the value - // of the header will be used as the retry back off interval, with jitter added. + // of the header will be used as the retry back off interval, with jitter + // added. If multiple headers are specified they are tried in the order + // given and the first matching header in the response is used. // // The value of the header must be either: // 1. An interval in seconds as an integer, for example: ``120``. From 89460445bad96682a6abb93e55d0d7f357645f03 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Wed, 22 Jul 2020 18:03:11 +1000 Subject: [PATCH 09/35] Make sure there is at least one header Signed-off-by: Martin Matusiak --- api/envoy/config/route/v3/route_components.proto | 2 +- api/envoy/config/route/v4alpha/route_components.proto | 2 +- .../envoy/config/route/v3/route_components.proto | 2 +- .../envoy/config/route/v4alpha/route_components.proto | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index 1180c6bd0395..f0dfcc74712f 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1099,7 +1099,7 @@ message RetryPolicy { // 1. An interval in seconds as an integer, for example: ``120``. // 2. A unix timestamp as an integer, for example: ``1595320702``. The timestamp // must be in the future relative to the current system time. - repeated HeaderMatcher reset_headers = 1; + repeated HeaderMatcher reset_headers = 1 [(validate.rules).repeated = {min_items: 1}]; // Specifies the maximum rate limited retry back off interval that Envoy // will allow. If a rate limited reset header contains an interval longer diff --git a/api/envoy/config/route/v4alpha/route_components.proto b/api/envoy/config/route/v4alpha/route_components.proto index e228c271640d..06e061815f9a 100644 --- a/api/envoy/config/route/v4alpha/route_components.proto +++ b/api/envoy/config/route/v4alpha/route_components.proto @@ -1080,7 +1080,7 @@ message RetryPolicy { // 1. An interval in seconds as an integer, for example: ``120``. // 2. A unix timestamp as an integer, for example: ``1595320702``. The timestamp // must be in the future relative to the current system time. - repeated HeaderMatcher reset_headers = 1; + repeated HeaderMatcher reset_headers = 1 [(validate.rules).repeated = {min_items: 1}]; // Specifies the maximum rate limited retry back off interval that Envoy // will allow. If a rate limited reset header contains an interval longer diff --git a/generated_api_shadow/envoy/config/route/v3/route_components.proto b/generated_api_shadow/envoy/config/route/v3/route_components.proto index 392f4dcc6884..3880bafc6ee1 100644 --- a/generated_api_shadow/envoy/config/route/v3/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v3/route_components.proto @@ -1106,7 +1106,7 @@ message RetryPolicy { // 1. An interval in seconds as an integer, for example: ``120``. // 2. A unix timestamp as an integer, for example: ``1595320702``. The timestamp // must be in the future relative to the current system time. - repeated HeaderMatcher reset_headers = 1; + repeated HeaderMatcher reset_headers = 1 [(validate.rules).repeated = {min_items: 1}]; // Specifies the maximum rate limited retry back off interval that Envoy // will allow. If a rate limited reset header contains an interval longer diff --git a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto index 8769968c772a..a77e63d7d0b2 100644 --- a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto @@ -1108,7 +1108,7 @@ message RetryPolicy { // 1. An interval in seconds as an integer, for example: ``120``. // 2. A unix timestamp as an integer, for example: ``1595320702``. The timestamp // must be in the future relative to the current system time. - repeated HeaderMatcher reset_headers = 1; + repeated HeaderMatcher reset_headers = 1 [(validate.rules).repeated = {min_items: 1}]; // Specifies the maximum rate limited retry back off interval that Envoy // will allow. If a rate limited reset header contains an interval longer From 0f5eb625004b43696aa6c68fa79400219ab0963f Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Thu, 23 Jul 2020 06:58:32 +1000 Subject: [PATCH 10/35] Update test to agree with protobuf Signed-off-by: Martin Matusiak --- test/common/router/config_impl_test.cc | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 7c50e444425c..557c6e2ffa4b 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -3564,18 +3564,14 @@ TEST_F(RouteMatcherTest, RateLimitedRetryBackOff) { prefix: "/no-backoff" route: cluster: www - - match: - prefix: "/empty-backoff" - route: - cluster: www - retry_policy: - rate_limited_retry_back_off: {} - match: prefix: "/sub-ms-interval" route: cluster: www retry_policy: rate_limited_retry_back_off: + reset_headers: + - name: Retry-After reset_max_interval: 0.0001s # < 1 ms - match: prefix: "/typical-backoff" @@ -3602,17 +3598,6 @@ TEST_F(RouteMatcherTest, RateLimitedRetryBackOff) { .rateLimitedResetHeaders() .size()); - // has empty ratelimit retry back off - EXPECT_EQ(absl::nullopt, config.route(genHeaders("www.lyft.com", "/empty-backoff", "GET"), 0) - ->routeEntry() - ->retryPolicy() - .rateLimitedResetMaxInterval()); - EXPECT_EQ(0, config.route(genHeaders("www.lyft.com", "/empty-backoff", "GET"), 0) - ->routeEntry() - ->retryPolicy() - .rateLimitedResetHeaders() - .size()); - // has sub millisecond interval EXPECT_EQ(absl::optional(1), config.route(genHeaders("www.lyft.com", "/sub-ms-interval", "GET"), 0) From c96500886713cd5bfec938a5dcf12cf884b77883 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Thu, 23 Jul 2020 08:47:06 +1000 Subject: [PATCH 11/35] Update version history Signed-off-by: Martin Matusiak --- docs/root/version_history/current.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 77c3ff27b6bf..52d2bd9774b4 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -60,6 +60,12 @@ New Features * lua: added Lua APIs to access :ref:`SSL connection info ` object. * postgres network filter: :ref:`metadata ` is produced based on SQL query. * ratelimit: added :ref:`enable_x_ratelimit_headers ` option to enable `X-RateLimit-*` headers as defined in `draft RFC `_. +* ratelimit: added :ref:`enable_x_ratelimit_headers ` option to enable `X-RateLimit-*` headers as defined in `draft RFC `_. +* redis: added fault injection support :ref:`fault injection for redis proxy `, described further in :ref:`configuration documentation `. +* router: added a new :ref:`rate limited retry back off + ` + strategy that uses headers like `Retry-After` or `X-RateLimit-Reset` to + decide the back off interval. * router: added new :ref:`envoy-ratelimited` retry policy, which allows retrying envoy's own rate limited responses. From d2ae1a087160d83d68a1ac9aeb74ca16fab8b608 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Thu, 23 Jul 2020 11:28:57 +1000 Subject: [PATCH 12/35] Clang tidy prefers I use empty() Signed-off-by: Martin Matusiak --- source/common/router/retry_state_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc index f47015a4005b..696bc3cfaf2d 100644 --- a/source/common/router/retry_state_impl.cc +++ b/source/common/router/retry_state_impl.cc @@ -373,7 +373,7 @@ RetryStatus RetryStateImpl::shouldRetryHeaders(const Http::ResponseHeaderMap& re // Yes, we will retry based on the headers - try to parse a rate limited reset interval from the // response. - if (would_retry && ratelimited_reset_headers_.size() > 0) { + if (would_retry && !ratelimited_reset_headers_.empty()) { const auto backoff_interval = parseRateLimitedResetInterval(response_headers); if (backoff_interval.has_value()) { ratelimited_backoff_strategy_ = std::make_unique( From 96e5364c6d77bb8d3da1e0de155ab21bacda424b Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Thu, 23 Jul 2020 18:22:52 +1000 Subject: [PATCH 13/35] Move default to RetryPolicy Signed-off-by: Martin Matusiak --- include/envoy/router/router.h | 2 +- source/common/http/async_client_impl.h | 4 ++-- source/common/router/config_impl.cc | 10 +++++++--- source/common/router/config_impl.h | 4 ++-- source/common/router/retry_state_impl.cc | 5 ++--- source/common/router/retry_state_impl.h | 2 +- test/common/router/config_impl_test.cc | 14 +++++++------- test/common/router/retry_state_impl_test.cc | 4 ++-- test/mocks/router/mocks.h | 4 ++-- 9 files changed, 26 insertions(+), 23 deletions(-) diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 71fcbdba6c68..754714d068bb 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -246,7 +246,7 @@ class RetryPolicy { * @return absl::optional limit placed on a rate limited retry * interval. */ - virtual absl::optional rateLimitedResetMaxInterval() const PURE; + virtual std::chrono::milliseconds rateLimitedResetMaxInterval() const PURE; }; /** diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index c7cb930306a3..9b794edb4b9a 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -155,8 +155,8 @@ class AsyncStreamImpl : public AsyncClient::Stream, const std::vector& rateLimitedResetHeaders() const override { return ratelimited_reset_headers_; } - absl::optional rateLimitedResetMaxInterval() const override { - return absl::nullopt; + std::chrono::milliseconds rateLimitedResetMaxInterval() const override { + return std::chrono::milliseconds(300000); } const std::vector retriable_status_codes_{}; diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index fc644a518eb0..ff898e101aae 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -125,10 +125,14 @@ RetryPolicyImpl::RetryPolicyImpl(const envoy::config::route::v3::RetryPolicy& re ratelimited_reset_headers_ = Http::HeaderUtility::buildHeaderMatcherVector( retry_policy.rate_limited_retry_back_off().reset_headers()); - ratelimited_reset_max_interval_ = + absl::optional ratelimited_reset_max_interval = PROTOBUF_GET_OPTIONAL_MS(retry_policy.rate_limited_retry_back_off(), reset_max_interval); - if (ratelimited_reset_max_interval_ && ((*ratelimited_reset_max_interval_).count()) < 1) { - ratelimited_reset_max_interval_ = std::chrono::milliseconds(1); + if (ratelimited_reset_max_interval.has_value()) { + std::chrono::milliseconds max_interval = ratelimited_reset_max_interval.value(); + if (max_interval.count() < 1) { + max_interval = std::chrono::milliseconds(1); + } + ratelimited_reset_max_interval_ = max_interval; } } } diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 206c9e37aa41..79ca406567a1 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -275,7 +275,7 @@ class RetryPolicyImpl : public RetryPolicy { const std::vector& rateLimitedResetHeaders() const override { return ratelimited_reset_headers_; } - absl::optional rateLimitedResetMaxInterval() const override { + std::chrono::milliseconds rateLimitedResetMaxInterval() const override { return ratelimited_reset_max_interval_; } @@ -301,7 +301,7 @@ class RetryPolicyImpl : public RetryPolicy { absl::optional base_interval_; absl::optional max_interval_; std::vector ratelimited_reset_headers_{}; - absl::optional ratelimited_reset_max_interval_; + std::chrono::milliseconds ratelimited_reset_max_interval_{300000}; ProtobufMessage::ValidationVisitor* validation_visitor_{}; }; diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc index 696bc3cfaf2d..5c6facc127fa 100644 --- a/source/common/router/retry_state_impl.cc +++ b/source/common/router/retry_state_impl.cc @@ -77,7 +77,8 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, retry_priority_(route_policy.retryPriority()), retriable_status_codes_(route_policy.retriableStatusCodes()), retriable_headers_(route_policy.retriableHeaders()), - ratelimited_reset_headers_(route_policy.rateLimitedResetHeaders()) { + ratelimited_reset_headers_(route_policy.rateLimitedResetHeaders()), + ratelimited_reset_max_interval_(route_policy.rateLimitedResetMaxInterval()) { std::chrono::milliseconds base_interval( runtime_.snapshot().getInteger("upstream.base_retry_backoff_ms", 25)); @@ -159,8 +160,6 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, if (absl::SimpleAtoi(max_interval, &out)) { ratelimited_reset_max_interval_ = std::chrono::milliseconds(out); } - } else if (route_policy.rateLimitedResetMaxInterval().has_value()) { - ratelimited_reset_max_interval_ = route_policy.rateLimitedResetMaxInterval().value(); } if (request_headers.EnvoyRateLimitedResetHeaders()) { diff --git a/source/common/router/retry_state_impl.h b/source/common/router/retry_state_impl.h index 72949aa63938..6808171b43b4 100644 --- a/source/common/router/retry_state_impl.h +++ b/source/common/router/retry_state_impl.h @@ -130,7 +130,7 @@ class RetryStateImpl : public RetryState { std::vector retriable_status_codes_; std::vector retriable_headers_; std::vector ratelimited_reset_headers_{}; - std::chrono::milliseconds ratelimited_reset_max_interval_{300000}; + std::chrono::milliseconds ratelimited_reset_max_interval_{}; }; } // namespace Router diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 557c6e2ffa4b..aab5da6e2a86 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -3588,10 +3588,11 @@ TEST_F(RouteMatcherTest, RateLimitedRetryBackOff) { TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true); // has no ratelimit retry back off - EXPECT_EQ(absl::nullopt, config.route(genHeaders("www.lyft.com", "/no-backoff", "GET"), 0) - ->routeEntry() - ->retryPolicy() - .rateLimitedResetMaxInterval()); + EXPECT_EQ(std::chrono::milliseconds(300000), + config.route(genHeaders("www.lyft.com", "/no-backoff", "GET"), 0) + ->routeEntry() + ->retryPolicy() + .rateLimitedResetMaxInterval()); EXPECT_EQ(0, config.route(genHeaders("www.lyft.com", "/no-backoff", "GET"), 0) ->routeEntry() ->retryPolicy() @@ -3599,7 +3600,7 @@ TEST_F(RouteMatcherTest, RateLimitedRetryBackOff) { .size()); // has sub millisecond interval - EXPECT_EQ(absl::optional(1), + EXPECT_EQ(std::chrono::milliseconds(1), config.route(genHeaders("www.lyft.com", "/sub-ms-interval", "GET"), 0) ->routeEntry() ->retryPolicy() @@ -3616,8 +3617,7 @@ TEST_F(RouteMatcherTest, RateLimitedRetryBackOff) { EXPECT_TRUE(retry_policy.rateLimitedResetHeaders()[0]->matchesHeaders(expected_0)); EXPECT_TRUE(retry_policy.rateLimitedResetHeaders()[1]->matchesHeaders(expected_1)); - EXPECT_EQ(absl::optional(50), - retry_policy.rateLimitedResetMaxInterval()); + EXPECT_EQ(std::chrono::milliseconds(50), retry_policy.rateLimitedResetMaxInterval()); } TEST_F(RouteMatcherTest, HedgeRouteLevel) { diff --git a/test/common/router/retry_state_impl_test.cc b/test/common/router/retry_state_impl_test.cc index 27775717021c..49daa7172a7b 100644 --- a/test/common/router/retry_state_impl_test.cc +++ b/test/common/router/retry_state_impl_test.cc @@ -938,7 +938,7 @@ TEST_F(RouterRetryStateImplTest, ParseRateLimitResetMaxInterval) { } // The remaining cases expect the config to have a max_interval - policy_.ratelimited_reset_max_interval_ = absl::optional(3000); + policy_.ratelimited_reset_max_interval_ = std::chrono::milliseconds(4000); // Value set in config is used as fallback { @@ -946,7 +946,7 @@ TEST_F(RouterRetryStateImplTest, ParseRateLimitResetMaxInterval) { setup(request_headers); EXPECT_TRUE(state_->enabled()); - EXPECT_EQ(std::chrono::milliseconds(3000), state_->rateLimitedResetMaxInterval()); + EXPECT_EQ(std::chrono::milliseconds(4000), state_->rateLimitedResetMaxInterval()); } // Value set in headers takes precedence over config diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index a79af202e548..07d62ee0d2c1 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -119,7 +119,7 @@ class TestRetryPolicy : public RetryPolicy { absl::optional baseInterval() const override { return base_interval_; } absl::optional maxInterval() const override { return max_interval_; } - absl::optional rateLimitedResetMaxInterval() const override { + std::chrono::milliseconds rateLimitedResetMaxInterval() const override { return ratelimited_reset_max_interval_; } const std::vector& rateLimitedResetHeaders() const override { @@ -136,7 +136,7 @@ class TestRetryPolicy : public RetryPolicy { absl::optional base_interval_{}; absl::optional max_interval_{}; std::vector ratelimited_reset_headers_; - absl::optional ratelimited_reset_max_interval_{}; + std::chrono::milliseconds ratelimited_reset_max_interval_{300000}; }; class MockInternalRedirectPolicy : public InternalRedirectPolicy { From 97fcc0b2c4fcfdf7dc717be2a90266a74feea853 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Wed, 29 Jul 2020 16:47:01 +1000 Subject: [PATCH 14/35] Remove new stats from virtual cluster Signed-off-by: Martin Matusiak --- docs/root/configuration/http/http_filters/router_filter.rst | 2 -- include/envoy/router/router.h | 2 -- source/common/router/retry_state_impl.cc | 6 ------ test/common/router/retry_state_impl_test.cc | 2 -- 4 files changed, 12 deletions(-) diff --git a/docs/root/configuration/http/http_filters/router_filter.rst b/docs/root/configuration/http/http_filters/router_filter.rst index b8a9e895a858..6abc596627fc 100644 --- a/docs/root/configuration/http/http_filters/router_filter.rst +++ b/docs/root/configuration/http/http_filters/router_filter.rst @@ -425,8 +425,6 @@ statistics: upstream_rq_<\*xx>, Counter, "Aggregate HTTP response codes (e.g., 2xx, 3xx, etc.)" upstream_rq_<\*>, Counter, "Specific HTTP response codes (e.g., 201, 302, etc.)" upstream_rq_retry, Counter, Total request retries - upstream_rq_retry_backoff_exponential, Counter, Total retries using the exponential backoff strategy - upstream_rq_retry_backoff_ratelimited, Counter, Total retries using the ratelimited backoff strategy upstream_rq_retry_limit_exceeded, Counter, Total requests not retried due to exceeding :ref:`the configured number of maximum retries ` upstream_rq_retry_overflow, Counter, Total requests not retried due to circuit breaking or exceeding the :ref:`retry budgets ` upstream_rq_retry_success, Counter, Total request retry successes diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 754714d068bb..5549c9416468 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -448,8 +448,6 @@ using ShadowPolicyPtr = std::unique_ptr; */ #define ALL_VIRTUAL_CLUSTER_STATS(COUNTER) \ COUNTER(upstream_rq_retry) \ - COUNTER(upstream_rq_retry_backoff_exponential) \ - COUNTER(upstream_rq_retry_backoff_ratelimited) \ COUNTER(upstream_rq_retry_limit_exceeded) \ COUNTER(upstream_rq_retry_overflow) \ COUNTER(upstream_rq_retry_success) \ diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc index 5c6facc127fa..bae5f61c0b30 100644 --- a/source/common/router/retry_state_impl.cc +++ b/source/common/router/retry_state_impl.cc @@ -195,18 +195,12 @@ void RetryStateImpl::enableBackoffTimer() { ratelimited_backoff_strategy_.reset(); cluster_.stats().upstream_rq_retry_backoff_ratelimited_.inc(); - if (vcluster_) { - vcluster_->stats().upstream_rq_retry_backoff_ratelimited_.inc(); - } } else { // Otherwise we use a fully jittered exponential backoff algorithm. retry_timer_->enableTimer(std::chrono::milliseconds(backoff_strategy_->nextBackOffMs())); cluster_.stats().upstream_rq_retry_backoff_exponential_.inc(); - if (vcluster_) { - vcluster_->stats().upstream_rq_retry_backoff_exponential_.inc(); - } } } diff --git a/test/common/router/retry_state_impl_test.cc b/test/common/router/retry_state_impl_test.cc index 49daa7172a7b..3182ed146628 100644 --- a/test/common/router/retry_state_impl_test.cc +++ b/test/common/router/retry_state_impl_test.cc @@ -1137,8 +1137,6 @@ TEST_F(RouterRetryStateImplTest, RateLimitedRetryBackoffStrategy) { EXPECT_EQ(2UL, cluster_.stats().upstream_rq_retry_backoff_ratelimited_.value()); EXPECT_EQ(1UL, cluster_.stats().upstream_rq_retry_backoff_exponential_.value()); - EXPECT_EQ(2UL, virtual_cluster_.stats().upstream_rq_retry_backoff_ratelimited_.value()); - EXPECT_EQ(1UL, virtual_cluster_.stats().upstream_rq_retry_backoff_exponential_.value()); } TEST_F(RouterRetryStateImplTest, HostSelectionAttempts) { From 52b39b70ed5d02089e4402608ff10cd4c9db7c86 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Fri, 31 Jul 2020 12:54:08 +1000 Subject: [PATCH 15/35] Improve the docs Signed-off-by: Martin Matusiak --- api/BUILD | 1 - .../config/route/v3/route_components.proto | 80 +++++++++++++++++-- .../route/v4alpha/route_components.proto | 80 +++++++++++++++++-- .../http/http_filters/router_filter.rst | 9 ++- .../intro/arch_overview/http/http_routing.rst | 5 +- .../config/route/v3/route_components.proto | 77 ++++++++++++++++-- .../route/v4alpha/route_components.proto | 80 +++++++++++++++++-- 7 files changed, 308 insertions(+), 24 deletions(-) diff --git a/api/BUILD b/api/BUILD index 99bd1b119c62..c4018489440f 100644 --- a/api/BUILD +++ b/api/BUILD @@ -250,7 +250,6 @@ proto_library( "//envoy/service/discovery/v3:pkg", "//envoy/service/endpoint/v3:pkg", "//envoy/service/event_reporting/v3:pkg", - "//envoy/service/extension/v3:pkg", "//envoy/service/health/v3:pkg", "//envoy/service/listener/v3:pkg", "//envoy/service/load_stats/v3:pkg", diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index f0dfcc74712f..33c2dab780a8 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1087,7 +1087,77 @@ message RetryPolicy { google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}]; } + // A retry back-off strategy that applies when the upstream server rate limits + // the request. + // + // Given this configuration: + // + // .. code-block:: yaml + // + // rate_limited_retry_back_off: + // reset_headers: + // - name: Retry-After + // format: SECONDS + // - name: X-RateLimit-Reset + // format: UNIX_TIMESTAMP + // max_interval: "300s" + // + // The following algorithm will apply: + // + // 1. If the response contains the header ``Retry-After`` its value must be on + // the form ``120`` (an integer that represents the number of seconds to + // wait before retrying). If so, this value is used as the back-off interval. + // 2. Otherwise, if the response contains the header ``X-RateLimit-Reset`` its + // value must be on the form ``1595320702`` (an integer that represents the + // point in time at which to retry, as a Unix timestamp in seconds). If so, + // the current time is subtracted from this value and the result is used as + // the back-off interval. + // 3. Otherwise, Envoy will use the default + // :ref:`exponential back-off ` + // strategy. + // + // No matter which format is used, if the resulting back-off interval exceeds + // ``max_interval`` it is discarded and the next header in ``reset_headers`` + // is tried. If a request timeout is configured for the route it will further + // limit how long the request will be allowed to run. + // + // To prevent many clients retrying at the same point in time jitter is added + // to the back-off interval, so the resulting interval is decided by taking: + // ``random(interval, interval * 1.5)``. + // + // .. attention:: + // + // Configuring ``rate_limited_retry_back_off`` will not by itself cause a request + // to be retried. You will still need to configure the right retry policy to match + // the responses from the upstream server. message RateLimitedRetryBackOff { + enum ResetHeaderFormat { + SECONDS = 0; + UNIX_TIMESTAMP = 1; + } + + message ResetHeader { + string name = 1 [ + (validate.rules).string = {min_bytes: 1 well_known_regex: HTTP_HEADER_NAME strict: false} + ]; + + ResetHeaderFormat format = 2 [(validate.rules).message = {required: true}]; + } + + // Specifies the reset headers (like ``Retry-After`` or ``X-RateLimit-Reset``) + // to match against the response. Headers are tried in order, and matched case + // insensitive. The first header to be parsed successfully is used. If no headers + // match the default exponential back-off is used instead. + repeated ResetHeader reset_headers_ng = 3 [(validate.rules).repeated = {min_items: 1}]; + + // Specifies the maximum back off interval that Envoy will allow. If a reset + // header contains an interval longer than this then it will be discarded and + // the next header will be tried. Defaults to 300 seconds. + google.protobuf.Duration max_interval = 4 [(validate.rules).duration = {gt {}}]; + + + + // Specifies the rate limited retry reset headers (like ``Retry-After`` or // ``X-RateLimit-Reset``) to match against the response. If a request is // being retried and the response contains one of these headers the value @@ -1158,11 +1228,11 @@ message RetryPolicy { // describes Envoy's back-off algorithm. RetryBackOff retry_back_off = 8; - // Specifies parameters that control a retry back off based on rate limited - // responses. When a request is rate limited by the server the server may - // include a header like ``Retry-After`` or ``X-RateLimit-Reset`` to provide - // feedback to the client on how long to wait before retrying. If configured, - // the rate limited retry back off strategy will be used instead of the + // Specifies parameters that control a retry back-off strategy that is used + // when the request is rate limited by the upstream server. The server may + // return a response header like ``Retry-After`` or ``X-RateLimit-Reset`` to + // provide feedback to the client on how long to wait before retrying. If + // configured, this back-off strategy will be used instead of the // default exponential back off strategy (configured using `retry_back_off`) // whenever a response includes the matching headers. RateLimitedRetryBackOff rate_limited_retry_back_off = 11; diff --git a/api/envoy/config/route/v4alpha/route_components.proto b/api/envoy/config/route/v4alpha/route_components.proto index 06e061815f9a..854507d81c81 100644 --- a/api/envoy/config/route/v4alpha/route_components.proto +++ b/api/envoy/config/route/v4alpha/route_components.proto @@ -1065,10 +1065,80 @@ message RetryPolicy { google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}]; } + // A retry back-off strategy that applies when the upstream server rate limits + // the request. + // + // Given this configuration: + // + // .. code-block:: yaml + // + // rate_limited_retry_back_off: + // reset_headers: + // - name: Retry-After + // format: SECONDS + // - name: X-RateLimit-Reset + // format: UNIX_TIMESTAMP + // max_interval: "300s" + // + // The following algorithm will apply: + // + // 1. If the response contains the header ``Retry-After`` its value must be on + // the form ``120`` (an integer that represents the number of seconds to + // wait before retrying). If so, this value is used as the back-off interval. + // 2. Otherwise, if the response contains the header ``X-RateLimit-Reset`` its + // value must be on the form ``1595320702`` (an integer that represents the + // point in time at which to retry, as a Unix timestamp in seconds). If so, + // the current time is subtracted from this value and the result is used as + // the back-off interval. + // 3. Otherwise, Envoy will use the default + // :ref:`exponential back-off ` + // strategy. + // + // No matter which format is used, if the resulting back-off interval exceeds + // ``max_interval`` it is discarded and the next header in ``reset_headers`` + // is tried. If a request timeout is configured for the route it will further + // limit how long the request will be allowed to run. + // + // To prevent many clients retrying at the same point in time jitter is added + // to the back-off interval, so the resulting interval is decided by taking: + // ``random(interval, interval * 1.5)``. + // + // .. attention:: + // + // Configuring ``rate_limited_retry_back_off`` will not by itself cause a request + // to be retried. You will still need to configure the right retry policy to match + // the responses from the upstream server. message RateLimitedRetryBackOff { option (udpa.annotations.versioning).previous_message_type = "envoy.config.route.v3.RetryPolicy.RateLimitedRetryBackOff"; + enum ResetHeaderFormat { + SECONDS = 0; + UNIX_TIMESTAMP = 1; + } + + message ResetHeader { + option (udpa.annotations.versioning).previous_message_type = + "envoy.config.route.v3.RetryPolicy.RateLimitedRetryBackOff.ResetHeader"; + + string name = 1 [ + (validate.rules).string = {min_bytes: 1 well_known_regex: HTTP_HEADER_NAME strict: false} + ]; + + ResetHeaderFormat format = 2 [(validate.rules).message = {required: true}]; + } + + // Specifies the reset headers (like ``Retry-After`` or ``X-RateLimit-Reset``) + // to match against the response. Headers are tried in order, and matched case + // insensitive. The first header to be parsed successfully is used. If no headers + // match the default exponential back-off is used instead. + repeated ResetHeader reset_headers_ng = 3 [(validate.rules).repeated = {min_items: 1}]; + + // Specifies the maximum back off interval that Envoy will allow. If a reset + // header contains an interval longer than this then it will be discarded and + // the next header will be tried. Defaults to 300 seconds. + google.protobuf.Duration max_interval = 4 [(validate.rules).duration = {gt {}}]; + // Specifies the rate limited retry reset headers (like ``Retry-After`` or // ``X-RateLimit-Reset``) to match against the response. If a request is // being retried and the response contains one of these headers the value @@ -1138,11 +1208,11 @@ message RetryPolicy { // describes Envoy's back-off algorithm. RetryBackOff retry_back_off = 8; - // Specifies parameters that control a retry back off based on rate limited - // responses. When a request is rate limited by the server the server may - // include a header like ``Retry-After`` or ``X-RateLimit-Reset`` to provide - // feedback to the client on how long to wait before retrying. If configured, - // the rate limited retry back off strategy will be used instead of the + // Specifies parameters that control a retry back-off strategy that is used + // when the request is rate limited by the upstream server. The server may + // return a response header like ``Retry-After`` or ``X-RateLimit-Reset`` to + // provide feedback to the client on how long to wait before retrying. If + // configured, this back-off strategy will be used instead of the // default exponential back off strategy (configured using `retry_back_off`) // whenever a response includes the matching headers. RateLimitedRetryBackOff rate_limited_retry_back_off = 11; diff --git a/docs/root/configuration/http/http_filters/router_filter.rst b/docs/root/configuration/http/http_filters/router_filter.rst index 6abc596627fc..29be58bbcd98 100644 --- a/docs/root/configuration/http/http_filters/router_filter.rst +++ b/docs/root/configuration/http/http_filters/router_filter.rst @@ -42,7 +42,7 @@ A few notes on how Envoy does retries: retries. Thus if the request timeout is set to 3s, and the first request attempt takes 2.7s, the retry (including back-off) has .3s to complete. This is by design to avoid an exponential retry/timeout explosion. -* Envoy uses a fully jittered exponential back-off algorithm for retries with a default base +* By default, Envoy uses a fully jittered exponential back-off algorithm for retries with a default base interval of 25ms. Given a base interval B and retry number N, the back-off for the retry is in the range :math:`\big[0, (2^N-1)B\big)`. For example, given the default interval, the first retry will be delayed randomly by 0-24ms, the 2nd by 0-74ms, the 3rd by 0-174ms, and so on. The @@ -51,6 +51,13 @@ A few notes on how Envoy does retries: upstream.base_retry_backoff_ms runtime parameter. The back-off intervals can also be modified by configuring the retry policy's :ref:`retry back-off `. +* Envoy can also be configured to use feedback from the upstream server to decide the interval between + retries. Response headers like ``Retry-After`` or ``X-RateLimit-Reset`` instruct the client how long + to wait before re-trying. The retry policy's + :ref:`rate limited retry back off ` + strategy can be configured to expect a particular header, and if that header is present in the response Envoy + will use its value to decide the back-off. If the header is not present, or if it cannot be parsed + successfully, Envoy will use the default exponential back-off algorithm instead. .. _config_http_filters_router_x-envoy-retry-on: diff --git a/docs/root/intro/arch_overview/http/http_routing.rst b/docs/root/intro/arch_overview/http/http_routing.rst index ff24b0fd512e..884963dfcb80 100644 --- a/docs/root/intro/arch_overview/http/http_routing.rst +++ b/docs/root/intro/arch_overview/http/http_routing.rst @@ -106,8 +106,9 @@ Envoy allows retries to be configured both in the :ref:`route configuration ` as well as for specific requests via :ref:`request headers `. The following configurations are possible: -* **Maximum number of retries**: Envoy will continue to retry any number of times. An exponential - backoff algorithm is used between each retry. Additionally, *all retries are contained within the +* **Maximum number of retries**: Envoy will continue to retry any number of times. The intervals between + retries are decided either by an exponential backoff algorithm (the default), or based on feedback + from the upstream server via headers (if present). Additionally, *all retries are contained within the overall request timeout*. This avoids long request times due to a large number of retries. * **Retry conditions**: Envoy can retry on different types of conditions depending on application requirements. For example, network failure, all 5xx response codes, idempotent 4xx response codes, diff --git a/generated_api_shadow/envoy/config/route/v3/route_components.proto b/generated_api_shadow/envoy/config/route/v3/route_components.proto index 3880bafc6ee1..2ae71c260178 100644 --- a/generated_api_shadow/envoy/config/route/v3/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v3/route_components.proto @@ -1094,7 +1094,74 @@ message RetryPolicy { google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}]; } + // A retry back-off strategy that applies when the upstream server rate limits + // the request. + // + // Given this configuration: + // + // .. code-block:: yaml + // + // rate_limited_retry_back_off: + // reset_headers: + // - name: Retry-After + // format: SECONDS + // - name: X-RateLimit-Reset + // format: UNIX_TIMESTAMP + // max_interval: "300s" + // + // The following algorithm will apply: + // + // 1. If the response contains the header ``Retry-After`` its value must be on + // the form ``120`` (an integer that represents the number of seconds to + // wait before retrying). If so, this value is used as the back-off interval. + // 2. Otherwise, if the response contains the header ``X-RateLimit-Reset`` its + // value must be on the form ``1595320702`` (an integer that represents the + // point in time at which to retry, as a Unix timestamp in seconds). If so, + // the current time is subtracted from this value and the result is used as + // the back-off interval. + // 3. Otherwise, Envoy will use the default + // :ref:`exponential back-off ` + // strategy. + // + // No matter which format is used, if the resulting back-off interval exceeds + // ``max_interval`` it is discarded and the next header in ``reset_headers`` + // is tried. If a request timeout is configured for the route it will further + // limit how long the request will be allowed to run. + // + // To prevent many clients retrying at the same point in time jitter is added + // to the back-off interval, so the resulting interval is decided by taking: + // ``random(interval, interval * 1.5)``. + // + // .. attention:: + // + // Configuring ``rate_limited_retry_back_off`` will not by itself cause a request + // to be retried. You will still need to configure the right retry policy to match + // the responses from the upstream server. message RateLimitedRetryBackOff { + enum ResetHeaderFormat { + SECONDS = 0; + UNIX_TIMESTAMP = 1; + } + + message ResetHeader { + string name = 1 [ + (validate.rules).string = {min_bytes: 1 well_known_regex: HTTP_HEADER_NAME strict: false} + ]; + + ResetHeaderFormat format = 2 [(validate.rules).message = {required: true}]; + } + + // Specifies the reset headers (like ``Retry-After`` or ``X-RateLimit-Reset``) + // to match against the response. Headers are tried in order, and matched case + // insensitive. The first header to be parsed successfully is used. If no headers + // match the default exponential back-off is used instead. + repeated ResetHeader reset_headers_ng = 3 [(validate.rules).repeated = {min_items: 1}]; + + // Specifies the maximum back off interval that Envoy will allow. If a reset + // header contains an interval longer than this then it will be discarded and + // the next header will be tried. Defaults to 300 seconds. + google.protobuf.Duration max_interval = 4 [(validate.rules).duration = {gt {}}]; + // Specifies the rate limited retry reset headers (like ``Retry-After`` or // ``X-RateLimit-Reset``) to match against the response. If a request is // being retried and the response contains one of these headers the value @@ -1165,11 +1232,11 @@ message RetryPolicy { // describes Envoy's back-off algorithm. RetryBackOff retry_back_off = 8; - // Specifies parameters that control a retry back off based on rate limited - // responses. When a request is rate limited by the server the server may - // include a header like ``Retry-After`` or ``X-RateLimit-Reset`` to provide - // feedback to the client on how long to wait before retrying. If configured, - // the rate limited retry back off strategy will be used instead of the + // Specifies parameters that control a retry back-off strategy that is used + // when the request is rate limited by the upstream server. The server may + // return a response header like ``Retry-After`` or ``X-RateLimit-Reset`` to + // provide feedback to the client on how long to wait before retrying. If + // configured, this back-off strategy will be used instead of the // default exponential back off strategy (configured using `retry_back_off`) // whenever a response includes the matching headers. RateLimitedRetryBackOff rate_limited_retry_back_off = 11; diff --git a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto index a77e63d7d0b2..12ad173ce371 100644 --- a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto @@ -1093,10 +1093,80 @@ message RetryPolicy { google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}]; } + // A retry back-off strategy that applies when the upstream server rate limits + // the request. + // + // Given this configuration: + // + // .. code-block:: yaml + // + // rate_limited_retry_back_off: + // reset_headers: + // - name: Retry-After + // format: SECONDS + // - name: X-RateLimit-Reset + // format: UNIX_TIMESTAMP + // max_interval: "300s" + // + // The following algorithm will apply: + // + // 1. If the response contains the header ``Retry-After`` its value must be on + // the form ``120`` (an integer that represents the number of seconds to + // wait before retrying). If so, this value is used as the back-off interval. + // 2. Otherwise, if the response contains the header ``X-RateLimit-Reset`` its + // value must be on the form ``1595320702`` (an integer that represents the + // point in time at which to retry, as a Unix timestamp in seconds). If so, + // the current time is subtracted from this value and the result is used as + // the back-off interval. + // 3. Otherwise, Envoy will use the default + // :ref:`exponential back-off ` + // strategy. + // + // No matter which format is used, if the resulting back-off interval exceeds + // ``max_interval`` it is discarded and the next header in ``reset_headers`` + // is tried. If a request timeout is configured for the route it will further + // limit how long the request will be allowed to run. + // + // To prevent many clients retrying at the same point in time jitter is added + // to the back-off interval, so the resulting interval is decided by taking: + // ``random(interval, interval * 1.5)``. + // + // .. attention:: + // + // Configuring ``rate_limited_retry_back_off`` will not by itself cause a request + // to be retried. You will still need to configure the right retry policy to match + // the responses from the upstream server. message RateLimitedRetryBackOff { option (udpa.annotations.versioning).previous_message_type = "envoy.config.route.v3.RetryPolicy.RateLimitedRetryBackOff"; + enum ResetHeaderFormat { + SECONDS = 0; + UNIX_TIMESTAMP = 1; + } + + message ResetHeader { + option (udpa.annotations.versioning).previous_message_type = + "envoy.config.route.v3.RetryPolicy.RateLimitedRetryBackOff.ResetHeader"; + + string name = 1 [ + (validate.rules).string = {min_bytes: 1 well_known_regex: HTTP_HEADER_NAME strict: false} + ]; + + ResetHeaderFormat format = 2 [(validate.rules).message = {required: true}]; + } + + // Specifies the reset headers (like ``Retry-After`` or ``X-RateLimit-Reset``) + // to match against the response. Headers are tried in order, and matched case + // insensitive. The first header to be parsed successfully is used. If no headers + // match the default exponential back-off is used instead. + repeated ResetHeader reset_headers_ng = 3 [(validate.rules).repeated = {min_items: 1}]; + + // Specifies the maximum back off interval that Envoy will allow. If a reset + // header contains an interval longer than this then it will be discarded and + // the next header will be tried. Defaults to 300 seconds. + google.protobuf.Duration max_interval = 4 [(validate.rules).duration = {gt {}}]; + // Specifies the rate limited retry reset headers (like ``Retry-After`` or // ``X-RateLimit-Reset``) to match against the response. If a request is // being retried and the response contains one of these headers the value @@ -1166,11 +1236,11 @@ message RetryPolicy { // describes Envoy's back-off algorithm. RetryBackOff retry_back_off = 8; - // Specifies parameters that control a retry back off based on rate limited - // responses. When a request is rate limited by the server the server may - // include a header like ``Retry-After`` or ``X-RateLimit-Reset`` to provide - // feedback to the client on how long to wait before retrying. If configured, - // the rate limited retry back off strategy will be used instead of the + // Specifies parameters that control a retry back-off strategy that is used + // when the request is rate limited by the upstream server. The server may + // return a response header like ``Retry-After`` or ``X-RateLimit-Reset`` to + // provide feedback to the client on how long to wait before retrying. If + // configured, this back-off strategy will be used instead of the // default exponential back off strategy (configured using `retry_back_off`) // whenever a response includes the matching headers. RateLimitedRetryBackOff rate_limited_retry_back_off = 11; From cc6d1465dfef7eb76119bbb2c8d0126cff01d82e Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Fri, 31 Jul 2020 13:28:12 +1000 Subject: [PATCH 16/35] Remove header x-envoy-ratelimited-reset-max-interval-ms Signed-off-by: Martin Matusiak --- .../config/route/v3/route_components.proto | 3 -- .../http/http_filters/router_filter.rst | 6 --- include/envoy/http/header_map.h | 1 - include/envoy/router/router.h | 5 --- source/common/router/retry_state_impl.cc | 10 ----- source/common/router/retry_state_impl.h | 4 -- test/common/router/retry_state_impl_test.cc | 41 ------------------- test/mocks/router/mocks.h | 1 - 8 files changed, 71 deletions(-) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index 33c2dab780a8..812ffbdd336a 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1155,9 +1155,6 @@ message RetryPolicy { // the next header will be tried. Defaults to 300 seconds. google.protobuf.Duration max_interval = 4 [(validate.rules).duration = {gt {}}]; - - - // Specifies the rate limited retry reset headers (like ``Retry-After`` or // ``X-RateLimit-Reset``) to match against the response. If a request is // being retried and the response contains one of these headers the value diff --git a/docs/root/configuration/http/http_filters/router_filter.rst b/docs/root/configuration/http/http_filters/router_filter.rst index 29be58bbcd98..3df7c9522c60 100644 --- a/docs/root/configuration/http/http_filters/router_filter.rst +++ b/docs/root/configuration/http/http_filters/router_filter.rst @@ -270,12 +270,6 @@ x-envoy-ratelimited-reset-headers Setting this header will extend the list of rate limited reset headers specified in the :ref:`route configuration rate limited retry back off `. -x-envoy-ratelimited-reset-max-interval-ms -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -Setting this header will override the rate limited reset max interval specified in the :ref:`route configuration rate limited retry back off -`. - .. _config_http_filters_router_x-envoy-decorator-operation: x-envoy-decorator-operation diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 0866c34e9ae2..f75c15d10277 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -273,7 +273,6 @@ class HeaderEntry { HEADER_FUNC(EnvoyIpTags) \ HEADER_FUNC(EnvoyMaxRetries) \ HEADER_FUNC(EnvoyRateLimitedResetHeaders) \ - HEADER_FUNC(EnvoyRateLimitedResetMaxIntervalMs) \ HEADER_FUNC(EnvoyRetryOn) \ HEADER_FUNC(EnvoyRetryGrpcOn) \ HEADER_FUNC(EnvoyRetriableStatusCodes) \ diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 5549c9416468..7dc9adf1f62b 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -399,11 +399,6 @@ class RetryState { * @return the rate limited reset headers used to match against rate limited responses. */ virtual const std::vector& rateLimitedResetHeaders() const PURE; - - /** - * @return the upper limit placed on an interval parsed from a rate limited reset header. - */ - virtual std::chrono::milliseconds rateLimitedResetMaxInterval() const PURE; }; using RetryStatePtr = std::unique_ptr; diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc index bae5f61c0b30..858f712ba263 100644 --- a/source/common/router/retry_state_impl.cc +++ b/source/common/router/retry_state_impl.cc @@ -53,7 +53,6 @@ RetryStatePtr RetryStateImpl::create(const RetryPolicy& route_policy, request_headers.removeEnvoyRetryGrpcOn(); request_headers.removeEnvoyMaxRetries(); request_headers.removeEnvoyRateLimitedResetHeaders(); - request_headers.removeEnvoyRateLimitedResetMaxIntervalMs(); if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.consume_all_retry_headers")) { request_headers.removeEnvoyHedgeOnPerTryTimeout(); request_headers.removeEnvoyRetriableHeaderNames(); @@ -153,15 +152,6 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, } } - if (request_headers.EnvoyRateLimitedResetMaxIntervalMs()) { - const auto max_interval = - request_headers.EnvoyRateLimitedResetMaxIntervalMs()->value().getStringView(); - unsigned long out; - if (absl::SimpleAtoi(max_interval, &out)) { - ratelimited_reset_max_interval_ = std::chrono::milliseconds(out); - } - } - if (request_headers.EnvoyRateLimitedResetHeaders()) { // Ratelimit reset headers in the configuration are specified via HeaderMatcher. // Giving the same flexibility via request header would require the user diff --git a/source/common/router/retry_state_impl.h b/source/common/router/retry_state_impl.h index 6808171b43b4..d7875ee9153f 100644 --- a/source/common/router/retry_state_impl.h +++ b/source/common/router/retry_state_impl.h @@ -95,10 +95,6 @@ class RetryStateImpl : public RetryState { return ratelimited_reset_headers_; } - std::chrono::milliseconds rateLimitedResetMaxInterval() const override { - return ratelimited_reset_max_interval_; - } - private: RetryStateImpl(const RetryPolicy& route_policy, Http::RequestHeaderMap& request_headers, const Upstream::ClusterInfo& cluster, const VirtualCluster* vcluster, diff --git a/test/common/router/retry_state_impl_test.cc b/test/common/router/retry_state_impl_test.cc index 3182ed146628..f2abeedbf36e 100644 --- a/test/common/router/retry_state_impl_test.cc +++ b/test/common/router/retry_state_impl_test.cc @@ -927,39 +927,6 @@ TEST_F(RouterRetryStateImplTest, CustomBackOffIntervalDefaultMax) { retry_timer_->invokeCallback(); } -TEST_F(RouterRetryStateImplTest, ParseRateLimitResetMaxInterval) { - // No value set in via config nor headers so we get the default value - { - Http::TestRequestHeaderMapImpl request_headers{{"x-envoy-retry-on", "5xx"}}; - setup(request_headers); - EXPECT_TRUE(state_->enabled()); - - EXPECT_EQ(std::chrono::milliseconds(300000), state_->rateLimitedResetMaxInterval()); - } - - // The remaining cases expect the config to have a max_interval - policy_.ratelimited_reset_max_interval_ = std::chrono::milliseconds(4000); - - // Value set in config is used as fallback - { - Http::TestRequestHeaderMapImpl request_headers{{"x-envoy-retry-on", "5xx"}}; - setup(request_headers); - EXPECT_TRUE(state_->enabled()); - - EXPECT_EQ(std::chrono::milliseconds(4000), state_->rateLimitedResetMaxInterval()); - } - - // Value set in headers takes precedence over config - { - Http::TestRequestHeaderMapImpl request_headers{ - {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-max-interval-ms", "1000"}}; - setup(request_headers); - EXPECT_TRUE(state_->enabled()); - - EXPECT_EQ(std::chrono::milliseconds(1000), state_->rateLimitedResetMaxInterval()); - } -} - TEST_F(RouterRetryStateImplTest, ParseRateLimitedResetHeaders) { Protobuf::RepeatedPtrField matchers; auto* matcher = matchers.Add(); @@ -1359,7 +1326,6 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { {"x-envoy-retry-on", "5xx,retriable-header-names,retriable-status-codes"}, {"x-envoy-retry-grpc-on", "resource-exhausted"}, {"x-envoy-ratelimited-reset-headers", "Retry-After"}, - {"x-envoy-ratelimited-reset-max-interval-ms", "1000"}, {"x-envoy-retriable-header-names", "X-Upstream-Pushback"}, {"x-envoy-retriable-status-codes", "418,420"}, {"x-envoy-max-retries", "7"}, @@ -1373,7 +1339,6 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { EXPECT_FALSE(request_headers.has("x-envoy-retry-grpc-on")); EXPECT_FALSE(request_headers.has("x-envoy-max-retries")); EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-headers")); - EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-max-interval-ms")); EXPECT_FALSE(request_headers.has("x-envoy-retriable-header-names")); EXPECT_FALSE(request_headers.has("x-envoy-retriable-status-codes")); EXPECT_FALSE(request_headers.has("x-envoy-hedge-on-per-try-timeout")); @@ -1384,7 +1349,6 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { { Http::TestRequestHeaderMapImpl request_headers{ {"x-envoy-ratelimited-reset-headers", "Retry-After"}, - {"x-envoy-ratelimited-reset-max-interval-ms", "1000"}, {"x-envoy-retriable-header-names", "X-Upstream-Pushback"}, {"x-envoy-retriable-status-codes", "418,420"}, {"x-envoy-max-retries", "7"}, @@ -1398,7 +1362,6 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { EXPECT_FALSE(request_headers.has("x-envoy-retry-grpc-on")); EXPECT_FALSE(request_headers.has("x-envoy-max-retries")); EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-headers")); - EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-max-interval-ms")); EXPECT_FALSE(request_headers.has("x-envoy-retriable-header-names")); EXPECT_FALSE(request_headers.has("x-envoy-retriable-status-codes")); EXPECT_FALSE(request_headers.has("x-envoy-hedge-on-per-try-timeout")); @@ -1415,7 +1378,6 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { {"x-envoy-retry-on", "5xx,retriable-header-names,retriable-status-codes"}, {"x-envoy-retry-grpc-on", "resource-exhausted"}, {"x-envoy-ratelimited-reset-headers", "Retry-After"}, - {"x-envoy-ratelimited-reset-max-interval-ms", "1000"}, {"x-envoy-retriable-header-names", "X-Upstream-Pushback"}, {"x-envoy-retriable-status-codes", "418,420"}, {"x-envoy-max-retries", "7"}, @@ -1429,7 +1391,6 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { EXPECT_FALSE(request_headers.has("x-envoy-retry-grpc-on")); EXPECT_FALSE(request_headers.has("x-envoy-max-retries")); EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-headers")); - EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-max-interval-ms")); EXPECT_TRUE(request_headers.has("x-envoy-retriable-header-names")); EXPECT_TRUE(request_headers.has("x-envoy-retriable-status-codes")); EXPECT_TRUE(request_headers.has("x-envoy-hedge-on-per-try-timeout")); @@ -1444,7 +1405,6 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { Http::TestRequestHeaderMapImpl request_headers{ {"x-envoy-ratelimited-reset-headers", "Retry-After"}, - {"x-envoy-ratelimited-reset-max-interval-ms", "1000"}, {"x-envoy-retriable-header-names", "X-Upstream-Pushback"}, {"x-envoy-retriable-status-codes", "418,420"}, {"x-envoy-max-retries", "7"}, @@ -1458,7 +1418,6 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { EXPECT_FALSE(request_headers.has("x-envoy-retry-grpc-on")); EXPECT_FALSE(request_headers.has("x-envoy-max-retries")); EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-headers")); - EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-max-interval-ms")); EXPECT_TRUE(request_headers.has("x-envoy-retriable-header-names")); EXPECT_TRUE(request_headers.has("x-envoy-retriable-status-codes")); EXPECT_TRUE(request_headers.has("x-envoy-hedge-on-per-try-timeout")); diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 07d62ee0d2c1..644397ab89b7 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -181,7 +181,6 @@ class MockRetryState : public RetryState { MOCK_METHOD(uint32_t, hostSelectionMaxAttempts, (), (const)); MOCK_METHOD(const std::vector&, rateLimitedResetHeaders, (), (const)); - MOCK_METHOD(std::chrono::milliseconds, rateLimitedResetMaxInterval, (), (const)); DoRetryCallback callback_; }; From e4dd05cb21b86d19a5e4434a5d3a39583f630a42 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Sat, 1 Aug 2020 13:39:27 +1000 Subject: [PATCH 17/35] Fix api CI build Signed-off-by: Martin Matusiak --- api/BUILD | 1 + api/envoy/config/route/v3/route_components.proto | 2 +- api/envoy/config/route/v4alpha/route_components.proto | 2 +- .../envoy/config/route/v3/route_components.proto | 2 +- .../envoy/config/route/v4alpha/route_components.proto | 2 +- 5 files changed, 5 insertions(+), 4 deletions(-) diff --git a/api/BUILD b/api/BUILD index c4018489440f..99bd1b119c62 100644 --- a/api/BUILD +++ b/api/BUILD @@ -250,6 +250,7 @@ proto_library( "//envoy/service/discovery/v3:pkg", "//envoy/service/endpoint/v3:pkg", "//envoy/service/event_reporting/v3:pkg", + "//envoy/service/extension/v3:pkg", "//envoy/service/health/v3:pkg", "//envoy/service/listener/v3:pkg", "//envoy/service/load_stats/v3:pkg", diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index 812ffbdd336a..4fecc345e350 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1141,7 +1141,7 @@ message RetryPolicy { (validate.rules).string = {min_bytes: 1 well_known_regex: HTTP_HEADER_NAME strict: false} ]; - ResetHeaderFormat format = 2 [(validate.rules).message = {required: true}]; + ResetHeaderFormat format = 2 [(validate.rules).enum = {defined_only: true}]; } // Specifies the reset headers (like ``Retry-After`` or ``X-RateLimit-Reset``) diff --git a/api/envoy/config/route/v4alpha/route_components.proto b/api/envoy/config/route/v4alpha/route_components.proto index 854507d81c81..1f038e804cfd 100644 --- a/api/envoy/config/route/v4alpha/route_components.proto +++ b/api/envoy/config/route/v4alpha/route_components.proto @@ -1125,7 +1125,7 @@ message RetryPolicy { (validate.rules).string = {min_bytes: 1 well_known_regex: HTTP_HEADER_NAME strict: false} ]; - ResetHeaderFormat format = 2 [(validate.rules).message = {required: true}]; + ResetHeaderFormat format = 2 [(validate.rules).enum = {defined_only: true}]; } // Specifies the reset headers (like ``Retry-After`` or ``X-RateLimit-Reset``) diff --git a/generated_api_shadow/envoy/config/route/v3/route_components.proto b/generated_api_shadow/envoy/config/route/v3/route_components.proto index 2ae71c260178..aa84110f4ab1 100644 --- a/generated_api_shadow/envoy/config/route/v3/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v3/route_components.proto @@ -1148,7 +1148,7 @@ message RetryPolicy { (validate.rules).string = {min_bytes: 1 well_known_regex: HTTP_HEADER_NAME strict: false} ]; - ResetHeaderFormat format = 2 [(validate.rules).message = {required: true}]; + ResetHeaderFormat format = 2 [(validate.rules).enum = {defined_only: true}]; } // Specifies the reset headers (like ``Retry-After`` or ``X-RateLimit-Reset``) diff --git a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto index 12ad173ce371..0c53d5332564 100644 --- a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto @@ -1153,7 +1153,7 @@ message RetryPolicy { (validate.rules).string = {min_bytes: 1 well_known_regex: HTTP_HEADER_NAME strict: false} ]; - ResetHeaderFormat format = 2 [(validate.rules).message = {required: true}]; + ResetHeaderFormat format = 2 [(validate.rules).enum = {defined_only: true}]; } // Specifies the reset headers (like ``Retry-After`` or ``X-RateLimit-Reset``) From 2edaf26826b886fd81d33910948ad070b1a799e9 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Sat, 1 Aug 2020 14:16:00 +1000 Subject: [PATCH 18/35] Fix merge after rebase Signed-off-by: Martin Matusiak --- docs/root/version_history/current.rst | 6 ------ source/common/http/headers.h | 2 -- 2 files changed, 8 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 52d2bd9774b4..28d13204dba9 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -73,12 +73,6 @@ New Features * stats: added optional histograms to :ref:`cluster stats ` that track headers and body sizes of requests and responses. * stats: allow configuring histogram buckets for stats sinks and admin endpoints that support it. -* router: added a new :ref:`rate limited retry back off - ` strategy that uses - headers like `Retry-After` or `X-RateLimit-Reset` to decide the back off - interval. -* stats: added optional histograms to :ref:`cluster stats ` - that track headers and body sizes of requests and responses. * tap: added :ref:`generic body matcher` to scan http requests and responses for text or hex patterns. * tcp: switched the TCP connection pool to the new "shared" connection pool, sharing a common code base with HTTP and HTTP/2. Any unexpected behavioral changes can be temporarily reverted by setting `envoy.reloadable_features.new_tcp_connection_pool` to false. * watchdog: support randomizing the watchdog's kill timeout to prevent synchronized kills via a maximium jitter parameter :ref:`max_kill_timeout_jitter`. diff --git a/source/common/http/headers.h b/source/common/http/headers.h index dc4806985642..be09eb175d19 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -159,8 +159,6 @@ class HeaderValues { const LowerCaseString EnvoyRateLimited{absl::StrCat(prefix(), "-ratelimited")}; const LowerCaseString EnvoyRateLimitedResetHeaders{ absl::StrCat(prefix(), "-ratelimited-reset-headers")}; - const LowerCaseString EnvoyRateLimitedResetMaxIntervalMs{ - absl::StrCat(prefix(), "-ratelimited-reset-max-interval-ms")}; const LowerCaseString EnvoyRetryOn{absl::StrCat(prefix(), "-retry-on")}; const LowerCaseString EnvoyRetryGrpcOn{absl::StrCat(prefix(), "-retry-grpc-on")}; const LowerCaseString EnvoyRetriableStatusCodes{ From fad4cee5f65bf20b9151a53f16698169ffa4c38c Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Sat, 1 Aug 2020 15:16:54 +1000 Subject: [PATCH 19/35] Implement new ResetHeaderParser Signed-off-by: Martin Matusiak --- .../config/route/v3/route_components.proto | 48 ++--- .../route/v4alpha/route_components.proto | 54 ++--- .../http/http_filters/router_filter.rst | 6 - .../config/route/v3/route_components.proto | 48 ++--- .../route/v4alpha/route_components.proto | 54 ++--- include/envoy/http/BUILD | 1 + include/envoy/http/header_map.h | 28 ++- include/envoy/router/router.h | 19 +- source/common/http/async_client_impl.h | 8 +- source/common/http/header_utility.cc | 55 +++++ source/common/http/header_utility.h | 38 +++- source/common/http/headers.h | 2 - source/common/router/config_impl.cc | 12 +- source/common/router/config_impl.h | 12 +- source/common/router/retry_state_impl.cc | 69 +----- source/common/router/retry_state_impl.h | 10 +- test/common/http/BUILD | 1 + test/common/http/header_utility_test.cc | 198 ++++++++++++++++-- test/common/router/config_impl_test.cc | 44 ++-- test/common/router/retry_state_impl_test.cc | 161 +++++--------- test/mocks/router/mocks.h | 16 +- 21 files changed, 465 insertions(+), 419 deletions(-) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index 4fecc345e350..c3e1c53b1f1c 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -1037,6 +1037,11 @@ message RouteAction { message RetryPolicy { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.route.RetryPolicy"; + enum ResetHeaderFormat { + SECONDS = 0; + UNIX_TIMESTAMP = 1; + } + message RetryPriority { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.route.RetryPolicy.RetryPriority"; @@ -1087,6 +1092,13 @@ message RetryPolicy { google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}]; } + message ResetHeader { + string name = 1 + [(validate.rules).string = {min_bytes: 1 well_known_regex: HTTP_HEADER_NAME strict: false}]; + + ResetHeaderFormat format = 2 [(validate.rules).enum = {defined_only: true}]; + } + // A retry back-off strategy that applies when the upstream server rate limits // the request. // @@ -1131,48 +1143,16 @@ message RetryPolicy { // to be retried. You will still need to configure the right retry policy to match // the responses from the upstream server. message RateLimitedRetryBackOff { - enum ResetHeaderFormat { - SECONDS = 0; - UNIX_TIMESTAMP = 1; - } - - message ResetHeader { - string name = 1 [ - (validate.rules).string = {min_bytes: 1 well_known_regex: HTTP_HEADER_NAME strict: false} - ]; - - ResetHeaderFormat format = 2 [(validate.rules).enum = {defined_only: true}]; - } - // Specifies the reset headers (like ``Retry-After`` or ``X-RateLimit-Reset``) // to match against the response. Headers are tried in order, and matched case // insensitive. The first header to be parsed successfully is used. If no headers // match the default exponential back-off is used instead. - repeated ResetHeader reset_headers_ng = 3 [(validate.rules).repeated = {min_items: 1}]; + repeated ResetHeader reset_headers = 1 [(validate.rules).repeated = {min_items: 1}]; // Specifies the maximum back off interval that Envoy will allow. If a reset // header contains an interval longer than this then it will be discarded and // the next header will be tried. Defaults to 300 seconds. - google.protobuf.Duration max_interval = 4 [(validate.rules).duration = {gt {}}]; - - // Specifies the rate limited retry reset headers (like ``Retry-After`` or - // ``X-RateLimit-Reset``) to match against the response. If a request is - // being retried and the response contains one of these headers the value - // of the header will be used as the retry back off interval, with jitter - // added. If multiple headers are specified they are tried in the order - // given and the first matching header in the response is used. - // - // The value of the header must be either: - // 1. An interval in seconds as an integer, for example: ``120``. - // 2. A unix timestamp as an integer, for example: ``1595320702``. The timestamp - // must be in the future relative to the current system time. - repeated HeaderMatcher reset_headers = 1 [(validate.rules).repeated = {min_items: 1}]; - - // Specifies the maximum rate limited retry back off interval that Envoy - // will allow. If a rate limited reset header contains an interval longer - // than this then the default exponential back off strategy will be used - // instead. Defaults to 300 seconds. - google.protobuf.Duration reset_max_interval = 2 [(validate.rules).duration = {gt {}}]; + google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}]; } // Specifies the conditions under which retry takes place. These are the same diff --git a/api/envoy/config/route/v4alpha/route_components.proto b/api/envoy/config/route/v4alpha/route_components.proto index 1f038e804cfd..2bf9a1f3eef5 100644 --- a/api/envoy/config/route/v4alpha/route_components.proto +++ b/api/envoy/config/route/v4alpha/route_components.proto @@ -1015,6 +1015,11 @@ message RouteAction { message RetryPolicy { option (udpa.annotations.versioning).previous_message_type = "envoy.config.route.v3.RetryPolicy"; + enum ResetHeaderFormat { + SECONDS = 0; + UNIX_TIMESTAMP = 1; + } + message RetryPriority { option (udpa.annotations.versioning).previous_message_type = "envoy.config.route.v3.RetryPolicy.RetryPriority"; @@ -1065,6 +1070,16 @@ message RetryPolicy { google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}]; } + message ResetHeader { + option (udpa.annotations.versioning).previous_message_type = + "envoy.config.route.v3.RetryPolicy.ResetHeader"; + + string name = 1 + [(validate.rules).string = {min_bytes: 1 well_known_regex: HTTP_HEADER_NAME strict: false}]; + + ResetHeaderFormat format = 2 [(validate.rules).enum = {defined_only: true}]; + } + // A retry back-off strategy that applies when the upstream server rate limits // the request. // @@ -1112,51 +1127,16 @@ message RetryPolicy { option (udpa.annotations.versioning).previous_message_type = "envoy.config.route.v3.RetryPolicy.RateLimitedRetryBackOff"; - enum ResetHeaderFormat { - SECONDS = 0; - UNIX_TIMESTAMP = 1; - } - - message ResetHeader { - option (udpa.annotations.versioning).previous_message_type = - "envoy.config.route.v3.RetryPolicy.RateLimitedRetryBackOff.ResetHeader"; - - string name = 1 [ - (validate.rules).string = {min_bytes: 1 well_known_regex: HTTP_HEADER_NAME strict: false} - ]; - - ResetHeaderFormat format = 2 [(validate.rules).enum = {defined_only: true}]; - } - // Specifies the reset headers (like ``Retry-After`` or ``X-RateLimit-Reset``) // to match against the response. Headers are tried in order, and matched case // insensitive. The first header to be parsed successfully is used. If no headers // match the default exponential back-off is used instead. - repeated ResetHeader reset_headers_ng = 3 [(validate.rules).repeated = {min_items: 1}]; + repeated ResetHeader reset_headers = 1 [(validate.rules).repeated = {min_items: 1}]; // Specifies the maximum back off interval that Envoy will allow. If a reset // header contains an interval longer than this then it will be discarded and // the next header will be tried. Defaults to 300 seconds. - google.protobuf.Duration max_interval = 4 [(validate.rules).duration = {gt {}}]; - - // Specifies the rate limited retry reset headers (like ``Retry-After`` or - // ``X-RateLimit-Reset``) to match against the response. If a request is - // being retried and the response contains one of these headers the value - // of the header will be used as the retry back off interval, with jitter - // added. If multiple headers are specified they are tried in the order - // given and the first matching header in the response is used. - // - // The value of the header must be either: - // 1. An interval in seconds as an integer, for example: ``120``. - // 2. A unix timestamp as an integer, for example: ``1595320702``. The timestamp - // must be in the future relative to the current system time. - repeated HeaderMatcher reset_headers = 1 [(validate.rules).repeated = {min_items: 1}]; - - // Specifies the maximum rate limited retry back off interval that Envoy - // will allow. If a rate limited reset header contains an interval longer - // than this then the default exponential back off strategy will be used - // instead. Defaults to 300 seconds. - google.protobuf.Duration reset_max_interval = 2 [(validate.rules).duration = {gt {}}]; + google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}]; } // Specifies the conditions under which retry takes place. These are the same diff --git a/docs/root/configuration/http/http_filters/router_filter.rst b/docs/root/configuration/http/http_filters/router_filter.rst index 3df7c9522c60..0c5ecb353783 100644 --- a/docs/root/configuration/http/http_filters/router_filter.rst +++ b/docs/root/configuration/http/http_filters/router_filter.rst @@ -264,12 +264,6 @@ in flight. The value of the header should be "true" or "false", and is ignored if invalid. -x-envoy-ratelimited-reset-headers -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -Setting this header will extend the list of rate limited reset headers specified in the :ref:`route configuration rate limited retry back off -`. - .. _config_http_filters_router_x-envoy-decorator-operation: x-envoy-decorator-operation diff --git a/generated_api_shadow/envoy/config/route/v3/route_components.proto b/generated_api_shadow/envoy/config/route/v3/route_components.proto index aa84110f4ab1..7a6ab59871e5 100644 --- a/generated_api_shadow/envoy/config/route/v3/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v3/route_components.proto @@ -1048,6 +1048,11 @@ message RouteAction { message RetryPolicy { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.route.RetryPolicy"; + enum ResetHeaderFormat { + SECONDS = 0; + UNIX_TIMESTAMP = 1; + } + message RetryPriority { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.route.RetryPolicy.RetryPriority"; @@ -1094,6 +1099,13 @@ message RetryPolicy { google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}]; } + message ResetHeader { + string name = 1 + [(validate.rules).string = {min_bytes: 1 well_known_regex: HTTP_HEADER_NAME strict: false}]; + + ResetHeaderFormat format = 2 [(validate.rules).enum = {defined_only: true}]; + } + // A retry back-off strategy that applies when the upstream server rate limits // the request. // @@ -1138,48 +1150,16 @@ message RetryPolicy { // to be retried. You will still need to configure the right retry policy to match // the responses from the upstream server. message RateLimitedRetryBackOff { - enum ResetHeaderFormat { - SECONDS = 0; - UNIX_TIMESTAMP = 1; - } - - message ResetHeader { - string name = 1 [ - (validate.rules).string = {min_bytes: 1 well_known_regex: HTTP_HEADER_NAME strict: false} - ]; - - ResetHeaderFormat format = 2 [(validate.rules).enum = {defined_only: true}]; - } - // Specifies the reset headers (like ``Retry-After`` or ``X-RateLimit-Reset``) // to match against the response. Headers are tried in order, and matched case // insensitive. The first header to be parsed successfully is used. If no headers // match the default exponential back-off is used instead. - repeated ResetHeader reset_headers_ng = 3 [(validate.rules).repeated = {min_items: 1}]; + repeated ResetHeader reset_headers = 1 [(validate.rules).repeated = {min_items: 1}]; // Specifies the maximum back off interval that Envoy will allow. If a reset // header contains an interval longer than this then it will be discarded and // the next header will be tried. Defaults to 300 seconds. - google.protobuf.Duration max_interval = 4 [(validate.rules).duration = {gt {}}]; - - // Specifies the rate limited retry reset headers (like ``Retry-After`` or - // ``X-RateLimit-Reset``) to match against the response. If a request is - // being retried and the response contains one of these headers the value - // of the header will be used as the retry back off interval, with jitter - // added. If multiple headers are specified they are tried in the order - // given and the first matching header in the response is used. - // - // The value of the header must be either: - // 1. An interval in seconds as an integer, for example: ``120``. - // 2. A unix timestamp as an integer, for example: ``1595320702``. The timestamp - // must be in the future relative to the current system time. - repeated HeaderMatcher reset_headers = 1 [(validate.rules).repeated = {min_items: 1}]; - - // Specifies the maximum rate limited retry back off interval that Envoy - // will allow. If a rate limited reset header contains an interval longer - // than this then the default exponential back off strategy will be used - // instead. Defaults to 300 seconds. - google.protobuf.Duration reset_max_interval = 2 [(validate.rules).duration = {gt {}}]; + google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}]; } // Specifies the conditions under which retry takes place. These are the same diff --git a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto index 0c53d5332564..1acfdf0afccb 100644 --- a/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v4alpha/route_components.proto @@ -1043,6 +1043,11 @@ message RouteAction { message RetryPolicy { option (udpa.annotations.versioning).previous_message_type = "envoy.config.route.v3.RetryPolicy"; + enum ResetHeaderFormat { + SECONDS = 0; + UNIX_TIMESTAMP = 1; + } + message RetryPriority { option (udpa.annotations.versioning).previous_message_type = "envoy.config.route.v3.RetryPolicy.RetryPriority"; @@ -1093,6 +1098,16 @@ message RetryPolicy { google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}]; } + message ResetHeader { + option (udpa.annotations.versioning).previous_message_type = + "envoy.config.route.v3.RetryPolicy.ResetHeader"; + + string name = 1 + [(validate.rules).string = {min_bytes: 1 well_known_regex: HTTP_HEADER_NAME strict: false}]; + + ResetHeaderFormat format = 2 [(validate.rules).enum = {defined_only: true}]; + } + // A retry back-off strategy that applies when the upstream server rate limits // the request. // @@ -1140,51 +1155,16 @@ message RetryPolicy { option (udpa.annotations.versioning).previous_message_type = "envoy.config.route.v3.RetryPolicy.RateLimitedRetryBackOff"; - enum ResetHeaderFormat { - SECONDS = 0; - UNIX_TIMESTAMP = 1; - } - - message ResetHeader { - option (udpa.annotations.versioning).previous_message_type = - "envoy.config.route.v3.RetryPolicy.RateLimitedRetryBackOff.ResetHeader"; - - string name = 1 [ - (validate.rules).string = {min_bytes: 1 well_known_regex: HTTP_HEADER_NAME strict: false} - ]; - - ResetHeaderFormat format = 2 [(validate.rules).enum = {defined_only: true}]; - } - // Specifies the reset headers (like ``Retry-After`` or ``X-RateLimit-Reset``) // to match against the response. Headers are tried in order, and matched case // insensitive. The first header to be parsed successfully is used. If no headers // match the default exponential back-off is used instead. - repeated ResetHeader reset_headers_ng = 3 [(validate.rules).repeated = {min_items: 1}]; + repeated ResetHeader reset_headers = 1 [(validate.rules).repeated = {min_items: 1}]; // Specifies the maximum back off interval that Envoy will allow. If a reset // header contains an interval longer than this then it will be discarded and // the next header will be tried. Defaults to 300 seconds. - google.protobuf.Duration max_interval = 4 [(validate.rules).duration = {gt {}}]; - - // Specifies the rate limited retry reset headers (like ``Retry-After`` or - // ``X-RateLimit-Reset``) to match against the response. If a request is - // being retried and the response contains one of these headers the value - // of the header will be used as the retry back off interval, with jitter - // added. If multiple headers are specified they are tried in the order - // given and the first matching header in the response is used. - // - // The value of the header must be either: - // 1. An interval in seconds as an integer, for example: ``120``. - // 2. A unix timestamp as an integer, for example: ``1595320702``. The timestamp - // must be in the future relative to the current system time. - repeated HeaderMatcher reset_headers = 1 [(validate.rules).repeated = {min_items: 1}]; - - // Specifies the maximum rate limited retry back off interval that Envoy - // will allow. If a rate limited reset header contains an interval longer - // than this then the default exponential back off strategy will be used - // instead. Defaults to 300 seconds. - google.protobuf.Duration reset_max_interval = 2 [(validate.rules).duration = {gt {}}]; + google.protobuf.Duration max_interval = 2 [(validate.rules).duration = {gt {}}]; } // Specifies the conditions under which retry takes place. These are the same diff --git a/include/envoy/http/BUILD b/include/envoy/http/BUILD index f17ce1cb5e14..c116af28147c 100644 --- a/include/envoy/http/BUILD +++ b/include/envoy/http/BUILD @@ -102,6 +102,7 @@ envoy_cc_library( "abseil_inlined_vector", ], deps = [ + "//include/envoy/common:time_interface", "//source/common/common:assert_lib", "//source/common/common:hash_lib", ], diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index f75c15d10277..7a7bbdba00c7 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -9,6 +9,7 @@ #include #include "envoy/common/pure.h" +#include "envoy/common/time.h" #include "common/common/assert.h" #include "common/common/hash.h" @@ -272,7 +273,6 @@ class HeaderEntry { HEADER_FUNC(EnvoyInternalRequest) \ HEADER_FUNC(EnvoyIpTags) \ HEADER_FUNC(EnvoyMaxRetries) \ - HEADER_FUNC(EnvoyRateLimitedResetHeaders) \ HEADER_FUNC(EnvoyRetryOn) \ HEADER_FUNC(EnvoyRetryGrpcOn) \ HEADER_FUNC(EnvoyRetriableStatusCodes) \ @@ -774,15 +774,6 @@ class HeaderMatcher { public: virtual ~HeaderMatcher() = default; - /** - * This getter exists so that once matchHeaders() has been called on a HeaderMap the concrete - * header in that map can be extracted by using this getter to discover its name. This is possible - * in all cases, except when invert_match_ is true, because in the latter case name_ would *not* - * reflect the matching header. - * @return the header name the matcher will match against. - */ - virtual const LowerCaseString* name() const PURE; - /** * Check whether header matcher matches any headers in a given HeaderMap. */ @@ -791,5 +782,22 @@ class HeaderMatcher { using HeaderMatcherSharedPtr = std::shared_ptr; +/** + * An interface to be implemented by rate limited reset header parsers. + */ +class ResetHeaderParser { +public: + virtual ~ResetHeaderParser() = default; + + /** + * Iterate over the headers, choose the first one that matches by name, and try to parse its + * value. + */ + virtual absl::optional + parseInterval(TimeSource& time_source, const HeaderMap& headers) const PURE; +}; + +using ResetHeaderParserSharedPtr = std::shared_ptr; + } // namespace Http } // namespace Envoy diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 7dc9adf1f62b..a82d21bf65d7 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -237,16 +237,16 @@ class RetryPolicy { virtual absl::optional maxInterval() const PURE; /** - * @return std::vector& list of response header - * matchers that will be attempted to extract a rate limited maximum retry interval. + * @return std::vector& list of reset header + * parsers that will be used to extract a retry back-off interval from response headers. */ - virtual const std::vector& rateLimitedResetHeaders() const PURE; + virtual const std::vector& resetHeaders() const PURE; /** - * @return absl::optional limit placed on a rate limited retry - * interval. + * @return absl::optional upper limit placed on a retry + * back-off interval parsed from response headers. */ - virtual std::chrono::milliseconds rateLimitedResetMaxInterval() const PURE; + virtual std::chrono::milliseconds resetMaxInterval() const PURE; }; /** @@ -312,7 +312,7 @@ class RetryState { * @return the interval if parsing was successful. */ virtual absl::optional - parseRateLimitedResetInterval(const Http::ResponseHeaderMap& response_headers) const PURE; + parseResetInterval(const Http::ResponseHeaderMap& response_headers) const PURE; /** * Determine whether a request should be retried based on the response headers. @@ -394,11 +394,6 @@ class RetryState { * return how many times host selection should be reattempted during host selection. */ virtual uint32_t hostSelectionMaxAttempts() const PURE; - - /** - * @return the rate limited reset headers used to match against rate limited responses. - */ - virtual const std::vector& rateLimitedResetHeaders() const PURE; }; using RetryStatePtr = std::unique_ptr; diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 9b794edb4b9a..f298fc043a9e 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -152,17 +152,17 @@ class AsyncStreamImpl : public AsyncClient::Stream, return absl::nullopt; } absl::optional maxInterval() const override { return absl::nullopt; } - const std::vector& rateLimitedResetHeaders() const override { - return ratelimited_reset_headers_; + const std::vector& resetHeaders() const override { + return reset_headers_; } - std::chrono::milliseconds rateLimitedResetMaxInterval() const override { + std::chrono::milliseconds resetMaxInterval() const override { return std::chrono::milliseconds(300000); } const std::vector retriable_status_codes_{}; const std::vector retriable_headers_{}; const std::vector retriable_request_headers_{}; - const std::vector ratelimited_reset_headers_{}; + const std::vector reset_headers_{}; }; struct NullConfig : public Router::Config { diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index c293f29e16cd..0ba9186ccf30 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -139,6 +139,61 @@ bool HeaderUtility::matchHeaders(const HeaderMap& request_headers, const HeaderD return match != header_data.invert_match_; } +HeaderUtility::ResetHeaderData::ResetHeaderData( + const envoy::config::route::v3::RetryPolicy::ResetHeader& config) + : name_(config.name()) { + switch (config.format()) { + case envoy::config::route::v3::RetryPolicy::SECONDS: + format_ = HeaderUtility::ResetHeaderFormat::Seconds; + break; + case envoy::config::route::v3::RetryPolicy::UNIX_TIMESTAMP: + format_ = HeaderUtility::ResetHeaderFormat::UnixTimestamp; + break; + default: + NOT_REACHED_GCOVR_EXCL_LINE; + } +} + +absl::optional +HeaderUtility::ResetHeaderData::parseInterval(TimeSource& time_source, + const HeaderMap& headers) const { + const HeaderEntry* header = headers.get(name_); + + if (header == nullptr) { + return absl::nullopt; + } + + const auto& header_value = header->value().getStringView(); + uint64_t num_seconds{}; + + switch (format_) { + case HeaderUtility::ResetHeaderFormat::Seconds: + if (absl::SimpleAtoi(header_value, &num_seconds)) { + return absl::optional(num_seconds * 1000UL); + } + break; + + case HeaderUtility::ResetHeaderFormat::UnixTimestamp: + if (absl::SimpleAtoi(header_value, &num_seconds)) { + const auto time_now = time_source.systemTime().time_since_epoch(); + const uint64_t timestamp = std::chrono::duration_cast(time_now).count(); + + if (num_seconds < timestamp) { + return absl::nullopt; + } + + const uint64_t interval = num_seconds - timestamp; + return absl::optional(interval * 1000UL); + } + break; + + default: + NOT_REACHED_GCOVR_EXCL_LINE; + } + + return absl::nullopt; +} + bool HeaderUtility::headerValueIsValid(const absl::string_view header_value) { return nghttp2_check_header_value(reinterpret_cast(header_value.data()), header_value.size()) != 0; diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index 7304286f5d4c..92ec62b83c51 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -20,6 +20,7 @@ namespace Http { class HeaderUtility { public: enum class HeaderMatchType { Value, Regex, Range, Present, Prefix, Suffix }; + enum class ResetHeaderFormat { Seconds, UnixTimestamp }; /** * Get all instances of the header key specified, and return the values in the vector provided. @@ -47,13 +48,6 @@ class HeaderUtility { const bool invert_match_; // HeaderMatcher - const LowerCaseString* name() const override { - if (invert_match_) { - return nullptr; - } - return &name_; - } - bool matchesHeaders(const HeaderMap& headers) const override { return HeaderUtility::matchHeaders(headers, *this); }; @@ -97,6 +91,35 @@ class HeaderUtility { static bool matchHeaders(const HeaderMap& request_headers, const HeaderData& config_header); + // A ResetHeaderData specifies a header name and a format to match against + // response headers that are used to signal a rate limit interval reset, such + // as Retry-After or X-RateLimit-Reset. + struct ResetHeaderData : public ResetHeaderParser { + ResetHeaderData(const envoy::config::route::v3::RetryPolicy::ResetHeader& config); + + const LowerCaseString name_; + ResetHeaderFormat format_; + + // RateLimitedResetHeaderParser + virtual absl::optional + parseInterval(TimeSource& time_source, const HeaderMap& headers) const override; + }; + + using ResetHeaderDataPtr = std::unique_ptr; + + /** + * Build a vector of ResetHeaderParserSharedPtr given input config. + */ + static std::vector buildResetHeaderParserVector( + const Protobuf::RepeatedPtrField& + reset_headers) { + std::vector ret; + for (const auto& reset_header : reset_headers) { + ret.emplace_back(std::make_shared(reset_header)); + } + return ret; + } + /** * Validates that a header value is valid, according to RFC 7230, section 3.2. * http://tools.ietf.org/html/rfc7230#section-3.2 @@ -165,5 +188,6 @@ class HeaderUtility { */ static void stripPortFromHost(RequestHeaderMap& headers, uint32_t listener_port); }; + } // namespace Http } // namespace Envoy diff --git a/source/common/http/headers.h b/source/common/http/headers.h index be09eb175d19..73f866f7b60f 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -157,8 +157,6 @@ class HeaderValues { const LowerCaseString EnvoyOriginalPath{absl::StrCat(prefix(), "-original-path")}; const LowerCaseString EnvoyOverloaded{absl::StrCat(prefix(), "-overloaded")}; const LowerCaseString EnvoyRateLimited{absl::StrCat(prefix(), "-ratelimited")}; - const LowerCaseString EnvoyRateLimitedResetHeaders{ - absl::StrCat(prefix(), "-ratelimited-reset-headers")}; const LowerCaseString EnvoyRetryOn{absl::StrCat(prefix(), "-retry-on")}; const LowerCaseString EnvoyRetryGrpcOn{absl::StrCat(prefix(), "-retry-grpc-on")}; const LowerCaseString EnvoyRetriableStatusCodes{ diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index ff898e101aae..b25efda14b67 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -122,17 +122,17 @@ RetryPolicyImpl::RetryPolicyImpl(const envoy::config::route::v3::RetryPolicy& re } if (retry_policy.has_rate_limited_retry_back_off()) { - ratelimited_reset_headers_ = Http::HeaderUtility::buildHeaderMatcherVector( + reset_headers_ = Http::HeaderUtility::buildResetHeaderParserVector( retry_policy.rate_limited_retry_back_off().reset_headers()); - absl::optional ratelimited_reset_max_interval = - PROTOBUF_GET_OPTIONAL_MS(retry_policy.rate_limited_retry_back_off(), reset_max_interval); - if (ratelimited_reset_max_interval.has_value()) { - std::chrono::milliseconds max_interval = ratelimited_reset_max_interval.value(); + absl::optional reset_max_interval = + PROTOBUF_GET_OPTIONAL_MS(retry_policy.rate_limited_retry_back_off(), max_interval); + if (reset_max_interval.has_value()) { + std::chrono::milliseconds max_interval = reset_max_interval.value(); if (max_interval.count() < 1) { max_interval = std::chrono::milliseconds(1); } - ratelimited_reset_max_interval_ = max_interval; + reset_max_interval_ = max_interval; } } } diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 79ca406567a1..87eb4d521384 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -272,12 +272,10 @@ class RetryPolicyImpl : public RetryPolicy { } absl::optional baseInterval() const override { return base_interval_; } absl::optional maxInterval() const override { return max_interval_; } - const std::vector& rateLimitedResetHeaders() const override { - return ratelimited_reset_headers_; - } - std::chrono::milliseconds rateLimitedResetMaxInterval() const override { - return ratelimited_reset_max_interval_; + const std::vector& resetHeaders() const override { + return reset_headers_; } + std::chrono::milliseconds resetMaxInterval() const override { return reset_max_interval_; } private: std::chrono::milliseconds per_try_timeout_{0}; @@ -300,8 +298,8 @@ class RetryPolicyImpl : public RetryPolicy { std::vector retriable_request_headers_; absl::optional base_interval_; absl::optional max_interval_; - std::vector ratelimited_reset_headers_{}; - std::chrono::milliseconds ratelimited_reset_max_interval_{300000}; + std::vector reset_headers_{}; + std::chrono::milliseconds reset_max_interval_{300000}; ProtobufMessage::ValidationVisitor* validation_visitor_{}; }; diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc index 858f712ba263..938850c47f42 100644 --- a/source/common/router/retry_state_impl.cc +++ b/source/common/router/retry_state_impl.cc @@ -52,7 +52,6 @@ RetryStatePtr RetryStateImpl::create(const RetryPolicy& route_policy, request_headers.removeEnvoyRetryOn(); request_headers.removeEnvoyRetryGrpcOn(); request_headers.removeEnvoyMaxRetries(); - request_headers.removeEnvoyRateLimitedResetHeaders(); if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.consume_all_retry_headers")) { request_headers.removeEnvoyHedgeOnPerTryTimeout(); request_headers.removeEnvoyRetriableHeaderNames(); @@ -76,8 +75,8 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, retry_priority_(route_policy.retryPriority()), retriable_status_codes_(route_policy.retriableStatusCodes()), retriable_headers_(route_policy.retriableHeaders()), - ratelimited_reset_headers_(route_policy.rateLimitedResetHeaders()), - ratelimited_reset_max_interval_(route_policy.rateLimitedResetMaxInterval()) { + reset_headers_(route_policy.resetHeaders()), + reset_max_interval_(route_policy.resetMaxInterval()) { std::chrono::milliseconds base_interval( runtime_.snapshot().getInteger("upstream.base_retry_backoff_ms", 25)); @@ -151,21 +150,6 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, std::make_shared(header_matcher)); } } - - if (request_headers.EnvoyRateLimitedResetHeaders()) { - // Ratelimit reset headers in the configuration are specified via HeaderMatcher. - // Giving the same flexibility via request header would require the user - // to provide a HeaderMatcher serialized into a string. To avoid this extra - // complexity we only support name-only header matchers via request - // header. Anything more sophisticated needs to be provided via config. - for (const auto header_name : StringUtil::splitToken( - request_headers.EnvoyRateLimitedResetHeaders()->value().getStringView(), ",")) { - envoy::config::route::v3::HeaderMatcher header_matcher; - header_matcher.set_name(std::string(absl::StripAsciiWhitespace(header_name))); - ratelimited_reset_headers_.emplace_back( - std::make_shared(header_matcher)); - } - } } RetryStateImpl::~RetryStateImpl() { resetRetry(); } @@ -246,45 +230,12 @@ std::pair RetryStateImpl::parseRetryGrpcOn(absl::string_view ret return {ret, all_fields_valid}; } -absl::optional RetryStateImpl::parseRateLimitedResetInterval( - const Http::ResponseHeaderMap& response_headers) const { - bool parsed_value = false; - uint64_t num_seconds{}; - - for (const auto& reset_header : ratelimited_reset_headers_) { - if (reset_header->matchesHeaders(response_headers)) { - const Http::LowerCaseString* header_name = reset_header->name(); - - if (header_name != nullptr) { - const Http::HeaderEntry* entry = response_headers.get(*header_name); - - if (entry != nullptr) { - const auto& header_value = entry->value().getStringView(); - - // Try to parse the value of the header as an int storing the number of seconds - if (absl::SimpleAtoi(header_value, &num_seconds)) { - parsed_value = true; - break; - } - } - } - } - } - - if (parsed_value) { - const auto time_now = time_source_.systemTime().time_since_epoch(); - uint64_t timestamp = std::chrono::duration_cast(time_now).count(); - - // Is the value big enough to be a unix timestamp rather than an interval? - if (num_seconds > timestamp) { - num_seconds = num_seconds - timestamp; - } - - const auto interval = std::chrono::milliseconds(num_seconds * 1000UL); - - // Is the interval value within our accepted range? - if (interval <= ratelimited_reset_max_interval_) { - return absl::optional(interval); +absl::optional +RetryStateImpl::parseResetInterval(const Http::ResponseHeaderMap& response_headers) const { + for (const auto& reset_header : reset_headers_) { + const auto interval = reset_header->parseInterval(time_source_, response_headers); + if (interval.has_value() && interval.value() <= reset_max_interval_) { + return interval; } } @@ -356,8 +307,8 @@ RetryStatus RetryStateImpl::shouldRetryHeaders(const Http::ResponseHeaderMap& re // Yes, we will retry based on the headers - try to parse a rate limited reset interval from the // response. - if (would_retry && !ratelimited_reset_headers_.empty()) { - const auto backoff_interval = parseRateLimitedResetInterval(response_headers); + if (would_retry && !reset_headers_.empty()) { + const auto backoff_interval = parseResetInterval(response_headers); if (backoff_interval.has_value()) { ratelimited_backoff_strategy_ = std::make_unique( backoff_interval.value().count(), random_); diff --git a/source/common/router/retry_state_impl.h b/source/common/router/retry_state_impl.h index d7875ee9153f..a89566338260 100644 --- a/source/common/router/retry_state_impl.h +++ b/source/common/router/retry_state_impl.h @@ -54,7 +54,7 @@ class RetryStateImpl : public RetryState { // Router::RetryState bool enabled() override { return retry_on_ != 0; } absl::optional - parseRateLimitedResetInterval(const Http::ResponseHeaderMap& response_headers) const override; + parseResetInterval(const Http::ResponseHeaderMap& response_headers) const override; RetryStatus shouldRetryHeaders(const Http::ResponseHeaderMap& response_headers, DoRetryCallback callback) override; // Returns true if the retry policy would retry the passed headers. Does not @@ -91,10 +91,6 @@ class RetryStateImpl : public RetryState { uint32_t hostSelectionMaxAttempts() const override { return host_selection_max_attempts_; } - const std::vector& rateLimitedResetHeaders() const override { - return ratelimited_reset_headers_; - } - private: RetryStateImpl(const RetryPolicy& route_policy, Http::RequestHeaderMap& request_headers, const Upstream::ClusterInfo& cluster, const VirtualCluster* vcluster, @@ -125,8 +121,8 @@ class RetryStateImpl : public RetryState { uint32_t host_selection_max_attempts_; std::vector retriable_status_codes_; std::vector retriable_headers_; - std::vector ratelimited_reset_headers_{}; - std::chrono::milliseconds ratelimited_reset_max_interval_{}; + std::vector reset_headers_{}; + std::chrono::milliseconds reset_max_interval_{}; }; } // namespace Router diff --git a/test/common/http/BUILD b/test/common/http/BUILD index b317052c4414..9c526dce39df 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -323,6 +323,7 @@ envoy_cc_test( srcs = ["header_utility_test.cc"], deps = [ "//source/common/http:header_utility_lib", + "//test/test_common:simulated_time_system_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/route/v3:pkg_cc_proto", ], diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index a88024a64a3b..15c5b59add7b 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -8,6 +8,7 @@ #include "common/http/header_utility.h" #include "common/json/json_loader.h" +#include "test/test_common/simulated_time_system.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" @@ -499,35 +500,194 @@ invert_match: true EXPECT_FALSE(HeaderUtility::matchHeaders(unmatching_headers, header_data)); } -TEST(MatchHeadersTest, HeaderNameGetter) { - TestRequestHeaderMapImpl matching_headers{{"match-header", "value"}}; +envoy::config::route::v3::RetryPolicy::ResetHeader +parseResetHeaderParserFromYaml(const std::string& yaml) { + envoy::config::route::v3::RetryPolicy::ResetHeader reset_header; + TestUtility::loadFromYaml(yaml, reset_header); + return reset_header; +} + +TEST(ResetHeaderDataConstructorTest, FormatUnset) { const std::string yaml = R"EOF( -name: match-header +name: retry-after )EOF"; - std::vector header_data; - header_data.push_back( - std::make_unique(parseHeaderMatcherFromYaml(yaml))); - EXPECT_TRUE(HeaderUtility::matchHeaders(matching_headers, header_data)); + HeaderUtility::ResetHeaderData reset_header_data = + HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); - const LowerCaseString* header_name = header_data[0]->name(); - EXPECT_EQ("match-header", header_name->get()); + EXPECT_EQ("retry-after", reset_header_data.name_.get()); + EXPECT_EQ(HeaderUtility::ResetHeaderFormat::Seconds, reset_header_data.format_); } -TEST(MatchHeadersTest, HeaderNameGetterInverse) { - TestRequestHeaderMapImpl unmatching_headers{{"other-header", "value"}}; +TEST(ResetHeaderDataConstructorTest, FormatSeconds) { const std::string yaml = R"EOF( -name: match-header -invert_match: true +name: retry-after +format: SECONDS )EOF"; - std::vector header_data; - header_data.push_back( - std::make_unique(parseHeaderMatcherFromYaml(yaml))); - EXPECT_TRUE(HeaderUtility::matchHeaders(unmatching_headers, header_data)); + HeaderUtility::ResetHeaderData reset_header_data = + HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); + + EXPECT_EQ("retry-after", reset_header_data.name_.get()); + EXPECT_EQ(HeaderUtility::ResetHeaderFormat::Seconds, reset_header_data.format_); +} + +TEST(ResetHeaderDataConstructorTest, FormatUnixTimestamp) { + const std::string yaml = R"EOF( +name: retry-after +format: UNIX_TIMESTAMP + )EOF"; + + HeaderUtility::ResetHeaderData reset_header_data = + HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); + + EXPECT_EQ("retry-after", reset_header_data.name_.get()); + EXPECT_EQ(HeaderUtility::ResetHeaderFormat::UnixTimestamp, reset_header_data.format_); +} + +class ResetHeaderDataParseIntervalTest : public testing::Test { +public: + ResetHeaderDataParseIntervalTest() { + const time_t known_date_time = 1000000000; + test_time_.setSystemTime(std::chrono::system_clock::from_time_t(known_date_time)); + } + + Event::SimulatedTimeSystem test_time_; +}; + +TEST_F(ResetHeaderDataParseIntervalTest, NoHeaderMatches) { + const std::string yaml = R"EOF( +name: retry-after +format: SECONDS + )EOF"; + + HeaderUtility::ResetHeaderData reset_header_data = + HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); + + TestResponseHeaderMapImpl response_headers{{":status", "200"}}; + + EXPECT_EQ(absl::nullopt, + reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); +} + +TEST_F(ResetHeaderDataParseIntervalTest, HeaderMatchesButUnsupportedFormatDate) { + const std::string yaml = R"EOF( +name: retry-after +format: SECONDS + )EOF"; + + HeaderUtility::ResetHeaderData reset_header_data = + HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); + + TestResponseHeaderMapImpl response_headers{{"retry-after", "Fri, 17 Jul 2020 11:59:51 GMT"}}; + + EXPECT_EQ(absl::nullopt, + reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); +} + +TEST_F(ResetHeaderDataParseIntervalTest, HeaderMatchesButUnsupportedFormatFloat) { + const std::string yaml = R"EOF( +name: retry-after +format: SECONDS + )EOF"; + + HeaderUtility::ResetHeaderData reset_header_data = + HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); + + TestResponseHeaderMapImpl response_headers{{"retry-after", "2.5"}}; + + EXPECT_EQ(absl::nullopt, + reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); +} + +TEST_F(ResetHeaderDataParseIntervalTest, HeaderMatchesSupportedFormatSeconds) { + const std::string yaml = R"EOF( +name: retry-after +format: SECONDS + )EOF"; + + HeaderUtility::ResetHeaderData reset_header_data = + HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); + + TestResponseHeaderMapImpl response_headers{{"retry-after", "5"}}; + + EXPECT_EQ(absl::optional(5000), + reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); +} + +TEST_F(ResetHeaderDataParseIntervalTest, HeaderMatchesSupportedFormatSecondsCaseInsensitive) { + const std::string yaml = R"EOF( +name: retry-after +format: SECONDS + )EOF"; + + HeaderUtility::ResetHeaderData reset_header_data = + HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); + + TestResponseHeaderMapImpl response_headers{{"Retry-After", "5"}}; + + EXPECT_EQ(absl::optional(5000), + reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); +} + +TEST_F(ResetHeaderDataParseIntervalTest, HeaderMatchesButUnsupportedFormatTimestampFloat) { + const std::string yaml = R"EOF( +name: retry-after +format: UNIX_TIMESTAMP + )EOF"; + + HeaderUtility::ResetHeaderData reset_header_data = + HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); + + TestResponseHeaderMapImpl response_headers{{"retry-after", "1595320702.1234"}}; + + EXPECT_EQ(absl::nullopt, + reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); +} + +TEST_F(ResetHeaderDataParseIntervalTest, HeaderMatchesSupportedFormatTimestampButInThePast) { + const std::string yaml = R"EOF( +name: retry-after +format: UNIX_TIMESTAMP + )EOF"; + + HeaderUtility::ResetHeaderData reset_header_data = + HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); + + TestResponseHeaderMapImpl response_headers{{"retry-after", "999999999"}}; + + EXPECT_EQ(absl::nullopt, + reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); +} + +TEST_F(ResetHeaderDataParseIntervalTest, HeaderMatchesSupportedFormatTimestampEmptyInterval) { + const std::string yaml = R"EOF( +name: retry-after +format: UNIX_TIMESTAMP + )EOF"; + + HeaderUtility::ResetHeaderData reset_header_data = + HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); + + TestResponseHeaderMapImpl response_headers{{"retry-after", "1000000000"}}; + + EXPECT_EQ(absl::optional(0), + reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); +} + +TEST_F(ResetHeaderDataParseIntervalTest, HeaderMatchesSupportedFormatTimestampNonEmptyInterval) { + const std::string yaml = R"EOF( +name: retry-after +format: UNIX_TIMESTAMP + )EOF"; + + HeaderUtility::ResetHeaderData reset_header_data = + HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); + + TestResponseHeaderMapImpl response_headers{{"retry-after", "1000000007"}}; - const LowerCaseString* header_name = header_data[0]->name(); - EXPECT_EQ(nullptr, header_name); + EXPECT_EQ(absl::optional(7000), + reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); } TEST(HeaderIsValidTest, InvalidHeaderValuesAreRejected) { diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index aab5da6e2a86..7facc7a0004b 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -306,6 +306,7 @@ most_specific_header_mutations_wins: {0} Stats::TestSymbolTable symbol_table_; Api::ApiPtr api_; NiceMock factory_context_; + Event::SimulatedTimeSystem test_time_; }; class RouteMatcherTest : public testing::Test, public ConfigImplTestBase {}; @@ -3572,7 +3573,8 @@ TEST_F(RouteMatcherTest, RateLimitedRetryBackOff) { rate_limited_retry_back_off: reset_headers: - name: Retry-After - reset_max_interval: 0.0001s # < 1 ms + format: SECONDS + max_interval: 0.0001s # < 1 ms - match: prefix: "/typical-backoff" route: @@ -3581,43 +3583,55 @@ TEST_F(RouteMatcherTest, RateLimitedRetryBackOff) { rate_limited_retry_back_off: reset_headers: - name: Retry-After + format: SECONDS - name: RateLimit-Reset - reset_max_interval: 0.050s + format: UNIX_TIMESTAMP + max_interval: 0.050s )EOF"; + const time_t known_date_time = 1000000000; + test_time_.setSystemTime(std::chrono::system_clock::from_time_t(known_date_time)); + TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true); // has no ratelimit retry back off + EXPECT_EQ(true, config.route(genHeaders("www.lyft.com", "/no-backoff", "GET"), 0) + ->routeEntry() + ->retryPolicy() + .resetHeaders() + .empty()); EXPECT_EQ(std::chrono::milliseconds(300000), config.route(genHeaders("www.lyft.com", "/no-backoff", "GET"), 0) ->routeEntry() ->retryPolicy() - .rateLimitedResetMaxInterval()); - EXPECT_EQ(0, config.route(genHeaders("www.lyft.com", "/no-backoff", "GET"), 0) + .resetMaxInterval()); + + // has sub millisecond interval + EXPECT_EQ(1, config.route(genHeaders("www.lyft.com", "/sub-ms-interval", "GET"), 0) ->routeEntry() ->retryPolicy() - .rateLimitedResetHeaders() + .resetHeaders() .size()); - - // has sub millisecond interval EXPECT_EQ(std::chrono::milliseconds(1), config.route(genHeaders("www.lyft.com", "/sub-ms-interval", "GET"), 0) ->routeEntry() ->retryPolicy() - .rateLimitedResetMaxInterval()); + .resetMaxInterval()); - // has a ratelimit retry back off + // a typical configuration Http::TestRequestHeaderMapImpl headers = genHeaders("www.lyft.com", "/typical-backoff", "GET"); const auto& retry_policy = config.route(headers, 0)->routeEntry()->retryPolicy(); - EXPECT_EQ(2, retry_policy.rateLimitedResetHeaders().size()); + EXPECT_EQ(2, retry_policy.resetHeaders().size()); - Http::TestResponseHeaderMapImpl expected_0{{"Retry-After", "not-used"}}; - Http::TestResponseHeaderMapImpl expected_1{{"RateLimit-Reset", "not-used"}}; + Http::TestResponseHeaderMapImpl expected_0{{"Retry-After", "2"}}; + Http::TestResponseHeaderMapImpl expected_1{{"RateLimit-Reset", "1000000005"}}; - EXPECT_TRUE(retry_policy.rateLimitedResetHeaders()[0]->matchesHeaders(expected_0)); - EXPECT_TRUE(retry_policy.rateLimitedResetHeaders()[1]->matchesHeaders(expected_1)); + EXPECT_EQ(std::chrono::milliseconds(2000), + retry_policy.resetHeaders()[0]->parseInterval(test_time_.timeSystem(), expected_0)); + EXPECT_EQ(std::chrono::milliseconds(5000), + retry_policy.resetHeaders()[1]->parseInterval(test_time_.timeSystem(), expected_1)); - EXPECT_EQ(std::chrono::milliseconds(50), retry_policy.rateLimitedResetMaxInterval()); + EXPECT_EQ(std::chrono::milliseconds(50), retry_policy.resetMaxInterval()); } TEST_F(RouteMatcherTest, HedgeRouteLevel) { diff --git a/test/common/router/retry_state_impl_test.cc b/test/common/router/retry_state_impl_test.cc index f2abeedbf36e..4a751225acdf 100644 --- a/test/common/router/retry_state_impl_test.cc +++ b/test/common/router/retry_state_impl_test.cc @@ -927,161 +927,104 @@ TEST_F(RouterRetryStateImplTest, CustomBackOffIntervalDefaultMax) { retry_timer_->invokeCallback(); } -TEST_F(RouterRetryStateImplTest, ParseRateLimitedResetHeaders) { - Protobuf::RepeatedPtrField matchers; - auto* matcher = matchers.Add(); - matcher->set_name("Retry-After"); - - policy_.ratelimited_reset_headers_ = Http::HeaderUtility::buildHeaderMatcherVector(matchers); - - Http::TestRequestHeaderMapImpl request_headers{ - {"x-envoy-retry-on", "5xx"}, - {"x-envoy-ratelimited-reset-headers", "RateLimit-Reset, X-RateLimit-Reset"}}; - setup(request_headers); - EXPECT_TRUE(state_->enabled()); - - Http::TestResponseHeaderMapImpl expected_0{{"Retry-After", "not-used"}}; - Http::TestResponseHeaderMapImpl expected_1{{"RateLimit-Reset", "not-used"}}; - Http::TestResponseHeaderMapImpl expected_2{{"X-RateLimit-Reset", "not-used"}}; - - EXPECT_TRUE(state_->rateLimitedResetHeaders()[0]->matchesHeaders(expected_0)); - EXPECT_TRUE(state_->rateLimitedResetHeaders()[1]->matchesHeaders(expected_1)); - EXPECT_TRUE(state_->rateLimitedResetHeaders()[2]->matchesHeaders(expected_2)); -} - TEST_F(RouterRetryStateImplTest, ParseRateLimitedResetInterval) { // Set a fixed system time to be used for all these tests const time_t known_date_time = 1000000000; test_time_.setSystemTime(std::chrono::system_clock::from_time_t(known_date_time)); - // Failure case: The reset header name doesn't match - { - Http::TestRequestHeaderMapImpl request_headers{ - {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-headers", "RateLimit-Reset"}}; - setup(request_headers); - EXPECT_TRUE(state_->enabled()); - - Http::TestResponseHeaderMapImpl response_headers{{":status", "429"}, {"Retry-After", "2"}}; - EXPECT_EQ(absl::nullopt, state_->parseRateLimitedResetInterval(response_headers)); - } + Protobuf::RepeatedPtrField reset_headers; + auto* reset_header_1 = reset_headers.Add(); + reset_header_1->set_name("Retry-After"); + reset_header_1->set_format(envoy::config::route::v3::RetryPolicy::SECONDS); - // Failure case: The reset header value is not a valid interval - { - Http::TestRequestHeaderMapImpl request_headers{ - {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-headers", "Retry-After"}}; - setup(request_headers); - EXPECT_TRUE(state_->enabled()); + auto* reset_header_2 = reset_headers.Add(); + reset_header_2->set_name("X-RateLimit-Reset"); + reset_header_2->set_format(envoy::config::route::v3::RetryPolicy::UNIX_TIMESTAMP); - Http::TestResponseHeaderMapImpl response_headers{ - {":status", "429"}, {"Retry-After", "Fri, 17 Jul 2020 11:59:51 GMT"}}; - EXPECT_EQ(absl::nullopt, state_->parseRateLimitedResetInterval(response_headers)); - } + policy_.reset_headers_ = Http::HeaderUtility::buildResetHeaderParserVector(reset_headers); - // Failure case: The reset header value is a valid interval but out of range (5min+) + // Failure case: Matches reset header (seconds) but exceeds max_interval (>5min) { - Http::TestRequestHeaderMapImpl request_headers{ - {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-headers", "Retry-After"}}; + Http::TestRequestHeaderMapImpl request_headers{{"x-envoy-retry-on", "5xx"}}; setup(request_headers); EXPECT_TRUE(state_->enabled()); Http::TestResponseHeaderMapImpl response_headers{{":status", "429"}, {"Retry-After", "301"}}; - EXPECT_EQ(absl::nullopt, state_->parseRateLimitedResetInterval(response_headers)); - } - - // The only reset header matches and the header value is in within range - { - Http::TestRequestHeaderMapImpl request_headers{ - {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-headers", "Retry-After"}}; - setup(request_headers); - EXPECT_TRUE(state_->enabled()); - - Http::TestResponseHeaderMapImpl response_headers{{":status", "429"}, {"Retry-After", "300"}}; - EXPECT_EQ(absl::optional(300000), - state_->parseRateLimitedResetInterval(response_headers)); + EXPECT_EQ(absl::nullopt, state_->parseResetInterval(response_headers)); } - // The second reset header matches and the header value is in within range + // Failure case: Matches reset header (timestamp) but exceeds max_interval (>5min) { - Http::TestRequestHeaderMapImpl request_headers{ - {"x-envoy-retry-on", "5xx"}, - {"x-envoy-ratelimited-reset-headers", "Retry-After, X-RateLimit-Reset"}}; + Http::TestRequestHeaderMapImpl request_headers{{"x-envoy-retry-on", "5xx"}}; setup(request_headers); EXPECT_TRUE(state_->enabled()); Http::TestResponseHeaderMapImpl response_headers{{":status", "429"}, - {"x-ratelimit-reset", "2"}}; - EXPECT_EQ(absl::optional(2000), - state_->parseRateLimitedResetInterval(response_headers)); + {"X-RateLimit-Reset", "1000000301"}}; + EXPECT_EQ(absl::nullopt, state_->parseResetInterval(response_headers)); } - // The second and third reset header match and the first of these two is used + // The only reset header matches (seconds) and the header value is in within range { - Http::TestRequestHeaderMapImpl request_headers{ - {"x-envoy-retry-on", "5xx"}, - {"x-envoy-ratelimited-reset-headers", "Retry-After,X-RateLimit-Reset,X-Retry-After"}}; - setup(request_headers); - EXPECT_TRUE(state_->enabled()); - - Http::TestResponseHeaderMapImpl response_headers{ - {":status", "429"}, {"x-ratelimit-reset", "2"}, {"X-Retry-After", "3"}}; - EXPECT_EQ(absl::optional(2000), - state_->parseRateLimitedResetInterval(response_headers)); - } - - // The header value is a timestamp before now() - { - Http::TestRequestHeaderMapImpl request_headers{ - {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-headers", "Retry-After"}}; + Http::TestRequestHeaderMapImpl request_headers{{"x-envoy-retry-on", "5xx"}}; setup(request_headers); EXPECT_TRUE(state_->enabled()); - Http::TestResponseHeaderMapImpl response_headers{{":status", "429"}, - {"retry-after", "999999999"}}; - EXPECT_EQ(absl::nullopt, state_->parseRateLimitedResetInterval(response_headers)); + Http::TestResponseHeaderMapImpl response_headers{{":status", "429"}, {"Retry-After", "300"}}; + EXPECT_EQ(absl::optional(300000), + state_->parseResetInterval(response_headers)); } - // The header value is a timestamp after now() but out of range (5min+) + // The only reset header matches (timestamp) and the header value is in within range { - Http::TestRequestHeaderMapImpl request_headers{ - {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-headers", "Retry-After"}}; + Http::TestRequestHeaderMapImpl request_headers{{"x-envoy-retry-on", "5xx"}}; setup(request_headers); EXPECT_TRUE(state_->enabled()); Http::TestResponseHeaderMapImpl response_headers{{":status", "429"}, - {"retry-after", "1000000301"}}; - EXPECT_EQ(absl::nullopt, state_->parseRateLimitedResetInterval(response_headers)); + {"x-ratelimit-reset", "1000000300"}}; + EXPECT_EQ(absl::optional(300000), + state_->parseResetInterval(response_headers)); } - // The header value is a timestamp after now() and within range (<=5min) + // The second (timestamp) and third (seconds) reset headers match but Retry-After comes first in + // reset_headers so it is used { - Http::TestRequestHeaderMapImpl request_headers{ - {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-headers", "Retry-After"}}; + Http::TestRequestHeaderMapImpl request_headers{{"x-envoy-retry-on", "5xx"}}; setup(request_headers); EXPECT_TRUE(state_->enabled()); - Http::TestResponseHeaderMapImpl response_headers{{":status", "429"}, - {"retry-after", "1000000300"}}; - EXPECT_EQ(absl::optional(300000), - state_->parseRateLimitedResetInterval(response_headers)); + Http::TestResponseHeaderMapImpl response_headers{ + {":status", "429"}, {"x-ratelimit-reset", "1000000002"}, {"retry-after", "3"}}; + EXPECT_EQ(absl::optional(3000), + state_->parseResetInterval(response_headers)); } } TEST_F(RouterRetryStateImplTest, RateLimitedRetryBackoffStrategy) { + Protobuf::RepeatedPtrField reset_headers; + auto* reset_header = reset_headers.Add(); + reset_header->set_name("Retry-After"); + reset_header->set_format(envoy::config::route::v3::RetryPolicy::SECONDS); + policy_.num_retries_ = 3; + policy_.reset_headers_ = Http::HeaderUtility::buildResetHeaderParserVector(reset_headers); - Http::TestRequestHeaderMapImpl request_headers{ - {"x-envoy-retry-on", "5xx"}, {"x-envoy-ratelimited-reset-headers", "Retry-After"}}; + Http::TestRequestHeaderMapImpl request_headers{{"x-envoy-retry-on", "5xx"}}; setup(request_headers); EXPECT_TRUE(state_->enabled()); retry_timer_ = new Event::MockTimer(&dispatcher_); - Http::TestResponseHeaderMapImpl response_headers_reset{{":status", "500"}, {"retry-after", "2"}}; + Http::TestResponseHeaderMapImpl response_headers_reset_1{{":status", "500"}, + {"retry-after", "2"}}; Http::TestResponseHeaderMapImpl response_headers_plain{{":status", "500"}}; + Http::TestResponseHeaderMapImpl response_headers_reset_2{{":status", "500"}, + {"retry-after", "5"}}; // reset header present -> ratelimit backoff used EXPECT_CALL(random_, random()).WillOnce(Return(190)); EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(2190), _)); - EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryHeaders(response_headers_reset, callback_)); + EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryHeaders(response_headers_reset_1, callback_)); EXPECT_CALL(callback_ready_, ready()); retry_timer_->invokeCallback(); @@ -1093,14 +1036,14 @@ TEST_F(RouterRetryStateImplTest, RateLimitedRetryBackoffStrategy) { retry_timer_->invokeCallback(); // reset header present -> ratelimit backoff used - EXPECT_CALL(random_, random()).WillOnce(Return(190)); - EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(2190), _)); - EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryHeaders(response_headers_reset, callback_)); + EXPECT_CALL(random_, random()).WillOnce(Return(2190)); + EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(7190), _)); + EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryHeaders(response_headers_reset_2, callback_)); EXPECT_CALL(callback_ready_, ready()); retry_timer_->invokeCallback(); EXPECT_EQ(RetryStatus::NoRetryLimitExceeded, - state_->shouldRetryHeaders(response_headers_reset, callback_)); + state_->shouldRetryHeaders(response_headers_reset_2, callback_)); EXPECT_EQ(2UL, cluster_.stats().upstream_rq_retry_backoff_ratelimited_.value()); EXPECT_EQ(1UL, cluster_.stats().upstream_rq_retry_backoff_exponential_.value()); @@ -1325,7 +1268,6 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { Http::TestRequestHeaderMapImpl request_headers{ {"x-envoy-retry-on", "5xx,retriable-header-names,retriable-status-codes"}, {"x-envoy-retry-grpc-on", "resource-exhausted"}, - {"x-envoy-ratelimited-reset-headers", "Retry-After"}, {"x-envoy-retriable-header-names", "X-Upstream-Pushback"}, {"x-envoy-retriable-status-codes", "418,420"}, {"x-envoy-max-retries", "7"}, @@ -1338,7 +1280,6 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { EXPECT_FALSE(request_headers.has("x-envoy-retry-on")); EXPECT_FALSE(request_headers.has("x-envoy-retry-grpc-on")); EXPECT_FALSE(request_headers.has("x-envoy-max-retries")); - EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-headers")); EXPECT_FALSE(request_headers.has("x-envoy-retriable-header-names")); EXPECT_FALSE(request_headers.has("x-envoy-retriable-status-codes")); EXPECT_FALSE(request_headers.has("x-envoy-hedge-on-per-try-timeout")); @@ -1348,7 +1289,6 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { // Make sure retry related headers are removed even if the policy is disabled. { Http::TestRequestHeaderMapImpl request_headers{ - {"x-envoy-ratelimited-reset-headers", "Retry-After"}, {"x-envoy-retriable-header-names", "X-Upstream-Pushback"}, {"x-envoy-retriable-status-codes", "418,420"}, {"x-envoy-max-retries", "7"}, @@ -1361,7 +1301,6 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { EXPECT_FALSE(request_headers.has("x-envoy-retry-on")); EXPECT_FALSE(request_headers.has("x-envoy-retry-grpc-on")); EXPECT_FALSE(request_headers.has("x-envoy-max-retries")); - EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-headers")); EXPECT_FALSE(request_headers.has("x-envoy-retriable-header-names")); EXPECT_FALSE(request_headers.has("x-envoy-retriable-status-codes")); EXPECT_FALSE(request_headers.has("x-envoy-hedge-on-per-try-timeout")); @@ -1377,7 +1316,6 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { Http::TestRequestHeaderMapImpl request_headers{ {"x-envoy-retry-on", "5xx,retriable-header-names,retriable-status-codes"}, {"x-envoy-retry-grpc-on", "resource-exhausted"}, - {"x-envoy-ratelimited-reset-headers", "Retry-After"}, {"x-envoy-retriable-header-names", "X-Upstream-Pushback"}, {"x-envoy-retriable-status-codes", "418,420"}, {"x-envoy-max-retries", "7"}, @@ -1390,7 +1328,6 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { EXPECT_FALSE(request_headers.has("x-envoy-retry-on")); EXPECT_FALSE(request_headers.has("x-envoy-retry-grpc-on")); EXPECT_FALSE(request_headers.has("x-envoy-max-retries")); - EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-headers")); EXPECT_TRUE(request_headers.has("x-envoy-retriable-header-names")); EXPECT_TRUE(request_headers.has("x-envoy-retriable-status-codes")); EXPECT_TRUE(request_headers.has("x-envoy-hedge-on-per-try-timeout")); @@ -1404,7 +1341,6 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { {{"envoy.reloadable_features.consume_all_retry_headers", "false"}}); Http::TestRequestHeaderMapImpl request_headers{ - {"x-envoy-ratelimited-reset-headers", "Retry-After"}, {"x-envoy-retriable-header-names", "X-Upstream-Pushback"}, {"x-envoy-retriable-status-codes", "418,420"}, {"x-envoy-max-retries", "7"}, @@ -1417,7 +1353,6 @@ TEST_F(RouterRetryStateImplTest, RemoveAllRetryHeaders) { EXPECT_FALSE(request_headers.has("x-envoy-retry-on")); EXPECT_FALSE(request_headers.has("x-envoy-retry-grpc-on")); EXPECT_FALSE(request_headers.has("x-envoy-max-retries")); - EXPECT_FALSE(request_headers.has("x-envoy-ratelimited-reset-headers")); EXPECT_TRUE(request_headers.has("x-envoy-retriable-header-names")); EXPECT_TRUE(request_headers.has("x-envoy-retriable-status-codes")); EXPECT_TRUE(request_headers.has("x-envoy-hedge-on-per-try-timeout")); diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 644397ab89b7..f3fda134f32f 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -119,11 +119,9 @@ class TestRetryPolicy : public RetryPolicy { absl::optional baseInterval() const override { return base_interval_; } absl::optional maxInterval() const override { return max_interval_; } - std::chrono::milliseconds rateLimitedResetMaxInterval() const override { - return ratelimited_reset_max_interval_; - } - const std::vector& rateLimitedResetHeaders() const override { - return ratelimited_reset_headers_; + std::chrono::milliseconds resetMaxInterval() const override { return reset_max_interval_; } + const std::vector& resetHeaders() const override { + return reset_headers_; } std::chrono::milliseconds per_try_timeout_{0}; @@ -135,8 +133,8 @@ class TestRetryPolicy : public RetryPolicy { std::vector retriable_request_headers_; absl::optional base_interval_{}; absl::optional max_interval_{}; - std::vector ratelimited_reset_headers_; - std::chrono::milliseconds ratelimited_reset_max_interval_{300000}; + std::vector reset_headers_{}; + std::chrono::milliseconds reset_max_interval_{300000}; }; class MockInternalRedirectPolicy : public InternalRedirectPolicy { @@ -165,7 +163,7 @@ class MockRetryState : public RetryState { void expectResetRetry(); MOCK_METHOD(bool, enabled, ()); - MOCK_METHOD(absl::optional, parseRateLimitedResetInterval, + MOCK_METHOD(absl::optional, parseResetInterval, (const Http::ResponseHeaderMap& response_headers), (const)); MOCK_METHOD(RetryStatus, shouldRetryHeaders, (const Http::ResponseHeaderMap& response_headers, DoRetryCallback callback)); @@ -179,8 +177,6 @@ class MockRetryState : public RetryState { (const Upstream::PrioritySet&, const Upstream::HealthyAndDegradedLoad&, const Upstream::RetryPriority::PriorityMappingFunc&)); MOCK_METHOD(uint32_t, hostSelectionMaxAttempts, (), (const)); - MOCK_METHOD(const std::vector&, rateLimitedResetHeaders, (), - (const)); DoRetryCallback callback_; }; From fbd7ae1ed1b3a7f9a076a3d1388b913857f8c1b7 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Sat, 1 Aug 2020 23:14:27 +1000 Subject: [PATCH 20/35] Update stats integration test Signed-off-by: Martin Matusiak --- test/integration/stats_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index f66a9b8a14bf..04de34985c60 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -308,7 +308,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // https://github.com/envoyproxy/envoy/issues/12209 // EXPECT_MEMORY_EQ(m_per_cluster, 44949); } - EXPECT_MEMORY_LE(m_per_cluster, 46000); // Round up to allow platform variations. + EXPECT_MEMORY_LE(m_per_cluster, 46100); // Round up to allow platform variations. } TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { From 2fc0bec66ad09fe88a7d21006343224045218e90 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Sun, 2 Aug 2020 00:35:30 +1000 Subject: [PATCH 21/35] Remove unnecessary virtual keyword Signed-off-by: Martin Matusiak --- source/common/http/header_utility.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index 92ec62b83c51..e0fdc9afc275 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -101,7 +101,7 @@ class HeaderUtility { ResetHeaderFormat format_; // RateLimitedResetHeaderParser - virtual absl::optional + absl::optional parseInterval(TimeSource& time_source, const HeaderMap& headers) const override; }; From d5d314d657027038cee3cbf510eeb7bdfbc9b049 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Sun, 2 Aug 2020 07:49:35 +1000 Subject: [PATCH 22/35] Fix stats integration test (mac) Signed-off-by: Martin Matusiak --- test/integration/stats_integration_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index 04de34985c60..03b0dab10ef0 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -308,7 +308,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // https://github.com/envoyproxy/envoy/issues/12209 // EXPECT_MEMORY_EQ(m_per_cluster, 44949); } - EXPECT_MEMORY_LE(m_per_cluster, 46100); // Round up to allow platform variations. + EXPECT_MEMORY_LE(m_per_cluster, 46500); // Round up to allow platform variations. } TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { @@ -386,7 +386,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // https://github.com/envoyproxy/envoy/issues/12209 // EXPECT_MEMORY_EQ(m_per_cluster, 37061); } - EXPECT_MEMORY_LE(m_per_cluster, 38000); // Round up to allow platform variations. + EXPECT_MEMORY_LE(m_per_cluster, 38500); // Round up to allow platform variations. } TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { From b2854743c1fdec8092bb0c5533895dd1b2e4b1f5 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Wed, 5 Aug 2020 16:00:20 +1000 Subject: [PATCH 23/35] Move ResetHeaderParser interface into router.h Signed-off-by: Martin Matusiak --- include/envoy/http/BUILD | 1 - include/envoy/http/header_map.h | 18 -- include/envoy/router/BUILD | 1 + include/envoy/router/router.h | 22 +- source/common/http/async_client_impl.h | 4 +- source/common/http/header_utility.cc | 55 ----- source/common/http/header_utility.h | 31 --- source/common/router/BUILD | 15 ++ source/common/router/config_impl.cc | 3 +- source/common/router/config_impl.h | 4 +- source/common/router/reset_header_parser.cc | 68 ++++++ source/common/router/reset_header_parser.h | 59 +++++ source/common/router/retry_state_impl.h | 2 +- test/common/http/BUILD | 1 - test/common/http/header_utility_test.cc | 191 ---------------- test/common/router/BUILD | 13 ++ .../common/router/reset_header_parser_test.cc | 210 ++++++++++++++++++ test/common/router/retry_state_impl_test.cc | 5 +- test/mocks/router/mocks.h | 4 +- 19 files changed, 397 insertions(+), 310 deletions(-) create mode 100644 source/common/router/reset_header_parser.cc create mode 100644 source/common/router/reset_header_parser.h create mode 100644 test/common/router/reset_header_parser_test.cc diff --git a/include/envoy/http/BUILD b/include/envoy/http/BUILD index c116af28147c..f17ce1cb5e14 100644 --- a/include/envoy/http/BUILD +++ b/include/envoy/http/BUILD @@ -102,7 +102,6 @@ envoy_cc_library( "abseil_inlined_vector", ], deps = [ - "//include/envoy/common:time_interface", "//source/common/common:assert_lib", "//source/common/common:hash_lib", ], diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 7a7bbdba00c7..bc5e9338a2dc 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -9,7 +9,6 @@ #include #include "envoy/common/pure.h" -#include "envoy/common/time.h" #include "common/common/assert.h" #include "common/common/hash.h" @@ -782,22 +781,5 @@ class HeaderMatcher { using HeaderMatcherSharedPtr = std::shared_ptr; -/** - * An interface to be implemented by rate limited reset header parsers. - */ -class ResetHeaderParser { -public: - virtual ~ResetHeaderParser() = default; - - /** - * Iterate over the headers, choose the first one that matches by name, and try to parse its - * value. - */ - virtual absl::optional - parseInterval(TimeSource& time_source, const HeaderMap& headers) const PURE; -}; - -using ResetHeaderParserSharedPtr = std::shared_ptr; - } // namespace Http } // namespace Envoy diff --git a/include/envoy/router/BUILD b/include/envoy/router/BUILD index 85b6058ed878..f3e5c450d04e 100644 --- a/include/envoy/router/BUILD +++ b/include/envoy/router/BUILD @@ -58,6 +58,7 @@ envoy_cc_library( "//include/envoy/access_log:access_log_interface", "//include/envoy/common:conn_pool_interface", "//include/envoy/common:matchers_interface", + "//include/envoy/common:time_interface", "//include/envoy/config:typed_metadata_interface", "//include/envoy/http:codec_interface", "//include/envoy/http:codes_interface", diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index a82d21bf65d7..271d838d7b4f 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -150,6 +150,23 @@ class CorsPolicy { virtual bool shadowEnabled() const PURE; }; +/** + * An interface to be implemented by rate limited reset header parsers. + */ +class ResetHeaderParser { +public: + virtual ~ResetHeaderParser() = default; + + /** + * Iterate over the headers, choose the first one that matches by name, and try to parse its + * value. + */ + virtual absl::optional + parseInterval(TimeSource& time_source, const Http::HeaderMap& headers) const PURE; +}; + +using ResetHeaderParserSharedPtr = std::shared_ptr; + /** * Route level retry policy. */ @@ -240,10 +257,10 @@ class RetryPolicy { * @return std::vector& list of reset header * parsers that will be used to extract a retry back-off interval from response headers. */ - virtual const std::vector& resetHeaders() const PURE; + virtual const std::vector& resetHeaders() const PURE; /** - * @return absl::optional upper limit placed on a retry + * @return std::chrono::milliseconds upper limit placed on a retry * back-off interval parsed from response headers. */ virtual std::chrono::milliseconds resetMaxInterval() const PURE; @@ -389,7 +406,6 @@ class RetryState { const Upstream::PrioritySet& priority_set, const Upstream::HealthyAndDegradedLoad& original_priority_load, const Upstream::RetryPriority::PriorityMappingFunc& priority_mapping_func) PURE; - /** * return how many times host selection should be reattempted during host selection. */ diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index f298fc043a9e..6342b0a3af1f 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -152,7 +152,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, return absl::nullopt; } absl::optional maxInterval() const override { return absl::nullopt; } - const std::vector& resetHeaders() const override { + const std::vector& resetHeaders() const override { return reset_headers_; } std::chrono::milliseconds resetMaxInterval() const override { @@ -162,7 +162,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, const std::vector retriable_status_codes_{}; const std::vector retriable_headers_{}; const std::vector retriable_request_headers_{}; - const std::vector reset_headers_{}; + const std::vector reset_headers_{}; }; struct NullConfig : public Router::Config { diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 0ba9186ccf30..c293f29e16cd 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -139,61 +139,6 @@ bool HeaderUtility::matchHeaders(const HeaderMap& request_headers, const HeaderD return match != header_data.invert_match_; } -HeaderUtility::ResetHeaderData::ResetHeaderData( - const envoy::config::route::v3::RetryPolicy::ResetHeader& config) - : name_(config.name()) { - switch (config.format()) { - case envoy::config::route::v3::RetryPolicy::SECONDS: - format_ = HeaderUtility::ResetHeaderFormat::Seconds; - break; - case envoy::config::route::v3::RetryPolicy::UNIX_TIMESTAMP: - format_ = HeaderUtility::ResetHeaderFormat::UnixTimestamp; - break; - default: - NOT_REACHED_GCOVR_EXCL_LINE; - } -} - -absl::optional -HeaderUtility::ResetHeaderData::parseInterval(TimeSource& time_source, - const HeaderMap& headers) const { - const HeaderEntry* header = headers.get(name_); - - if (header == nullptr) { - return absl::nullopt; - } - - const auto& header_value = header->value().getStringView(); - uint64_t num_seconds{}; - - switch (format_) { - case HeaderUtility::ResetHeaderFormat::Seconds: - if (absl::SimpleAtoi(header_value, &num_seconds)) { - return absl::optional(num_seconds * 1000UL); - } - break; - - case HeaderUtility::ResetHeaderFormat::UnixTimestamp: - if (absl::SimpleAtoi(header_value, &num_seconds)) { - const auto time_now = time_source.systemTime().time_since_epoch(); - const uint64_t timestamp = std::chrono::duration_cast(time_now).count(); - - if (num_seconds < timestamp) { - return absl::nullopt; - } - - const uint64_t interval = num_seconds - timestamp; - return absl::optional(interval * 1000UL); - } - break; - - default: - NOT_REACHED_GCOVR_EXCL_LINE; - } - - return absl::nullopt; -} - bool HeaderUtility::headerValueIsValid(const absl::string_view header_value) { return nghttp2_check_header_value(reinterpret_cast(header_value.data()), header_value.size()) != 0; diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index e0fdc9afc275..22992f1927f9 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -20,7 +20,6 @@ namespace Http { class HeaderUtility { public: enum class HeaderMatchType { Value, Regex, Range, Present, Prefix, Suffix }; - enum class ResetHeaderFormat { Seconds, UnixTimestamp }; /** * Get all instances of the header key specified, and return the values in the vector provided. @@ -91,35 +90,6 @@ class HeaderUtility { static bool matchHeaders(const HeaderMap& request_headers, const HeaderData& config_header); - // A ResetHeaderData specifies a header name and a format to match against - // response headers that are used to signal a rate limit interval reset, such - // as Retry-After or X-RateLimit-Reset. - struct ResetHeaderData : public ResetHeaderParser { - ResetHeaderData(const envoy::config::route::v3::RetryPolicy::ResetHeader& config); - - const LowerCaseString name_; - ResetHeaderFormat format_; - - // RateLimitedResetHeaderParser - absl::optional - parseInterval(TimeSource& time_source, const HeaderMap& headers) const override; - }; - - using ResetHeaderDataPtr = std::unique_ptr; - - /** - * Build a vector of ResetHeaderParserSharedPtr given input config. - */ - static std::vector buildResetHeaderParserVector( - const Protobuf::RepeatedPtrField& - reset_headers) { - std::vector ret; - for (const auto& reset_header : reset_headers) { - ret.emplace_back(std::make_shared(reset_header)); - } - return ret; - } - /** * Validates that a header value is valid, according to RFC 7230, section 3.2. * http://tools.ietf.org/html/rfc7230#section-3.2 @@ -188,6 +158,5 @@ class HeaderUtility { */ static void stripPortFromHost(RequestHeaderMap& headers, uint32_t listener_port); }; - } // namespace Http } // namespace Envoy diff --git a/source/common/router/BUILD b/source/common/router/BUILD index 5f9e2b8cbc96..a2825b775dda 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -37,6 +37,7 @@ envoy_cc_library( ":header_formatter_lib", ":header_parser_lib", ":metadatamatchcriteria_lib", + ":reset_header_parser_lib", ":retry_state_lib", ":router_ratelimit_lib", ":tls_context_match_criteria_lib", @@ -378,6 +379,20 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "reset_header_parser_lib", + srcs = ["reset_header_parser.cc"], + hdrs = ["reset_header_parser.h"], + deps = [ + "//include/envoy/common:time_interface", + "//include/envoy/http:header_map_interface", + "//include/envoy/router:router_interface", + "//source/common/http:headers_lib", + "//source/common/protobuf:utility_lib", + "@envoy_api//envoy/config/route/v3:pkg_cc_proto", + ], +) + envoy_cc_library( name = "string_accessor_lib", hdrs = ["string_accessor_impl.h"], diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index b25efda14b67..858ca36254fa 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -33,6 +33,7 @@ #include "common/http/utility.h" #include "common/protobuf/protobuf.h" #include "common/protobuf/utility.h" +#include "common/router/reset_header_parser.h" #include "common/router/retry_state_impl.h" #include "common/runtime/runtime_features.h" #include "common/tracing/http_tracer_impl.h" @@ -122,7 +123,7 @@ RetryPolicyImpl::RetryPolicyImpl(const envoy::config::route::v3::RetryPolicy& re } if (retry_policy.has_rate_limited_retry_back_off()) { - reset_headers_ = Http::HeaderUtility::buildResetHeaderParserVector( + reset_headers_ = ResetHeaderParserImpl::buildResetHeaderParserVector( retry_policy.rate_limited_retry_back_off().reset_headers()); absl::optional reset_max_interval = diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 87eb4d521384..9b04112a8843 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -272,7 +272,7 @@ class RetryPolicyImpl : public RetryPolicy { } absl::optional baseInterval() const override { return base_interval_; } absl::optional maxInterval() const override { return max_interval_; } - const std::vector& resetHeaders() const override { + const std::vector& resetHeaders() const override { return reset_headers_; } std::chrono::milliseconds resetMaxInterval() const override { return reset_max_interval_; } @@ -298,7 +298,7 @@ class RetryPolicyImpl : public RetryPolicy { std::vector retriable_request_headers_; absl::optional base_interval_; absl::optional max_interval_; - std::vector reset_headers_{}; + std::vector reset_headers_{}; std::chrono::milliseconds reset_max_interval_{300000}; ProtobufMessage::ValidationVisitor* validation_visitor_{}; }; diff --git a/source/common/router/reset_header_parser.cc b/source/common/router/reset_header_parser.cc new file mode 100644 index 000000000000..f5d61e9ca714 --- /dev/null +++ b/source/common/router/reset_header_parser.cc @@ -0,0 +1,68 @@ +#include "common/router/reset_header_parser.h" + +#include + +#include "common/common/assert.h" + +#include "absl/strings/numbers.h" + +namespace Envoy { +namespace Router { + +ResetHeaderParserImpl::ResetHeaderParserImpl( + const envoy::config::route::v3::RetryPolicy::ResetHeader& config) + : name_(config.name()) { + switch (config.format()) { + case envoy::config::route::v3::RetryPolicy::SECONDS: + format_ = ResetHeaderFormat::Seconds; + break; + case envoy::config::route::v3::RetryPolicy::UNIX_TIMESTAMP: + format_ = ResetHeaderFormat::UnixTimestamp; + break; + default: + NOT_REACHED_GCOVR_EXCL_LINE; + } +} + +absl::optional +ResetHeaderParserImpl::parseInterval(TimeSource& time_source, + const Http::HeaderMap& headers) const { + const Http::HeaderEntry* header = headers.get(name_); + + if (header == nullptr) { + return absl::nullopt; + } + + const auto& header_value = header->value().getStringView(); + uint64_t num_seconds{}; + + switch (format_) { + case ResetHeaderFormat::Seconds: + if (absl::SimpleAtoi(header_value, &num_seconds)) { + return absl::optional(num_seconds * 1000UL); + } + break; + + case ResetHeaderFormat::UnixTimestamp: + if (absl::SimpleAtoi(header_value, &num_seconds)) { + const auto time_now = time_source.systemTime().time_since_epoch(); + const uint64_t timestamp = std::chrono::duration_cast(time_now).count(); + + if (num_seconds < timestamp) { + return absl::nullopt; + } + + const uint64_t interval = num_seconds - timestamp; + return absl::optional(interval * 1000UL); + } + break; + + default: + NOT_REACHED_GCOVR_EXCL_LINE; + } + + return absl::nullopt; +} + +} // namespace Router +} // namespace Envoy diff --git a/source/common/router/reset_header_parser.h b/source/common/router/reset_header_parser.h new file mode 100644 index 000000000000..789886393cdd --- /dev/null +++ b/source/common/router/reset_header_parser.h @@ -0,0 +1,59 @@ +#pragma once + +#include +#include +#include + +#include "envoy/common/time.h" +#include "envoy/config/route/v3/route_components.pb.h" +#include "envoy/http/header_map.h" +#include "envoy/router/router.h" + +#include "common/protobuf/protobuf.h" + +#include "absl/types/optional.h" + +namespace Envoy { +namespace Router { + +enum class ResetHeaderFormat { Seconds, UnixTimestamp }; + +/* + * A ResetHeaderParser specifies a header name and a format to match against + * response headers that are used to signal a rate limit interval reset, such + * as Retry-After or X-RateLimit-Reset. + */ +class ResetHeaderParserImpl : public ResetHeaderParser { +public: + /** + * Build a vector of ResetHeaderParserSharedPtr given input config. + */ + static std::vector buildResetHeaderParserVector( + const Protobuf::RepeatedPtrField& + reset_headers) { + std::vector ret; + for (const auto& reset_header : reset_headers) { + ret.emplace_back(std::make_shared(reset_header)); + } + return ret; + } + + ResetHeaderParserImpl(const envoy::config::route::v3::RetryPolicy::ResetHeader& config); + + const Http::LowerCaseString& name() const { return name_; } + ResetHeaderFormat format() const { return format_; } + + /** + * Iterate over the headers, choose the first one that matches by name, and try to parse its + * value. + */ + absl::optional + parseInterval(TimeSource& time_source, const Http::HeaderMap& headers) const override; + +private: + const Http::LowerCaseString name_; + ResetHeaderFormat format_; +}; + +} // namespace Router +} // namespace Envoy diff --git a/source/common/router/retry_state_impl.h b/source/common/router/retry_state_impl.h index a89566338260..8931b5bf3751 100644 --- a/source/common/router/retry_state_impl.h +++ b/source/common/router/retry_state_impl.h @@ -121,7 +121,7 @@ class RetryStateImpl : public RetryState { uint32_t host_selection_max_attempts_; std::vector retriable_status_codes_; std::vector retriable_headers_; - std::vector reset_headers_{}; + std::vector reset_headers_{}; std::chrono::milliseconds reset_max_interval_{}; }; diff --git a/test/common/http/BUILD b/test/common/http/BUILD index 9c526dce39df..b317052c4414 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -323,7 +323,6 @@ envoy_cc_test( srcs = ["header_utility_test.cc"], deps = [ "//source/common/http:header_utility_lib", - "//test/test_common:simulated_time_system_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/route/v3:pkg_cc_proto", ], diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index 15c5b59add7b..dc8c831c650d 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -8,7 +8,6 @@ #include "common/http/header_utility.h" #include "common/json/json_loader.h" -#include "test/test_common/simulated_time_system.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" @@ -500,196 +499,6 @@ invert_match: true EXPECT_FALSE(HeaderUtility::matchHeaders(unmatching_headers, header_data)); } -envoy::config::route::v3::RetryPolicy::ResetHeader -parseResetHeaderParserFromYaml(const std::string& yaml) { - envoy::config::route::v3::RetryPolicy::ResetHeader reset_header; - TestUtility::loadFromYaml(yaml, reset_header); - return reset_header; -} - -TEST(ResetHeaderDataConstructorTest, FormatUnset) { - const std::string yaml = R"EOF( -name: retry-after - )EOF"; - - HeaderUtility::ResetHeaderData reset_header_data = - HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); - - EXPECT_EQ("retry-after", reset_header_data.name_.get()); - EXPECT_EQ(HeaderUtility::ResetHeaderFormat::Seconds, reset_header_data.format_); -} - -TEST(ResetHeaderDataConstructorTest, FormatSeconds) { - const std::string yaml = R"EOF( -name: retry-after -format: SECONDS - )EOF"; - - HeaderUtility::ResetHeaderData reset_header_data = - HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); - - EXPECT_EQ("retry-after", reset_header_data.name_.get()); - EXPECT_EQ(HeaderUtility::ResetHeaderFormat::Seconds, reset_header_data.format_); -} - -TEST(ResetHeaderDataConstructorTest, FormatUnixTimestamp) { - const std::string yaml = R"EOF( -name: retry-after -format: UNIX_TIMESTAMP - )EOF"; - - HeaderUtility::ResetHeaderData reset_header_data = - HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); - - EXPECT_EQ("retry-after", reset_header_data.name_.get()); - EXPECT_EQ(HeaderUtility::ResetHeaderFormat::UnixTimestamp, reset_header_data.format_); -} - -class ResetHeaderDataParseIntervalTest : public testing::Test { -public: - ResetHeaderDataParseIntervalTest() { - const time_t known_date_time = 1000000000; - test_time_.setSystemTime(std::chrono::system_clock::from_time_t(known_date_time)); - } - - Event::SimulatedTimeSystem test_time_; -}; - -TEST_F(ResetHeaderDataParseIntervalTest, NoHeaderMatches) { - const std::string yaml = R"EOF( -name: retry-after -format: SECONDS - )EOF"; - - HeaderUtility::ResetHeaderData reset_header_data = - HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); - - TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - - EXPECT_EQ(absl::nullopt, - reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); -} - -TEST_F(ResetHeaderDataParseIntervalTest, HeaderMatchesButUnsupportedFormatDate) { - const std::string yaml = R"EOF( -name: retry-after -format: SECONDS - )EOF"; - - HeaderUtility::ResetHeaderData reset_header_data = - HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); - - TestResponseHeaderMapImpl response_headers{{"retry-after", "Fri, 17 Jul 2020 11:59:51 GMT"}}; - - EXPECT_EQ(absl::nullopt, - reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); -} - -TEST_F(ResetHeaderDataParseIntervalTest, HeaderMatchesButUnsupportedFormatFloat) { - const std::string yaml = R"EOF( -name: retry-after -format: SECONDS - )EOF"; - - HeaderUtility::ResetHeaderData reset_header_data = - HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); - - TestResponseHeaderMapImpl response_headers{{"retry-after", "2.5"}}; - - EXPECT_EQ(absl::nullopt, - reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); -} - -TEST_F(ResetHeaderDataParseIntervalTest, HeaderMatchesSupportedFormatSeconds) { - const std::string yaml = R"EOF( -name: retry-after -format: SECONDS - )EOF"; - - HeaderUtility::ResetHeaderData reset_header_data = - HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); - - TestResponseHeaderMapImpl response_headers{{"retry-after", "5"}}; - - EXPECT_EQ(absl::optional(5000), - reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); -} - -TEST_F(ResetHeaderDataParseIntervalTest, HeaderMatchesSupportedFormatSecondsCaseInsensitive) { - const std::string yaml = R"EOF( -name: retry-after -format: SECONDS - )EOF"; - - HeaderUtility::ResetHeaderData reset_header_data = - HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); - - TestResponseHeaderMapImpl response_headers{{"Retry-After", "5"}}; - - EXPECT_EQ(absl::optional(5000), - reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); -} - -TEST_F(ResetHeaderDataParseIntervalTest, HeaderMatchesButUnsupportedFormatTimestampFloat) { - const std::string yaml = R"EOF( -name: retry-after -format: UNIX_TIMESTAMP - )EOF"; - - HeaderUtility::ResetHeaderData reset_header_data = - HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); - - TestResponseHeaderMapImpl response_headers{{"retry-after", "1595320702.1234"}}; - - EXPECT_EQ(absl::nullopt, - reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); -} - -TEST_F(ResetHeaderDataParseIntervalTest, HeaderMatchesSupportedFormatTimestampButInThePast) { - const std::string yaml = R"EOF( -name: retry-after -format: UNIX_TIMESTAMP - )EOF"; - - HeaderUtility::ResetHeaderData reset_header_data = - HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); - - TestResponseHeaderMapImpl response_headers{{"retry-after", "999999999"}}; - - EXPECT_EQ(absl::nullopt, - reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); -} - -TEST_F(ResetHeaderDataParseIntervalTest, HeaderMatchesSupportedFormatTimestampEmptyInterval) { - const std::string yaml = R"EOF( -name: retry-after -format: UNIX_TIMESTAMP - )EOF"; - - HeaderUtility::ResetHeaderData reset_header_data = - HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); - - TestResponseHeaderMapImpl response_headers{{"retry-after", "1000000000"}}; - - EXPECT_EQ(absl::optional(0), - reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); -} - -TEST_F(ResetHeaderDataParseIntervalTest, HeaderMatchesSupportedFormatTimestampNonEmptyInterval) { - const std::string yaml = R"EOF( -name: retry-after -format: UNIX_TIMESTAMP - )EOF"; - - HeaderUtility::ResetHeaderData reset_header_data = - HeaderUtility::ResetHeaderData(parseResetHeaderParserFromYaml(yaml)); - - TestResponseHeaderMapImpl response_headers{{"retry-after", "1000000007"}}; - - EXPECT_EQ(absl::optional(7000), - reset_header_data.parseInterval(test_time_.timeSystem(), response_headers)); -} - TEST(HeaderIsValidTest, InvalidHeaderValuesAreRejected) { // ASCII values 1-31 are control characters (with the exception of ASCII // values 9, 10, and 13 which are a horizontal tab, line feed, and carriage diff --git a/test/common/router/BUILD b/test/common/router/BUILD index a377e5672fdd..0fad319d411c 100644 --- a/test/common/router/BUILD +++ b/test/common/router/BUILD @@ -61,6 +61,18 @@ envoy_cc_fuzz_test( ], ) +envoy_cc_test( + name = "reset_header_parser_test", + srcs = ["reset_header_parser_test.cc"], + deps = [ + "//source/common/http:header_utility_lib", + "//source/common/router:reset_header_parser_lib", + "//test/test_common:simulated_time_system_lib", + "//test/test_common:utility_lib", + "@envoy_api//envoy/config/route/v3:pkg_cc_proto", + ], +) + envoy_cc_test( name = "rds_impl_test", srcs = ["rds_impl_test.cc"], @@ -155,6 +167,7 @@ envoy_cc_test( srcs = ["retry_state_impl_test.cc"], deps = [ "//source/common/http:header_map_lib", + "//source/common/router:reset_header_parser_lib", "//source/common/router:retry_state_lib", "//source/common/upstream:resource_manager_lib", "//test/mocks/router:router_mocks", diff --git a/test/common/router/reset_header_parser_test.cc b/test/common/router/reset_header_parser_test.cc new file mode 100644 index 000000000000..bb6135390db4 --- /dev/null +++ b/test/common/router/reset_header_parser_test.cc @@ -0,0 +1,210 @@ +#include "envoy/config/route/v3/route_components.pb.h" +#include "envoy/http/protocol.h" +#include "envoy/json/json_object.h" + +#include "common/json/json_loader.h" +#include "common/router/reset_header_parser.h" + +#include "test/test_common/simulated_time_system.h" +#include "test/test_common/utility.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Router { +namespace { + +envoy::config::route::v3::RetryPolicy::ResetHeader +parseResetHeaderParserFromYaml(const std::string& yaml) { + envoy::config::route::v3::RetryPolicy::ResetHeader reset_header; + TestUtility::loadFromYaml(yaml, reset_header); + return reset_header; +} + +TEST(ResetHeaderParserConstructorTest, FormatUnset) { + const std::string yaml = R"EOF( +name: retry-after + )EOF"; + + ResetHeaderParserImpl reset_header_parser = + ResetHeaderParserImpl(parseResetHeaderParserFromYaml(yaml)); + + EXPECT_EQ("retry-after", reset_header_parser.name().get()); + EXPECT_EQ(ResetHeaderFormat::Seconds, reset_header_parser.format()); +} + +TEST(ResetHeaderParserConstructorTest, FormatSeconds) { + const std::string yaml = R"EOF( +name: retry-after +format: SECONDS + )EOF"; + + ResetHeaderParserImpl reset_header_parser = + ResetHeaderParserImpl(parseResetHeaderParserFromYaml(yaml)); + + EXPECT_EQ("retry-after", reset_header_parser.name().get()); + EXPECT_EQ(ResetHeaderFormat::Seconds, reset_header_parser.format()); +} + +TEST(ResetHeaderParserConstructorTest, FormatUnixTimestamp) { + const std::string yaml = R"EOF( +name: retry-after +format: UNIX_TIMESTAMP + )EOF"; + + ResetHeaderParserImpl reset_header_parser = + ResetHeaderParserImpl(parseResetHeaderParserFromYaml(yaml)); + + EXPECT_EQ("retry-after", reset_header_parser.name().get()); + EXPECT_EQ(ResetHeaderFormat::UnixTimestamp, reset_header_parser.format()); +} + +class ResetHeaderParserParseIntervalTest : public testing::Test { +public: + ResetHeaderParserParseIntervalTest() { + const time_t known_date_time = 1000000000; + test_time_.setSystemTime(std::chrono::system_clock::from_time_t(known_date_time)); + } + + Event::SimulatedTimeSystem test_time_; +}; + +TEST_F(ResetHeaderParserParseIntervalTest, NoHeaderMatches) { + const std::string yaml = R"EOF( +name: retry-after +format: SECONDS + )EOF"; + + ResetHeaderParserImpl reset_header_parser = + ResetHeaderParserImpl(parseResetHeaderParserFromYaml(yaml)); + + Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}}; + + EXPECT_EQ(absl::nullopt, + reset_header_parser.parseInterval(test_time_.timeSystem(), response_headers)); +} + +TEST_F(ResetHeaderParserParseIntervalTest, HeaderMatchesButUnsupportedFormatDate) { + const std::string yaml = R"EOF( +name: retry-after +format: SECONDS + )EOF"; + + ResetHeaderParserImpl reset_header_parser = + ResetHeaderParserImpl(parseResetHeaderParserFromYaml(yaml)); + + Http::TestResponseHeaderMapImpl response_headers{ + {"retry-after", "Fri, 17 Jul 2020 11:59:51 GMT"}}; + + EXPECT_EQ(absl::nullopt, + reset_header_parser.parseInterval(test_time_.timeSystem(), response_headers)); +} + +TEST_F(ResetHeaderParserParseIntervalTest, HeaderMatchesButUnsupportedFormatFloat) { + const std::string yaml = R"EOF( +name: retry-after +format: SECONDS + )EOF"; + + ResetHeaderParserImpl reset_header_parser = + ResetHeaderParserImpl(parseResetHeaderParserFromYaml(yaml)); + + Http::TestResponseHeaderMapImpl response_headers{{"retry-after", "2.5"}}; + + EXPECT_EQ(absl::nullopt, + reset_header_parser.parseInterval(test_time_.timeSystem(), response_headers)); +} + +TEST_F(ResetHeaderParserParseIntervalTest, HeaderMatchesSupportedFormatSeconds) { + const std::string yaml = R"EOF( +name: retry-after +format: SECONDS + )EOF"; + + ResetHeaderParserImpl reset_header_parser = + ResetHeaderParserImpl(parseResetHeaderParserFromYaml(yaml)); + + Http::TestResponseHeaderMapImpl response_headers{{"retry-after", "5"}}; + + EXPECT_EQ(absl::optional(5000), + reset_header_parser.parseInterval(test_time_.timeSystem(), response_headers)); +} + +TEST_F(ResetHeaderParserParseIntervalTest, HeaderMatchesSupportedFormatSecondsCaseInsensitive) { + const std::string yaml = R"EOF( +name: retry-after +format: SECONDS + )EOF"; + + ResetHeaderParserImpl reset_header_parser = + ResetHeaderParserImpl(parseResetHeaderParserFromYaml(yaml)); + + Http::TestResponseHeaderMapImpl response_headers{{"Retry-After", "5"}}; + + EXPECT_EQ(absl::optional(5000), + reset_header_parser.parseInterval(test_time_.timeSystem(), response_headers)); +} + +TEST_F(ResetHeaderParserParseIntervalTest, HeaderMatchesButUnsupportedFormatTimestampFloat) { + const std::string yaml = R"EOF( +name: retry-after +format: UNIX_TIMESTAMP + )EOF"; + + ResetHeaderParserImpl reset_header_parser = + ResetHeaderParserImpl(parseResetHeaderParserFromYaml(yaml)); + + Http::TestResponseHeaderMapImpl response_headers{{"retry-after", "1595320702.1234"}}; + + EXPECT_EQ(absl::nullopt, + reset_header_parser.parseInterval(test_time_.timeSystem(), response_headers)); +} + +TEST_F(ResetHeaderParserParseIntervalTest, HeaderMatchesSupportedFormatTimestampButInThePast) { + const std::string yaml = R"EOF( +name: retry-after +format: UNIX_TIMESTAMP + )EOF"; + + ResetHeaderParserImpl reset_header_parser = + ResetHeaderParserImpl(parseResetHeaderParserFromYaml(yaml)); + + Http::TestResponseHeaderMapImpl response_headers{{"retry-after", "999999999"}}; + + EXPECT_EQ(absl::nullopt, + reset_header_parser.parseInterval(test_time_.timeSystem(), response_headers)); +} + +TEST_F(ResetHeaderParserParseIntervalTest, HeaderMatchesSupportedFormatTimestampEmptyInterval) { + const std::string yaml = R"EOF( +name: retry-after +format: UNIX_TIMESTAMP + )EOF"; + + ResetHeaderParserImpl reset_header_parser = + ResetHeaderParserImpl(parseResetHeaderParserFromYaml(yaml)); + + Http::TestResponseHeaderMapImpl response_headers{{"retry-after", "1000000000"}}; + + EXPECT_EQ(absl::optional(0), + reset_header_parser.parseInterval(test_time_.timeSystem(), response_headers)); +} + +TEST_F(ResetHeaderParserParseIntervalTest, HeaderMatchesSupportedFormatTimestampNonEmptyInterval) { + const std::string yaml = R"EOF( +name: retry-after +format: UNIX_TIMESTAMP + )EOF"; + + ResetHeaderParserImpl reset_header_parser = + ResetHeaderParserImpl(parseResetHeaderParserFromYaml(yaml)); + + Http::TestResponseHeaderMapImpl response_headers{{"retry-after", "1000000007"}}; + + EXPECT_EQ(absl::optional(7000), + reset_header_parser.parseInterval(test_time_.timeSystem(), response_headers)); +} + +} // namespace +} // namespace Router +} // namespace Envoy diff --git a/test/common/router/retry_state_impl_test.cc b/test/common/router/retry_state_impl_test.cc index 4a751225acdf..f162b0e8fd5f 100644 --- a/test/common/router/retry_state_impl_test.cc +++ b/test/common/router/retry_state_impl_test.cc @@ -4,6 +4,7 @@ #include "envoy/stats/stats.h" #include "common/http/header_map_impl.h" +#include "common/router/reset_header_parser.h" #include "common/router/retry_state_impl.h" #include "common/upstream/resource_manager_impl.h" @@ -941,7 +942,7 @@ TEST_F(RouterRetryStateImplTest, ParseRateLimitedResetInterval) { reset_header_2->set_name("X-RateLimit-Reset"); reset_header_2->set_format(envoy::config::route::v3::RetryPolicy::UNIX_TIMESTAMP); - policy_.reset_headers_ = Http::HeaderUtility::buildResetHeaderParserVector(reset_headers); + policy_.reset_headers_ = ResetHeaderParserImpl::buildResetHeaderParserVector(reset_headers); // Failure case: Matches reset header (seconds) but exceeds max_interval (>5min) { @@ -1008,7 +1009,7 @@ TEST_F(RouterRetryStateImplTest, RateLimitedRetryBackoffStrategy) { reset_header->set_format(envoy::config::route::v3::RetryPolicy::SECONDS); policy_.num_retries_ = 3; - policy_.reset_headers_ = Http::HeaderUtility::buildResetHeaderParserVector(reset_headers); + policy_.reset_headers_ = ResetHeaderParserImpl::buildResetHeaderParserVector(reset_headers); Http::TestRequestHeaderMapImpl request_headers{{"x-envoy-retry-on", "5xx"}}; setup(request_headers); diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index f3fda134f32f..f0c35d2e72fc 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -120,7 +120,7 @@ class TestRetryPolicy : public RetryPolicy { absl::optional baseInterval() const override { return base_interval_; } absl::optional maxInterval() const override { return max_interval_; } std::chrono::milliseconds resetMaxInterval() const override { return reset_max_interval_; } - const std::vector& resetHeaders() const override { + const std::vector& resetHeaders() const override { return reset_headers_; } @@ -133,7 +133,7 @@ class TestRetryPolicy : public RetryPolicy { std::vector retriable_request_headers_; absl::optional base_interval_{}; absl::optional max_interval_{}; - std::vector reset_headers_{}; + std::vector reset_headers_{}; std::chrono::milliseconds reset_max_interval_{300000}; }; From 775aa7e7ba64744a1428b7347dcc35200dbdac2e Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Wed, 5 Aug 2020 17:54:05 +1000 Subject: [PATCH 24/35] Remove reference to redis which was removed on master Signed-off-by: Martin Matusiak --- docs/root/version_history/current.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 28d13204dba9..b1b619e45bbe 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -61,7 +61,6 @@ New Features * postgres network filter: :ref:`metadata ` is produced based on SQL query. * ratelimit: added :ref:`enable_x_ratelimit_headers ` option to enable `X-RateLimit-*` headers as defined in `draft RFC `_. * ratelimit: added :ref:`enable_x_ratelimit_headers ` option to enable `X-RateLimit-*` headers as defined in `draft RFC `_. -* redis: added fault injection support :ref:`fault injection for redis proxy `, described further in :ref:`configuration documentation `. * router: added a new :ref:`rate limited retry back off ` strategy that uses headers like `Retry-After` or `X-RateLimit-Reset` to From 37d667f1fe592052efa8ab4bd6246e8b18a4d365 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Tue, 11 Aug 2020 08:30:58 +1000 Subject: [PATCH 25/35] Fix bad merge Signed-off-by: Martin Matusiak --- docs/root/version_history/current.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index b1b619e45bbe..6986e56067df 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -60,7 +60,6 @@ New Features * lua: added Lua APIs to access :ref:`SSL connection info ` object. * postgres network filter: :ref:`metadata ` is produced based on SQL query. * ratelimit: added :ref:`enable_x_ratelimit_headers ` option to enable `X-RateLimit-*` headers as defined in `draft RFC `_. -* ratelimit: added :ref:`enable_x_ratelimit_headers ` option to enable `X-RateLimit-*` headers as defined in `draft RFC `_. * router: added a new :ref:`rate limited retry back off ` strategy that uses headers like `Retry-After` or `X-RateLimit-Reset` to From 6b82264bd541f693912c1890c4ca18074d8220bb Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Tue, 11 Aug 2020 08:31:28 +1000 Subject: [PATCH 26/35] PR feedback Signed-off-by: Martin Matusiak --- source/common/router/reset_header_parser.h | 2 +- test/common/router/retry_state_impl_test.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/router/reset_header_parser.h b/source/common/router/reset_header_parser.h index 789886393cdd..776486d339b1 100644 --- a/source/common/router/reset_header_parser.h +++ b/source/common/router/reset_header_parser.h @@ -18,7 +18,7 @@ namespace Router { enum class ResetHeaderFormat { Seconds, UnixTimestamp }; -/* +/** * A ResetHeaderParser specifies a header name and a format to match against * response headers that are used to signal a rate limit interval reset, such * as Retry-After or X-RateLimit-Reset. diff --git a/test/common/router/retry_state_impl_test.cc b/test/common/router/retry_state_impl_test.cc index f162b0e8fd5f..f7bc5cb87c36 100644 --- a/test/common/router/retry_state_impl_test.cc +++ b/test/common/router/retry_state_impl_test.cc @@ -1029,7 +1029,7 @@ TEST_F(RouterRetryStateImplTest, RateLimitedRetryBackoffStrategy) { EXPECT_CALL(callback_ready_, ready()); retry_timer_->invokeCallback(); - // reset header present -> exponential backoff used + // reset header not present -> exponential backoff used EXPECT_CALL(random_, random()).WillOnce(Return(190)); EXPECT_CALL(*retry_timer_, enableTimer(std::chrono::milliseconds(15), _)); EXPECT_EQ(RetryStatus::Yes, state_->shouldRetryHeaders(response_headers_plain, callback_)); From 59b06d4444a87e3b12f833129010eb237b43360b Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Tue, 11 Aug 2020 10:18:49 +1000 Subject: [PATCH 27/35] Update stats integration test again after rebase Signed-off-by: Martin Matusiak --- test/integration/stats_integration_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index 03b0dab10ef0..bad61b8d6b98 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -308,7 +308,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // https://github.com/envoyproxy/envoy/issues/12209 // EXPECT_MEMORY_EQ(m_per_cluster, 44949); } - EXPECT_MEMORY_LE(m_per_cluster, 46500); // Round up to allow platform variations. + EXPECT_MEMORY_LE(m_per_cluster, 47000); // Round up to allow platform variations. } TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { @@ -386,7 +386,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // https://github.com/envoyproxy/envoy/issues/12209 // EXPECT_MEMORY_EQ(m_per_cluster, 37061); } - EXPECT_MEMORY_LE(m_per_cluster, 38500); // Round up to allow platform variations. + EXPECT_MEMORY_LE(m_per_cluster, 39000); // Round up to allow platform variations. } TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { From 1bef83e4d5a63cbbbce2fd9e95b7cc5e0b6cea9e Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Tue, 11 Aug 2020 14:42:16 +1000 Subject: [PATCH 28/35] Roll back bad partial merge to stats integration test Signed-off-by: Martin Matusiak --- test/integration/stats_integration_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index bad61b8d6b98..f66a9b8a14bf 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -308,7 +308,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // https://github.com/envoyproxy/envoy/issues/12209 // EXPECT_MEMORY_EQ(m_per_cluster, 44949); } - EXPECT_MEMORY_LE(m_per_cluster, 47000); // Round up to allow platform variations. + EXPECT_MEMORY_LE(m_per_cluster, 46000); // Round up to allow platform variations. } TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { @@ -386,7 +386,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // https://github.com/envoyproxy/envoy/issues/12209 // EXPECT_MEMORY_EQ(m_per_cluster, 37061); } - EXPECT_MEMORY_LE(m_per_cluster, 39000); // Round up to allow platform variations. + EXPECT_MEMORY_LE(m_per_cluster, 38000); // Round up to allow platform variations. } TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { From 4abc240f51741c030b4c5909189e8c6c7386f49e Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Tue, 11 Aug 2020 15:27:30 +1000 Subject: [PATCH 29/35] Fix merge with master Signed-off-by: Martin Matusiak --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 29858ae38d74..dbc26c264bf5 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -60,11 +60,11 @@ New Features * lua: added Lua APIs to access :ref:`SSL connection info ` object. * postgres network filter: :ref:`metadata ` is produced based on SQL query. * ratelimit: added :ref:`enable_x_ratelimit_headers ` option to enable `X-RateLimit-*` headers as defined in `draft RFC `_. +* rbac filter: added a log action to the :ref:`RBAC filter ` which sets dynamic metadata to inform access loggers whether to log. * router: added a new :ref:`rate limited retry back off ` strategy that uses headers like `Retry-After` or `X-RateLimit-Reset` to decide the back off interval. -* rbac filter: added a log action to the :ref:`RBAC filter ` which sets dynamic metadata to inform access loggers whether to log. * router: added new :ref:`envoy-ratelimited` retry policy, which allows retrying envoy's own rate limited responses. From b9c33f1c40e7f1025312979d8ac25d421795b326 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Tue, 11 Aug 2020 16:33:41 +1000 Subject: [PATCH 30/35] Update stats integration test again Signed-off-by: Martin Matusiak --- test/integration/stats_integration_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index f66a9b8a14bf..a6f20bb0b520 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -288,6 +288,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // 2020/07/21 12034 44811 46000 Add configurable histogram buckets. // 2020/07/31 12035 45002 46000 Init manager store unready targets in hash map. // 2020/08/10 12275 44949 46000 Re-organize tls histogram maps to improve continuity. + // 2020/08/11 12202 44949 46100 router: add new retry back-off strategy // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -308,7 +309,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // https://github.com/envoyproxy/envoy/issues/12209 // EXPECT_MEMORY_EQ(m_per_cluster, 44949); } - EXPECT_MEMORY_LE(m_per_cluster, 46000); // Round up to allow platform variations. + EXPECT_MEMORY_LE(m_per_cluster, 46100); // Round up to allow platform variations. } TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { From 77e0ff9d90bf8ec50eb3096df3bf90689ce29b08 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Tue, 11 Aug 2020 17:59:02 +1000 Subject: [PATCH 31/35] Fix stats integration test on mac again Signed-off-by: Martin Matusiak --- test/integration/stats_integration_test.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index a6f20bb0b520..2ffc1ade58c9 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -288,7 +288,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // 2020/07/21 12034 44811 46000 Add configurable histogram buckets. // 2020/07/31 12035 45002 46000 Init manager store unready targets in hash map. // 2020/08/10 12275 44949 46000 Re-organize tls histogram maps to improve continuity. - // 2020/08/11 12202 44949 46100 router: add new retry back-off strategy + // 2020/08/11 12202 44949 46500 router: add new retry back-off strategy // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -309,7 +309,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // https://github.com/envoyproxy/envoy/issues/12209 // EXPECT_MEMORY_EQ(m_per_cluster, 44949); } - EXPECT_MEMORY_LE(m_per_cluster, 46100); // Round up to allow platform variations. + EXPECT_MEMORY_LE(m_per_cluster, 46500); // Round up to allow platform variations. } TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { @@ -367,6 +367,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // 2020/07/21 12034 36923 38000 Add configurable histogram buckets. // 2020/07/31 12035 37114 38000 Init manager store unready targets in hash map. // 2020/08/10 12275 37061 38000 Re-organize tls histogram maps to improve continuity. + // 2020/08/11 12202 37061 38500 router: add new retry back-off strategy // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -387,7 +388,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // https://github.com/envoyproxy/envoy/issues/12209 // EXPECT_MEMORY_EQ(m_per_cluster, 37061); } - EXPECT_MEMORY_LE(m_per_cluster, 38000); // Round up to allow platform variations. + EXPECT_MEMORY_LE(m_per_cluster, 38500); // Round up to allow platform variations. } TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { From d7d798ce5f5831aac8e4f031c23da69fba647a5f Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Thu, 13 Aug 2020 09:45:20 +1000 Subject: [PATCH 32/35] Fix bad merge in release notes Signed-off-by: Martin Matusiak --- docs/root/version_history/current.rst | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index f4def65319c1..ca3ceb4a25d3 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -62,11 +62,7 @@ New Features * postgres network filter: :ref:`metadata ` is produced based on SQL query. * ratelimit: added :ref:`enable_x_ratelimit_headers ` option to enable `X-RateLimit-*` headers as defined in `draft RFC `_. * rbac filter: added a log action to the :ref:`RBAC filter ` which sets dynamic metadata to inform access loggers whether to log. -* router: added a new :ref:`rate limited retry back off - ` - strategy that uses headers like `Retry-After` or `X-RateLimit-Reset` to - decide the back off interval. -* redis: added fault injection support :ref:`fault injection for redis proxy `, described further in :ref:`configuration documentation `. +* router: added a new :ref:`rate limited retry back off ` strategy that uses headers like `Retry-After` or `X-RateLimit-Reset` to decide the back off interval. * router: added new :ref:`envoy-ratelimited` retry policy, which allows retrying envoy's own rate limited responses. From 78b319ebb2e35743456d67c39215bbe6bedc43a1 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Thu, 13 Aug 2020 10:59:58 +1000 Subject: [PATCH 33/35] Kick CI Signed-off-by: Martin Matusiak From f7fc6b46f50028e7a54e0c0b30f40546cfa24d59 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Thu, 13 Aug 2020 15:20:25 +1000 Subject: [PATCH 34/35] Kick CI Signed-off-by: Martin Matusiak From 8521e028ad1324888b3292c896105015d88dd763 Mon Sep 17 00:00:00 2001 From: Martin Matusiak Date: Fri, 14 Aug 2020 06:50:02 +1000 Subject: [PATCH 35/35] Fix line that got lost in bad merge Signed-off-by: Martin Matusiak --- docs/root/version_history/current.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index ca3ceb4a25d3..f7c2f197efce 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -62,6 +62,7 @@ New Features * postgres network filter: :ref:`metadata ` is produced based on SQL query. * ratelimit: added :ref:`enable_x_ratelimit_headers ` option to enable `X-RateLimit-*` headers as defined in `draft RFC `_. * rbac filter: added a log action to the :ref:`RBAC filter ` which sets dynamic metadata to inform access loggers whether to log. +* redis: added fault injection support :ref:`fault injection for redis proxy `, described further in :ref:`configuration documentation `. * router: added a new :ref:`rate limited retry back off ` strategy that uses headers like `Retry-After` or `X-RateLimit-Reset` to decide the back off interval. * router: added new :ref:`envoy-ratelimited`