dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.38k stars 10k forks source link

Add a Redis-backed token bucket RateLimiter implementation #41861

Open halter73 opened 2 years ago

halter73 commented 2 years ago

Is your feature request related to a problem? Please describe the problem.

In the runtime repo, we have included a bunch of built-in, in-memory RateLimiter implementations like ConcurrencyLimiter, FixedWindowRateLimiter, SlidingWindowRateLimiter and TokenBucketRateLimiter. It would be nice to add a rate limiter that works across multiple hosts in a distributed environment. In the past, for things like our RedisCache IDistributedCache implementation, we've used StackExchange.Redis to implement a solution in the aspnetcore repo for a runtime abstraction. I think we should do the same things for this.

Describe the solution you'd like

I think we should implement a token bucket RateLimiter based on Redis using StackExchange.Redis that can be used in distributed environments. I imagine the options for this rate limiter will be a combination of what's in TokenBucketRateLimiterOptions and RedisCacheOptions.

We might want to make it easier to reuse existing IConnectionMultiplexer since there will likely be multiple instances by default. Another way to avoid unnecessary Redis connections might be to implement PartitionedRateLimiter<string> instead of RateLimiter.

@BrennanConroy @wtgodbe

Additional context

Customers are asking for this.

ghost commented 2 years ago

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

adityamandaleeka commented 2 years ago

Worth considering this scenario when working on this too: https://github.com/dotnet/aspnetcore/issues/41861

Expecho commented 9 months ago

Is this now moved to .Net 9 or what is the current status?

amcasey commented 8 months ago

@mgravell Does this align with the Redis work you're planning?

ReubenBond commented 8 months ago

BTW, I have proof-of-concept implementations here, which might serve as inspiration (or example of what not to do - either way is fine to me): https://github.com/ReubenBond/DistributedRateLimiting.Redis/tree/main/DistributedRateLimiting.Redis

mgravell commented 8 months ago

@amcasey this is certainly a topic I've discussed with Aditya, but this isn't what I'm focusing on right now; I have thoughts on it, in particular around avoiding latency overheads via broadcast, but that also depends on how strict we need to be ("N per T" is very different depending on whether there's an implied "give-or-take" hand-wave in there), but I haven't actively spiked anything. I'll take a look at Reuben's braindump.

(after eyeball) @ReubenBond the approach itself looks fine (I think it still pays latency for the core acquire, but maybe that's OK in most scenarios), but purely meant constructively: there are a few things in that impl that can be improved, in particular around redis compatibility re cluster routing and side-effect vs verbatim script replication; I suspect you tested on a standalone (non-cluster) newish server, which would have hidden some landmines from you - but: they exist. Happy to take a moment to discuss these at your convenience, if you want to learn a little more redis nuance.

md-redwan-hossain commented 6 months ago

Is there any progress of this feature?

adityamandaleeka commented 6 months ago

Hey @md-redwan-hossain, we haven't worked more on this since the last update. We're almost certainly not going to have time for this in the .NET 9 timeframe.

We're definitely aware of the need for a better distributed story for rate limiting.