router: add support for retry options predicate extensions#18058
router: add support for retry options predicate extensions#18058mattklein123 merged 14 commits intomainfrom
Conversation
This was built for Envoy Mobile, but will allow generic modification of router behavior between retries. Currently it only supports modifying upstream socket options (to in practice impact interface binding), but in the future is likely to be extended to modify timeouts, retry back off times, request headers, etc. Signed-off-by: Matt Klein <mklein@lyft.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
Looks good!
Left an API comment, and a small nit.
Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
|
/assign-from @envoyproxy/senior-maintainers |
|
@envoyproxy/senior-maintainers assignee is @ggreenway |
Signed-off-by: Matt Klein <mklein@lyft.com>
ggreenway
left a comment
ggreenway
left a comment
There was a problem hiding this comment.
Looks good overall. Think it's worth adding an integration test to make sure option updates really work?
/wait
| new Http::TestResponseHeaderMapImpl{{":status", "503"}}); | ||
| EXPECT_CALL(cm_.thread_local_cluster_.conn_pool_.host_->outlier_detector_, | ||
| putHttpResponseCode(503)); | ||
| // NOLINTNEXTLINE(clang-analyzer-core.CallAndMessage) |
There was a problem hiding this comment.
Why is this needed? This looks like a bunch of similar code throughout the file.
There was a problem hiding this comment.
I really have no idea, but tidy was flagging this line, so I put in this exclusion.
I thought about this, but I'm not quite sure how to do this since right now it only supports setting socket options. Can you think of any socket option that I could set that would somehow be obvious in an integration test? |
Could set a no-op option, and verify that it at least makes it use a new connpool. I don't know that there's a good one to directly verify a socket option on the remote end. |
Do you know offhand a good no-op option? I guess I could set some TCP keep alive settings or something. |
|
Yeah, keepalive is a good choice. Widely available, and won't do anything bad/harmful in the test. |
Signed-off-by: Matt Klein <mklein@lyft.com>
|
Thanks for the nudge, it turns out that we are not setting hashKey() on most socket options, making this feature not actually work in practice. :) cc @goaway |
Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
3 participants
This was built for Envoy Mobile, but will allow generic modification
of router behavior between retries. Currently it only supports modifying
upstream socket options (to in practice impact interface binding), but
in the future is likely to be extended to modify timeouts, retry back
off times, request headers, etc.
Risk Level: Low. New feature.
Testing: Unit tests.
Docs Changes: N/A
Release Notes: Added.
Platform Specific Features: N/A