diff --git a/README.md b/README.md index 09267f9..3629212 100644 --- a/README.md +++ b/README.md @@ -79,10 +79,9 @@ The application uses Redis (`spring.data.redis.host`/`port`, default `localhost: | Cache | What | TTL | |---|---|---| | `sdlc-systems` | SDLC systems by id, read on every create, update and SDLC call | `resourceapi.cache.sdlc-systems-ttl` (10m) | -| `external-projects` | `GET /api/v2/projects/{id}/external` answers per SDLC system and external id; "unknown" answers are not cached | `resourceapi.cache.external-projects-ttl` (5m) | +| `external-projects` | `GET /api/v2/projects/{id}/external` answers per SDLC system and external id | `resourceapi.cache.external-projects-ttl` (5m); "the SDLC system doesn't know this project" (stored as a cached null) only `resourceapi.cache.missing-external-projects-ttl` (30s), so a project created there is found soon | -A background sync always asks the SDLC system and refreshes the cached answer (or removes it when the SDLC -system no longer knows the project). SDLC systems are not changed +A background sync always asks the SDLC system and replaces the cached answer with its own. SDLC systems are not changed through this API; one changed directly in the database is picked up when its entry expires. The cache is an optimization: if Redis is unreachable, cache reads and writes are logged and skipped (each waits at most the 500 ms Redis timeouts) and requests are served without it. @@ -97,6 +96,28 @@ standalone Redis is supported for the lock: with Sentinel or Cluster the applica run unlocked; TLS uses the JVM's trust store (Boot SSL bundles aren't passed to Redisson). (Redisson's Spring Boot starter isn't used: it targets Spring Boot 3. Redisson brings a JCache provider, so `spring.cache.type=redis` is set explicitly.) +**Rate limits** (Redisson `RRateLimiter`, sliding windows shared by all instances: a used permit becomes +available again one period later, which is also what `Retry-After` says): + +| Limit | Default | When exceeded | +|---|---|---| +| Calls to one SDLC system (`resourceapi.rate-limit.sdlc.*`), protecting its API quota; cache hits don't count | 10 per 1s, waiting up to 1s for a permit | `503` + `Retry-After` (our own limit, not the SDLC system's failure); a background sync logs it | +| `POST /api/v2/projects/{id}/sync` per project (`resourceapi.rate-limit.sync.*`); requests refused anyway (unknown project, sync already running) don't count, one refused because no background slot is free (`503`) does | 6 per minute | `429` + `Retry-After` | +| All `/api/**` requests per client IP (`resourceapi.rate-limit.client.*`); behind a reverse proxy the IP comes from `X-Forwarded-For`, trusted only from internal proxy addresses (`server.forward-headers-strategy=native`, see below) | 100 per second | `429` + `Retry-After` | + +A changed rate is applied to the existing limiter when an instance first uses it; unused limiters expire from +Redis after 10 periods (at least a minute), so per-IP limiters don't pile up. Invalid limits (e.g. +`permits=0`) stop the application at startup. If Redis can't be reached, requests are allowed (limiting +protects, losing it shouldn't stop the application), and Redis is not asked again for 10 seconds, so an +outage doesn't add a timeout to every request. + +The per-IP limit only means something if the application sees the client's address. Tomcat takes it from +`X-Forwarded-For` when the request comes from an address matching `server.tomcat.remoteip.internal-proxies` +(by default 10/8, 192.168/16, 172.16/12, 169.254/16, 127/8 and IPv6 local addresses). If the reverse proxy or +load balancer has another address, set that property to match it; otherwise every request appears to come from +the proxy and all clients share one limit. Never match addresses clients can connect from directly, or they +can choose their own key by sending `X-Forwarded-For`. + **Local development**: `./mvnw spring-boot:run` starts Redis from `compose.yaml` automatically (`spring-boot-docker-compose`); with `java -jar`, start it yourself with `docker compose up -d` (it listens on `localhost:6379`). Tests start their own Redis with Testcontainers, so they need Docker. diff --git a/src/main/java/com/company/resourceapi/config/CacheConfig.java b/src/main/java/com/company/resourceapi/config/CacheConfig.java index 85554ee..8b0533d 100644 --- a/src/main/java/com/company/resourceapi/config/CacheConfig.java +++ b/src/main/java/com/company/resourceapi/config/CacheConfig.java @@ -10,6 +10,7 @@ import org.springframework.cache.annotation.EnableCaching; import org.springframework.cache.interceptor.CacheErrorHandler; import org.springframework.cache.interceptor.LoggingCacheErrorHandler; +import org.springframework.cache.support.NullValue; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Role; @@ -26,7 +27,7 @@ @EnableCaching @EnableConfigurationProperties(CacheTtlProperties.class) // cached values are rebuilt from JSON by type name: reflection a native image must know about -@RegisterReflectionForBinding({SdlcSystem.class, ExternalProject.class}) +@RegisterReflectionForBinding({SdlcSystem.class, ExternalProject.class, NullValue.class}) public class CacheConfig { public static final String SDLC_SYSTEMS = "sdlc-systems"; @@ -34,7 +35,8 @@ public class CacheConfig { /** * Starts from Boot's defaults (spring.cache.redis.*: key prefix, null values, ...) and only - * replaces the value serializer and sets each cache's time to live. + * replaces the value serializer and sets each cache's time to live. External projects rely on + * null values being cached (spring.cache.redis.cache-null-values, true by default). */ @Bean RedisCacheManagerBuilderCustomizer redisCacheCustomizer(CacheTtlProperties ttl) { @@ -42,15 +44,22 @@ RedisCacheManagerBuilderCustomizer redisCacheCustomizer(CacheTtlProperties ttl) .allowIfSubType("com.company.resourceapi.") .allowIfSubType("java.lang.") .allowIfSubType("java.time.") + // Spring's marker for a cached null ("the SDLC system doesn't know this project") + .allowIfSubType(NullValue.class) .build(); var serializer = GenericJacksonJsonRedisSerializer.builder() .enableDefaultTyping(typeValidator) + .enableSpringCacheNullValueSupport() .build(); return builder -> { var defaults = builder.cacheDefaults().serializeValuesWith(SerializationPair.fromSerializer(serializer)); builder.cacheDefaults(defaults) .withCacheConfiguration(SDLC_SYSTEMS, defaults.entryTtl(ttl.sdlcSystemsTtl())) - .withCacheConfiguration(EXTERNAL_PROJECTS, defaults.entryTtl(ttl.externalProjectsTtl())); + // a found project is kept longer than "unknown", so a project created there is found soon + .withCacheConfiguration(EXTERNAL_PROJECTS, defaults.entryTtl((key, value) -> + value == null || value instanceof NullValue + ? ttl.missingExternalProjectsTtl() + : ttl.externalProjectsTtl())); }; } diff --git a/src/main/java/com/company/resourceapi/config/CacheTtlProperties.java b/src/main/java/com/company/resourceapi/config/CacheTtlProperties.java index e1399f2..597d037 100644 --- a/src/main/java/com/company/resourceapi/config/CacheTtlProperties.java +++ b/src/main/java/com/company/resourceapi/config/CacheTtlProperties.java @@ -9,9 +9,12 @@ * * @param sdlcSystemsTtl SDLC systems by id; they change rarely and only outside this API * @param externalProjectsTtl projects as described by their SDLC system ({@code GET .../external}) + * @param missingExternalProjectsTtl "the SDLC system doesn't know this project" answers; short, so a + * project created there is found soon */ @ConfigurationProperties("resourceapi.cache") public record CacheTtlProperties( @DefaultValue("10m") Duration sdlcSystemsTtl, - @DefaultValue("5m") Duration externalProjectsTtl) { + @DefaultValue("5m") Duration externalProjectsTtl, + @DefaultValue("30s") Duration missingExternalProjectsTtl) { } diff --git a/src/main/java/com/company/resourceapi/config/RateLimitProperties.java b/src/main/java/com/company/resourceapi/config/RateLimitProperties.java new file mode 100644 index 0000000..b580eeb --- /dev/null +++ b/src/main/java/com/company/resourceapi/config/RateLimitProperties.java @@ -0,0 +1,42 @@ +package com.company.resourceapi.config; + +import jakarta.validation.Valid; +import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Positive; +import java.time.Duration; +import org.springframework.aot.hint.MemberCategory; +import org.springframework.aot.hint.annotation.RegisterReflection; +import org.springframework.boot.context.properties.ConfigurationProperties; +import org.springframework.validation.annotation.Validated; + +/** + * Distributed rate limits ({@code resourceapi.rate-limit.*}), shared by all instances through Redis. + * Validated at startup: a broken limit would otherwise fail, or silently stop limiting, at runtime. + * + * @param sdlc calls to one SDLC system (protects its API quota) + * @param sync sync requests for one project + * @param client API requests from one client (IP address) + */ +@Validated +@ConfigurationProperties("resourceapi.rate-limit") +// validation reads the record fields reflectively, which a native image must know about +@RegisterReflection(classes = {RateLimitProperties.class, RateLimitProperties.Limit.class}, + memberCategories = {MemberCategory.ACCESS_DECLARED_FIELDS, MemberCategory.INVOKE_DECLARED_METHODS}) +public record RateLimitProperties(@NotNull @Valid Limit sdlc, @NotNull @Valid Limit sync, @NotNull @Valid Limit client) { + + /** + * At most {@code permits} per sliding window of {@code period}: each permit becomes available again + * one period after it was used. + * + * @param permits how many per period + * @param period the sliding window + * @param maxWait how long a caller may wait for a permit before being refused + */ + public record Limit(@Positive long permits, @NotNull Duration period, @NotNull Duration maxWait) { + + /** When a permit is surely available again: a used permit only comes back after a full period. */ + public long retryAfterSeconds() { + return Math.max(1, (long) Math.ceil(period.toMillis() / 1000.0)); + } + } +} diff --git a/src/main/java/com/company/resourceapi/config/RedissonConfig.java b/src/main/java/com/company/resourceapi/config/RedissonConfig.java index 96bf712..26c96ba 100644 --- a/src/main/java/com/company/resourceapi/config/RedissonConfig.java +++ b/src/main/java/com/company/resourceapi/config/RedissonConfig.java @@ -10,6 +10,7 @@ import org.springframework.beans.factory.config.BeanFactoryPostProcessor; import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression; +import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.boot.data.redis.autoconfigure.DataRedisConnectionDetails; import org.springframework.boot.data.redis.autoconfigure.DataRedisProperties; import org.springframework.context.annotation.Bean; @@ -25,6 +26,7 @@ * back to running without it. TLS uses the JVM's trust store (Boot SSL bundles aren't passed on). */ @Configuration +@EnableConfigurationProperties(RateLimitProperties.class) public class RedissonConfig { static final String REDISSON_CLIENT = "redissonClient"; diff --git a/src/main/java/com/company/resourceapi/exceptions/RateLimitExceededException.java b/src/main/java/com/company/resourceapi/exceptions/RateLimitExceededException.java new file mode 100644 index 0000000..1163ea0 --- /dev/null +++ b/src/main/java/com/company/resourceapi/exceptions/RateLimitExceededException.java @@ -0,0 +1,20 @@ +package com.company.resourceapi.exceptions; + +import lombok.Getter; + +/** One of our own rate limits refused the request; it can be retried after {@link #getRetryAfterSeconds()}. */ +@Getter +public class RateLimitExceededException extends RuntimeException { + + /** Whose limit: the caller's own requests (429) or the application's calls to a system (503). */ + public enum Scope { CALLER, UPSTREAM } + + private final Scope scope; + private final long retryAfterSeconds; + + public RateLimitExceededException(Scope scope, String message, long retryAfterSeconds) { + super(message); + this.scope = scope; + this.retryAfterSeconds = retryAfterSeconds; + } +} diff --git a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java index 415c1fc..88d39e2 100644 --- a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java +++ b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java @@ -4,39 +4,50 @@ import com.company.resourceapi.clients.ExternalProject; import com.company.resourceapi.clients.SdlcProjectClient; +import com.company.resourceapi.config.RateLimitProperties; import com.company.resourceapi.entities.SdlcSystem; +import com.company.resourceapi.exceptions.RateLimitExceededException; +import com.company.resourceapi.exceptions.RateLimitExceededException.Scope; import java.util.Optional; import lombok.RequiredArgsConstructor; -import org.springframework.cache.annotation.CacheEvict; import org.springframework.cache.annotation.CachePut; import org.springframework.cache.annotation.Cacheable; -import org.springframework.cache.annotation.Caching; import org.springframework.stereotype.Service; -/** Projects as described by their SDLC system, cached per SDLC system and external id. */ +/** + * Projects as described by their SDLC system, cached per SDLC system and external id, and rate limited + * per SDLC system across all instances. + */ @Service @RequiredArgsConstructor public class ExternalProjectService { private final SdlcProjectClient sdlcProjectClient; + private final RateLimiters rateLimiters; + private final RateLimitProperties rateLimits; - /** Cached; a project unknown to the SDLC system isn't cached, so it is found once it exists. */ - @Cacheable(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", unless = "#result == null") + /** + * Cached, including the answer "the SDLC system doesn't know this project" (cached as null), which + * is kept only briefly so a project created there is found soon (see CacheConfig). + */ + @Cacheable(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId") public Optional findProject(SdlcSystem sdlcSystem, String externalId) { - return sdlcProjectClient.fetchProject(sdlcSystem, externalId); + return fetch(sdlcSystem, externalId); } - /** - * Always asks the SDLC system and updates the cache with the answer: a found project replaces the - * cached one, a project the SDLC system no longer knows removes it. - */ - @Caching( - put = @CachePut(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", - unless = "#result == null"), - // unlike for @CachePut, #result is the Optional itself here (late evictions get the raw return value) - evict = @CacheEvict(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", - condition = "#result == null or #result.isEmpty()")) + /** Always asks the SDLC system and caches its answer, found or unknown, replacing the cached one. */ + @CachePut(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId") public Optional refreshProject(SdlcSystem sdlcSystem, String externalId) { + return fetch(sdlcSystem, externalId); + } + + /** Only real calls count against the SDLC system's rate limit, not cache hits. */ + private Optional fetch(SdlcSystem sdlcSystem, String externalId) { + var limit = rateLimits.sdlc(); + if (!rateLimiters.tryAcquire("sdlc-rate:" + sdlcSystem.getId(), limit)) { + throw new RateLimitExceededException(Scope.UPSTREAM, + "Rate limit for SdlcSystem %d reached.".formatted(sdlcSystem.getId()), limit.retryAfterSeconds()); + } return sdlcProjectClient.fetchProject(sdlcSystem, externalId); } } diff --git a/src/main/java/com/company/resourceapi/services/ProjectSyncService.java b/src/main/java/com/company/resourceapi/services/ProjectSyncService.java index 5dea543..06ab4b0 100644 --- a/src/main/java/com/company/resourceapi/services/ProjectSyncService.java +++ b/src/main/java/com/company/resourceapi/services/ProjectSyncService.java @@ -1,6 +1,9 @@ package com.company.resourceapi.services; +import com.company.resourceapi.config.RateLimitProperties; import com.company.resourceapi.exceptions.NotFoundException; +import com.company.resourceapi.exceptions.RateLimitExceededException; +import com.company.resourceapi.exceptions.RateLimitExceededException.Scope; import com.company.resourceapi.exceptions.SyncInProgressException; import lombok.RequiredArgsConstructor; import org.springframework.stereotype.Service; @@ -13,11 +16,14 @@ public class ProjectSyncService { private final ProjectService projectService; private final ProjectSyncLock projectSyncLock; private final ProjectSyncWorker projectSyncWorker; + private final RateLimiters rateLimiters; + private final RateLimitProperties rateLimits; /** * Starts refreshing the project from its SDLC system in the background. * * @throws NotFoundException if the project doesn't exist + * @throws RateLimitExceededException if the project was synced too often recently * @throws SyncInProgressException if a sync of the project is still running * @throws org.springframework.core.task.TaskRejectedException if no background slot is free */ @@ -27,6 +33,15 @@ public void requestSync(long projectId) throws NotFoundException { var lease = projectSyncLock.tryAcquire(projectId) .orElseThrow(() -> new SyncInProgressException(projectId)); + + // counted only for syncs that could run: an unknown project or a running sync doesn't use a permit + var limit = rateLimits.sync(); + if (!rateLimiters.tryAcquire("sync-rate:" + projectId, limit)) { + lease.release(); + throw new RateLimitExceededException(Scope.CALLER, + "Too many sync requests for project %d.".formatted(projectId), limit.retryAfterSeconds()); + } + try { // the background task releases the lock when it ends, however it ends projectSyncWorker.syncName(projectId).whenComplete((result, failure) -> lease.release()); diff --git a/src/main/java/com/company/resourceapi/services/RateLimiters.java b/src/main/java/com/company/resourceapi/services/RateLimiters.java new file mode 100644 index 0000000..787c239 --- /dev/null +++ b/src/main/java/com/company/resourceapi/services/RateLimiters.java @@ -0,0 +1,124 @@ +package com.company.resourceapi.services; + +import com.company.resourceapi.config.RateLimitProperties; +import java.time.Duration; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; +import lombok.extern.slf4j.Slf4j; +import org.redisson.api.RRateLimiter; +import org.redisson.api.RateType; +import org.redisson.api.RedissonClient; +import org.springframework.beans.factory.ObjectProvider; +import org.springframework.stereotype.Component; + +/** + * Sliding-window rate limiters shared by all instances (Redisson {@link RRateLimiter}), one per name. + * + *

A limiter is set up with the configured rate the first time this instance uses it (updating a + * limiter created with another rate), and again if Redis dropped it after it expired from disuse. + * Without Redis (unreachable, or no Redisson client) requests are allowed: limiting is a protection, + * losing it shouldn't stop the application. After a failure Redis is left alone for a short while, + * so an outage doesn't add a Redis timeout to every request. + */ +@Component +@Slf4j +public class RateLimiters { + + /** Unused limiters are removed from Redis after this many periods, but not sooner than MIN_KEEP_ALIVE. */ + private static final int KEEP_ALIVE_PERIODS = 10; + private static final Duration MIN_KEEP_ALIVE = Duration.ofMinutes(1); + /** How long requests are allowed without asking Redis after it failed. */ + private static final Duration PAUSE_AFTER_FAILURE = Duration.ofSeconds(10); + /** Limiters this instance has set up, most recently used kept (bounded: one per project). */ + private static final int MAX_REMEMBERED = 10_000; + + private final RedissonClient redisson; + /** System.nanoTime() until which Redis isn't asked; 0 while it works. */ + private volatile long pausedUntil; + private final Map setUp = Collections.synchronizedMap(new LinkedHashMap<>(16, 0.75f, true) { + @Override + protected boolean removeEldestEntry(Map.Entry eldest) { + return size() > MAX_REMEMBERED; + } + }); + + public RateLimiters(ObjectProvider redisson) { + this.redisson = redisson.getIfAvailable(); + } + + /** @return whether a permit was obtained, waiting at most the limit's max wait */ + public boolean tryAcquire(String name, RateLimitProperties.Limit limit) { + if (redisson == null || isPaused()) { + return true; + } + try { + var limiter = redisson.getRateLimiter(name); + if (!setUp.containsKey(name)) { + setUp(name, limiter, limit); + } + try { + return acquire(limiter, limit); + } catch (RuntimeException e) { + if (!isNotInitialized(e)) { + throw e; + } + // expired from Redis since this instance set it up + setUp(name, limiter, limit); + return acquire(limiter, limit); + } + } catch (RuntimeException e) { + pausedUntil = System.nanoTime() + PAUSE_AFTER_FAILURE.toNanos(); + log.warn("Rate limiter {} unavailable, allowing requests without rate limits for {}: {}", + name, PAUSE_AFTER_FAILURE, e.getMessage()); + return true; + } + } + + private boolean isPaused() { + long until = pausedUntil; + if (until == 0) { + return false; + } + if (System.nanoTime() - until < 0) { + return true; + } + pausedUntil = 0; + return false; + } + + private static Duration keepAlive(RateLimitProperties.Limit limit) { + var periods = limit.period().multipliedBy(KEEP_ALIVE_PERIODS); + return periods.compareTo(MIN_KEEP_ALIVE) > 0 ? periods : MIN_KEEP_ALIVE; + } + + private static boolean acquire(RRateLimiter limiter, RateLimitProperties.Limit limit) { + return limiter.tryAcquireAsync(1, limit.maxWait()).toCompletableFuture().join(); + } + + private void setUp(String name, RRateLimiter limiter, RateLimitProperties.Limit limit) { + var keepAlive = keepAlive(limit); + boolean created = limiter.trySetRateAsync(RateType.OVERALL, limit.permits(), limit.period(), keepAlive) + .toCompletableFuture().join(); + if (!created) { + // an existing limiter keeps its rate unless replaced, e.g. after the configuration changed + var config = limiter.getConfigAsync().toCompletableFuture().join(); + if (config.getRate() != limit.permits() || config.getRateInterval() != limit.period().toMillis()) { + log.info("Updating rate limiter {} to {} per {}", name, limit.permits(), limit.period()); + limiter.setRateAsync(RateType.OVERALL, limit.permits(), limit.period(), keepAlive) + .toCompletableFuture().join(); + } + } + // only once it is set up: a failure above is retried on the next call + setUp.put(name, Boolean.TRUE); + } + + private static boolean isNotInitialized(Throwable e) { + for (Throwable cause = e; cause != null; cause = cause.getCause()) { + if (cause.getMessage() != null && cause.getMessage().contains("not initialized")) { + return true; + } + } + return false; + } +} diff --git a/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java b/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java index 3630122..cdb0b5d 100644 --- a/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java +++ b/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java @@ -4,8 +4,10 @@ import static org.springframework.http.HttpStatus.CONFLICT; import static org.springframework.http.HttpStatus.NOT_FOUND; import static org.springframework.http.HttpStatus.SERVICE_UNAVAILABLE; +import static org.springframework.http.HttpStatus.TOO_MANY_REQUESTS; import com.company.resourceapi.exceptions.NotFoundException; +import com.company.resourceapi.exceptions.RateLimitExceededException; import com.company.resourceapi.exceptions.SyncInProgressException; import lombok.extern.slf4j.Slf4j; import org.springframework.core.task.TaskRejectedException; @@ -31,6 +33,23 @@ ResponseEntity handleSyncInProgress(SyncInProgressException e) { return ResponseEntity.status(CONFLICT).build(); } + /** + * One of our rate limits: the caller's own requests are 429; our calls to an SDLC system being + * limited is a temporary unavailability on our side, 503. + */ + @ExceptionHandler(RateLimitExceededException.class) + ResponseEntity handleRateLimitExceeded(RateLimitExceededException e) { + var caller = e.getScope() == RateLimitExceededException.Scope.CALLER; + // a refused caller is expected, possibly many times a second; holding back our own calls is worth seeing + if (caller) { + log.debug(e.getMessage()); + } else { + log.info(e.getMessage()); + } + var status = caller ? TOO_MANY_REQUESTS : SERVICE_UNAVAILABLE; + return ResponseEntity.status(status).header(RETRY_AFTER, String.valueOf(e.getRetryAfterSeconds())).build(); + } + /** The background task executor is at its concurrency limit; the client can retry shortly. */ @ExceptionHandler(TaskRejectedException.class) ResponseEntity handleTaskRejected(TaskRejectedException e) { diff --git a/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java b/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java new file mode 100644 index 0000000..d552051 --- /dev/null +++ b/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java @@ -0,0 +1,27 @@ +package com.company.resourceapi.web; + +import com.company.resourceapi.config.RateLimitProperties; +import com.company.resourceapi.services.RateLimiters; +import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.boot.web.servlet.FilterRegistrationBean; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.core.Ordered; +import org.springframework.web.servlet.HandlerExceptionResolver; + +/** Registers {@link ClientRateLimitFilter} for the API only (not Swagger UI or the OpenAPI document). */ +@Configuration +public class ClientRateLimitConfig { + + @Bean + FilterRegistrationBean clientRateLimitFilter(RateLimiters rateLimiters, + RateLimitProperties rateLimits, + @Qualifier("handlerExceptionResolver") HandlerExceptionResolver exceptionResolver) { + var registration = new FilterRegistrationBean<>( + new ClientRateLimitFilter(rateLimiters, rateLimits, exceptionResolver)); + registration.addUrlPatterns("/api/*"); + // after the server observation (+1) and trace id (+2) filters + registration.setOrder(Ordered.HIGHEST_PRECEDENCE + 3); + return registration; + } +} diff --git a/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java b/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java new file mode 100644 index 0000000..570a96a --- /dev/null +++ b/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java @@ -0,0 +1,44 @@ +package com.company.resourceapi.web; + +import com.company.resourceapi.config.RateLimitProperties; +import com.company.resourceapi.exceptions.RateLimitExceededException; +import com.company.resourceapi.exceptions.RateLimitExceededException.Scope; +import com.company.resourceapi.services.RateLimiters; +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import java.io.IOException; +import lombok.RequiredArgsConstructor; +import org.springframework.web.filter.OncePerRequestFilter; +import org.springframework.web.servlet.HandlerExceptionResolver; + +/** + * Limits API requests per client IP address, across all instances (429 + Retry-After). Behind a + * reverse proxy the address comes from X-Forwarded-For, which Tomcat only trusts from internal proxy + * addresses ({@code server.forward-headers-strategy=native}), so clients can't pick their own key. + * + *

Registered for {@code /api/*} by {@link ClientRateLimitConfig}, after the trace id filter, so a + * refused request still gets its X-Trace-Id. The refusal is answered like any other + * {@link RateLimitExceededException}, by the application's exception handling. + */ +@RequiredArgsConstructor +public class ClientRateLimitFilter extends OncePerRequestFilter { + + private final RateLimiters rateLimiters; + private final RateLimitProperties rateLimits; + private final HandlerExceptionResolver exceptionResolver; + + @Override + protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, + FilterChain filterChain) throws ServletException, IOException { + var limit = rateLimits.client(); + if (!rateLimiters.tryAcquire("client-rate:" + request.getRemoteAddr(), limit)) { + var refusal = new RateLimitExceededException(Scope.CALLER, + "Too many requests from " + request.getRemoteAddr(), limit.retryAfterSeconds()); + exceptionResolver.resolveException(request, response, null, refusal); + return; + } + filterChain.doFilter(request, response); + } +} diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 5c44835..ade8268 100755 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -22,13 +22,27 @@ spring.data.redis.repositories.enabled=false # explicit: Redisson brings a JCache provider, which Boot would otherwise prefer over Redis spring.cache.type=redis spring.cache.redis.key-prefix=resourceapi:: -spring.cache.redis.cache-null-values=false resourceapi.cache.sdlc-systems-ttl=10m resourceapi.cache.external-projects-ttl=5m +resourceapi.cache.missing-external-projects-ttl=30s # redisson (locks, rate limiters) uses the same Redis; its object names get this prefix resourceapi.redisson.key-prefix=resourceapi: # a project's sync lock expires after this even if never released (e.g. the instance died) resourceapi.sync.lock-lease-time=60s +# distributed rate limits (Redisson): calls to one SDLC system (503 + Retry-After when exceeded after +# waiting max-wait), and sync requests per project (429 + Retry-After) +resourceapi.rate-limit.sdlc.permits=10 +resourceapi.rate-limit.sdlc.period=1s +resourceapi.rate-limit.sdlc.max-wait=1s +resourceapi.rate-limit.sync.permits=6 +resourceapi.rate-limit.sync.period=1m +resourceapi.rate-limit.sync.max-wait=0s +# API requests per client IP (429 + Retry-After); behind a reverse proxy the client IP comes from +# X-Forwarded-For, trusted only from internal proxy addresses (Tomcat's RemoteIpValve) +resourceapi.rate-limit.client.permits=100 +resourceapi.rate-limit.client.period=1s +resourceapi.rate-limit.client.max-wait=0s +server.forward-headers-strategy=native # datasource spring.datasource.url=jdbc:h2:mem:test;MODE=MySQL;DATABASE_TO_LOWER=TRUE spring.sql.init.platform=h2 diff --git a/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java b/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java index 7fa5fff..d15b903 100644 --- a/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java +++ b/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java @@ -38,7 +38,7 @@ import org.springframework.test.web.servlet.MockMvc; /** Redis cache in front of the SDLC systems, against a real Redis (RedisTestContainerInitializer). */ -@SpringBootTest +@SpringBootTest(properties = "resourceapi.cache.missing-external-projects-ttl=3s") @AutoConfigureMockMvc class CacheIntegrationTest { @@ -92,29 +92,65 @@ void givenRepeatedLookupsThenSdlcSystemIsAskedOnce() throws Exception { sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) .willReturn(okJson("{\"key\": \"CACHED\", \"name\": \"From SDLC\"}"))); - for (int i = 0; i < 3; i++) { + mockMvc.perform(get(externalUrl())).andExpect(status().isOk()); + // counted after the first lookup: the HTTP client may retry a GET on a stale pooled connection + int callsForFirstLookup = sdlcSystemServer.findAll(getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))).size(); + + for (int i = 0; i < 2; i++) { mockMvc.perform(get(externalUrl())) .andExpect(status().isOk()) .andExpect(jsonPath("$.name", equalTo("From SDLC"))); } - sdlcSystemServer.verify(1, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + // the next lookups are served from the cache + sdlcSystemServer.verify(callsForFirstLookup, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); // stored as JSON in Redis and read back as the record, not a map assertThat(cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey(), ExternalProject.class)) .isEqualTo(new ExternalProject("CACHED", "From SDLC", null)); } @Test - void givenProjectUnknownToSdlcSystemThenItIsNotCached() throws Exception { + void givenProjectUnknownToSdlcSystemThenThatIsRememberedBriefly() throws Exception { sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) .willReturn(notFound().withHeader("Content-Type", "application/json").withBody("{}"))); mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); + int callsForFirstLookup = sdlcSystemServer.findAll(getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))).size(); mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); - // asked again, so the project is found as soon as it exists in the SDLC system - sdlcSystemServer.verify(2, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); - assertThat(cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey())).isNull(); + // the "unknown" answer is remembered (as a cached null), with the short time to live (3s here) + sdlcSystemServer.verify(callsForFirstLookup, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + var cached = cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey()); + assertThat(cached).isNotNull(); + assertThat(cached.get()).isNull(); + assertThat(redisTemplate.getExpire(redisKey())).isBetween(1L, 3L); + + // the project is created in the SDLC system: found once the "unknown" answer expired + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(okJson("{\"key\": \"CACHED\", \"name\": \"Created later\"}"))); + await(() -> statusOf(externalUrl()) == 200); + // ... and kept with the long time to live (5m) + assertThat(redisTemplate.getExpire(redisKey())).isGreaterThan(60L); + } + + @Test + void givenProjectRememberedAsUnknownWhenSyncedThenItIsFoundRightAway() throws Exception { + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(notFound().withHeader("Content-Type", "application/json").withBody("{}"))); + mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); + + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(okJson("{\"key\": \"CACHED\", \"name\": \"Created later\"}"))); + mockMvc.perform(post(ProjectRestController.ENDPOINT + "/" + projectId + "/sync")) + .andExpect(status().isAccepted()); + await(() -> projectRepository.findById(projectId).orElseThrow().getName().equals("Created later")); + int callsUntilSynced = sdlcSystemServer.findAll(getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))).size(); + + // the sync replaced the "unknown" answer with the project, well within its 3s + assertThat(cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey(), ExternalProject.class)) + .isEqualTo(new ExternalProject("CACHED", "Created later", null)); + mockMvc.perform(get(externalUrl())).andExpect(status().isOk()); + sdlcSystemServer.verify(callsUntilSynced, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); } @Test @@ -158,6 +194,10 @@ private String externalKey() { return sdlcSystem.getId() + ":" + EXTERNAL_ID; } + private String redisKey() { + return keyPrefix + EXTERNAL_PROJECTS + "::" + externalKey(); + } + private static void await(BooleanSupplier condition) throws InterruptedException { var deadline = Instant.now().plus(Duration.ofSeconds(5)); while (!condition.getAsBoolean()) { @@ -191,4 +231,12 @@ void givenProjectGoneFromSdlcSystemWhenSyncedThenCachedEntryIsRemoved() throws E mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); } + + private int statusOf(String url) { + try { + return mockMvc.perform(get(url)).andReturn().getResponse().getStatus(); + } catch (Exception e) { + throw new IllegalStateException(e); + } + } } diff --git a/src/test/java/com/company/resourceapi/ratelimit/ClientRateLimitIntegrationTest.java b/src/test/java/com/company/resourceapi/ratelimit/ClientRateLimitIntegrationTest.java new file mode 100644 index 0000000..5de44fd --- /dev/null +++ b/src/test/java/com/company/resourceapi/ratelimit/ClientRateLimitIntegrationTest.java @@ -0,0 +1,55 @@ +package com.company.resourceapi.ratelimit; + +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import com.company.resourceapi.controllers.ProjectRestController; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.request.RequestPostProcessor; + +/** Requests per client address, shared by all instances through Redis. */ +@SpringBootTest(properties = { + "resourceapi.rate-limit.client.permits=3", + "resourceapi.rate-limit.client.period=2s"}) +@AutoConfigureMockMvc +class ClientRateLimitIntegrationTest { + + private static final String PROJECT_URL = ProjectRestController.ENDPOINT + "/1"; + + @Autowired + private MockMvc mockMvc; + + @Test + void givenTooManyRequestsFromOneClientThenFurtherOnesAreRefused() throws Exception { + for (int i = 0; i < 3; i++) { + mockMvc.perform(get(PROJECT_URL).with(from("10.0.0.1"))).andExpect(status().isOk()); + } + mockMvc.perform(get(PROJECT_URL).with(from("10.0.0.1"))) + .andExpect(status().isTooManyRequests()) + .andExpect(header().string("Retry-After", "2")) + .andExpect(header().exists("X-Trace-Id")); + + // another client has its own limit + mockMvc.perform(get(PROJECT_URL).with(from("10.0.0.2"))).andExpect(status().isOk()); + } + + @Test + void givenNonApiPathsThenTheyAreNotLimited() throws Exception { + for (int i = 0; i < 5; i++) { + mockMvc.perform(get("/v3/api-docs").with(from("10.0.0.3"))).andExpect(status().isOk()); + } + mockMvc.perform(get(PROJECT_URL).with(from("10.0.0.3"))).andExpect(status().isOk()); + } + + private static RequestPostProcessor from(String address) { + return request -> { + request.setRemoteAddr(address); + return request; + }; + } +} diff --git a/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java b/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java new file mode 100644 index 0000000..28884ed --- /dev/null +++ b/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java @@ -0,0 +1,204 @@ +package com.company.resourceapi.ratelimit; + +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.okJson; +import static com.github.tomakehurst.wiremock.client.WireMock.urlPathMatching; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static org.assertj.core.api.Assertions.assertThat; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import com.company.resourceapi.controllers.ProjectRestController; +import com.company.resourceapi.entities.Project; +import com.company.resourceapi.entities.SdlcSystem; +import com.company.resourceapi.repositories.ProjectRepository; +import com.company.resourceapi.repositories.SdlcSystemRepository; +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.junit5.WireMockExtension; +import java.time.Duration; +import java.time.Instant; +import java.util.concurrent.TimeUnit; +import java.util.function.BooleanSupplier; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.redisson.api.RateType; +import org.redisson.api.RedissonClient; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; +import org.springframework.test.web.servlet.MockMvc; + +/** Distributed rate limits against a real Redis; "another instance" uses the same limiter directly. */ +@SpringBootTest(properties = { + "resourceapi.rate-limit.sdlc.permits=2", + "resourceapi.rate-limit.sdlc.period=2s", + "resourceapi.rate-limit.sdlc.max-wait=0s", + "resourceapi.rate-limit.sync.permits=2", + "resourceapi.rate-limit.sync.period=1m"}) +@AutoConfigureMockMvc +class RateLimitIntegrationTest { + + @RegisterExtension + static WireMockExtension sdlcSystemServer = WireMockExtension.newInstance() + .options(wireMockConfig().dynamicPort()) + .build(); + + @Autowired + private MockMvc mockMvc; + + @Autowired + private SdlcSystemRepository sdlcSystemRepository; + + @Autowired + private ProjectRepository projectRepository; + + @Autowired + private RedissonClient redisson; + + private SdlcSystem sdlcSystem; + + @BeforeEach + void setup() { + sdlcSystem = new SdlcSystem(); + sdlcSystem.setBaseUrl(sdlcSystemServer.baseUrl()); + sdlcSystem = sdlcSystemRepository.save(sdlcSystem); + sdlcSystemServer.stubFor(WireMock.get(urlPathMatching("/rest/api/2/project/.*")) + .willReturn(okJson("{\"key\": \"ANY\", \"name\": \"Synced\"}"))); + } + + @Test + void givenTooManyCallsToOneSdlcSystemThenFurtherCallsAreRefused() throws Exception { + mockMvc.perform(get(externalUrl(project("A")))).andExpect(status().isOk()); + mockMvc.perform(get(externalUrl(project("B")))).andExpect(status().isOk()); + mockMvc.perform(get(externalUrl(project("C")))) + .andExpect(status().isServiceUnavailable()) + .andExpect(header().string("Retry-After", "2")); + + sdlcSystemServer.verify(2, getRequestedFor(urlPathMatching("/rest/api/2/project/.*"))); + } + + @Test + void givenPermitsUsedByAnotherInstanceThenThisInstanceIsLimitedToo() throws Exception { + var shared = redisson.getRateLimiter("sdlc-rate:" + sdlcSystem.getId()); + shared.trySetRate(RateType.OVERALL, 2, Duration.ofSeconds(2), Duration.ofHours(1)); + assertThat(shared.tryAcquire(2)).isTrue(); + + mockMvc.perform(get(externalUrl(project("A")))).andExpect(status().isServiceUnavailable()); + + // the permits refill after the period + var url = externalUrl(project("B")); + await(() -> statusOf(url) == 200, Duration.ofSeconds(5)); + } + + @Test + void givenCachedAnswersThenOnlyRealCallsCount() throws Exception { + var url = externalUrl(project("A")); + for (int i = 0; i < 5; i++) { + mockMvc.perform(get(url)).andExpect(status().isOk()); + } + + sdlcSystemServer.verify(1, getRequestedFor(urlPathMatching("/rest/api/2/project/.*"))); + mockMvc.perform(get(externalUrl(project("B")))).andExpect(status().isOk()); + } + + @Test + void givenChangedConfigurationThenExistingLimiterIsUpdated() throws Exception { + var limiter = redisson.getRateLimiter("sdlc-rate:" + sdlcSystem.getId()); + limiter.trySetRate(RateType.OVERALL, 100, Duration.ofSeconds(1), Duration.ofHours(1)); + + mockMvc.perform(get(externalUrl(project("A")))).andExpect(status().isOk()); + + var config = limiter.getConfig(); + assertThat(config.getRate()).isEqualTo(2); + assertThat(config.getRateInterval()).isEqualTo(Duration.ofSeconds(2).toMillis()); + } + + @Test + void givenTooManySyncRequestsForOneProjectThenTheyAreRefused() throws Exception { + long projectId = project("SYNCED"); + var syncUrl = ProjectRestController.ENDPOINT + "/" + projectId + "/sync"; + + for (int i = 0; i < 2; i++) { + mockMvc.perform(post(syncUrl)).andExpect(status().isAccepted()); + // let it finish, so the lock doesn't refuse the next one + await(() -> !redisson.getLock("project-sync:" + projectId).isLocked(), Duration.ofSeconds(5)); + } + mockMvc.perform(post(syncUrl)) + .andExpect(status().isTooManyRequests()) + .andExpect(header().string("Retry-After", "60")); + } + + private long project(String externalId) { + var project = new Project(); + project.setExternalId(externalId); + project.setSdlcSystem(sdlcSystem); + return projectRepository.save(project).getId(); + } + + private static String externalUrl(long projectId) { + return ProjectRestController.ENDPOINT + "/" + projectId + "/external"; + } + + private int statusOf(String url) { + try { + return mockMvc.perform(get(url)).andReturn().getResponse().getStatus(); + } catch (Exception e) { + throw new IllegalStateException(e); + } + } + + private static void await(BooleanSupplier condition, Duration timeout) throws InterruptedException { + var deadline = Instant.now().plus(timeout); + while (!condition.getAsBoolean()) { + assertThat(Instant.now()).as("condition not met within " + timeout).isBefore(deadline); + Thread.sleep(100); + } + } + + @Test + void givenSyncAlreadyRunningThenRefusedRequestDoesNotUseAPermit() throws Exception { + long projectId = project("BUSY"); + var syncUrl = ProjectRestController.ENDPOINT + "/" + projectId + "/sync"; + var otherInstanceLock = redisson.getLock("project-sync:" + projectId); + otherInstanceLock.tryLockAsync(0, 10, TimeUnit.SECONDS, 4714L).toCompletableFuture().join(); + try { + for (int i = 0; i < 3; i++) { + mockMvc.perform(post(syncUrl)).andExpect(status().isConflict()); + } + } finally { + otherInstanceLock.unlockAsync(4714L).toCompletableFuture().join(); + } + + // both permits are still there + mockMvc.perform(post(syncUrl)).andExpect(status().isAccepted()); + // let that sync finish here, not during the next test + await(() -> !otherInstanceLock.isLocked(), Duration.ofSeconds(5)); + } + + @Test + void givenLimiterThenItExpiresFromRedisSoonAfterItsLastUse() throws Exception { + mockMvc.perform(get(externalUrl(project("A")))).andExpect(status().isOk()); + + // 10 periods of 2s is less than the 1-minute minimum + long ttl = redisson.getRateLimiter("sdlc-rate:" + sdlcSystem.getId()).remainTimeToLive(); + assertThat(ttl).isBetween(Duration.ofSeconds(50).toMillis(), Duration.ofMinutes(1).toMillis()); + } + + @Test + void givenLimiterRemovedFromRedisThenItIsSetUpAgain() throws Exception { + mockMvc.perform(get(externalUrl(project("A")))).andExpect(status().isOk()); + var limiter = redisson.getRateLimiter("sdlc-rate:" + sdlcSystem.getId()); + // expired from disuse (or flushed) + assertThat(limiter.delete()).isTrue(); + + mockMvc.perform(get(externalUrl(project("B")))).andExpect(status().isOk()); + + assertThat(limiter.getConfig().getRate()).isEqualTo(2); + // a fresh window: B used one permit, C uses the second, D is refused + mockMvc.perform(get(externalUrl(project("C")))).andExpect(status().isOk()); + mockMvc.perform(get(externalUrl(project("D")))).andExpect(status().isServiceUnavailable()); + } +} diff --git a/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java b/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java new file mode 100644 index 0000000..3540100 --- /dev/null +++ b/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java @@ -0,0 +1,31 @@ +package com.company.resourceapi.ratelimit; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.company.resourceapi.config.RateLimitProperties; +import com.company.resourceapi.config.RateLimitProperties.Limit; +import jakarta.validation.Validation; +import java.time.Duration; +import org.junit.jupiter.api.Test; + +class RateLimitPropertiesTest { + + private final jakarta.validation.Validator validator = Validation.buildDefaultValidatorFactory().getValidator(); + + @Test + void givenInvalidLimitsThenTheyAreRejected() { + var noPermits = new Limit(0, Duration.ofSeconds(1), Duration.ZERO); + var noPeriod = new Limit(5, null, Duration.ZERO); + var valid = new Limit(5, Duration.ofSeconds(1), Duration.ZERO); + + assertThat(validator.validate(new RateLimitProperties(noPermits, valid, valid))).hasSize(1); + assertThat(validator.validate(new RateLimitProperties(valid, noPeriod, valid))).hasSize(1); + assertThat(validator.validate(new RateLimitProperties(valid, valid, valid))).isEmpty(); + } + + @Test + void givenLimitThenRetryAfterIsOneFullWindow() { + assertThat(new Limit(6, Duration.ofMinutes(1), Duration.ZERO).retryAfterSeconds()).isEqualTo(60); + assertThat(new Limit(10, Duration.ofMillis(500), Duration.ZERO).retryAfterSeconds()).isEqualTo(1); + } +} diff --git a/src/test/java/com/company/resourceapi/ratelimit/RateLimitersTest.java b/src/test/java/com/company/resourceapi/ratelimit/RateLimitersTest.java new file mode 100644 index 0000000..6b6f7aa --- /dev/null +++ b/src/test/java/com/company/resourceapi/ratelimit/RateLimitersTest.java @@ -0,0 +1,49 @@ +package com.company.resourceapi.ratelimit; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.company.resourceapi.config.RateLimitProperties.Limit; +import com.company.resourceapi.services.RateLimiters; +import java.time.Duration; +import org.junit.jupiter.api.Test; +import org.redisson.api.RedissonClient; +import org.springframework.beans.factory.ObjectProvider; + +/** Behavior without a working Redis; with one, see RateLimitIntegrationTest. */ +class RateLimitersTest { + + private static final Limit LIMIT = new Limit(1, Duration.ofSeconds(1), Duration.ZERO); + + @Test + void givenRedisFailsThenRequestsAreAllowedWithoutAskingItAgainForAWhile() { + var redisson = mock(RedissonClient.class); + when(redisson.getRateLimiter(anyString())).thenThrow(new IllegalStateException("Redis is down")); + var rateLimiters = new RateLimiters(providerOf(redisson)); + + for (int i = 0; i < 5; i++) { + assertThat(rateLimiters.tryAcquire("test", LIMIT)).isTrue(); + } + + // one failed attempt, then Redis is left alone instead of each request waiting for its timeout + verify(redisson, times(1)).getRateLimiter(anyString()); + } + + @Test + void givenNoRedissonClientThenRequestsAreAllowed() { + var rateLimiters = new RateLimiters(providerOf(null)); + + assertThat(rateLimiters.tryAcquire("test", LIMIT)).isTrue(); + } + + @SuppressWarnings("unchecked") + private static ObjectProvider providerOf(RedissonClient redisson) { + ObjectProvider provider = mock(ObjectProvider.class); + when(provider.getIfAvailable()).thenReturn(redisson); + return provider; + } +} diff --git a/src/test/java/com/company/resourceapi/services/ProjectSyncBackpressureTest.java b/src/test/java/com/company/resourceapi/services/ProjectSyncBackpressureTest.java index 0781f63..947f5fa 100644 --- a/src/test/java/com/company/resourceapi/services/ProjectSyncBackpressureTest.java +++ b/src/test/java/com/company/resourceapi/services/ProjectSyncBackpressureTest.java @@ -24,7 +24,10 @@ import org.springframework.test.web.servlet.MockMvc; /** Background syncs are bounded: beyond the executor's limit they are rejected, not queued. */ -@SpringBootTest(properties = "spring.task.execution.simple.concurrency-limit=1") +@SpringBootTest(properties = { + "spring.task.execution.simple.concurrency-limit=1", + // this test retries refused syncs; the per-project sync rate limit isn't what it is about + "resourceapi.rate-limit.sync.permits=1000"}) @AutoConfigureMockMvc class ProjectSyncBackpressureTest { diff --git a/src/test/resources/config/application.properties b/src/test/resources/config/application.properties index c2a9ab1..67b2d67 100644 --- a/src/test/resources/config/application.properties +++ b/src/test/resources/config/application.properties @@ -1,2 +1,4 @@ # Each test application context gets its own in-memory database so schema.sql/data.sql run on a clean slate spring.datasource.url=jdbc:h2:mem:test-${random.uuid};MODE=MySQL;DATABASE_TO_LOWER=TRUE +# tests send many requests from one address; ClientRateLimitIntegrationTest sets its own limit +resourceapi.rate-limit.client.permits=100000