From 3a1c716c73ff27328dc6e5ae7d2fc31ec36478c7 Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Wed, 23 Feb 2022 10:21:29 -0500 Subject: [PATCH] Remove an unused rate limiter --- .../RateLimitsConfiguration.java | 24 ----- .../DynamicRateLimitsConfiguration.java | 2 - .../limits/CardinalityRateLimiter.java | 89 ------------------- .../limits/CardinalityRateLimiterTest.java | 54 ----------- 4 files changed, 169 deletions(-) delete mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/limits/CardinalityRateLimiter.java delete mode 100644 service/src/test/java/org/whispersystems/textsecuregcm/limits/CardinalityRateLimiterTest.java diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/RateLimitsConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/RateLimitsConfiguration.java index da493bb46..897aa80e0 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/RateLimitsConfiguration.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/RateLimitsConfiguration.java @@ -164,28 +164,4 @@ public class RateLimitsConfiguration { return leakRatePerMinute; } } - - public static class CardinalityRateLimitConfiguration { - @JsonProperty - private int maxCardinality; - - @JsonProperty - private Duration ttl; - - public CardinalityRateLimitConfiguration() { - } - - public CardinalityRateLimitConfiguration(int maxCardinality, Duration ttl) { - this.maxCardinality = maxCardinality; - this.ttl = ttl; - } - - public int getMaxCardinality() { - return maxCardinality; - } - - public Duration getTtl() { - return ttl; - } - } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicRateLimitsConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicRateLimitsConfiguration.java index 8ab639bbc..e304209cd 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicRateLimitsConfiguration.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/dynamic/DynamicRateLimitsConfiguration.java @@ -1,9 +1,7 @@ package org.whispersystems.textsecuregcm.configuration.dynamic; import com.fasterxml.jackson.annotation.JsonProperty; -import org.whispersystems.textsecuregcm.configuration.RateLimitsConfiguration.CardinalityRateLimitConfiguration; import org.whispersystems.textsecuregcm.configuration.RateLimitsConfiguration.RateLimitConfiguration; -import java.time.Duration; public class DynamicRateLimitsConfiguration { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/limits/CardinalityRateLimiter.java b/service/src/main/java/org/whispersystems/textsecuregcm/limits/CardinalityRateLimiter.java deleted file mode 100644 index 4f8698d94..000000000 --- a/service/src/main/java/org/whispersystems/textsecuregcm/limits/CardinalityRateLimiter.java +++ /dev/null @@ -1,89 +0,0 @@ -/* - * Copyright 2021 Signal Messenger, LLC - * SPDX-License-Identifier: AGPL-3.0-only - */ - -package org.whispersystems.textsecuregcm.limits; - -import java.time.Duration; -import org.whispersystems.textsecuregcm.configuration.RateLimitsConfiguration.CardinalityRateLimitConfiguration; -import org.whispersystems.textsecuregcm.controllers.RateLimitExceededException; -import org.whispersystems.textsecuregcm.redis.FaultTolerantRedisCluster; - -/** - * A cardinality rate limiter prevents an actor from taking some action if that actor has attempted to take that action - * on too many targets in a fixed period of time. Behind the scenes, we estimate the target count using a - * hyper-log-log data structure; as a consequence, the number of targets is an approximation, and this rate limiter - * should not be used in cases where precise time or target limits are required. - */ -public class CardinalityRateLimiter { - - private final FaultTolerantRedisCluster cacheCluster; - - private final String name; - - private final Duration ttl; - private final int defaultMaxCardinality; - - public CardinalityRateLimiter(final FaultTolerantRedisCluster cacheCluster, final String name, final Duration ttl, final int defaultMaxCardinality) { - this.cacheCluster = cacheCluster; - - this.name = name; - - this.ttl = ttl; - this.defaultMaxCardinality = defaultMaxCardinality; - } - - public void validate(final String key, final String target, final int maxCardinality) throws RateLimitExceededException { - - final boolean rateLimitExceeded = cacheCluster.withCluster(connection -> { - final String hllKey = getHllKey(key); - - final boolean changed = connection.sync().pfadd(hllKey, target) == 1; - final long cardinality = connection.sync().pfcount(hllKey); - - final boolean mayNeedExpiration = changed && cardinality == 1; - - // If the set already existed, we can assume it already had an expiration time and can save a round trip by - // skipping the ttl check. - if (mayNeedExpiration && connection.sync().ttl(hllKey) == -1) { - connection.sync().expire(hllKey, ttl.toSeconds()); - } - - return cardinality > maxCardinality; - }); - - if (rateLimitExceeded) { - long remainingTtl = getRemainingTtl(key); - throw new RateLimitExceededException(remainingTtl >= 0 ? Duration.ofSeconds(remainingTtl) : null); - } - } - - private String getHllKey(final String key) { - return "hll_rate_limit::" + name + "::" + key; - } - - public Duration getInitialTtl() { - return ttl; - } - - /** - * Get the remaining ttl for the specified key - * - * @param key with timeout to check - * @return the ttl, or negative in the case of error - */ - public long getRemainingTtl(final String key) { - // ttl() returns -2 if key does not exist, -1 if key has no expiration - return cacheCluster.withCluster(connection -> connection.sync().ttl(getHllKey(key))); - } - - public int getDefaultMaxCardinality() { - return defaultMaxCardinality; - } - - public boolean hasConfiguration(final CardinalityRateLimitConfiguration configuration) { - return defaultMaxCardinality == configuration.getMaxCardinality() && ttl.equals(configuration.getTtl()); - } - -} diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/limits/CardinalityRateLimiterTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/limits/CardinalityRateLimiterTest.java deleted file mode 100644 index e670a799b..000000000 --- a/service/src/test/java/org/whispersystems/textsecuregcm/limits/CardinalityRateLimiterTest.java +++ /dev/null @@ -1,54 +0,0 @@ -/* - * Copyright 2021 Signal Messenger, LLC - * SPDX-License-Identifier: AGPL-3.0-only - */ - -package org.whispersystems.textsecuregcm.limits; - -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; - -import java.time.Duration; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.RegisterExtension; -import org.whispersystems.textsecuregcm.controllers.RateLimitExceededException; -import org.whispersystems.textsecuregcm.redis.RedisClusterExtension; - -class CardinalityRateLimiterTest { - - @RegisterExtension - static final RedisClusterExtension REDIS_CLUSTER_EXTENSION = RedisClusterExtension.builder().build(); - - @Test - void testValidate() { - final int maxCardinality = 10; - final CardinalityRateLimiter rateLimiter = - new CardinalityRateLimiter(REDIS_CLUSTER_EXTENSION.getRedisCluster(), "test", Duration.ofDays(1), - maxCardinality); - - final String source = "+18005551234"; - int validatedAttempts = 0; - int blockedAttempts = 0; - - for (int i = 0; i < maxCardinality * 2; i++) { - try { - rateLimiter.validate(source, String.valueOf(i), rateLimiter.getDefaultMaxCardinality()); - validatedAttempts++; - } catch (final RateLimitExceededException e) { - blockedAttempts++; - } - } - - assertTrue(validatedAttempts >= maxCardinality); - assertTrue(blockedAttempts > 0); - - final String secondSource = "+18005554321"; - - try { - rateLimiter.validate(secondSource, "test", rateLimiter.getDefaultMaxCardinality()); - } catch (final RateLimitExceededException e) { - fail("New source should not trigger a rate limit exception on first attempted validation"); - } - } - -}