API additions for RateLimitingMiddleware

[wtgodbe]

NOTE - This is the original draft of the proposal. The updated proposal is at this comment: #42667 (comment)

namespace Microsoft.AspNetCore.RateLimiting

+    public interface IRateLimiterMetadata
+    {
+    }

+    public interface IRequireRateLimiterMetadata : IRateLimiterMetadata
+    {
+        string PolicyName { get; }
+    }

+    public class RequireRateLimiterMetadata : IRequireRateLimiterMetadata 
+    {
+        public RequireRateLimiterMetadata (string policyName)
+        public string PolicyName { get; }
+    }

+    public interface IDisableRateLimiterMetadata : IRateLimiterMetadata
+    {
+    }

+    public class DisableRateLimiterMetadata : IDisableRateLimiterMetadata 
+    {
+        public RequireRateLimiterMetadata ()
+    }

    public interface IRateLimiterPolicy<TPartitionKey>
    {
-        Func<OnRejectedContext, CancellationToken, ValueTask>? OnRejected { get; }
+        ValueTask OnRejected(OnRejectedContext onRejectedContext, CancellationToken cancellationToken)
    }

namespace Microsoft.AspNetCore.Builder

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

+   public static class RateLimiterServiceCollectionExtensions
+    {
+        public static IServiceCollection AddRateLimiter(this IServiceCollection services, Action<RateLimiterOptions > configureOptions)
+    }

    public static class RateLimiterOptionsExtensions
    {
-        public static RateLimiterOptions AddNoLimiter(this RateLimiterOptions options, string policyName)
    }

    public static class RateLimiterEndpointConventionBuilderExtensions
    {
+        public static TBuilder DisableRateLimiting<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder
    }

Q's - keep OnRejected as a Func because it's nullable? Change extension methods like AddTokenBucketLimiter to AddTokenBucketLimiterPolicy? Add an attribute for MVC Controllers?

[BrennanConroy]

I like the idea of skipping the middleware suggested in #41667 (comment)

[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]

Meeting notes:

• Let's get rid of the IRateLimiterMetadata inheritance hiearcy.

  • Rename IRequireRateLimiterMetadata to IRateLimiterMetadata. And RequireRateLimiterMetadata to RateLimiterMetadata.
  • IDisableRateLimiterMetadata stays the same name.

• Should we allow policies defined inline?

  • It seems straightforward enough to implement well with the little time we have and it would be convenient.

• UseRateLimiter(this IApplicationBuilder app, RateLimiterOptions options) should stay. We should still add AddRateLimiter. This is the same pattern as AddWebSockets.

• Let's keep Func<OnRejectedContext, CancellationToken, ValueTask>? OnRejected { get; } so we know whether or not to call the global OnRejected callback. If we make it a normal method instead, we'd have to be okay with okay with always running the global OnRejected first.

• Let's add attributes!

  • How about [RateLimit("MyPolicy")]?
  • Anything else?
    • Not at the moment. In the future (post .NET 7), it might be nice to specify rate limiting options directly via endpoint metadata though.

• Let's put everything in the ASP.NET Core shared framework. We'll have to include System.Threading.RateLimiting in the ASP.NET Core shared framework similar to System.IO.Pipelines.

• One last curveball from @DamianEdwards: Why can't I configure the rate limiter options (e.g. TokenBucketRateLimiterOptions) with a callback like every other "option" type?

  • The properties are read-only. You need to use the constructor.
  • Let's fix that and add setters and a default ctor to all the options types. It will have to go through runtime API review.
  • We'll also have to update methods like AddTokenBucketLimiter to take an Action<TokenBucketRateLimiterOptions> instead of TokenBucketRateLimiterOptions directly.

[davidfowl]

This looks great so far 😄

[Kahbazi]

IDisableRateLimiterMetadata stays the same name.

How about this one:

+    public interface IDisableRateLimiterMetadata
+    {
+        bool IsDisabled { get; }
+    }

This is according to the Guidelines and can be overriden if necessary.

How about [RateLimit("MyPolicy")]?

Also [DisableRateLimit]?

We'll also have to update methods like AddTokenBucketLimiter to take an Action instead of TokenBucketRateLimiterOptions directly.

Can it be Action<TokenBucketRateLimiterOptions, HttpContext>? This way the options could be based on the custom metadata on an endpoint.

[davidfowl]

Agree with @Kahbazi on the disable attribute. We need attributes for anything we have methods for.

[DamianEdwards]

Why does the IDisableRateLimiterMetadata need a property at all? Shouldn't it simply be a marker, like IAllowAnonymous?

[Kahbazi]

Why does the IDisableRateLimiterMetadata need a property at all? Shouldn't it simply be a marker, like IAllowAnonymous?

I'm just quoting the Guidance which exists in ASP.NET Core Docs

The best way to follow these guidelines is to avoid defining marker metadata:

Don't just look for the presence of a metadata type.
Define a property on the metadata and check the property.

And this could be done if the metadata has a property.

public class SuppressDisableateLimitAttribute : Attribute, IDisableRateLimiterMetadata 
{
    public bool IsDisabled => false;
}

[DisableRateLimit]
public class MyController : Controller
{
    public void Unlimit() { }

    [SuppressDisableateLimit]
    public void Limited() { }
}

[davidfowl]

This example applies to route groups as well

[DamianEdwards]

@Kahbazi hmm, do we have an example of us actually doing that somewhere in the framework? We don't follow that for IAuthorizeData AFAIK, i.e. we simply have IAllowAnonymous that circumvents anything defined via IAuthorizeData in metadata.

In your example, am I understanding correctly that this is supposed to facilitate re-enabling rate limiting when other metadata for the endpoint already disables it? How do we do that in the AuthZ system? The code seems to suggest we just ignore any AuthZ failures if the allow anonymous metadata is present at all.

[Kahbazi]

@DamianEdwards You are right. This is not how it's done today in AUTH or CORS. Now if there's an IAllowAnonymous or IDisableCorsAttribute metadata on an endpoint there's no way to negate its effect.
Although this is not a bad thing to have. I suggest add this to Auth and Cors as well. It could be achieve without breaking changes using default interface methods.

[wtgodbe]

Runtime API proposal: dotnet/runtime#72389

[wtgodbe]

namespace Microsoft.AspNetCore.RateLimiting

+    public interface IRateLimiterMetadata
+    {
+        string PolicyName { get; }
+    }

+    public class RateLimiterMetadata : IRateLimiterMetadata 
+    {
+        public RequireRateLimiterMetadata (string policyName)
+        public string PolicyName { get; }
+    }

+    public interface IDisableRateLimiterMetadata
+    {
+    }

+    public class DisableRateLimiterMetadata : IDisableRateLimiterMetadata 
+    {
+        public DisableRateLimiterMetadata ()
+    }

+    public class EnableRateLimitingAttribute : Attribute, IRateLimiterMetadata
+    {
+        public EnableRateLimitingAttribute (string policyName)
+        string PolicyName { get; }
+    }

+    public class DisableRateLimitingAttribute : Attribute, IDisableRateLimiterMetadata
+    {
+        public DisableRateLimitingAttribute ()
+    }

+    public interface IRateLimiterPolicyMetadata<TPartitionKey>
+    {
+        IRateLimiterPolicy<TPartitionKey> Policy { get; }
+    }

+    public class RateLimiterPolicyMetadata<TPartitionKey>  : IRateLimiterPolicyMetadata<TPartitionKey> 
+    {
+        public RateLimiterPolicyMetadata (IRateLimiterPolicy<TPartitionKey> policy)
+        public IRateLimiterPolicy<TPartitionKey> Policy { get; }
+    }

namespace Microsoft.AspNetCore.Builder

+   public static class RateLimiterServiceCollectionExtensions
+    {
+        public static IServiceCollection AddRateLimiter(this IServiceCollection services, Action<RateLimiterOptions > configureOptions)
+    }

    public static class RateLimiterOptionsExtensions
    {
-       public static RateLimiterOptions AddNoLimiter(this RateLimiterOptions options, string policyName)
-       public static RateLimiterOptions AddTokenBucketLimiter(this RateLimiterOptions options, string policyName, TokenBucketRateLimiterOptions tokenBucketRateLimiterOptions)
+       public static RateLimiterOptions AddTokenBucketLimiter(this RateLimiterOptions options, string policyName, Action<TokenBucketRateLimiterOptions> configureOptions)
-       public static RateLimiterOptions AddFixedWindowLimiter(this RateLimiterOptions options, string policyName, FixedWindowRateLimiterOptions fixedWindowRateLimiterOptions)
+       public static RateLimiterOptions AddFixedWindowLimiter(this RateLimiterOptions options, string policyName, Action< FixedWindowRateLimiterOptions> configureOptions)
-       public static RateLimiterOptions AddSlidingWindowLimiter(this RateLimiterOptions options, string policyName, SlidingWindowRateLimiterOptions slidingWindowRateLimiterOptions)
+       public static RateLimiterOptions AddSlidingWindowLimiter(this RateLimiterOptions options, string policyName, Action< SlidingWindowRateLimiterOptions> configureOptions)
-       public static RateLimiterOptions AddConcurrencyLimiter(this RateLimiterOptions options, string policyName, ConcurrencyLimiterOptions concurrencyLimiterOptions)
+       public static RateLimiterOptions AddConcurrencyLimiter(this RateLimiterOptions options, string policyName, Action< ConcurrencyLimiterOptions> configureOptions)
    }

    public static class RateLimiterEndpointConventionBuilderExtensions
    {
+       public static TBuilder DisableRateLimiting<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder
+       public static TBuilder RequireRateLimiting<TBuilder, TPartitionKey>(this TBuilder builder, IRateLimiterPolicy<TPartitionKey> policy) where TBuilder : IEndpointConventionBuilder
    }
        

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

[Kahbazi]

1. What's the difference between IEnableRateLimitingAttribute and IRateLimiterMetadata? Isn't one metadata interface for enabling rate limiting is enough? The same thing goes for IDisableRateLimitingAttribute and IDisableRateLimiterMetadata.
2. How about remove the Attribute suffix from the interface metadata? IDisableRateLimitingAttribute -> IDisableRateLimiting. IEnableRateLimitingAttribute -> IEnableRateLimiting
3. What would happen if there are multiple IRateLimiterPolicyMetadata<TPartitionKey> on an endpoint?
4. @DamianEdwards So it's a No on adding IsDisabled to the metadata?

[wtgodbe]

What's the difference between IEnableRateLimitingAttribute and IRateLimiterMetadata? Isn't one metadata interface for enabling rate limiting is enough? The same thing goes for IDisableRateLimitingAttribute and IDisableRateLimiterMetadata.

Yes, the only difference is that one is an attribute. We can probably combine them as you suggested in 2.

What would happen if there are multiple IRateLimiterPolicyMetadata on an endpoint?

Right now, the first one added gets used. We don't yet throw an exception when multiple get added to the same endpoint, but we could start doing so here:

aspnetcore/src/Middleware/RateLimiting/src/RateLimiterEndpointConventionBuilderExtensions.cs

Lines 25 to 28 in 1380236

	builder.Add(endpointBuilder =>
	{
	endpointBuilder.Metadata.Add(new RateLimiterMetadata(policyName));
	});

Though that gets less consistent when the metadata & attribute are combined.

So it's a No on adding IsDisabled to the metadata?

I'm leaning towards following the CORS pattern, but I'm open to the change. We'll discuss in API review.

[davidfowl]

The attribute should be a concrete type not an interface so I assume it should be:

public class EnableRateLimitingAttribute : Attribute, IRateLimiterMetadata 

[Kahbazi]

+ public static RateLimiterOptions AddTokenBucketLimiter(this RateLimiterOptions options, string policyName, Action<TokenBucketRateLimiterOptions> configureOptions)
+ public static RateLimiterOptions AddTokenBucketLimiter(this RateLimiterOptions options, string policyName, Action<TokenBucketRateLimiterOptions, HttpContext> configureOptions)

Also I think it's useful to have an overload with HttpContext as well. It could be used to set the rate limiter options based on the current endpoint.

rateLimiterOptions.AddTokenBucketLimiter("policy", (options, context) => 
{
    options.TokensPerPeriod = context.GetEndpoint().Metadata.GetMetadata<MyLimiterMetadata>().TokensPerPeriod;
};

[halter73]

API Review Notes:

• Do we need interfaces and non-attribute classes for metadata.
  • Not technically, but we have interfaces for both CORS and Auth, so we're breaking the pattern here if we don't.
  • That said, it's not clear whether we prefer an interface hierarchy or a bool property to indicate what's disabled.
  • If we decide how the interfaces should work, we could add them later to the attributes.
  • DisableRateLimitingAttribute always wins if present
  • Let us seal the attributes as well.
• RateLimiterPolicyMetadata<TPartitionKey> attribute?
  • Attributes don't make sense when you're specifying an instance.
  • Let us remove RateLimiterPolicyMetadata<TPartitionKey> for now and give the policy an unspeakable name and reference it using EnableRateLimitingAttribute.
• We like the IServiceCollection Add* extension method for configuring options
  • We still allow configuring options via UseRateLimiting(options). We will prefer the more local options if specified.
• We changed the rate limiting options type to be mutable so they can follow the more conventional pattern of configuring options in ASP.NET
• Is RequireRateLimiting<TBuilder, TPartitionKey> going to require specifying both generic parameters all the time?
  • Since the TPartitionKey can be inferred from the policy instance we pass in, this shouldn't be necessary.

namespace Microsoft.AspNetCore.RateLimiting
+    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
+    public sealed class EnableRateLimitingAttribute : Attribute
+    {
+        public EnableRateLimitingAttribute(string policyName);
+        string PolicyName { get; }
+    }
+
+    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
+    public sealed class DisableRateLimitingAttribute : Attribute
+    {
+        public DisableRateLimitingAttribute();
+    }

namespace Microsoft.AspNetCore.Builder

+   public static class RateLimiterServiceCollectionExtensions
+    {
+        public static IServiceCollection AddRateLimiter(this IServiceCollection services, Action<RateLimiterOptions> configureOptions);
+    }

    public static class RateLimiterOptionsExtensions
    {
-       public static RateLimiterOptions AddNoLimiter(this RateLimiterOptions options, string policyName)
-       public static RateLimiterOptions AddTokenBucketLimiter(this RateLimiterOptions options, string policyName, TokenBucketRateLimiterOptions tokenBucketRateLimiterOptions)
+       public static RateLimiterOptions AddTokenBucketLimiter(this RateLimiterOptions options, string policyName, Action<TokenBucketRateLimiterOptions> configureOptions)
-       public static RateLimiterOptions AddFixedWindowLimiter(this RateLimiterOptions options, string policyName, FixedWindowRateLimiterOptions fixedWindowRateLimiterOptions)
+       public static RateLimiterOptions AddFixedWindowLimiter(this RateLimiterOptions options, string policyName, Action< FixedWindowRateLimiterOptions> configureOptions)
-       public static RateLimiterOptions AddSlidingWindowLimiter(this RateLimiterOptions options, string policyName, SlidingWindowRateLimiterOptions slidingWindowRateLimiterOptions)
+       public static RateLimiterOptions AddSlidingWindowLimiter(this RateLimiterOptions options, string policyName, Action< SlidingWindowRateLimiterOptions> configureOptions)
-       public static RateLimiterOptions AddConcurrencyLimiter(this RateLimiterOptions options, string policyName, ConcurrencyLimiterOptions concurrencyLimiterOptions)
+       public static RateLimiterOptions AddConcurrencyLimiter(this RateLimiterOptions options, string policyName, Action< ConcurrencyLimiterOptions> configureOptions)
    }

    public static class RateLimiterEndpointConventionBuilderExtensions
    {
+       public static TBuilder DisableRateLimiting<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder
+       public static TBuilder RequireRateLimiting<TBuilder, TPartitionKey>(this TBuilder builder, IRateLimiterPolicy<TPartitionKey> policy) where TBuilder : IEndpointConventionBuilder
    }

[wtgodbe]

Also I think it's useful to have an overload with HttpContext as well. It could be used to set the rate limiter options based on the current endpoint.

We think that this isn't a broad enough use case to merit its addition at the moment, though we could be open to it in the future. It would also add complexity in the way we instantiate RateLimiters/Policies, since for the ones added in this way, we'd have to wait until we were in the middleware to do so.

[wtgodbe]

I'm proposing a slight change to the approved API.

namespace Microsoft.AspNetCore.RateLimiting
     [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
     public sealed class EnableRateLimitingAttribute : Attribute
     {
        public EnableRateLimitingAttribute(string policyName);
-        string PolicyName { get; }
+        string? PolicyName { get; }
     }

By putting an EnableRateLimitingAttribute on the endpoint with a null PolicyName, and setting an internal DefaultRateLimiterPolicy field on that EnableRateLimitingAttribute. PolicyName will only ever be null when we go through this code path – we’ll still throw if the user tries to set the PolicyName as null.

Another option which I just thought of would be to not make PolicyName nullable & have RequireRateLimiting<TBuilder, TPartitionKey> set a new EnableRateLimitingAttribute with some arbitrary PolicyName like the empty string – the presence of a DefaultRateLimiterPolicy on the EnableRateLimitingAttribute should be sufficient to tell the middleware that the attribute came thru this code path, and to just ignore the PolicyName.

[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]

I think the nullability change makes sense here. Someone could easily name their policy "DefaultRateLimiterPolicy", and now there might be some confusion about what policy that actually means. We could just a more obscure name and prevent anyone from adding a conflicting policy name, but I prefer making it clear that a policy can be unnamed.

I think we should approve this API.

[halter73]

API review Notes:

• We prefer nullability over a well-known name because it makes it clearer these polices haven't been given a real name.
• We don't need to change the constructor because we need the extension method to add an inline policy. This is not supported directly with the attribute.

API Approved! (Diff from previously approved API)

namespace Microsoft.AspNetCore.RateLimiting

 [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
 public sealed class EnableRateLimitingAttribute : Attribute
 {
    public EnableRateLimitingAttribute(string policyName);

-    string PolicyName { get; }
+    string? PolicyName { get; }
 }