feat: add handling for X-RateLimit-Reset header in retry middleware…#604
feat: add handling for X-RateLimit-Reset header in retry middleware…#604robertolopezlopez wants to merge 5 commits into
X-RateLimit-Reset header in retry middleware…#604Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
robertolopezlopez
assigned
PeterSchafer,
CatalinSnyk and
robertolopezlopez and unassigned
PeterSchafer and
CatalinSnyk
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
robertolopezlopez
force-pushed
the
feat/DAT-1452
branch
from
f2ce967 to
0e1a7e3
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| if headerXRateLimitResetValue := response.Header.Get("X-RateLimit-Reset"); len(headerXRateLimitResetValue) > 0 { | ||
| fixRetryDelay = parseRetryDelay(headerXRateLimitResetValue) | ||
| } | ||
| if fixRetryDelay > maxRetryAfter { |
There was a problem hiding this comment.
Question: @robertolopezlopez did we verify if the existing maxRetryAfter is still a sensible max value for X-RateLimit-Reset?
There was a problem hiding this comment.
we probably need to find out first how long the rate limiting window lasts, I have just asked the platform team
There was a problem hiding this comment.
The max value WE put in the reset (right now) is 3600 (one hour). But this may change in the future.
This is due to the buckets that we have on most (if not all) our rate limits:
• Second
• Minute
• HourThis is all configured in the API Gateway
https://github.com/snyk/api-gateway/blob/main/helm/templates/rate-limits/api-rest.yaml
So:
- 1 hour
- This span length may change in the future
- The linked helm template does not give us any significant information, as the real values are not injected yet (there are just placeholders).
In our code: maxRetryAfter == 10 min. Comparing that to 1 hour, I think maxRetryAfter value makes sense or at least is within the retry window. Does it make sense to you? (you indeed have better insight)
|
@robertolopezlopez did you create a CLI branch for this change already? Would be great to do, so that we can get these changes tested and merged there. |
|
PR Reviewer Guide 🔍
|
| // filterRetryError strips sentinel errors used only inside the retry loop so callers receive the last HTTP response. | ||
| func (rm RetryMiddleware) filterRetryError(err error, actualAttempts int) error { | ||
| if errors.Is(err, errRetryNecessary) { | ||
| rm.logger.Warn().Msgf("Retry ultimately failed after %d attempts", actualAttempts) | ||
| finalError = nil | ||
| return nil | ||
| } | ||
|
|
||
| return finalResponse, finalError | ||
| if errors.Is(err, errRetryDelayMaxExceeded) { | ||
| rm.logger.Warn().Msg("Suggested retry delay from Retry-After or X-RateLimit-Reset exceeds maximum allowed wait; returning last HTTP response") | ||
| return nil | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
We can change this to processRetryError or something like that where we still return an error with information so we can surface it.
It seems we have some ErrorCatalog we can use
cc: @PeterSchafer
4 participants
Description
X-RateLimit-Reset response header support.
When the user runs into rate limiting issues, Envoy returns a 429 response with this header. According to envoy docs and confirmed by platform team, it has numeric int format.
Checklist
make test)make generate)make lint)go get github.com/snyk/go-application-framework@YOUR_LATEST_GAF_COMMITin thecliv2directory.go.modto point to your local GAF code.go mod tidyin thecliv2directory.go.modandgo.sumchanges.