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

[RFC] Introduce a RateLimiter component #37546

Merged
merged 1 commit into from
Sep 16, 2020
Merged

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Jul 9, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Refs #37444
License MIT
Doc PR tbd

Based on the discussions in #37444, I decided to write a general purpose RateLimiter component. This implementation uses the token bucket algorithm, inspired by the Go's time/rate library and the PHP bandwidth-throttle/token-bucket package (which is unmaintained for years).

Usage

The component has two main methods:

  • Limiter::reserve(int $tokens, int $maxTime), allocates $tokens and returns a Reservation containing the wait time. Use this method if your process wants to wait before consuming the token.
  • Limiter::consume(int $tokens), checks if $tokens are available now and discards the reservation if that's not the case. Use this method if you want to skip when there are not enough tokens at this moment.

The component uses the Lock component to make sure it can be used in parallel processes.

Example:

<?php

namespace App\EventListener;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\RateLimiter\LimiterFactory;

class LimitListener
{
    private $limiterFactory;

    public function __construct(LimiterFactory $apiLimiterFactory)
    {
        $this->limiterFactory = $apiLimiterFactory;
    }

    public function __invoke(RequestEvent $event)
    {
        $ip = $event->getRequest()->getClientIp();

        $limiter = $this->limiterFactory->createLimiter(preg_replace('/[^a-zA-Z0-9]/', '-', $ip));
        if (!$limiter->consume()) {
            $event->setResponse(new Response('Too many requests', 429));
        }
    }
}

Usefullness of the component

I think a generic rate limiter is usefull in quite some places:

State of the art

There are some rate limiting packages in PHP, but I think there is no precendent for this component:

  • graham-campbell/throttle is heavily relying on Laravel. It is however very popular, proofing there is a need for such feature
  • nikolaposa/rate-limit does not implement reservation of tokens and as such less feature complete. Also its architecture combines the rate limiter and storage, making it harder to implement different storages.

Todo

If it is agreed that this component can bring something to Symfony, it needs some more love:

  • Add more tests
  • Integrate with the FrameworkBundle
  • Add sliding window implementation
  • Add integration with the Security component
  • Maybe add more storage implementations? I didn't want to duplicate storage functionalities already existing in the Lock and Cache component, thus I for now focused mostly on integrating the Cache adapters. But maybe a special Doctrine adapter makes sense?

@carsonbot carsonbot added Status: Needs Review RFC RFC = Request For Comments (proposals about features that you want to be discussed) Feature labels Jul 9, 2020
@fabpot
Copy link
Member

fabpot commented Jul 10, 2020

Thanks for working on this. I agree that having such a component in core would be wonderful.

src/Symfony/Component/RateLimiter/LimiterFactory.php Outdated Show resolved Hide resolved
src/Symfony/Component/RateLimiter/TokenBucket.php Outdated Show resolved Hide resolved
src/Symfony/Component/RateLimiter/TokenBucket.php Outdated Show resolved Hide resolved
src/Symfony/Component/RateLimiter/Rate.php Outdated Show resolved Hide resolved
src/Symfony/Component/RateLimiter/Rate.php Outdated Show resolved Hide resolved
@stof
Copy link
Member

stof commented Jul 10, 2020

* Rate limiting outgoing API calls (e.g. HttpClient), to prevent hitting upstream API limits.

Based on the naming of https://schneems.com/2020/06/25/rate-limiting-rate-throttling-and-how-they-work-together/, what HttpClient would need is a rate throttling implementation, not a rate limiting one (which does not exactly work the same).
Heroku published a blog post recently about their rate throttling algorithm in their SDK btw: https://blog.heroku.com/rate-throttle-api-client

Are you sure that the same component can be used for both ?

@wouterj
Copy link
Member Author

wouterj commented Jul 10, 2020

Are you sure that the same component can be used for both ?

Yes, I think so, that's where the two different methods come into play. The example shown in the PR description is a throttling example (it throttles the number of time echo's we are creating).

An example of a rate limiter can be:

<?php

require 'vendor/autoload.php';

$request = new \Symfony\Component\HttpFoundation\Request::createFromGlobals();

$factory = new \Symfony\Component\RateLimiter\LimiterFactory();
$limiter = $factory->createLimiter(10, \Symfony\Component\RateLimiter\Rate::perSecond(1), $request->getClientIp());

if (!$limiter->consume()) {
    (new \Symfony\Component\HttpFoundation\Response(null, 429))->send();
}

// ...

@stof
Copy link
Member

stof commented Jul 10, 2020

@wouterj but for an HttpClient, the proper implementation of rate throttling is to adapt to the rate limits of the target API, not to implement rate limits on its own (as it might not be the only client involved to consume the rate limits) That's why I'm doubting that the same component would be used for that.

@wouterj
Copy link
Member Author

wouterj commented Jul 10, 2020

Ah, I've now read the Heroku article you posted and I see what you mean. I still think this component works for both throttling and limiting, but API call throttling might need a different approach indeed. For the focus of this PR, let's remove the API throttling usecase from this PR and keep the API throttling discussions in #37471.

@kbond
Copy link
Member

kbond commented Jul 16, 2020

Heh, interesting timing! I'm just finalizing a generic throttling library of my own: https://github.com/zenstruck/governator

It is basically a port of the throttling system from Laravel (core) and uses the fixed window strategy. I used the lock component for inspiration on configuring the stores.

This certainly looks more robust and would love to see rate limiting in Symfony core!

@wouterj wouterj force-pushed the rate-limiter branch 2 times, most recently from a22ede9 to c72c323 Compare July 21, 2020 15:31
@wouterj
Copy link
Member Author

wouterj commented Jul 21, 2020

This PR now includes integration into the FrameworkBundle using a LimiterFactory (this copies the approach of the Lock component), see updated example in the PR description.

I've come to the conclusion that a generic limiter component must at least provide 2 different limiter algorithms. I propose the following 2 algorithms:

  • Token bucket (originally in this PR), allows limiting bursts and rates: "After 5 failed attempts, you can only try once every 15 minutes."
  • Sliding window, common in API limiting: "You can make a maximum of 50 requests per hour"

Other possible algorithms are: leaky bucket (more like a queue) or fixed window (implemented by Laravel). See https://medium.com/figma-design/an-alternative-approach-to-rate-limiting-f8a06cf7c94c and https://blog.cloudflare.com/counting-things-a-lot-of-different-things/ on reasons to use sliding window instead of the fixed window implemented in Laravel.

@wouterj wouterj force-pushed the rate-limiter branch 2 times, most recently from d242a07 to b09f4b0 Compare July 22, 2020 07:51
@wouterj
Copy link
Member Author

wouterj commented Sep 7, 2020

So I've been coming back to this PR locally a couple of times and I'm a bit stuck.

The component as-is provides a working Token Bucket limiter implementation. This works great for something like login throttling, but might not be the best limiter algorithm for API limits (sliding window/fixed window is a better choice). However, I can't find a way to add the same "reservation" support to these window algorithms. Where I need help: Should we provide one limiter interface for different algorithms, or should we create 2 separate interfaces?

Also, using the cache component for storage is nice, but directly implementing e.g. Redis can improve performance significantly I think (as you can use REdis commands directly, instead of a "fetching -> php code -> storing" loop). I think that's out of the initial scope of this PR, but something to be taken into account.

@fabpot
Copy link
Member

fabpot commented Sep 7, 2020

I don't see any problem in providing 2 interfaces.
Regarding Redis, we can probably improve performance in another PR.

@dunglas
Copy link
Member

dunglas commented Sep 13, 2020

Regarding web API limits, isn't it better to handle them at the edge of the network? Most web servers (NGINX, Apache) and proxies such as Cloudflare support fixing rate limits. Enforcing the limits at this level prevent to start a PHP process for nothing.

@wouterj wouterj force-pushed the rate-limiter branch 3 times, most recently from 8653296 to 09788fc Compare September 13, 2020 17:41
src/Symfony/Component/Lock/NoLock.php Outdated Show resolved Hide resolved
src/Symfony/Component/Lock/NoLock.php Outdated Show resolved Hide resolved
src/Symfony/Component/RateLimiter/LimiterFactory.php Outdated Show resolved Hide resolved
src/Symfony/Component/RateLimiter/LimiterFactory.php Outdated Show resolved Hide resolved
src/Symfony/Component/RateLimiter/LimiterFactory.php Outdated Show resolved Hide resolved
src/Symfony/Component/RateLimiter/Reservation.php Outdated Show resolved Hide resolved
src/Symfony/Component/RateLimiter/TokenBucket.php Outdated Show resolved Hide resolved
src/Symfony/Component/RateLimiter/TokenBucketLimiter.php Outdated Show resolved Hide resolved
src/Symfony/Component/RateLimiter/TokenBucketLimiter.php Outdated Show resolved Hide resolved
src/Symfony/Component/RateLimiter/Window.php Outdated Show resolved Hide resolved
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

The component should be marked as experimental (via @experimental in 5.2 in all classes).

src/Symfony/Component/RateLimiter/CompoundLimiter.php Outdated Show resolved Hide resolved
src/Symfony/Component/RateLimiter/FixedWindowLimiter.php Outdated Show resolved Hide resolved
src/Symfony/Component/RateLimiter/README.md Show resolved Hide resolved
src/Symfony/Component/RateLimiter/LimiterFactory.php Outdated Show resolved Hide resolved
@wouterj
Copy link
Member Author

wouterj commented Sep 16, 2020

Alright, I've updated the PR conform your comments (I think I agree that application wide limiters are probably quite rare).

@fabpot
Copy link
Member

fabpot commented Sep 16, 2020

Thank you @wouterj.

@fabpot fabpot merged commit 7cd5bbf into symfony:master Sep 16, 2020
@wouterj wouterj deleted the rate-limiter branch September 16, 2020 13:51
fabpot added a commit that referenced this pull request Sep 17, 2020
This PR was squashed before being merged into the 5.2-dev branch.

Discussion
----------

[Security] Added login throttling feature

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix #37266
| License       | MIT
| Doc PR        | tbd

This "recreates" #37444 based on the RateLimiter component from #37546 <s>(commits are included in this branch atm)</s>.

Login throttling can be enabled on any user-based authenticator (thanks to the `UserBadge`) with this configuration:

```yaml
security:
    firewalls:
        default:
            # default limits to 5 login attempts per minute, the number can be configured via "max_attempts"
            login_throttling: ~

            # or you can define your own RateLimiter on framework.rate_limiter and configure it instead:
            login_throttling:
                limiter: login
```

Commits
-------

afdd805 [Security] Added login throttling feature
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
wouterj added a commit to wouterj/symfony that referenced this pull request Oct 11, 2020
Relates to symfony#37546 (comment)

I'm still not sure about a factory class that is called "Limiter", as it
doesn't implement the "LimiterInterface" and doesn't represent a limiter.
wouterj added a commit to wouterj/symfony that referenced this pull request Oct 14, 2020
Relates to symfony#37546 (comment)

I'm still not sure about a factory class that is called "Limiter", as it
doesn't implement the "LimiterInterface" and doesn't represent a limiter.
wouterj added a commit to wouterj/symfony that referenced this pull request Oct 14, 2020
Relates to symfony#37546 (comment)

I'm still not sure about a factory class that is called "Limiter", as it
doesn't implement the "LimiterInterface" and doesn't represent a limiter.
wouterj added a commit to wouterj/symfony that referenced this pull request Oct 15, 2020
Relates to symfony#37546 (comment)

I'm still not sure about a factory class that is called "Limiter", as it
doesn't implement the "LimiterInterface" and doesn't represent a limiter.
@kdefives
Copy link

I know this PR is already merge but do you know if it is a good idea to use this feature to build a RateLimiter middleware for Guzzle Http client?
I explain how I use this experimental feature here: https://codereview.stackexchange.com/q/257312/239259

Any feedback appreciated :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.