failing tests for rate limiter policy validation when 'default' or 'disable' used #2283
Conversation
jakebanks
changed the title
| if (string.Equals(RateLimitingConstants.Default, rateLimiterPolicyName, StringComparison.OrdinalIgnoreCase) | ||
| || string.Equals(RateLimitingConstants.Disable, rateLimiterPolicyName, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I think this check (
name is Default or Disable) should be moved up to before we callGetPolicyAsync, 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.
There was a problem hiding this comment.
Compare with ValidateCorsPolicyAsync
There was a problem hiding this comment.
I adjusted this to match how we validate the auth policy in the prior method.
|
There should not be policies in the collection with these names, ever, that's what the check is for. However, we need to allow for a null result in that case. |
|
@jakebanks please ack the CLA. |
|
@microsoft-github-policy-service agree company="PageUpPeople" |
sorry @Tratcher I only just caught up with your changes, done now (changes LGTM btw) |
|
Thanks |
Debugging during a test run I can see that
_rateLimiterPolicyProvider.GetPolicyAsync(rateLimiterPolicyName)always returns null for the valuesdefaultordisable, and the reason seems to be that the collection the method is reading from is empty - should it contain some default policies?