NET-5115 Add retry + timeout filters for api-gateway#18324
NET-5115 Add retry + timeout filters for api-gateway#18324sarahalsmiller merged 2 commits intomainfrom
Conversation
github-actions
bot
added
theme/api
sarahalsmiller
changed the title
agent/structs/config_entry_routes.go
Outdated
There was a problem hiding this comment.
minor: do we want postfix these names with Filter to make it clear they're filters on a route or do you think that's too verbose/not really necessary?
There was a problem hiding this comment.
The reason I didn't is because the filter types on the gateway spec didn't have the filter suffix. If its something we agree we should add I don't have strong feelings about it.
There was a problem hiding this comment.
I'm in favor of the Filter postfix, looking at the naming conventions in the gateway-api repo over here for inspiration
There was a problem hiding this comment.
I'm in favor of the Filter suffix as well, and I think we can either drop HTTPRoute at the beginning in its entirety or just make it HTTP so either something like:
HTTPTimeoutFilterorTimeoutFilter(if we want to re-use this on say GRPC protocols in the future)
There was a problem hiding this comment.
We landed on HTTPRoute prefix for the corresponding CRD over in consul-k8s so that the names are self-documenting. Things here in the consul repo don't have to match since we already have some filter naming conventions without the prefix.
There was a problem hiding this comment.
Changed it to TimeoutFilter and RetryFilter, and the CRD work will be named HTTPRouteTimeoutFilter and HTTPRouteRetryFilter 👍
sarahalsmiller
force-pushed
the
NET-4889-consul-initial-work
branch
from
7a2e068 to
2329e8b
Compare
nathancoleman
left a comment
nathancoleman
left a comment
There was a problem hiding this comment.
Nice work! Just sharing some thoughts here
sarahalsmiller
force-pushed
the
NET-4889-consul-initial-work
branch
4 times, most recently
from
8a996c1 to
cf2bf7f
Compare
sarahalsmiller
force-pushed
the
NET-4889-consul-initial-work
branch
from
b4783b4 to
17bd17b
Compare
nathancoleman
changed the title
| NumRetries *uint32 | ||
| RetryOn []string | ||
| RetryOnStatusCodes []uint32 | ||
| RetryOnConnectFailure *bool |
There was a problem hiding this comment.
I suspect we can actually remove RetryOnConnectFailure based on this code + comment pointing out that there are two ways of getting "connect-failure" added to the RetryOn list. The standalone field is referred to as "legacy".
I'm fine leaving for now since our code is already built to handle both, but I'd like to discuss with the team prior to release whether we want to drop the "legacy" option from our API surface.
Description
Initial updates to allow the API Gateway from the consul core side to allow API Gateway HTTPRoutes to have configurable retries and timeouts. There will be follow up work in consul-k8s.
Expecting some feedback/updates on the name scheme, but this PR should contain everything it needs to make this functional on the consul side.
Testing & Reproduction steps
Links
https://hashicorp.atlassian.net/browse/NET-5115
PR Checklist