-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
rate limiter as middleware #1343
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1343 +/- ##
==========================================
+ Coverage 84.39% 85.03% +0.63%
==========================================
Files 27 28 +1
Lines 2019 2305 +286
==========================================
+ Hits 1704 1960 +256
- Misses 205 225 +20
- Partials 110 120 +10
Continue to review full report at Codecov.
|
@vishr if u review the code, I'll continue for distributed rate limiter feature. I'm thinking distributed rate limiter with Redis or Memcached it will be configurable and don't want to depend on it directly. Do u have any comment about it? |
@alperhankendi I definitely want it. However, couple of points:
|
Distributed rate limiter has been added. @vishr According to your comment the middleware is just know the RedisClient interface and the client imp. will be injected outside the Echo.
middleware usage;
|
Out of curiosity @vishr, why hasn't this been merged to the main branch yet? This looks rather solid. |
…t ip or header(key))
…t ip or header(key))
…t ip or header(key))
…t ip or header(key))
I have changed the limitation logic. now there are Strategy options. you can limit your API based on client-ip or header key. middleware using;
TODO:
@vishr it seems unit test coverage failed. I have tried to pump the coverage rate. rate_limiter.go file coverage is about %92 waiting for your comment |
Looking forward for this feature |
Client:nil, | ||
SkipRateLimiterInternalError:false, | ||
} | ||
limiterImp *limiter |
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.
To support the use-case of creating multiple rate limiters (1 for minute, hour, day, etc...) can we move this from global to instance based?
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.
imo it makes thinks complex. What you have in your mind, can you give more specific example?
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.
this is a way to support multiple rate limiters.
rateLimiter1:=middleware.RateLimiterWithConfig(middleware.RateLimiterConfig{
LimitConfig: middleware.LimiterConfig{
Max: 2,
Strategy: "header",
Duration: time.Minute,
Key: "test",
HeaderTokenExtractorHandler: func(headerValue string) string {
return headerValue
},
},
})
rateLimiter2:=middleware.RateLimiterWithConfig(middleware.RateLimiterConfig{
LimitConfig: middleware.LimiterConfig{
Max: 2,
Strategy: "ip",
Duration: time.Minute,
},})
e.GET("/", func(c echo.Context) error {
return c.String(http.StatusOK, "Hello, World!")
},rateLimiter1)
e.GET("/status", func(c echo.Context) error {
return c.String(http.StatusOK, "Hello, World!")
},rateLimiter2)
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.
Are you sure? I think in your example here, rateLimiter2
's config would overwrite rateLimiter1
, so yes, you'd have 2 rate limiters, but both would be using the same limiterImp
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.
can you give more specific example
The most specific example I can think of is:
e.GET("/", func(c echo.Context) error {
return c.String(http.StatusOK, "Hello, World!")
}, rateLimiter1Min, rateLimiter60Min, rateLimiter1440Min)
For example, we could set 10,000 requests max per day, but we don't want all 10,000 to happen in the first minute, so the minute rate limiter could be 60.
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.
What I've presented above, is simple/clean, and I've seen in some node.js libraries, but there's another approach that's even more interesting.
Dynamic rate limits per request.
For example:
e.GET("/", func(c echo.Context) error {
return c.String(http.StatusOK, "Hello, World!")
}, createRateLimiterMiddleware(func(c *echo.Context) ([]middleware.RateLimiterConfig, error) {
user := extractUse(c)
path := extractPath(c)
if user.role == "premium" {
return []middleware.RateLimiterConfig{middleware.RateLimiterConfig{Max: 1000, Duration: time.Hour}}, nil
}
return defaultRateLimiterConfigs(), nil
}))
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.
Are you sure? I think in your example here,
rateLimiter2
's config would overwriterateLimiter1
, so yes, you'd have 2 rate limiters, but both would be using the samelimiterImp
you're right. there is only one limiterimp so first registered middleware is working and rest of the limiters will be ignored as you said it should work with many limiters so Im gonna change it to support multiple limiters. many thanks
response.Header().Set("X-Ratelimit-Limit", strconv.FormatInt(int64(result.Total), 10)) | ||
response.Header().Set("X-Ratelimit-Remaining", strconv.FormatInt(int64(result.Remaining), 10)) | ||
response.Header().Set("X-Ratelimit-Reset", strconv.FormatInt(result.Reset.Unix(), 10)) |
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.
If you decide to support multiple instances, these header names could be modified like:
fmt.Sprintf("X-Ratelimit-Remaining-%s", config.Prefix)
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.
A prefix is used for the cache keys. We may use hostName instead of prefix.
something like;
hostName,_ := os.HostName()
fmt.Sprintf("X-Ratelimit-Remaining-%s", hostName)
or
fmt.Sprintf("X-Ratelimit-Host-%s", hostName)
I think new header is more proper.
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.
It would be great if this patch implements the ratelimit I-D here https://tools.ietf.org/html/draft-polli-ratelimit-headers-01.
You just need to return a delta-seconds
instead of a unix-timestamp
. See comments for further infos.
thanks @jotto for the valuation feedback. can check the new commit |
TokenExtractorHandler added. some code fixes
Hi @alperhankendi, it would be great if this middleware aligns to the new ratelimit standardization proposal. The proposal was:
The proposalVery similar to your current implementation, but header names have no
Moreover, you're welcome to contribute to the spec and add your implementation here ioggstream/draft-polli-ratelimit-headers#1 |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Hi there! what is the status of this implementation? @alperhankendi are you going to adapt this PR to #1343 (comment) ? Thanks a lot! |
HI, Any chance to get this feature soon? |
Look into github.com/didip/tollbooth maybe it will be good for you. |
Thank you @Demetri0 |
When will this be pushed as a feature? |
why do you think it should? |
@jotto @alperhankendi |
rate limiter middleware usage;
The default config;
the same request is limited with 100 in a minute.
ps: Distributed RateLimiter implementation is in progress. I'm thinking about to use Redis for it.
for 200-response these fields will be added to the response header.
for 429 - to many request code these fields will be added to the response header.
TODO: