[UNDERTOW-2537] Implement bit shift window rate limiter and limiter h…#1777
[UNDERTOW-2537] Implement bit shift window rate limiter and limiter h…#1777baranowb wants to merge 1 commit intoundertow-io:mainfrom
Conversation
baranowb
added
enhancement
| } | ||
| } | ||
|
|
||
| private String getIPAddress(final HttpServerExchange exchange, final boolean warn) { |
There was a problem hiding this comment.
This is not ideal, this code is repeated in Handler class to get log details as well.
| final String current = String.valueOf(currentPrefix); | ||
| final String next = String.valueOf(currentPrefix + 1); | ||
|
|
||
| ConcurrentHashMap.KeySetView<String, AtomicInteger> keys = requestCounter.keySet(); |
There was a problem hiding this comment.
String could be turned into Long with high bits holding IP in pure form and lower working as in this case. But its harder to follow.
baranowb
force-pushed
the
UNDERTOW-2537
branch
from
ba488f4 to
d911301
Compare
core/src/main/java/io/undertow/server/handlers/ratelimit/RateLimiter.java
Outdated
Show resolved
Hide resolved
baranowb
force-pushed
the
UNDERTOW-2537
branch
3 times, most recently
from
1e0c5ca to
4e10443
Compare
| * {@link RateLimiter} has single, common window for all entries. For which keys are computed with bit shifting ops. This is not | ||
| * precise, but has very low performance impact. | ||
| */ | ||
| public class BitShiftSingleWindowRateLimiter implements RateLimiter<HttpServerExchange> { |
There was a problem hiding this comment.
Yes, @baranowb . But, just to better understand your pov, you have added the parameter yourself, is that correct? Or is it based on something else that had already the parameters?
There was a problem hiding this comment.
yes, I envisioned it as being flexible enough for users to limit based on their needs/input, but given that we are kind of always constrained to Exchange, I dont think we need this at this point? KISS ?
@fl4via ^
There was a problem hiding this comment.
hmm, on second look I need to refresh this PR in my head.
baranowb
added
the
waiting peer review
| /** | ||
| * Utility method to generate key from exchange. | ||
| */ | ||
| K generateKey(HttpServerExchange e); |
There was a problem hiding this comment.
This: #1777 (review) - is directly result of obfuscation, in order to have it agnostic in implementation. This is awkward at best ?
baranowb
force-pushed
the
UNDERTOW-2537
branch
2 times, most recently
from
3d6c68f to
cf06a3c
Compare
baranowb
force-pushed
the
UNDERTOW-2537
branch
from
cf06a3c to
82e969c
Compare
|
This is on hold for now, as we are discussing how we are going to move on with handlers, that are directly exposed in the server without going through a proper WildFly RFE process |
| * {@link RateLimiter} has single, common window for all entries. For which keys are computed with bit shifting ops. This is not | ||
| * precise, but has very low performance impact. | ||
| */ | ||
| public class BitShiftSingleWindowRateLimiter implements RateLimiter<HttpServerExchange> { |
There was a problem hiding this comment.
Yes, @baranowb . But, just to better understand your pov, you have added the parameter yourself, is that correct? Or is it based on something else that had already the parameters?
| } | ||
|
|
||
| if (currentRequestCount > rateLimiter.getRequestLimit()) { | ||
| UndertowLogger.REQUEST_LOGGER.exchangeExceedsRequestRateLimit(exchange.getRequestURI(), ipAddress, |
There was a problem hiding this comment.
Printing these warning messages will cause the server log to fill up quickly in case of an attack, thus enabling a DoS. We need to log this only once and, once it hits the limit, log it at debug level.
fl4via
added
waiting PR update
baranowb
removed
the
waiting PR update
2 participants
…andler
Issue: https://issues.redhat.com/browse/UNDERTOW-2537
This needs review. Initially I was going to have Long as key and use bit shifts to encode there everything, but:
If needs be I can switch it to above without problem. It follows similar concept of common sliding window with low accuracy as apache does in its impl. However interface should allow to implement different stategies as well relatively easily .
TestCase is a bit of PITa as to test different aspects it has to be at least 60s( might be less) so it chugs some time on CI...