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

Append filter url to cache key #19

Closed
letientai299 opened this issue Dec 5, 2017 · 4 comments
Closed

Append filter url to cache key #19

letientai299 opened this issue Dec 5, 2017 · 4 comments

Comments

@letientai299
Copy link

letientai299 commented Dec 5, 2017

I suppose that the expression is evaluated into cache key, if so, only it is not enough to distinct the 2 rate limit configurations. How about adding filter url as prefix for cache key?

Below are what I'm observing.

This doesn't work:

bucket4j:
  enabled: true
  filters:
  - cache-name: buckets
    filter-method: zuul
    url: '/userauth/userInfoes(|/)'
    rate-limits:
    - filter-key-type: expression
      execute-condition: '@securityService.isRequestType("POST")'
      expression: '@securityService.extractRemoteAddress()'
      bandwidths:
      - capacity: 5
        time: 10
        unit: minutes

  - cache-name: buckets
    filter-method: zuul
    url: '/userauth/signUpCheck/.*'
    rate-limits:
    - filter-key-type: expression
      execute-condition: '@securityService.isRequestType("POST")'
      expression: '@securityService.extractRemoteAddress()'
      bandwidths:
      - capacity: 20
        time: 10
        unit: minutes

Append the request url make it work:

bucket4j:
  enabled: true
  filters:
  - cache-name: buckets
    filter-method: zuul
    url: '/userauth/userInfoes(|/)'
    rate-limits:
    - filter-key-type: expression
      execute-condition: '@securityService.isRequestType("POST")'
      expression: '"/userauth/userInfoes:" + @securityService.extractRemoteAddress()'
      bandwidths:
      - capacity: 5
        time: 10
        unit: minutes

  - cache-name: buckets
    filter-method: zuul
    url: '/userauth/signUpCheck/.*'
    rate-limits:
    - filter-key-type: expression
      execute-condition: '@securityService.isRequestType("POST")'
      expression: '"/userauth/signUpCheck:" + @securityService.extractRemoteAddress()'
      bandwidths:
      - capacity: 20
        time: 10
        unit: minutes

Here's how I test the above configuration.

// Fake service.
@RestController
@RequestMapping(path = PATH_USER_AUTH)
public class UserAuthService {
  public static final String PATH_USER_AUTH = "/userauth";

  @RequestMapping(
      path = "/**",
      method = {HEAD, GET, POST, PUT, PATCH, DELETE}
  )
  public ResponseEntity whatever() {
    return ResponseEntity.ok().build();
  }
}
// integration test code 
@Slf4j
@SpringBootTest
@AutoConfigureMockMvc
@RunWith(SpringRunner.class)
public class UserAuthRateLimitTest {

  private static final String USER_INFOES = UserAuthService.PATH_USER_AUTH + "/userInfoes/";
  private static final String SIGN_UP_CHECK = UserAuthService.PATH_USER_AUTH + "/signUpCheck/";

  @Autowired private MockMvc mockMvc;

  @Test(timeout = 4000)
  public void userInfoes_post_tooMuch() throws Exception {
    testExceedLimit(USER_INFOES, 5);
  }

  @Test(timeout = 4000)
  public void signUpCheck_post_tooMuch() throws Exception {
    testExceedLimit(SIGN_UP_CHECK, 20);
  }

  private void testExceedLimit(String path, int expectedSuccessCallBeforeFail) {
    AtomicInteger count = new AtomicInteger(0);
    Supplier<Integer> supplier = () -> {
      try {
        log.debug("Send request #{}", count.get());
        return mockMvc.perform(post(path)).andReturn().getResponse().getStatus();
      } catch (Exception e) {
        return -1;
      }
    };

    while (supplier.get() == 200) {
      count.getAndIncrement();
    }

    assertThat(count.get()).isEqualTo(expectedSuccessCallBeforeFail);
    assertThat(supplier.get()).isEqualTo(HttpStatus.TOO_MANY_REQUESTS.value());
  }
}

userInfoes_post_tooMuch will fail as it is executed after signUpCheck_post_tooMuch, and the signUpCheck... set cache value to 20, which is exceed the limit for userInfoes.

MarcGiffing added a commit that referenced this issue Dec 5, 2017
@MarcGiffing
Copy link
Owner

like that (123e092)?

@letientai299
Copy link
Author

Haha. When will you release new version?

MarcGiffing added a commit that referenced this issue Dec 6, 2017
@MarcGiffing
Copy link
Owner

Done!. It should be available in the maven central in the next hours. I would be nice if you can can test it. Thanks!

@letientai299
Copy link
Author

Tested. It's pass my tests. Thanks for quick actions.

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

No branches or pull requests

2 participants