From 1f68b94cf36822947ffc1d13c527bc6786ff351c Mon Sep 17 00:00:00 2001 From: Snow Pettersen Date: Mon, 8 Oct 2018 14:22:15 -0400 Subject: [PATCH 1/6] router: add grpc-retry-on "internal" policy This allows configuring a gRPC retry polic which retries on internal (13) responses. Signed-off-by: Snow Pettersen --- .../configuration/http_filters/router_filter.rst | 2 ++ include/envoy/router/router.h | 1 + source/common/http/headers.h | 1 + source/common/router/retry_state_impl.cc | 8 ++++++-- test/common/router/retry_state_impl_test.cc | 14 ++++++++++++++ 5 files changed, 24 insertions(+), 2 deletions(-) diff --git a/docs/root/configuration/http_filters/router_filter.rst b/docs/root/configuration/http_filters/router_filter.rst index 2ae24ac7e73be..1b2d704157449 100644 --- a/docs/root/configuration/http_filters/router_filter.rst +++ b/docs/root/configuration/http_filters/router_filter.rst @@ -122,6 +122,8 @@ cancelled deadline-exceeded Envoy will attempt a retry if the gRPC status code in the response headers is "deadline-exceeded" (4) +internal + Envoy will attempt to retry if the gRPC status code in the response headers is "internal" (13) resource-exhausted Envoy will attempt a retry if the gRPC status code in the response headers is "resource-exhausted" (8) diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 5129ba9feef3e..acad46189ff7a 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -151,6 +151,7 @@ class RetryPolicy { static const uint32_t RETRY_ON_GRPC_DEADLINE_EXCEEDED = 0x40; static const uint32_t RETRY_ON_GRPC_RESOURCE_EXHAUSTED = 0x80; static const uint32_t RETRY_ON_GRPC_UNAVAILABLE = 0x100; + static const uint32_t RETRY_ON_GRPC_INTERNAL = 0x101; // clang-format on virtual ~RetryPolicy() {} diff --git a/source/common/http/headers.h b/source/common/http/headers.h index abe23615dfa23..d611ed6792208 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -152,6 +152,7 @@ class HeaderValues { const std::string DeadlineExceeded{"deadline-exceeded"}; const std::string ResourceExhausted{"resource-exhausted"}; const std::string Unavailable{"unavailable"}; + const std::string Internal{"internal"}; } EnvoyRetryOnGrpcValues; struct { diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc index 0fddc40c2264a..44c660574b399 100644 --- a/source/common/router/retry_state_impl.cc +++ b/source/common/router/retry_state_impl.cc @@ -119,6 +119,8 @@ uint32_t RetryStateImpl::parseRetryGrpcOn(absl::string_view retry_grpc_on_header ret |= RetryPolicy::RETRY_ON_GRPC_RESOURCE_EXHAUSTED; } else if (retry_on == Http::Headers::get().EnvoyRetryOnGrpcValues.Unavailable) { ret |= RetryPolicy::RETRY_ON_GRPC_UNAVAILABLE; + } else if (retry_on == Http::Headers::get().EnvoyRetryOnGrpcValues.Internal) { + ret |= RetryPolicy::RETRY_ON_GRPC_INTERNAL; } } @@ -219,7 +221,7 @@ bool RetryStateImpl::wouldRetry(const Http::HeaderMap* response_headers, if (retry_on_ & (RetryPolicy::RETRY_ON_GRPC_CANCELLED | RetryPolicy::RETRY_ON_GRPC_DEADLINE_EXCEEDED | RetryPolicy::RETRY_ON_GRPC_RESOURCE_EXHAUSTED | - RetryPolicy::RETRY_ON_GRPC_UNAVAILABLE) && + RetryPolicy::RETRY_ON_GRPC_UNAVAILABLE | RetryPolicy::RETRY_ON_GRPC_INTERNAL) && response_headers) { absl::optional status = Grpc::Common::getGrpcStatus(*response_headers); @@ -231,7 +233,9 @@ bool RetryStateImpl::wouldRetry(const Http::HeaderMap* response_headers, (status.value() == Grpc::Status::ResourceExhausted && (retry_on_ & RetryPolicy::RETRY_ON_GRPC_RESOURCE_EXHAUSTED)) || (status.value() == Grpc::Status::Unavailable && - (retry_on_ & RetryPolicy::RETRY_ON_GRPC_UNAVAILABLE))) { + (retry_on_ & RetryPolicy::RETRY_ON_GRPC_UNAVAILABLE)) || + (status.value() == Grpc::Status::Internal && + (retry_on_ & RetryPolicy::RETRY_ON_GRPC_INTERNAL))) { return true; } } diff --git a/test/common/router/retry_state_impl_test.cc b/test/common/router/retry_state_impl_test.cc index f5931569ff351..3402978e5b95a 100644 --- a/test/common/router/retry_state_impl_test.cc +++ b/test/common/router/retry_state_impl_test.cc @@ -242,6 +242,20 @@ TEST_F(RouterRetryStateImplTest, PolicyGrpcUnavilable) { EXPECT_EQ(RetryStatus::No, state_->shouldRetry(&response_headers, no_reset_, callback_)); } +TEST_F(RouterRetryStateImplTest, PolicyGrpcInternal) { + Http::TestHeaderMapImpl request_headers{{"x-envoy-retry-grpc-on", "internal"}}; + setup(request_headers); + EXPECT_TRUE(state_->enabled()); + + Http::TestHeaderMapImpl response_headers{{":status", "200"}, {"grpc-status", "13"}}; + expectTimerCreateAndEnable(); + EXPECT_EQ(RetryStatus::Yes, state_->shouldRetry(&response_headers, no_reset_, callback_)); + EXPECT_CALL(callback_ready_, ready()); + retry_timer_->callback_(); + + EXPECT_EQ(RetryStatus::No, state_->shouldRetry(&response_headers, no_reset_, callback_)); +} + TEST_F(RouterRetryStateImplTest, Policy5xxRemote200RemoteReset) { // Don't retry after reply start. Http::TestHeaderMapImpl request_headers{{"x-envoy-retry-on", "5xx"}}; From 5b19af9d67678d3df398b547d03dfae77093a72f Mon Sep 17 00:00:00 2001 From: Snow Pettersen Date: Mon, 8 Oct 2018 14:51:08 -0400 Subject: [PATCH 2/6] format Signed-off-by: Snow Pettersen --- source/common/router/retry_state_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc index 44c660574b399..1632a8ed0d0f8 100644 --- a/source/common/router/retry_state_impl.cc +++ b/source/common/router/retry_state_impl.cc @@ -220,8 +220,8 @@ bool RetryStateImpl::wouldRetry(const Http::HeaderMap* response_headers, if (retry_on_ & (RetryPolicy::RETRY_ON_GRPC_CANCELLED | RetryPolicy::RETRY_ON_GRPC_DEADLINE_EXCEEDED | - RetryPolicy::RETRY_ON_GRPC_RESOURCE_EXHAUSTED | - RetryPolicy::RETRY_ON_GRPC_UNAVAILABLE | RetryPolicy::RETRY_ON_GRPC_INTERNAL) && + RetryPolicy::RETRY_ON_GRPC_RESOURCE_EXHAUSTED | RetryPolicy::RETRY_ON_GRPC_UNAVAILABLE | + RetryPolicy::RETRY_ON_GRPC_INTERNAL) && response_headers) { absl::optional status = Grpc::Common::getGrpcStatus(*response_headers); From e9e07e492f6752c676dfc553b503d80154d607a1 Mon Sep 17 00:00:00 2001 From: Snow Pettersen Date: Mon, 8 Oct 2018 22:42:23 -0700 Subject: [PATCH 3/6] use right bitmask + fix doc nit Signed-off-by: Snow Pettersen --- docs/root/configuration/http_filters/router_filter.rst | 1 + include/envoy/router/router.h | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/root/configuration/http_filters/router_filter.rst b/docs/root/configuration/http_filters/router_filter.rst index 1b2d704157449..46b8b51c467b2 100644 --- a/docs/root/configuration/http_filters/router_filter.rst +++ b/docs/root/configuration/http_filters/router_filter.rst @@ -124,6 +124,7 @@ deadline-exceeded internal Envoy will attempt to retry if the gRPC status code in the response headers is "internal" (13) + resource-exhausted Envoy will attempt a retry if the gRPC status code in the response headers is "resource-exhausted" (8) diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index acad46189ff7a..b5a2736a8a638 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -151,7 +151,7 @@ class RetryPolicy { static const uint32_t RETRY_ON_GRPC_DEADLINE_EXCEEDED = 0x40; static const uint32_t RETRY_ON_GRPC_RESOURCE_EXHAUSTED = 0x80; static const uint32_t RETRY_ON_GRPC_UNAVAILABLE = 0x100; - static const uint32_t RETRY_ON_GRPC_INTERNAL = 0x101; + static const uint32_t RETRY_ON_GRPC_INTERNAL = 0x200; // clang-format on virtual ~RetryPolicy() {} From a0d39f3d9cd907c7b8e6ed72a86ebf79c9969b06 Mon Sep 17 00:00:00 2001 From: Snow Pettersen Date: Tue, 9 Oct 2018 18:32:24 -0400 Subject: [PATCH 4/6] add release note Signed-off-by: Snow Pettersen --- docs/root/intro/version_history.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index d68c522b1bf57..bf706619adf46 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -7,6 +7,7 @@ Version history * router: added ability to configure arbitrary :ref:`retriable status codes. ` * router: added ability to set attempt count in upstream requests, see :ref:`virtual host's include request attempt count flag `. +* router: added internal :ref:`grpc-retry-on ` policy. * router: when :ref:`max_grpc_timeout ` is set, Envoy will now add or update the grpc-timeout header to reflect Envoy's expected timeout. * stats: added :ref:`stats_matcher ` to the bootstrap config for granular control of stat instantiation. From 69a904df893b3dc1de7b70927822018eb2a1de25 Mon Sep 17 00:00:00 2001 From: Snow Pettersen Date: Tue, 9 Oct 2018 18:39:38 -0400 Subject: [PATCH 5/6] fix docs Signed-off-by: Snow Pettersen --- docs/root/intro/version_history.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index bf706619adf46..75c2d1b4cd58a 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -7,7 +7,7 @@ Version history * router: added ability to configure arbitrary :ref:`retriable status codes. ` * router: added ability to set attempt count in upstream requests, see :ref:`virtual host's include request attempt count flag `. -* router: added internal :ref:`grpc-retry-on ` policy. +* router: added internal :ref:`grpc-retry-on ` policy. * router: when :ref:`max_grpc_timeout ` is set, Envoy will now add or update the grpc-timeout header to reflect Envoy's expected timeout. * stats: added :ref:`stats_matcher ` to the bootstrap config for granular control of stat instantiation. From 3c510506e521e46f8534ea8447bcce36c519ec60 Mon Sep 17 00:00:00 2001 From: Snow Pettersen Date: Tue, 9 Oct 2018 19:21:31 -0400 Subject: [PATCH 6/6] kick ci Signed-off-by: Snow Pettersen