Skip to content

Make RateLimiterMiddleware endpoint-aware #41667

Closed
@wtgodbe

Description

@wtgodbe

Allow users to piecemeal add named RateLimiters that apply to specific endpoints - will achieve this w/ IEndpointConventionBuilder. Some discussion starting at #41655 (comment)

namespace Microsoft.AspNetCore.RateLimiting
{
    public interface IRateLimiterPolicy<TKey>
    {
        public int CustomRejectionStatusCode { get; }
        public RateLimitPartition<TKey> GetPartition(HttpContext httpContext);
    }

    public sealed class RateLimiterOptions
    {
        public PartitionedRateLimiter<HttpContext> Limiter { get; set; }
        public Func<HttpContext, RateLimitLease, Task> OnRejected { get; set; }
        public int DefaultRejectionStatusCode{ get; set; }
        public RateLimiterOptions AddPolicy<TKey>(string name, Func<HttpContext, RateLimitPartition<TKey>> partitioner, bool global = false)
        public RateLimiterOptions AddPolicy<TKey, TPolicy>(string name, bool global = false) where TPolicy : IRateLimiterPolicy<TKey>
    }

    public static class RateLimiterApplicationBuilderExtensions
    {
        public static IApplicationBuilder UseRateLimiter(this IApplicationBuilder app)
        public static IApplicationBuilder UseRateLimiter(this IApplicationBuilder app, RateLimiterOptions options)
    }

    public static class RateLimiterEndpointConventionBuilderExtensions
    {
        public static TBuilder RequireRateLimiting<TBuilder>(this TBuilder builder, String name) where TBuilder : IEndpointConventionBuilder
    }

    public static class RateLimiterOptionsExtensions
    {
        public static RateLimiterOptions AddTokenBucketRateLimiter(this RateLimiterOptions options, string name, TokenBucketRateLimiterOptions tokenBucketRateLimiterOptions, bool global = false)
        public static RateLimiterOptions AddFixedWindowRateLimiter(this RateLimiterOptions options, string name, FixedWindowRateLimiterOptions fixedWindowRateLimiterOptions, bool global = false)
        public static RateLimiterOptions AddSlidingWindowRateLimiter(this RateLimiterOptions options, string name, SlidingWindowRateLimiterOptions slidingWindowRateLimiterOptions, bool global = false)
        public static RateLimiterOptions AddConcurrencyLimiter(this RateLimiterOptions options, string name, ConcurrencyLimiterOptions concurrencyLimiterOptions, bool global = false)
        public static RateLimiterOptions AddNoLimiter(this RateLimiterOptions options, string name, bool global = false)
    }
}

Activity

self-assigned this
on May 12, 2022
added this to the 7.0-preview6 milestone on May 12, 2022
Kahbazi

Kahbazi commented on Jun 12, 2022

@Kahbazi
Member

Is global = true equivalent to default policy like what there is in Cors?

public void AddDefaultPolicy(CorsPolicy policy)

If not a default policy would be useful. Also there could be a NoRateLimiting method on endpoint too to skip the middleware completely.

wtgodbe

wtgodbe commented on Jun 13, 2022

@wtgodbe
MemberAuthor

Is global = true equivalent to default policy like what there is in Cors?

global = true means the limiter can be shared across endpoints that request it, not that it will be shared across every endpoint. The default is false to avoid users accidentally supplying 2 limiter policies with the same name across 2 different endpoints - we want the default behavior to be that that is 2 separate limiters.

There's not currently a way w/ the runtime APIs to mix endpoint-specific & truly global limiters - if you want a truly global limiter, you can set the Limiter on RateLimiterOptions.

If not a default policy would be useful. Also there could be a NoRateLimiting method on endpoint too to skip the middleware completely.

Good call, just added an extension method for AddNoLimiter

wtgodbe

wtgodbe commented on Jun 13, 2022

@wtgodbe
MemberAuthor

Notes - name params should be policyName
Inline policies #39840 should be in for 7, not necessarily needed for this PR
bool global should be PolicyScope enum, which we can document

  • Actually, don't have global at all - Limiters can not be shared across policies (if 2 policies have same key, we'll disambiguate). Users can apply the same policy to multiple endpoints, which will share limiters across endpoints

Only 1 policy metadata will be resolved on each endpoint - last one wins (imagine applying policy to whole group, then different policy to 1 endpoint in group). Check the endpoint's metadata, see if we have any policy in our last matching that.

Follow-up - allow user to set feature on context that has policy name. Call to .Create checks feature, then endpoint metadata

added
api-ready-for-reviewAPI is ready for formal API review - https://github.com/dotnet/apireviews
on Jun 13, 2022
ghost

ghost commented on Jun 13, 2022

@ghost

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.
halter73

halter73 commented on Jun 13, 2022

@halter73
Member

API Review Notes:

  • We need to see some samples.
  • This should be a diff so we can see what API is new vs which API exists.
  • Is there a way to define a policy inline for the rate limiter?
  • In AddTokenBucketRateLimiter and similar, the string name parameter should be a string policyName parameter.
    • All string name parameters should be string policyName.
  • What about "global = false" what is that about?
    services.AddLimiter("myRolePolicy", context =>
    {
        if (context.IsAdmin()) // Pretend this extension method is defined
        {
            return RateLimitPartion.CreateNoLimiter("admin");
        }
        else
        {
            return RateLimitPartion.CreateTokenBucketLimiter("pleb", //...
        }
    });
    
    services.AddLimiter("myUserNamePolicy", context =>
        RateLimitPartion.Create(httpContext.User.Identity.Name, //...
    • Non-global should be the only option. We don't think it's realistic for different policies to want to share RateLimitPartionKey.
  • Do we want to be able to specify OnRejected per policy? We can already set CustomRejectionStatusCode with IRateLimiterPolicy.
    • Didn't get to this.
  • Can RateLimiterOptions.Limiter be merged with the endpoint-specific stuff? If you don't want to use the endpoint-specific stuff, you just don't define policies or call RequireRateLimiting.
  • Can I use a something on the HttpContext to select a policy name if the request is not hitting an endpoint?
    • Maybe a feature would let you set a string? RateLimterPolicyName. Who would set the feature considering middleware hasn't run yet? We had a similar issue with output caching cc @sebastienros
    • Maybe we could have an Func<HttpContext, string?> RateLimiterOptions.PolicyNameCallback
added
api-needs-workAPI needs work before it is approved, it is NOT ready for implementation
and removed
api-ready-for-reviewAPI is ready for formal API review - https://github.com/dotnet/apireviews
on Jun 13, 2022

27 remaining items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

api-approvedAPI was approved in API review, it can be implementedarea-networkingIncludes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Type

No type

Projects

No projects

Relationships

None yet

    Development

    No branches or pull requests

      Participants

      @halter73@davidfowl@BrennanConroy@amcasey@wtgodbe

      Issue actions

        Make RateLimiterMiddleware endpoint-aware · Issue #41667 · dotnet/aspnetcore