-
Notifications
You must be signed in to change notification settings - Fork 266
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
Asp.NET Rate Limiter middleware support #618
Comments
Hello @GallaFrancesco, thanks for opening this issue. I'll add a new sample to this project showing how one can use this ASP.NET rate limiter middleware with Giraffe either this week or the next. |
Sounds great, looking forward to it. Thank you! |
I created this PR right now -> #619. It adds a global rate limiting middleware sample. I'm not sure if we can use the per-endpoint rate limiting configuration. I can take a proper look at this in the future, when I have more free time. |
It may be worth tweaking this function -> https://github.com/giraffe-fsharp/Giraffe/blob/master/src/Giraffe/EndpointRouting.fs#L278. We can eventually pattern match to the case where the user wants to add the rate limiting middleware on specific endpoints only. |
Thank you @64J0 that's really handy. I can look into modifying the Map* functions to be able to rate limit specific endpoints - the global rate limiter can be useful but having per-endpoint would allow Giraffe to support the same level of granularity allowed by Asp.NET. |
I was considering other approaches for this implementation, and maybe we can start using custom attributes on top of the handlers, like what we have for ASP.NET. So, based on these attributes (or the lack of them), we can map to the appropriate ASP.NET route configuration. Got this idea after reading this documentation for output caching on .NET 8: https://learn.microsoft.com/en-us/aspnet/core/performance/caching/output?view=aspnetcore-8.0#configure-one-endpoint-or-page. This code could give some ideas: https://github.com/haf/expecto/blob/main/Expecto/Model.fs#L108. |
I (personally) find that using attributes might be less consistent with the Giraffe approach to middlewares, while the API for rate limiting might be similar to Giraffe's authentication support: https://github.com/giraffe-fsharp/Giraffe/blob/master/DOCUMENTATION.md#requiresauthentication I realized though that the feature request in the original issue description is wrong: the user-defined HTTP handler should always be the last in the middleware pipeline, while the rate limiter could be applied directly to the request handlers, e.g.: let webApp =
choose [
route "/" >=> text "Hello World"
route "/limited" >=>
requiresRateLimiting policy >=>
choose [
GET >=> getHandler
POST >=> submitHandler
]
] |
I wish it was that simple. In this case, since we're aiming to use the ASP.NET feature, we would need to rewrite the Giraffe routing mechanism to account for scenarios that we want to use something like |
I updated the PR code to add a mechanism that lets you configure these ASP.NET extensions passing a list. Please let me know what you think. |
https://learn.microsoft.com/en-us/aspnet/core/performance/rate-limit?view=aspnetcore-8.0 is a rate limiting middleware implementation and I'm struggling to make this work in Giraffe. Two main reasons:
RequireRateLimiting()
in a similar way as this sample Asp.NET project: https://github.com/dotnet/AspNetCore.Docs.Samples/blob/23fbafb68257b59bf6f3775fe12c51afc2f242e3/fundamentals/middleware/rate-limit/WebRateLimitAuth/Program.cs#L163It would be ideal to support a middleware such as:
Though I could not find any example of this.
AddTokenBucketLimiter
is failing when trying to call it from F#, witherror FS0193: Type constraint mismatch. The type 'RateLimiterOptions' is not compatible with type 'unit'
, see the following snippet.The text was updated successfully, but these errors were encountered: