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

Add redis rate limiter #10211

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

longquan0104
Copy link
Contributor

@longquan0104 longquan0104 commented Nov 6, 2023

What does this PR do?

The rate limiter middleware currently is not used with Redis. From my very narrow knowledge, other systems usually use Redis for rate-limiting features. So I tried to add this. The config for the Redis may be referenced here

Motivation

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

#6042

@longquan0104 longquan0104 marked this pull request as ready for review November 10, 2023 13:42
@jspdown
Copy link
Contributor

jspdown commented Nov 13, 2023

Thanks for opening this PR 😃

As you can see, your PR has been marked with the status/1-needs-design-review label. It means that we are actively reviewing
your work and the scope of the change is considered significant enough to first check how it will fit into Traefik
(e.g. configuration, UX, architecture, ...)

@jspdown
Copy link
Contributor

jspdown commented Nov 13, 2023

From a UX point of view, I'm worried about the GCRA algorithm used in go-redis/redis-rate.
We are currently relying on x/time/rate which uses the Token Bucket algorithm.
This is problem since you are configuring both in-memory and redis-based rate limiters from the same configuration struct. So, depending on whether you provide a Redis config or not, you will get a different result and options will have different meaning.

If we don't want to confuse people, which is already a subject difficult to explain and document, I think we should try to
use the same algorithm in both.

@rtribotte
Copy link
Member

Hello @longquan0104,

Thank you for your for this contribution.

Regrettably, due to our current focus on v2.11 and the imminent release of v3.0, the review of your pull request will experience a delay.

Additionally, as mentioned in previous comments, there are outstanding questions related to the proposed changes. It would have been beneficial to address these in a discussion on a proposal issue before the pull request, ensuring clarity and coherence.

We appreciate your understanding and patience during this time. Your contribution is valued, and we look forward to working together to address the questions and integrate your feature into an upcoming release.

@longquan0104 longquan0104 reopened this Dec 25, 2023
@longquan0104 longquan0104 force-pushed the feature/redis_rate_limiter branch 2 times, most recently from cbe405e to f9c17e2 Compare December 25, 2023 12:38
@longquan0104
Copy link
Contributor Author

Sorry for disappearing for a month. I came back and made some updates for this.

@longquan0104
Copy link
Contributor Author

Hi @jspdown , I've just made an update, please help me to review it. Thank you ❤️

@longquan0104
Copy link
Contributor Author

Please let me know if any updates need to be done @jspdown.

@longquan0104 longquan0104 requested a review from jspdown May 27, 2024 06:27
pkg/config/dynamic/middlewares.go Outdated Show resolved Hide resolved
pkg/middlewares/ratelimiter/in_memory_limiter.go Outdated Show resolved Hide resolved
pkg/middlewares/ratelimiter/redis_limiter.go Outdated Show resolved Hide resolved
pkg/middlewares/ratelimiter/redis_limiter.go Outdated Show resolved Hide resolved
pkg/middlewares/ratelimiter/rate_limiter.go Outdated Show resolved Hide resolved
pkg/middlewares/ratelimiter/redisrate/rate.go Outdated Show resolved Hide resolved
pkg/middlewares/ratelimiter/redisrate/lua.go Outdated Show resolved Hide resolved
pkg/middlewares/ratelimiter/redisrate/lua.go Outdated Show resolved Hide resolved
pkg/middlewares/ratelimiter/redisrate/lua.go Outdated Show resolved Hide resolved
pkg/middlewares/ratelimiter/redisrate/rate_test.go Outdated Show resolved Hide resolved
@longquan0104 longquan0104 force-pushed the feature/redis_rate_limiter branch 2 times, most recently from 7bf00a6 to 388049c Compare June 4, 2024 17:20
@longquan0104
Copy link
Contributor Author

Hello @jspdown , just make some updates. Please let me know if there is any change I should make. Thank you

@ldez
Copy link
Contributor

ldez commented Jun 14, 2024

You don't have to rebase or "merge with master" your PR unless you have conflicts.

@longquan0104
Copy link
Contributor Author

Oops. Sorry for this mess.

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

7 participants