Make RateLimitingMiddleware endpoint aware #42417
Conversation
| @@ -50,6 +50,8 @@ | |||
| <UsingToolNetFrameworkReferenceAssemblies Condition="'$(OS)' != 'Windows_NT'">true</UsingToolNetFrameworkReferenceAssemblies> | |||
| <!-- Disable XLIFF tasks --> | |||
| <UsingToolXliff>false</UsingToolXliff> | |||
| <!-- Use custom version of Roslyn Compiler --> | |||
| <UsingToolMicrosoftNetCompilers>true</UsingToolMicrosoftNetCompilers> | |||
There was a problem hiding this comment.
This is to allow for the required keyword I use in many places in this PR
src/Middleware/RateLimiting/src/RateLimiterEndpointConventionBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
Actually, never mind - we reference |
src/Middleware/RateLimiting/samples/RateLimitingSample/Program.cs
Outdated
Show resolved
Hide resolved
src/Middleware/RateLimiting/src/RateLimiterOptionsExtensions.cs
Outdated
Show resolved
Hide resolved
| var helloName = "helloPolicy"; | ||
|
|
||
| // Define endpoint limiters and a global limiter. | ||
| var options = new RateLimiterOptions() |
There was a problem hiding this comment.
Why isn't this using services?
There was a problem hiding this comment.
Updated the sample to inject an ILogger - anything else you'd like to see?
There was a problem hiding this comment.
I was asking why the options was being newed up instead of a call to AddRateLimiting on builder.Services.
There was a problem hiding this comment.
We removed that extension in the last API review: #37384 (comment), in favor of just the UseRateLimiter(options) extension on IApplicationBuilder. We could bring it back in the next iteration
There was a problem hiding this comment.
OK that doesn't look ideal at all. It doesn't look like any of the other middleware that is configured with this level of policy.
@Tratcher @halter73 @BrennanConroy What was the thinking here?
There was a problem hiding this comment.
I think we need a do over here to align with the patterns we have in the rest of the stack. The newly authored output caching middleware also follows this pattern
There was a problem hiding this comment.
There are a few other places we do it this way:
But I think long term it's probably better to switch to UseRateLimiter on the ServiceCollection w/ an Action. Given that we already shipped this version of the API for preview6, I'd like to get this PR in as-is (modulo the other minor feedback) before the preview7 snap tomorrow, then go through another API review to switch to the action-based approach (as well as a couple other small changes) to be merged later this week
There was a problem hiding this comment.
@davidfowl You're late to the party! You should read my thoughts on this at #41655 (comment) when @Elfocrash made the same point if you haven't yet. We're way more consistent with this options pattern (or should I say lack of options pattern) for middleware with no services than I would have thought. We're rarely this consistent with anything else 🤣.
But I think long term it's probably better to switch to UseRateLimiter on the ServiceCollection w/ an Action. Given that we already shipped this version of the API for preview6, I'd like to get this PR in as-is (modulo the other minor feedback) before the preview7 snap tomorrow, then go through another API review to switch to the action-based approach (as well as a couple other small changes) to be merged later this week
Completely agree.
I do understand no matter how consistent we are with not using the options pattern in this specific scenario, it's rare to use middleware without any default services but with interesting options must people will want to configure. And this does feel awkward when you're more used to configuring options for middleware that consumes services.
Is it likely that UseRateLimiter will need to register default services for .NET 7? If so, configuring options when adding the services like you do for most things would avoid this controversy entirely!
There was a problem hiding this comment.
None of those options are this complex. There's no policy, no methods, just properties on pocos.
| /// <summary> | ||
| /// An interface which can be used to identify a type which provides metadata needed for enabling request rate limiting support. | ||
| /// </summary> | ||
| internal interface IRateLimiterMetadata |
There was a problem hiding this comment.
We talked about this during preview6 API discussions and decided to leave it out for preview6 and talk about making it public in preview7.
src/Middleware/RateLimiting/src/DefaultKeyTypeEqualityComparer.cs
Outdated
Show resolved
Hide resolved
| namespace Microsoft.AspNetCore.RateLimiting; | ||
| internal struct LeaseContext : IDisposable |
There was a problem hiding this comment.
I don't know enough about the subject but does making this struct to readonly ref improve anything!?
|
Looks like the updated compiler doesn't like the
|
src/Middleware/RateLimiting/src/DefaultKeyTypeEqualityComparer.cs
Outdated
Show resolved
Hide resolved
| var helloName = "helloPolicy"; | ||
|
|
||
| // Define endpoint limiters and a global limiter. | ||
| var options = new RateLimiterOptions() |
There was a problem hiding this comment.
@davidfowl You're late to the party! You should read my thoughts on this at #41655 (comment) when @Elfocrash made the same point if you haven't yet. We're way more consistent with this options pattern (or should I say lack of options pattern) for middleware with no services than I would have thought. We're rarely this consistent with anything else 🤣.
But I think long term it's probably better to switch to UseRateLimiter on the ServiceCollection w/ an Action. Given that we already shipped this version of the API for preview6, I'd like to get this PR in as-is (modulo the other minor feedback) before the preview7 snap tomorrow, then go through another API review to switch to the action-based approach (as well as a couple other small changes) to be merged later this week
Completely agree.
I do understand no matter how consistent we are with not using the options pattern in this specific scenario, it's rare to use middleware without any default services but with interesting options must people will want to configure. And this does feel awkward when you're more used to configuring options for middleware that consumes services.
Is it likely that UseRateLimiter will need to register default services for .NET 7? If so, configuring options when adding the services like you do for most things would avoid this controversy entirely!
src/Middleware/RateLimiting/samples/RateLimitingSample/SampleRateLimiterPolicy.cs
Outdated
Show resolved
Hide resolved
src/Middleware/RateLimiting/src/DefaultKeyTypeEqualityComparer.cs
Outdated
Show resolved
Hide resolved
| @@ -14,11 +14,11 @@ public interface IRateLimiterPolicy<TPartitionKey> | |||
| /// <summary> | |||
| /// Gets the <see cref="Func{OnRejectedContext, CancellationToken, ValueTask}"/> that handles requests rejected by this middleware. | |||
| /// </summary> | |||
| public Func<OnRejectedContext, CancellationToken, ValueTask>? OnRejected { get; } | |||
| Func<OnRejectedContext, CancellationToken, ValueTask>? OnRejected { get; } | |||
There was a problem hiding this comment.
The reason it's a Func instead of a method is because of the policy.OnRejected is not null check here. This seems preferrable than adding a bool OverridesOnRejected { get; } to determine whether the policy.
Maybe it's okay if we always run the global OnRejected Func and the policy's OnRejected method though.
ghost
added this to the 
adityamandaleeka
added
the
blog-candidate
|
@wtgodbe, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work! Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers. This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement. Thanks! |
amcasey
added
area-middleware
#41667
Follows the CORS pattern of adding metadata to endpoints w/ an
IEndpointConventionBuilder. Users provide policies, which are aFunc<HttpContext, RateLimitPartition<TKey>>, where aRateLimitPartitionis a func for creating a RateLimiter (used by the runtime to create PartitionedRateLimiters); and a OnRejected function. We provide convenience methods for the built-in RateLimiters in runtime (AddConcurrencyLimiter, etc), which don't support customOnRejectedfuncs. These policies will always apply to endpoints - the user can alsosetaGlobalLimiteron the option which will be applied to every request (and will be chained w/ the endpoint specific policies - global first).Still adding tests & samples, but wanted to open this now to get eyes on it.