dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.28k stars 4.74k forks source link

FixedWindowRateLimiter miscalculates `Retry-After` value #92557

Open aleksatoroman opened 1 year ago

aleksatoroman commented 1 year ago

Description

When using the FixedWindowRateLimiter, the value set for the Retry-After header reflects the entire time window specified in the policy configuration rather than the remaining time until the next permissible request.

Reproduction Steps

  1. Utilize the official example provided here. The relevant code snippet is:
_rateLimiterOptions.OnRejected = (context, token) =>
{
    if (context.Lease.TryGetMetadata(MetadataName.RetryAfter, out var retryAfter))
    {
        context.HttpContext.Response.Headers.RetryAfter =
            ((int) retryAfter.TotalSeconds).ToString(NumberFormatInfo.InvariantInfo);
    }

    return ValueTask.CompletedTask;
};

2.

Configure a Window option for, let's say, 00:01:00. After reaching the maximum request limit for this window, inspect the Retry-After header. A visual representation of the issue can be found

image

Expected behavior

The Retry-After header should reflect the remaining time until the next request is permissible, like some other implementations are doing, such as Redis-based rate limiting that is built on top of this one found here.

Actual behavior

The Retry-After header only reflects the static window value defined in the policy's configuration, ignoring the elapsed time.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

This RateLimiting middleware is used in combination with my YARP reverse proxy gateway based on the following documentation: YARP rate limiting

ghost commented 1 year ago

Tagging subscribers to this area: @mangod9 See info in area-owners.md if you want to be subscribed.

Issue Details
### Description When using the `FixedWindowRateLimiter`, the value set for the `Retry-After` header reflects the entire time window specified in the policy configuration rather than the remaining time until the next permissible request. ### Reproduction Steps 1. Utilize the official example provided [here](https://learn.microsoft.com/en-gb/aspnet/core/performance/rate-limit?view=aspnetcore-8.0). The relevant code snippet is: ```cs _rateLimiterOptions.OnRejected = (context, token) => { if (context.Lease.TryGetMetadata(MetadataName.RetryAfter, out var retryAfter)) { context.HttpContext.Response.Headers.RetryAfter = ((int) retryAfter.TotalSeconds).ToString(NumberFormatInfo.InvariantInfo); } return ValueTask.CompletedTask; }; ``` 2. Configure a `Window` option for, let's say, `00:01:00`. After reaching the maximum request limit for this window, inspect the Retry-After header. A visual representation of the issue can be found ![image](https://github.com/dotnet/runtime/assets/114148475/0c1c1cb5-ee04-4808-886a-d14cf48d32fe) ### Expected behavior The `Retry-After` header should reflect the remaining time until the next request is permissible, like some other implementations are doing, such as Redis-based rate limiting that is built on top of this one found [here](https://github.com/cristipufu/aspnetcore-redis-rate-limiting). ### Actual behavior The `Retry-After` header only reflects the static window value defined in the policy's configuration, ignoring the elapsed time. ### Regression? _No response_ ### Known Workarounds _No response_ ### Configuration _No response_ ### Other information This `RateLimiting` middleware is used in combination with my `YARP` reverse proxy gateway based on the following documentation: [YARP rate limiting](https://microsoft.github.io/reverse-proxy/articles/rate-limiting.html)
Author: aleksatoroman
Assignees: -
Labels: `area-System.Threading`, `untriaged`
Milestone: -
mangod9 commented 1 year ago

@BrennanConroy