#2138 Introduce ASP.NET rate limiting

[yjorayev]

Closes #2138

• #2138

Discussion thread

• https://github.com/ThreeMammals/Ocelot/discussions/2138#discussioncomment-11116502

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

Predecessor

• #2294 We need to merge this PR first due to the agreement on the JSON schema, which includes global configuration and specific Metadata options.

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

[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 da9d6fab6c704baa925d3c1c5fc984f6a9e9c717 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?! :sunglasses: Why not to call it with a help of reflection technology? :speak_no_evil:

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

[raman-m]

@yjorayev Hi Yhlas! Do you have the enthusiasm proceed with development? Could you begin by addressing the merge conflicts, please?

If you have no objections, I would like to invite you to review the following PR →

• #2294 where we will address the issue of various types of limiters.

[raman-m]

Predecessor

• #2294

@yjorayev FYI We need to merge this PR first due to the agreement on the JSON schema, which includes global configuration and specific Metadata options.

P.S. Could you fix all the merge conflicts by simply merging develop into the feature branch, please?

[raman-m]

@ggnaegi commented on Nov 14, 2024

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.

We don't need to reinvent anything, just a useful JSON configuration. ASP.NET middleware can be part of Ocelot pipeline at position before Ocelot's own middleware, here: https://github.com/ThreeMammals/Ocelot/blob/d5e6d9aef3265495a23e380d2145a769a0d5a7c3/src/Ocelot/Middleware/OcelotPipelineExtensions.cs#L77-L80 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!

However, at least we can retain Microsoft's RateLimiter implementations and replace the current ones.

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.