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

draft out rate limit details #3510

Merged
merged 5 commits into from
Mar 28, 2024
Merged

draft out rate limit details #3510

merged 5 commits into from
Mar 28, 2024

Conversation

christinaausley
Copy link
Contributor

Description

Closes https://github.com/camunda/developer-experience/issues/274.

When should this change go live?

  • This change is not yet live and should not be merged until {ADD_DATE} (apply hold label or convert to draft PR)?
  • There is no urgency with this change.
  • This change or page is part of a marketing blog, conference talk, or something else on a schedule.
  • This functionality is already available but undocumented.
  • This is a bug fix or security concern.

PR Checklist

  • I have added changes to the relevant /versioned_docs directory, or they are not for an already released version.
  • I have added changes to the main /docs directory (aka /next/), or they are not for future versions.
  • My changes require an Engineering review, and I've assigned an engineering manager or tech lead as a reviewer, or my changes do not require an Engineering review.
  • My changes require a technical writer review, and I've assigned @christinaausley as a reviewer, or my changes do not require a technical writer review.

@christinaausley christinaausley added component:docs Documentation improvements, including new or updated content component:console Issues related with Console project labels Mar 22, 2024
@christinaausley christinaausley self-assigned this Mar 22, 2024
@christinaausley
Copy link
Contributor Author

Will backport pending review 👍

Copy link
Contributor

github-actions bot commented Mar 22, 2024

👋 🤖 ✅ Looks like the changes were ported across versions, nice job! 🎉

You can read more about the versioning within our docs in our documentation guidelines.

Copy link

@daniel-ewing daniel-ewing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some changes. You might not like these changes. If you don't, then feel free to ignore me. For the last two lines, is there any way to indent the bullets (make a sub-bullet list):

The OAuth service has a built-in token request rate limit of about one request per second for all clients with the same source IP address.
The officially offered client libraries (as well as the NodeJS and Spring clients) have already integrated with the auth routine, handle obtaining and refreshing an access token, and make use of a local cache.
If too many token requests are executed from the same source IP address in a short time, all token request from that source IP address are blocked for a certain time. Since an access token has a 24 hour validity period, it must be: cached on the client side, reused while still valid, refreshed via a new token request once its validity period has expired.
When the rate limit gets triggered, the client will receive an HTTP 429 response. Note the following workarounds:

  • Cache the token as it is valid for 24 hours. The official SDKs already do this by default.
  • Keep the SDK up to date. We have noted issues in older versions of the Java SDK which did not correctly cache the token.
  • Given the rate limit applies to clients with the same source IP address, be mindful of:
    -- Unexpected clients running within your infrastructure.
    -- Updating all clients to use a current API key if you delete an API key and create a new one.

thomas-polnik
thomas-polnik previously approved these changes Mar 25, 2024
@christinaausley
Copy link
Contributor Author

@daniel-ewing Adjusted based on your comment 👍

Copy link
Member

@multani multani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's also worth mentioning that all the requests are rate-limited, whether the request was successful or not.

It's especially important wrt. the last point you mentioned ("updating clients to use a new API key"): if a client can't authenticate because the credentials are expired and it "loops" trying to fetch a new token, its requests will count towards the rate limit.

I'm not sure what would be the best place to write this: maybe in the 3rd paragraph "if too many token requests are executed from the same source IP address in a short time"?

docs/apis-tools/administration-api/authentication.md Outdated Show resolved Hide resolved
@multani
Copy link
Member

multani commented Mar 26, 2024

I think it's also worth mentioning that all the requests are rate-limited, whether the request was successful or not.

It's especially important wrt. the last point you mentioned ("updating clients to use a new API key"): if a client can't authenticate because the credentials are expired and it "loops" trying to fetch a new token, its requests will count towards the rate limit.

@christinaausley do you have an idea also how to phrase this? This could also help support by pointing out to customers that problems on their side can affect themselves.

@daniel-ewing
Copy link

Hi @christinaausley, following up on @multani's comment, does anyone like this:

The OAuth service has a built-in token request rate limit of about one request per second for all clients with the same source IP address.

I don't know the markdown syntax for a warning box - make this part a warning:
All token requests count toward the rate limit, whether they are successful or not. If any client is running with an expired or invalid API key, that client will continually make token requests. That client will cause the rate limit to be exceeded for that IP address and it can block valid token requests from completing.

The officially offered ...

@christinaausley
Copy link
Contributor Author

@daniel-ewing @multani thank you for your help here! Added a note 👍

@christinaausley christinaausley requested a review from multani March 27, 2024 18:09
Copy link
Member

@multani multani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me, thanks @christinaausley and @daniel-ewing !
@daniel-ewing do you think this will help Support team to redirect relevant support tickets to in the future?

@daniel-ewing
Copy link

@multani, I hope so! But if not, we tried...

@christinaausley christinaausley merged commit 7bc0f7f into main Mar 28, 2024
7 checks passed
@christinaausley christinaausley deleted the 274-clarify-rate-limit branch March 28, 2024 13:26
theburi pushed a commit that referenced this pull request Apr 3, 2024
* draft out rate limit details

* address comment

* backport

* remove built in

* add note
theburi pushed a commit that referenced this pull request Jun 5, 2024
* draft out rate limit details

* address comment

* backport

* remove built in

* add note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:console Issues related with Console project component:docs Documentation improvements, including new or updated content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants