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

Idea: Define rules via attributes #5

Closed
kspearrin opened this issue Nov 12, 2016 · 6 comments
Closed

Idea: Define rules via attributes #5

kspearrin opened this issue Nov 12, 2016 · 6 comments

Comments

@kspearrin
Copy link

kspearrin commented Nov 12, 2016

In addition to defining rules via the appsettings.json configuration, I'd suggest adding a way to define rules on specific endpoints via Attributes.

Something like:

[HttpGet("values/{id}")]
[RateLimit( Period = "15m", Limit = 5)]
public string Values(int id)
{
    return "";
}

IMO, this creates a cleaner implementation in solutions that have a large list of endpoints with different rate limit settings. Microsoft did this same thing with routing, which used to be all defined in one big Route.Config.cs but has now moved to attributes as the preferable way in MVC. Of course you could still support the settings approach too for defining things like global rules.

Thoughts?

@stefanprodan
Copy link
Owner

I've implemented this 3 years ago for ASP.NET Web API, you can see the docs here.

Attribute-based rate limiting has a down side: action filters are executed after the middlewares. If you are using OAuth or any other type of authorization, the rate limit will happen after the auth logic, making the whole point of IP rate limiting useless. I agree that is easier to apply the limits using attributes and if you only want to rate limit based on client id then you probably don't care that the limiting is done after the middleware stack is executed. I will consider adding this in a future release.

@kspearrin
Copy link
Author

I didn't think about the fact that the current middleware approach comes before action filters, however, like you mentioned this would still work fine for client based rate limiting. Thanks for giving it a consideration.

@stefanprodan
Copy link
Owner

Even for client based rate limiting the attribute usage is very limited. If you have different limits for each subscription (like most API products have) then you can't use the attribute because the rate limit values are stored in the database and you clearly don't want to hardcode those in your code inside the attribute declaration. I think attribute-based rate limiting works only for public APIs that are applying the same limits to all their users, like Twitter does.

@kspearrin
Copy link
Author

You don't always have to query the database to get information like that. For example, JWT tokens could hold information about the client's rate limits.

@stefanprodan
Copy link
Owner

stefanprodan commented Nov 12, 2016

My approach is to store the client limits in cache (local or distributed). If it's local cache, then at app startup I load the rate limits from db in cache, then I can update the cache if a client gets removed or added while the app is running. If the app needs scaling, then I use Redis to store the limits so no matter the app instance a clients ends up on, the limit is applied. The JWT token approach will not work if your app has more then one instance because the load balancer in front of your app will forward a client to different instances based on the load. To rate limit a client that can call multiple app nodes in parallel, you need a mechanism to create a distributed lock and increment the rate limit counter atomically.

@kspearrin
Copy link
Author

kspearrin commented Nov 12, 2016

I think we misunderstood each other. I was referring to the JWT token holding what the client's rate limit policies are (as opposed to having to query the DB for this information), not their counter. Having looked further into how client rate limiting works in this library though (I've just been using IP rate limiting for my current implementation), I don't think that is even supported by the library. It seems that client policies are also hard coded into settings.

Update: Nevermind. It seems you can:

https://github.com/stefanprodan/AspNetCoreRateLimit/wiki/ClientRateLimitMiddleware#update-rate-limits-at-runtime

I think I am overthinking this a bit and we're getting a bit off topic. I'll go ahead and close this issue for now since it seems there are many reasons at play as to why it can't be easily supported. Thanks!

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

No branches or pull requests

2 participants