diff --git a/api/envoy/api/v2/route/route_components.proto b/api/envoy/api/v2/route/route_components.proto index a0a9324e7288a..8f95d98e24c58 100644 --- a/api/envoy/api/v2/route/route_components.proto +++ b/api/envoy/api/v2/route/route_components.proto @@ -32,7 +32,7 @@ option (udpa.annotations.file_migrate).move_to_package = "envoy.config.route.v3" // host header. This allows a single listener to service multiple top level domain path trees. Once // a virtual host is selected based on the domain, the routes are processed in order to see which // upstream cluster to route to or whether to perform a redirect. -// [#next-free-field: 19] +// [#next-free-field: 20] message VirtualHost { enum TlsRequirementType { // No TLS requirement for the virtual host. @@ -143,8 +143,20 @@ message VirtualHost { // This header is unaffected by the // :ref:`suppress_envoy_headers // ` flag. + // + // [#next-major-version: rename to include_attempt_count_in_request.] bool include_request_attempt_count = 14; + // Decides whether the :ref:`x-envoy-attempt-count + // ` header should be included + // in the downstream response. Setting this option will cause the router to override any existing header + // value, so in the case of two Envoys on the request path with this option enabled, the downstream + // will see the attempt count as perceived by the Envoy closest upstream from itself. Defaults to false. + // This header is unaffected by the + // :ref:`suppress_envoy_headers + // ` flag. + bool include_attempt_count_in_response = 19; + // Indicates the retry policy for all routes in this virtual host. Note that setting a // route level entry will take precedence over this config and it'll be treated // independently (e.g.: values are not inherited). diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index cfb499438830c..4340400a02e4d 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -32,7 +32,7 @@ option java_multiple_files = true; // host header. This allows a single listener to service multiple top level domain path trees. Once // a virtual host is selected based on the domain, the routes are processed in order to see which // upstream cluster to route to or whether to perform a redirect. -// [#next-free-field: 19] +// [#next-free-field: 20] message VirtualHost { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.route.VirtualHost"; @@ -140,8 +140,20 @@ message VirtualHost { // This header is unaffected by the // :ref:`suppress_envoy_headers // ` flag. + // + // [#next-major-version: rename to include_attempt_count_in_request.] bool include_request_attempt_count = 14; + // Decides whether the :ref:`x-envoy-attempt-count + // ` header should be included + // in the downstream response. Setting this option will cause the router to override any existing header + // value, so in the case of two Envoys on the request path with this option enabled, the downstream + // will see the attempt count as perceived by the Envoy closest upstream from itself. Defaults to false. + // This header is unaffected by the + // :ref:`suppress_envoy_headers + // ` flag. + bool include_attempt_count_in_response = 19; + // Indicates the retry policy for all routes in this virtual host. Note that setting a // route level entry will take precedence over this config and it'll be treated // independently (e.g.: values are not inherited). diff --git a/docs/root/configuration/http/http_filters/router_filter.rst b/docs/root/configuration/http/http_filters/router_filter.rst index ec99a134d8945..475fd9e93dd4c 100644 --- a/docs/root/configuration/http/http_filters/router_filter.rst +++ b/docs/root/configuration/http/http_filters/router_filter.rst @@ -36,8 +36,8 @@ or :ref:`config_http_filters_router_x-envoy-retry-grpc-on` headers are not speci A few notes on how Envoy does retries: * The route timeout (set via :ref:`config_http_filters_router_x-envoy-upstream-rq-timeout-ms` or the - :ref:`timeout ` in route configuration or set via - `grpc-timeout header `_ by specifying + :ref:`timeout ` in route configuration or set via + `grpc-timeout header `_ by specifying :ref:`max_grpc_timeout ` in route configuration) **includes** all 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 @@ -232,8 +232,8 @@ x-envoy-upstream-rq-timeout-ms ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Setting this header on egress requests will cause Envoy to override the :ref:`route configuration timeout -` or gRPC client timeout set via `grpc-timeout header -`_ by specifying :ref:`max_grpc_timeout +` or gRPC client timeout set via `grpc-timeout header +`_ by specifying :ref:`max_grpc_timeout `. The timeout must be specified in millisecond units. See also :ref:`config_http_filters_router_x-envoy-upstream-rq-per-try-timeout-ms`. @@ -319,7 +319,13 @@ x-envoy-attempt-count Sent to the upstream to indicate which attempt the current request is in a series of retries. The value will be "1" on the initial request, incrementing by one for each retry. Only set if the -:ref:`include_attempt_count_header ` +:ref:`include_request_attempt_count ` +flag is set to true. + +Sent to the downstream to indicate how many upstream requests took place. The header will be absent if +the router did not send any upstream requests. The value will be "1" if only the original upstream +request was sent, incrementing by one for each retry. Only set if the +:ref:`include_attempt_count_in_response ` flag is set to true. .. _config_http_filters_router_x-envoy-expected-rq-timeout-ms: diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 29f51dd27b668..1ce3c5d3367a0 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -41,6 +41,8 @@ Version history * router: added support for :ref:`regex_rewrite ` for path rewriting using regular expressions and capture groups. * router: don't ignore :ref:`per_try_timeout ` when the :ref:`global route timeout ` is disabled. +* router: added ability to set attempt count in downstream response, see :ref:`virtual host's include response + attempt count config `. * router: strip whitespace for :ref:`retry_on `, :ref:`grpc-retry-on header ` and :ref:`retry-on header `. * runtime: enabling the runtime feature "envoy.deprecated_features.allow_deprecated_extension_names" disables the use of deprecated extension names. diff --git a/generated_api_shadow/envoy/api/v2/route/route_components.proto b/generated_api_shadow/envoy/api/v2/route/route_components.proto index a0a9324e7288a..8f95d98e24c58 100644 --- a/generated_api_shadow/envoy/api/v2/route/route_components.proto +++ b/generated_api_shadow/envoy/api/v2/route/route_components.proto @@ -32,7 +32,7 @@ option (udpa.annotations.file_migrate).move_to_package = "envoy.config.route.v3" // host header. This allows a single listener to service multiple top level domain path trees. Once // a virtual host is selected based on the domain, the routes are processed in order to see which // upstream cluster to route to or whether to perform a redirect. -// [#next-free-field: 19] +// [#next-free-field: 20] message VirtualHost { enum TlsRequirementType { // No TLS requirement for the virtual host. @@ -143,8 +143,20 @@ message VirtualHost { // This header is unaffected by the // :ref:`suppress_envoy_headers // ` flag. + // + // [#next-major-version: rename to include_attempt_count_in_request.] bool include_request_attempt_count = 14; + // Decides whether the :ref:`x-envoy-attempt-count + // ` header should be included + // in the downstream response. Setting this option will cause the router to override any existing header + // value, so in the case of two Envoys on the request path with this option enabled, the downstream + // will see the attempt count as perceived by the Envoy closest upstream from itself. Defaults to false. + // This header is unaffected by the + // :ref:`suppress_envoy_headers + // ` flag. + bool include_attempt_count_in_response = 19; + // Indicates the retry policy for all routes in this virtual host. Note that setting a // route level entry will take precedence over this config and it'll be treated // independently (e.g.: values are not inherited). 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 c95ffd1ebcf16..c7d670554ab60 100644 --- a/generated_api_shadow/envoy/config/route/v3/route_components.proto +++ b/generated_api_shadow/envoy/config/route/v3/route_components.proto @@ -32,7 +32,7 @@ option java_multiple_files = true; // host header. This allows a single listener to service multiple top level domain path trees. Once // a virtual host is selected based on the domain, the routes are processed in order to see which // upstream cluster to route to or whether to perform a redirect. -// [#next-free-field: 19] +// [#next-free-field: 20] message VirtualHost { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.route.VirtualHost"; @@ -146,8 +146,20 @@ message VirtualHost { // This header is unaffected by the // :ref:`suppress_envoy_headers // ` flag. + // + // [#next-major-version: rename to include_attempt_count_in_request.] bool include_request_attempt_count = 14; + // Decides whether the :ref:`x-envoy-attempt-count + // ` header should be included + // in the downstream response. Setting this option will cause the router to override any existing header + // value, so in the case of two Envoys on the request path with this option enabled, the downstream + // will see the attempt count as perceived by the Envoy closest upstream from itself. Defaults to false. + // This header is unaffected by the + // :ref:`suppress_envoy_headers + // ` flag. + bool include_attempt_count_in_response = 19; + // Indicates the retry policy for all routes in this virtual host. Note that setting a // route level entry will take precedence over this config and it'll be treated // independently (e.g.: values are not inherited). diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index febd31e781e0e..8dcc22a6be09c 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -274,7 +274,6 @@ class HeaderEntry { HEADER_FUNC(AccessControlRequestMethod) \ HEADER_FUNC(Authorization) \ HEADER_FUNC(ClientTraceId) \ - HEADER_FUNC(EnvoyAttemptCount) \ HEADER_FUNC(EnvoyDownstreamServiceCluster) \ HEADER_FUNC(EnvoyDownstreamServiceNode) \ HEADER_FUNC(EnvoyExpectedRequestTimeoutMs) \ @@ -344,6 +343,7 @@ class HeaderEntry { HEADER_FUNC(Connection) \ HEADER_FUNC(ContentLength) \ HEADER_FUNC(ContentType) \ + HEADER_FUNC(EnvoyAttemptCount) \ HEADER_FUNC(EnvoyDecoratorOperation) \ HEADER_FUNC(KeepAlive) \ HEADER_FUNC(NoChunks) \ diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 2aa207cf0ba34..d3b9c3c500890 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -443,7 +443,12 @@ class VirtualHost { /** * @return bool whether to include the request count header in upstream requests. */ - virtual bool includeAttemptCount() const PURE; + virtual bool includeAttemptCountInRequest() const PURE; + + /** + * @return bool whether to include the request count header in the downstream response. + */ + virtual bool includeAttemptCountInResponse() const PURE; /** * @return uint32_t any route cap on bytes which should be buffered for shadowing or retries. @@ -778,7 +783,14 @@ class RouteEntry : public ResponseEntry { * count header. * @return bool whether x-envoy-attempt-count should be included on the upstream request. */ - virtual bool includeAttemptCount() const PURE; + virtual bool includeAttemptCountInRequest() const PURE; + + /** + * True if the virtual host this RouteEntry belongs to is configured to include the attempt + * count header. + * @return bool whether x-envoy-attempt-count should be included on the downstream response. + */ + virtual bool includeAttemptCountInResponse() const PURE; using UpgradeMap = std::map; /** diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 92d8567104660..608b2188dc1bd 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -184,7 +184,8 @@ class AsyncStreamImpl : public AsyncClient::Stream, const Router::RouteSpecificFilterConfig* perFilterConfig(const std::string&) const override { return nullptr; } - bool includeAttemptCount() const override { return false; } + bool includeAttemptCountInRequest() const override { return false; } + bool includeAttemptCountInResponse() const override { return false; } uint32_t retryShadowBufferLimit() const override { return std::numeric_limits::max(); } @@ -268,7 +269,8 @@ class AsyncStreamImpl : public AsyncClient::Stream, return nullptr; } - bool includeAttemptCount() const override { return false; } + bool includeAttemptCountInRequest() const override { return false; } + bool includeAttemptCountInResponse() const override { return false; } const Router::RouteEntry::UpgradeMap& upgradeMap() const override { return upgrade_map_; } Router::InternalRedirectAction internalRedirectAction() const override { return Router::InternalRedirectAction::PassThrough; diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 79462f947afc5..64e0e33f91f9e 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -940,7 +940,8 @@ VirtualHostImpl::VirtualHostImpl(const envoy::config::route::v3::VirtualHost& vi validator), retry_shadow_buffer_limit_(PROTOBUF_GET_WRAPPED_OR_DEFAULT( virtual_host, per_request_buffer_limit_bytes, std::numeric_limits::max())), - include_attempt_count_(virtual_host.include_request_attempt_count()), + include_attempt_count_in_request_(virtual_host.include_request_attempt_count()), + include_attempt_count_in_response_(virtual_host.include_attempt_count_in_response()), virtual_cluster_catch_all_(stat_name_pool_) { switch (virtual_host.require_tls()) { diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 87157fdc2eab3..c2e654b00e847 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -174,7 +174,8 @@ class VirtualHostImpl : public VirtualHost { const RateLimitPolicy& rateLimitPolicy() const override { return rate_limit_policy_; } const Config& routeConfig() const override; const RouteSpecificFilterConfig* perFilterConfig(const std::string&) const override; - bool includeAttemptCount() const override { return include_attempt_count_; } + bool includeAttemptCountInRequest() const override { return include_attempt_count_in_request_; } + bool includeAttemptCountInResponse() const override { return include_attempt_count_in_response_; } const absl::optional& retryPolicy() const { return retry_policy_; } @@ -223,7 +224,8 @@ class VirtualHostImpl : public VirtualHost { HeaderParserPtr response_headers_parser_; PerFilterConfigs per_filter_configs_; uint32_t retry_shadow_buffer_limit_{std::numeric_limits::max()}; - const bool include_attempt_count_; + const bool include_attempt_count_in_request_; + const bool include_attempt_count_in_response_; absl::optional retry_policy_; absl::optional hedge_policy_; const CatchAllVirtualCluster virtual_cluster_catch_all_; @@ -451,7 +453,12 @@ class RouteEntryImplBase : public RouteEntry, const envoy::config::core::v3::Metadata& metadata() const override { return metadata_; } const Envoy::Config::TypedMetadata& typedMetadata() const override { return typed_metadata_; } const PathMatchCriterion& pathMatchCriterion() const override { return *this; } - bool includeAttemptCount() const override { return vhost_.includeAttemptCount(); } + bool includeAttemptCountInRequest() const override { + return vhost_.includeAttemptCountInRequest(); + } + bool includeAttemptCountInResponse() const override { + return vhost_.includeAttemptCountInResponse(); + } const UpgradeMap& upgradeMap() const override { return upgrade_map_; } InternalRedirectAction internalRedirectAction() const override { return internal_redirect_action_; @@ -572,7 +579,12 @@ class RouteEntryImplBase : public RouteEntry, return parent_->pathMatchCriterion(); } - bool includeAttemptCount() const override { return parent_->includeAttemptCount(); } + bool includeAttemptCountInRequest() const override { + return parent_->includeAttemptCountInRequest(); + } + bool includeAttemptCountInResponse() const override { + return parent_->includeAttemptCountInResponse(); + } const UpgradeMap& upgradeMap() const override { return parent_->upgradeMap(); } InternalRedirectAction internalRedirectAction() const override { return parent_->internalRedirectAction(); diff --git a/source/common/router/router.cc b/source/common/router/router.cc index ed0b02eb8be98..44d1d58ee3cf1 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -405,7 +405,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, // Initialize the `modify_headers` function as a no-op (so we don't have to remember to check it // against nullptr before calling it), and feed it behavior later if/when we have cluster info // headers to append. - std::function modify_headers = [](Http::HeaderMap&) {}; + std::function modify_headers = [](Http::ResponseHeaderMap&) {}; // Determine if there is a route entry or a direct response for the request. route_ = callbacks_->route(); @@ -453,7 +453,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, callbacks_->streamInfo().setRouteName(route_entry_->routeName()); if (debug_config && debug_config->append_cluster_) { // The cluster name will be appended to any local or upstream responses from this point. - modify_headers = [this, debug_config](Http::HeaderMap& headers) { + modify_headers = [this, debug_config](Http::ResponseHeaderMap& headers) { headers.addCopy(debug_config->cluster_header_.value_or(Http::Headers::get().EnvoyCluster), route_entry_->clusterName()); }; @@ -550,7 +550,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, if (debug_config && debug_config->append_upstream_host_) { // The hostname and address will be appended to any local or upstream responses from this point, // possibly in addition to the cluster name. - modify_headers = [modify_headers, debug_config, conn_pool](Http::HeaderMap& headers) { + modify_headers = [modify_headers, debug_config, conn_pool](Http::ResponseHeaderMap& headers) { modify_headers(headers); headers.addCopy( debug_config->hostname_header_.value_or(Http::Headers::get().EnvoyUpstreamHostname), @@ -563,7 +563,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, // If we've been instructed not to forward the request upstream, send an empty local response. if (debug_config && debug_config->do_not_forward_) { - modify_headers = [modify_headers, debug_config](Http::HeaderMap& headers) { + modify_headers = [modify_headers, debug_config](Http::ResponseHeaderMap& headers) { modify_headers(headers); headers.addCopy( debug_config->not_forwarded_header_.value_or(Http::Headers::get().EnvoyNotForwarded), @@ -585,11 +585,24 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, headers.removeEnvoyUpstreamRequestTimeoutAltResponse(); } - include_attempt_count_ = route_entry_->includeAttemptCount(); - if (include_attempt_count_) { + include_attempt_count_in_request_ = route_entry_->includeAttemptCountInRequest(); + if (include_attempt_count_in_request_) { headers.setEnvoyAttemptCount(attempt_count_); } + // The router has reached a point where it is going to try to send a request upstream, + // so now modify_headers should attach x-envoy-attempt-count to the downstream response if the + // config flag is true. + if (route_entry_->includeAttemptCountInResponse()) { + modify_headers = [modify_headers, this](Http::ResponseHeaderMap& headers) { + modify_headers(headers); + + // This header is added without checking for config_.suppress_envoy_headers_ to mirror what is + // done for upstream requests. + headers.setEnvoyAttemptCount(attempt_count_); + }; + } + // Inject the active span's tracing context into the request headers. callbacks_->activeSpan().injectContext(headers); @@ -1416,7 +1429,7 @@ void Filter::doRetry() { return; } - if (include_attempt_count_) { + if (include_attempt_count_in_request_) { downstream_headers_->setEnvoyAttemptCount(attempt_count_); } diff --git a/source/common/router/router.h b/source/common/router/router.h index 62bdb9d212f6b..7eaf5daeddd15 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -469,7 +469,7 @@ class Filter : Logger::Loggable, MonotonicTime downstream_request_complete_time_; uint32_t retry_shadow_buffer_limit_{std::numeric_limits::max()}; MetadataMatchCriteriaConstPtr metadata_match_; - std::function modify_headers_; + std::function modify_headers_; std::vector> active_shadow_policies_{}; // list of cookies to add to upstream headers @@ -478,7 +478,7 @@ class Filter : Logger::Loggable, bool downstream_response_started_ : 1; bool downstream_end_stream_ : 1; bool is_retry_ : 1; - bool include_attempt_count_ : 1; + bool include_attempt_count_in_request_ : 1; bool attempting_internal_redirect_with_complete_stream_ : 1; uint32_t attempt_count_{1}; uint32_t pending_retries_{0}; diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index d17b09311c561..d7fde7e1f1144 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -1291,7 +1291,8 @@ TEST_F(AsyncClientImplUnitTest, RouteImplInitTest) { EXPECT_TRUE(route_impl_.routeEntry()->virtualHost().rateLimitPolicy().empty()); EXPECT_EQ(nullptr, route_impl_.routeEntry()->virtualHost().corsPolicy()); EXPECT_EQ(nullptr, route_impl_.routeEntry()->virtualHost().perFilterConfig("bar")); - EXPECT_FALSE(route_impl_.routeEntry()->virtualHost().includeAttemptCount()); + EXPECT_FALSE(route_impl_.routeEntry()->virtualHost().includeAttemptCountInRequest()); + EXPECT_FALSE(route_impl_.routeEntry()->virtualHost().includeAttemptCountInResponse()); EXPECT_FALSE(route_impl_.routeEntry()->virtualHost().routeConfig().usesVhds()); EXPECT_EQ(nullptr, route_impl_.routeEntry()->tlsContextMatchCriteria()); } diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index cef5b608b7289..d232c86b26c90 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -2820,6 +2820,7 @@ TEST_F(RouteMatcherTest, AttemptCountHeader) { - name: "www2" domains: ["www.lyft.com"] include_request_attempt_count: true + include_attempt_count_in_response: true routes: - match: { prefix: "/"} route: @@ -2830,7 +2831,11 @@ TEST_F(RouteMatcherTest, AttemptCountHeader) { EXPECT_TRUE(config.route(genHeaders("www.lyft.com", "/foo", "GET"), 0) ->routeEntry() - ->includeAttemptCount()); + ->includeAttemptCountInRequest()); + + EXPECT_TRUE(config.route(genHeaders("www.lyft.com", "/foo", "GET"), 0) + ->routeEntry() + ->includeAttemptCountInResponse()); } TEST_F(RouteMatcherTest, ClusterNotFoundResponseCode) { diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 66a8b480caeb7..b7708eb7440ae 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -205,9 +205,9 @@ class RouterTestBase : public testing::Test { router_.onDestroy(); } - void verifyAttemptCountBasic(bool set_include_attempt_count, absl::optional preset_count, - int expected_count) { - setIncludeAttemptCount(set_include_attempt_count); + void verifyAttemptCountInRequestBasic(bool set_include_attempt_count_in_request, + absl::optional preset_count, int expected_count) { + setIncludeAttemptCountInRequest(set_include_attempt_count_in_request); EXPECT_CALL(cm_.conn_pool_, newStream(_, _)).WillOnce(Return(&cancellable_)); expectResponseTimerCreate(); @@ -228,6 +228,43 @@ class RouterTestBase : public testing::Test { EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); } + void verifyAttemptCountInResponseBasic(bool set_include_attempt_count_in_response, + absl::optional preset_count, int expected_count) { + setIncludeAttemptCountInResponse(set_include_attempt_count_in_response); + + NiceMock encoder1; + Http::ResponseDecoder* response_decoder = nullptr; + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)) + .WillOnce(Invoke( + [&](Http::ResponseDecoder& decoder, + Http::ConnectionPool::Callbacks& callbacks) -> Http::ConnectionPool::Cancellable* { + response_decoder = &decoder; + callbacks.onPoolReady(encoder1, cm_.conn_pool_.host_, upstream_stream_info_); + return nullptr; + })); + expectResponseTimerCreate(); + + Http::TestRequestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + + Http::ResponseHeaderMapPtr response_headers( + new Http::TestResponseHeaderMapImpl{{":status", "200"}}); + if (preset_count) { + response_headers->setEnvoyAttemptCount(preset_count.value()); + } + + EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(200)); + EXPECT_CALL(callbacks_, encodeHeaders_(_, true)) + .WillOnce(Invoke([expected_count](Http::ResponseHeaderMap& headers, bool) { + EXPECT_EQ( + expected_count, + atoi(std::string(headers.EnvoyAttemptCount()->value().getStringView()).c_str())); + })); + response_decoder->decodeHeaders(std::move(response_headers), true); + EXPECT_TRUE(verifyHostUpstreamStats(1, 0)); + } + void sendRequest(bool end_stream = true) { if (end_stream) { EXPECT_CALL(callbacks_.dispatcher_, createTimer_(_)).Times(1); @@ -265,8 +302,14 @@ class RouterTestBase : public testing::Test { StreamInfo::FilterState::LifeSpan::DownstreamRequest); } - void setIncludeAttemptCount(bool include) { - ON_CALL(callbacks_.route_->route_entry_, includeAttemptCount()).WillByDefault(Return(include)); + void setIncludeAttemptCountInRequest(bool include) { + ON_CALL(callbacks_.route_->route_entry_, includeAttemptCountInRequest()) + .WillByDefault(Return(include)); + } + + void setIncludeAttemptCountInResponse(bool include) { + ON_CALL(callbacks_.route_->route_entry_, includeAttemptCountInResponse()) + .WillByDefault(Return(include)); } void enableHedgeOnPerTryTimeout() { @@ -897,8 +940,8 @@ TEST_F(RouterTest, EnvoyUpstreamServiceTime) { // Validate that x-envoy-attempt-count is added to request headers when the option is true. TEST_F(RouterTest, EnvoyAttemptCountInRequest) { - verifyAttemptCountBasic( - /* set_include_attempt_count */ true, + verifyAttemptCountInRequestBasic( + /* set_include_attempt_count_in_request */ true, /* preset_count*/ absl::nullopt, /* expected_count */ 1); } @@ -906,8 +949,8 @@ TEST_F(RouterTest, EnvoyAttemptCountInRequest) { // Validate that x-envoy-attempt-count is overwritten by the router on request headers, if the // header is sent from the downstream and the option is set to true. TEST_F(RouterTest, EnvoyAttemptCountInRequestOverwritten) { - verifyAttemptCountBasic( - /* set_include_attempt_count */ true, + verifyAttemptCountInRequestBasic( + /* set_include_attempt_count_in_request */ true, /* preset_count*/ 123, /* expected_count */ 1); } @@ -915,14 +958,14 @@ TEST_F(RouterTest, EnvoyAttemptCountInRequestOverwritten) { // Validate that x-envoy-attempt-count is not overwritten by the router on request headers, if the // header is sent from the downstream and the option is set to false. TEST_F(RouterTest, EnvoyAttemptCountInRequestNotOverwritten) { - verifyAttemptCountBasic( - /* set_include_attempt_count */ false, + verifyAttemptCountInRequestBasic( + /* set_include_attempt_count_in_request */ false, /* preset_count*/ 123, /* expected_count */ 123); } TEST_F(RouterTest, EnvoyAttemptCountInRequestUpdatedInRetries) { - setIncludeAttemptCount(true); + setIncludeAttemptCountInRequest(true); NiceMock encoder1; Http::ResponseDecoder* response_decoder = nullptr; @@ -977,6 +1020,140 @@ TEST_F(RouterTest, EnvoyAttemptCountInRequestUpdatedInRetries) { EXPECT_TRUE(verifyHostUpstreamStats(1, 1)); } +// Validate that x-envoy-attempt-count is added when option is true. +TEST_F(RouterTest, EnvoyAttemptCountInResponse) { + verifyAttemptCountInResponseBasic( + /* set_include_attempt_count_in_response */ true, + /* preset_count */ absl::nullopt, + /* expected_count */ 1); +} + +// Validate that x-envoy-attempt-count is overwritten by the router on response headers, if the +// header is sent from the upstream and the option is set to true. +TEST_F(RouterTest, EnvoyAttemptCountInResponseOverwritten) { + verifyAttemptCountInResponseBasic( + /* set_include_attempt_count_in_response */ true, + /* preset_count */ 123, + /* expected_count */ 1); +} + +// Validate that x-envoy-attempt-count is not overwritten by the router on response headers, if the +// header is sent from the upstream and the option is not set to true. +TEST_F(RouterTest, EnvoyAttemptCountInResponseNotOverwritten) { + verifyAttemptCountInResponseBasic( + /* set_include_attempt_count_in_response */ false, + /* preset_count */ 123, + /* expected_count */ 123); +} + +// Validate that we don't set x-envoy-attempt-count in responses before an upstream attempt is made. +TEST_F(RouterTestSuppressEnvoyHeaders, EnvoyAttemptCountInResponseNotPresent) { + setIncludeAttemptCountInResponse(true); + + EXPECT_CALL(*cm_.thread_local_cluster_.cluster_.info_, maintenanceMode()).WillOnce(Return(true)); + + Http::TestResponseHeaderMapImpl response_headers{ + {":status", "503"}, {"content-length", "16"}, {"content-type", "text/plain"}}; + EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false)); + EXPECT_CALL(callbacks_, encodeData(_, true)); + EXPECT_CALL(callbacks_.stream_info_, setResponseFlag(StreamInfo::ResponseFlag::UpstreamOverflow)); + + Http::TestRequestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); +} + +// Validate that x-envoy-attempt-count is present in local replies after an upstream attempt is +// made. +TEST_F(RouterTest, EnvoyAttemptCountInResponsePresentWithLocalReply) { + setIncludeAttemptCountInResponse(true); + + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)) + .WillOnce(Invoke([&](Http::StreamDecoder&, Http::ConnectionPool::Callbacks& callbacks) + -> Http::ConnectionPool::Cancellable* { + callbacks.onPoolFailure(Http::ConnectionPool::PoolFailureReason::ConnectionFailure, + absl::string_view(), cm_.conn_pool_.host_); + return nullptr; + })); + + Http::TestResponseHeaderMapImpl response_headers{{":status", "503"}, + {"content-length", "91"}, + {"content-type", "text/plain"}, + {"x-envoy-attempt-count", "1"}}; + EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false)); + EXPECT_CALL(callbacks_, encodeData(_, true)); + EXPECT_CALL(callbacks_.stream_info_, + setResponseFlag(StreamInfo::ResponseFlag::UpstreamConnectionFailure)); + EXPECT_CALL(callbacks_.stream_info_, onUpstreamHostSelected(_)) + .WillOnce(Invoke([&](const Upstream::HostDescriptionConstSharedPtr host) -> void { + EXPECT_EQ(host_address_, host->address()); + })); + + Http::TestRequestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + EXPECT_TRUE(verifyHostUpstreamStats(0, 1)); + EXPECT_EQ(callbacks_.details_, "upstream_reset_before_response_started{connection failure}"); +} + +// Validate that the x-envoy-attempt-count header in the downstream response reflects the number of +// of upstream requests that occurred when retries take place. +TEST_F(RouterTest, EnvoyAttemptCountInResponseWithRetries) { + setIncludeAttemptCountInResponse(true); + + NiceMock encoder1; + Http::ResponseDecoder* response_decoder = nullptr; + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)) + .WillOnce(Invoke( + [&](Http::ResponseDecoder& decoder, + Http::ConnectionPool::Callbacks& callbacks) -> Http::ConnectionPool::Cancellable* { + response_decoder = &decoder; + callbacks.onPoolReady(encoder1, cm_.conn_pool_.host_, upstream_stream_info_); + return nullptr; + })); + expectResponseTimerCreate(); + + Http::TestRequestHeaderMapImpl headers{{"x-envoy-retry-on", "5xx"}, {"x-envoy-internal", "true"}}; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + + // 5xx response. + router_.retry_state_->expectHeadersRetry(); + Http::ResponseHeaderMapPtr response_headers1( + new Http::TestResponseHeaderMapImpl{{":status", "503"}}); + EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(503)); + response_decoder->decodeHeaders(std::move(response_headers1), true); + EXPECT_TRUE(verifyHostUpstreamStats(0, 1)); + + // We expect the 5xx response to kick off a new request. + EXPECT_CALL(encoder1.stream_, resetStream(_)).Times(0); + NiceMock encoder2; + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)) + .WillOnce(Invoke( + [&](Http::ResponseDecoder& decoder, + Http::ConnectionPool::Callbacks& callbacks) -> Http::ConnectionPool::Cancellable* { + response_decoder = &decoder; + callbacks.onPoolReady(encoder2, cm_.conn_pool_.host_, upstream_stream_info_); + return nullptr; + })); + router_.retry_state_->callback_(); + + // Normal response. + EXPECT_CALL(*router_.retry_state_, shouldRetryHeaders(_, _)).WillOnce(Return(RetryStatus::No)); + EXPECT_CALL(cm_.conn_pool_.host_->health_checker_, setUnhealthy()).Times(0); + Http::ResponseHeaderMapPtr response_headers2( + new Http::TestResponseHeaderMapImpl{{":status", "200"}}); + EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(200)); + EXPECT_CALL(callbacks_, encodeHeaders_(_, true)) + .WillOnce(Invoke([](Http::ResponseHeaderMap& headers, bool) { + // Because a retry happened the number of attempts in the response headers should be 2. + EXPECT_EQ(2, + atoi(std::string(headers.EnvoyAttemptCount()->value().getStringView()).c_str())); + })); + response_decoder->decodeHeaders(std::move(response_headers2), true); + EXPECT_TRUE(verifyHostUpstreamStats(1, 1)); +} + // Validate that the cluster is appended to the response when configured. void RouterTestBase::testAppendCluster(absl::optional cluster_header_name) { auto debug_config = std::make_unique( diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 9a16749531a8d..8b95262d93d27 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -667,6 +667,7 @@ void HttpIntegrationTest::testRetry() { void HttpIntegrationTest::testRetryAttemptCountHeader() { auto host = config_helper_.createVirtualHost("host", "/test_retry"); host.set_include_request_attempt_count(true); + host.set_include_attempt_count_in_response(true); config_helper_.addVirtualHost(host); initialize(); @@ -708,6 +709,9 @@ void HttpIntegrationTest::testRetryAttemptCountHeader() { EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ(512U, response->body().size()); + EXPECT_EQ( + 2, + atoi(std::string(response->headers().EnvoyAttemptCount()->value().getStringView()).c_str())); } void HttpIntegrationTest::testGrpcRetry() { diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 905e84a94a2af..16a22cec1eec6 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -295,11 +295,12 @@ TEST_P(ProtocolIntegrationTest, Retry) { EXPECT_EQ(512U, response->body().size()); } -// Tests that the x-envoy-attempt-count header is properly set on the upstream request -// and updated after the request is retried. +// Tests that the x-envoy-attempt-count header is properly set on the upstream request and the +// downstream response, and updated after the request is retried. TEST_P(DownstreamProtocolIntegrationTest, RetryAttemptCountHeader) { auto host = config_helper_.createVirtualHost("host", "/test_retry"); host.set_include_request_attempt_count(true); + host.set_include_attempt_count_in_response(true); config_helper_.addVirtualHost(host); initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); @@ -340,6 +341,9 @@ TEST_P(DownstreamProtocolIntegrationTest, RetryAttemptCountHeader) { EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ(512U, response->body().size()); + EXPECT_EQ( + 2, + atoi(std::string(response->headers().EnvoyAttemptCount()->value().getStringView()).c_str())); } // Verifies that a retry priority can be configured and affect the host selected during retries. diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index f25cbb12a3971..b5ef51ca4dbcf 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -241,7 +241,8 @@ class MockVirtualHost : public VirtualHost { MOCK_METHOD(const CorsPolicy*, corsPolicy, (), (const)); MOCK_METHOD(const Config&, routeConfig, (), (const)); MOCK_METHOD(const RouteSpecificFilterConfig*, perFilterConfig, (const std::string&), (const)); - MOCK_METHOD(bool, includeAttemptCount, (), (const)); + MOCK_METHOD(bool, includeAttemptCountInRequest, (), (const)); + MOCK_METHOD(bool, includeAttemptCountInResponse, (), (const)); MOCK_METHOD(Upstream::RetryPrioritySharedPtr, retryPriority, ()); MOCK_METHOD(Upstream::RetryHostPredicateSharedPtr, retryHostPredicate, ()); MOCK_METHOD(uint32_t, retryShadowBufferLimit, (), (const)); @@ -347,7 +348,8 @@ class MockRouteEntry : public RouteEntry { MOCK_METHOD(const Envoy::Config::TypedMetadata&, typedMetadata, (), (const)); MOCK_METHOD(const PathMatchCriterion&, pathMatchCriterion, (), (const)); MOCK_METHOD(const RouteSpecificFilterConfig*, perFilterConfig, (const std::string&), (const)); - MOCK_METHOD(bool, includeAttemptCount, (), (const)); + MOCK_METHOD(bool, includeAttemptCountInRequest, (), (const)); + MOCK_METHOD(bool, includeAttemptCountInResponse, (), (const)); MOCK_METHOD(const UpgradeMap&, upgradeMap, (), (const)); MOCK_METHOD(InternalRedirectAction, internalRedirectAction, (), (const)); MOCK_METHOD(uint32_t, maxInternalRedirects, (), (const));