Add the ability to retry on reset connection to service-routers #12890

aoskotsky-amplify · 2022-04-29T04:55:12Z

Description

This adds a ~~RetryOnReset~~ RetryOn option to service-router configs.

My apps are getting an error intermittently when using consul connect: upstream connect error or disconnect/reset before headers. reset reason: connection termination. Envoy has an option to automatically retry in case of this kind of error. Consul does not support this option at the moment so I'm implementing it here.

Testing & Reproduction steps

Create a service-router with the RetryOn option like below:

{
    "Kind": "service-router",
    "Name": "test",
    "Routes": [
        {
            "Match": {
                "HTTP": {
                    "PathPrefix": "/"
                }
            },
            "Destination": {
                "NumRetries": 3,
                "RetryOn": ["reset"],
                "Service": "test"
            }
        }
    ]
}

Links

Issue #10274

PR Checklist

updated test coverage
external facing docs updated
not a security concern
checklist folder consulted

hashicorp-cla · 2022-04-29T04:55:16Z

All committers have signed the CLA.

Amier3 · 2022-05-02T14:25:07Z

Hey @aoskotsky-amplify

Thanks for the first-time contribution ( hope to see many more 👀 )! Generally we expose envoy-config on a per-case basis, but on first glance this use-case seems pretty solid. Since this wasn't made off an associated github issue, i'll talk this over with the team and let you know tommorow if we'd like to expose this config to everyone.

Then hopefully we'll get this reviewed and merged shortly after 👍

Amier3 · 2022-05-02T16:52:41Z

@aoskotsky-amplify

Blake just let me know that this change came from a conversation on gitter, so you're all good to go 👍 . We'll get this reviewed shortly and thank you again for the contribution!

aoskotsky-amplify · 2022-05-02T16:56:13Z

@aoskotsky-amplify

Blake just let me know that this change came from a conversation on gitter, so you're all good to go 👍 . We'll get this reviewed shortly and thank you again for the contribution!

Awesome. Thanks!

blake · 2022-05-05T07:52:08Z

Hi @aoskotsky-amplify,

Thank you for this contribution. I had an opportunity to discuss this PR with the team. Here's the feedback from our review.

Envoy supports a number of retry conditions. At the time of writing, there are ten (10) x-envoy-retry-on conditions and five (5) x-envoy-retry-grpc-on conditions. Instead of exposing condition through their own variables–as is the current approach used in Consul and the one followed by this PR–we think a better solution is to expose retry_on as a top-level array in the service router configuration where users can specify a list of retry conditions.

Would you be willing to modify your PR to accommodate this alternative UX? If so, we can share a few UX options we are considering and get your feedback on them prior to you making any changes to the PR.

aoskotsky-amplify · 2022-05-05T12:03:33Z

Sure, I can modify the PR. What UX options are you considering?

blake · 2022-05-17T00:14:45Z

Hi @aoskotsky-amplify, here's the UX we were considering.

It would probably be beneficial to have input validation on the array to limit the retry conditions that can be specified. After looking at Envoy's docs, I believe we'd only want to allow the following conditions to be set:

5xx
gateway-error
reset
connect-failure
envoy-ratelimited
retriable-4xx
refused-stream

gRPC retry conditions

cancelled
deadline-exceeded
internal
resource-exhausted
unavailable

Special handling for certain conditions

`connect-failure`

The connect-failure condition would need some special handling since there is currently a dedicated variable for enabling that retry condition, RetryOnConnectFailure.

If RetryOnConnectFailure is set to true, and connect-failure is not already present in retryPolicy.RetryOn, the connect-failure value should be appended to the list of retry conditions.

`retriable-status-codes`

I don't believe it makes sense to allow retriable-status-codes to be set directly in the the array of retry conditions because it is dependent on retryPolicy.RetriableStatusCodes also being configured. Since Consul does not configure Envoy to allow the x-envoy-retriable-status-codes header to be used to provide this list of codes (this is an alternative method for specifying the retry codes), the only option users have is to specify the list with RetryOnStatusCodes in the service router configuration.

In this case, I think it makes sense for that condition to only be enabled if one or more status codes are specified in RetryOnStatusCodes.

`retriable-headers`

The retriable-headers is dependent on a list of retriable headers also being configured. Similar to the retriable status codes, the list of headers cannot be specified using the x-envoy-retriable-header-names HTTP header.

Being that Consul does not have a configuration option that allows configuring retryPolicy.RetriableHeaders, I do not think it makes sense to allow this retry condition to be specified in the list.

What are your thoughts on this proposed UX?

aoskotsky-amplify · 2022-05-17T02:40:26Z

Hi @blake, that UX makes sense to me. I'll start working on it.

I have a couple of questions though:

Does Consul support service-router configs with gRPC services? I see the service-router docs say services need to be HTTP. If if it does, do you think the retry conditions should be a separate config option? Would all the options be available under a RetryOn option or would there separately be a GrpcRetryOn?
Do you think in the future there should be a RetryOnHeaders option in a separate PR?

blake · 2022-05-17T19:10:04Z

Hi @aoskotsky-amplify,

Service routers do support routing to gRPC services. The beginning of the doc implies under requirements that service routers can only be used with services using the http protocol, but this is not correct. Service routers can also route based on gRPC methods. See https://www.consul.io/docs/connect/config-entries/service-router#grpc-routing for an example.

Do you think in the future there should be a RetryOnHeaders option in a separate PR?

Yes, we would like to support this feature as well. I think it makes sense to add it in a separate PR.

aoskotsky-amplify · 2022-05-17T19:15:54Z

Oh I see. Do you think there should be separate options like RetryOn and GrpcRetryOn?

github-actions · 2022-07-17T01:36:18Z

This pull request has been automatically flagged for inactivity because it has not been acted upon in the last 60 days. It will be closed if no new activity occurs in the next 30 days. Please feel free to re-open to resurrect the change if you feel this has happened by mistake. Thank you for your contributions.

Add a RetryOn option instead of RetryOnReset so that we can support the remaining envoy retry conditions

aoskotsky-amplify · 2022-08-16T05:11:10Z

@blake I updated this PR with your suggestions. Can you give this another review? Thanks

aoskotsky-amplify · 2022-09-06T14:02:05Z

Hey @blake @Amier3 can you please give this PR another review?

aoskotsky-amplify · 2022-10-03T17:14:42Z

@mkeeler could someone from Hashicorp review this PR? Thanks!

mkeeler

The code looks good to me. In addition to the one request I added a comment for I would also ask that the ServiceRouteDestination type in api/config_entry_discoverychain.go also be updated to reflect the RetryOn field (and a test case added to TestAPI_ConfigEntry_DiscoveryChain in the corresponding test file).

As for your question about whether we should have a RetryOnGRPC separate from the main RetryOn field. My opinion is that since Envoy makes no distinction in the routing policy and can do the right thing, then its best to keep it as 1 field in Consul.

mkeeler · 2022-10-03T19:17:28Z

agent/structs/config_entry_discoverychain.go

@@ -251,6 +257,26 @@ func isValidHTTPMethod(method string) bool {
 	}
 }

+func isValidRetryCondition(retryOn string) bool {


It would be good to add a test for this validation function in agent/structs/config_entry_discoverychain_test.go to enumerate all the valid retry conditions. It is obvious that it works today but if we refactor things in the future it would be great to know if we break something.

Additionally it would be great if you could add two more cases to TestServiceRouterConfigEntry in that same file. One to ensure that a router with this field configured with correct entries is accepted and one to test the validation error.

Updated. Let me know if the tests look fine now

trujillo-adam

Just a clarification to the docs

trujillo-adam · 2022-10-03T21:16:59Z

website/content/docs/connect/config-entries/service-router.mdx

+    {
+      name: 'RetryOn',
+      type: 'array<string>',
+      description: `Allows retrying requests for a list of conditions. One of:


Suggested change

description: `Allows retrying requests for a list of conditions. One of:

description: `Allows Consul to retry requests when the requests meet a specific set of conditions. You can specify one of the following lists:

Is that what this means?

Yes. How about this? Allows Consul to retry requests when they meet one of a set of conditions. The available conditions are:

That's pretty good. We can probably even trim it down a bit more and improve consistency with the style guide:

Allows Consul to retry requests when they meet one of the following sets of conditions:

mkeeler

LGTM

Add the ability to retry on reset connection to service-routers

7116a96

Update docs

a9eaa8c

aoskotsky-amplify requested a review from a team as a code owner April 29, 2022 05:02

Update integration test

cedde57

github-actions bot added the meta/stale Automatically flagged for inactivity by stalebot label Jul 17, 2022

aoskotsky-amplify and others added 4 commits August 14, 2022 20:43

Merge branch 'hashicorp:main' into add_retry_on_reset_option

2922241

Support additional envoy retry conditions

da15a79

Add a RetryOn option instead of RetryOnReset so that we can support the remaining envoy retry conditions

fix description

ebe26bb

run go fmt

945cab0

github-actions bot removed the meta/stale Automatically flagged for inactivity by stalebot label Aug 17, 2022

mkeeler requested changes Oct 3, 2022

View reviewed changes

trujillo-adam reviewed Oct 3, 2022

View reviewed changes

aoskotsky-amplify and others added 3 commits October 3, 2022 17:28

Merge branch 'hashicorp:main' into add_retry_on_reset_option

c9b5f08

Add more tests and other code review feedback

8a1a715

Remove new line

4e2a4a2

aoskotsky-amplify requested a review from mkeeler October 4, 2022 05:17

aoskotsky-amplify requested review from trujillo-adam and mkeeler and removed request for mkeeler and trujillo-adam October 4, 2022 05:17

mkeeler approved these changes Oct 4, 2022

View reviewed changes

mkeeler added pr/no-changelog PR does not need a corresponding .changelog entry backport/1.14 and removed backport/1.14 labels Oct 4, 2022

mkeeler merged commit 13da2c5 into hashicorp:main Oct 5, 2022

mkeeler mentioned this pull request Oct 5, 2022

Create 12890.txt #14895

Merged

mkeeler added a commit that referenced this pull request Oct 5, 2022

Add changelog entry for #12890

2811925

This was referenced Oct 6, 2022

Retry policy in service-router is not working as expected #10274

Closed

Consul connect provide a way to configure envoy retry_on policy #7210

Closed

Support configuring Envoy retry_policy using Consul proxy.upstream[].config #8475

Closed

ghost mentioned this pull request Oct 19, 2022

Add retries to flaky web integration tests. grapl-security/grapl#2066

Merged

david-yu mentioned this pull request Nov 13, 2023

"retryOn" configuration on ServiceRouter CRD hashicorp/consul-k8s#3207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to retry on reset connection to service-routers #12890

Add the ability to retry on reset connection to service-routers #12890

aoskotsky-amplify commented Apr 29, 2022 •

edited

Loading

hashicorp-cla commented Apr 29, 2022 •

edited

Loading

Amier3 commented May 2, 2022

Amier3 commented May 2, 2022

aoskotsky-amplify commented May 2, 2022

blake commented May 5, 2022

aoskotsky-amplify commented May 5, 2022

blake commented May 17, 2022

aoskotsky-amplify commented May 17, 2022

blake commented May 17, 2022

aoskotsky-amplify commented May 17, 2022

github-actions bot commented Jul 17, 2022

aoskotsky-amplify commented Aug 16, 2022

aoskotsky-amplify commented Sep 6, 2022 •

edited

Loading

aoskotsky-amplify commented Oct 3, 2022

mkeeler left a comment

mkeeler Oct 3, 2022

aoskotsky-amplify Oct 4, 2022

trujillo-adam left a comment

trujillo-adam Oct 3, 2022

aoskotsky-amplify Oct 3, 2022

trujillo-adam Oct 3, 2022

aoskotsky-amplify Oct 4, 2022

mkeeler left a comment

	description: `Allows retrying requests for a list of conditions. One of:
	description: `Allows Consul to retry requests when the requests meet a specific set of conditions. You can specify one of the following lists:

Add the ability to retry on reset connection to service-routers #12890

Add the ability to retry on reset connection to service-routers #12890

Conversation

aoskotsky-amplify commented Apr 29, 2022 • edited Loading

Description

Testing & Reproduction steps

Links

PR Checklist

hashicorp-cla commented Apr 29, 2022 • edited Loading

Amier3 commented May 2, 2022

Amier3 commented May 2, 2022

aoskotsky-amplify commented May 2, 2022

blake commented May 5, 2022

aoskotsky-amplify commented May 5, 2022

blake commented May 17, 2022

Special handling for certain conditions

connect-failure

retriable-status-codes

retriable-headers

aoskotsky-amplify commented May 17, 2022

blake commented May 17, 2022

aoskotsky-amplify commented May 17, 2022

github-actions bot commented Jul 17, 2022

aoskotsky-amplify commented Aug 16, 2022

aoskotsky-amplify commented Sep 6, 2022 • edited Loading

aoskotsky-amplify commented Oct 3, 2022

mkeeler left a comment

Choose a reason for hiding this comment

mkeeler Oct 3, 2022

Choose a reason for hiding this comment

aoskotsky-amplify Oct 4, 2022

Choose a reason for hiding this comment

trujillo-adam left a comment

Choose a reason for hiding this comment

trujillo-adam Oct 3, 2022

Choose a reason for hiding this comment

aoskotsky-amplify Oct 3, 2022

Choose a reason for hiding this comment

trujillo-adam Oct 3, 2022

Choose a reason for hiding this comment

aoskotsky-amplify Oct 4, 2022

Choose a reason for hiding this comment

mkeeler left a comment

Choose a reason for hiding this comment

aoskotsky-amplify commented Apr 29, 2022 •

edited

Loading

hashicorp-cla commented Apr 29, 2022 •

edited

Loading

`connect-failure`

`retriable-status-codes`

`retriable-headers`

aoskotsky-amplify commented Sep 6, 2022 •

edited

Loading