API Refactor: Rate-limiting middleware plus scaffolding #1563
Conversation
- Middleware interface provides pre and post processing steps - Provides interceptor implemementations for gRPC - Implements logger middleware - Implements per-rpc and per-caller rate limiting Signed-off-by: Andrew Harding <andrew.harding@hpe.com>
azdagron
requested review from
amartinezfayo,
APTy,
evan2645 and
mcpherrinm
as code owners
Signed-off-by: Andrew Harding <andrew.harding@hpe.com>
| ) | ||
|
|
||
| var ( | ||
| // NewRateLimiter is used to create a new ratelimiter. It returns a limiter |
There was a problem hiding this comment.
| // NewRateLimiter is used to create a new ratelimiter. It returns a limiter | |
| // newRawRateLimiter is used to create a new ratelimiter. It returns a limiter |
| Burst() int | ||
| } | ||
|
|
||
| // NoLimit returns a rate limiter does not rate limit. It is used to configure |
There was a problem hiding this comment.
| // NoLimit returns a rate limiter does not rate limit. It is used to configure | |
| // NoLimit returns a rate limiter that does not rate limit. It is used to configure |
| // WithRateLimits returns a middleware that performs rate limiting for the | ||
| // group of methods descripted by the rateLimits map. It provides the | ||
| // configured rate limiter to the method handlers via the request context. If | ||
| // the middleware is invoked for a method is not described in the map, it will |
There was a problem hiding this comment.
| // the middleware is invoked for a method is not described in the map, it will | |
| // the middleware is invoked for a method that is not described in the map, it will |
| lim.mtx.RUnlock() | ||
| return limiter | ||
| } | ||
| lim.mtx.RUnlock() |
There was a problem hiding this comment.
why not call defer lim.mtx.Unlock() right after lim.mtx.RLock()? at the beginning?
There was a problem hiding this comment.
That would cause a deadlock below where this function takes the write lock to create a new limiter under this address.
| } | ||
|
|
||
| type rateLimitsMiddleware struct { | ||
| limiters map[string]api.RateLimiter |
There was a problem hiding this comment.
can this be accessed concurrently? should we have a mutex?
There was a problem hiding this comment.
It shouldn't be mutated after passing it into rate limits, so concurrent access should be safe. I'll document the intended ownership on the WithRateLimits call.
|
Yep, there is some unit-test coverage around middleware and stuff that is required. I'll have those in follow up PRs. This also doesn't cover pruning the per-ip limit map. I've added a TODO and will implement that as well. |
Signed-off-by: Andrew Harding <andrew.harding@hpe.com>
** Update ** This PR does not handle periodic pruning of the per-ip limiter map. It also doesn't have direct unit-tests for the middleware chaining, etc. These will come in follow up PRs.