aspnetcore icon indicating copy to clipboard operation
aspnetcore copied to clipboard

Rate Limiting configuration - policy validation

Open mburumaxwell opened this issue 2 years ago • 2 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues

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

The ASP.NET Core rate limiting middleware is great, but "limited" in terms of policy validation. Let's start with some code that you can write today in .NET 7:

builder.Services.AddRateLimiter(options =>
{
    options.AddFixedWindowLimiter("customPolicy", opt =>
    {
        opt.PermitLimit = 4;
        opt.Window = TimeSpan.FromSeconds(12);
        opt.QueueProcessingOrder = QueueProcessingOrder.OldestFirst;
        opt.QueueLimit = 2;
    });
    // ...
});

There is no way to validate that customPolicy actually exists. This is useful when configuring multiple routes from configuration such as is the case for YARP. See https://github.com/microsoft/reverse-proxy/pull/1967

Describe the solution you'd like

It would be preferred to something similar to IAuthorizationPolicyProvider implemented via DefaultAuthorizationPolicyProvider and ICorsPolicyProvider implemented via DefaultCorsPolicyProvider

public interface IAuthorizationPolicyProvider
{
    Task<IRateLimiterPolicy<DefaultKeyType>?> GetDefaultPolicyAsync();
    Task<IRateLimiterPolicy<DefaultKeyType>?> GetPolicyAsync(string policyName);
}

Additional context

RateLimiterOptions.PolicyMap is internal hence this feature cannot be added in another library or the final application.

mburumaxwell avatar Dec 20 '22 06:12 mburumaxwell

Triage: this seems like a reasonable suggestion (a way to find out about these issues at load-time rather than run-time).

Would an API that returned a bool (indicating whether the policy exists) be sufficient?

adityamandaleeka avatar Jan 04 '23 23:01 adityamandaleeka

@mburumaxwell Can you update your comment to make it follow the API proposal template here? https://github.com/dotnet/aspnetcore/issues/new?assignees=&labels=api-suggestion&template=30_api_proposal.md&title=

adityamandaleeka avatar Jan 04 '23 23:01 adityamandaleeka

@adityamandaleeka this is done

mburumaxwell avatar Jan 10 '23 09:01 mburumaxwell

Thanks @mburumaxwell

adityamandaleeka avatar Jan 10 '23 21:01 adityamandaleeka

What steps follow to get this to be in the next version of AspNetCore?

mburumaxwell avatar Jan 13 '23 14:01 mburumaxwell

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

ghost avatar Jan 13 '23 21:01 ghost

This proposal will be discussed by our team in an upcoming API review meeting, after which we'll provide feedback/suggestions.

Once a proposal gets to the api-approved state, we'll be ready to take a PR to implement the change.

adityamandaleeka avatar Jan 13 '23 21:01 adityamandaleeka

Thanks for contacting us.

We're moving this issue to the .NET 8 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.

ghost avatar Jan 30 '23 23:01 ghost

API Review Notes:

  • We don't like making the DefaultKeyType public if possible.
  • It's not clear what you'd use the IRateLimiterPolicy<DefaultKeyType> for other than just checking that it exists like YARP does.

We think the API needs work. Maybe it could be combined with the rate limit feature proposal. #45658

halter73 avatar Jul 24 '23 17:07 halter73

The proposal in #45658 cannot work because the validation would only be available where a HttpContext is yet YARP needs validation elsewhere.

If I understand correctly, the main issue is making DefaultKeyType public. What if instead we just have boolean values?

namespace Microsoft.AspNetCore.RateLimiting;

+ public interface IRateLimiterPolicyProvider
+ {
+     bool DefaultPolicyExists();
+     bool PolicyExists(string policyName);
+ }
+
+ public class DefaultRateLimiterPolicyProvider : IRateLimiterPolicyProvider
+ {
+     private readonly RateLimiterOptions _options;
+     
+     public DefaultRateLimiterPolicyProvider(IOptions<RateLimiterOptions> options)
+     {
+     
+     }
+     
+     public bool PolicyExists(string policyName)
+     {
+         options.PolicyMap.ContainsKey(policyName) || options.UnactivatedPolicyMap.ContainsKey(policyName);
+     }
+ }

mburumaxwell avatar Sep 15 '23 05:09 mburumaxwell