Skip to content
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.

Sign up for GitHub

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

failing tests for rate limiter policy validation when 'default' or 'disable' used #2283

Merged
merged 3 commits into from
Nov 13, 2023
Merged

failing tests for rate limiter policy validation when 'default' or 'disable' used #2283

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fefa289
failing tests for rate limiter policy validation
jakebanks Oct 19, 2023
07e2a12
Update ConfigValidator.cs
Tratcher Oct 19, 2023
5067a33
Mimic auth
Tratcher Nov 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/ReverseProxy/Configuration/ConfigValidator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,7 @@ private async ValueTask ValidateRateLimiterPolicyAsync(IList<Exception> errors,
return;
}

//TODO: this only gets invoked if a custom RateLimiter policy is named one of the following, AND it gets used. Is there a better way to check this at startup? or is it unlikely that the policy will be registered but not used?
if (string.Equals(RateLimitingConstants.Default, rateLimiterPolicyName, StringComparison.OrdinalIgnoreCase)
|| string.Equals(RateLimitingConstants.Disable, rateLimiterPolicyName, StringComparison.OrdinalIgnoreCase))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the policy is not null then this line will throw, even if I haven't defined a custom RateLimiter policy with a conflicting name

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current logic looks like it will report an error for a policy with such a name no matter if that policy exists or not, which is unfortunate.

I think this check (name is Default or Disable) should be moved up to before we call GetPolicyAsync, and just early-exit without reporting an error.

The logic that is acting on these policy names looks correct though.
https://github.com/mburumaxwell/reverse-proxy/blob/1be98d88cd42340d67e40fb352bd3431677e6b71/src/ReverseProxy/Routing/ProxyEndpointFactory.cs#L121-L132

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this check (name is Default or Disable) should be moved up to before we call GetPolicyAsync, and just early-exit without reporting an error.

We do want to report a name conflict though. This check is correct for doing that.

What's missing is that if the policy is null, we should not produce an error for the names Default or Disable. That goes on line 311.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compare with ValidateCorsPolicyAsync

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I adjusted this to match how we validate the auth policy in the prior method.

{
Expand Down
47 changes: 47 additions & 0 deletions test/ReverseProxy.Tests/Configuration/ConfigValidatorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,53 @@ public async Task Rejects_ReservedCorsPolicyIsUsed(string corsPolicy)
Assert.Contains(result, err => err.Message.Equals($"The application has registered a CORS policy named '{corsPolicy}' that conflicts with the reserved CORS policy name used on this route. The registered policy name needs to be changed for this route to function."));
}

#if NET7_0_OR_GREATER
[Theory]
[InlineData("Default")]
[InlineData("Disable")]
public async Task Accepts_BuiltInRateLimiterPolicy(string rateLimiterPolicy)
{
var route = new RouteConfig {
RouteId = "route1",
Match = new RouteMatch
{
Hosts = new[] { "localhost" },
},
ClusterId = "cluster1",
RateLimiterPolicy = rateLimiterPolicy
};

var services = CreateServices();
var validator = services.GetRequiredService<IConfigValidator>();

var result = await validator.ValidateRouteAsync(route);

Assert.Empty(result);
}

[Theory]
[InlineData("NotAPolicy")]
public async Task Rejects_InvalidRateLimiterPolicy(string rateLimiterPolicy)
{
var route = new RouteConfig {
RouteId = "route1",
Match = new RouteMatch
{
Hosts = new[] { "localhost" },
},
ClusterId = "cluster1",
RateLimiterPolicy = rateLimiterPolicy };

var services = CreateServices();
var validator = services.GetRequiredService<IConfigValidator>();

var result = await validator.ValidateRouteAsync(route);

Assert.NotEmpty(result);
Assert.Contains(result, err => err.Message.Contains($"RateLimiter policy '{rateLimiterPolicy}' not found"));
}
#endif

[Fact]
public async Task EmptyCluster_Works()
{
Expand Down