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

poc: bucket state as consumed tokens instead of remaining tokens #60

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LeweyM
Copy link

@LeweyM LeweyM commented Apr 12, 2024

Description

Context

Currently, we use Redis to store a count of the remaining tokens in a bucket. This is effective and conceptually simple.
For example, using the following configuration:

size: 3

The request would return remaining:2, the second call remaining:1 and the 3rd remaining:0. Internally, when a key does not exist we consider this to be a 'full' bucket, so with a value of 3 in this example, and then we store the remaining tokens at the key for subsequent calls.

The 'drip' functionality represents tokens being added over time to the bucket. Based on the refill rate and the time of the last call, we can say that X tokens would have been added to the token. This means that we add those tokens to the bucket when we calculate the drip.

Problem

When introducing ERL, we must also consider the previous states of the bucket. This is because we need to 'top up' the bucket when ERL is activated.

Consider the following example:

size: 1
erl_size: 3
  • 1st request
    • the call is conformant and the bucket goes down to 0 internally
  • 2nd request
    • the normal bucket size is exceeded, so we need start using the new ERL size.
    • the user has only made 2 calls, but the internal value is still 0.
    • In order to make new calls to be able to make use of erl_size: 3, we have to 'top up' the bucket internally or it will stay at 0 and will be considered non-conformant.

The issue is: How many tokens should we 'top up'?

In the above example, we should 'top up' by 2. This is because we know the user has just consumed their normal size configuration, in this example 1. So, we should provide the user with the erl_size - size so they can take full advantage of ERL.

However, consider that a user has consumed all their erl_size requests, and their activation period expires. In the next request, we know that the user has just consumed their erl_size configuration, so we should not 'top up' any extra tokens.

If we we topped up using the first method, then a user would be able to consume all their erl_size requests, and then would be allowed to consume erl_size + (erl_size - size) requests directly afterwards. In fact, this is what we currently allow, and has lead to the following spiky patterns in permitted traffic;
image

The underlying issue is as follows: With our current implementation, we cannot 'top up' a bucket because we do not know the previous bucket consumption at the point of rate limiting. All we know if that there are 0 tokens remaining from either an ERL or a normally configured bucket.

Proposed Solution

Somehow, we need to keep this information.

The solution proposed in this PR is to invert the information stored in Redis. Instead of counting remaining tokens, it would count consumed tokens.

This change means a few things;

  • checking conformant is now different for ERL and normal modes.
    • previously, conformant = bucket > 0
    • now, normal mode is conformant = bucket < size
    • and for erl mode conformant = bucket < erl_size
  • when activating ERL, no 'top up' is needed.
  • the 'drip' now removes consumed tokens, instead of adding to remaining tokens

conceptually, this is not as simple as just counting the number of remaining tokens in a bucket. However, it has the advantage that when a rate limit is exceeded, the information is retained for how many tokens were used to exceed the limit. This is irrelevant when a bucket is a fixed size, but useful for our current purposes of variable sized buckets.

References

Include any links supporting this change such as a:

  • GitHub Issue/PR number addressed or fixed
  • Auth0 Community post
  • StackOverflow post
  • Support forum thread
  • Related pull requests/issues from other repos

If there are no references, simply delete this section.

Testing

  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not the default branch

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

Successfully merging this pull request may close these issues.

1 participant