diff --git a/src/Amazon.SecretsManager.Extensions.Caching/ISecretsManagerCache.cs b/src/Amazon.SecretsManager.Extensions.Caching/ISecretsManagerCache.cs index 7a090ca..7971740 100644 --- a/src/Amazon.SecretsManager.Extensions.Caching/ISecretsManagerCache.cs +++ b/src/Amazon.SecretsManager.Extensions.Caching/ISecretsManagerCache.cs @@ -25,10 +25,13 @@ public interface ISecretsManagerCache : IDisposable { /// - /// Returns the cache entry corresponding to the specified secret if it exists in the cache. + /// Asynchronously returns the cache entry corresponding to the specified secret if it exists in the cache. /// Otherwise, the secret value is fetched from Secrets Manager and a new cache entry is created. /// - SecretCacheItem GetCachedSecret(string secretId); + /// The secret identifier (ARN or friendly name). + /// A token to cancel the asynchronous operation. + /// A task that represents the asynchronous operation. The task result contains the for the specified secret. + Task GetCachedSecret(string secretId, CancellationToken cancellationToken = default); /// /// Asynchronously retrieves the specified SecretBinary after calling . diff --git a/src/Amazon.SecretsManager.Extensions.Caching/SecretCacheItem.cs b/src/Amazon.SecretsManager.Extensions.Caching/SecretCacheItem.cs index 29dbd36..5ce4f68 100755 --- a/src/Amazon.SecretsManager.Extensions.Caching/SecretCacheItem.cs +++ b/src/Amazon.SecretsManager.Extensions.Caching/SecretCacheItem.cs @@ -25,8 +25,9 @@ namespace Amazon.SecretsManager.Extensions.Caching /// public class SecretCacheItem : SecretCacheObject { - /// The cached secret value versions for this cached secret. + /// The cached secret value versions for this cached secret. private readonly MemoryCache versions = new MemoryCache(new MemoryCacheOptions()); + private readonly SemaphoreSlim versionsLock = new SemaphoreSlim(1, 1); private const ushort MAX_VERSIONS_CACHE_SIZE = 10; public SecretCacheItem(String secretId, IAmazonSecretsManager client, SecretCacheConfiguration config) @@ -48,7 +49,7 @@ protected override async Task ExecuteRefreshAsync(Cancel /// protected override async Task GetSecretValueAsync(DescribeSecretResponse result, CancellationToken cancellationToken = default) { - SecretCacheVersion version = GetVersion(result); + SecretCacheVersion version = await GetVersion(result, cancellationToken); if (version == null) { return null; @@ -72,10 +73,13 @@ public override bool Equals(object obj) } /// - /// Retrieves the SecretCacheVersion corresponding to the Version Stage - /// specified by the SecretCacheConfiguration. + /// Asynchronously retrieves the corresponding to the version stage + /// specified by the . /// - private SecretCacheVersion GetVersion(DescribeSecretResponse describeResult) + /// The describe secret response containing version-to-stage mappings. + /// A token to cancel the asynchronous operation. + /// A task that represents the asynchronous operation. The task result contains the matching , or null if no version matches the configured stage. + private async Task GetVersion(DescribeSecretResponse describeResult, CancellationToken cancellationToken = default) { if (null == describeResult?.VersionIdsToStages) return null; String currentVersionId = null; @@ -90,13 +94,25 @@ private SecretCacheVersion GetVersion(DescribeSecretResponse describeResult) if (currentVersionId != null) { SecretCacheVersion version = versions.Get(currentVersionId); - if (null == version) + if (version == null) { - version = versions.Set(currentVersionId, new SecretCacheVersion(secretId, currentVersionId, client, config)); - if (versions.Count > MAX_VERSIONS_CACHE_SIZE) + await this.versionsLock.WaitAsync(cancellationToken); + try { - TrimCacheToSizeLimit(); + version = versions.GetOrCreate(currentVersionId, entry => + { + return new SecretCacheVersion(secretId, currentVersionId, client, config); + }); + + if (versions.Count > MAX_VERSIONS_CACHE_SIZE) + { + TrimCacheToSizeLimit(); + } } + finally + { + this.versionsLock.Release(); + } } return version; } @@ -105,7 +121,7 @@ private SecretCacheVersion GetVersion(DescribeSecretResponse describeResult) private void TrimCacheToSizeLimit() { - versions.Compact((double)(versions.Count - config.MaxCacheSize) / versions.Count); + versions.Compact((double)(versions.Count - MAX_VERSIONS_CACHE_SIZE) / versions.Count); } } -} \ No newline at end of file +} diff --git a/src/Amazon.SecretsManager.Extensions.Caching/SecretCacheObject.cs b/src/Amazon.SecretsManager.Extensions.Caching/SecretCacheObject.cs index 70f261a..06502fc 100755 --- a/src/Amazon.SecretsManager.Extensions.Caching/SecretCacheObject.cs +++ b/src/Amazon.SecretsManager.Extensions.Caching/SecretCacheObject.cs @@ -25,12 +25,12 @@ public abstract class SecretCacheObject private readonly JitteredDelay exceptionJitteredDelay; private readonly JitteredDelay forceRefreshJitteredDelay; + /// A private object to synchronize access to certain methods. + private readonly SemaphoreSlim objLock; + /// The secret identifier for this cached object. protected String secretId; - /// A private object to synchronize access to certain methods. - protected static readonly SemaphoreSlim Lock = new SemaphoreSlim(1,1); - /// The AWS Secrets Manager client to use for requesting secrets. protected IAmazonSecretsManager client; @@ -57,8 +57,6 @@ public abstract class SecretCacheObject /// The time to wait before retrying a failed AWS Secrets Manager request. private DateTime nextRetryTime = DateTime.MinValue; - public static readonly ThreadLocal random = new ThreadLocal(() => new Random(Environment.TickCount)); - /// @@ -79,6 +77,7 @@ public SecretCacheObject(String secretId, IAmazonSecretsManager client, SecretCa this.forceRefreshJitteredDelay = new JitteredDelay( config.ForceRefreshDelayBase, config.ForceRefreshDelayJitter); + this.objLock = new SemaphoreSlim(1,1); } protected abstract Task ExecuteRefreshAsync(CancellationToken cancellationToken = default); @@ -162,7 +161,6 @@ private async Task RefreshAsync(CancellationToken cancellationToken = defa /// Thrown when the is cancelled during the backoff delay. public async Task RefreshNowAsync(CancellationToken cancellationToken = default) { - refreshNeeded = true; // When forcing a refresh, always sleep with a random jitter // to prevent coding errors that could be calling refreshNow // in a loop. @@ -184,14 +182,15 @@ public async Task RefreshNowAsync(CancellationToken cancellationToken = de // Perform the requested refresh. bool success = false; - await Lock.WaitAsync(cancellationToken); + await objLock.WaitAsync(cancellationToken); + refreshNeeded = true; try { success = await RefreshAsync(cancellationToken); } finally { - Lock.Release(); + objLock.Release(); } return (null == exception && success); } @@ -204,14 +203,14 @@ public async Task RefreshNowAsync(CancellationToken cancellationToken = de public async Task GetSecretValue(CancellationToken cancellationToken) { bool success = false; - await Lock.WaitAsync(cancellationToken); + await objLock.WaitAsync(cancellationToken); try { success = await RefreshAsync(cancellationToken); } finally { - Lock.Release(); + objLock.Release(); } if (!success && null == data && null != exception) diff --git a/src/Amazon.SecretsManager.Extensions.Caching/SecretsManagerCache.cs b/src/Amazon.SecretsManager.Extensions.Caching/SecretsManagerCache.cs index ce77a6d..a5252e6 100755 --- a/src/Amazon.SecretsManager.Extensions.Caching/SecretsManagerCache.cs +++ b/src/Amazon.SecretsManager.Extensions.Caching/SecretsManagerCache.cs @@ -29,6 +29,7 @@ public class SecretsManagerCache : ISecretsManagerCache private readonly SecretCacheConfiguration config; private readonly MemoryCacheEntryOptions cacheItemPolicy; private readonly MemoryCache cache = new MemoryCache(new MemoryCacheOptions{ CompactionPercentage = 0 }); + private readonly SemaphoreSlim cacheLock = new SemaphoreSlim(1,1); /// /// Initializes a new instance of the class. @@ -88,9 +89,12 @@ public void Dispose() /// /// Asynchronously retrieves the specified SecretString after calling . /// + /// The secret identifier (ARN or friendly name). + /// A token to cancel the asynchronous operation. + /// A task that represents the asynchronous operation. The task result contains the secret string value, or null if not present. public async Task GetSecretString(String secretId, CancellationToken cancellationToken = default) { - SecretCacheItem secret = GetCachedSecret(secretId); + SecretCacheItem secret = await GetCachedSecret(secretId, cancellationToken); GetSecretValueResponse response = null; response = await secret.GetSecretValue(cancellationToken); return response?.SecretString; @@ -99,38 +103,62 @@ public async Task GetSecretString(String secretId, CancellationToken can /// /// Asynchronously retrieves the specified SecretBinary after calling . /// + /// The secret identifier (ARN or friendly name). + /// A token to cancel the asynchronous operation. + /// A task that represents the asynchronous operation. The task result contains the secret binary value, or null if not present. public async Task GetSecretBinary(String secretId, CancellationToken cancellationToken = default) { - SecretCacheItem secret = GetCachedSecret(secretId); + SecretCacheItem secret = await GetCachedSecret(secretId, cancellationToken); GetSecretValueResponse response = null; response = await secret.GetSecretValue(cancellationToken); return response?.SecretBinary?.ToArray(); } /// - /// Requests the secret value from SecretsManager asynchronously and updates the cache entry with any changes. + /// Requests the secret value from Secrets Manager asynchronously and updates the cache entry with any changes. /// If there is no existing cache entry, a new one is created. - /// Returns true or false depending on if the refresh is successful. /// + /// The secret identifier (ARN or friendly name). + /// A token to cancel the asynchronous operation. + /// A task that represents the asynchronous operation. The task result is true if the refresh succeeded; otherwise, false. public async Task RefreshNowAsync(String secretId, CancellationToken cancellationToken = default) { - return await GetCachedSecret(secretId).RefreshNowAsync(cancellationToken); + SecretCacheItem secretCacheItem = await GetCachedSecret(secretId, cancellationToken); + return await secretCacheItem.RefreshNowAsync(cancellationToken); } /// - /// Returns the cache entry corresponding to the specified secret if it exists in the cache. + /// Asynchronously returns the cache entry corresponding to the specified secret if it exists in the cache. /// Otherwise, the secret value is fetched from Secrets Manager and a new cache entry is created. /// - public SecretCacheItem GetCachedSecret(string secretId) + /// The secret identifier (ARN or friendly name). + /// A token to cancel the asynchronous operation. + /// A task that represents the asynchronous operation. The task result contains the for the specified secret. + public async Task GetCachedSecret(string secretId, CancellationToken cancellationToken = default) { SecretCacheItem secret = cache.Get(secretId); + if (secret == null) { - secret = cache.Set(secretId, new SecretCacheItem(secretId, secretsManager, config), cacheItemPolicy); - if (cache.Count > config.MaxCacheSize) + await this.cacheLock.WaitAsync(cancellationToken); + + try + { + secret = cache.GetOrCreate(secretId, entry => + { + entry.SetOptions(cacheItemPolicy); + return new SecretCacheItem(secretId, secretsManager, config); + }); + + if (cache.Count > config.MaxCacheSize) + { + // Trim cache size to MaxCacheSize, evicting entries using LRU. + cache.Compact((double)(cache.Count - config.MaxCacheSize) / cache.Count); + } + } + finally { - // Trim cache size to MaxCacheSize, evicting entries using LRU. - cache.Compact((double)(cache.Count - config.MaxCacheSize) / cache.Count); + this.cacheLock.Release(); } } diff --git a/test/Amazon.SecretsManager.Extensions.Caching.UnitTests/CacheTests.cs b/test/Amazon.SecretsManager.Extensions.Caching.UnitTests/CacheTests.cs index 263d4a1..73648ee 100755 --- a/test/Amazon.SecretsManager.Extensions.Caching.UnitTests/CacheTests.cs +++ b/test/Amazon.SecretsManager.Extensions.Caching.UnitTests/CacheTests.cs @@ -17,6 +17,7 @@ namespace Amazon.SecretsManager.Extensions.Caching.UnitTests using System.Collections.Generic; using System.IO; using System.Linq; + using System.Reflection; using System.Threading; using System.Threading.Tasks; using Amazon.Runtime; @@ -585,7 +586,7 @@ public async Task HookSecretCacheTest() .ReturnsAsync(describeSecretResponse1) .ReturnsAsync(describeSecretResponse1) .ThrowsAsync(new AmazonSecretsManagerException("This should not be called")); - + TestHook testHook = new TestHook(); SecretsManagerCache cache = new SecretsManagerCache(secretsManager.Object, new SecretCacheConfiguration { CacheHook = testHook }); @@ -601,5 +602,227 @@ public async Task HookSecretCacheTest() } Assert.Equal(4, testHook.GetCount()); } + + [Fact] + public async Task VersionsCacheTrimsBeyondMaxSize() + { + // Verify that the internal versions cache is trimmed when it goes over the maximum allotted size + // + // Uses RefreshNowAsync on the same SecretCacheItem to force + // repeated DescribeSecret calls, each returning a different version ID. + // This accumulates versions in the same SecretCacheItem's internal cache. + int describeCallCount = 0; + int getSecretValueCallCount = 0; + Mock secretsManager = new Mock(MockBehavior.Strict); + + secretsManager.Setup(i => i.DescribeSecretAsync(It.Is(j => j.SecretId == "RotatingSecret"), It.IsAny())) + .ReturnsAsync(() => + { + int count = Interlocked.Increment(ref describeCallCount); + string versionId = count.ToString().PadLeft(32, '0'); + return new DescribeSecretResponse + { + VersionIdsToStages = new Dictionary> + { + { versionId, new List { "AWSCURRENT" } } + } + }; + }); + + secretsManager.Setup(i => i.GetSecretValueAsync(It.Is(j => j.SecretId == "RotatingSecret"), It.IsAny())) + .ReturnsAsync((GetSecretValueRequest req, CancellationToken _) => + { + Interlocked.Increment(ref getSecretValueCallCount); + return new GetSecretValueResponse + { + Name = "RotatingSecret", + VersionId = req.VersionId, + SecretString = $"Value-{req.VersionId}" + }; + }); + + // Minimal force-refresh delay so RefreshNowAsync completes quickly + var config = new SecretCacheConfiguration + { + MaxCacheSize = 1000, + ForceRefreshDelayBase = TimeSpan.FromMilliseconds(1), + ForceRefreshDelayJitter = TimeSpan.FromMilliseconds(1) + }; + + SecretsManagerCache cache = new SecretsManagerCache(secretsManager.Object, config); + + // Initial fetch — populates the SecretCacheItem with version 1 + string result = await cache.GetSecretString("RotatingSecret"); + Assert.NotNull(result); + + // Get a reference to the same SecretCacheItem for reflection later + SecretCacheItem cacheItem = await cache.GetCachedSecret("RotatingSecret"); + + // Read the MAX_VERSIONS_CACHE_SIZE constant via reflection + int maxVersionsCacheSize = (ushort)typeof(SecretCacheItem) + .GetField("MAX_VERSIONS_CACHE_SIZE", BindingFlags.NonPublic | BindingFlags.Static) + .GetValue(null); + + // Force enough refreshes to exceed capacity, each returning a new version ID + int refreshCount = maxVersionsCacheSize + 5; + for (int i = 0; i < refreshCount; i++) + { + await cache.RefreshNowAsync("RotatingSecret"); + string value = await cache.GetSecretString("RotatingSecret"); + Assert.NotNull(value); + } + + // 1 initial + refreshCount forced refreshes + int expectedCalls = 1 + refreshCount; + Assert.Equal(expectedCalls, describeCallCount); + Assert.Equal(expectedCalls, getSecretValueCallCount); + + // Verify the internal versions cache was trimmed to MAX_VERSIONS_CACHE_SIZE + FieldInfo versionsField = typeof(SecretCacheItem).GetField("versions", BindingFlags.NonPublic | BindingFlags.Instance); + dynamic versionsCache = versionsField.GetValue(cacheItem); + int versionsCacheCount = (int)versionsCache.Count; + Assert.Equal(maxVersionsCacheSize, versionsCacheCount); + } + + [Trait("Category", "Concurrency")] + [Fact] + public async Task ConcurrentGetSecretStringOnlyRefreshesOnce() + { + int describeCallCount = 0; + var barrier = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + Mock secretsManager = new Mock(MockBehavior.Strict); + secretsManager.Setup(i => i.GetSecretValueAsync(It.Is(j => j.SecretId == secretStringResponse1.Name), It.IsAny())) + .ReturnsAsync(secretStringResponse1); + secretsManager.Setup(i => i.DescribeSecretAsync(It.Is(j => j.SecretId == secretStringResponse1.Name), It.IsAny())) + .Returns(async (DescribeSecretRequest _, CancellationToken __) => + { + await barrier.Task; + Interlocked.Increment(ref describeCallCount); + return describeSecretResponse1; + }); + + SecretsManagerCache cache = new SecretsManagerCache(secretsManager.Object); + + // Fire 20 concurrent requests for the same secret + var tasks = Enumerable.Range(0, 20) + .Select(_ => cache.GetSecretString(secretStringResponse1.Name)) + .ToArray(); + + // Release all callers simultaneously to force contention + barrier.SetResult(true); + + string[] results = await Task.WhenAll(tasks); + + // All concurrent callers should receive the correct value + foreach (string result in results) + { + Assert.Equal(secretStringResponse1.SecretString, result); + } + + // Only one DescribeSecret call should have been made despite 20 concurrent requests + Assert.Equal(1, describeCallCount); + } + + [Trait("Category", "Concurrency")] + [Fact] + public async Task ConcurrentGetSecretStringDifferentSecretsSucceeds() + { + var barrier = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + Mock secretsManager = new Mock(MockBehavior.Strict); + secretsManager.Setup(i => i.GetSecretValueAsync(It.Is(j => j.SecretId == secretStringResponse1.Name), It.IsAny())) + .ReturnsAsync(secretStringResponse1); + secretsManager.Setup(i => i.GetSecretValueAsync(It.Is(j => j.SecretId == secretStringResponse3.Name), It.IsAny())) + .ReturnsAsync(secretStringResponse3); + secretsManager.Setup(i => i.DescribeSecretAsync(It.Is(j => j.SecretId == secretStringResponse1.Name), It.IsAny())) + .Returns(async (DescribeSecretRequest _, CancellationToken __) => + { + await barrier.Task; + return describeSecretResponse1; + }); + secretsManager.Setup(i => i.DescribeSecretAsync(It.Is(j => j.SecretId == secretStringResponse3.Name), It.IsAny())) + .Returns(async (DescribeSecretRequest _, CancellationToken __) => + { + await barrier.Task; + return describeSecretResponse1; + }); + + SecretsManagerCache cache = new SecretsManagerCache(secretsManager.Object); + + // Interleave requests for two different secrets concurrently + var tasks = new List>(); + for (int i = 0; i < 10; i++) + { + tasks.Add(cache.GetSecretString(secretStringResponse1.Name)); + tasks.Add(cache.GetSecretString(secretStringResponse3.Name)); + } + + // Release all callers simultaneously to force contention + barrier.SetResult(true); + + string[] results = await Task.WhenAll(tasks); + + // Verify each result matches its corresponding secret (even indices = secret1, odd = secret3) + for (int i = 0; i < results.Length; i++) + { + string expected = i % 2 == 0 ? secretStringResponse1.SecretString : secretStringResponse3.SecretString; + Assert.Equal(expected, results[i]); + } + } + + [Trait("Category", "Concurrency")] + [Fact] + public async Task ConcurrentRefreshNowDoesNotCorruptState() + { + int refreshCount = 0; + var barrier = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + Mock secretsManager = new Mock(MockBehavior.Strict); + secretsManager.Setup(i => i.GetSecretValueAsync(It.Is(j => j.SecretId == secretStringResponse1.Name), It.IsAny())) + .ReturnsAsync(secretStringResponse1); + secretsManager.Setup(i => i.DescribeSecretAsync(It.Is(j => j.SecretId == secretStringResponse1.Name), It.IsAny())) + .Returns(async (DescribeSecretRequest _, CancellationToken __) => + { + return describeSecretResponse1; + }); + + var fastConfig = new SecretCacheConfiguration + { + ForceRefreshDelayBase = TimeSpan.FromMilliseconds(1), + ForceRefreshDelayJitter = TimeSpan.FromMilliseconds(1) + }; + + SecretsManagerCache cache = new SecretsManagerCache(secretsManager.Object, fastConfig); + + // Populate the cache first + await cache.GetSecretString(secretStringResponse1.Name); + + // Set up DescribeSecretAsync with a barrier to have more deterministic interleaving + secretsManager.Setup(i => i.DescribeSecretAsync(It.Is(j => j.SecretId == secretStringResponse1.Name), It.IsAny())) + .Returns(async (DescribeSecretRequest _, CancellationToken __) => + { + await barrier.Task; + Interlocked.Increment(ref refreshCount); + return describeSecretResponse1; + }); + + // Launch 10 concurrent RefreshNowAsync calls to stress internal state transitions + var tasks = Enumerable.Range(0, 10) + .Select(_ => cache.RefreshNowAsync(secretStringResponse1.Name)) + .ToArray(); + + // Release the barrier so they interleave + barrier.SetResult(true); + + bool[] results = await Task.WhenAll(tasks); + + // Every RefreshNowAsync call should have completed successfully + Assert.All(results, success => Assert.True(success)); + + // DescribeSecret should have been called at least once beyond the initial populate + Assert.Equal(10, refreshCount); + + // The cache should remain consistent after concurrent refreshes + string value = await cache.GetSecretString(secretStringResponse1.Name); + Assert.Equal(secretStringResponse1.SecretString, value); + } } -} \ No newline at end of file +}