-
Notifications
You must be signed in to change notification settings - Fork 1.7k
#2138 Introduce ASP.NET rate limiting #2188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
#2138 Introduce ASP.NET rate limiting #2188
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for PR creation!
Please move current test/Ocelot.IntegrationTests/RateLimitingTests to Ocelot.AcceptanceTests project, we already have a suitable class: ClientRateLimitingTests. However, these tests only cover the Ocelot feature "Rate Limit by Client's Header." Since you are proposing a new feature, a new testing class should be created.
P.S. I recommend (and require) inheriting from Steps, which offers many useful testing helpers, so there's no need to reinvent the wheel.
|
Yhlas, thank you once again. Here's the current status: I will request an official code review after transitioning tests to acceptance testing. Please remember to draft the preliminary docs. Regarding the delivery vision, I cannot include it in the current Autumn'24 milestone due to anticipated multiple development rounds. Post-release, our team intends to upgrade Ocelot to .NET 9. It appears your feature may be incorporated into the release alongside this upgrade. Therefore, I suggest testing the PR code with the current .NET 9 RC. Is that okay? Consequently, we will target two frameworks in the project files: |
I targeted Ocelot project and a sample to net 9.0-rc. No issue on manual testing. I will start working on docs for this in a few days. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can enhance the quality of the PR: my suggestions are outlined below 👇
FYI, I will revisit this PR once it has been reviewed by a team member as well.
P.S. Keep in mind Line-Ending Gotchas aka EOL Fun. I saw some files have fake changes because of changed line endings.
test/Ocelot.UnitTests/Configuration/Validation/RouteFluentValidatorTests.cs
Outdated
Show resolved
Hide resolved
|
@raman-m That's a great idea, but we are stuck with the .NET rate limiters specs, you can't modify them on the fly, as you could with the current limiters. The rate limiters policies must be known, and therefore configured during startup |
@ggnaegi, what is your suggestion? |
…Context` response accessed via `IHttpContextAccessor` (ThreeMammals#1307) * set rate limiting headers on the proper httpcontext * fix Retry-After header * merge fix * refactor of ClientRateLimitTests * merge fix * Fix build after rebasing * EOL: test/Ocelot.AcceptanceTests/Steps.cs * Add `RateLimitingSteps` * code review by @raman-m * Inject IHttpContextAccessor, not IServiceProvider * Ocelot's rate-limiting headers have become legacy * Headers definition life hack * A good StackOverflow link --------- Co-authored-by: Jolanta Łukawska <jolanta.lukawska@outlook.com> Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>
Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>
59ef966 to
e8987ad
Compare
908d84f to
0678e7a
Compare
|
@yjorayev Hi Yhlas! If you have no objections, I would like to invite you to review the following PR →
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Yhlas!
Do you recall what you were doing a year ago? Do you plan to continue contributing further?
With the merge conflicts resolved and #2294 successfully integrated, it seems like the perfect time to move forward with development. Unfortunately, the acceptance test is currently failing.
| builder.Services.AddRateLimiter(op => | ||
| { | ||
| op.AddFixedWindowLimiter(policyName: "fixed", options => | ||
| { | ||
| options.PermitLimit = 2; | ||
| options.Window = TimeSpan.FromSeconds(12); | ||
| options.QueueProcessingOrder = QueueProcessingOrder.OldestFirst; | ||
| options.QueueLimit = 0; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree! In this case, we haven't proposed anything new.
I think we should provide the user with an option to choose between configuration approaches. We could consider having a separate Ocelot rate-limiting rule to manage app auto-configurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raman-m it's basically what i mentioned:
Yes, that's the main concern; we essentially need to reinvent the wheel for the middleware part. I've tried to "dynamize" the policies myself, but I believe it’s not possible.
The policies are setup during startup, you can't modify them after that, and you can't mix them (as of 2024, maybe now it's possible).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't mix policies, but we can define as many as needed, right?
From your words, did you mean that the "fixed" policy is global in this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's global and they can't be modified.
| /// <value> | ||
| /// A string of rate limit policy name. | ||
| /// </value> | ||
| public string Policy { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to consider removing this property and using FileRateLimitByAspNetRule.Policy instead.
| When(route => route.RateLimitOptions != null && route.RateLimitOptions.EnableRateLimiting != false, () => | ||
| When(route => route.RateLimitOptions != null | ||
| && route.RateLimitOptions.EnableRateLimiting != false | ||
| && string.IsNullOrWhiteSpace(route.RateLimitOptions.Policy), () => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to validate the rule algorithm properties, even if the ASP.NET policy is in place.
| fromRule.Period.IfEmpty(RateLimitRule.DefaultPeriod), | ||
| fromRule.PeriodTimespan.HasValue ? $"{fromRule.PeriodTimespan.Value}s" : fromRule.Wait, | ||
| fromRule.Limit ?? RateLimitRule.ZeroLimit); | ||
| Policy = fromRule.Policy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An option of a flawed rule 😄 Consider removing!
| QuotaMessage = fromOptions.QuotaMessage.IfEmpty(DefaultQuotaMessage); | ||
| KeyPrefix = fromOptions.KeyPrefix.IfEmpty(DefaultCounterPrefix); | ||
| Rule = fromOptions.Rule ?? RateLimitRule.Empty; | ||
| Policy = fromOptions.Policy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine because of the copy constructor ✔️
| var globalRateLimitOptions = configurationRoot.Get<FileConfiguration>()?.GlobalConfiguration?.RateLimitOptions; | ||
| var rejectStatusCode = globalRateLimitOptions?.HttpStatusCode ?? StatusCodes.Status429TooManyRequests; | ||
| var rejectedMessage = globalRateLimitOptions?.QuotaExceededMessage ?? "API calls quota exceeded!"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging in the wrong place can be risky.
We've assigned the IRateLimitOptionsCreator service specifically for merging options. Please refer to the RateLimitOptionsCreator class for details.
| rejectedContext.HttpContext.Response.StatusCode = rejectStatusCode; | ||
| await rejectedContext.HttpContext.Response.WriteAsync(rejectedMessage, token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's amusing that we implement the same approach in Ocelot's middleware. Perhaps adding a virtual method in the middleware to connect to the handler could work? 😉
|
|
||
| Services.AddRateLimiting(); // Feature: Rate Limiting | ||
| #if NET7_0_OR_GREATER | ||
| Services.AddAspNetRateLimiting(configurationRoot); // Feature: AspNet Rate Limiting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I strongly believe we shouldn't introduce a new feature, whether public or internal. Since the method is already auto-enabled internally, it seems we can leave it as-is. However, we definitely need to review this design.
|
|
||
| public static class RateLimitingMiddlewareExtensions | ||
| { | ||
| public static IApplicationBuilder UseRateLimiting(this IApplicationBuilder builder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| [Fact] | ||
| [Trait("Feat", "2138")] | ||
| public async Task Should_RateLimit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, this test fails ❗
|
@ggnaegi commented on Nov 14, 2024
We don't need to reinvent anything, just a useful JSON configuration. Ocelot/src/Ocelot/Middleware/OcelotPipelineExtensions.cs Lines 77 to 80 in d5e6d9a
Place it after DownstreamRequestInitialiserMiddleware but before RateLimitingMiddleware. Inheriting from the ASP.NET Core one might seem like a crazy idea, but it’s definitely possible!
Since we seem to agree on independent evolution, we should avoid replacing or combining anything. Instead, we need to design an effective embedded module using JSON configurations, supplemented by additional C# helpers if necessary. |

Closes #2138
Discussion thread
Proposed Changes
ocelot.json. It defaults to Ocelot's existing rate limiter if not specifiedDocumentation block
Predecessor
We need to merge this PR first due to the agreement on the JSON schema, which includes global configuration and specific
Metadataoptions.