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

Allow users to specify the redis key via a strategy of some kind #3694

Open
spencergibb opened this issue Feb 13, 2025 · 2 comments
Open

Allow users to specify the redis key via a strategy of some kind #3694

spencergibb opened this issue Feb 13, 2025 · 2 comments

Comments

@spencergibb
Copy link
Member

Previously, there was a bug that leaked rate limiter keys across routes #2288

#2517 added the routeId to that key, but that may not be ideal in all situations.

See #2517 (comment)

@iis-MarkKuang
Copy link

I read #2517 and felt the same way, there are 2 scenarios in my case where some flexibility could be helpful:

  1. Share the same bucket quota across more than one route;
  2. Allow for different bucket quota and replenish rate for different keys.

In my case, I had to write my own version of the RedisRateLimiter(to be exact, re-write the isAllowed function) to implement these, where the passed in ApiKey is retrieved from the Exchange object, as the previous ApiKey check filter stores the apikey in the exchange once the apikey is validated.

public Mono<Response> isAllowed(ApiKey apiKey) {
    // Check if the rate limiter is initialized.
    if (!this.initialized.get()) {
      throw new IllegalStateException("RedisRateLimiter is not initialized");
    }

    // Use apikey from context(Put in by previous ApiKeyAuthGatewayFactory).
    if (apiKey == null) {
      log.warn("ApiKey doesn't exist, " + apiKey.getId());
      return Mono.just(new Response(false, getHeaders(0L)));
    }

    int requestedTokens = 1;

    // Invokes LUA script for rate-limiting based on tokens bucket algorithm.
    try {
      List<String> keys = getKeys(apiKey.getId());

      int replenishRate = apiKey.getReplenishRate();
      int burstCapacity = apiKey.getBurstCapacity();

      // The arguments to the LUA script. time() returns unix timestamp in seconds.
      List<String> scriptArgs = Arrays.asList(String.valueOf(replenishRate), String.valueOf(burstCapacity), "", String.valueOf(requestedTokens));
      // Get whether allowed(boolean) and tokens left(int) by executing lua script.
      Flux<List<Long>> flux = this.redisTemplate.execute(this.script, keys, scriptArgs);
      return flux.onErrorResume(throwable -> {
        log.error("Error calling rate limiter lua", throwable);
        return Flux.just(Arrays.asList(1L, -1L));
      }).reduce(new ArrayList<Long>(), (longs, l) -> {
        longs.addAll(l);
        return longs;
      }).map(results -> {
        boolean allowed = results.get(0) == 1L;
        Long tokensLeft = results.get(1);

        Response response = new Response(allowed, getHeaders(tokensLeft));

        return response;
      });
    }
    catch (Exception e) {

      log.error(RATE_LIMITER_GENERAL_ERROR, e);
    }
    return Mono.just(new Response(true, getHeaders(-1L)));
  }

Maybe some design is needed to form the redis/bucket4j rate limit keys to better support these requirements.

@LorenzoLuconi
Copy link

I think this change causes more problems than solutions

I just added a comment to #2517.

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

No branches or pull requests

3 participants