#2138 Introduce ASP.NET rate limiting

[yjorayev]

New Feature

Closes #2138

• When will ASP.NET Core Implementation of Rate Limiter start? #2138

Discussion thread

• When will ASP.NET Core Implementation of Rate Limiter start? #2138 (comment)

Proposed Changes

• Includes changes to use .Net middleware
• Middleware type for rate limiting can be specified in ocelot.json. It defaults to Ocelot's existing rate limiter if not specified
• Also added some Unit Tests, Integration test and a sample

Documentation block

• RST sources: Rate Limiting | Future and ASP.NET Implementation
• Html docs: Rate Limiting | Future and ASP.NET Implementation

[raman-m]

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.

[raman-m]

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: net8.0 and net9.0

[yjorayev]

... 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: net8.0 and net9.0

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!

[raman-m]

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.

[raman-m]

How do you use it? Is this built-in Visual Studio feature to call APIs?

[yjorayev]

Little "play" icon shows near each request definition that can be clicked to send http request. Below screenshot is from Rider, but VS has this feature too.

Docs for .http

[raman-m]

But this is Rider IDE! Thanks for the link!
I will learn the same feature for dev productivity in Visual Studio...

[raman-m]

Interface Breaking Change❗

Can old Ocelot applications compile successfully with this change?

[yjorayev]

I will add a new method instead of overriding existing one. I assumed this method is for internal purposes, but now I realize that it was a wrong assumption given that this method is public.

[raman-m]

The decision hinges on the ultimate logic! Perhaps separating the internal extension methods is not essential.
I advise to revisit this once all the logic has been developed.

[raman-m]

Generally, it should be acceptable; however, I would advise separating the setup for each rate limiting mode.

[yjorayev]

separated

[raman-m]

Good, but it can be encapsulated in UseRateLimiting method (line 80), right?

[yjorayev]

done

[ggnaegi]

@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

[raman-m]

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.

@ggnaegi, what is your suggestion?
Could you please clarify the problem for the author?
Why not conduct a separate code review yourself?

[raman-m]

Unfortunately, merge conflicts have arisen due to the recent merging of PR #1307 and its commit da9d6fa into the develop branch. I recommend to perform rebasing.

[ggnaegi]

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.

@ggnaegi, what is your suggestion? Could you please clarify the problem for the author? Why not conduct a separate code review yourself?

@yjorayev @raman-m
I'am wondering if we could replace all the current rate limiters in the request pipeline, like this (haven't checked), then it would make sense imho:

public class CustomRateLimiterMiddleware
{
    private readonly RequestDelegate _next;
    private readonly TokenBucketRateLimiter _rateLimiter;

    public CustomRateLimiterMiddleware(RequestDelegate next)
    {
        _next = next;
        
        // Create a rate limiter with a custom configuration
        _rateLimiter = new TokenBucketRateLimiter(
            new TokenBucketRateLimiterOptions
            {
                TokenLimit = 10,
                QueueProcessingOrder = QueueProcessingOrder.OldestFirst,
                QueueLimit = 5,
                ReplenishmentPeriod = TimeSpan.FromSeconds(1),
                TokensPerPeriod = 1,
                AutoReplenishment = true
            });
    }

    public async Task InvokeAsync(HttpContext context)
    {
        // Attempt to lease a token for each request
        var lease = await _rateLimiter.WaitAsync();

        if (lease.IsAcquired)
        {
            try
            {
                await _next(context);
            }
            finally
            {
                lease.Dispose();
            }
        }
        else
        {
            context.Response.StatusCode = StatusCodes.Status429TooManyRequests;
        }
    }
}

[yjorayev]

@ggnaegi, @raman-m I think we can make this work. Couple things:

1. We will need to be able to get rate limiter policy by name. Simple dictionary might suffice.
2. If we go this route, we might find ourselves re-implementing(copying?) CombinedAcquire and CombinedWaitAsync methods of RateLimitingMiddleware.

What do you think?

[ggnaegi]

@ggnaegi, @raman-m I think we can make this work. Couple things:

1. We will need to be able to get rate limiter policy by name. Simple dictionary might suffice.
2. If we go this route, we might find ourselves re-implementing(copying?) CombinedAcquire and CombinedWaitAsync methods of RateLimitingMiddleware.

What do you think?

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. However, at least we can retain Microsoft's RateLimiter implementations and replace the current ones.

I’d go down that path, and if you look at the code, there isn’t much to modify.

@yjorayev @raman-m

[raman-m]

@ggnaegi @yjorayev

Hello, guys! What's up?

Has .NET 9 been released already? Did I miss the event? 😄

@yjorayev Feel free to request the 2nd code review if you're ready.

[raman-m]

@yjorayev commented on Nov 14

1. We will need to be able to get rate limiter policy by name. Simple dictionary might suffice.

No objections!

2. If we go this route, we might find ourselves re-implementing(copying?) CombinedAcquire and CombinedWaitAsync methods of RateLimitingMiddleware.

I don't recommend copying and pasting code from the MS codebase for long-term purposes because the internal implementation can change with .NET version updates. However, it can be acceptable in the short term until we develop a more practical design solution based on public methods.

Let's hack this private method?! 😎 Why not to call it with a help of reflection technology? 🙊

[yjorayev]

I don't recommend copying and pasting code from the MS codebase for long-term purposes because the internal implementation can change with .NET version updates. However, it can be acceptable in the short term until we develop a more practical design solution based on public methods.

Let's hack this private method?! 😎 Why not to call it with a help of reflection technology? 🙊

I disagree about calling private methods, because private method signature can(probably will) change even with minor version upgrades.

What are the advantages of calling the private method via reflection in contrast to my original proposal in this PR? I understand that my proposal is also sort of a hack, but it only interacts with public methods and classes which is less likely to change.

[ggnaegi]

I don't recommend copying and pasting code from the MS codebase for long-term purposes because the internal implementation can change with .NET version updates. However, it can be acceptable in the short term until we develop a more practical design solution based on public methods.

Let's hack this private method?! 😎 Why not to call it with a help of reflection technology? 🙊

I disagree about calling private methods, because private method signature can(probably will) change even with minor version upgrades.

What are the advantages of calling the private method via reflection in contrast to my original proposal in this PR? I understand that my proposal is also sort of a hack, but it only interacts with public methods and classes which is less likely to change.

@yjorayev @raman-m Mmmh, i haven't read this bit of trolling from Raman. My point was: Use the classes FixedWindowRateLimiter, SlidingWindowRateLimiter etc. and avoid our constructs that are less reliable.