Add retriable headers retry policy. by olegshaldybin · Pull Request #8187 · envoyproxy/envoy

olegshaldybin · 2019-09-09T21:51:54Z

Signed-off-by: Oleg Shaldibin olegsh@google.com

Description: implements the retriable headers retry policy as described in #8185.
Risk Level: low.
Testing: unit tests, manual testing.
Docs Changes: TBD.
Release Notes: new 'retriable-headers' retry policy.
Fixes #8185.

junr03 · 2019-09-10T20:53:54Z

@zuercher do you mind taking a look at this?

zuercher

Nice! Some minor comments, but otherwise looks good.

I believe our current policy is that this needs to be reviewed by @envoyproxy/api-shepherds as well.

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

test/common/router/retry_state_impl_test.cc

include/envoy/router/router.h

repokitteh-read-only · 2019-09-11T20:00:46Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8187 was synchronize by olegshaldibin.

see: more, trace.

zuercher · 2019-09-11T21:29:00Z

test/common/router/retry_state_impl_test.cc

The spell checker tagged "retrieable". The other test failure looks like a flake.

Also, we prefer not to have force-pushed changes since it can mess up github's comment tracking. We'll squash the commits on merge so you don't have to worry about keeping the history clean.

Ah, sorry about that: I'll have to force-push one more time b/c I already rebased my branch on upstream master. It shouldn't change the content of existing commits though, as I added new changes as separate commits.

mattklein123

Overall LGTM but from an API perspective I would like to standardize on using HeaderMatcher for something like this. Thank you!

/wait

mattklein123 · 2019-09-11T21:32:52Z

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

Can we future proof this by using a repeated HeaderMatcher and related utility functions for this?

Note this change will require release notes.

Ah, didn't know about HeaderMatcher, seems like a great fit for this, thanks Matt!

Do you have any preference on how to handle retriable headers provided by the request headers? Some possibilities include:

support a comma-separated list of header names and construct key-only HeaderMatcher's for them;

disallow specifying them via request headers at all;

provide some serialization mechanism for HeaderMatcher.

I like options 1 and 2, as 3 seems too difficult for a user to craft but don't really have strong feelings about that.

Yeah definitely not (3) IMO. I would be fine with just doing (2) for now and we can see if anyone asks for header specification later / add a TODO (assuming you don't need this).

Now using HeaderMatcher, PTAL. I kept the header-specified header names but renamed the header to 'x-envoy-retriable-header-names' to make sure it's explicit. We may have a use case for specifying headers that way; at the same time I am not opposed to just removing that if it seems inconsistent.

If we're happy with the change I'll add a release note and docs.

Configured via 'retriable-headers' retry policy and 'retriable_headers' list of headers (both can be set via config or request headers) . If the upstream response has any of the retriable headers set, retry will be triggered. Signed-off-by: Oleg Shaldibin <olegsh@google.com>

Signed-off-by: Oleg Shaldibin <olegsh@google.com>

olegshaldybin · 2019-09-12T21:32:45Z

/retest

repokitteh-read-only · 2019-09-12T21:32:48Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #8187 (comment) was created by @olegshaldibin.

see: more, trace.

mattklein123

API LGTM. Will defer to @zuercher for further review. Thank you!

zuercher

Code looks good!

I should have noticed this before, but we need to update docs/root/configuration/http/http_filters/router_filter.rst.

There should be an entry for retriable-headers under x-envoy-retry-on and an entry for the x-envoy-retriable-headers header that describes the header presence logic and points to the retry config for users that want more complex matching.

source/common/http/header_utility.h

Signed-off-by: Oleg Shaldibin <olegsh@google.com>

olegshaldybin · 2019-09-16T19:35:11Z

Added docs and a release note.

zuercher

Thanks!

olegshaldybin · 2019-09-16T20:48:40Z

@mattklein123 I also need a re-approval for API (no code changes, just follow-up docs commits).

mattklein123

Thank you!

Configured via 'retriable-headers' retry policy and 'retriable_headers' list of headers (both can be set via config or request headers) . If the upstream response has any of the retriable headers set, retry will be triggered. Signed-off-by: Oleg Shaldibin <olegsh@google.com>

olegshaldybin mentioned this pull request Sep 9, 2019

Retry policy based on response headers #8185

Closed

junr03 requested a review from zuercher September 10, 2019 20:54

junr03 assigned zuercher Sep 11, 2019

zuercher reviewed Sep 11, 2019

View reviewed changes

api/envoy/api/v2/route/route.proto Outdated Show resolved Hide resolved

test/common/router/retry_state_impl_test.cc Outdated Show resolved Hide resolved

include/envoy/router/router.h Outdated Show resolved Hide resolved

zuercher added the api-review-required API review required by @envoyproxy/api-shepherds label Sep 11, 2019

olegshaldybin force-pushed the retriable-headers branch from b9de585 to 64f4d5c Compare September 11, 2019 20:00

olegshaldybin force-pushed the retriable-headers branch from 64f4d5c to b6de9b8 Compare September 11, 2019 20:05

mattklein123 self-assigned this Sep 11, 2019

zuercher reviewed Sep 11, 2019

View reviewed changes

mattklein123 requested changes Sep 11, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 11, 2019

olegshaldybin added 3 commits September 12, 2019 11:13

Use HeaderMatcher in retriable headers retry policy configuration

cbd725a

Signed-off-by: Oleg Shaldibin <olegsh@google.com>

Rename 'x-envoy-retriable-headers' to 'x-envoy-retriable-header-names'

efed86e

Signed-off-by: Oleg Shaldibin <olegsh@google.com>

olegshaldybin force-pushed the retriable-headers branch from b6de9b8 to efed86e Compare September 12, 2019 18:50

repokitteh-read-only bot removed the waiting label Sep 12, 2019

Fix formatting

1bcc487

Signed-off-by: Oleg Shaldibin <olegsh@google.com>

olegshaldybin requested a review from mattklein123 September 12, 2019 19:05

mattklein123 previously approved these changes Sep 13, 2019

View reviewed changes

mattklein123 removed their assignment Sep 13, 2019

olegshaldybin requested a review from zuercher September 14, 2019 01:08

zuercher reviewed Sep 16, 2019

View reviewed changes

source/common/http/header_utility.h Show resolved Hide resolved

olegshaldybin added 2 commits September 16, 2019 10:54

Fix a method comment

cf117b9

Signed-off-by: Oleg Shaldibin <olegsh@google.com>

Add docs and a release note.

a085b1f

Signed-off-by: Oleg Shaldibin <olegsh@google.com>

olegshaldybin dismissed mattklein123’s stale review via a085b1f September 16, 2019 19:34

zuercher approved these changes Sep 16, 2019

View reviewed changes

olegshaldybin requested a review from mattklein123 September 16, 2019 20:48

mattklein123 approved these changes Sep 17, 2019

View reviewed changes

mattklein123 merged commit 81460d8 into envoyproxy:master Sep 17, 2019

Conversation

olegshaldybin commented Sep 9, 2019

Uh oh!

junr03 commented Sep 10, 2019

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

repokitteh-read-only bot commented Sep 11, 2019

Uh oh!

zuercher Sep 11, 2019

Choose a reason for hiding this comment

Uh oh!

olegshaldybin Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Sep 11, 2019

Choose a reason for hiding this comment

Uh oh!

olegshaldybin Sep 11, 2019

Choose a reason for hiding this comment

Uh oh!

olegshaldybin Sep 11, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

olegshaldybin Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

olegshaldybin commented Sep 12, 2019

Uh oh!

repokitteh-read-only bot commented Sep 12, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

olegshaldybin commented Sep 16, 2019

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

olegshaldybin commented Sep 16, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants