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.
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
ratelimit: do not default to ipstrategy too early #6713
Conversation
adding no merge because current test does not actually prevent regression for now. |
after consideration, I don't think the regression should be prevented in the unit tests of the ratelimiter package, since SetDefaults is applied out of this package. However, I'm wondering if we should enforce that the fields of SourceCriterion are all mutually exclusive (they already actually are), and in which case we could add a test reflecting that property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
And we already take care of setting ipstrategy as the default in the rate limiter constructor anyway. Fixes traefik#6687
What does this PR do?
This PR removes ipstrategy being systematically set as the default source
criterion, which was causing any other source criterion provided in the config
to be ignored.
We already take care of setting ipstrategy as the default in the rate limiter
constructor anyway.
Motivation
Fixes #6687
More
[ ] Added/updated documentationAdditional Notes