Skip to content

[gh-2288] Fix for redis rate limiter not working with multiple routes#2517

Merged
spencergibb merged 1 commit intospring-cloud:mainfrom
muchnik:gh-2288-fix-redis-rate-limit-multiple-routes
Sep 27, 2024
Merged

[gh-2288] Fix for redis rate limiter not working with multiple routes#2517
spencergibb merged 1 commit intospring-cloud:mainfrom
muchnik:gh-2288-fix-redis-rate-limit-multiple-routes

Conversation

@muchnik
Copy link
Copy Markdown
Contributor

@muchnik muchnik commented Feb 11, 2022

This PR fixes (gh-2288) bug, which caused the same bucket is being reused by redis rate limiter over multiple routes.
The reason for this was a non-unique key that was created for storing bucket.
Now redis bucket key is unique between routes.

@DidierLoiseau
Copy link
Copy Markdown

That’s quite a drastic change because this means that it won’t be possible to share the same rate limiter across routes, whereas, before this PR, it was already possible to get per route rate limiters with a custom KeyResolver (which is what we actually did).

@spencergibb
Copy link
Copy Markdown
Member

@DidierLoiseau I think you were operating a bit unintentionally where this fixed a real bug. I'm curious if the new bucket4j implementation might work in your case. I've also opened #3694

@LorenzoLuconi
Copy link
Copy Markdown

This is a breaking change.
I just upgraded to version 4.2 and found that all my limits, shared by dozens of routes, no longer work.
For me it was a feature not a bug. I could choose to add the route_id by creating my KeyResolver and now I can no longer do that.

With a KeyResolver like this I could get the same result that is now imposed:

Mono.fromSupplier( () -> {
                    final Route route = exchange.getAttribute(GATEWAY_ROUTE_ATTR);
                    return route.getId() + "." + "...my_key...";
 });

The only option I think at this point is to duplicate the RedisRateLimiter code and fix it: I don't think I can extend RedisRateLimiter because unfortunately getKeys is a static method and I can`t override it.

I hope this modification can be overhauled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants