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.

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

Prevent from no rate limiting when average is zero #9621

Merged
merged 3 commits into from Jan 3, 2023

Conversation

witalisoft
Copy link
Contributor

@witalisoft witalisoft commented Dec 27, 2022

What does this PR do?

Docs about ratelimit says that you can omit rate limiting by specifying average as 0. In traefik version < 2.7.2 it was true version 2.7.2 introduce a bug and you cannot disable rate limiting in that way. The bug was probably related to bumping package golang.org/x/time. This change treats zero limits differently and now rate.Inf should be set when average == 0.

Motivation

To restore the option with disabling rate limiting, which broke when the bug we relied on got fixed in x/time/rate:
https://go-review.googlesource.com/c/time/+/323429

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

@mpl
Copy link
Collaborator

mpl commented Dec 27, 2022

We had already noted that we were relying on a buggy behaviour of x/time, and we had opened an issue for that:
golang/go#47221
Are you saying that they finally fixed the issue above in x/time, and that this PR adjusts things consequently?

@witalisoft
Copy link
Contributor Author

witalisoft commented Dec 27, 2022

I wasn't aware of this issue was already submitted and I don't have any idea if this was fixed. My PR findings were discovered because of the docs vs code difference and the PR fix an issue with the following scenario.

@mpl mpl force-pushed the fix-disabling-ratelimiter branch 2 times, most recently from 699ac18 to bf1ecfb Compare January 2, 2023 16:35
@mpl mpl changed the base branch from master to v2.9 January 2, 2023 16:37
@mpl mpl force-pushed the fix-disabling-ratelimiter branch from 4201925 to de25cde Compare January 3, 2023 10:03
Copy link
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

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

Thanks 👍

Copy link
Member

@tomMoulard tomMoulard left a comment

Choose a reason for hiding this comment

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

LGTM 👌

@tomMoulard tomMoulard added this to the 2.9 milestone Jan 3, 2023
@traefiker traefiker merged commit 0861c47 into traefik:v2.9 Jan 3, 2023
@rtribotte rtribotte changed the title fix no rate limiting if average is 0 Prevent from no rate limiting when average is zero Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants