router: allowing request-header-based retry checks #8449
Conversation
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]>
| @@ -931,10 +931,13 @@ message RetryPolicy { | |||
| // describes Envoy's back-off algorithm. | |||
| RetryBackOff retry_back_off = 8; | |||
|
|
|||
| // HTTP headers that trigger a retry if present in the response. A retry will be | |||
| // 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. | |||
| repeated HeaderMatcher retriable_headers = 9; | |||
There was a problem hiding this comment.
in retrospect this probably should have been called retriable_response_headers :)
There was a problem hiding this comment.
@htuch shall we handle field rename like this in protoxform when upgrade to next-major-version?
There was a problem hiding this comment.
Yeah we should add this capability as an annotation or comment.
There was a problem hiding this comment.
Yes, I will be adding annotations for this later. For now, you can use [#next-major-version:] or file an explicit GitHub issue and put it in the v3 API label.
source/common/router/config_impl.cc
Outdated
| @@ -69,6 +78,7 @@ RetryPolicyImpl::RetryPolicyImpl(const envoy::api::v2::route::RetryPolicy& retry | |||
| ProtobufMessage::ValidationVisitor& validation_visitor) | |||
| : retriable_headers_( | |||
| Http::HeaderUtility::buildHeaderMatcherVector(retry_policy.retriable_headers())), | |||
| retriable_request_headers_(optionalHeaderMatcher(retry_policy.retriable_request_headers())), | |||
There was a problem hiding this comment.
should retriable_headers_ also be optional and use the optionalHeaderMatcher helper? is there a reason for these two to be different?
source/common/router/config_impl.h
Outdated
| @@ -260,6 +264,7 @@ class RetryPolicyImpl : public RetryPolicy { | |||
| uint32_t host_selection_attempts_{1}; | |||
| std::vector<uint32_t> retriable_status_codes_; | |||
| std::vector<Http::HeaderMatcherSharedPtr> retriable_headers_; | |||
| absl::optional<std::vector<Http::HeaderMatcherSharedPtr>> retriable_request_headers_; | |||
There was a problem hiding this comment.
if having it be optional a memory optimization? iirc making it optional doesn't decrease its size: https://github.com/abseil/abseil-cpp/blob/master/absl/types/optional.h#L91
There was a problem hiding this comment.
Yeah, I assumed naively that it saved memory. TIL.
I do worry about the memory impact of feature creep on the retry state, but given it's include not apis we can always switch to pointers (which definitely don't take as much space) at some point in the future, so I'll stick with the types we're already using for now.
| Http::TestHeaderMapImpl request_headers{{"x-envoy-retry-on", "5xx"}}; | ||
| setup(request_headers); | ||
| EXPECT_FALSE(state_->enabled()); | ||
| EXPECT_EQ(RetryStatus::No, state_->shouldRetryReset(overflow_reset_, callback_)); |
There was a problem hiding this comment.
probably not needed to check this?
Signed-off-by: Alyssa Wilk <[email protected]>
alyssawilk
added
the
api-review-required
| @@ -931,10 +931,13 @@ message RetryPolicy { | |||
| // describes Envoy's back-off algorithm. | |||
| RetryBackOff retry_back_off = 8; | |||
|
|
|||
| // HTTP headers that trigger a retry if present in the response. A retry will be | |||
| // 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. | |||
| repeated HeaderMatcher retriable_headers = 9; | |||
There was a problem hiding this comment.
Yeah we should add this capability as an annotation or comment.
| absl::optional<std::chrono::milliseconds> baseInterval() const override { | ||
| return absl::nullopt; | ||
| } | ||
| absl::optional<std::chrono::milliseconds> maxInterval() const override { return absl::nullopt; } | ||
|
|
||
| const std::vector<uint32_t> retriable_status_codes_{}; | ||
| const std::vector<Http::HeaderMatcherSharedPtr> retriable_headers_{}; | ||
| const std::vector<Http::HeaderMatcherSharedPtr> retriable_request_headers_{}; |
There was a problem hiding this comment.
super nit: The {} is not needed on any of these.
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]>
Risk Level: Low (new code is config guarded)
Testing: new unit tests
Docs Changes: n/a
Release Notes: yes