Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

router: add API to retry reset before request #35074

Merged
merged 10 commits into from
Jul 12, 2024

Conversation

keithmattix
Copy link
Contributor

Fixes #10007
Commit Message: Add new reset-before-request retry API that will only retry reset connections if the headers haven't been delivered.
Additional Description:
Risk Level: Low
Testing: Unit and e2e
Docs Changes: Add new reset-before-request
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

When proxying to upstreams that may be performing non-idempotent
operations, it may be desirable to only retry connection rests when the
upstream has not yet seen the request. We add the reset-before-request
option to provide this functionality.

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix keithmattix changed the title Router/retry reset before request router: add API to retry reset before request Jul 6, 2024
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice feature. Mind calling it out in release notes (changelogs/current.rst) as well?
/wait

source/common/router/router.cc Outdated Show resolved Hide resolved
source/common/router/upstream_request.cc Outdated Show resolved Hide resolved
test/integration/http_integration.cc Show resolved Hide resolved
test/integration/http_integration.cc Show resolved Hide resolved
Instead of using a bool + callback approach in the router, when the
reset-before-request retry policy is set, we check the upstream timings
to see if the first upstream byte has been transferred.

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix
Copy link
Contributor Author

Nice feature. Mind calling it out in release notes (changelogs/current.rst) as well? /wait

Thanks for the feedback! Release notes are added and refactoring is complete

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix keithmattix requested a review from alyssawilk July 9, 2024 19:38
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this looks much cleaner, thanks for iterating!

/wait

envoy/router/router_filter_interface.h Outdated Show resolved Hide resolved
source/common/router/router.cc Outdated Show resolved Hide resolved
source/common/router/router.cc Outdated Show resolved Hide resolved
source/common/router/router.cc Show resolved Hide resolved
source/common/router/retry_state_impl.cc Outdated Show resolved Hide resolved
source/common/router/upstream_codec_filter.cc Outdated Show resolved Hide resolved
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix
Copy link
Contributor Author

/retest

@keithmattix keithmattix requested a review from alyssawilk July 10, 2024 18:39
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. LGTM overall except only one comment.

source/common/router/router.cc Outdated Show resolved Hide resolved
Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@keithmattix keithmattix requested a review from wbpcode July 12, 2024 03:24
@wbpcode wbpcode merged commit c60d185 into envoyproxy:main Jul 12, 2024
51 checks passed
@keithmattix keithmattix deleted the router/retry-reset-before-request branch July 17, 2024 14:42
keithmattix added a commit to istio/community that referenced this pull request Jul 18, 2024
Istio has very few maintainers here and though I'm still learning, I've contributed a handful of upstream envoy PRs as well as 2 recent changes to istio-proxy's telemetry. If existing maintainers have other areas they'd like to see more contributions in, I welcome the feedback!

PRs:
- envoyproxy/envoy#35074
- envoyproxy/envoy#33857
- envoyproxy/envoy#33362
- envoyproxy/envoy#32961
- istio/proxy#5617
- istio/proxy#5514
istio-testing pushed a commit to istio/community that referenced this pull request Jul 19, 2024
Istio has very few maintainers here and though I'm still learning, I've contributed a handful of upstream envoy PRs as well as 2 recent changes to istio-proxy's telemetry. If existing maintainers have other areas they'd like to see more contributions in, I welcome the feedback!

PRs:
- envoyproxy/envoy#35074
- envoyproxy/envoy#33857
- envoyproxy/envoy#33362
- envoyproxy/envoy#32961
- istio/proxy#5617
- istio/proxy#5514
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

safe retry on reset
3 participants