diff --git a/README.md b/README.md index 3b7c054..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. @@ -104,10 +103,20 @@ available again one period later, which is also what `Retry-After` says): |---|---|---| | 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 an hour. 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). +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 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 index 451a044..b580eeb 100644 --- a/src/main/java/com/company/resourceapi/config/RateLimitProperties.java +++ b/src/main/java/com/company/resourceapi/config/RateLimitProperties.java @@ -13,15 +13,16 @@ * 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 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) { +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 diff --git a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java index f1d428e..88d39e2 100644 --- a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java +++ b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java @@ -10,10 +10,8 @@ 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; /** @@ -28,22 +26,17 @@ public class ExternalProjectService { 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 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); } diff --git a/src/main/java/com/company/resourceapi/services/RateLimiters.java b/src/main/java/com/company/resourceapi/services/RateLimiters.java index f5bcefa..787c239 100644 --- a/src/main/java/com/company/resourceapi/services/RateLimiters.java +++ b/src/main/java/com/company/resourceapi/services/RateLimiters.java @@ -18,18 +18,24 @@ *

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. + * 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. */ - private static final Duration KEEP_ALIVE = Duration.ofHours(1); + /** 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) { @@ -43,7 +49,7 @@ public RateLimiters(ObjectProvider redisson) { /** @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) { + if (redisson == null || isPaused()) { return true; } try { @@ -62,17 +68,36 @@ public boolean tryAcquire(String name, RateLimitProperties.Limit limit) { return acquire(limiter, limit); } } catch (RuntimeException e) { - log.warn("Rate limiter {} unavailable, allowing the request: {}", name, e.getMessage()); + 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 = Duration.ofMillis(Math.max(KEEP_ALIVE.toMillis(), limit.period().toMillis())); + var keepAlive = keepAlive(limit); boolean created = limiter.trySetRateAsync(RateType.OVERALL, limit.permits(), limit.period(), keepAlive) .toCompletableFuture().join(); if (!created) { diff --git a/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java b/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java index 50ed5a8..cdb0b5d 100644 --- a/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java +++ b/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java @@ -39,8 +39,14 @@ ResponseEntity handleSyncInProgress(SyncInProgressException e) { */ @ExceptionHandler(RateLimitExceededException.class) ResponseEntity handleRateLimitExceeded(RateLimitExceededException e) { - log.info(e.getMessage()); - var status = e.getScope() == RateLimitExceededException.Scope.CALLER ? TOO_MANY_REQUESTS : SERVICE_UNAVAILABLE; + 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(); } 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 1cd9fae..ade8268 100755 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -22,9 +22,9 @@ 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) @@ -37,6 +37,12 @@ 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 index 0c18a57..28884ed 100644 --- a/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java +++ b/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java @@ -178,6 +178,15 @@ void givenSyncAlreadyRunningThenRefusedRequestDoesNotUseAPermit() throws Excepti 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()); diff --git a/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java b/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java index b6a8b81..3540100 100644 --- a/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java +++ b/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java @@ -18,9 +18,9 @@ void givenInvalidLimitsThenTheyAreRejected() { 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))).hasSize(1); - assertThat(validator.validate(new RateLimitProperties(valid, noPeriod))).hasSize(1); - assertThat(validator.validate(new RateLimitProperties(valid, valid))).isEmpty(); + 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 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/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