Refactor RateLimiter #534
Refactor RateLimiter #534
Conversation
adutra
requested review from
jbonofre,
ashvina,
RussellSpitzer,
snazy,
vvcephei,
takidau,
jackye1995,
flyrain,
eric-maynard,
collado-mike and
ebyhr
as code owners
| import org.junit.jupiter.api.Assertions; | ||
|
|
||
| /** Utility class for testing rate limiters. Lets you easily assert the result of tryAcquire(). */ | ||
| public class RateLimitResultAsserter { | ||
| private final RateLimiter rateLimiter; | ||
| public class TokenBucketResultAsserter { |
There was a problem hiding this comment.
I don't think it makes sense to refactor this class specifically. In isolation, making repeated assertions on a generic BooleanSupplier expecting it to return a sequence of trues then false is odd. The expectation that the boolean will change is coupled to the fact that the BooleanSupplier is a RateLimiter.
There was a problem hiding this comment.
It's just because this class is now used for assertions on two distinct classes that have no common interface.
There was a problem hiding this comment.
I wonder if renaming it to something more generic would make it less confusing, as right now it's a middle-ground between a RateLimiterResultAsserter and a BooleanSupplierAsserter. We could rename the class & methods to:
- BooleanSupplierAsserter
- expectTrue
- expectFalse
Alternatively, we could delete the class since it doesn't do much.
There was a problem hiding this comment.
Alternatively, we could delete the class since it doesn't do much.
In fact that's probably the best option 👍
| @JsonProperty("tokenBucketFactory") | ||
| private TokenBucketFactory getTokenBucketFactory() { | ||
| return tokenBucketFactory; | ||
| } | ||
|
|
||
| @JsonProperty("tokenBucketFactory") | ||
| @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "type") | ||
| public void setTokenBucketFactory(@Nullable TokenBucketFactory tokenBucketFactory) { | ||
| this.tokenBucketFactory = tokenBucketFactory; | ||
| } | ||
|
|
There was a problem hiding this comment.
Is there a way to scope this config down to the RealmTokenBucketRateLimiter itself? Otherwise the tokenBucketFactory can be unused if you're using a different limiter.
There was a problem hiding this comment.
I tried but I'm afraid that it won't be achievable with Dropwizard configuration system + HK2 dependency injection.
BTW we already have the same situation: DefaultPolarisAuthenticator requires a TokenBrokerFactory, but
TestInlineBearerTokenPolarisAuthenticator doesn't – if you use the latter, the tokenBroker configuration section would be ignored.
| private final long requestsPerSecond; | ||
| private final long windowSeconds; | ||
| private final Clock clock; | ||
| private final Map<String, TokenBucket> perRealmBuckets = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Not new here (so not blocking this PR), but this can be abused to cause an OOM in Polaris if an attacker issues requests with random realm-IDs.
This PR is motivated by the following issues: * `RateLimiter` implementations are typically request-scoped, because they are sensitive to the realm context, but keeping a cache of token buckets per realm must be delegated to a separate, application-scoped bean: this is solved by introducing a new `TokenBucketFactory` bean. * `TokenBucketRateLimiter` should NOT implement `RateLimiter`, because that would result in two available beans for this interface: the one selected by configuration (`realm-token-bucket` or `no-op`), and this one which is always available, thus triggering an unresolvable bean error when using the Quarkus runtime. Instead, this class is now just a general-purpose Token Bucket implementation. This PR has one user-facing change: * The configuration options for the `realm-token-bucket` rate limiter were moved from the `rateLimiter` section to the `tokenBucketFactory` section.
adutra
force-pushed
the
token-bucket-refactor
branch
from
a45f5d6 to
5629b7d
Compare
This PR is motivated by the following issues:
RateLimiterimplementations are typically request-scoped, because they are sensitive to the realm context, but keeping a cache of token buckets per realm must be delegated to a separate, application-scoped bean: this is solved by introducing a newTokenBucketFactorybean.TokenBucketRateLimitershould NOT implementRateLimiter, because that would result in two available beans for this interface: the one selected by configuration (realm-token-bucketorno-op), and this one which is always available, thus triggering an unresolvable bean error when using the Quarkus runtime. Instead, this class is now just a general-purpose Token Bucket implementation.This PR has one minor user-facing change:
realm-token-bucketrate limiter were moved from therateLimitersection to thetokenBucketFactorysection.