-
Notifications
You must be signed in to change notification settings - Fork 9
Optimization for concurrent Key Vault Ref resolution - deduplicate requests to the same secret #328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,11 @@ export interface IKeyValueAdapter { | |
| */ | ||
| processKeyValue(setting: ConfigurationSetting): Promise<[string, unknown]>; | ||
|
|
||
| /** | ||
| * This method deduplicates and warms up Key Vault secret requests so that processKeyValue only reads from cache. | ||
| */ | ||
| preload?(settings: ConfigurationSetting[]): Promise<void>; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method should not be optional. We should make it and its comment consistent with
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the comment we should explain the parameter: whether it should be just a bunch of configuration settings or a set of certain type of configuration settings that the adapter can process
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually prefer to let preload only take settings that the adapter can process. This will match the orchestrator(appConfigurationImpl) + kv adapter design. The orchestrator should call preload like this: for (const adapter of this.#adapters) {
await adapter.preload(settings.filter(s => adapter.canProcess(s)));
}The above code has very clear intention.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With that concern, I think maybe we should make |
||
|
|
||
| /** | ||
| * This method is called when a change is detected in the configuration setting. | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -49,6 +49,27 @@ export class AzureKeyVaultKeyValueAdapter implements IKeyValueAdapter { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| async preload(settings: ConfigurationSetting[]): Promise<void> { | ||||||
| if (!this.#keyVaultOptions) { | ||||||
| return; // nothing to do; processKeyValue will throw the proper ArgumentError | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| } | ||||||
| const uniqueSecretIdentifiers = new Map<string, KeyVaultSecretIdentifier>(); | ||||||
| for (const setting of settings) { | ||||||
| if (!this.canProcess(setting)) { | ||||||
| continue; | ||||||
| } | ||||||
| try { | ||||||
| const secretIdentifier = parseKeyVaultSecretIdentifier( | ||||||
| parseSecretReference(setting).value.secretId | ||||||
| ); | ||||||
| uniqueSecretIdentifiers.set(secretIdentifier.sourceId, secretIdentifier); // dedup by sourceId | ||||||
| } catch { | ||||||
| // Skip invalid references; processKeyValue re-parses and raises KeyVaultReferenceError with context. | ||||||
| } | ||||||
| } | ||||||
| await this.#keyVaultSecretProvider.preloadSecrets([...uniqueSecretIdentifiers.values()]); | ||||||
| } | ||||||
|
|
||||||
| async onChangeDetected(): Promise<void> { | ||||||
| this.#keyVaultSecretProvider.clearCache(); | ||||||
| return; | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,16 +33,41 @@ export class AzureKeyVaultSecretProvider { | |
| } | ||
| } | ||
|
|
||
| async getSecretValue(secretIdentifier: KeyVaultSecretIdentifier): Promise<unknown> { | ||
| // Fetches the given unique secrets to warm the cache. | ||
| async preloadSecrets(secretIdentifiers: KeyVaultSecretIdentifier[]): Promise<void> { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. preload is the concept from the abstraction of kv adapter. This business logic should be placed in adapter implementation. I think secret provider should offer two methods: 1. loadSecretValue 2. getSecretValue
|
||
| const loadSecret = async (secretIdentifier: KeyVaultSecretIdentifier) => { | ||
| try { | ||
| await this.#loadSecretValue(secretIdentifier); | ||
| } catch { | ||
| // Leave uncached; getSecretValue re-fetches and surfaces the error during resolution. | ||
| } | ||
| }; | ||
|
|
||
| if (this.#keyVaultOptions?.parallelSecretResolutionEnabled) { | ||
| await Promise.all(secretIdentifiers.map(loadSecret)); | ||
| } else { | ||
| for (const secretIdentifier of secretIdentifiers) { | ||
| await loadSecret(secretIdentifier); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| async #loadSecretValue(secretIdentifier: KeyVaultSecretIdentifier): Promise<void> { | ||
| const identifierKey = secretIdentifier.sourceId; | ||
| const shouldRefresh = this.#secretRefreshTimer?.canRefresh() ?? false; | ||
| if (this.#cachedSecretValues.has(identifierKey) && !shouldRefresh) { | ||
| return; // already cached and still fresh | ||
| } | ||
| this.#cachedSecretValues.set(identifierKey, await this.#getSecretValueFromKeyVault(secretIdentifier)); | ||
| } | ||
|
|
||
| // If the refresh interval is not expired, return the cached value if available. | ||
| if (this.#cachedSecretValues.has(identifierKey) && | ||
| (!this.#secretRefreshTimer || !this.#secretRefreshTimer.canRefresh())) { | ||
| return this.#cachedSecretValues.get(identifierKey); | ||
| async getSecretValue(secretIdentifier: KeyVaultSecretIdentifier): Promise<unknown> { | ||
| const identifierKey = secretIdentifier.sourceId; | ||
| if (this.#cachedSecretValues.has(identifierKey)) { | ||
| return this.#cachedSecretValues.get(identifierKey); | ||
| } | ||
|
|
||
| // Fallback to fetching the secret value from Key Vault. | ||
| // Fallback for secrets that preload skipped or failed to fetch. | ||
| const secretValue = await this.#getSecretValueFromKeyVault(secretIdentifier); | ||
| this.#cachedSecretValues.set(identifierKey, secretValue); | ||
| return secretValue; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should call preload hook in
loadSelectedKeyValuelike this: