router: add gateway-error retry-on policy#2354
router: add gateway-error retry-on policy#2354mattklein123 merged 5 commits intoenvoyproxy:masterfrom dio:issue-2306-more-granular-5xx-retry-policy
Conversation
mattklein123
left a comment
There was a problem hiding this comment.
Generally looks great, thanks. Small comment.
There was a problem hiding this comment.
You need to check here that response_headers is not nullptr. Please add a test to cover this case.
There was a problem hiding this comment.
Hi, @mattklein123 thanks for the review. Since this policy has the same behavior with 5xx, I assume I need to have reset overflow and remote reset tests in place?
There was a problem hiding this comment.
Or vice versa? Hum, it seems so. So I think, regarding this policy, I need to not retry if the response_headers is nullptr. Is that correct?
There was a problem hiding this comment.
Per your other comment, I think you actually want || here for the same reason as the 5xx case. A reset will always result in a 503 or 504 (IIRC for timeout), so should be included.
There was a problem hiding this comment.
Ah yes, sorry for misunderstanding that. Will update it.
|
@dio LGTM but needs a DCO fix. |
|
OH yeah, sorry for that. I thought I have run that Thanks @mattklein123. |
|
@dio apologies, one last thing: Can you please add a release note about the new feature here: https://github.com/envoyproxy/envoy/blob/master/RAW_RELEASE_NOTES.md. Thank you! |
Signed-off-by: Dhi Aurrahman <dio@hooq.tv>
The relevant consts are defined in envoy/router/router.h not envoy/router.h. Signed-off-by: Dhi Aurrahman <dio@hooq.tv>
Signed-off-by: Dhi Aurrahman <dio@hooq.tv>
Signed-off-by: Dhi Aurrahman <dio@hooq.tv>
Signed-off-by: Dhi Aurrahman <dio@hooq.tv>
|
@mattklein123 added. Thanks for reminding me! |
* Moved creation of client handler to onNewConnection callback Added logic for building shared attributes Shared attributes are build before the check call. Updated logic for return destination IP & Port for handling the case when upstream cluster is missing. * Fix/augment unit tests
To account for recent changes to defaults: actions/runner-images#5595 Signed-off-by: JP Simard <jp@jpsim.com>
To account for recent changes to defaults: actions/runner-images#5595 Signed-off-by: JP Simard <jp@jpsim.com>
router: add gateway-error retry-on policy
Description: Add gateway-error (consider 502, 503 and 504 only) as a subset of 5xx retry-on policy.
Risk Level: Low
Testing: unit test
Docs Changes:
Release Notes: N/A
Fixes #2300