From db109e5456d1ec8546b1217f419257ab067e1a66 Mon Sep 17 00:00:00 2001 From: Sushaanth Srinivasan <80441330+SushaanthSrinivasan@users.noreply.github.com> Date: Mon, 28 Sep 2026 09:49:24 -0700 Subject: [PATCH] fix: set a 2 second API call attempt timeout on clients the cache builds The SDK sets no attempt timeout by default, so an endpoint that accepts the connection and never answers could block getSecretString for about 2 minutes across retries. Clients the cache builds now get a 2 second apiCallAttemptTimeout unless their builder sets one, and share one daemon timer thread. A built client passed in is used unchanged. --- README.md | 15 ++ .../secretsmanager/caching/SecretCache.java | 80 ++++++++-- .../caching/SecretCacheConfiguration.java | 11 ++ .../caching/SecretCacheTest.java | 141 ++++++++++++++++++ 4 files changed, 236 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index ba7796f..a534045 100644 --- a/README.md +++ b/README.md @@ -106,6 +106,21 @@ String secret = cache.getSecretString("my-secret-id"); **NOTE**: PQ-TLS uses the AWS Common Runtime (CRT) which relies on system libraries and may not work as expected on macOS or Windows at this time ([ref](https://github.com/awslabs/aws-crt-java#tls-behavior)). +### Request Timeouts + +When the cache builds the Secrets Manager client, each API call attempt times out after `SecretCacheConfiguration.DEFAULT_API_CALL_ATTEMPT_TIMEOUT` (2 seconds). That covers `new SecretCache()`, a `SecretCacheConfiguration` with no client (including the Post-Quantum TLS example above), and a `SecretsManagerClientBuilder` you pass in. The SDK sets no attempt timeout by default, so without this an endpoint that stops answering can block the call for a long time. A timed out attempt is retried like any other network error. + +To use a different value, pass in a builder that sets `apiCallAttemptTimeout`; the cache keeps your value. The SDK has no setting that turns the timeout off, so use a large value if you expect slow calls, or pass in a built client. Only `apiCallAttemptTimeout` counts: a builder that sets just `apiCallTimeout` or HTTP client timeouts still gets the 2 second attempt timeout. + +If you pass in a built `SecretsManagerClient`, the cache uses it unchanged, so set the timeout yourself: + +```java +SecretsManagerClient client = SecretsManagerClient.builder() + .overrideConfiguration(c -> c.apiCallAttemptTimeout(Duration.ofSeconds(2))) + .build(); +SecretCache cache = new SecretCache(client); +``` + ## License This library is licensed under the Apache 2.0 License. diff --git a/src/main/java/com/amazonaws/secretsmanager/caching/SecretCache.java b/src/main/java/com/amazonaws/secretsmanager/caching/SecretCache.java index ad79122..725d56d 100644 --- a/src/main/java/com/amazonaws/secretsmanager/caching/SecretCache.java +++ b/src/main/java/com/amazonaws/secretsmanager/caching/SecretCache.java @@ -13,6 +13,9 @@ package com.amazonaws.secretsmanager.caching; import java.nio.ByteBuffer; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledThreadPoolExecutor; +import java.util.concurrent.TimeUnit; import com.amazonaws.secretsmanager.caching.cache.LRUCache; import com.amazonaws.secretsmanager.caching.cache.SecretCacheItem; @@ -55,9 +58,18 @@ public class SecretCache implements AutoCloseable { /** The AWS Secrets Manager client to use when requesting secrets. */ private final SecretsManagerClient client; + /** + * Runs the API call attempt timeouts of every client the cache builds, unless its + * builder sets its own executor. Without it, each client would start its own pool + * of SDK timer threads, which only stop when the client is closed, and the cache + * does not close its client. The SDK never shuts down an executor it is given. + */ + private static final ScheduledExecutorService TIMEOUT_EXECUTOR = timeoutExecutor(); + /** * Constructs a new secret cache using the standard AWS Secrets Manager client - * with default options. + * with default options, and + * {@link SecretCacheConfiguration#DEFAULT_API_CALL_ATTEMPT_TIMEOUT}. */ public SecretCache() { this(SecretsManagerClient.builder()); @@ -66,7 +78,12 @@ public SecretCache() { /** * Constructs a new secret cache using an AWS Secrets Manager client created * using the - * provided builder. + * provided builder. Unless the builder sets its own API call attempt timeout, + * {@link SecretCacheConfiguration#DEFAULT_API_CALL_ATTEMPT_TIMEOUT} is used. The + * timeout, user agent and, unless the builder sets its own scheduled executor, the + * cache's shared timeout executor are written into the builder itself, so other + * clients later built from it get them too. Do not shut that executor down; every + * cache uses it. * * @param builder The builder to use for creating the AWS Secrets Manager * client. @@ -75,10 +92,7 @@ public SecretCache() { justification = "Delegates to constructor that validates before field initialization") public SecretCache(SecretsManagerClientBuilder builder) { this(new SecretCacheConfiguration().withClient(builder - .overrideConfiguration( - builder.overrideConfiguration().toBuilder() - .putAdvancedOption(SdkAdvancedClientOption.USER_AGENT_SUFFIX, VersionInfo.USER_AGENT) - .build()) + .overrideConfiguration(withCacheDefaults(builder.overrideConfiguration())) .build())); } @@ -95,7 +109,9 @@ public SecretCache(SecretsManagerClient client) { } /** - * Constructs a new secret cache using the provided cache configuration. + * Constructs a new secret cache using the provided cache configuration. If the + * configuration has no client, the cache builds one with + * {@link SecretCacheConfiguration#DEFAULT_API_CALL_ATTEMPT_TIMEOUT}. * * @param config The secret cache configuration. * @throws IllegalArgumentException if both a custom client and postQuantumTlsEnabled @@ -116,9 +132,6 @@ public SecretCache(SecretCacheConfiguration config) { this.cache = new LRUCache(config.getMaxCacheSize()); this.config = config; - ClientOverrideConfiguration defaultOverride = ClientOverrideConfiguration.builder() - .putAdvancedOption(SdkAdvancedClientOption.USER_AGENT_SUFFIX, VersionInfo.USER_AGENT).build(); - if (config.getClient() != null) { this.client = config.getClient(); } else { @@ -130,8 +143,53 @@ public SecretCache(SecretCacheConfiguration config) { .build()); } - this.client = builder.overrideConfiguration(defaultOverride).build(); + this.client = builder.overrideConfiguration( + withCacheDefaults(ClientOverrideConfiguration.builder().build())).build(); + } + } + + /** + * Adds the cache's user agent suffix to the given override configuration and, + * unless it already sets them, the default API call attempt timeout and the + * cache's shared timeout executor. + * + * @param override The override configuration of the client being built. + * @return The override configuration to build the client with. + */ + private static ClientOverrideConfiguration withCacheDefaults(ClientOverrideConfiguration override) { + ClientOverrideConfiguration.Builder builder = override.toBuilder() + .putAdvancedOption(SdkAdvancedClientOption.USER_AGENT_SUFFIX, VersionInfo.USER_AGENT); + if (!override.apiCallAttemptTimeout().isPresent()) { + builder.apiCallAttemptTimeout(SecretCacheConfiguration.DEFAULT_API_CALL_ATTEMPT_TIMEOUT); } + if (!override.scheduledExecutorService().isPresent()) { + builder.scheduledExecutorService(TIMEOUT_EXECUTOR); + } + return builder.build(); + } + + /** + * Creates the shared timeout executor. Its one daemon thread is started by the + * first timed request and exits after a minute with nothing to do, so an + * application server can unload the application. Cancelled timeouts are removed + * straight away. + * + * @return The shared timeout executor. + */ + private static ScheduledExecutorService timeoutExecutor() { + ScheduledThreadPoolExecutor executor = new ScheduledThreadPoolExecutor(1, runnable -> { + Thread thread = new Thread(runnable, "aws-secretsmanager-caching-timeout"); + thread.setDaemon(true); + // Don't keep the class loader of whichever application thread happened to start it. + thread.setContextClassLoader(SecretCache.class.getClassLoader()); + return thread; + }); + executor.setRemoveOnCancelPolicy(true); + // The keep-alive must be set first: it is 0 by default on Java 8, which + // allowCoreThreadTimeOut rejects. + executor.setKeepAliveTime(1, TimeUnit.MINUTES); + executor.allowCoreThreadTimeOut(true); + return executor; } /** diff --git a/src/main/java/com/amazonaws/secretsmanager/caching/SecretCacheConfiguration.java b/src/main/java/com/amazonaws/secretsmanager/caching/SecretCacheConfiguration.java index 325be90..230f190 100644 --- a/src/main/java/com/amazonaws/secretsmanager/caching/SecretCacheConfiguration.java +++ b/src/main/java/com/amazonaws/secretsmanager/caching/SecretCacheConfiguration.java @@ -13,6 +13,7 @@ package com.amazonaws.secretsmanager.caching; +import java.time.Duration; import java.util.concurrent.TimeUnit; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; @@ -39,6 +40,16 @@ public class SecretCacheConfiguration { */ public static final long DEFAULT_FORCE_REFRESH_JITTER = 100; + /** + * The default timeout for each API call attempt made by a client that the cache + * builds, unless the client builder already sets one. The SDK has no attempt + * timeout by default, so an endpoint that accepts a connection and never answers + * blocks the caller until the HTTP client's own timeouts, if any, give up. A timed + * out attempt is retried like any other network error. A client passed in + * directly keeps its own timeouts. + */ + public static final Duration DEFAULT_API_CALL_ATTEMPT_TIMEOUT = Duration.ofSeconds(2); + /** The client this cache instance will use for accessing AWS Secrets Manager. */ private SecretsManagerClient client = null; diff --git a/src/test/java/com/amazonaws/secretsmanager/caching/SecretCacheTest.java b/src/test/java/com/amazonaws/secretsmanager/caching/SecretCacheTest.java index 3e96373..814f24d 100644 --- a/src/test/java/com/amazonaws/secretsmanager/caching/SecretCacheTest.java +++ b/src/test/java/com/amazonaws/secretsmanager/caching/SecretCacheTest.java @@ -13,14 +13,20 @@ package com.amazonaws.secretsmanager.caching; +import java.net.InetAddress; +import java.net.ServerSocket; +import java.net.URI; import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; +import java.time.Duration; import java.util.Arrays; import java.util.Collection; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.TimeUnit; import java.util.function.IntConsumer; +import java.util.stream.Collectors; import org.mockito.ArgumentMatcher; import org.mockito.ArgumentMatchers; @@ -31,7 +37,12 @@ import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.awscore.retry.AwsRetryStrategy; import software.amazon.awssdk.core.SdkBytes; +import software.amazon.awssdk.core.exception.ApiCallAttemptTimeoutException; +import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.secretsmanager.SecretsManagerClient; import software.amazon.awssdk.services.secretsmanager.SecretsManagerClientBuilder; import software.amazon.awssdk.services.secretsmanager.model.DescribeSecretRequest; @@ -526,4 +537,134 @@ public void testSecretCacheWithPQTLSEnabledNoClient() { } } } + + /** + * Opens a socket that is never accepted. It completes the TCP handshake but + * never answers, like an endpoint that hangs after connecting. + */ + private static ServerSocket hungEndpoint() throws Exception { + return new ServerSocket(0, 50, InetAddress.getByName("127.0.0.1")); + } + + private static SecretsManagerClientBuilder hungEndpointClientBuilder(ServerSocket hung) { + return SecretsManagerClient.builder() + .endpointOverride(URI.create("http://127.0.0.1:" + hung.getLocalPort())) + .region(Region.US_WEST_2) + .credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create("akid", "secret"))); + } + + private static long millisToTimeOut(SecretCache cache) { + long start = System.nanoTime(); + Assert.expectThrows(ApiCallAttemptTimeoutException.class, () -> cache.getSecretString("test")); + return TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start); + } + + @Test + public void testDefaultApiCallAttemptTimeout() { + Assert.assertEquals(SecretCacheConfiguration.DEFAULT_API_CALL_ATTEMPT_TIMEOUT, Duration.ofSeconds(2)); + } + + @Test + public void testBuilderClientTimesOutOnHungEndpoint() throws Exception { + try (ServerSocket hung = hungEndpoint(); + SecretCache cache = new SecretCache(hungEndpointClientBuilder(hung) + .overrideConfiguration(c -> c.retryStrategy(AwsRetryStrategy.doNotRetry())))) { + long elapsed = millisToTimeOut(cache); + Assert.assertTrue(elapsed >= 2000 && elapsed < 5000, "Took " + elapsed + " ms to give up"); + } + } + + @Test + public void testBuilderClientRetriesTimedOutAttempt() throws Exception { + try (ServerSocket hung = hungEndpoint(); + SecretCache cache = new SecretCache(hungEndpointClientBuilder(hung) + .overrideConfiguration(c -> c.retryStrategy( + AwsRetryStrategy.standardRetryStrategy().toBuilder().maxAttempts(2).build())))) { + // Two 2 second attempts, plus a short backoff between them. + long elapsed = millisToTimeOut(cache); + Assert.assertTrue(elapsed >= 4000 && elapsed < 8000, "Took " + elapsed + " ms to give up"); + } + } + + @Test + public void testBuilderKeepsOwnAttemptTimeout() throws Exception { + try (ServerSocket hung = hungEndpoint(); + SecretCache cache = new SecretCache(hungEndpointClientBuilder(hung) + .overrideConfiguration(c -> c.retryStrategy(AwsRetryStrategy.doNotRetry()) + .apiCallAttemptTimeout(Duration.ofMillis(200))))) { + long elapsed = millisToTimeOut(cache); + Assert.assertTrue(elapsed < 2000, + "Took " + elapsed + " ms to give up, so the builder's own timeout was replaced"); + } + } + + @Test + public void testDefaultClientTimesOutOnHungEndpoint() throws Exception { + String[] names = {"aws.region", "aws.endpointUrl", "aws.accessKeyId", "aws.secretAccessKey", "aws.maxAttempts"}; + Map original = new HashMap<>(); + for (String name : names) { + original.put(name, System.getProperty(name)); + } + try (ServerSocket hung = hungEndpoint()) { + System.setProperty("aws.region", "us-west-2"); + System.setProperty("aws.endpointUrl", "http://127.0.0.1:" + hung.getLocalPort()); + System.setProperty("aws.accessKeyId", "akid"); + System.setProperty("aws.secretAccessKey", "secret"); + System.setProperty("aws.maxAttempts", "1"); + try (SecretCache cache = new SecretCache(new SecretCacheConfiguration())) { + long elapsed = millisToTimeOut(cache); + Assert.assertTrue(elapsed >= 2000 && elapsed < 5000, "Took " + elapsed + " ms to give up"); + } + } finally { + for (String name : names) { + if (original.get(name) != null) { + System.setProperty(name, original.get(name)); + } else { + System.clearProperty(name); + } + } + } + } + + private static List threadsNamed(String prefix) { + return Thread.getAllStackTraces().keySet().stream() + .filter(t -> t.getName().startsWith(prefix)) + .collect(Collectors.toList()); + } + + @Test + public void testCachesShareOneTimeoutExecutor() throws Exception { + // Each client would otherwise start its own SDK timer threads, which stay + // alive because the cache doesn't close its client. That includes a builder + // that sets its own timeout. + int sdkThreadsBefore = threadsNamed("sdk-ScheduledExecutor").size(); + try (ServerSocket hung = hungEndpoint()) { + for (int i = 0; i < 3; i++) { + try (SecretCache cache = new SecretCache(hungEndpointClientBuilder(hung) + .overrideConfiguration(c -> c.retryStrategy(AwsRetryStrategy.doNotRetry()))); + SecretCache ownTimeout = new SecretCache(hungEndpointClientBuilder(hung) + .overrideConfiguration(c -> c.retryStrategy(AwsRetryStrategy.doNotRetry()) + .apiCallAttemptTimeout(Duration.ofMillis(200))))) { + millisToTimeOut(cache); + millisToTimeOut(ownTimeout); + } + } + } + Assert.assertEquals(threadsNamed("sdk-ScheduledExecutor").size(), sdkThreadsBefore); + + List cacheThreads = threadsNamed("aws-secretsmanager-caching-timeout"); + Assert.assertEquals(cacheThreads.size(), 1); + Assert.assertTrue(cacheThreads.get(0).isDaemon()); + Assert.assertSame(cacheThreads.get(0).getContextClassLoader(), SecretCache.class.getClassLoader()); + } + + @Test + public void testCacheStillWorksAfterClose() { + Mockito.when(asm.describeSecret(Mockito.any(DescribeSecretRequest.class))).thenReturn(describeSecretResponse); + Mockito.when(describeSecretResponse.versionIdsToStages()).thenReturn(null); + SecretCache cache = new SecretCache(asm); + cache.close(); + Assert.assertNull(cache.getSecretString("test")); + Mockito.verify(asm, Mockito.never()).close(); + } }