-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
adds middleware for rate limiting #1724
adds middleware for rate limiting #1724
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1724 +/- ##
==========================================
+ Coverage 86.88% 87.32% +0.44%
==========================================
Files 30 31 +1
Lines 2089 2162 +73
==========================================
+ Hits 1815 1888 +73
Misses 175 175
Partials 99 99
Continue to review full report at Codecov.
|
a945c80
to
9b63f99
Compare
Using TODO: Find a more reliable method for testing the rate limiting which does not involve using the I am open to any suggestions on this time issue. |
Lovely overall! |
TODO: Implement an expire functionality in order to automatically manage the amount of visitors that are currently active. Added a last seen property to the visitor struct. I intend on adding an Optional |
c6e7fc6
to
8d34f11
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks even better now...
adds default error handler
Now Custom Error Handler can be specified using mw := middleware.RateLimiterWithConfig(RateLimiterConfig{
ErrorHandler: func(c echo.Context) error {
return c.JSON(http.StatusTooManyRequests, nil)
},
Store: inMemoryStore,
}) |
…ndler for rate limiting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pafuent you missed again all the fun. Feedback welcome, ready to merge.
@iambenkay Good work. Can you prepare a PR for the docs too in echox?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iambenkay I like the overall implementation. Please let me know if my comments makes sense for you.
@lammel Yep, it seems that I missed the fun 😢 (it was hard to resist the temptation to stop working and have some fun when I saw the emails back and forth)
@iambenkay please address this two comments: |
…limiter-middleware
…iambenkay/echo into feature/rate-limiter-middleware
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. We should just discuss the Allow signature.
middleware/rate_limiter.go
Outdated
// RateLimiterStore is the interface to be implemented by custom stores. | ||
RateLimiterStore interface { | ||
// Stores for the rate limiter have to implement the Allow method | ||
Allow(identifier string) bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allow may have internal errors due to hash limits or other issues.
In this case it may make sense to make the signature to be Allow(identifier string) bool, error
to be able to handle this special cases as we will be able to pass the error also to the DenyHandler
. Use case would be for example to allow access (not apply rate limit) for DB connection errors (or other internal errors).
Not sure if this is needed, but it was also suggested by @aldas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay let me include this. it is a valid point.
@iambenkay Pushed some comment improvements to resolve some remaining comments. |
@iambenkay Can you do a documentation PR in labstack/echox too? |
Yes! |
…y/echo into feature/rate-limiter-middleware
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pafuent Approved from my side. Feel free to merge.
Thanks (especially for your patience) @iambenkay.
Btw. tested the limiter locally with a simple hello world and it worked as expected with up to 30000 req/s and my laptop (from single local IP)
That's super! |
@pafuent calling your attention to this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iambenkay Thanks for this new middleware and thanks for your patience
What's New?
This feature implements a store agnostic rate limiting middleware i.e configurable with any store of user's choice.
Here is a dummy snippet of how a certain (unconventional) redis store might be integrated with the rate limiting middleware
I threw in an InMemory implementation for people like me who want to get on the go fast.
closes #1721