Skip to content

Commit

Permalink
Increase default MaxRetries for Key Vault client (#592)
Browse files Browse the repository at this point in the history 
* increase default maxretries for key vault client to max value

* update minimum secret refresh interval to 1 minute, enforce for refresh all operations as well

* PR comments

* PR comments

* update max retries default for key vault

* update to use sourceid as key in cached secret dictionary

* move retries number to const int

* call invalidate cache with previous setting if current one is null

* add comment explaining number of key vault retries

* pull out utcnow as a variable in for loop in clearcache
  • Loading branch information
@amerjusupovic
amerjusupovic authored Oct 2, 2024
1 parent 7d89929 commit ca08677
Show file tree
Hide file tree
Showing 11 changed files with 93 additions and 31 deletions.
11 changes: 10 additions & 1 deletion ...ft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationKeyVaultOptions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,17 @@ namespace Microsoft.Extensions.Configuration.AzureAppConfiguration
/// </summary>
public class AzureAppConfigurationKeyVaultOptions
{
// 6 retries is the highest number that will make the total retry time comfortably fall under the default startup timeout of 100 seconds.
// This allows the provider to throw a KeyVaultReferenceException with all relevant information and halt startup instead of timing out.
private const int KeyVaultMaxRetries = 6;

internal TokenCredential Credential;
internal SecretClientOptions ClientOptions = new SecretClientOptions();
internal SecretClientOptions ClientOptions = new SecretClientOptions
{
Retry = {
MaxRetries = KeyVaultMaxRetries
}
};
internal List<SecretClient> SecretClients = new List<SecretClient>();
internal Func<Uri, ValueTask<string>> SecretResolver;
internal Dictionary<string, TimeSpan> SecretRefreshIntervals = new Dictionary<string, TimeSpan>();
Expand Down
10 changes: 9 additions & 1 deletion ...Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,15 @@ await CallWithRequestTracing(
// Invalidate the cached Key Vault secret (if any) for this ConfigurationSetting
foreach (IKeyValueAdapter adapter in _options.Adapters)
{
adapter.InvalidateCache(change.Current);
// If the current setting is null, try to invalidate the previous setting instead
if (change.Current != null)
{
adapter.InvalidateCache(change.Current);
}
else if (change.Previous != null)
{
adapter.InvalidateCache(change.Previous);
}
}
}
}
Expand Down
10 changes: 9 additions & 1 deletion ...onfiguration.AzureAppConfiguration/AzureKeyVaultReference/AzureKeyVaultKeyValueAdapter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,15 @@ public void InvalidateCache(ConfigurationSetting setting = null)
}
else
{
_secretProvider.RemoveSecretFromCache(setting.Key);
if (CanProcess(setting))
{
string secretRefUri = ParseSecretReferenceUri(setting);

if (!string.IsNullOrEmpty(secretRefUri) && Uri.TryCreate(secretRefUri, UriKind.Absolute, out Uri secretUri) && KeyVaultSecretIdentifier.TryCreate(secretUri, out KeyVaultSecretIdentifier secretIdentifier))
{
_secretProvider.RemoveSecretFromCache(secretIdentifier.SourceId);
}
}
}
}

Expand Down
56 changes: 37 additions & 19 deletions ...Configuration.AzureAppConfiguration/AzureKeyVaultReference/AzureKeyVaultSecretProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@ internal class AzureKeyVaultSecretProvider
{
private readonly AzureAppConfigurationKeyVaultOptions _keyVaultOptions;
private readonly IDictionary<string, SecretClient> _secretClients;
private readonly Dictionary<string, CachedKeyVaultSecret> _cachedKeyVaultSecrets;
private string _nextRefreshKey;
private readonly Dictionary<Uri, CachedKeyVaultSecret> _cachedKeyVaultSecrets;
private Uri _nextRefreshSourceId;
private DateTimeOffset? _nextRefreshTime;

public AzureKeyVaultSecretProvider(AzureAppConfigurationKeyVaultOptions keyVaultOptions = null)
{
_keyVaultOptions = keyVaultOptions ?? new AzureAppConfigurationKeyVaultOptions();
_cachedKeyVaultSecrets = new Dictionary<string, CachedKeyVaultSecret>(StringComparer.OrdinalIgnoreCase);
_cachedKeyVaultSecrets = new Dictionary<Uri, CachedKeyVaultSecret>();
_secretClients = new Dictionary<string, SecretClient>(StringComparer.OrdinalIgnoreCase);

if (_keyVaultOptions.SecretClients != null)
Expand All @@ -39,7 +39,7 @@ public async Task<string> GetSecretValue(KeyVaultSecretIdentifier secretIdentifi
{
string secretValue = null;

if (_cachedKeyVaultSecrets.TryGetValue(key, out CachedKeyVaultSecret cachedSecret) &&
if (_cachedKeyVaultSecrets.TryGetValue(secretIdentifier.SourceId, out CachedKeyVaultSecret cachedSecret) &&
(!cachedSecret.RefreshAt.HasValue || DateTimeOffset.UtcNow < cachedSecret.RefreshAt.Value))
{
return cachedSecret.SecretValue;
Expand Down Expand Up @@ -68,12 +68,12 @@ public async Task<string> GetSecretValue(KeyVaultSecretIdentifier secretIdentifi
secretValue = await _keyVaultOptions.SecretResolver(secretIdentifier.SourceId).ConfigureAwait(false);
}

cachedSecret = new CachedKeyVaultSecret(secretValue);
cachedSecret = new CachedKeyVaultSecret(secretValue, secretIdentifier.SourceId);
success = true;
}
finally
{
SetSecretInCache(key, cachedSecret, success);
SetSecretInCache(secretIdentifier.SourceId, key, cachedSecret, success);
}

return secretValue;
Expand All @@ -86,16 +86,34 @@ public bool ShouldRefreshKeyVaultSecrets()

public void ClearCache()
{
_cachedKeyVaultSecrets.Clear();
_nextRefreshKey = null;
_nextRefreshTime = null;
var sourceIdsToRemove = new List<Uri>();

var utcNow = DateTimeOffset.UtcNow;

foreach (KeyValuePair<Uri, CachedKeyVaultSecret> secret in _cachedKeyVaultSecrets)
{
if (secret.Value.LastRefreshTime + RefreshConstants.MinimumSecretRefreshInterval < utcNow)
{
sourceIdsToRemove.Add(secret.Key);
}
}

foreach (Uri sourceId in sourceIdsToRemove)
{
_cachedKeyVaultSecrets.Remove(sourceId);
}

if (_cachedKeyVaultSecrets.Any())
{
UpdateNextRefreshableSecretFromCache();
}
}

public void RemoveSecretFromCache(string key)
public void RemoveSecretFromCache(Uri sourceId)
{
_cachedKeyVaultSecrets.Remove(key);
_cachedKeyVaultSecrets.Remove(sourceId);

if (key == _nextRefreshKey)
if (sourceId == _nextRefreshSourceId)
{
UpdateNextRefreshableSecretFromCache();
}
Expand Down Expand Up @@ -125,39 +143,39 @@ private SecretClient GetSecretClient(Uri secretUri)
return client;
}

private void SetSecretInCache(string key, CachedKeyVaultSecret cachedSecret, bool success = true)
private void SetSecretInCache(Uri sourceId, string key, CachedKeyVaultSecret cachedSecret, bool success = true)
{
if (cachedSecret == null)
{
cachedSecret = new CachedKeyVaultSecret();
}

UpdateCacheExpirationTimeForSecret(key, cachedSecret, success);
_cachedKeyVaultSecrets[key] = cachedSecret;
_cachedKeyVaultSecrets[sourceId] = cachedSecret;

if (key == _nextRefreshKey)
if (sourceId == _nextRefreshSourceId)
{
UpdateNextRefreshableSecretFromCache();
}
else if ((cachedSecret.RefreshAt.HasValue && _nextRefreshTime.HasValue && cachedSecret.RefreshAt.Value < _nextRefreshTime.Value)
|| (cachedSecret.RefreshAt.HasValue && !_nextRefreshTime.HasValue))
{
_nextRefreshKey = key;
_nextRefreshSourceId = sourceId;
_nextRefreshTime = cachedSecret.RefreshAt.Value;
}
}

private void UpdateNextRefreshableSecretFromCache()
{
_nextRefreshKey = null;
_nextRefreshSourceId = null;
_nextRefreshTime = DateTimeOffset.MaxValue;

foreach (KeyValuePair<string, CachedKeyVaultSecret> secret in _cachedKeyVaultSecrets)
foreach (KeyValuePair<Uri, CachedKeyVaultSecret> secret in _cachedKeyVaultSecrets)
{
if (secret.Value.RefreshAt.HasValue && secret.Value.RefreshAt.Value < _nextRefreshTime)
{
_nextRefreshTime = secret.Value.RefreshAt;
_nextRefreshKey = secret.Key;
_nextRefreshSourceId = secret.Key;
}
}

Expand Down
14 changes: 13 additions & 1 deletion ...nsions.Configuration.AzureAppConfiguration/AzureKeyVaultReference/CachedKeyVaultSecret.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,23 @@ internal class CachedKeyVaultSecret
/// </summary>
public int RefreshAttempts { get; set; }

public CachedKeyVaultSecret(string secretValue = null, DateTimeOffset? refreshAt = null, int refreshAttempts = 0)
/// <summary>
/// The last time this secret was reloaded from Key Vault.
/// </summary>
public DateTimeOffset LastRefreshTime { get; set; }

/// <summary>
/// The source <see cref="Uri"/> for this secret.
/// </summary>
public Uri SourceId { get; }

public CachedKeyVaultSecret(string secretValue = null, Uri sourceId = null, DateTimeOffset? refreshAt = null, int refreshAttempts = 0)
{
SecretValue = secretValue;
RefreshAt = refreshAt;
LastRefreshTime = DateTimeOffset.UtcNow;
RefreshAttempts = refreshAttempts;
SourceId = sourceId;
}
}
}
2 changes: 1 addition & 1 deletion src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Constants/RefreshConstants.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ internal class RefreshConstants
public static readonly TimeSpan MinimumFeatureFlagsCacheExpirationInterval = TimeSpan.FromSeconds(1);

// Key Vault secrets
public static readonly TimeSpan MinimumSecretRefreshInterval = TimeSpan.FromSeconds(1);
public static readonly TimeSpan MinimumSecretRefreshInterval = TimeSpan.FromMinutes(1);

// Backoff during refresh failures
public static readonly TimeSpan DefaultMinBackoff = TimeSpan.FromSeconds(30);
Expand Down
5 changes: 5 additions & 0 deletions ...xtensions.Configuration.AzureAppConfiguration/Extensions/ConfigurationClientExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public static async Task<KeyValueChange> GetKeyValueChange(this ConfigurationCli
return new KeyValueChange
{
ChangeType = KeyValueChangeType.Modified,
Previous = setting,
Current = response.Value,
Key = setting.Key,
Label = setting.Label
Expand All @@ -47,6 +48,7 @@ public static async Task<KeyValueChange> GetKeyValueChange(this ConfigurationCli
return new KeyValueChange
{
ChangeType = KeyValueChangeType.Deleted,
Previous = setting,
Current = null,
Key = setting.Key,
Label = setting.Label
Expand All @@ -56,6 +58,7 @@ public static async Task<KeyValueChange> GetKeyValueChange(this ConfigurationCli
return new KeyValueChange
{
ChangeType = KeyValueChangeType.None,
Previous = setting,
Current = setting,
Key = setting.Key,
Label = setting.Label
Expand Down Expand Up @@ -158,6 +161,7 @@ await TracingUtils.CallWithRequestTracing(options.RequestTracingEnabled, Request
ChangeType = KeyValueChangeType.Modified,
Key = setting.Key,
Label = options.Label.NormalizeNull(),
Previous = null,
Current = setting
});
string key = setting.Key.Substring(FeatureManagementConstants.FeatureFlagMarker.Length);
Expand All @@ -176,6 +180,7 @@ await TracingUtils.CallWithRequestTracing(options.RequestTracingEnabled, Request
ChangeType = KeyValueChangeType.Deleted,
Key = kvp.Key,
Label = options.Label.NormalizeNull(),
Previous = null,
Current = null
});
string key = kvp.Key.Substring(FeatureManagementConstants.FeatureFlagMarker.Length);
Expand Down
2 changes: 2 additions & 0 deletions src/Microsoft.Extensions.Configuration.AzureAppConfiguration/KeyValueChange.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,7 @@ internal struct KeyValueChange
public string Label { get; set; }

public ConfigurationSetting Current { get; set; }

public ConfigurationSetting Previous { get; set; }
}
}
8 changes: 4 additions & 4 deletions tests/Tests.AzureAppConfiguration/KeyVaultReferenceTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@ Response<ConfigurationSetting> GetIfChanged(ConfigurationSetting setting, bool o
public async Task CachedSecretIsInvalidatedWhenRefreshAllIsTrue()
{
IConfigurationRefresher refresher = null;
TimeSpan cacheExpirationTime = TimeSpan.FromSeconds(1);
TimeSpan cacheExpirationTime = TimeSpan.FromSeconds(60);

var mockResponse = new Mock<Response>();
var mockClient = new Mock<ConfigurationClient>(MockBehavior.Strict);
Expand Down Expand Up @@ -830,7 +830,7 @@ Response<ConfigurationSetting> GetIfChanged(ConfigurationSetting setting, bool o
public async Task SecretIsReloadedFromKeyVaultWhenCacheExpires()
{
IConfigurationRefresher refresher = null;
TimeSpan cacheExpirationTime = TimeSpan.FromSeconds(1);
TimeSpan cacheExpirationTime = TimeSpan.FromSeconds(60);

var mockResponse = new Mock<Response>();
var mockClient = new Mock<ConfigurationClient>(MockBehavior.Strict);
Expand Down Expand Up @@ -873,7 +873,7 @@ public async Task SecretIsReloadedFromKeyVaultWhenCacheExpires()
public async Task SecretsWithDefaultRefreshInterval()
{
IConfigurationRefresher refresher = null;
TimeSpan shortCacheExpirationTime = TimeSpan.FromSeconds(1);
TimeSpan shortCacheExpirationTime = TimeSpan.FromSeconds(60);

var mockResponse = new Mock<Response>();
var mockClient = new Mock<ConfigurationClient>(MockBehavior.Strict);
Expand Down Expand Up @@ -918,7 +918,7 @@ public async Task SecretsWithDefaultRefreshInterval()
public async Task SecretsWithDifferentRefreshIntervals()
{
IConfigurationRefresher refresher = null;
TimeSpan shortCacheExpirationTime = TimeSpan.FromSeconds(1);
TimeSpan shortCacheExpirationTime = TimeSpan.FromSeconds(60);
TimeSpan longCacheExpirationTime = TimeSpan.FromDays(1);

var mockResponse = new Mock<Response>();
Expand Down
2 changes: 1 addition & 1 deletion tests/Tests.AzureAppConfiguration/LoggingTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,7 @@ public async Task ValidateCorrectKeyVaultSecretLoggedDuringRefresh()
refreshOptions.Register("TestKey1", "label", true)
.SetCacheExpiration(CacheExpirationTime);
});
options.ConfigureKeyVault(kv => kv.Register(mockSecretClient.Object).SetSecretRefreshInterval(CacheExpirationTime));
options.ConfigureKeyVault(kv => kv.Register(mockSecretClient.Object).SetSecretRefreshInterval(TimeSpan.FromSeconds(60)));
refresher = options.GetRefresher();
})
.Build();
Expand Down
4 changes: 2 additions & 2 deletions tests/Tests.AzureAppConfiguration/MapTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ public void MapResolveKeyVaultReferenceThrowsExceptionInAdapter()
.AddAzureAppConfiguration(options =>
{
options.ClientManager = mockClientManager;
options.ConfigureKeyVault(kv => kv.Register(mockSecretClient.Object).SetSecretRefreshInterval(TimeSpan.FromSeconds(1)));
options.ConfigureKeyVault(kv => kv.Register(mockSecretClient.Object).SetSecretRefreshInterval(TimeSpan.FromSeconds(60)));
options.Map((setting) =>
{
if (setting.ContentType == KeyVaultConstants.ContentType + "; charset=utf-8")
Expand Down Expand Up @@ -446,7 +446,7 @@ public void MapAsyncResolveKeyVaultReference()
.AddAzureAppConfiguration(options =>
{
options.ClientManager = mockClientManager;
options.ConfigureKeyVault(kv => kv.Register(mockSecretClient.Object).SetSecretRefreshInterval(TimeSpan.FromSeconds(1)));
options.ConfigureKeyVault(kv => kv.Register(mockSecretClient.Object).SetSecretRefreshInterval(TimeSpan.FromSeconds(60)));
options.Map(async (setting) =>
{
if (setting.ContentType == KeyVaultConstants.ContentType + "; charset=utf-8")
Expand Down

0 comments on commit ca08677

Please sign in to comment.