-
Notifications
You must be signed in to change notification settings - Fork 149
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
feat: Add option to prevent hitting the rate limit #450
base: 4.1.0
Are you sure you want to change the base?
Conversation
513d267
to
efc1da9
Compare
#### Summary This uses our forked version with the PR from okta/okta-sdk-golang#450 (until approved) to prevent hitting Okta rate limits and blocking until the limit has reset (or context was cancelled). This follows Okta's best practices https://developer.okta.com/docs/reference/rl-best-practices/#check-your-rate-limits-with-okta-s-rate-limit-headers
This PR has been marked stale because there has been no activity within the last 28 days. To keep this PR active, remove the |
Not stale |
@erezrokah This make sense, can you add the test for it? Thank you |
65c3d99
to
10ff630
Compare
Sure 💯 Added in 0cf4f9a |
return nil, fmt.Errorf("should not have made more than 2 calls") | ||
} | ||
count++ | ||
return MockRateLimitedResponse(count, resetTime), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't use the existing MockResponse
function as I want the Date
header to be created after the request was sent so I can check it comes after the reset time
Hi @erezrokah, I believe your test is not correctly formed. The existing func Mock429RateLimitedResponse() *http.Response {
zulu := time.Now().UTC()
header := http.Header{}
header.Add("Date", zulu.Format(time.RFC1123))
header.Add("Content-Type", "application/json")
header.Add("X-Okta-Request-id", "a-request-id")
header.Add("X-Rate-Limit-Limit", strconv.Itoa(50))
header.Add("X-Rate-Limit-Remaining", strconv.Itoa(0))
header.Add("X-Rate-Limit-Reset", strconv.Itoa(int(zulu.Add(time.Second).Unix())))
errBody := `{"errorCode":"E0000047",`+
`"errorSummary":"API call exceeded rate limit due to too many requests.",`+
`"errorLink":E0000047,`+
`"errorId":"sampleVWFhy6K7rS9-IWcjsk_",`+
`"errorCauses":[]}`
return &http.Response{
Status: http.StatusText(http.StatusTooManyRequests),
StatusCode: http.StatusTooManyRequests,
Body: httpmock.NewRespBodyFromString(errBody),
Header: header,
ContentLength: int64(len(errBody)),
}
} Note Okta does not include an HTTP response header of Per the documentation: The HTTP status code for Rate Rimited API responses is
|
func CheckResponseForError(resp *http.Response) error { | |
statusCode := resp.StatusCode | |
if statusCode >= http.StatusOK && statusCode < http.StatusBadRequest { | |
return nil | |
} | |
e := Error{} | |
if (statusCode == http.StatusUnauthorized || statusCode == http.StatusForbidden) && | |
strings.Contains(resp.Header.Get("Www-Authenticate"), "Bearer") { | |
for _, v := range strings.Split(resp.Header.Get("Www-Authenticate"), ", ") { | |
if strings.Contains(v, "error_description") { | |
_, err := toml.Decode(v, &e) | |
if err != nil { | |
e.ErrorSummary = "unauthorized" | |
} | |
return &e | |
} | |
} | |
} | |
bodyBytes, err := io.ReadAll(resp.Body) | |
if err != nil { | |
return err | |
} | |
copyBodyBytes := make([]byte, len(bodyBytes)) | |
copy(copyBodyBytes, bodyBytes) | |
_ = resp.Body.Close() | |
resp.Body = io.NopCloser(bytes.NewBuffer(bodyBytes)) | |
_ = json.NewDecoder(bytes.NewReader(copyBodyBytes)).Decode(&e) | |
if statusCode == http.StatusInternalServerError { | |
e.ErrorSummary += fmt.Sprintf(", x-okta-request-id=%s", resp.Header.Get("x-okta-request-id")) | |
} | |
return &e | |
} |
The CheckResponseForError()
function effectively decodes the Error JSON response into and returns an okta.Error{}
struct, with a few specific and minor exceptions.
okta.Error{}
(click to expand...)
https://github.com/okta/okta-sdk-golang/blob/master/okta/error.go#L24-L32
So, when the HTTP Response Header includes X-Rate-Limit-Remaining: 0
, the HTTP Status Code must also be HTTP 429
, and the response body will be one of the example HTTP 429 error responses, which for most APIs will be E0000047
: Too many requests exception:
The E0000047
example verbatim (click to expand...)
HTTP/1.1 429 Too Many Requests
Content-Type: application/json
{
"errorCode": "E0000047",
"errorSummary": "API call exceeded rate limit due to too many requests.",
"errorLink": E0000047,
"errorId": "sampleVWFhy6K7rS9-IWcjsk_",
"errorCauses": []
}
A more accurate example of E0000047
, including the appropriate headers (click to expand...)
summary>
HTTP/1.1 429 Too Many Requests
Content-Type: "application/json"
X-Rate-Limit-Limit: 20
X-Rate-Limit-Remaining: 0
X-Rate-Limit-Reset: 1717958209
Content-Length: 204
{
"errorCode": "E0000047",
"errorSummary": "API call exceeded rate limit due to too many requests.",
"errorLink": E0000047,
"errorId": "sampleVWFhy6K7rS9-IWcjsk_",
"errorCauses": []
}
An example of a non-rate limited (HTTP 200
) response (click to expand...)
HTTP/1.1 200 OK
Content-Type: "application/json"
X-Rate-Limit-Limit: 20
X-Rate-Limit-Remaining: 19
X-Rate-Limit-Reset: 1717958209
Content-Length: 46
[{"id": "example-id-1"},{"id":"example-id-2"}]
Thanks for the detailed response @rickb-monad ❤️ I'll take a look this week and update the PR |
This PR has been marked stale because there has been no activity within the last 28 days. To keep this PR active, remove the |
Not stale, will try to address the comments this week |
This PR has been marked stale because there has been no activity within the last 28 days. To keep this PR active, remove the |
Not stale, sorry for not being able to follow up on this yet |
Summary
Mostly opening for feedback, happy to add tests and docs if this approach makes sense.
This PR sleeps until the rate limit reset time in case the next request will hit the limit.
This should prevent sending requests that are known to cause 429 errors.
I updated the auto generated code for simplicity but will update the generation templates if this change makes sense.
Fixes #434
Type of PR
Test Information
Go Version:
Os Version:
OpenAPI Spec Version:
Signoff
make fmt
on my code