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

feat: Add option to prevent hitting the rate limit #450

Open
wants to merge 3 commits into
base: 4.1.0
Choose a base branch
from

Conversation

erezrokah
Copy link

@erezrokah erezrokah commented Apr 17, 2024

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

  • Bug Fix (non-breaking fixes to existing functionality)
  • New Feature (non-breaking changes that add new functionality)
  • Documentation update
  • Test Updates
  • Other (Please describe the type)

Test Information

  • My PR required test updates

Go Version:
Os Version:
OpenAPI Spec Version:

Signoff

  • I have submitted a CLA for this PR
  • Each commit message explains what the commit does
  • I have updated documentation to explain what my PR does
  • My code is covered by tests if required
  • I ran make fmt on my code
  • I did not edit any automatically generated files

Copy link

This PR has been marked stale because there has been no activity within the last 28 days. To keep this PR active, remove the stale label.

@github-actions github-actions bot added the stale label May 17, 2024
@erezrokah
Copy link
Author

Not stale

@github-actions github-actions bot removed the stale label May 18, 2024
@duytiennguyen-okta
Copy link
Contributor

@erezrokah This make sense, can you add the test for it? Thank you

@erezrokah erezrokah changed the base branch from 4.0.0 to 4.1.0 May 23, 2024 11:03
@erezrokah erezrokah changed the base branch from 4.1.0 to 4.0.0 May 23, 2024 11:03
@erezrokah erezrokah changed the base branch from 4.0.0 to 4.1.0 May 23, 2024 11:05
@erezrokah
Copy link
Author

@erezrokah This make sense, can you add the test for it? Thank you

Sure 💯 Added in 0cf4f9a

return nil, fmt.Errorf("should not have made more than 2 calls")
}
count++
return MockRateLimitedResponse(count, resetTime), nil
Copy link
Author

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

@rickb-monad
Copy link

Hi @erezrokah, I believe your test is not correctly formed.

The existing func Mock429Response() *http.Response and new MockRateLimitedResponse(requestId int, reset time.Time) *http.Response functions are duplicate, and should be merged/combined and updated as follows:

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 X-Okta-Now as was in the previous Mock429Response(), and the
go standard library time.RFC1123 format constant is correct layout for the Date header.

Per the documentation:

The HTTP status code for Rate Rimited API responses is HTTP 429 (not HTTP 200), and the HTTP response body is the JSON encoded form of the okta.Error{} struct. The Okta SDK Request Executor will return an error (which is of type okta.Error{}) via the CheckResponseForError(resp *http.Response) error function.

CheckResponseForError(resp *http.Response) error (click to expand...)

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"}]

@erezrokah
Copy link
Author

Hi @erezrokah, I believe your test is not correctly formed.

Thanks for the detailed response @rickb-monad ❤️ I'll take a look this week and update the PR

Copy link

This PR has been marked stale because there has been no activity within the last 28 days. To keep this PR active, remove the stale label.

@github-actions github-actions bot added the stale label Jul 10, 2024
@erezrokah
Copy link
Author

Not stale, will try to address the comments this week

@github-actions github-actions bot removed the stale label Jul 11, 2024
Copy link

github-actions bot commented Aug 8, 2024

This PR has been marked stale because there has been no activity within the last 28 days. To keep this PR active, remove the stale label.

@github-actions github-actions bot added the stale label Aug 8, 2024
@erezrokah
Copy link
Author

Not stale, sorry for not being able to follow up on this yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants