rate limit sample by Rick-Anderson · Pull Request #13 · dotnet/AspNetCore.Docs.Samples

Rick-Anderson · 2022-08-09T03:10:11Z

Contributes to dotnet/AspNetCore.Docs#26515
All the supporting MVC and Razor Pages (RP) files were merged in #12 to make it easier to review the relevant code.
When dotnet/aspnetcore#43053 makes it to a preview release, sample will be updated.
Sample lives in this repository. Once code is approved, I'll start working on the doc.

BrennanConroy

We probably also want a sample showing custom IRateLimiterPolicy<T> implementation and usage.

Co-authored-by: Brennan <brecon@microsoft.com>

Rick-Anderson · 2022-08-12T02:35:40Z

Sign in to view

+    }
+
+    public Func<OnRejectedContext, CancellationToken, ValueTask>? 
+                                                         OnRejected { get => _onRejected; }


Is it OK to have OnRejected here and also in RateLimiterOptions directly?

Right, policy OnRejected will win.

Sign in to view

+    }
+
+    public Func<OnRejectedContext, CancellationToken, ValueTask>? 
+                                                         OnRejected { get => _onRejected; }


Co-authored-by: Kahbazi <akahbazi@gmail.com>

Sign in to view

+var builder = WebApplication.CreateBuilder(args);
+var app = builder.Build();
+
+static string GetTicks() => DateTime.Now.Ticks.ToString().Substring(14);


BrennanConroy · 2022-08-13T02:05:44Z

From the browser I get a 200 and the previous response while the output window shows the fail: and callstack.

I assume you're just refreshing the browser page when you get limited? This would cause the request to be aborted by the client. Like Will said, we might want to change the behavior here. MVC often swallows the OCE from client aborts and just makes sure the request is aborted. https://source.dot.net/#Microsoft.AspNetCore.Mvc.Core/FileResultHelper.cs,47

Rick-Anderson · 2022-08-13T18:15:15Z

I assume you're just refreshing the browser page when you get limited?

Yes, I didn't see that behavior when testing with JMeter on an Azure app. After getting that exception in the output window for several days, I can't reproduce that now and get the 429 response I set in OnRejected. The code hasn't changed and I'm on the same 7.0.100-preview.7.22377.5 version.

Kahbazi · 2022-08-13T15:57:31Z

+        }
+
+        context.HttpContext.Response.StatusCode = StatusCodes.Status429TooManyRequests;
+        context?.HttpContext?.RequestServices?.GetService<ILoggerFactory>()?


I don't think theses null checks are necessary.

Suggested change

context?.HttpContext?.RequestServices?.GetService<ILoggerFactory>()?

context.HttpContext.RequestServices.GetService<ILoggerFactory>()

They all have Dereference of possibly null reference.

Kahbazi · 2022-08-13T18:32:43Z

+            return RateLimitPartition.CreateTokenBucketLimiter("Anon", key =>
+                new TokenBucketRateLimiterOptions(tokenLimit: 5,
+                    queueProcessingOrder: QueueProcessingOrder.OldestFirst,
+                    queueLimit: 1,
+                    replenishmentPeriod: TimeSpan.FromSeconds(5),
+                    tokensPerPeriod: 1,
+                    autoReplenishment: true));


How about to use a NoLimiter here? an unauthenticated user could call this endpoint so many times and block all unauthenticated enpoints for everyone.

That's a good point and something I'll call out. A DOS attack is something to consider on all limiters and with no limiter. I think the simplest samples I just added will have hard coded limits, the last two sample will use configuration.

Sign in to view

+        var accessToken = httpContext?.Features?.Get<IAuthenticateResultFeature>()?
+        .AuthenticateResult?.Properties?.GetTokenValue("access_token")?.ToString()


wtgodbe · 2022-08-16T22:36:56Z

dotnet/aspnetcore#43320 should fix the behavior you're seeing w/ TaskCanceledException

Rick-Anderson · 2022-08-16T22:38:19Z

+
+app.Run();
+// </snippet_adm>
+#elif JWT


It's better to also include JWT Auth handler, so it is clear in the sample that where does the "access_token" come form. builder.Services.AddAuthentication().AddJwt(...);

@Kahbazi I made a new sample for this and removed GetTokenValue("access_token") from the other sample. This one is easy to test with dotnet user-jwts

Rick-Anderson · 2022-08-18T22:36:29Z

@wtgodbe @BrennanConroy I've just started the rate limiting middleware article so it would be helpful to get this sample reviewed and merged.

wtgodbe

Seems good other than the 2 API change reactions - it'll also be good to add examples of controllers (using the new attributes) once RC1 comes out.

wtgodbe · 2022-08-18T22:40:10Z

+
+static string GetTicks() => (DateTime.Now.Ticks & 0x11111).ToString("00000");
+
+app.UseRateLimiter(new RateLimiterOptions()


Long term I'd prefer using AddRateLimiter w/ services: https://github.com/dotnet/aspnetcore/blob/4cf483ee7a1cb470b8b2290a174ad34f450a5eab/src/Middleware/RateLimiting/src/RateLimiterServiceCollectionExtensions.cs#L20

wtgodbe · 2022-08-18T22:43:39Z

+app.Configuration.GetSection(MyRateLimitOptions.MyRateLimit).Bind(myOptions);
+
+app.UseRateLimiter(new RateLimiterOptions()
+    .AddNoLimiter(policyName: jwtPolicyName)


What's the intention here? Adding a no limiter is functionally equivalent to not adding any endpoint limiter for this endpoint - the global limiter will still run. We're removing AddNoLimiter in rc1 (we are also adding an attribute that allows you to disable rate limiting entirely for an endpoint, including the global limiter)

wtgodbe · 2022-08-18T22:44:02Z

+    }
+
+    public Func<OnRejectedContext, CancellationToken, ValueTask>? 
+                                                         OnRejected { get => _onRejected; }


Right, policy OnRejected will win.

Rick-Anderson · 2022-08-19T06:00:42Z

+    private readonly MyRateLimitOptions _options;
+
+    public SampleRateLimiterPolicy(ILogger<SampleRateLimiterPolicy> logger,
+                                   IOptions<MyRateLimitOptions> options)


@Kahbazi I'm now passing in IOptions so I can use config for setting the values.

Rick-Anderson · 2022-08-19T06:02:54Z

+    //.AddPolicy<string>(completePolicyName, 
+    //           new SampleRateLimiterPolicy(NullLogger<SampleRateLimiterPolicy>.Instance,
+    //           (IOptions < MyRateLimitOptions >)myConfigSection))


@Kahbazi how do I pass IOptions<MyRateLimitOptions> here?

@Kahbazi Is there any advantage to showing new SampleRateLimiterPolicy()? Maybe we should just use .AddPolicy<string, SampleRateLimiterPolicy>(helloPolicy) like we do on the next line.

how do I pass IOptions here?

When you have the MyRateLimitOptions instance, you can create it with Microsoft.Extensions.Options.Options.Create<MyRateLimitOptions>(options).

@Kahbazi Is there any advantage to showing new SampleRateLimiterPolicy()? Maybe we should just use .AddPolicy<string, SampleRateLimiterPolicy>(helloPolicy) like we do on the next line.

Yeah, I agree. Having just .AddPolicy<string, SampleRateLimiterPolicy>(helloPolicy) for sample is enough I think.

rate limit sample

31b66ce

Rick-Anderson requested review from BrennanConroy and wtgodbe August 9, 2022 03:22

Rick-Anderson mentioned this pull request Aug 9, 2022

Rate limit middleware /5 dotnet/AspNetCore.Docs#26623

Closed

Kahbazi reviewed Aug 9, 2022

View reviewed changes

Comment thread fundamentals/middleware/rate-limit/WebRateLimitAuth/Program.cs Outdated

Comment thread fundamentals/middleware/rate-limit/WebRateLimitAuth/Program.cs Outdated

BrennanConroy reviewed Aug 9, 2022

View reviewed changes

Comment thread fundamentals/middleware/rate-limit/WebRateLimitAuth/Program.cs Outdated

Update fundamentals/middleware/rate-limit/WebRateLimitAuth/Program.cs

11c1b6c

Co-authored-by: Brennan <brecon@microsoft.com>

This comment was marked as resolved.

Sign in to view

Rick-Anderson added 2 commits August 9, 2022 13:57

react to feedback

daab3ce

react to feedback

a1728c8

Rick-Anderson commented Aug 12, 2022

View reviewed changes

Comment thread fundamentals/middleware/rate-limit/WebRateLimitAuth/SampleRateLimiterPolicy.cs

Rick-Anderson commented Aug 12, 2022

View reviewed changes

This comment was marked as off-topic.

Sign in to view

react to feedback

c509983

Kahbazi reviewed Aug 12, 2022

View reviewed changes

Update fundamentals/middleware/rate-limit/WebRateLimitAuth/Program.cs

46f7db8

Co-authored-by: Kahbazi <akahbazi@gmail.com>

This comment was marked as resolved.

Sign in to view

Rick-Anderson requested a review from BrennanConroy August 12, 2022 22:30

react to feedback

e7ae186

Rick-Anderson commented Aug 13, 2022

View reviewed changes

Rick-Anderson added 4 commits August 12, 2022 16:08

react to feedback

8ec8545

react to feedback

f0712e9

react to feedback

b305e04

react to feedback

7125e80

Kahbazi reviewed Aug 13, 2022

View reviewed changes

Rick-Anderson added 4 commits August 15, 2022 16:05

react to feedback

76a5301

react to feedback

b76a0ef

react to feedback

e78945c

react to feedback

4acdc05

This comment was marked as resolved.

Sign in to view

Rick-Anderson commented Aug 16, 2022

View reviewed changes

Rick-Anderson mentioned this pull request Aug 17, 2022

Rate limiting /1 dotnet/AspNetCore.Docs#26756

Merged

wtgodbe approved these changes Aug 18, 2022

View reviewed changes

Rick-Anderson mentioned this pull request Aug 18, 2022

write sample using service for rate limit #14

Closed

react to feedback

257f38f

Rick-Anderson commented Aug 19, 2022

View reviewed changes

react to feedback

30344b5

Rick-Anderson merged commit c563496 into main Aug 19, 2022

Rick-Anderson deleted the rl2 branch August 19, 2022 23:31

	context?.HttpContext?.RequestServices?.GetService<ILoggerFactory>()?
	context.HttpContext.RequestServices.GetService<ILoggerFactory>()

		var accessToken = httpContext?.Features?.Get<IAuthenticateResultFeature>()?
		.AuthenticateResult?.Properties?.GetTokenValue("access_token")?.ToString()


		static string GetTicks() => (DateTime.Now.Ticks & 0x11111).ToString("00000");

		app.UseRateLimiter(new RateLimiterOptions()

Conversation

Rick-Anderson commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BrennanConroy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

BrennanConroy commented Aug 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rick-Anderson commented Aug 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

wtgodbe commented Aug 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rick-Anderson commented Aug 18, 2022

Uh oh!

wtgodbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Rick-Anderson commented Aug 9, 2022 •

edited

Loading

BrennanConroy commented Aug 13, 2022 •

edited

Loading