Secret refresh support#20
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for refreshing Key Vault secrets alongside key-value settings in Azure App Configuration. Key changes include:
- Validation for the secret refresh interval against a minimal allowed duration.
- New refresh logic and timer for Key Vault secrets with corresponding changes in the loading and testing of secrets.
- Updates in tests and options to support the new secret refresh functionality.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| azureappconfiguration/utils.go | Added validation for secret refresh interval |
| azureappconfiguration/refresh_test.go | Updated mocks and assertions to test secret refresh |
| azureappconfiguration/options.go | Introduced RefreshOptions in KeyVaultOptions |
| azureappconfiguration/constants.go | Defined minimalSecretRefreshInterval constant |
| azureappconfiguration/azureappconfiguration_test.go | Adjusted secret assertions in tests |
| azureappconfiguration/azureappconfiguration.go | Added secrets map, secret refresh timer, and refresh logic for secrets |
Comments suppressed due to low confidence (2)
azureappconfiguration/azureappconfiguration.go:369
- When resolving Key Vault secrets concurrently, a failure in any goroutine returns a partially filled secrets map along with an error. Consider aborting immediately upon error to avoid propagating incomplete secret data.
if err := eg.Wait(); err != nil { return secrets, fmt.Errorf("failed to resolve Key Vault references: %w", err) }
azureappconfiguration/azureappconfiguration.go:197
- [nitpick] Consider adding a clarifying comment that the refresh configuration is valid if either the KeyValue or Secret refresh timer is set, as this may not be immediately obvious to readers.
if azappcfg.kvRefreshTimer == nil && azappcfg.secretRefreshTimer == nil {
linglingye001
force-pushed
the
linglingye/sentinelBasedRefresh
branch
from
b6bf5ec to
c733192
Compare
linglingye001
force-pushed
the
linglingye/secretRefresh
branch
from
d0f4481 to
0cac003
Compare
| if azappcfg.kvRefreshTimer == nil { | ||
| return fmt.Errorf("refresh is not configured") | ||
| if azappcfg.kvRefreshTimer == nil && azappcfg.secretRefreshTimer == nil { | ||
| return fmt.Errorf("refresh is not enabled for key values or key vault data") |
There was a problem hiding this comment.
refresh is not enabled for key values or key vault references
There was a problem hiding this comment.
Just want to make the log and error message more clear, for secret refresh scenario, the key vault references don't change and the provider actually refresh the secret value, what do you think replacing key vault references with Key Vault secret in secret refresh scenarios?
There was a problem hiding this comment.
"key vault reference" is an official term in azure app config document. "key vault data" sounds odd since we never call things with it.
I agree with you we need make the message more clear, we are not really refreshing the key vault references, how about this?
Neither key values refresh nor key vault reference reloading is enabled.
We use the word "reload" for this scenario, https://learn.microsoft.com/en-us/azure/azure-app-configuration/reload-key-vault-secrets-dotnet
There was a problem hiding this comment.
"key vault data" is used in k8s provider reference doc. 'reload' looks good to me.
No description provided.