Skip to content
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

Multiple RateLimiter per Endpoint #4

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Multiple RateLimiter per Endpoint #4

wants to merge 1 commit into from

Conversation

Kahbazi
Copy link
Owner

@Kahbazi Kahbazi commented Aug 2, 2022

This is a draft of what I had in my mind in dotnet/aspnetcore#42691
I was hoping to get this in ASP.Net Core RateLimiting middleware.

There are two kinds of RateLimiter here:

  1. One that could be shared between endpoints (IRateLimitPolicy)
  2. One that is unique for the selected endpoint (IRateLimitInline)
  • Each endpoint can have multiple IRateLimitPolicy or IRateLimitInline. The rate limiters are executed in order that is given by the metadata. (if needed an Order property could be added to them as well)
  • When one rate limiter reached its limit, the others won't be executed (although this could be an option)

I would appreciate to have feedbacks if you got the chance.
@wtgodbe @halter73 @BrennanConroy @Tratcher @davidfowl

TODO:

  • Implement OnRejected
  • Add DefaultPolicy or GlobalPolicy
  • Combine IsAcquired with WaitAsync

@halter73
Copy link

halter73 commented Aug 2, 2022

This does seem like a pretty reasonable approach to multiple policies on an endpoint. I think we might do this, but I'm a little hesitant to rush more features this late into .NET 7.

I was worried that different policies having different partitioning schemes could lead to confusion. But looking at this, I don't think that will really be a problem. Different partition schemes for different policies on the same endpoint should be fine and isn't that hard to reason about.

I'm also a little concerned that the first/outermost policy will contribute to things like the concurrency limit while still awaiting permits from inner polices, but maybe this is fine. This already happens for the global policy.

It seems to me that we can support multiple policies in the future version of .NET pretty cleanly without breaking API if we want to. Do you have a concrete scenario where you need an endpoint to have multiple policies on an endpoint?

@Kahbazi
Copy link
Owner Author

Kahbazi commented Aug 6, 2022

@halter73 Thanks for the feedback.

I'm also a little concerned that the first/outermost policy will contribute to things like the concurrency limit while still awaiting permits from inner polices, but maybe this is fine. This already happens for the global policy.

Yes also the outermost policy could block the request and return a RetryAfter. And after that period the inner policy could block it again and send another RetryAfter, which could be confusing for the caller.

It seems to me that we can support multiple policies in the future version of .NET pretty cleanly without breaking API.

But it would break the behavior. Right now if there's multiple policy on an endpoint, only one of them would apply. Maybe throw an exception for .NET 7.0 if there are multiple policy? but then one can not override a policy. 🤔

Do you have a concrete scenario where you need an endpoint to have multiple policies on an endpoint?

It's not final yet, but it is probably something like this.
First based on IP, then Client Id, then Access Token, Username, and finally combination of Username and Endpoint name.

I also need to break the chain for certain conditions. For example if requests came from 127.0.0.1, I don't want the other rate limiter to execute. I'll probably write my own CreateNoLimiter with a special metadata in its lease and check for that to stop the chain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants