Redis Rate Limiter might not work when configure with multiple routes

[phisixersai]

Version: 3.0.2

I'm now trying to configure RequestRateLimiterGatewayFilterFactory with RedisRateLimiter for my services.
But for different endpoints, we want to set different replenishRate and burstCapacity.
Then I looked into the implementation, I found it might not work for multiple routes.

For example, with the following route configuration, route-1 and route-2 use RedisRateLimiter but with different replenishRate and burstCapacity :

spring:
  cloud:
    gateway:
      routes:
      - id: route-1
        uri: https://example1.org
        filters:
        - name: RequestRateLimiter
          args:
            redis-rate-limiter.replenishRate: 1
            redis-rate-limiter.burstCapacity: 2
            redis-rate-limiter.requestedTokens: 1
      - id: route-2
        uri: https://example2.org
        filters:
        - name: RequestRateLimiter
          args:
            redis-rate-limiter.replenishRate: 10
            redis-rate-limiter.burstCapacity: 20
            redis-rate-limiter.requestedTokens: 1

Assuming we are using the default KeyResolver, which using the principal to identify the request.
I found this part of code that get keys of Redis.
It only uses id (principal in this case) to form the Redis key.
Then, for both route-1 and route-2, RedisRateLimiter will get request_rate_limiter.{id}.tokens from Redis to calculate (here)
Seems the tokens will be messed up when the same principal hitting both route-1 and route-2 endpoints.

My questions are:

• Is this behavior by design?
• If no, maybe it should be fixed to use both id (provided by KeyResolver) and Route ID? like "request_rate_limiter.{user1_route1}.tokens
• If yes, do you have advice on how we handle the situation when we want to configure multiple routes with RedisRateLimiter?
  (maybe a custom KeyResolver to resolve both principal and routeid?)

[sj1231984]

I found this problem, too。
I've customized RedisRateLimiter class。Overriding the isAllowed method，like this:

[Ivanbmstu]

i got this issue too, i used example from documentation https://docs.spring.io/spring-cloud-gateway/docs/3.0.3/reference/html/#the-redis-ratelimiter

Can you please fix documentation (add routeId from exchange attributes to queryParam value) to prevent further errors in future?

[muchnik]

@spencergibb Why is this marked as enhancement -- it looks like critical bug as i see.

I may follow a PR to fix this, if the proposal above about adding a routeId into redis key meets the needs.

[muchnik]

@spencergibb
Followed with a PR:
#2517

[jizhuozhi]

@spencergibb Followed with a PR: #2517

This seems to be because we use the same KeyResolver for different buckets.

But we can't directly add routeId to all current-limiting keys, because in some scenarios, users will want to use different APIs on the same client Use shared quotas and weights are different for each API.

If we force each key to be associated with a routeId, then the user must carefully evaluate the number of possible calls to each API, and require a certain buffer space to ensure that he does not exceed the quota allocated to the client.

[jizhuozhi]

@spencergibb @muchnik
The problem also seems to be because we have buckets configured for each API, but we don't know the relationship between these buckets (if they should be shared). I have two different solutions for this problem:

1. As @muchnik said, we isolate the RateLimiters of all routeIds, and mark in the document that the current-limiting data between all routeIds is not shared with each other (this does not seem very friendly)
2. Based on the isolation of all routeIds, we provide an explicit mechanism for sharing buckets

spring:
  cloud:
    gateway:
      filter:
        request-rate-limiter:
          buckets:
            foo:
              redis-rate-limiter.replenishRate: 10
              redis-rate-limiter.burstCapacity: 100
            bar:
              redis-rate-limiter.replenishRate: 10
              redis-rate-limiter.burstCapacity: 100
      routes:
        - id: route-1
          uri: https://example1.org
          filters:
            - name: RequestRateLimiter
              args:
                redis-rate-limiter.requestedTokens: 1
                bucket: foo
        - id: route-2
          uri: https://example2.org
          filters:
            - name: RequestRateLimiter
              args:
                redis-rate-limiter.requestedTokens: 1
                bucket: bar

[muchnik]

I'd thinking about @jizhuozhi p.2, but maybe just introduce unique flag argument?
So if rate limiter should not share same bucket across routes -- we just set true and using routeId as part of bucket key, false (as default) if bucket key just using keyResolver strategy.

spring:
  cloud:
    gateway:
      routes:
      - id: requestratelimiter_route
        uri: https://example.org
        filters:
        - name: RequestRateLimiter
          args:
            redis-rate-limiter.replenishRate: 10
            redis-rate-limiter.burstCapacity: 20
            redis-rate-limiter.requestedTokens: 1
            redis-rate-limiter.uniqueBucketPerRoute: true

Second aproach is to use additional KeyResolver interface method that accepts routeId as second argument, so users can implement it if they need to differentiate between routes (that aproach i like more).

[jizhuozhi]

I'd thinking about @jizhuozhi p.2, but maybe just introduce unique flag argument? So if rate limiter should not share same bucket across routes -- we just set true and using routeId as part of bucket key, false (as default) if bucket key just using keyResolver strategy.

spring:
  cloud:
    gateway:
      routes:
      - id: requestratelimiter_route
        uri: https://example.org
        filters:
        - name: RequestRateLimiter
          args:
            redis-rate-limiter.replenishRate: 10
            redis-rate-limiter.burstCapacity: 20
            redis-rate-limiter.requestedTokens: 1
            redis-rate-limiter.uniqueBucketPerRoute: true

Second aproach is to use additional KeyResolver interface method that accepts routeId as second argument, so users can implement it if they need to differentiate between routes (that aproach i like more).

@muchnik But at this time how could we group the routes (A, B, C, D) to (A, B) and (C, D)?

[muchnik]

@jizhuozhi Leave that details to user, which is implementing KeyResolver using routeIds?

[jizhuozhi]

@muchnik And also, this problem is not just RedisRateLimiter, we need to have an explicit isolation mechanism for all RequestRateLimiter services (including user-defined RateLimiter).

spring:
  cloud:
    gateway:
      routes:
      - id: requestratelimiter_route
        uri: https://example.org
        filters:
        - name: RequestRateLimiter
          args:
            redis-rate-limiter.replenishRate: 10
            redis-rate-limiter.burstCapacity: 20
            redis-rate-limiter.requestedTokens: 1
            isolated: true # In this condition, we will append the routeId after key