Skip to content

Resolve key vault references concurrently#736

Open
linglingye001 wants to merge 12 commits into
mainfrom
linglingye/resolve-secret-parallel
Open

Resolve key vault references concurrently#736
linglingye001 wants to merge 12 commits into
mainfrom
linglingye/resolve-secret-parallel

Conversation

@linglingye001

@linglingye001 linglingye001 commented May 20, 2026

Copy link
Copy Markdown
Member

This PR introduces an opt-in capability to resolve Azure Key Vault references concurrently during Azure App Configuration load, aiming to reduce startup time when many Key Vault references are present. #735

Changes:

  • Add ParallelSecretResolutionEnabled option under Key Vault configuration and plumb it into provider options.
  • Update configuration loading to optionally process adapter resolution concurrently.
  • Add unit tests covering parallel resolution behavior and default sequential behavior

@linglingye001
linglingye001 requested a review from Copilot May 20, 2026 07:47
@linglingye001
linglingye001 force-pushed the linglingye/resolve-secret-parallel branch from 5c8223e to 61295ef Compare May 20, 2026 07:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces an opt-in capability to resolve Azure Key Vault references concurrently during Azure App Configuration load, aiming to reduce startup time when many Key Vault references are present.

Changes:

  • Add ParallelSecretResolutionEnabled option under Key Vault configuration and plumb it into provider options.
  • Update configuration loading to optionally process adapter resolution concurrently and merge results deterministically.
  • Add unit tests covering parallel resolution behavior and default sequential behavior; add locking in the Key Vault secret provider to support concurrent access.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/Tests.AzureAppConfiguration/Unit/KeyVaultReferenceTests.cs Adds tests validating parallel Key Vault resolution and default sequential behavior.
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureKeyVaultReference/AzureKeyVaultSecretProvider.cs Adds synchronization around secret caching and refresh bookkeeping for thread safety under concurrency.
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs Adds parallel adapter processing path and factors merge logic into a helper.
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationOptions.cs Stores whether parallel secret resolution is enabled after Key Vault configuration.
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationKeyVaultOptions.cs Introduces the public ParallelSecretResolutionEnabled toggle with documentation.
Comments suppressed due to low confidence (1)

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs:654

  • In parallel mode, all adapter tasks are dispatched before any failures are observed (via Task.WhenAll). If a single Key Vault reference is invalid/unavailable, the load will still issue requests for the remaining references, increasing latency and side effects (extra Key Vault traffic) even though the overall load fails. Consider failing fast by cancelling remaining work when the first task faults (e.g., linked CancellationTokenSource + cancel on first exception, or processing in bounded batches).
                // Dispatch adapter processing for all settings concurrently. Only Key Vault references
                // perform network I/O during adapter processing; other adapters complete synchronously.
                // Insertion order in 'data' is preserved when merging results so prefix-stripping and
                // last-write-wins behavior remain unchanged.
                var pendingTasks = new List<Task<IEnumerable<KeyValuePair<string, string>>>>(data.Count);

                foreach (KeyValuePair<string, ConfigurationSetting> kvp in data)
                {
                    if (_requestTracingEnabled && _requestTracingOptions != null)
                    {
                        _requestTracingOptions.UpdateAiConfigurationTracing(kvp.Value.ContentType);
                    }

                    pendingTasks.Add(ProcessAdapters(kvp.Value, cancellationToken));
                }

                IEnumerable<KeyValuePair<string, string>>[] results = await Task.WhenAll(pendingTasks).ConfigureAwait(false);

                for (int i = 0; i < results.Length; i++)
                {
                    MergeIntoApplicationData(applicationData, results[i]);
                }
            }

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

@avanigupta

Copy link
Copy Markdown
Member

I'm thinking of a different approach for this PR. What if we added a new method to the IKeyValueAdapter interface which pre-loads data before ProcessKeyValue is called. The pre-loader would be responsible for parallelization of KeyVault calls, and it would update the secret cache so that by the time ProcessKeyValue is invoked, it just reads from cache (no I/O in ProcessKeyValue). This would preserve the precedence order for config composition. Other adapters dont have anything to preload, so they are no-op.

Task PreloadAsync(IEnumerable<ConfigurationSetting> settings, Logger logger, CancellationToken cancellationToken);

Semantics: "Given all the settings about to be processed, pre-warm your caches so that ProcessKeyValue can return instantly."

Comment thread src/Microsoft.Extensions.Configuration.AzureAppConfiguration/IKeyValueAdapter.cs Outdated
Comment on lines +160 to +192
using (var semaphore = new SemaphoreSlim(KeyVaultConstants.MaxParallelSecretResolution))
{
var tasks = new Task[toFetch.Count];

for (int i = 0; i < toFetch.Count; i++)
{
(KeyVaultSecretIdentifier identifier, ConfigurationSetting setting, string secretRefUri) = toFetch[i];
tasks[i] = PreloadSecretAsync(identifier, setting, secretRefUri, semaphore, logger, cancellationToken);
}

await Task.WhenAll(tasks).ConfigureAwait(false);
}
}
else
{
foreach ((KeyVaultSecretIdentifier identifier, ConfigurationSetting setting, string secretRefUri) in toFetch)
{
await PreloadSecretAsync(identifier, setting, secretRefUri, semaphore: null, logger, cancellationToken).ConfigureAwait(false);
}
}
}

private async Task PreloadSecretAsync(KeyVaultSecretIdentifier identifier, ConfigurationSetting setting, string secretRefUri, SemaphoreSlim semaphore, Logger logger, CancellationToken cancellationToken)
{
if (semaphore != null)
{
await semaphore.WaitAsync(cancellationToken).ConfigureAwait(false);
}

try
{
await _secretProvider.GetSecretValue(identifier, setting.Key, setting.Label, logger, cancellationToken).ConfigureAwait(false);
}

@zhiyuanliang-ms zhiyuanliang-ms Jul 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation will still create a Task instance for each secret references. We can have a fixed size task array and use worker pattern to resolve secret in parallel.

int workerCount = Math.Min(KeyVaultConstants.MaxParallelSecretResolution, toFetch.Count);

int nextIndex = -1;
Task[] workers = new Task[workerCount];

for (int i = 0; i < workerCount; i++)
{
    workers[i] = ((Func<Task>)(async () =>
    {
        while (true)
        {
            int index = Interlocked.Increment(ref nextIndex);

            if (index >= toFetch.Count)
            {
                return;
            }

            var secretToFetch = toFetch[index];

            await load(secretToFetch);
        }
    }))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The worker pool would be the better if there're thousands of references per load. While in most scenarios KV refs < 100,

  • The workload (N = number of KV references) is small; the allocation savings of the worker pool is not obvious.
  • The semaphore version is more readable and each item maps cleanly to one task, which makes the KeyVaultReferenceException propagation (already carefully wired to carry Key/Label/SecretIdentifier) straightforward.
  • The worker pool needs extra hand-written machinery (exception capture so a dying worker doesn't silently drop items), more surface for subtle bugs.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline, use semaphoreSlim pattern for code readability and simplicity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants