From 7e5f7ab9f821293fec9e5f4c396e4a273f47a29f Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Fri, 28 Mar 2025 15:52:22 +0800 Subject: [PATCH 01/16] resolve conflict --- azureappconfiguration/options.go | 4 +-- internal/refreshtimer/refresh_timer.go | 46 ++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) create mode 100644 internal/refreshtimer/refresh_timer.go diff --git a/azureappconfiguration/options.go b/azureappconfiguration/options.go index a8e0d81..84c4a44 100644 --- a/azureappconfiguration/options.go +++ b/azureappconfiguration/options.go @@ -22,9 +22,9 @@ type Options struct { // Selectors defines what key-values to load from Azure App Configuration // Each selector combines a key filter and label filter // If selectors are not provided, all key-values with no label are loaded by default. - Selectors []Selector + Selectors []Selector // RefreshOptions contains optional parameters to configure the behavior of key-value settings refresh - RefreshOptions KeyValueRefreshOptions + RefreshOptions KeyValueRefreshOptions // KeyVaultOptions configures how Key Vault references are resolved. KeyVaultOptions KeyVaultOptions diff --git a/internal/refreshtimer/refresh_timer.go b/internal/refreshtimer/refresh_timer.go new file mode 100644 index 0000000..15847a0 --- /dev/null +++ b/internal/refreshtimer/refresh_timer.go @@ -0,0 +1,46 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package refreshtimer + +import "time" + +// RefreshTimer manages the timing for refresh operations +type RefreshTimer struct { + interval time.Duration // How often refreshes should occur + nextRefreshTime time.Time // When the next refresh should occur +} + +// RefreshCondition interface defines the methods a refresh timer should implement +type RefreshCondition interface { + ShouldRefresh() bool + Reset() +} + +const ( + DefaultRefreshInterval time.Duration = 30 * time.Second +) + +// New creates a new refresh timer with the specified interval +// If interval is zero or negative, it falls back to the DefaultRefreshInterval +func New(interval time.Duration) *RefreshTimer { + // Use default interval if not specified or invalid + if interval <= 0 { + interval = DefaultRefreshInterval + } + + return &RefreshTimer{ + interval: interval, + nextRefreshTime: time.Now().Add(interval), + } +} + +// ShouldRefresh checks whether it's time for a refresh +func (rt *RefreshTimer) ShouldRefresh() bool { + return !time.Now().Before(rt.nextRefreshTime) +} + +// Reset resets the timer for the next refresh cycle +func (rt *RefreshTimer) Reset() { + rt.nextRefreshTime = time.Now().Add(rt.interval) +} From 0d8d251741615ac4e770a759b28ec0bc23775c40 Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Fri, 28 Mar 2025 15:52:50 +0800 Subject: [PATCH 02/16] add tests --- refresh_test.go | 264 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 264 insertions(+) create mode 100644 refresh_test.go diff --git a/refresh_test.go b/refresh_test.go new file mode 100644 index 0000000..a5b1b51 --- /dev/null +++ b/refresh_test.go @@ -0,0 +1,264 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package azureappconfiguration + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/refreshtimer" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// mockETagsClient implements the eTagsClient interface for testing +type mockETagsClient struct { + changed bool + checkCallCount int + err error +} + +func (m *mockETagsClient) checkIfETagChanged(ctx context.Context) (bool, error) { + m.checkCallCount++ + if m.err != nil { + return false, m.err + } + return m.changed, nil +} + +// mockRefreshCondition implements the refreshtimer.RefreshCondition interface for testing +type mockRefreshCondition struct { + shouldRefresh bool + resetCalled bool +} + +func (m *mockRefreshCondition) ShouldRefresh() bool { + return m.shouldRefresh +} + +func (m *mockRefreshCondition) Reset() { + m.resetCalled = true +} + +func TestRefresh_NotConfigured(t *testing.T) { + // Setup a provider with no refresh configuration + azappcfg := &AzureAppConfiguration{} + + // Attempt to refresh + err := azappcfg.Refresh(context.Background()) + + // Verify that an error is returned + require.Error(t, err) + assert.Contains(t, err.Error(), "refresh is not configured") +} + +func TestRefresh_AlreadyInProgress(t *testing.T) { + // Setup a provider with refresh already in progress + azappcfg := &AzureAppConfiguration{ + kvRefreshTimer: &mockRefreshCondition{}, + refreshInProgress: true, + } + + // Attempt to refresh + err := azappcfg.Refresh(context.Background()) + + // Verify no error and that we returned early + assert.NoError(t, err) +} + +func TestRefresh_NotTimeToRefresh(t *testing.T) { + // Setup a provider with a timer that indicates it's not time to refresh + mockTimer := &mockRefreshCondition{shouldRefresh: false} + azappcfg := &AzureAppConfiguration{ + kvRefreshTimer: mockTimer, + } + + // Attempt to refresh + err := azappcfg.Refresh(context.Background()) + + // Verify no error and that we returned early + assert.NoError(t, err) + // Timer should not be reset if we're not refreshing + assert.False(t, mockTimer.resetCalled) +} + +func TestRefresh_NoChanges(t *testing.T) { + // Setup mock clients + mockTimer := &mockRefreshCondition{shouldRefresh: true} + mockEtags := &mockETagsClient{changed: false} + + // Setup a provider + azappcfg := &AzureAppConfiguration{ + kvRefreshTimer: mockTimer, + watchedSettingsMonitor: mockEtags, + } + + // Attempt to refresh + err := azappcfg.Refresh(context.Background()) + + // Verify no error and that refresh was attempted but no changes were detected + assert.NoError(t, err) + assert.Equal(t, 1, mockEtags.checkCallCount) + assert.True(t, mockTimer.resetCalled, "Timer should be reset even when no changes detected") +} + +func TestRefreshEnabled_EmptyWatchedSettings(t *testing.T) { + // Test verifying validation when refresh is enabled but no watched settings + options := &Options{ + RefreshOptions: KeyValueRefreshOptions{ + Enabled: true, // Enabled but without watched settings + WatchedSettings: []WatchedSetting{}, + }, + } + + // Verify error + err := verifyOptions(options) + require.Error(t, err) + assert.Contains(t, err.Error(), "watched settings cannot be empty") +} + +func TestRefreshEnabled_IntervalTooShort(t *testing.T) { + // Test verifying validation when refresh interval is too short + options := &Options{ + RefreshOptions: KeyValueRefreshOptions{ + Enabled: true, + Interval: 500 * time.Millisecond, // Too short, should be at least minimalRefreshInterval + WatchedSettings: []WatchedSetting{ + {Key: "test-key", Label: "test-label"}, + }, + }, + } + + // Verify error + err := verifyOptions(options) + require.Error(t, err) + assert.Contains(t, err.Error(), "key value refresh interval cannot be less than") +} + +func TestRefreshEnabled_EmptyWatchedSettingKey(t *testing.T) { + // Test verifying validation when a watched setting has an empty key + options := &Options{ + RefreshOptions: KeyValueRefreshOptions{ + Enabled: true, + WatchedSettings: []WatchedSetting{ + {Key: "", Label: "test-label"}, // Empty key should be rejected + }, + }, + } + + // Verify error + err := verifyOptions(options) + require.Error(t, err) + assert.Contains(t, err.Error(), "watched setting key cannot be empty") +} + +func TestRefreshEnabled_InvalidWatchedSettingKey(t *testing.T) { + // Test verifying validation when watched setting keys contain invalid chars + options := &Options{ + RefreshOptions: KeyValueRefreshOptions{ + Enabled: true, + WatchedSettings: []WatchedSetting{ + {Key: "test*key", Label: "test-label"}, // Key contains wildcard, not allowed + }, + }, + } + + // Verify error + err := verifyOptions(options) + require.Error(t, err) + assert.Contains(t, err.Error(), "watched setting key cannot contain") +} + +func TestRefreshEnabled_InvalidWatchedSettingLabel(t *testing.T) { + // Test verifying validation when watched setting labels contain invalid chars + options := &Options{ + RefreshOptions: KeyValueRefreshOptions{ + Enabled: true, + WatchedSettings: []WatchedSetting{ + {Key: "test-key", Label: "test*label"}, // Label contains wildcard, not allowed + }, + }, + } + + // Verify error + err := verifyOptions(options) + require.Error(t, err) + assert.Contains(t, err.Error(), "watched setting label cannot contain") +} + +func TestRefreshEnabled_ValidSettings(t *testing.T) { + // Test verifying valid refresh options pass validation + options := &Options{ + RefreshOptions: KeyValueRefreshOptions{ + Enabled: true, + Interval: 5 * time.Second, // Valid interval + WatchedSettings: []WatchedSetting{ + {Key: "test-key-1", Label: "test-label-1"}, + {Key: "test-key-2", Label: ""}, // Empty label should be normalized later + }, + }, + } + + // Verify no error + err := verifyOptions(options) + assert.NoError(t, err) +} + +func TestNormalizedWatchedSettings(t *testing.T) { + // Test the normalizedWatchedSettings function + settings := []WatchedSetting{ + {Key: "key1", Label: "label1"}, + {Key: "key2", Label: ""}, // Empty label should be set to defaultLabel + } + + normalized := normalizedWatchedSettings(settings) + + // Verify results + assert.Len(t, normalized, 2) + assert.Equal(t, "key1", normalized[0].Key) + assert.Equal(t, "label1", normalized[0].Label) + assert.Equal(t, "key2", normalized[1].Key) + assert.Equal(t, defaultLabel, normalized[1].Label) +} + +func TestRefresh_ErrorDuringETagCheck(t *testing.T) { + // Setup mocks + mockTimer := &mockRefreshCondition{shouldRefresh: true} + mockEtags := &mockETagsClient{ + err: fmt.Errorf("etag check failed"), + } + + // Setup provider + azappcfg := &AzureAppConfiguration{ + kvRefreshTimer: mockTimer, + watchedSettingsMonitor: mockEtags, + } + + // Attempt to refresh + err := azappcfg.Refresh(context.Background()) + + // Verify error and that timer was not reset + assert.Error(t, err) + assert.Contains(t, err.Error(), "etag check failed") + assert.False(t, mockTimer.resetCalled, "Timer should not be reset on error") +} + +// Additional test to verify real RefreshTimer behavior +func TestRealRefreshTimer(t *testing.T) { + // Create a real refresh timer with a short interval + timer := refreshtimer.New(100 * time.Millisecond) + + // Initially it should not be time to refresh + assert.False(t, timer.ShouldRefresh(), "New timer should not immediately indicate refresh needed") + + // After the interval passes, it should indicate time to refresh + time.Sleep(110 * time.Millisecond) + assert.True(t, timer.ShouldRefresh(), "Timer should indicate refresh needed after interval") + + // After reset, it should not be time to refresh again + timer.Reset() + assert.False(t, timer.ShouldRefresh(), "Timer should not indicate refresh needed right after reset") +} From 8b385cb76a2ac61b899d64e0d0615ade7d54d139 Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Fri, 11 Apr 2025 14:51:52 +0800 Subject: [PATCH 03/16] rebase --- .../internal}/refreshtimer/refresh_timer.go | 0 refresh_test.go | 264 ------------------ 2 files changed, 264 deletions(-) rename {internal => azureappconfiguration/internal}/refreshtimer/refresh_timer.go (100%) delete mode 100644 refresh_test.go diff --git a/internal/refreshtimer/refresh_timer.go b/azureappconfiguration/internal/refreshtimer/refresh_timer.go similarity index 100% rename from internal/refreshtimer/refresh_timer.go rename to azureappconfiguration/internal/refreshtimer/refresh_timer.go diff --git a/refresh_test.go b/refresh_test.go deleted file mode 100644 index a5b1b51..0000000 --- a/refresh_test.go +++ /dev/null @@ -1,264 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -package azureappconfiguration - -import ( - "context" - "fmt" - "testing" - "time" - - "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/refreshtimer" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// mockETagsClient implements the eTagsClient interface for testing -type mockETagsClient struct { - changed bool - checkCallCount int - err error -} - -func (m *mockETagsClient) checkIfETagChanged(ctx context.Context) (bool, error) { - m.checkCallCount++ - if m.err != nil { - return false, m.err - } - return m.changed, nil -} - -// mockRefreshCondition implements the refreshtimer.RefreshCondition interface for testing -type mockRefreshCondition struct { - shouldRefresh bool - resetCalled bool -} - -func (m *mockRefreshCondition) ShouldRefresh() bool { - return m.shouldRefresh -} - -func (m *mockRefreshCondition) Reset() { - m.resetCalled = true -} - -func TestRefresh_NotConfigured(t *testing.T) { - // Setup a provider with no refresh configuration - azappcfg := &AzureAppConfiguration{} - - // Attempt to refresh - err := azappcfg.Refresh(context.Background()) - - // Verify that an error is returned - require.Error(t, err) - assert.Contains(t, err.Error(), "refresh is not configured") -} - -func TestRefresh_AlreadyInProgress(t *testing.T) { - // Setup a provider with refresh already in progress - azappcfg := &AzureAppConfiguration{ - kvRefreshTimer: &mockRefreshCondition{}, - refreshInProgress: true, - } - - // Attempt to refresh - err := azappcfg.Refresh(context.Background()) - - // Verify no error and that we returned early - assert.NoError(t, err) -} - -func TestRefresh_NotTimeToRefresh(t *testing.T) { - // Setup a provider with a timer that indicates it's not time to refresh - mockTimer := &mockRefreshCondition{shouldRefresh: false} - azappcfg := &AzureAppConfiguration{ - kvRefreshTimer: mockTimer, - } - - // Attempt to refresh - err := azappcfg.Refresh(context.Background()) - - // Verify no error and that we returned early - assert.NoError(t, err) - // Timer should not be reset if we're not refreshing - assert.False(t, mockTimer.resetCalled) -} - -func TestRefresh_NoChanges(t *testing.T) { - // Setup mock clients - mockTimer := &mockRefreshCondition{shouldRefresh: true} - mockEtags := &mockETagsClient{changed: false} - - // Setup a provider - azappcfg := &AzureAppConfiguration{ - kvRefreshTimer: mockTimer, - watchedSettingsMonitor: mockEtags, - } - - // Attempt to refresh - err := azappcfg.Refresh(context.Background()) - - // Verify no error and that refresh was attempted but no changes were detected - assert.NoError(t, err) - assert.Equal(t, 1, mockEtags.checkCallCount) - assert.True(t, mockTimer.resetCalled, "Timer should be reset even when no changes detected") -} - -func TestRefreshEnabled_EmptyWatchedSettings(t *testing.T) { - // Test verifying validation when refresh is enabled but no watched settings - options := &Options{ - RefreshOptions: KeyValueRefreshOptions{ - Enabled: true, // Enabled but without watched settings - WatchedSettings: []WatchedSetting{}, - }, - } - - // Verify error - err := verifyOptions(options) - require.Error(t, err) - assert.Contains(t, err.Error(), "watched settings cannot be empty") -} - -func TestRefreshEnabled_IntervalTooShort(t *testing.T) { - // Test verifying validation when refresh interval is too short - options := &Options{ - RefreshOptions: KeyValueRefreshOptions{ - Enabled: true, - Interval: 500 * time.Millisecond, // Too short, should be at least minimalRefreshInterval - WatchedSettings: []WatchedSetting{ - {Key: "test-key", Label: "test-label"}, - }, - }, - } - - // Verify error - err := verifyOptions(options) - require.Error(t, err) - assert.Contains(t, err.Error(), "key value refresh interval cannot be less than") -} - -func TestRefreshEnabled_EmptyWatchedSettingKey(t *testing.T) { - // Test verifying validation when a watched setting has an empty key - options := &Options{ - RefreshOptions: KeyValueRefreshOptions{ - Enabled: true, - WatchedSettings: []WatchedSetting{ - {Key: "", Label: "test-label"}, // Empty key should be rejected - }, - }, - } - - // Verify error - err := verifyOptions(options) - require.Error(t, err) - assert.Contains(t, err.Error(), "watched setting key cannot be empty") -} - -func TestRefreshEnabled_InvalidWatchedSettingKey(t *testing.T) { - // Test verifying validation when watched setting keys contain invalid chars - options := &Options{ - RefreshOptions: KeyValueRefreshOptions{ - Enabled: true, - WatchedSettings: []WatchedSetting{ - {Key: "test*key", Label: "test-label"}, // Key contains wildcard, not allowed - }, - }, - } - - // Verify error - err := verifyOptions(options) - require.Error(t, err) - assert.Contains(t, err.Error(), "watched setting key cannot contain") -} - -func TestRefreshEnabled_InvalidWatchedSettingLabel(t *testing.T) { - // Test verifying validation when watched setting labels contain invalid chars - options := &Options{ - RefreshOptions: KeyValueRefreshOptions{ - Enabled: true, - WatchedSettings: []WatchedSetting{ - {Key: "test-key", Label: "test*label"}, // Label contains wildcard, not allowed - }, - }, - } - - // Verify error - err := verifyOptions(options) - require.Error(t, err) - assert.Contains(t, err.Error(), "watched setting label cannot contain") -} - -func TestRefreshEnabled_ValidSettings(t *testing.T) { - // Test verifying valid refresh options pass validation - options := &Options{ - RefreshOptions: KeyValueRefreshOptions{ - Enabled: true, - Interval: 5 * time.Second, // Valid interval - WatchedSettings: []WatchedSetting{ - {Key: "test-key-1", Label: "test-label-1"}, - {Key: "test-key-2", Label: ""}, // Empty label should be normalized later - }, - }, - } - - // Verify no error - err := verifyOptions(options) - assert.NoError(t, err) -} - -func TestNormalizedWatchedSettings(t *testing.T) { - // Test the normalizedWatchedSettings function - settings := []WatchedSetting{ - {Key: "key1", Label: "label1"}, - {Key: "key2", Label: ""}, // Empty label should be set to defaultLabel - } - - normalized := normalizedWatchedSettings(settings) - - // Verify results - assert.Len(t, normalized, 2) - assert.Equal(t, "key1", normalized[0].Key) - assert.Equal(t, "label1", normalized[0].Label) - assert.Equal(t, "key2", normalized[1].Key) - assert.Equal(t, defaultLabel, normalized[1].Label) -} - -func TestRefresh_ErrorDuringETagCheck(t *testing.T) { - // Setup mocks - mockTimer := &mockRefreshCondition{shouldRefresh: true} - mockEtags := &mockETagsClient{ - err: fmt.Errorf("etag check failed"), - } - - // Setup provider - azappcfg := &AzureAppConfiguration{ - kvRefreshTimer: mockTimer, - watchedSettingsMonitor: mockEtags, - } - - // Attempt to refresh - err := azappcfg.Refresh(context.Background()) - - // Verify error and that timer was not reset - assert.Error(t, err) - assert.Contains(t, err.Error(), "etag check failed") - assert.False(t, mockTimer.resetCalled, "Timer should not be reset on error") -} - -// Additional test to verify real RefreshTimer behavior -func TestRealRefreshTimer(t *testing.T) { - // Create a real refresh timer with a short interval - timer := refreshtimer.New(100 * time.Millisecond) - - // Initially it should not be time to refresh - assert.False(t, timer.ShouldRefresh(), "New timer should not immediately indicate refresh needed") - - // After the interval passes, it should indicate time to refresh - time.Sleep(110 * time.Millisecond) - assert.True(t, timer.ShouldRefresh(), "Timer should indicate refresh needed after interval") - - // After reset, it should not be time to refresh again - timer.Reset() - assert.False(t, timer.ShouldRefresh(), "Timer should not indicate refresh needed right after reset") -} From 83489e5df3182cac4c0dbd51b7a6337443c235ad Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Mon, 14 Apr 2025 19:51:52 +0800 Subject: [PATCH 04/16] update --- .../azureappconfiguration.go | 4 -- .../internal/refreshtimer/refresh_timer.go | 46 ------------------- azureappconfiguration/options.go | 4 +- 3 files changed, 2 insertions(+), 52 deletions(-) delete mode 100644 azureappconfiguration/internal/refreshtimer/refresh_timer.go diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index a25d941..5173ba2 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -92,11 +92,7 @@ func Load(ctx context.Context, authentication AuthenticationOptions, options *Op if options.RefreshOptions.Enabled { azappcfg.kvRefreshTimer = refresh.NewTimer(options.RefreshOptions.Interval) - azappcfg.watchedSettings = normalizedWatchedSettings(options.RefreshOptions.WatchedSettings) azappcfg.sentinelETags = make(map[WatchedSetting]*azcore.ETag) - } - - if err := azappcfg.load(ctx); err != nil { return nil, err } // Set the initial load finished flag diff --git a/azureappconfiguration/internal/refreshtimer/refresh_timer.go b/azureappconfiguration/internal/refreshtimer/refresh_timer.go deleted file mode 100644 index 15847a0..0000000 --- a/azureappconfiguration/internal/refreshtimer/refresh_timer.go +++ /dev/null @@ -1,46 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -package refreshtimer - -import "time" - -// RefreshTimer manages the timing for refresh operations -type RefreshTimer struct { - interval time.Duration // How often refreshes should occur - nextRefreshTime time.Time // When the next refresh should occur -} - -// RefreshCondition interface defines the methods a refresh timer should implement -type RefreshCondition interface { - ShouldRefresh() bool - Reset() -} - -const ( - DefaultRefreshInterval time.Duration = 30 * time.Second -) - -// New creates a new refresh timer with the specified interval -// If interval is zero or negative, it falls back to the DefaultRefreshInterval -func New(interval time.Duration) *RefreshTimer { - // Use default interval if not specified or invalid - if interval <= 0 { - interval = DefaultRefreshInterval - } - - return &RefreshTimer{ - interval: interval, - nextRefreshTime: time.Now().Add(interval), - } -} - -// ShouldRefresh checks whether it's time for a refresh -func (rt *RefreshTimer) ShouldRefresh() bool { - return !time.Now().Before(rt.nextRefreshTime) -} - -// Reset resets the timer for the next refresh cycle -func (rt *RefreshTimer) Reset() { - rt.nextRefreshTime = time.Now().Add(rt.interval) -} diff --git a/azureappconfiguration/options.go b/azureappconfiguration/options.go index 84c4a44..a8e0d81 100644 --- a/azureappconfiguration/options.go +++ b/azureappconfiguration/options.go @@ -22,9 +22,9 @@ type Options struct { // Selectors defines what key-values to load from Azure App Configuration // Each selector combines a key filter and label filter // If selectors are not provided, all key-values with no label are loaded by default. - Selectors []Selector + Selectors []Selector // RefreshOptions contains optional parameters to configure the behavior of key-value settings refresh - RefreshOptions KeyValueRefreshOptions + RefreshOptions KeyValueRefreshOptions // KeyVaultOptions configures how Key Vault references are resolved. KeyVaultOptions KeyVaultOptions From 6a06eb372c7f34a031a78be256e8e27f879f7631 Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Wed, 16 Apr 2025 12:58:03 +0800 Subject: [PATCH 05/16] update --- azureappconfiguration/azureappconfiguration.go | 1 + 1 file changed, 1 insertion(+) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index 5173ba2..06ecce3 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -92,6 +92,7 @@ func Load(ctx context.Context, authentication AuthenticationOptions, options *Op if options.RefreshOptions.Enabled { azappcfg.kvRefreshTimer = refresh.NewTimer(options.RefreshOptions.Interval) + azappcfg.watchedSettings = normalizedWatchedSettings(options.RefreshOptions.WatchedSettings) azappcfg.sentinelETags = make(map[WatchedSetting]*azcore.ETag) return nil, err } From c7d8d744c9fd5db31d9bc32699f638f4489aba41 Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Thu, 17 Apr 2025 17:11:18 +0800 Subject: [PATCH 06/16] update --- azureappconfiguration/azureappconfiguration.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index 06ecce3..2797719 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -43,8 +43,8 @@ type AzureAppConfiguration struct { onRefreshSuccess []func() tracingOptions tracing.Options - clientManager *configurationClientManager - resolver *keyVaultReferenceResolver + clientManager *configurationClientManager + resolver *keyVaultReferenceResolver refreshInProgress atomic.Bool } @@ -94,6 +94,9 @@ func Load(ctx context.Context, authentication AuthenticationOptions, options *Op azappcfg.kvRefreshTimer = refresh.NewTimer(options.RefreshOptions.Interval) azappcfg.watchedSettings = normalizedWatchedSettings(options.RefreshOptions.WatchedSettings) azappcfg.sentinelETags = make(map[WatchedSetting]*azcore.ETag) + } + + if err := azappcfg.load(ctx); err != nil { return nil, err } // Set the initial load finished flag @@ -510,5 +513,5 @@ func (azappcfg *AzureAppConfiguration) newKeyValueRefreshClient() refreshClient client: azappcfg.clientManager.staticClient.client, tracingOptions: azappcfg.tracingOptions, }, - } + } } From 3cb66bdf2b9ade7102fde8c76e848aa7b2c39fab Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Fri, 18 Apr 2025 18:06:40 +0800 Subject: [PATCH 07/16] secret refresh --- .../azureappconfiguration.go | 152 +++++++++++++----- .../azureappconfiguration_test.go | 10 +- azureappconfiguration/constants.go | 3 +- azureappconfiguration/options.go | 10 ++ azureappconfiguration/refresh_test.go | 10 +- azureappconfiguration/utils.go | 7 + 6 files changed, 145 insertions(+), 47 deletions(-) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index 2797719..33bf6ed 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -19,6 +19,7 @@ import ( "log" "os" "strconv" + "maps" "strings" "sync" "sync/atomic" @@ -33,14 +34,18 @@ import ( // An AzureAppConfiguration is a configuration provider that stores and manages settings sourced from Azure App Configuration. type AzureAppConfiguration struct { - keyValues map[string]any + keyValues map[string]any + secrets map[string]string + kvSelectors []Selector trimPrefixes []string watchedSettings []WatchedSetting - sentinelETags map[WatchedSetting]*azcore.ETag - kvRefreshTimer refresh.Condition - onRefreshSuccess []func() + sentinelETags map[WatchedSetting]*azcore.ETag + keyVaultRefs map[string]string // unversioned Key Vault references + kvRefreshTimer refresh.Condition + secretRefreshTimer refresh.Condition + onRefreshSuccess []func() tracingOptions tracing.Options clientManager *configurationClientManager @@ -81,6 +86,7 @@ func Load(ctx context.Context, authentication AuthenticationOptions, options *Op azappcfg := new(AzureAppConfiguration) azappcfg.tracingOptions = configureTracingOptions(options) azappcfg.keyValues = make(map[string]any) + azappcfg.secrets = make(map[string]string) azappcfg.kvSelectors = deduplicateSelectors(options.Selectors) azappcfg.trimPrefixes = options.TrimKeyPrefixes azappcfg.clientManager = clientManager @@ -96,6 +102,11 @@ func Load(ctx context.Context, authentication AuthenticationOptions, options *Op azappcfg.sentinelETags = make(map[WatchedSetting]*azcore.ETag) } + if options.KeyVaultOptions.RefreshOptions.Enabled { + azappcfg.secretRefreshTimer = refresh.NewTimer(options.KeyVaultOptions.RefreshOptions.Interval) + azappcfg.keyVaultRefs = make(map[string]string) + } + if err := azappcfg.load(ctx); err != nil { return nil, err } @@ -187,7 +198,7 @@ func (azappcfg *AzureAppConfiguration) GetBytes(options *ConstructionOptions) ([ // Returns: // - An error if refresh is not configured, or if the refresh operation fails func (azappcfg *AzureAppConfiguration) Refresh(ctx context.Context) error { - if azappcfg.kvRefreshTimer == nil { + if azappcfg.kvRefreshTimer == nil && azappcfg.secretRefreshTimer == nil { return fmt.Errorf("refresh is not configured") } @@ -199,19 +210,24 @@ func (azappcfg *AzureAppConfiguration) Refresh(ctx context.Context) error { // Reset the flag when we're done defer azappcfg.refreshInProgress.Store(false) - // Check if it's time to perform a refresh based on the timer interval - if !azappcfg.kvRefreshTimer.ShouldRefresh() { - return nil - } - // Attempt to refresh and check if any values were actually updated - refreshed, err := azappcfg.refreshKeyValues(ctx, azappcfg.newKeyValueRefreshClient()) + keyValueRefreshed, err := azappcfg.refreshKeyValues(ctx, azappcfg.newKeyValueRefreshClient()) if err != nil { return fmt.Errorf("failed to refresh configuration: %w", err) } + // Attempt to refresh secrets and check if any values were actually updated + // Key Value refresh process includes secret refresh process, no need to refresh secrets if key values are refreshed + secretRefreshed := false + if !keyValueRefreshed { + secretRefreshed, err = azappcfg.refreshSecrets(ctx) + if err != nil { + return fmt.Errorf("failed to refresh secrets: %w", err) + } + } + // Only execute callbacks if actual changes were applied - if refreshed { + if keyValueRefreshed || secretRefreshed { for _, callback := range azappcfg.onRefreshSuccess { if callback != nil { callback() @@ -283,6 +299,7 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin var useAIConfiguration, useAIChatCompletionConfiguration bool kvSettings := make(map[string]any, len(settingsResponse.settings)) keyVaultRefs := make(map[string]string) + unversionedKVRefs := make(map[string]string) for _, setting := range settingsResponse.settings { if setting.Key == nil { continue @@ -303,6 +320,9 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin continue // ignore feature flag while getting key value settings case secretReferenceContentType: keyVaultRefs[trimmedKey] = *setting.Value + if secretMetadata, _ := parse(*setting.Value); secretMetadata != nil && secretMetadata.version == "" { + unversionedKVRefs[trimmedKey] = *setting.Value + } default: if isJsonContentType(setting.ContentType) { var v any @@ -326,43 +346,63 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin azappcfg.tracingOptions.UseAIConfiguration = useAIConfiguration azappcfg.tracingOptions.UseAIChatCompletionConfiguration = useAIChatCompletionConfiguration - var eg errgroup.Group - resolvedSecrets := sync.Map{} - if len(keyVaultRefs) > 0 { - if azappcfg.resolver.credential == nil && azappcfg.resolver.secretResolver == nil { - return fmt.Errorf("no Key Vault credential or SecretResolver configured") - } + secrets, err := azappcfg.loadSecret(ctx, keyVaultRefs) + if err != nil { + return fmt.Errorf("failed to load secrets: %w", err) + } - for key, kvRef := range keyVaultRefs { - key, kvRef := key, kvRef - eg.Go(func() error { - resolvedSecret, err := azappcfg.resolver.resolveSecret(ctx, kvRef) - if err != nil { - return fmt.Errorf("fail to resolve the Key Vault reference '%s': %s", key, err.Error()) - } - resolvedSecrets.Store(key, resolvedSecret) - return nil - }) - } + azappcfg.keyValues = kvSettings + azappcfg.secrets = secrets + azappcfg.keyVaultRefs = unversionedKVRefs - if err := eg.Wait(); err != nil { - return err - } + return nil +} + +func (azappcfg *AzureAppConfiguration) loadSecret(ctx context.Context, keyVaultRefs map[string]string) (map[string]string, error) { + secrets := make(map[string]string) + if len(keyVaultRefs) == 0 { + return secrets, nil + } + + if azappcfg.resolver.credential == nil && azappcfg.resolver.secretResolver == nil { + return secrets, fmt.Errorf("no Key Vault credential or SecretResolver configured") + } + + resolvedSecrets := sync.Map{} + var eg errgroup.Group + for key, kvRef := range keyVaultRefs { + key, kvRef := key, kvRef + eg.Go(func() error { + resolvedSecret, err := azappcfg.resolver.resolveSecret(ctx, kvRef) + if err != nil { + return fmt.Errorf("fail to resolve the Key Vault reference '%s': %s", key, err.Error()) + } + resolvedSecrets.Store(key, resolvedSecret) + return nil + }) + } + + if err := eg.Wait(); err != nil { + return secrets, fmt.Errorf("failed to resolve Key Vault references: %w", err) } resolvedSecrets.Range(func(key, value interface{}) bool { - kvSettings[key.(string)] = value.(string) + secrets[key.(string)] = value.(string) return true }) - azappcfg.keyValues = kvSettings - - return nil + return secrets, nil } // refreshKeyValues checks if any watched settings have changed and reloads configuration if needed // Returns true if configuration was actually refreshed, false otherwise func (azappcfg *AzureAppConfiguration) refreshKeyValues(ctx context.Context, refreshClient refreshClient) (bool, error) { + if azappcfg.kvRefreshTimer == nil || + !azappcfg.kvRefreshTimer.ShouldRefresh() { + // Timer not expired, no need to refresh + return false, nil + } + // Check if any ETags have changed eTagChanged, err := refreshClient.monitor.checkIfETagChanged(ctx) if err != nil { @@ -402,6 +442,40 @@ func (azappcfg *AzureAppConfiguration) refreshKeyValues(ctx context.Context, ref return true, nil } +func (azappcfg *AzureAppConfiguration) refreshSecrets(ctx context.Context) (bool, error) { + if azappcfg.secretRefreshTimer == nil || + !azappcfg.secretRefreshTimer.ShouldRefresh() { + // Timer not expired, no need to refresh + return false, nil + } + + if len(azappcfg.keyVaultRefs) == 0 { + azappcfg.secretRefreshTimer.Reset() + return false, nil + } + + unversionedSecrets, err := azappcfg.loadSecret(ctx, azappcfg.keyVaultRefs) + if err != nil { + return false, fmt.Errorf("failed to refresh secrets: %w", err) + } + + // Check if any secrets have changed + changed := false + secrets := make(map[string]string) + maps.Copy(secrets, azappcfg.secrets) + for key, newSecret := range unversionedSecrets { + if oldSecret, exists := secrets[key]; !exists || oldSecret != newSecret { + changed = true + secrets[key] = newSecret + } + } + + // Reset the timer only after successful refresh + azappcfg.secrets = secrets + azappcfg.secretRefreshTimer.Reset() + return changed, nil +} + func (azappcfg *AzureAppConfiguration) trimPrefix(key string) string { result := key for _, prefix := range azappcfg.trimPrefixes { @@ -456,6 +530,10 @@ func (azappcfg *AzureAppConfiguration) constructHierarchicalMap(separator string tree.Insert(strings.Split(k, separator), v) } + for k, v := range azappcfg.secrets { + tree.Insert(strings.Split(k, separator), v) + } + return tree.Build() } @@ -513,5 +591,5 @@ func (azappcfg *AzureAppConfiguration) newKeyValueRefreshClient() refreshClient client: azappcfg.clientManager.staticClient.client, tracingOptions: azappcfg.tracingOptions, }, - } + } } diff --git a/azureappconfiguration/azureappconfiguration_test.go b/azureappconfiguration/azureappconfiguration_test.go index 38feef2..71da403 100644 --- a/azureappconfiguration/azureappconfiguration_test.go +++ b/azureappconfiguration/azureappconfiguration_test.go @@ -77,6 +77,7 @@ func TestLoadKeyValues_WithKeyVaultReferences(t *testing.T) { }, kvSelectors: deduplicateSelectors([]Selector{}), keyValues: make(map[string]any), + secrets: make(map[string]string), resolver: &keyVaultReferenceResolver{ clients: sync.Map{}, secretResolver: mockSecretResolver, @@ -87,7 +88,7 @@ func TestLoadKeyValues_WithKeyVaultReferences(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "value1", *azappcfg.keyValues["key1"].(*string)) - assert.Equal(t, "resolved-secret", azappcfg.keyValues["secret1"]) + assert.Equal(t, "resolved-secret", azappcfg.secrets["secret1"]) mockSettingsClient.AssertExpectations(t) mockSecretResolver.AssertExpectations(t) } @@ -695,6 +696,7 @@ func TestLoadKeyValues_WithConcurrentKeyVaultReferences(t *testing.T) { }, kvSelectors: deduplicateSelectors([]Selector{}), keyValues: make(map[string]any), + secrets: make(map[string]string), resolver: &keyVaultReferenceResolver{ clients: sync.Map{}, secretResolver: mockResolver, @@ -713,9 +715,9 @@ func TestLoadKeyValues_WithConcurrentKeyVaultReferences(t *testing.T) { // Verify results assert.NoError(t, err) assert.Equal(t, "value1", *azappcfg.keyValues["standard"].(*string)) - assert.Equal(t, "resolved-secret1", azappcfg.keyValues["secret1"]) - assert.Equal(t, "resolved-secret2", azappcfg.keyValues["secret2"]) - assert.Equal(t, "resolved-secret3", azappcfg.keyValues["secret3"]) + assert.Equal(t, "resolved-secret1", azappcfg.secrets["secret1"]) + assert.Equal(t, "resolved-secret2", azappcfg.secrets["secret2"]) + assert.Equal(t, "resolved-secret3", azappcfg.secrets["secret3"]) // Verify all resolver calls were made mockResolver.AssertNumberOfCalls(t, "ResolveSecret", 3) diff --git a/azureappconfiguration/constants.go b/azureappconfiguration/constants.go index 028387d..d3ccaa7 100644 --- a/azureappconfiguration/constants.go +++ b/azureappconfiguration/constants.go @@ -24,5 +24,6 @@ const ( // Refresh interval constants const ( // minimalRefreshInterval is the minimum allowed refresh interval for key-value settings - minimalRefreshInterval time.Duration = time.Second + minimalRefreshInterval time.Duration = time.Second + minimalSecretRefreshInterval time.Duration = 1 * time.Minute ) diff --git a/azureappconfiguration/options.go b/azureappconfiguration/options.go index a8e0d81..888b1d5 100644 --- a/azureappconfiguration/options.go +++ b/azureappconfiguration/options.go @@ -104,6 +104,16 @@ type KeyVaultOptions struct { // SecretResolver specifies a custom implementation for resolving Key Vault references. // When provided, this takes precedence over using the default resolver with Credential. SecretResolver SecretResolver + + // RefreshOptions specifies the behavior of key vault reference resolution refresh + // Refresh interval must be greater than 1 minute. + RefreshOptions RefreshOptions +} + +// RefreshOptions contains optional parameters to configure the behavior of refresh +type RefreshOptions struct { + Interval time.Duration + Enabled bool } // ConstructionOptions contains parameters for parsing keys with hierarchical structure. diff --git a/azureappconfiguration/refresh_test.go b/azureappconfiguration/refresh_test.go index c2ed49d..d7379d9 100644 --- a/azureappconfiguration/refresh_test.go +++ b/azureappconfiguration/refresh_test.go @@ -231,7 +231,7 @@ func (m *mockKvRefreshClient) getSettings(ctx context.Context) (*settingsRespons // TestRefreshKeyValues_NoChanges tests when no ETags change is detected func TestRefreshKeyValues_NoChanges(t *testing.T) { // Setup mocks - mockTimer := &mockRefreshCondition{} + mockTimer := &mockRefreshCondition{shouldRefresh: true} mockMonitor := &mockETagsClient{changed: false} mockLoader := &mockKvRefreshClient{} mockSentinels := &mockKvRefreshClient{} @@ -262,7 +262,7 @@ func TestRefreshKeyValues_NoChanges(t *testing.T) { // TestRefreshKeyValues_ChangesDetected tests when ETags changed and reload succeeds func TestRefreshKeyValues_ChangesDetected(t *testing.T) { // Setup mocks for successful refresh - mockTimer := &mockRefreshCondition{} + mockTimer := &mockRefreshCondition{shouldRefresh: true} mockMonitor := &mockETagsClient{changed: true} mockLoader := &mockKvRefreshClient{} mockSentinels := &mockKvRefreshClient{} @@ -294,7 +294,7 @@ func TestRefreshKeyValues_ChangesDetected(t *testing.T) { // TestRefreshKeyValues_LoaderError tests when loader client returns an error func TestRefreshKeyValues_LoaderError(t *testing.T) { // Setup mocks with loader error - mockTimer := &mockRefreshCondition{} + mockTimer := &mockRefreshCondition{shouldRefresh: true} mockMonitor := &mockETagsClient{changed: true} mockLoader := &mockKvRefreshClient{err: fmt.Errorf("loader error")} mockSentinels := &mockKvRefreshClient{} @@ -325,7 +325,7 @@ func TestRefreshKeyValues_LoaderError(t *testing.T) { // TestRefreshKeyValues_SentinelError tests when sentinel client returns an error func TestRefreshKeyValues_SentinelError(t *testing.T) { // Setup mocks with sentinel error - mockTimer := &mockRefreshCondition{} + mockTimer := &mockRefreshCondition{shouldRefresh: true} mockMonitor := &mockETagsClient{changed: true} mockLoader := &mockKvRefreshClient{} mockSentinels := &mockKvRefreshClient{err: fmt.Errorf("sentinel error")} @@ -358,7 +358,7 @@ func TestRefreshKeyValues_SentinelError(t *testing.T) { // TestRefreshKeyValues_MonitorError tests when monitor client returns an error func TestRefreshKeyValues_MonitorError(t *testing.T) { // Setup mocks with monitor error - mockTimer := &mockRefreshCondition{} + mockTimer := &mockRefreshCondition{shouldRefresh: true} mockMonitor := &mockETagsClient{err: fmt.Errorf("monitor error")} mockLoader := &mockKvRefreshClient{} mockSentinels := &mockKvRefreshClient{} diff --git a/azureappconfiguration/utils.go b/azureappconfiguration/utils.go index f567360..0e014c1 100644 --- a/azureappconfiguration/utils.go +++ b/azureappconfiguration/utils.go @@ -55,6 +55,13 @@ func verifyOptions(options *Options) error { } } + if options.KeyVaultOptions.RefreshOptions.Enabled { + if options.KeyVaultOptions.RefreshOptions.Interval != 0 && + options.KeyVaultOptions.RefreshOptions.Interval < minimalSecretRefreshInterval { + return fmt.Errorf("key vault refresh interval cannot be less than %s", minimalSecretRefreshInterval) + } + } + return nil } From 0cac0032b4bcb0f3007f36690c954c3f3a5bbd61 Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Thu, 24 Apr 2025 16:08:27 +0800 Subject: [PATCH 08/16] update --- azureappconfiguration/azureappconfiguration.go | 8 ++++---- azureappconfiguration/refresh_test.go | 16 ---------------- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index 33bf6ed..bb65905 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -17,9 +17,9 @@ import ( "encoding/json" "fmt" "log" + "maps" "os" "strconv" - "maps" "strings" "sync" "sync/atomic" @@ -46,10 +46,10 @@ type AzureAppConfiguration struct { kvRefreshTimer refresh.Condition secretRefreshTimer refresh.Condition onRefreshSuccess []func() - tracingOptions tracing.Options + tracingOptions tracing.Options - clientManager *configurationClientManager - resolver *keyVaultReferenceResolver + clientManager *configurationClientManager + resolver *keyVaultReferenceResolver refreshInProgress atomic.Bool } diff --git a/azureappconfiguration/refresh_test.go b/azureappconfiguration/refresh_test.go index d7379d9..e0ea267 100644 --- a/azureappconfiguration/refresh_test.go +++ b/azureappconfiguration/refresh_test.go @@ -57,22 +57,6 @@ func TestRefresh_NotConfigured(t *testing.T) { assert.Contains(t, err.Error(), "refresh is not configured") } -func TestRefresh_NotTimeToRefresh(t *testing.T) { - // Setup a provider with a timer that indicates it's not time to refresh - mockTimer := &mockRefreshCondition{shouldRefresh: false} - azappcfg := &AzureAppConfiguration{ - kvRefreshTimer: mockTimer, - } - - // Attempt to refresh - err := azappcfg.Refresh(context.Background()) - - // Verify no error and that we returned early - assert.NoError(t, err) - // Timer should not be reset if we're not refreshing - assert.False(t, mockTimer.resetCalled) -} - func TestRefreshEnabled_EmptyWatchedSettings(t *testing.T) { // Test verifying validation when refresh is enabled but no watched settings options := &Options{ From da228b020b9185994f2e9b455c8f0c7850ede34b Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Tue, 29 Apr 2025 17:43:01 +0800 Subject: [PATCH 09/16] update --- .../azureappconfiguration.go | 22 +++++++------------ .../azureappconfiguration_test.go | 10 ++++----- azureappconfiguration/constants.go | 3 +-- azureappconfiguration/utils.go | 4 ++-- 4 files changed, 15 insertions(+), 24 deletions(-) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index bb65905..86870c2 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -35,7 +35,6 @@ import ( // An AzureAppConfiguration is a configuration provider that stores and manages settings sourced from Azure App Configuration. type AzureAppConfiguration struct { keyValues map[string]any - secrets map[string]string kvSelectors []Selector trimPrefixes []string @@ -86,7 +85,6 @@ func Load(ctx context.Context, authentication AuthenticationOptions, options *Op azappcfg := new(AzureAppConfiguration) azappcfg.tracingOptions = configureTracingOptions(options) azappcfg.keyValues = make(map[string]any) - azappcfg.secrets = make(map[string]string) azappcfg.kvSelectors = deduplicateSelectors(options.Selectors) azappcfg.trimPrefixes = options.TrimKeyPrefixes azappcfg.clientManager = clientManager @@ -351,15 +349,15 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin return fmt.Errorf("failed to load secrets: %w", err) } + maps.Copy(kvSettings, secrets) azappcfg.keyValues = kvSettings - azappcfg.secrets = secrets azappcfg.keyVaultRefs = unversionedKVRefs return nil } -func (azappcfg *AzureAppConfiguration) loadSecret(ctx context.Context, keyVaultRefs map[string]string) (map[string]string, error) { - secrets := make(map[string]string) +func (azappcfg *AzureAppConfiguration) loadSecret(ctx context.Context, keyVaultRefs map[string]string) (map[string]any, error) { + secrets := make(map[string]any) if len(keyVaultRefs) == 0 { return secrets, nil } @@ -461,17 +459,17 @@ func (azappcfg *AzureAppConfiguration) refreshSecrets(ctx context.Context) (bool // Check if any secrets have changed changed := false - secrets := make(map[string]string) - maps.Copy(secrets, azappcfg.secrets) + keyValues := make(map[string]any) + maps.Copy(keyValues, azappcfg.keyValues) for key, newSecret := range unversionedSecrets { - if oldSecret, exists := secrets[key]; !exists || oldSecret != newSecret { + if oldSecret, exists := keyValues[key]; !exists || oldSecret != newSecret { changed = true - secrets[key] = newSecret + keyValues[key] = newSecret } } // Reset the timer only after successful refresh - azappcfg.secrets = secrets + azappcfg.keyValues = keyValues azappcfg.secretRefreshTimer.Reset() return changed, nil } @@ -530,10 +528,6 @@ func (azappcfg *AzureAppConfiguration) constructHierarchicalMap(separator string tree.Insert(strings.Split(k, separator), v) } - for k, v := range azappcfg.secrets { - tree.Insert(strings.Split(k, separator), v) - } - return tree.Build() } diff --git a/azureappconfiguration/azureappconfiguration_test.go b/azureappconfiguration/azureappconfiguration_test.go index 71da403..38feef2 100644 --- a/azureappconfiguration/azureappconfiguration_test.go +++ b/azureappconfiguration/azureappconfiguration_test.go @@ -77,7 +77,6 @@ func TestLoadKeyValues_WithKeyVaultReferences(t *testing.T) { }, kvSelectors: deduplicateSelectors([]Selector{}), keyValues: make(map[string]any), - secrets: make(map[string]string), resolver: &keyVaultReferenceResolver{ clients: sync.Map{}, secretResolver: mockSecretResolver, @@ -88,7 +87,7 @@ func TestLoadKeyValues_WithKeyVaultReferences(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "value1", *azappcfg.keyValues["key1"].(*string)) - assert.Equal(t, "resolved-secret", azappcfg.secrets["secret1"]) + assert.Equal(t, "resolved-secret", azappcfg.keyValues["secret1"]) mockSettingsClient.AssertExpectations(t) mockSecretResolver.AssertExpectations(t) } @@ -696,7 +695,6 @@ func TestLoadKeyValues_WithConcurrentKeyVaultReferences(t *testing.T) { }, kvSelectors: deduplicateSelectors([]Selector{}), keyValues: make(map[string]any), - secrets: make(map[string]string), resolver: &keyVaultReferenceResolver{ clients: sync.Map{}, secretResolver: mockResolver, @@ -715,9 +713,9 @@ func TestLoadKeyValues_WithConcurrentKeyVaultReferences(t *testing.T) { // Verify results assert.NoError(t, err) assert.Equal(t, "value1", *azappcfg.keyValues["standard"].(*string)) - assert.Equal(t, "resolved-secret1", azappcfg.secrets["secret1"]) - assert.Equal(t, "resolved-secret2", azappcfg.secrets["secret2"]) - assert.Equal(t, "resolved-secret3", azappcfg.secrets["secret3"]) + assert.Equal(t, "resolved-secret1", azappcfg.keyValues["secret1"]) + assert.Equal(t, "resolved-secret2", azappcfg.keyValues["secret2"]) + assert.Equal(t, "resolved-secret3", azappcfg.keyValues["secret3"]) // Verify all resolver calls were made mockResolver.AssertNumberOfCalls(t, "ResolveSecret", 3) diff --git a/azureappconfiguration/constants.go b/azureappconfiguration/constants.go index d3ccaa7..028387d 100644 --- a/azureappconfiguration/constants.go +++ b/azureappconfiguration/constants.go @@ -24,6 +24,5 @@ const ( // Refresh interval constants const ( // minimalRefreshInterval is the minimum allowed refresh interval for key-value settings - minimalRefreshInterval time.Duration = time.Second - minimalSecretRefreshInterval time.Duration = 1 * time.Minute + minimalRefreshInterval time.Duration = time.Second ) diff --git a/azureappconfiguration/utils.go b/azureappconfiguration/utils.go index 0e014c1..d7c56f8 100644 --- a/azureappconfiguration/utils.go +++ b/azureappconfiguration/utils.go @@ -57,8 +57,8 @@ func verifyOptions(options *Options) error { if options.KeyVaultOptions.RefreshOptions.Enabled { if options.KeyVaultOptions.RefreshOptions.Interval != 0 && - options.KeyVaultOptions.RefreshOptions.Interval < minimalSecretRefreshInterval { - return fmt.Errorf("key vault refresh interval cannot be less than %s", minimalSecretRefreshInterval) + options.KeyVaultOptions.RefreshOptions.Interval < minimalRefreshInterval { + return fmt.Errorf("key vault refresh interval cannot be less than %s", minimalRefreshInterval) } } From 87fa85a9956bb4b388ea050a8a33003d061d39e8 Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Wed, 30 Apr 2025 15:15:39 +0800 Subject: [PATCH 10/16] update --- azureappconfiguration/azureappconfiguration.go | 8 ++------ azureappconfiguration/keyvault.go | 16 ++++++++++++++++ azureappconfiguration/options.go | 7 +++++-- azureappconfiguration/refresh_test.go | 2 +- azureappconfiguration/utils.go | 2 +- 5 files changed, 25 insertions(+), 10 deletions(-) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index 86870c2..515337a 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -197,7 +197,7 @@ func (azappcfg *AzureAppConfiguration) GetBytes(options *ConstructionOptions) ([ // - An error if refresh is not configured, or if the refresh operation fails func (azappcfg *AzureAppConfiguration) Refresh(ctx context.Context) error { if azappcfg.kvRefreshTimer == nil && azappcfg.secretRefreshTimer == nil { - return fmt.Errorf("refresh is not configured") + return fmt.Errorf("refresh is not enabled for key values or key vault data") } // Try to set refreshInProgress to true, returning false if it was already true @@ -297,7 +297,6 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin var useAIConfiguration, useAIChatCompletionConfiguration bool kvSettings := make(map[string]any, len(settingsResponse.settings)) keyVaultRefs := make(map[string]string) - unversionedKVRefs := make(map[string]string) for _, setting := range settingsResponse.settings { if setting.Key == nil { continue @@ -318,9 +317,6 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin continue // ignore feature flag while getting key value settings case secretReferenceContentType: keyVaultRefs[trimmedKey] = *setting.Value - if secretMetadata, _ := parse(*setting.Value); secretMetadata != nil && secretMetadata.version == "" { - unversionedKVRefs[trimmedKey] = *setting.Value - } default: if isJsonContentType(setting.ContentType) { var v any @@ -351,7 +347,7 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin maps.Copy(kvSettings, secrets) azappcfg.keyValues = kvSettings - azappcfg.keyVaultRefs = unversionedKVRefs + azappcfg.keyVaultRefs = getUnversionedKeyVaultRefs(keyVaultRefs) return nil } diff --git a/azureappconfiguration/keyvault.go b/azureappconfiguration/keyvault.go index 5e607ab..b224fed 100644 --- a/azureappconfiguration/keyvault.go +++ b/azureappconfiguration/keyvault.go @@ -140,3 +140,19 @@ func parse(reference string) (*secretMetadata, error) { version: secretVersion, }, nil } + +func getUnversionedKeyVaultRefs(refs map[string]string) map[string]string { + unversionedRefs := make(map[string]string) + for key, value := range refs { + var kvRef keyVaultReference + // If it is an invalid key vault reference, error will be returned when resolveSecret is called + json.Unmarshal([]byte(value), &kvRef) + + // Parse the URI to get metadata (host, secret name, version) + if secretMeta, _ := parse(kvRef.URI); secretMeta != nil && secretMeta.version == "" { + unversionedRefs[key] = value + } + } + + return unversionedRefs +} diff --git a/azureappconfiguration/options.go b/azureappconfiguration/options.go index 888b1d5..af411a5 100644 --- a/azureappconfiguration/options.go +++ b/azureappconfiguration/options.go @@ -106,14 +106,17 @@ type KeyVaultOptions struct { SecretResolver SecretResolver // RefreshOptions specifies the behavior of key vault reference resolution refresh - // Refresh interval must be greater than 1 minute. RefreshOptions RefreshOptions } // RefreshOptions contains optional parameters to configure the behavior of refresh type RefreshOptions struct { + // Interval specifies the minimum time interval between consecutive refresh operations + // Must be greater than 1 second. Interval time.Duration - Enabled bool + + // Enabled specifies whether the provider should automatically refresh when data is changed. + Enabled bool } // ConstructionOptions contains parameters for parsing keys with hierarchical structure. diff --git a/azureappconfiguration/refresh_test.go b/azureappconfiguration/refresh_test.go index e0ea267..5a0b2e0 100644 --- a/azureappconfiguration/refresh_test.go +++ b/azureappconfiguration/refresh_test.go @@ -54,7 +54,7 @@ func TestRefresh_NotConfigured(t *testing.T) { // Verify that an error is returned require.Error(t, err) - assert.Contains(t, err.Error(), "refresh is not configured") + assert.Contains(t, err.Error(), "refresh is not enabled for key values or key vault data") } func TestRefreshEnabled_EmptyWatchedSettings(t *testing.T) { diff --git a/azureappconfiguration/utils.go b/azureappconfiguration/utils.go index d7c56f8..23ee47a 100644 --- a/azureappconfiguration/utils.go +++ b/azureappconfiguration/utils.go @@ -58,7 +58,7 @@ func verifyOptions(options *Options) error { if options.KeyVaultOptions.RefreshOptions.Enabled { if options.KeyVaultOptions.RefreshOptions.Interval != 0 && options.KeyVaultOptions.RefreshOptions.Interval < minimalRefreshInterval { - return fmt.Errorf("key vault refresh interval cannot be less than %s", minimalRefreshInterval) + return fmt.Errorf("key vault data refresh interval cannot be less than %s", minimalRefreshInterval) } } From 290ccefba125a0abd89f9de084a461a2f1b89cff Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Wed, 30 Apr 2025 17:01:45 +0800 Subject: [PATCH 11/16] de-deup settings --- .../azureappconfiguration.go | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index 515337a..5e93648 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -28,6 +28,7 @@ import ( "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/tracing" "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/tree" "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/data/azappconfig" decoder "github.com/go-viper/mapstructure/v2" "golang.org/x/sync/errgroup" ) @@ -214,13 +215,13 @@ func (azappcfg *AzureAppConfiguration) Refresh(ctx context.Context) error { return fmt.Errorf("failed to refresh configuration: %w", err) } - // Attempt to refresh secrets and check if any values were actually updated - // Key Value refresh process includes secret refresh process, no need to refresh secrets if key values are refreshed + // Attempt to refresh Key Vault secret and check if any values were actually updated + // No need to refresh Key Vault secret if key values are refreshed secretRefreshed := false if !keyValueRefreshed { - secretRefreshed, err = azappcfg.refreshSecrets(ctx) + secretRefreshed, err = azappcfg.refreshKeyVaultSecrets(ctx) if err != nil { - return fmt.Errorf("failed to refresh secrets: %w", err) + return fmt.Errorf("failed to refresh Azure Key Vault secret: %w", err) } } @@ -294,9 +295,8 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin return err } - var useAIConfiguration, useAIChatCompletionConfiguration bool - kvSettings := make(map[string]any, len(settingsResponse.settings)) - keyVaultRefs := make(map[string]string) + // de-duplicate settings + rawSettings := make(map[string]azappconfig.Setting, len(settingsResponse.settings)) for _, setting := range settingsResponse.settings { if setting.Key == nil { continue @@ -306,7 +306,13 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin log.Printf("Key of the setting '%s' is trimmed to the empty string, just ignore it", *setting.Key) continue } + rawSettings[trimmedKey] = setting + } + var useAIConfiguration, useAIChatCompletionConfiguration bool + kvSettings := make(map[string]any, len(settingsResponse.settings)) + keyVaultRefs := make(map[string]string) + for trimmedKey, setting := range rawSettings { if setting.ContentType == nil || setting.Value == nil { kvSettings[trimmedKey] = setting.Value continue @@ -340,7 +346,7 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin azappcfg.tracingOptions.UseAIConfiguration = useAIConfiguration azappcfg.tracingOptions.UseAIChatCompletionConfiguration = useAIChatCompletionConfiguration - secrets, err := azappcfg.loadSecret(ctx, keyVaultRefs) + secrets, err := azappcfg.fetchKeyVaultSecrets(ctx, keyVaultRefs) if err != nil { return fmt.Errorf("failed to load secrets: %w", err) } @@ -352,7 +358,7 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin return nil } -func (azappcfg *AzureAppConfiguration) loadSecret(ctx context.Context, keyVaultRefs map[string]string) (map[string]any, error) { +func (azappcfg *AzureAppConfiguration) fetchKeyVaultSecrets(ctx context.Context, keyVaultRefs map[string]string) (map[string]any, error) { secrets := make(map[string]any) if len(keyVaultRefs) == 0 { return secrets, nil @@ -436,7 +442,7 @@ func (azappcfg *AzureAppConfiguration) refreshKeyValues(ctx context.Context, ref return true, nil } -func (azappcfg *AzureAppConfiguration) refreshSecrets(ctx context.Context) (bool, error) { +func (azappcfg *AzureAppConfiguration) refreshKeyVaultSecrets(ctx context.Context) (bool, error) { if azappcfg.secretRefreshTimer == nil || !azappcfg.secretRefreshTimer.ShouldRefresh() { // Timer not expired, no need to refresh @@ -448,7 +454,7 @@ func (azappcfg *AzureAppConfiguration) refreshSecrets(ctx context.Context) (bool return false, nil } - unversionedSecrets, err := azappcfg.loadSecret(ctx, azappcfg.keyVaultRefs) + unversionedSecrets, err := azappcfg.fetchKeyVaultSecrets(ctx, azappcfg.keyVaultRefs) if err != nil { return false, fmt.Errorf("failed to refresh secrets: %w", err) } From 5b62fcfbe4e2cac9617b277e28abc242aa46e92f Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Wed, 30 Apr 2025 17:10:04 +0800 Subject: [PATCH 12/16] update --- azureappconfiguration/options.go | 2 +- azureappconfiguration/utils.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/azureappconfiguration/options.go b/azureappconfiguration/options.go index af411a5..b4afe31 100644 --- a/azureappconfiguration/options.go +++ b/azureappconfiguration/options.go @@ -105,7 +105,7 @@ type KeyVaultOptions struct { // When provided, this takes precedence over using the default resolver with Credential. SecretResolver SecretResolver - // RefreshOptions specifies the behavior of key vault reference resolution refresh + // RefreshOptions specifies the behavior of Key Vault reference resolution refresh RefreshOptions RefreshOptions } diff --git a/azureappconfiguration/utils.go b/azureappconfiguration/utils.go index 23ee47a..603d265 100644 --- a/azureappconfiguration/utils.go +++ b/azureappconfiguration/utils.go @@ -58,7 +58,7 @@ func verifyOptions(options *Options) error { if options.KeyVaultOptions.RefreshOptions.Enabled { if options.KeyVaultOptions.RefreshOptions.Interval != 0 && options.KeyVaultOptions.RefreshOptions.Interval < minimalRefreshInterval { - return fmt.Errorf("key vault data refresh interval cannot be less than %s", minimalRefreshInterval) + return fmt.Errorf("refresh interval of Azure Key Vault secret cannot be less than %s", minimalRefreshInterval) } } From d7240fc3fd19890fe4807d0693a7509fd0792950 Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Wed, 7 May 2025 13:51:11 +0800 Subject: [PATCH 13/16] add request tracing for key vault refresh --- azureappconfiguration/azureappconfiguration.go | 1 + azureappconfiguration/internal/tracing/tracing.go | 5 +++++ .../internal/tracing/tracing_test.go | 14 ++++++++++++++ 3 files changed, 20 insertions(+) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index 5e93648..a6c6408 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -104,6 +104,7 @@ func Load(ctx context.Context, authentication AuthenticationOptions, options *Op if options.KeyVaultOptions.RefreshOptions.Enabled { azappcfg.secretRefreshTimer = refresh.NewTimer(options.KeyVaultOptions.RefreshOptions.Interval) azappcfg.keyVaultRefs = make(map[string]string) + azappcfg.tracingOptions.KeyVaultRefreshConfigured = true } if err := azappcfg.load(ctx); err != nil { diff --git a/azureappconfiguration/internal/tracing/tracing.go b/azureappconfiguration/internal/tracing/tracing.go index fdecf93..f677eb2 100644 --- a/azureappconfiguration/internal/tracing/tracing.go +++ b/azureappconfiguration/internal/tracing/tracing.go @@ -35,6 +35,7 @@ const ( RequestTypeKey = "RequestType" HostTypeKey = "Host" KeyVaultConfiguredTag = "UsesKeyVault" + KeyVaultRefreshConfiguredTag = "RefreshesKeyVault" FeaturesKey = "Features" AIConfigurationTag = "AI" AIChatCompletionConfigurationTag = "AICC" @@ -52,6 +53,7 @@ type Options struct { InitialLoadFinished bool Host HostType KeyVaultConfigured bool + KeyVaultRefreshConfigured bool UseAIConfiguration bool UseAIChatCompletionConfiguration bool } @@ -87,7 +89,10 @@ func CreateCorrelationContextHeader(ctx context.Context, options Options) http.H if options.KeyVaultConfigured { output = append(output, KeyVaultConfiguredTag) + } + if options.KeyVaultRefreshConfigured { + output = append(output, KeyVaultRefreshConfiguredTag) } features := make([]string, 0) diff --git a/azureappconfiguration/internal/tracing/tracing_test.go b/azureappconfiguration/internal/tracing/tracing_test.go index 86336b0..90d7a50 100644 --- a/azureappconfiguration/internal/tracing/tracing_test.go +++ b/azureappconfiguration/internal/tracing/tracing_test.go @@ -71,6 +71,20 @@ func TestCreateCorrelationContextHeader(t *testing.T) { assert.Contains(t, corrContext, KeyVaultConfiguredTag) }) + t.Run("with KeyVaultRefresh configured", func(t *testing.T) { + ctx := context.Background() + options := Options{ + KeyVaultConfigured: true, + KeyVaultRefreshConfigured: true, + } + + header := CreateCorrelationContextHeader(ctx, options) + + // Should contain KeyVaultRefreshConfiguredTag + corrContext := header.Get(CorrelationContextHeader) + assert.Contains(t, corrContext, KeyVaultRefreshConfiguredTag) + }) + t.Run("with AI configuration", func(t *testing.T) { ctx := context.Background() options := Options{ From 715672c5b33fea21edb1cb1dd9277d5496c2d0d3 Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Wed, 7 May 2025 14:38:01 +0800 Subject: [PATCH 14/16] minimal key vault refresh interval update --- azureappconfiguration/constants.go | 2 ++ azureappconfiguration/utils.go | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/azureappconfiguration/constants.go b/azureappconfiguration/constants.go index 028387d..204be8a 100644 --- a/azureappconfiguration/constants.go +++ b/azureappconfiguration/constants.go @@ -25,4 +25,6 @@ const ( const ( // minimalRefreshInterval is the minimum allowed refresh interval for key-value settings minimalRefreshInterval time.Duration = time.Second + // minimalKeyVaultRefreshInterval is the minimum allowed refresh interval for Key Vault references + minimalKeyVaultRefreshInterval time.Duration = 1 * time.Minute ) diff --git a/azureappconfiguration/utils.go b/azureappconfiguration/utils.go index 603d265..ba30819 100644 --- a/azureappconfiguration/utils.go +++ b/azureappconfiguration/utils.go @@ -58,7 +58,7 @@ func verifyOptions(options *Options) error { if options.KeyVaultOptions.RefreshOptions.Enabled { if options.KeyVaultOptions.RefreshOptions.Interval != 0 && options.KeyVaultOptions.RefreshOptions.Interval < minimalRefreshInterval { - return fmt.Errorf("refresh interval of Azure Key Vault secret cannot be less than %s", minimalRefreshInterval) + return fmt.Errorf("refresh interval of Azure Key Vault secret cannot be less than %s", minimalKeyVaultRefreshInterval) } } From c7cb25305a948d0ff9590580e0733b178b1c16e7 Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Thu, 8 May 2025 14:03:56 +0800 Subject: [PATCH 15/16] add tests --- .../azureappconfiguration.go | 18 +- .../internal/tracing/tracing_test.go | 2 +- azureappconfiguration/options.go | 1 - azureappconfiguration/refresh_test.go | 178 +++++++++++++++++- 4 files changed, 187 insertions(+), 12 deletions(-) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index a6c6408..49eb63b 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -199,7 +199,7 @@ func (azappcfg *AzureAppConfiguration) GetBytes(options *ConstructionOptions) ([ // - An error if refresh is not configured, or if the refresh operation fails func (azappcfg *AzureAppConfiguration) Refresh(ctx context.Context) error { if azappcfg.kvRefreshTimer == nil && azappcfg.secretRefreshTimer == nil { - return fmt.Errorf("refresh is not enabled for key values or key vault data") + return fmt.Errorf("refresh is not enabled for either key values or Key Vault secrets") } // Try to set refreshInProgress to true, returning false if it was already true @@ -216,13 +216,13 @@ func (azappcfg *AzureAppConfiguration) Refresh(ctx context.Context) error { return fmt.Errorf("failed to refresh configuration: %w", err) } - // Attempt to refresh Key Vault secret and check if any values were actually updated - // No need to refresh Key Vault secret if key values are refreshed + // Attempt to reload Key Vault secrets and check if any values were actually updated + // No need to reload Key Vault secrets if key values are refreshed secretRefreshed := false if !keyValueRefreshed { secretRefreshed, err = azappcfg.refreshKeyVaultSecrets(ctx) if err != nil { - return fmt.Errorf("failed to refresh Azure Key Vault secret: %w", err) + return fmt.Errorf("failed to reload Key Vault secrets: %w", err) } } @@ -347,9 +347,9 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin azappcfg.tracingOptions.UseAIConfiguration = useAIConfiguration azappcfg.tracingOptions.UseAIChatCompletionConfiguration = useAIChatCompletionConfiguration - secrets, err := azappcfg.fetchKeyVaultSecrets(ctx, keyVaultRefs) + secrets, err := azappcfg.loadKeyVaultSecrets(ctx, keyVaultRefs) if err != nil { - return fmt.Errorf("failed to load secrets: %w", err) + return fmt.Errorf("failed to load Key Vault secrets: %w", err) } maps.Copy(kvSettings, secrets) @@ -359,7 +359,7 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin return nil } -func (azappcfg *AzureAppConfiguration) fetchKeyVaultSecrets(ctx context.Context, keyVaultRefs map[string]string) (map[string]any, error) { +func (azappcfg *AzureAppConfiguration) loadKeyVaultSecrets(ctx context.Context, keyVaultRefs map[string]string) (map[string]any, error) { secrets := make(map[string]any) if len(keyVaultRefs) == 0 { return secrets, nil @@ -455,9 +455,9 @@ func (azappcfg *AzureAppConfiguration) refreshKeyVaultSecrets(ctx context.Contex return false, nil } - unversionedSecrets, err := azappcfg.fetchKeyVaultSecrets(ctx, azappcfg.keyVaultRefs) + unversionedSecrets, err := azappcfg.loadKeyVaultSecrets(ctx, azappcfg.keyVaultRefs) if err != nil { - return false, fmt.Errorf("failed to refresh secrets: %w", err) + return false, fmt.Errorf("failed to reload Key Vault secrets: %w", err) } // Check if any secrets have changed diff --git a/azureappconfiguration/internal/tracing/tracing_test.go b/azureappconfiguration/internal/tracing/tracing_test.go index 90d7a50..4729901 100644 --- a/azureappconfiguration/internal/tracing/tracing_test.go +++ b/azureappconfiguration/internal/tracing/tracing_test.go @@ -74,7 +74,7 @@ func TestCreateCorrelationContextHeader(t *testing.T) { t.Run("with KeyVaultRefresh configured", func(t *testing.T) { ctx := context.Background() options := Options{ - KeyVaultConfigured: true, + KeyVaultConfigured: true, KeyVaultRefreshConfigured: true, } diff --git a/azureappconfiguration/options.go b/azureappconfiguration/options.go index b4afe31..d944f34 100644 --- a/azureappconfiguration/options.go +++ b/azureappconfiguration/options.go @@ -112,7 +112,6 @@ type KeyVaultOptions struct { // RefreshOptions contains optional parameters to configure the behavior of refresh type RefreshOptions struct { // Interval specifies the minimum time interval between consecutive refresh operations - // Must be greater than 1 second. Interval time.Duration // Enabled specifies whether the provider should automatically refresh when data is changed. diff --git a/azureappconfiguration/refresh_test.go b/azureappconfiguration/refresh_test.go index 5a0b2e0..d8a3ef4 100644 --- a/azureappconfiguration/refresh_test.go +++ b/azureappconfiguration/refresh_test.go @@ -5,7 +5,10 @@ package azureappconfiguration import ( "context" + "encoding/json" "fmt" + "net/url" + "sync" "testing" "time" @@ -54,7 +57,7 @@ func TestRefresh_NotConfigured(t *testing.T) { // Verify that an error is returned require.Error(t, err) - assert.Contains(t, err.Error(), "refresh is not enabled for key values or key vault data") + assert.Contains(t, err.Error(), "refresh is not enabled for either key values or Key Vault secrets") } func TestRefreshEnabled_EmptyWatchedSettings(t *testing.T) { @@ -387,3 +390,176 @@ func TestRefresh_AlreadyInProgress(t *testing.T) { // Verify no error and that we returned early assert.NoError(t, err) } + +func TestRefreshKeyVaultSecrets_WithMockResolver_Scenarios(t *testing.T) { + // resolutionInstruction defines how a specific Key Vault URI should be resolved by the mock. + type resolutionInstruction struct { + Value string + Err error + } + + tests := []struct { + name string + description string // Optional: for more clarity + + // Initial state for AzureAppConfiguration + initialTimer refresh.Condition + initialKeyVaultRefs map[string]string // map[appConfigKey]jsonURIString -> e.g., {"secretAppKey": `{"uri":"https://mykv.vault.azure.net/secrets/mysecret"}`} + initialKeyValues map[string]any // map[appConfigKey]currentValue + + // Configuration for the mockSecretResolver + // map[actualURIString]resolutionInstruction -> e.g., {"https://mykv.vault.azure.net/secrets/mysecret": {Value: "resolvedValue", Err: nil}} + secretResolutionConfig map[string]resolutionInstruction + + // Expected outcomes + expectedChanged bool + expectedErrSubstring string // Substring of the error expected from refreshKeyVaultSecrets + expectedTimerReset bool + expectedFinalKeyValues map[string]any + }{ + { + name: "Timer is nil", + initialTimer: nil, + initialKeyVaultRefs: map[string]string{"appSecret1": `{"uri":"https://kv.com/s/s1/"}`}, + initialKeyValues: map[string]any{"appSecret1": "oldVal1"}, + expectedChanged: false, + expectedTimerReset: false, + expectedFinalKeyValues: map[string]any{"appSecret1": "oldVal1"}, + }, + { + name: "Timer not expired", + initialTimer: &mockRefreshCondition{shouldRefresh: false}, + initialKeyVaultRefs: map[string]string{"appSecret1": `{"uri":"https://kv.com/s/s1/"}`}, + initialKeyValues: map[string]any{"appSecret1": "oldVal1"}, + expectedChanged: false, + expectedTimerReset: false, + expectedFinalKeyValues: map[string]any{"appSecret1": "oldVal1"}, + }, + { + name: "No keyVaultRefs, timer ready", + initialTimer: &mockRefreshCondition{shouldRefresh: true}, + initialKeyVaultRefs: map[string]string{}, + initialKeyValues: map[string]any{"appKey": "appVal"}, + expectedChanged: false, + expectedTimerReset: true, + expectedFinalKeyValues: map[string]any{"appKey": "appVal"}, + }, + { + name: "Secrets not changed, timer ready", + initialTimer: &mockRefreshCondition{shouldRefresh: true}, + initialKeyVaultRefs: map[string]string{"appSecret1": `{"uri":"https://myvault.vault.azure.net/secrets/s1"}`}, + initialKeyValues: map[string]any{"appSecret1": "currentVal", "appKey": "appVal"}, + secretResolutionConfig: map[string]resolutionInstruction{ + "https://myvault.vault.azure.net/secrets/s1": {Value: "currentVal"}, + }, + expectedChanged: false, + expectedTimerReset: true, + expectedFinalKeyValues: map[string]any{"appSecret1": "currentVal", "appKey": "appVal"}, + }, + { + name: "Secrets changed - existing secret updated, timer ready", + initialTimer: &mockRefreshCondition{shouldRefresh: true}, + initialKeyVaultRefs: map[string]string{"appSecret1": `{"uri":"https://myvault.vault.azure.net/secrets/s1"}`}, + initialKeyValues: map[string]any{"appSecret1": "oldVal1", "appKey": "appVal"}, + secretResolutionConfig: map[string]resolutionInstruction{ + "https://myvault.vault.azure.net/secrets/s1": {Value: "newVal1"}, + }, + expectedChanged: true, + expectedTimerReset: true, + expectedFinalKeyValues: map[string]any{ + "appSecret1": "newVal1", + "appKey": "appVal", + }, + }, + { + name: "Secrets changed - mix of updated, unchanged, timer ready", + initialTimer: &mockRefreshCondition{shouldRefresh: true}, + initialKeyVaultRefs: map[string]string{"s1": `{"uri":"https://myvault.vault.azure.net/secrets/s1"}`, "s3": `{"uri":"https://myvault.vault.azure.net/secrets/s3"}`}, + initialKeyValues: map[string]any{"s1": "oldVal1", "s3": "val3Unchanged", "appKey": "appVal"}, + secretResolutionConfig: map[string]resolutionInstruction{ + "https://myvault.vault.azure.net/secrets/s1": {Value: "newVal1"}, + "https://myvault.vault.azure.net/secrets/s3": {Value: "val3Unchanged"}, + }, + expectedChanged: true, + expectedTimerReset: true, + expectedFinalKeyValues: map[string]any{ + "s1": "newVal1", + "s3": "val3Unchanged", + "appKey": "appVal", + }, + }, + } + + ctx := context.Background() + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Setup + currentKeyValues := make(map[string]any) + if tc.initialKeyValues != nil { + for k, v := range tc.initialKeyValues { + currentKeyValues[k] = v + } + } + + mockResolver := new(mockSecretResolver) + azappcfg := &AzureAppConfiguration{ + secretRefreshTimer: tc.initialTimer, + keyVaultRefs: tc.initialKeyVaultRefs, + keyValues: currentKeyValues, + resolver: &keyVaultReferenceResolver{ + clients: sync.Map{}, + secretResolver: mockResolver, + }, + } + + if tc.initialKeyVaultRefs != nil && tc.secretResolutionConfig != nil { + for _, jsonRefString := range tc.initialKeyVaultRefs { + var kvRefInternal struct { // Re-declare locally or use the actual keyVaultReference type if accessible + URI string `json:"uri"` + } + err := json.Unmarshal([]byte(jsonRefString), &kvRefInternal) + if err != nil { + continue + } + actualURIString := kvRefInternal.URI + if actualURIString == "" { + continue + } + + if instruction, ok := tc.secretResolutionConfig[actualURIString]; ok { + parsedURL, parseErr := url.Parse(actualURIString) + require.NoError(t, parseErr, "Test setup: Failed to parse URI for mock expectation: %s", actualURIString) + mockResolver.On("ResolveSecret", ctx, *parsedURL).Return(instruction.Value, instruction.Err).Once() + } + } + } + + // Execute + changed, err := azappcfg.refreshKeyVaultSecrets(context.Background()) + + // Assert Error + if tc.expectedErrSubstring != "" { + require.Error(t, err, "Expected an error but got nil") + assert.Contains(t, err.Error(), tc.expectedErrSubstring, "Error message mismatch") + } else { + require.NoError(t, err, "Expected no error but got: %v", err) + } + + // Assert Changed Flag + assert.Equal(t, tc.expectedChanged, changed, "Changed flag mismatch") + + // Assert Timer Reset + if mockTimer, ok := tc.initialTimer.(*mockRefreshCondition); ok { + assert.Equal(t, tc.expectedTimerReset, mockTimer.resetCalled, "Timer reset state mismatch") + } else if tc.initialTimer == nil { + assert.False(t, tc.expectedTimerReset, "Timer was nil, reset should not be expected") + } + + // Assert Final KeyValues + assert.Equal(t, tc.expectedFinalKeyValues, azappcfg.keyValues, "Final keyValues mismatch") + + // Verify mock expectations + mockResolver.AssertExpectations(t) + }) + } +} From 4fd59be6cab65f7f23b11b0bb0c798f3a466a861 Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Thu, 8 May 2025 14:24:04 +0800 Subject: [PATCH 16/16] update --- azureappconfiguration/azureappconfiguration.go | 2 +- azureappconfiguration/options.go | 3 ++- azureappconfiguration/utils.go | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index 49eb63b..84f3ed0 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -366,7 +366,7 @@ func (azappcfg *AzureAppConfiguration) loadKeyVaultSecrets(ctx context.Context, } if azappcfg.resolver.credential == nil && azappcfg.resolver.secretResolver == nil { - return secrets, fmt.Errorf("no Key Vault credential or SecretResolver configured") + return secrets, fmt.Errorf("no Key Vault credential or SecretResolver was configured in KeyVaultOptions") } resolvedSecrets := sync.Map{} diff --git a/azureappconfiguration/options.go b/azureappconfiguration/options.go index d944f34..9716786 100644 --- a/azureappconfiguration/options.go +++ b/azureappconfiguration/options.go @@ -105,7 +105,8 @@ type KeyVaultOptions struct { // When provided, this takes precedence over using the default resolver with Credential. SecretResolver SecretResolver - // RefreshOptions specifies the behavior of Key Vault reference resolution refresh + // RefreshOptions specifies the behavior of Key Vault secrets refresh. + // Sets the refresh interval for periodically reloading secrets from Key Vault, must be greater than 1 minute. RefreshOptions RefreshOptions } diff --git a/azureappconfiguration/utils.go b/azureappconfiguration/utils.go index ba30819..6bb473a 100644 --- a/azureappconfiguration/utils.go +++ b/azureappconfiguration/utils.go @@ -58,7 +58,7 @@ func verifyOptions(options *Options) error { if options.KeyVaultOptions.RefreshOptions.Enabled { if options.KeyVaultOptions.RefreshOptions.Interval != 0 && options.KeyVaultOptions.RefreshOptions.Interval < minimalRefreshInterval { - return fmt.Errorf("refresh interval of Azure Key Vault secret cannot be less than %s", minimalKeyVaultRefreshInterval) + return fmt.Errorf("refresh interval of Key Vault secrets cannot be less than %s", minimalKeyVaultRefreshInterval) } }