Rate limiter implementation #203

romkatsu · 2020-01-05T18:53:17Z

Q	A
Is bugfix?	❌
New feature?	✔️
Breaks BC?	❌
Tests pass?	✔️
Fixed issues	#63

merge base repository

Update fork

update fork

src/Middleware/RateLimiter.php

tests/Middleware/RateLimiterTest.php

This reverts commit 1b22224.

rustamwin

Without a database repository implementation?

src/Middleware/RateLimiter.php

romkatsu · 2020-01-06T17:25:37Z

Without a database repository implementation?

I didn't plan on it, but I can try. What tasks can this be useful for?

rustamwin · 2020-01-07T12:59:17Z

I didn't plan on it, but I can try. What tasks can this be useful for?

See https://github.com/yiisoft/yii2/blob/master/framework/filters/RateLimiter.php and https://github.com/yiisoft/yii2/blob/master/framework/filters/RateLimitInterface.php

romkatsu · 2020-01-07T13:30:47Z

I didn't plan on it, but I can try. What tasks can this be useful for?

See https://github.com/yiisoft/yii2/blob/master/framework/filters/RateLimiter.php and https://github.com/yiisoft/yii2/blob/master/framework/filters/RateLimitInterface.php

added storage implementation, now it will be possible to add storage in the database

src/RateLimiter/CounterInterface.php

- Method prefix removed - Prefix is added to an interface name instead

samdark · 2020-01-08T00:36:14Z

src/RateLimiter/Counter.php

+        $value = $this->getCounterValue();
+        $value++;
+
+        $this->storage->set($this->id, $value, $this->interval);


While it makes code simple, this rate limiter implementation (fixed window) results in possibility of traffic spikes on the edge of the interval. The behavior for interval = 360 (6 minutes) and limit = 10000 is that after 10000 requests there will be 504 HTTP response for exactly 6 minutes. Then there could be quick 10000 requests again and another 6 minute pause etc. So you'll still have traffic spikes this way. That's a no go for a good rate limiter.

Good algorithms preventing traffic spikes are:

Leaky bucket https://en.wikipedia.org/wiki/Leaky_bucket. That one is in Yii 2.

Sliding window.

See:

https://blog.cloudflare.com/counting-things-a-lot-of-different-things/

https://habr.com/ru/post/448438/

I'd implement GCRA:

https://brandur.org/rate-limiting

https://blog.ian.stapletoncordas.co/2018/12/understanding-generic-cell-rate-limiting.html

And counts are lost. for example

A request get counter value (X)

B request get counter value (X)

A requrest set counter value (X+1)

B request set counter value (X+1)

In fact, X + 2. In multi server environments, this can mean a lot of lost counting.
I recommend that the storage interface have a incrementAndGet() method. Redis, mysql allow counting in atomic.

Yes, increment operation should be atomic.

rustamwin

All setters should be void or with prefixed

public function withLimit(int $limit): self
{
    $new = clone $this
    $new->limit = $limit;
    return $new;
}

rustamwin · 2020-01-08T09:47:27Z

src/RateLimiter/CacheCounterStorage.php

+/**
+ * CacheCounterStorage stores counter value in cache
+ */
+final class CacheCounterStorage implements CounterStorageInterface


I'd suggest naming class/interface without Storage

The purpose of it is to store counter data. How would you name it?

It's not about storage, it's about counting.

Nope. https://github.com/yiisoft/yii-web/pull/203/files#diff-e94ff4fb5bda21fe7b12afa594018f4c is about counting. This one is about storage. If these should be separate is another question.

Why storage? There is no need to read it, it is always only increment, if the result is higher than the limit then it is too many requests

CacheCounter implements CounterInterface // or CounterServiceInterface

Counter in current implementation is useless. Currently, a single Counter can be specified for middleware -> can be implemented to middleware.

3 layers:

LimiterDescriptor

CounterInterface

LimiterMiddlerware

I think the Counter class is not needed. Each counter should be individually

we've already discussed this.

What are possible examples?

Counter in current implementation is useless. Currently, a single Counter can be specified for middleware -> can be implemented to middleware.

Why useless? You can also use multiple instances of middleware.

$counter1 = (new Counter($storage))->setInterval(60); $counter2 = (new Counter($storage))->setInterval(86400); $middleware1 = (new RateLimiter($counter1, $responseFactory)) ->withLimit(10); $middleware2 = (new RateLimiter($counter2, $responseFactory)) ->withLimit(100); Route::get('/') ->prepend($middleware1) ->to(new ActionCaller(SiteController::class, 'index', $container)) ->name('site/index') Route::get('/') ->prepend($middleware2) ->to(new ActionCaller(SiteController::class, 'index', $container)) ->name('site/test')

Why useless? You can also use multiple instances of middleware.

the current implementation is the same as:

$counter = new CacheCounter(); $middleware1 = (new RateLimiter($counter, $responseFactory)) ->withLimit(10) ->withInterval(60); $middleware1 = (new RateLimiter($counter, $responseFactory)) ->withLimit(100) ->withInterval(86400);

kamarton · 2020-01-08T10:44:14Z

src/RateLimiter/CounterStorageInterface.php

+/**
+ * Counter value storage
+ */
+interface CounterStorageInterface


I suggest simplify it

interface CounterInterface { public function incrementAndGet(string $id, int $interval): int; }

kamarton · 2020-01-08T10:49:33Z

src/RateLimiter/Counter.php

+
+    private function generateIdFromRequest(ServerRequestInterface $request): string
+    {
+        return strtolower('rate-limiter-' . $request->getMethod() . '-' . $request->getUri()->getPath());


generate is not public, the strtolower() can be omitted.

Query request parameters can be anything, I just want the ID to be the same in terms of case.

kamarton · 2020-01-08T10:51:13Z

src/RateLimiter/RateLimiter.php

+ */
+final class RateLimiter implements MiddlewareInterface
+{
+    private int $limit = 1000;


Does the default value make sense?

kamarton · 2020-01-08T10:52:55Z

src/RateLimiter/Counter.php

+    {
+        $this->id = $this->generateId($request);
+
+        if (!$this->storage->has($this->id)) {


This is already the task of CounterInterface.

kamarton · 2020-01-08T10:55:48Z

src/RateLimiter/RateLimiter.php

+
+    private ResponseFactoryInterface $responseFactory;
+
+    private bool $autoincrement = true;


I don't really understand the point of leaving auto-increment. If autoincrement is disabled, it is more like just a "checker".

kamarton · 2020-01-09T06:06:39Z

Current implementation not allow multiple limit descriptor in a single middleware. It is necessary to record more intervals and more limits, eg.

ID                    interval  limit
sms/user/12             60s     2
sms/user/12           3600s     10
sms/users               60s     40
sms/users             3600s     250

romkatsu · 2020-01-10T19:13:05Z

Created a new pull request with the implementation of the GCRA.

I'm closing this one.

romkatsu and others added 7 commits September 22, 2019 16:58

Merge pull request #1 from yiisoft/master

6b86901

merge base repository

Merge pull request #2 from yiisoft/master

bde0a00

Update fork

Merge pull request #3 from yiisoft/master

532d758

Update fork

Merge pull request #4 from yiisoft/master

312ca3a

update fork

Merge pull request #5 from yiisoft/master

7245614

update fork

add middleware

00a5df2

add tests

6971cc7

samdark added the status:code review The pull request needs review. label Jan 5, 2020

samdark requested a review from a team January 5, 2020 21:10

Improve readability

1b22224

samdark suggested changes Jan 5, 2020

View reviewed changes

Revert "Improve readability"

bf4381c

This reverts commit 1b22224.

samdark added status:under development Someone is working on a pull request. and removed status:code review The pull request needs review. labels Jan 5, 2020

romkatsu added 2 commits January 6, 2020 07:58

fix name method and remove hasCounterValue

11a045b

fix tests

09a796d

romkatsu requested review from samdark and removed request for a team January 6, 2020 08:37

samdark added status:code review The pull request needs review. and removed status:under development Someone is working on a pull request. labels Jan 6, 2020

rustamwin suggested changes Jan 6, 2020

View reviewed changes

src/Middleware/RateLimiter.php Outdated Show resolved Hide resolved

fix names

05d40c6

romkatsu added 2 commits January 7, 2020 12:46

added counter and storage implementations

86498aa

fix code styles

fb45600

romkatsu requested a review from rustamwin January 7, 2020 13:31

samdark reviewed Jan 7, 2020

View reviewed changes

src/RateLimiter/CounterInterface.php Outdated Show resolved Hide resolved

remove counter interface

cc0a6e1

romkatsu requested a review from samdark January 7, 2020 23:05

samdark requested a review from a team January 7, 2020 23:09

samdark added 3 commits January 8, 2020 02:34

Rename StorageInterface and its methods

21de4e8

- Method prefix removed - Prefix is added to an interface name instead

Rename test to match source class name

2cac7f2

Add phpdoc

4d22d84

samdark reviewed Jan 8, 2020

View reviewed changes

samdark added status:under development Someone is working on a pull request. and removed status:code review The pull request needs review. labels Jan 8, 2020

rustamwin suggested changes Jan 8, 2020

View reviewed changes

kamarton reviewed Jan 8, 2020

View reviewed changes

fix name setters

61e390a

romkatsu mentioned this pull request Jan 10, 2020

Rate limiter implementation(gcra) #204

Merged

romkatsu closed this Jan 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rate limiter implementation #203

Rate limiter implementation #203

romkatsu commented Jan 5, 2020

rustamwin left a comment

romkatsu commented Jan 6, 2020

rustamwin commented Jan 7, 2020

romkatsu commented Jan 7, 2020

samdark Jan 8, 2020

samdark Jan 8, 2020 •

edited

Loading

kamarton Jan 8, 2020

samdark Jan 8, 2020

rustamwin left a comment •

edited

Loading

rustamwin Jan 8, 2020

samdark Jan 8, 2020

kamarton Jan 8, 2020

samdark Jan 8, 2020

kamarton Jan 8, 2020

kamarton Jan 9, 2020

kamarton Jan 9, 2020

romkatsu Jan 9, 2020

romkatsu Jan 9, 2020

kamarton Jan 9, 2020

kamarton Jan 8, 2020

kamarton Jan 8, 2020

romkatsu Jan 8, 2020

kamarton Jan 8, 2020

kamarton Jan 8, 2020

kamarton Jan 8, 2020

kamarton commented Jan 9, 2020

romkatsu commented Jan 10, 2020


		private ResponseFactoryInterface $responseFactory;

		private bool $autoincrement = true;

Rate limiter implementation #203

Rate limiter implementation #203

Conversation

romkatsu commented Jan 5, 2020

rustamwin left a comment

Choose a reason for hiding this comment

romkatsu commented Jan 6, 2020

rustamwin commented Jan 7, 2020

romkatsu commented Jan 7, 2020

Choose a reason for hiding this comment

samdark Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustamwin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamarton commented Jan 9, 2020

romkatsu commented Jan 10, 2020

samdark Jan 8, 2020 •

edited

Loading

rustamwin left a comment •

edited

Loading