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(); + } }