From df28774216c1867b25ec98ca0a656a51f3595ad3 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/12] resolve conflict --- .../azureappconfiguration.go | 219 +++++++++++++++++- azureappconfiguration/constants.go | 8 + azureappconfiguration/options.go | 22 ++ azureappconfiguration/settings_client.go | 62 +++++ azureappconfiguration/utils.go | 26 +++ internal/refreshtimer/refresh_timer.go | 46 ++++ 6 files changed, 372 insertions(+), 11 deletions(-) create mode 100644 internal/refreshtimer/refresh_timer.go diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index c91f570..5a890c6 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -23,21 +23,31 @@ import ( "sync" "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/tracing" + "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/refreshtimer" "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/tree" + "github.com/Azure/azure-sdk-for-go/sdk/azcore" decoder "github.com/go-viper/mapstructure/v2" "golang.org/x/sync/errgroup" ) // An AzureAppConfiguration is a configuration provider that stores and manages settings sourced from Azure App Configuration. type AzureAppConfiguration struct { - keyValues map[string]any - kvSelectors []Selector - trimPrefixes []string + keyValues map[string]any + kvSelectors []Selector + trimPrefixes []string + watchedSettings []WatchedSetting + + sentinalETags sync.Map + kvRefreshTimer refreshtimer.RefreshCondition + onRefreshSuccess []func() + tracingOptions tracing.Options - clientManager *configurationClientManager - resolver *keyVaultReferenceResolver + watchedSettingsMonitor eTagsClient + clientManager *configurationClientManager + resolver *keyVaultReferenceResolver - tracingOptions tracing.Options + refreshMutex sync.Mutex + refreshInProgress bool } // Load initializes a new AzureAppConfiguration instance and loads the configuration data from @@ -56,6 +66,10 @@ func Load(ctx context.Context, authentication AuthenticationOptions, options *Op return nil, err } + if err := verifyOptions(options); err != nil { + return nil, err + } + if options == nil { options = &Options{} } @@ -77,6 +91,11 @@ func Load(ctx context.Context, authentication AuthenticationOptions, options *Op credential: options.KeyVaultOptions.Credential, } + if options.RefreshOptions.Enabled { + azappcfg.kvRefreshTimer = refreshtimer.New(options.RefreshOptions.Interval) + azappcfg.watchedSettings = normalizedWatchedSettings(options.RefreshOptions.WatchedSettings) + } + if err := azappcfg.load(ctx); err != nil { return nil, err } @@ -150,14 +169,98 @@ func (azappcfg *AzureAppConfiguration) GetBytes(options *ConstructionOptions) ([ return json.Marshal(azappcfg.constructHierarchicalMap(options.Separator)) } +func (azappcfg *AzureAppConfiguration) Refresh(ctx context.Context) error { + if azappcfg.kvRefreshTimer == nil { + return fmt.Errorf("refresh is not configured") + } + + // Use a mutex to prevent concurrent refreshes + azappcfg.refreshMutex.Lock() + + // Check if refresh is already in progress + if azappcfg.refreshInProgress { + azappcfg.refreshMutex.Unlock() + return nil + } + + // Mark refresh as in progress and unlock the mutex after function completes + azappcfg.refreshInProgress = true + defer func() { + azappcfg.refreshInProgress = false + azappcfg.refreshMutex.Unlock() + }() + + // 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) + if err != nil { + return fmt.Errorf("failed to refresh configuration: %w", err) + } + + // Only execute callbacks if actual changes were applied + if refreshed { + for _, callback := range azappcfg.onRefreshSuccess { + if callback != nil { + callback() + } + } + } + + return nil +} + +func (azappcfg *AzureAppConfiguration) OnRefreshSuccess(callback func()) { + azappcfg.onRefreshSuccess = append(azappcfg.onRefreshSuccess, callback) +} + func (azappcfg *AzureAppConfiguration) load(ctx context.Context) error { - keyValuesClient := &selectorSettingsClient{ - selectors: azappcfg.kvSelectors, - client: azappcfg.clientManager.staticClient.client, - tracingOptions: azappcfg.tracingOptions, + eg, egCtx := errgroup.WithContext(ctx) + eg.Go(func() error { + keyValuesClient := &selectorSettingsClient{ + selectors: azappcfg.kvSelectors, + client: azappcfg.clientManager.staticClient.client, + tracingOptions: azappcfg.tracingOptions, + } + return azappcfg.loadKeyValues(egCtx, keyValuesClient) + }) + + if azappcfg.kvRefreshTimer != nil { + for _, watchedSetting := range azappcfg.watchedSettings { + setting := watchedSetting + eg.Go(func() error { + watchedClient := &watchedSettingClient{ + watchedSetting: setting, + client: azappcfg.clientManager.staticClient.client, + tracingOptions: azappcfg.tracingOptions, + } + return azappcfg.loadWatchedSetting(egCtx, watchedClient) + }) + } + } + + return eg.Wait() +} + +func (azappcfg *AzureAppConfiguration) loadWatchedSetting(ctx context.Context, settingsClient settingsClient) error { + settingsResponse, err := settingsClient.getSettings(ctx) + if err != nil { + return err + } + + var eTag *azcore.ETag + if settingsResponse != nil && len(settingsResponse.settings) > 0 { + eTag = settingsResponse.settings[0].ETag } - return azappcfg.loadKeyValues(ctx, keyValuesClient) + if watchedSettingClient, ok := settingsClient.(*watchedSettingClient); ok { + azappcfg.sentinalETags.Store(watchedSettingClient.watchedSetting, eTag) + } + + return nil } func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settingsClient settingsClient) error { @@ -246,6 +349,85 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin return 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) (bool, error) { + // Initialize the monitor if needed + if azappcfg.watchedSettingsMonitor == nil { + azappcfg.watchedSettingsMonitor = &watchedSettingClient{ + eTags: azappcfg.getSentinalETags(), + client: azappcfg.clientManager.staticClient.client, + } + } + + // Check if any ETags have changed + eTagChanged, err := azappcfg.watchedSettingsMonitor.checkIfETagChanged(ctx) + if err != nil { + return false, fmt.Errorf("failed to check if watched settings have changed: %w", err) + } + + if !eTagChanged { + // No changes detected, reset timer and return + azappcfg.kvRefreshTimer.Reset() + return false, nil + } + + // Create a client for loading all key values + keyValuesClient := &selectorSettingsClient{ + selectors: azappcfg.kvSelectors, + client: azappcfg.clientManager.staticClient.client, + } + + // Use an errgroup to reload key values and watched settings concurrently + eg, egCtx := errgroup.WithContext(ctx) + + // Reload key values in one goroutine + eg.Go(func() error { + return azappcfg.loadKeyValues(egCtx, keyValuesClient) + }) + + // Reload all watched settings to get new ETags in parallel + for _, watchedSetting := range azappcfg.watchedSettings { + setting := watchedSetting + eg.Go(func() error { + watchedClient := &watchedSettingClient{ + watchedSetting: setting, + client: azappcfg.clientManager.staticClient.client, + } + return azappcfg.loadWatchedSetting(egCtx, watchedClient) + }) + } + + // Wait for all reloads to complete + if err := eg.Wait(); err != nil { + // Don't reset the timer if reload failed + return false, fmt.Errorf("failed to reload configuration: %w", err) + } + + // Update the monitor with the new ETags + if client, ok := azappcfg.watchedSettingsMonitor.(*watchedSettingClient); ok { + client.eTags = azappcfg.getSentinalETags() + } + + // Reset the timer only after successful refresh + azappcfg.kvRefreshTimer.Reset() + return true, nil +} + +// getSentinalETags converts the sync.Map of sentinel ETags into a regular map +// for use with the watchedSettingClient +func (azappcfg *AzureAppConfiguration) getSentinalETags() map[WatchedSetting]*azcore.ETag { + eTags := make(map[WatchedSetting]*azcore.ETag) + azappcfg.sentinalETags.Range(func(key, value interface{}) bool { + watchedSetting := key.(WatchedSetting) + eTag := value.(*azcore.ETag) + eTags[watchedSetting] = eTag + return true + }) + + return eTags +} + func (azappcfg *AzureAppConfiguration) trimPrefix(key string) string { result := key for _, prefix := range azappcfg.trimPrefixes { @@ -324,3 +506,18 @@ func configureTracingOptions(options *Options) tracing.Options { return tracingOption } + +func normalizedWatchedSettings(s []WatchedSetting) []WatchedSetting { + result := make([]WatchedSetting, len(s)) + for i, setting := range s { + // Make a copy of the setting + normalizedSetting := setting + if normalizedSetting.Label == "" { + normalizedSetting.Label = defaultLabel + } + + result[i] = normalizedSetting + } + + return result +} diff --git a/azureappconfiguration/constants.go b/azureappconfiguration/constants.go index 608489f..028387d 100644 --- a/azureappconfiguration/constants.go +++ b/azureappconfiguration/constants.go @@ -3,6 +3,8 @@ package azureappconfiguration +import "time" + // Configuration client constants const ( endpointKey string = "Endpoint" @@ -18,3 +20,9 @@ const ( secretReferenceContentType string = "application/vnd.microsoft.appconfig.keyvaultref+json;charset=utf-8" featureFlagContentType string = "application/vnd.microsoft.appconfig.ff+json;charset=utf-8" ) + +// Refresh interval constants +const ( + // minimalRefreshInterval is the minimum allowed refresh interval for key-value settings + minimalRefreshInterval time.Duration = time.Second +) diff --git a/azureappconfiguration/options.go b/azureappconfiguration/options.go index f3f5371..1d16c4c 100644 --- a/azureappconfiguration/options.go +++ b/azureappconfiguration/options.go @@ -6,6 +6,7 @@ package azureappconfiguration import ( "context" "net/url" + "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/data/azappconfig" @@ -22,6 +23,8 @@ type Options struct { // 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 + // RefreshOptions contains optional parameters to configure the behavior of key-value settings refresh + RefreshOptions KeyValueRefreshOptions // KeyVaultOptions configures how Key Vault references are resolved. KeyVaultOptions KeyVaultOptions @@ -57,6 +60,25 @@ type Selector struct { LabelFilter string } +// KeyValueRefreshOptions contains optional parameters to configure the behavior of key-value settings refresh +type KeyValueRefreshOptions struct { + // WatchedSettings specifies the key-value settings to watch for changes + WatchedSettings []WatchedSetting + + // Interval specifies the minimum time interval between consecutive refresh operations for the watched settings + // Must be greater than 1 second. If not provided, the default interval 30 seconds will be used + Interval time.Duration + + // Enabled specifies whether the provider should automatically refresh when the configuration is changed. + Enabled bool +} + +// WatchedSetting specifies the key and label of a key-value setting to watch for changes +type WatchedSetting struct { + Key string + Label string +} + // SecretResolver is an interface to resolve secrets from Key Vault references. // Implement this interface to provide custom secret resolution logic. type SecretResolver interface { diff --git a/azureappconfiguration/settings_client.go b/azureappconfiguration/settings_client.go index 1889cfc..31572b9 100644 --- a/azureappconfiguration/settings_client.go +++ b/azureappconfiguration/settings_client.go @@ -5,9 +5,12 @@ package azureappconfiguration import ( "context" + "errors" + "log" "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/tracing" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" "github.com/Azure/azure-sdk-for-go/sdk/data/azappconfig" ) @@ -23,10 +26,21 @@ type selectorSettingsClient struct { tracingOptions tracing.Options } +type watchedSettingClient struct { + watchedSetting WatchedSetting + eTags map[WatchedSetting]*azcore.ETag + client *azappconfig.Client + tracingOptions tracing.Options +} + type settingsClient interface { getSettings(ctx context.Context) (*settingsResponse, error) } +type eTagsClient interface { + checkIfETagChanged(ctx context.Context) (bool, error) +} + func (s *selectorSettingsClient) getSettings(ctx context.Context) (*settingsResponse, error) { if s.tracingOptions.Enabled { ctx = policy.WithHTTPHeader(ctx, tracing.CreateCorrelationContextHeader(ctx, s.tracingOptions)) @@ -55,3 +69,51 @@ func (s *selectorSettingsClient) getSettings(ctx context.Context) (*settingsResp settings: settings, }, nil } + +func (c *watchedSettingClient) getSettings(ctx context.Context) (*settingsResponse, error) { + if c.tracingOptions.Enabled { + ctx = policy.WithHTTPHeader(ctx, tracing.CreateCorrelationContextHeader(ctx, c.tracingOptions)) + } + + response, err := c.client.GetSetting(ctx, c.watchedSetting.Key, &azappconfig.GetSettingOptions{Label: to.Ptr(c.watchedSetting.Label)}) + if err != nil { + var respErr *azcore.ResponseError + if errors.As(err, &respErr) && respErr.StatusCode == 404 { + label := c.watchedSetting.Label + if label == "" || label == "\x00" { // NUL is escaped to \x00 in golang + label = "no" + } + // If the watched setting is not found, not return error + log.Printf("Watched key '%s' with %s label does not exists", c.watchedSetting.Key, label) + return nil, nil + } + + return nil, err + } + + return &settingsResponse{ + settings: []azappconfig.Setting{response.Setting}, + }, nil +} + +func (c *watchedSettingClient) checkIfETagChanged(ctx context.Context) (bool, error) { + if c.tracingOptions.Enabled { + ctx = policy.WithHTTPHeader(ctx, tracing.CreateCorrelationContextHeader(ctx, c.tracingOptions)) + } + + for watchedSetting, ETag := range c.eTags { + _, err := c.client.GetSetting(ctx, watchedSetting.Key, &azappconfig.GetSettingOptions{Label: to.Ptr(watchedSetting.Label), OnlyIfChanged: ETag}) + if err != nil { + var respErr *azcore.ResponseError + if errors.As(err, &respErr) && (respErr.StatusCode == 404 || respErr.StatusCode == 304) { + continue + } + + return false, err + } + + return true, nil + } + + return false, nil +} diff --git a/azureappconfiguration/utils.go b/azureappconfiguration/utils.go index 48cd274..f567360 100644 --- a/azureappconfiguration/utils.go +++ b/azureappconfiguration/utils.go @@ -29,6 +29,32 @@ func verifyOptions(options *Options) error { return err } + if options.RefreshOptions.Enabled { + if options.RefreshOptions.Interval != 0 && + options.RefreshOptions.Interval < minimalRefreshInterval { + return fmt.Errorf("key value refresh interval cannot be less than %s", minimalRefreshInterval) + } + + if len(options.RefreshOptions.WatchedSettings) == 0 { + return fmt.Errorf("watched settings cannot be empty") + } + + for _, watchedSetting := range options.RefreshOptions.WatchedSettings { + if watchedSetting.Key == "" { + return fmt.Errorf("watched setting key cannot be empty") + } + + if strings.Contains(watchedSetting.Key, "*") || strings.Contains(watchedSetting.Key, ",") { + return fmt.Errorf("watched setting key cannot contain '*' or ','") + } + + if watchedSetting.Label != "" && + (strings.Contains(watchedSetting.Label, "*") || strings.Contains(watchedSetting.Label, ",")) { + return fmt.Errorf("watched setting label cannot contain '*' or ','") + } + } + } + return nil } 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 0f29f3fd366b959c5d70b26f856033c23ab6f077 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/12] 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 408aa1239b28b53534ab2c154343744f13e718fe Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Fri, 28 Mar 2025 16:53:48 +0800 Subject: [PATCH 03/12] fix typo --- azureappconfiguration/azureappconfiguration.go | 6 +++--- azureappconfiguration/settings_client.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index 5a890c6..fe467c7 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -37,7 +37,7 @@ type AzureAppConfiguration struct { trimPrefixes []string watchedSettings []WatchedSetting - sentinalETags sync.Map + sentinelETags sync.Map kvRefreshTimer refreshtimer.RefreshCondition onRefreshSuccess []func() tracingOptions tracing.Options @@ -257,7 +257,7 @@ func (azappcfg *AzureAppConfiguration) loadWatchedSetting(ctx context.Context, s } if watchedSettingClient, ok := settingsClient.(*watchedSettingClient); ok { - azappcfg.sentinalETags.Store(watchedSettingClient.watchedSetting, eTag) + azappcfg.sentinelETags.Store(watchedSettingClient.watchedSetting, eTag) } return nil @@ -418,7 +418,7 @@ func (azappcfg *AzureAppConfiguration) refreshKeyValues(ctx context.Context) (bo // for use with the watchedSettingClient func (azappcfg *AzureAppConfiguration) getSentinalETags() map[WatchedSetting]*azcore.ETag { eTags := make(map[WatchedSetting]*azcore.ETag) - azappcfg.sentinalETags.Range(func(key, value interface{}) bool { + azappcfg.sentinelETags.Range(func(key, value interface{}) bool { watchedSetting := key.(WatchedSetting) eTag := value.(*azcore.ETag) eTags[watchedSetting] = eTag diff --git a/azureappconfiguration/settings_client.go b/azureappconfiguration/settings_client.go index 31572b9..1d76693 100644 --- a/azureappconfiguration/settings_client.go +++ b/azureappconfiguration/settings_client.go @@ -84,7 +84,7 @@ func (c *watchedSettingClient) getSettings(ctx context.Context) (*settingsRespon label = "no" } // If the watched setting is not found, not return error - log.Printf("Watched key '%s' with %s label does not exists", c.watchedSetting.Key, label) + log.Printf("Watched key '%s' with %s label does not exist", c.watchedSetting.Key, label) return nil, nil } From cc5178a3ad9833e3f4aae0a1700402e72012bcab Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Wed, 9 Apr 2025 21:11:24 +0800 Subject: [PATCH 04/12] update --- .../azureappconfiguration.go | 94 +++++++++---------- azureappconfiguration/settings_client.go | 41 ++++---- 2 files changed, 70 insertions(+), 65 deletions(-) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index fe467c7..b24e798 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -37,7 +37,7 @@ type AzureAppConfiguration struct { trimPrefixes []string watchedSettings []WatchedSetting - sentinelETags sync.Map + sentinelETags map[WatchedSetting]*azcore.ETag kvRefreshTimer refreshtimer.RefreshCondition onRefreshSuccess []func() tracingOptions tracing.Options @@ -94,6 +94,7 @@ func Load(ctx context.Context, authentication AuthenticationOptions, options *Op if options.RefreshOptions.Enabled { azappcfg.kvRefreshTimer = refreshtimer.New(options.RefreshOptions.Interval) azappcfg.watchedSettings = normalizedWatchedSettings(options.RefreshOptions.WatchedSettings) + azappcfg.sentinelETags = make(map[WatchedSetting]*azcore.ETag) } if err := azappcfg.load(ctx); err != nil { @@ -169,6 +170,21 @@ func (azappcfg *AzureAppConfiguration) GetBytes(options *ConstructionOptions) ([ return json.Marshal(azappcfg.constructHierarchicalMap(options.Separator)) } +// Refresh manually triggers a refresh of the configuration from Azure App Configuration. +// It checks if any watched settings have changed, and if so, reloads all configuration data. +// +// The refresh only occurs if: +// - Refresh has been configured with RefreshOptions when the client was created +// - The configured refresh interval has elapsed since the last refresh +// - No other refresh operation is currently in progress +// +// If the configuration has changed, any callback functions registered with OnRefreshSuccess will be executed. +// +// Parameters: +// - ctx: The context for the operation. +// +// 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 { return fmt.Errorf("refresh is not configured") @@ -213,6 +229,15 @@ func (azappcfg *AzureAppConfiguration) Refresh(ctx context.Context) error { return nil } +// OnRefreshSuccess registers a callback function that will be executed whenever the configuration +// is successfully refreshed and actual changes were detected. +// +// Multiple callback functions can be registered, and they will be executed in the order they were added. +// Callbacks are only executed when configuration values actually change. They run synchronously +// in the thread that initiated the refresh. +// +// Parameters: +// - callback: A function with no parameters that will be called after a successful refresh func (azappcfg *AzureAppConfiguration) OnRefreshSuccess(callback func()) { azappcfg.onRefreshSuccess = append(azappcfg.onRefreshSuccess, callback) } @@ -228,36 +253,29 @@ func (azappcfg *AzureAppConfiguration) load(ctx context.Context) error { return azappcfg.loadKeyValues(egCtx, keyValuesClient) }) - if azappcfg.kvRefreshTimer != nil { - for _, watchedSetting := range azappcfg.watchedSettings { - setting := watchedSetting - eg.Go(func() error { - watchedClient := &watchedSettingClient{ - watchedSetting: setting, - client: azappcfg.clientManager.staticClient.client, - tracingOptions: azappcfg.tracingOptions, - } - return azappcfg.loadWatchedSetting(egCtx, watchedClient) - }) - } + if azappcfg.kvRefreshTimer != nil && len(azappcfg.watchedSettings) > 0 { + eg.Go(func() error { + watchedClient := &watchedSettingClient{ + watchedSettings: azappcfg.watchedSettings, + client: azappcfg.clientManager.staticClient.client, + tracingOptions: azappcfg.tracingOptions, + } + return azappcfg.loadWatchedSettings(egCtx, watchedClient) + }) } return eg.Wait() } -func (azappcfg *AzureAppConfiguration) loadWatchedSetting(ctx context.Context, settingsClient settingsClient) error { +func (azappcfg *AzureAppConfiguration) loadWatchedSettings(ctx context.Context, settingsClient settingsClient) error { settingsResponse, err := settingsClient.getSettings(ctx) if err != nil { return err } - var eTag *azcore.ETag - if settingsResponse != nil && len(settingsResponse.settings) > 0 { - eTag = settingsResponse.settings[0].ETag - } - - if watchedSettingClient, ok := settingsClient.(*watchedSettingClient); ok { - azappcfg.sentinelETags.Store(watchedSettingClient.watchedSetting, eTag) + // Store ETags for all watched settings + if settingsResponse != nil && settingsResponse.watchedETags != nil { + azappcfg.sentinelETags = settingsResponse.watchedETags } return nil @@ -355,8 +373,9 @@ func (azappcfg *AzureAppConfiguration) refreshKeyValues(ctx context.Context) (bo // Initialize the monitor if needed if azappcfg.watchedSettingsMonitor == nil { azappcfg.watchedSettingsMonitor = &watchedSettingClient{ - eTags: azappcfg.getSentinalETags(), - client: azappcfg.clientManager.staticClient.client, + watchedSettings: azappcfg.watchedSettings, + eTags: azappcfg.sentinelETags, + client: azappcfg.clientManager.staticClient.client, } } @@ -386,15 +405,13 @@ func (azappcfg *AzureAppConfiguration) refreshKeyValues(ctx context.Context) (bo return azappcfg.loadKeyValues(egCtx, keyValuesClient) }) - // Reload all watched settings to get new ETags in parallel - for _, watchedSetting := range azappcfg.watchedSettings { - setting := watchedSetting + if len(azappcfg.watchedSettings) > 0 { eg.Go(func() error { watchedClient := &watchedSettingClient{ - watchedSetting: setting, - client: azappcfg.clientManager.staticClient.client, + watchedSettings: azappcfg.watchedSettings, + client: azappcfg.clientManager.staticClient.client, } - return azappcfg.loadWatchedSetting(egCtx, watchedClient) + return azappcfg.loadWatchedSettings(egCtx, watchedClient) }) } @@ -404,30 +421,11 @@ func (azappcfg *AzureAppConfiguration) refreshKeyValues(ctx context.Context) (bo return false, fmt.Errorf("failed to reload configuration: %w", err) } - // Update the monitor with the new ETags - if client, ok := azappcfg.watchedSettingsMonitor.(*watchedSettingClient); ok { - client.eTags = azappcfg.getSentinalETags() - } - // Reset the timer only after successful refresh azappcfg.kvRefreshTimer.Reset() return true, nil } -// getSentinalETags converts the sync.Map of sentinel ETags into a regular map -// for use with the watchedSettingClient -func (azappcfg *AzureAppConfiguration) getSentinalETags() map[WatchedSetting]*azcore.ETag { - eTags := make(map[WatchedSetting]*azcore.ETag) - azappcfg.sentinelETags.Range(func(key, value interface{}) bool { - watchedSetting := key.(WatchedSetting) - eTag := value.(*azcore.ETag) - eTags[watchedSetting] = eTag - return true - }) - - return eTags -} - func (azappcfg *AzureAppConfiguration) trimPrefix(key string) string { result := key for _, prefix := range azappcfg.trimPrefixes { diff --git a/azureappconfiguration/settings_client.go b/azureappconfiguration/settings_client.go index 1d76693..1f942fe 100644 --- a/azureappconfiguration/settings_client.go +++ b/azureappconfiguration/settings_client.go @@ -16,8 +16,8 @@ import ( ) type settingsResponse struct { - settings []azappconfig.Setting - // TODO: pageETags + settings []azappconfig.Setting + watchedETags map[WatchedSetting]*azcore.ETag } type selectorSettingsClient struct { @@ -27,9 +27,9 @@ type selectorSettingsClient struct { } type watchedSettingClient struct { - watchedSetting WatchedSetting - eTags map[WatchedSetting]*azcore.ETag - client *azappconfig.Client + watchedSettings []WatchedSetting + eTags map[WatchedSetting]*azcore.ETag + client *azappconfig.Client tracingOptions tracing.Options } @@ -75,24 +75,31 @@ func (c *watchedSettingClient) getSettings(ctx context.Context) (*settingsRespon ctx = policy.WithHTTPHeader(ctx, tracing.CreateCorrelationContextHeader(ctx, c.tracingOptions)) } - response, err := c.client.GetSetting(ctx, c.watchedSetting.Key, &azappconfig.GetSettingOptions{Label: to.Ptr(c.watchedSetting.Label)}) - if err != nil { - var respErr *azcore.ResponseError - if errors.As(err, &respErr) && respErr.StatusCode == 404 { - label := c.watchedSetting.Label - if label == "" || label == "\x00" { // NUL is escaped to \x00 in golang - label = "no" + settings := make([]azappconfig.Setting, 0, len(c.watchedSettings)) + watchedETags := make(map[WatchedSetting]*azcore.ETag) + for _, watchedSetting := range c.watchedSettings { + response, err := c.client.GetSetting(ctx, watchedSetting.Key, &azappconfig.GetSettingOptions{Label: to.Ptr(watchedSetting.Label)}) + if err != nil { + var respErr *azcore.ResponseError + if errors.As(err, &respErr) && respErr.StatusCode == 404 { + label := watchedSetting.Label + if label == "" || label == "\x00" { // NUL is escaped to \x00 in golang + label = "no" + } + // If the watched setting is not found, log and continue + log.Printf("Watched key '%s' with %s label does not exist", watchedSetting.Key, label) + continue } - // If the watched setting is not found, not return error - log.Printf("Watched key '%s' with %s label does not exist", c.watchedSetting.Key, label) - return nil, nil + return nil, err } - return nil, err + settings = append(settings, response.Setting) + watchedETags[watchedSetting] = response.Setting.ETag } return &settingsResponse{ - settings: []azappconfig.Setting{response.Setting}, + settings: settings, + watchedETags: watchedETags, }, nil } From 402edf58a0ff2abb067a82b8d0cee056ca40728c Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Fri, 11 Apr 2025 14:51:52 +0800 Subject: [PATCH 05/12] rebase --- .../internal}/refreshtimer/refresh_timer.go | 0 refresh_test.go => azureappconfiguration/refresh_test.go | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename {internal => azureappconfiguration/internal}/refreshtimer/refresh_timer.go (100%) rename refresh_test.go => azureappconfiguration/refresh_test.go (100%) 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/azureappconfiguration/refresh_test.go similarity index 100% rename from refresh_test.go rename to azureappconfiguration/refresh_test.go From 76bb18478e07e28a74eb12b4bc2de4b4d0fe7d52 Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Mon, 14 Apr 2025 19:51:52 +0800 Subject: [PATCH 06/12] update --- .../azureappconfiguration.go | 71 +++--- .../refresh_timer.go => refresh/refresh.go} | 18 +- azureappconfiguration/options.go | 12 +- azureappconfiguration/refresh_test.go | 226 ++++++++++++++---- azureappconfiguration/settings_client.go | 6 + 5 files changed, 241 insertions(+), 92 deletions(-) rename azureappconfiguration/internal/{refreshtimer/refresh_timer.go => refresh/refresh.go} (70%) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index b24e798..1d81c7c 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -23,7 +23,7 @@ import ( "sync" "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/tracing" - "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/refreshtimer" + "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/refresh" "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/tree" "github.com/Azure/azure-sdk-for-go/sdk/azcore" decoder "github.com/go-viper/mapstructure/v2" @@ -38,15 +38,14 @@ type AzureAppConfiguration struct { watchedSettings []WatchedSetting sentinelETags map[WatchedSetting]*azcore.ETag - kvRefreshTimer refreshtimer.RefreshCondition + kvRefreshTimer refresh.Condition onRefreshSuccess []func() tracingOptions tracing.Options - watchedSettingsMonitor eTagsClient - clientManager *configurationClientManager - resolver *keyVaultReferenceResolver + clientManager *configurationClientManager + resolver *keyVaultReferenceResolver - refreshMutex sync.Mutex + refreshMutex sync.RWMutex refreshInProgress bool } @@ -92,7 +91,7 @@ func Load(ctx context.Context, authentication AuthenticationOptions, options *Op } if options.RefreshOptions.Enabled { - azappcfg.kvRefreshTimer = refreshtimer.New(options.RefreshOptions.Interval) + azappcfg.kvRefreshTimer = refresh.New(options.RefreshOptions.Interval) azappcfg.watchedSettings = normalizedWatchedSettings(options.RefreshOptions.WatchedSettings) azappcfg.sentinelETags = make(map[WatchedSetting]*azcore.ETag) } @@ -190,10 +189,16 @@ func (azappcfg *AzureAppConfiguration) Refresh(ctx context.Context) error { return fmt.Errorf("refresh is not configured") } - // Use a mutex to prevent concurrent refreshes - azappcfg.refreshMutex.Lock() + azappcfg.refreshMutex.RLock() + if azappcfg.refreshInProgress { + azappcfg.refreshMutex.RUnlock() + return nil + } + azappcfg.refreshMutex.RUnlock() - // Check if refresh is already in progress + // Use a write lock to update refresh status + azappcfg.refreshMutex.Lock() + // Double-check condition after acquiring the write lock if azappcfg.refreshInProgress { azappcfg.refreshMutex.Unlock() return nil @@ -212,7 +217,7 @@ func (azappcfg *AzureAppConfiguration) Refresh(ctx context.Context) error { } // Attempt to refresh and check if any values were actually updated - refreshed, err := azappcfg.refreshKeyValues(ctx) + refreshed, err := azappcfg.refreshKeyValues(ctx, azappcfg.newKvRefreshClient()) if err != nil { return fmt.Errorf("failed to refresh configuration: %w", err) } @@ -369,18 +374,9 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin // 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) (bool, error) { - // Initialize the monitor if needed - if azappcfg.watchedSettingsMonitor == nil { - azappcfg.watchedSettingsMonitor = &watchedSettingClient{ - watchedSettings: azappcfg.watchedSettings, - eTags: azappcfg.sentinelETags, - client: azappcfg.clientManager.staticClient.client, - } - } - +func (azappcfg *AzureAppConfiguration) refreshKeyValues(ctx context.Context, refreshClient refreshClient) (bool, error) { // Check if any ETags have changed - eTagChanged, err := azappcfg.watchedSettingsMonitor.checkIfETagChanged(ctx) + eTagChanged, err := refreshClient.monitor.checkIfETagChanged(ctx) if err != nil { return false, fmt.Errorf("failed to check if watched settings have changed: %w", err) } @@ -391,26 +387,18 @@ func (azappcfg *AzureAppConfiguration) refreshKeyValues(ctx context.Context) (bo return false, nil } - // Create a client for loading all key values - keyValuesClient := &selectorSettingsClient{ - selectors: azappcfg.kvSelectors, - client: azappcfg.clientManager.staticClient.client, - } - // Use an errgroup to reload key values and watched settings concurrently eg, egCtx := errgroup.WithContext(ctx) // Reload key values in one goroutine eg.Go(func() error { - return azappcfg.loadKeyValues(egCtx, keyValuesClient) + settingsClient := refreshClient.loader + return azappcfg.loadKeyValues(egCtx, settingsClient) }) if len(azappcfg.watchedSettings) > 0 { eg.Go(func() error { - watchedClient := &watchedSettingClient{ - watchedSettings: azappcfg.watchedSettings, - client: azappcfg.clientManager.staticClient.client, - } + watchedClient := refreshClient.sentinels return azappcfg.loadWatchedSettings(egCtx, watchedClient) }) } @@ -519,3 +507,20 @@ func normalizedWatchedSettings(s []WatchedSetting) []WatchedSetting { return result } + +func (azappcfg *AzureAppConfiguration) newKvRefreshClient() refreshClient { + return refreshClient{ + loader: &selectorSettingsClient{ + selectors: azappcfg.kvSelectors, + client: azappcfg.clientManager.staticClient.client, + }, + monitor: &watchedSettingClient{ + eTags: azappcfg.sentinelETags, + client: azappcfg.clientManager.staticClient.client, + }, + sentinels: &watchedSettingClient{ + watchedSettings: azappcfg.watchedSettings, + client: azappcfg.clientManager.staticClient.client, + }, + } +} diff --git a/azureappconfiguration/internal/refreshtimer/refresh_timer.go b/azureappconfiguration/internal/refresh/refresh.go similarity index 70% rename from azureappconfiguration/internal/refreshtimer/refresh_timer.go rename to azureappconfiguration/internal/refresh/refresh.go index 15847a0..0896302 100644 --- a/azureappconfiguration/internal/refreshtimer/refresh_timer.go +++ b/azureappconfiguration/internal/refresh/refresh.go @@ -1,18 +1,18 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -package refreshtimer +package refresh import "time" -// RefreshTimer manages the timing for refresh operations -type RefreshTimer struct { +// Timer manages the timing for refresh operations +type Timer 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 { +// Condition interface defines the methods a refresh timer should implement +type Condition interface { ShouldRefresh() bool Reset() } @@ -23,24 +23,24 @@ const ( // 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 { +func New(interval time.Duration) *Timer { // Use default interval if not specified or invalid if interval <= 0 { interval = DefaultRefreshInterval } - return &RefreshTimer{ + return &Timer{ interval: interval, nextRefreshTime: time.Now().Add(interval), } } // ShouldRefresh checks whether it's time for a refresh -func (rt *RefreshTimer) ShouldRefresh() bool { +func (rt *Timer) ShouldRefresh() bool { return !time.Now().Before(rt.nextRefreshTime) } // Reset resets the timer for the next refresh cycle -func (rt *RefreshTimer) Reset() { +func (rt *Timer) Reset() { rt.nextRefreshTime = time.Now().Add(rt.interval) } diff --git a/azureappconfiguration/options.go b/azureappconfiguration/options.go index 1d16c4c..a8e0d81 100644 --- a/azureappconfiguration/options.go +++ b/azureappconfiguration/options.go @@ -22,15 +22,15 @@ 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 // ClientOptions provides options for configuring the underlying Azure App Configuration client. - ClientOptions *azappconfig.ClientOptions + ClientOptions *azappconfig.ClientOptions } // AuthenticationOptions contains parameters for authenticating with the Azure App Configuration service. @@ -38,11 +38,11 @@ type Options struct { type AuthenticationOptions struct { // Credential is a token credential for Azure EntraID Authenticaiton. // Required when Endpoint is provided. - Credential azcore.TokenCredential + Credential azcore.TokenCredential // Endpoint is the URL of the Azure App Configuration service. // Required when using token-based authentication with Credential. - Endpoint string + Endpoint string // ConnectionString is the connection string for the Azure App Configuration service. ConnectionString string @@ -52,7 +52,7 @@ type AuthenticationOptions struct { type Selector struct { // KeyFilter specifies which keys to retrieve from Azure App Configuration. // It can include wildcards, e.g. "app*" will match all keys starting with "app". - KeyFilter string + KeyFilter string // LabelFilter specifies which labels to retrieve from Azure App Configuration. // Empty string or omitted value will use the default no-label filter. diff --git a/azureappconfiguration/refresh_test.go b/azureappconfiguration/refresh_test.go index a5b1b51..17e75ad 100644 --- a/azureappconfiguration/refresh_test.go +++ b/azureappconfiguration/refresh_test.go @@ -9,7 +9,9 @@ import ( "testing" "time" - "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/refreshtimer" + "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/refresh" + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/data/azappconfig" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -85,26 +87,6 @@ func TestRefresh_NotTimeToRefresh(t *testing.T) { 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{ @@ -224,32 +206,10 @@ func TestNormalizedWatchedSettings(t *testing.T) { 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) + timer := refresh.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") @@ -262,3 +222,181 @@ func TestRealRefreshTimer(t *testing.T) { timer.Reset() assert.False(t, timer.ShouldRefresh(), "Timer should not indicate refresh needed right after reset") } + +// mockKvRefreshClient implements the settingsClient interface for testing +type mockKvRefreshClient struct { + settings []azappconfig.Setting + watchedETags map[WatchedSetting]*azcore.ETag + getCallCount int + err error +} + +func (m *mockKvRefreshClient) getSettings(ctx context.Context) (*settingsResponse, error) { + m.getCallCount++ + if m.err != nil { + return nil, m.err + } + return &settingsResponse{ + settings: m.settings, + watchedETags: m.watchedETags, + }, nil +} + +// TestRefreshKeyValues_NoChanges tests when no ETags change is detected +func TestRefreshKeyValues_NoChanges(t *testing.T) { + // Setup mocks + mockTimer := &mockRefreshCondition{} + mockMonitor := &mockETagsClient{changed: false} + mockLoader := &mockKvRefreshClient{} + mockSentinels := &mockKvRefreshClient{} + + mockClient := refreshClient{ + loader: mockLoader, + monitor: mockMonitor, + sentinels: mockSentinels, + } + + // Setup provider + azappcfg := &AzureAppConfiguration{ + kvRefreshTimer: mockTimer, + } + + // Call refreshKeyValues + refreshed, err := azappcfg.refreshKeyValues(context.Background(), mockClient) + + // Verify results + assert.NoError(t, err) + assert.False(t, refreshed, "Should return false when no changes detected") + assert.Equal(t, 1, mockMonitor.checkCallCount, "Monitor should be called exactly once") + assert.Equal(t, 0, mockLoader.getCallCount, "Loader should not be called when no changes") + assert.Equal(t, 0, mockSentinels.getCallCount, "Sentinels should not be called when no changes") + assert.True(t, mockTimer.resetCalled, "Timer should be reset even when no changes") +} + +// TestRefreshKeyValues_ChangesDetected tests when ETags changed and reload succeeds +func TestRefreshKeyValues_ChangesDetected(t *testing.T) { + // Setup mocks for successful refresh + mockTimer := &mockRefreshCondition{} + mockMonitor := &mockETagsClient{changed: true} + mockLoader := &mockKvRefreshClient{} + mockSentinels := &mockKvRefreshClient{} + + mockClient := refreshClient{ + loader: mockLoader, + monitor: mockMonitor, + sentinels: mockSentinels, + } + + // Setup provider with watchedSettings + azappcfg := &AzureAppConfiguration{ + kvRefreshTimer: mockTimer, + watchedSettings: []WatchedSetting{{Key: "test", Label: "test"}}, + } + + // Call refreshKeyValues + refreshed, err := azappcfg.refreshKeyValues(context.Background(), mockClient) + + // Verify results + assert.NoError(t, err) + assert.True(t, refreshed, "Should return true when changes detected and applied") + assert.Equal(t, 1, mockMonitor.checkCallCount, "Monitor should be called exactly once") + assert.Equal(t, 1, mockLoader.getCallCount, "Loader should be called when changes detected") + assert.Equal(t, 1, mockSentinels.getCallCount, "Sentinels should be called when changes detected") + assert.True(t, mockTimer.resetCalled, "Timer should be reset after successful refresh") +} + +// TestRefreshKeyValues_LoaderError tests when loader client returns an error +func TestRefreshKeyValues_LoaderError(t *testing.T) { + // Setup mocks with loader error + mockTimer := &mockRefreshCondition{} + mockMonitor := &mockETagsClient{changed: true} + mockLoader := &mockKvRefreshClient{err: fmt.Errorf("loader error")} + mockSentinels := &mockKvRefreshClient{} + + mockClient := refreshClient{ + loader: mockLoader, + monitor: mockMonitor, + sentinels: mockSentinels, + } + + // Setup provider + azappcfg := &AzureAppConfiguration{ + kvRefreshTimer: mockTimer, + } + + // Call refreshKeyValues + refreshed, err := azappcfg.refreshKeyValues(context.Background(), mockClient) + + // Verify results + assert.Error(t, err) + assert.False(t, refreshed, "Should return false when error occurs") + assert.Contains(t, err.Error(), "loader error") + assert.Equal(t, 1, mockMonitor.checkCallCount, "Monitor should be called exactly once") + assert.Equal(t, 1, mockLoader.getCallCount, "Loader should be called when changes detected") + assert.False(t, mockTimer.resetCalled, "Timer should not be reset when error occurs") +} + +// TestRefreshKeyValues_SentinelError tests when sentinel client returns an error +func TestRefreshKeyValues_SentinelError(t *testing.T) { + // Setup mocks with sentinel error + mockTimer := &mockRefreshCondition{} + mockMonitor := &mockETagsClient{changed: true} + mockLoader := &mockKvRefreshClient{} + mockSentinels := &mockKvRefreshClient{err: fmt.Errorf("sentinel error")} + + mockClient := refreshClient{ + loader: mockLoader, + monitor: mockMonitor, + sentinels: mockSentinels, + } + + // Setup provider with watchedSettings to ensure sentinels are used + azappcfg := &AzureAppConfiguration{ + kvRefreshTimer: mockTimer, + watchedSettings: []WatchedSetting{{Key: "test", Label: "test"}}, + } + + // Call refreshKeyValues + refreshed, err := azappcfg.refreshKeyValues(context.Background(), mockClient) + + // Verify results + assert.Error(t, err) + assert.False(t, refreshed, "Should return false when error occurs") + assert.Contains(t, err.Error(), "sentinel error") + assert.Equal(t, 1, mockMonitor.checkCallCount, "Monitor should be called exactly once") + assert.Equal(t, 1, mockLoader.getCallCount, "Loader should be called when changes detected") + assert.Equal(t, 1, mockSentinels.getCallCount, "Sentinels should be called when changes detected") + assert.False(t, mockTimer.resetCalled, "Timer should not be reset when error occurs") +} + +// TestRefreshKeyValues_MonitorError tests when monitor client returns an error +func TestRefreshKeyValues_MonitorError(t *testing.T) { + // Setup mocks with monitor error + mockTimer := &mockRefreshCondition{} + mockMonitor := &mockETagsClient{err: fmt.Errorf("monitor error")} + mockLoader := &mockKvRefreshClient{} + mockSentinels := &mockKvRefreshClient{} + + mockClient := refreshClient{ + loader: mockLoader, + monitor: mockMonitor, + sentinels: mockSentinels, + } + + // Setup provider + azappcfg := &AzureAppConfiguration{ + kvRefreshTimer: mockTimer, + } + + // Call refreshKeyValues + refreshed, err := azappcfg.refreshKeyValues(context.Background(), mockClient) + + // Verify results + assert.Error(t, err) + assert.False(t, refreshed, "Should return false when error occurs") + assert.Contains(t, err.Error(), "monitor error") + assert.Equal(t, 1, mockMonitor.checkCallCount, "Monitor should be called exactly once") + assert.Equal(t, 0, mockLoader.getCallCount, "Loader should not be called when monitor fails") + assert.Equal(t, 0, mockSentinels.getCallCount, "Sentinels should not be called when monitor fails") + assert.False(t, mockTimer.resetCalled, "Timer should not be reset when error occurs") +} diff --git a/azureappconfiguration/settings_client.go b/azureappconfiguration/settings_client.go index 1f942fe..1603130 100644 --- a/azureappconfiguration/settings_client.go +++ b/azureappconfiguration/settings_client.go @@ -41,6 +41,12 @@ type eTagsClient interface { checkIfETagChanged(ctx context.Context) (bool, error) } +type refreshClient struct { + loader settingsClient + monitor eTagsClient + sentinels settingsClient +} + func (s *selectorSettingsClient) getSettings(ctx context.Context) (*settingsResponse, error) { if s.tracingOptions.Enabled { ctx = policy.WithHTTPHeader(ctx, tracing.CreateCorrelationContextHeader(ctx, s.tracingOptions)) From 80a328669b73ad5f4c81333deb65c6d921b0888d Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Wed, 16 Apr 2025 12:58:03 +0800 Subject: [PATCH 07/12] update --- azureappconfiguration/azureappconfiguration.go | 2 +- azureappconfiguration/internal/refresh/refresh.go | 4 ++-- azureappconfiguration/refresh_test.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index 1d81c7c..a3d35b1 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -91,7 +91,7 @@ func Load(ctx context.Context, authentication AuthenticationOptions, options *Op } if options.RefreshOptions.Enabled { - azappcfg.kvRefreshTimer = refresh.New(options.RefreshOptions.Interval) + azappcfg.kvRefreshTimer = refresh.NewTimer(options.RefreshOptions.Interval) azappcfg.watchedSettings = normalizedWatchedSettings(options.RefreshOptions.WatchedSettings) azappcfg.sentinelETags = make(map[WatchedSetting]*azcore.ETag) } diff --git a/azureappconfiguration/internal/refresh/refresh.go b/azureappconfiguration/internal/refresh/refresh.go index 0896302..ba48ce3 100644 --- a/azureappconfiguration/internal/refresh/refresh.go +++ b/azureappconfiguration/internal/refresh/refresh.go @@ -21,9 +21,9 @@ const ( DefaultRefreshInterval time.Duration = 30 * time.Second ) -// New creates a new refresh timer with the specified interval +// NewTimer 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) *Timer { +func NewTimer(interval time.Duration) *Timer { // Use default interval if not specified or invalid if interval <= 0 { interval = DefaultRefreshInterval diff --git a/azureappconfiguration/refresh_test.go b/azureappconfiguration/refresh_test.go index 17e75ad..c397f57 100644 --- a/azureappconfiguration/refresh_test.go +++ b/azureappconfiguration/refresh_test.go @@ -209,7 +209,7 @@ func TestNormalizedWatchedSettings(t *testing.T) { // Additional test to verify real RefreshTimer behavior func TestRealRefreshTimer(t *testing.T) { // Create a real refresh timer with a short interval - timer := refresh.New(100 * time.Millisecond) + timer := refresh.NewTimer(100 * time.Millisecond) // Initially it should not be time to refresh assert.False(t, timer.ShouldRefresh(), "New timer should not immediately indicate refresh needed") From 57bdb0cb878a3be12d9cf612fbe275df2ccf8501 Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Thu, 17 Apr 2025 17:11:18 +0800 Subject: [PATCH 08/12] update --- .../azureappconfiguration.go | 38 ++--- azureappconfiguration/refresh_test.go | 155 ++++++++++++++++-- 2 files changed, 154 insertions(+), 39 deletions(-) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index a3d35b1..50ca2aa 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -21,6 +21,7 @@ import ( "strconv" "strings" "sync" + "sync/atomic" "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/tracing" "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/refresh" @@ -42,11 +43,11 @@ type AzureAppConfiguration struct { onRefreshSuccess []func() tracingOptions tracing.Options - clientManager *configurationClientManager - resolver *keyVaultReferenceResolver + clientManager *configurationClientManager + resolver *keyVaultReferenceResolver + newKvRefreshClient func() refreshClient - refreshMutex sync.RWMutex - refreshInProgress bool + refreshInProgress atomic.Bool } // Load initializes a new AzureAppConfiguration instance and loads the configuration data from @@ -94,6 +95,7 @@ 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) + azappcfg.newKvRefreshClient = azappcfg.newKeyValueRefreshClient } if err := azappcfg.load(ctx); err != nil { @@ -189,27 +191,13 @@ func (azappcfg *AzureAppConfiguration) Refresh(ctx context.Context) error { return fmt.Errorf("refresh is not configured") } - azappcfg.refreshMutex.RLock() - if azappcfg.refreshInProgress { - azappcfg.refreshMutex.RUnlock() - return nil + // Try to set refreshInProgress to true, returning false if it was already true + if !azappcfg.refreshInProgress.CompareAndSwap(false, true) { + return nil // Another refresh is already in progress } - azappcfg.refreshMutex.RUnlock() - // Use a write lock to update refresh status - azappcfg.refreshMutex.Lock() - // Double-check condition after acquiring the write lock - if azappcfg.refreshInProgress { - azappcfg.refreshMutex.Unlock() - return nil - } - - // Mark refresh as in progress and unlock the mutex after function completes - azappcfg.refreshInProgress = true - defer func() { - azappcfg.refreshInProgress = false - azappcfg.refreshMutex.Unlock() - }() + // 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() { @@ -508,7 +496,7 @@ func normalizedWatchedSettings(s []WatchedSetting) []WatchedSetting { return result } -func (azappcfg *AzureAppConfiguration) newKvRefreshClient() refreshClient { +func (azappcfg *AzureAppConfiguration) newKeyValueRefreshClient() refreshClient { return refreshClient{ loader: &selectorSettingsClient{ selectors: azappcfg.kvSelectors, @@ -522,5 +510,5 @@ func (azappcfg *AzureAppConfiguration) newKvRefreshClient() refreshClient { watchedSettings: azappcfg.watchedSettings, client: azappcfg.clientManager.staticClient.client, }, - } + } } diff --git a/azureappconfiguration/refresh_test.go b/azureappconfiguration/refresh_test.go index c397f57..a991fcb 100644 --- a/azureappconfiguration/refresh_test.go +++ b/azureappconfiguration/refresh_test.go @@ -6,6 +6,8 @@ package azureappconfiguration import ( "context" "fmt" + "sync" + "sync/atomic" "testing" "time" @@ -57,20 +59,6 @@ func TestRefresh_NotConfigured(t *testing.T) { 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} @@ -400,3 +388,142 @@ func TestRefreshKeyValues_MonitorError(t *testing.T) { assert.Equal(t, 0, mockSentinels.getCallCount, "Sentinels should not be called when monitor fails") assert.False(t, mockTimer.resetCalled, "Timer should not be reset when error occurs") } + +// TestRefresh_AlreadyInProgress tests the new atomic implementation of refresh status checking +func TestRefresh_AlreadyInProgress(t *testing.T) { + // Setup a provider with refresh already in progress + azappcfg := &AzureAppConfiguration{ + kvRefreshTimer: &mockRefreshCondition{}, + } + + // Manually set the refresh in progress flag + azappcfg.refreshInProgress.Store(true) + + // Attempt to refresh + err := azappcfg.Refresh(context.Background()) + + // Verify no error and that we returned early + assert.NoError(t, err) +} + +// TestRefresh_ConcurrentCalls tests calling Refresh concurrently from multiple goroutines +func TestRefresh_ConcurrentCalls(t *testing.T) { + // Skip in short mode as race detector makes it slower + if testing.Short() { + t.Skip("Skipping concurrent refresh test in short mode") + } + + // Setup mock components + mockTimer := refresh.NewTimer(100 * time.Millisecond) + time.Sleep(100 * time.Millisecond) // Ensure timer is set to refresh + mockMonitor := &mockETagsClient{changed: true} + mockLoader := &mockKvRefreshClient{} + mockSentinels := &mockKvRefreshClient{} + + // Track actual refresh operations + refreshCount := int32(0) + + // Create a provider with the components needed for refresh + azappcfg := &AzureAppConfiguration{ + kvRefreshTimer: mockTimer, + watchedSettings: []WatchedSetting{{Key: "test", Label: "test"}}, + sentinelETags: make(map[WatchedSetting]*azcore.ETag), + onRefreshSuccess: []func(){ + func() { + // Count each successful refresh + atomic.AddInt32(&refreshCount, 1) + }, + }, + } + + // Override the newKvRefreshClient method to return our mocks + originalNewMethod := azappcfg.newKvRefreshClient + azappcfg.newKvRefreshClient = func() refreshClient { + return refreshClient{ + loader: mockLoader, + monitor: mockMonitor, + sentinels: mockSentinels, + } + } + defer func() { + // Restore original method after test + if originalNewMethod != nil { + azappcfg.newKvRefreshClient = originalNewMethod + } + }() + + // Number of concurrent goroutines to launch + const concurrentCalls = 10 + + // Use a wait group to ensure all goroutines complete + var wg sync.WaitGroup + wg.Add(concurrentCalls) + + // Launch multiple goroutines to call Refresh concurrently + for i := 0; i < concurrentCalls; i++ { + go func(idx int) { + defer wg.Done() + + // Call Refresh with a small delay between calls to increase chance of concurrency + time.Sleep(time.Millisecond * time.Duration(idx)) + err := azappcfg.Refresh(context.Background()) + + // Each call should succeed without error + assert.NoError(t, err, "Refresh call %d should not return error", idx) + }(i) + } + + // Wait for all goroutines to complete + wg.Wait() + + // Only one refresh operation should actually complete successfully + // Since refreshInProgress prevents multiple refreshes + assert.Equal(t, int32(1), refreshCount, "Only one refresh operation should have executed") +} + +// TestRefresh_SequentialCalls tests multiple sequential calls to Refresh +func TestRefresh_SequentialCalls(t *testing.T) { + // Setup mock components + mockTimer := &mockRefreshCondition{shouldRefresh: true} + mockMonitor := &mockETagsClient{changed: true} + mockLoader := &mockKvRefreshClient{} + mockSentinels := &mockKvRefreshClient{} + + // Track actual refresh operations + refreshCount := int32(0) + + // Create a provider with the components needed for refresh + azappcfg := &AzureAppConfiguration{ + kvRefreshTimer: mockTimer, + watchedSettings: []WatchedSetting{{Key: "test", Label: "test"}}, + sentinelETags: make(map[WatchedSetting]*azcore.ETag), + onRefreshSuccess: []func(){ + func() { + // Count each successful refresh + atomic.AddInt32(&refreshCount, 1) + }, + }, + } + + // Override the newKvRefreshClient method to return our mocks + azappcfg.newKvRefreshClient = func() refreshClient { + return refreshClient{ + loader: mockLoader, + monitor: mockMonitor, + sentinels: mockSentinels, + } + } + + // First call should perform a refresh + err1 := azappcfg.Refresh(context.Background()) + assert.NoError(t, err1) + assert.Equal(t, int32(1), refreshCount) + + // Reset the refreshInProgress flag to simulate completion + azappcfg.refreshInProgress.Store(false) + + // Second call should also perform a refresh + err2 := azappcfg.Refresh(context.Background()) + assert.NoError(t, err2) + assert.Equal(t, int32(2), refreshCount) +} From c733192a623c194ace8251ded03e4318eaa89be4 Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Wed, 23 Apr 2025 13:07:20 +0800 Subject: [PATCH 09/12] update --- .../azureappconfiguration.go | 25 +++++++++++-------- .../internal/tracing/tracing.go | 10 ++++---- .../internal/tracing/tracing_test.go | 8 +++--- azureappconfiguration/settings_client.go | 4 +-- 4 files changed, 26 insertions(+), 21 deletions(-) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index 50ca2aa..752c4fd 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -23,8 +23,8 @@ import ( "sync" "sync/atomic" - "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/tracing" "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/refresh" + "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" decoder "github.com/go-viper/mapstructure/v2" @@ -41,7 +41,7 @@ type AzureAppConfiguration struct { sentinelETags map[WatchedSetting]*azcore.ETag kvRefreshTimer refresh.Condition onRefreshSuccess []func() - tracingOptions tracing.Options + tracingOptions tracing.Options clientManager *configurationClientManager resolver *keyVaultReferenceResolver @@ -98,6 +98,7 @@ func Load(ctx context.Context, authentication AuthenticationOptions, options *Op azappcfg.newKvRefreshClient = azappcfg.newKeyValueRefreshClient } + ctx = context.WithValue(ctx, tracing.TracingKey, tracing.RequestTypeStartUp) if err := azappcfg.load(ctx); err != nil { return nil, err } @@ -205,6 +206,7 @@ func (azappcfg *AzureAppConfiguration) Refresh(ctx context.Context) error { } // Attempt to refresh and check if any values were actually updated + ctx = context.WithValue(ctx, tracing.TracingKey, tracing.RequestTypeWatch) refreshed, err := azappcfg.refreshKeyValues(ctx, azappcfg.newKvRefreshClient()) if err != nil { return fmt.Errorf("failed to refresh configuration: %w", err) @@ -239,8 +241,8 @@ func (azappcfg *AzureAppConfiguration) load(ctx context.Context) error { eg, egCtx := errgroup.WithContext(ctx) eg.Go(func() error { keyValuesClient := &selectorSettingsClient{ - selectors: azappcfg.kvSelectors, - client: azappcfg.clientManager.staticClient.client, + selectors: azappcfg.kvSelectors, + client: azappcfg.clientManager.staticClient.client, tracingOptions: azappcfg.tracingOptions, } return azappcfg.loadKeyValues(egCtx, keyValuesClient) @@ -251,7 +253,7 @@ func (azappcfg *AzureAppConfiguration) load(ctx context.Context) error { watchedClient := &watchedSettingClient{ watchedSettings: azappcfg.watchedSettings, client: azappcfg.clientManager.staticClient.client, - tracingOptions: azappcfg.tracingOptions, + tracingOptions: azappcfg.tracingOptions, } return azappcfg.loadWatchedSettings(egCtx, watchedClient) }) @@ -499,16 +501,19 @@ func normalizedWatchedSettings(s []WatchedSetting) []WatchedSetting { func (azappcfg *AzureAppConfiguration) newKeyValueRefreshClient() refreshClient { return refreshClient{ loader: &selectorSettingsClient{ - selectors: azappcfg.kvSelectors, - client: azappcfg.clientManager.staticClient.client, + selectors: azappcfg.kvSelectors, + client: azappcfg.clientManager.staticClient.client, + tracingOptions: azappcfg.tracingOptions, }, monitor: &watchedSettingClient{ - eTags: azappcfg.sentinelETags, - client: azappcfg.clientManager.staticClient.client, + eTags: azappcfg.sentinelETags, + client: azappcfg.clientManager.staticClient.client, + tracingOptions: azappcfg.tracingOptions, }, sentinels: &watchedSettingClient{ watchedSettings: azappcfg.watchedSettings, client: azappcfg.clientManager.staticClient.client, + tracingOptions: azappcfg.tracingOptions, }, - } + } } diff --git a/azureappconfiguration/internal/tracing/tracing.go b/azureappconfiguration/internal/tracing/tracing.go index 3b56fa5..79e27b8 100644 --- a/azureappconfiguration/internal/tracing/tracing.go +++ b/azureappconfiguration/internal/tracing/tracing.go @@ -11,12 +11,13 @@ import ( ) type RequestType string - +type RequestTracingKey string type HostType string const ( - RequestTypeStartUp RequestType = "StartUp" - RequestTypeWatch RequestType = "Watch" + TracingKey RequestTracingKey = "Tracing" + RequestTypeStartUp RequestType = "StartUp" + RequestTypeWatch RequestType = "Watch" HostTypeAzureFunction HostType = "AzureFunction" HostTypeAzureWebApp HostType = "AzureWebApp" @@ -32,7 +33,6 @@ const ( // Documentation : https://docs.microsoft.com/en-us/azure/service-fabric/service-fabric-environment-variables-reference EnvVarServiceFabric = "Fabric_NodeName" - RequestTracingKey = "Tracing" RequestTypeKey = "RequestType" HostTypeKey = "Host" KeyVaultConfiguredTag = "UsesKeyVault" @@ -75,7 +75,7 @@ func CreateCorrelationContextHeader(ctx context.Context, options Options) http.H header := http.Header{} output := make([]string, 0) - if tracing := ctx.Value(RequestTracingKey); tracing != nil { + if tracing := ctx.Value(TracingKey); tracing != nil { if tracing.(RequestType) == RequestTypeStartUp { output = append(output, RequestTypeKey+"="+string(RequestTypeStartUp)) } else if tracing.(RequestType) == RequestTypeWatch { diff --git a/azureappconfiguration/internal/tracing/tracing_test.go b/azureappconfiguration/internal/tracing/tracing_test.go index 03ac057..389b333 100644 --- a/azureappconfiguration/internal/tracing/tracing_test.go +++ b/azureappconfiguration/internal/tracing/tracing_test.go @@ -24,7 +24,7 @@ func TestCreateCorrelationContextHeader(t *testing.T) { }) t.Run("with RequestTypeStartUp", func(t *testing.T) { - ctx := context.WithValue(context.Background(), RequestTracingKey, RequestTypeStartUp) + ctx := context.WithValue(context.Background(), TracingKey, RequestTypeStartUp) options := Options{} header := CreateCorrelationContextHeader(ctx, options) @@ -35,7 +35,7 @@ func TestCreateCorrelationContextHeader(t *testing.T) { }) t.Run("with RequestTypeWatch", func(t *testing.T) { - ctx := context.WithValue(context.Background(), RequestTracingKey, RequestTypeWatch) + ctx := context.WithValue(context.Background(), TracingKey, RequestTypeWatch) options := Options{} header := CreateCorrelationContextHeader(ctx, options) @@ -132,7 +132,7 @@ func TestCreateCorrelationContextHeader(t *testing.T) { }) t.Run("with all options", func(t *testing.T) { - ctx := context.WithValue(context.Background(), RequestTracingKey, RequestTypeStartUp) + ctx := context.WithValue(context.Background(), TracingKey, RequestTypeStartUp) options := Options{ Host: HostTypeAzureFunction, KeyVaultConfigured: true, @@ -168,7 +168,7 @@ func TestCreateCorrelationContextHeader(t *testing.T) { }) t.Run("delimiter handling", func(t *testing.T) { - ctx := context.WithValue(context.Background(), RequestTracingKey, RequestTypeStartUp) + ctx := context.WithValue(context.Background(), TracingKey, RequestTypeStartUp) options := Options{ Host: HostTypeAzureWebApp, KeyVaultConfigured: true, diff --git a/azureappconfiguration/settings_client.go b/azureappconfiguration/settings_client.go index 1603130..212177b 100644 --- a/azureappconfiguration/settings_client.go +++ b/azureappconfiguration/settings_client.go @@ -9,8 +9,8 @@ import ( "log" "github.com/Azure/AppConfiguration-GoProvider/azureappconfiguration/internal/tracing" - "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" "github.com/Azure/azure-sdk-for-go/sdk/data/azappconfig" ) @@ -30,7 +30,7 @@ type watchedSettingClient struct { watchedSettings []WatchedSetting eTags map[WatchedSetting]*azcore.ETag client *azappconfig.Client - tracingOptions tracing.Options + tracingOptions tracing.Options } type settingsClient interface { From 3b800c8936918552e21edae86bf6016098a8880c Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Wed, 23 Apr 2025 14:14:55 +0800 Subject: [PATCH 10/12] update --- azureappconfiguration/azureappconfiguration.go | 5 +++-- .../internal/tracing/tracing.go | 17 ++++++++--------- .../internal/tracing/tracing_test.go | 18 ++++++++---------- 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index 752c4fd..f08fb21 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -98,9 +98,11 @@ func Load(ctx context.Context, authentication AuthenticationOptions, options *Op azappcfg.newKvRefreshClient = azappcfg.newKeyValueRefreshClient } - ctx = context.WithValue(ctx, tracing.TracingKey, tracing.RequestTypeStartUp) if err := azappcfg.load(ctx); err != nil { return nil, err + } else { + // If the initial load was successful, set the initial load finished flag + azappcfg.tracingOptions.InitialLoadFinished = true } return azappcfg, nil @@ -206,7 +208,6 @@ func (azappcfg *AzureAppConfiguration) Refresh(ctx context.Context) error { } // Attempt to refresh and check if any values were actually updated - ctx = context.WithValue(ctx, tracing.TracingKey, tracing.RequestTypeWatch) refreshed, err := azappcfg.refreshKeyValues(ctx, azappcfg.newKvRefreshClient()) if err != nil { return fmt.Errorf("failed to refresh configuration: %w", err) diff --git a/azureappconfiguration/internal/tracing/tracing.go b/azureappconfiguration/internal/tracing/tracing.go index 79e27b8..95032dd 100644 --- a/azureappconfiguration/internal/tracing/tracing.go +++ b/azureappconfiguration/internal/tracing/tracing.go @@ -15,9 +15,8 @@ type RequestTracingKey string type HostType string const ( - TracingKey RequestTracingKey = "Tracing" - RequestTypeStartUp RequestType = "StartUp" - RequestTypeWatch RequestType = "Watch" + RequestTypeStartUp RequestType = "StartUp" + RequestTypeWatch RequestType = "Watch" HostTypeAzureFunction HostType = "AzureFunction" HostTypeAzureWebApp HostType = "AzureWebApp" @@ -33,6 +32,7 @@ const ( // Documentation : https://docs.microsoft.com/en-us/azure/service-fabric/service-fabric-environment-variables-reference EnvVarServiceFabric = "Fabric_NodeName" + TracingKey = "Tracing" RequestTypeKey = "RequestType" HostTypeKey = "Host" KeyVaultConfiguredTag = "UsesKeyVault" @@ -50,6 +50,7 @@ const ( type Options struct { Enabled bool + InitialLoadFinished bool Host HostType KeyVaultConfigured bool UseAIConfiguration bool @@ -75,12 +76,10 @@ func CreateCorrelationContextHeader(ctx context.Context, options Options) http.H header := http.Header{} output := make([]string, 0) - if tracing := ctx.Value(TracingKey); tracing != nil { - if tracing.(RequestType) == RequestTypeStartUp { - output = append(output, RequestTypeKey+"="+string(RequestTypeStartUp)) - } else if tracing.(RequestType) == RequestTypeWatch { - output = append(output, RequestTypeKey+"="+string(RequestTypeWatch)) - } + if !options.InitialLoadFinished { + output = append(output, RequestTypeKey+"="+string(RequestTypeStartUp)) + } else { + output = append(output, RequestTypeKey+"="+string(RequestTypeWatch)) } if options.Host != "" { diff --git a/azureappconfiguration/internal/tracing/tracing_test.go b/azureappconfiguration/internal/tracing/tracing_test.go index 389b333..86336b0 100644 --- a/azureappconfiguration/internal/tracing/tracing_test.go +++ b/azureappconfiguration/internal/tracing/tracing_test.go @@ -20,14 +20,13 @@ func TestCreateCorrelationContextHeader(t *testing.T) { // The header should be empty but exist corrContext := header.Get(CorrelationContextHeader) - assert.Equal(t, "", corrContext) + assert.Equal(t, "RequestType=StartUp", corrContext) }) t.Run("with RequestTypeStartUp", func(t *testing.T) { - ctx := context.WithValue(context.Background(), TracingKey, RequestTypeStartUp) options := Options{} - header := CreateCorrelationContextHeader(ctx, options) + header := CreateCorrelationContextHeader(context.Background(), options) // Should contain RequestTypeStartUp corrContext := header.Get(CorrelationContextHeader) @@ -35,10 +34,11 @@ func TestCreateCorrelationContextHeader(t *testing.T) { }) t.Run("with RequestTypeWatch", func(t *testing.T) { - ctx := context.WithValue(context.Background(), TracingKey, RequestTypeWatch) - options := Options{} + options := Options{ + InitialLoadFinished: true, + } - header := CreateCorrelationContextHeader(ctx, options) + header := CreateCorrelationContextHeader(context.Background(), options) // Should contain RequestTypeWatch corrContext := header.Get(CorrelationContextHeader) @@ -132,7 +132,6 @@ func TestCreateCorrelationContextHeader(t *testing.T) { }) t.Run("with all options", func(t *testing.T) { - ctx := context.WithValue(context.Background(), TracingKey, RequestTypeStartUp) options := Options{ Host: HostTypeAzureFunction, KeyVaultConfigured: true, @@ -140,7 +139,7 @@ func TestCreateCorrelationContextHeader(t *testing.T) { UseAIChatCompletionConfiguration: true, } - header := CreateCorrelationContextHeader(ctx, options) + header := CreateCorrelationContextHeader(context.Background(), options) // Check the complete header corrContext := header.Get(CorrelationContextHeader) @@ -168,13 +167,12 @@ func TestCreateCorrelationContextHeader(t *testing.T) { }) t.Run("delimiter handling", func(t *testing.T) { - ctx := context.WithValue(context.Background(), TracingKey, RequestTypeStartUp) options := Options{ Host: HostTypeAzureWebApp, KeyVaultConfigured: true, } - header := CreateCorrelationContextHeader(ctx, options) + header := CreateCorrelationContextHeader(context.Background(), options) // Check the complete header corrContext := header.Get(CorrelationContextHeader) From b886cc95d8452312cdc0ff7bbfc1e6d89d0febd7 Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Wed, 23 Apr 2025 15:45:17 +0800 Subject: [PATCH 11/12] update --- .../azureappconfiguration.go | 8 +- azureappconfiguration/refresh_test.go | 124 ------------------ 2 files changed, 3 insertions(+), 129 deletions(-) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index f08fb21..a9056bc 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -43,9 +43,8 @@ type AzureAppConfiguration struct { onRefreshSuccess []func() tracingOptions tracing.Options - clientManager *configurationClientManager - resolver *keyVaultReferenceResolver - newKvRefreshClient func() refreshClient + clientManager *configurationClientManager + resolver *keyVaultReferenceResolver refreshInProgress atomic.Bool } @@ -95,7 +94,6 @@ 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) - azappcfg.newKvRefreshClient = azappcfg.newKeyValueRefreshClient } if err := azappcfg.load(ctx); err != nil { @@ -208,7 +206,7 @@ func (azappcfg *AzureAppConfiguration) Refresh(ctx context.Context) error { } // Attempt to refresh and check if any values were actually updated - refreshed, err := azappcfg.refreshKeyValues(ctx, azappcfg.newKvRefreshClient()) + refreshed, err := azappcfg.refreshKeyValues(ctx, azappcfg.newKeyValueRefreshClient()) if err != nil { return fmt.Errorf("failed to refresh configuration: %w", err) } diff --git a/azureappconfiguration/refresh_test.go b/azureappconfiguration/refresh_test.go index a991fcb..c2ed49d 100644 --- a/azureappconfiguration/refresh_test.go +++ b/azureappconfiguration/refresh_test.go @@ -6,8 +6,6 @@ package azureappconfiguration import ( "context" "fmt" - "sync" - "sync/atomic" "testing" "time" @@ -405,125 +403,3 @@ func TestRefresh_AlreadyInProgress(t *testing.T) { // Verify no error and that we returned early assert.NoError(t, err) } - -// TestRefresh_ConcurrentCalls tests calling Refresh concurrently from multiple goroutines -func TestRefresh_ConcurrentCalls(t *testing.T) { - // Skip in short mode as race detector makes it slower - if testing.Short() { - t.Skip("Skipping concurrent refresh test in short mode") - } - - // Setup mock components - mockTimer := refresh.NewTimer(100 * time.Millisecond) - time.Sleep(100 * time.Millisecond) // Ensure timer is set to refresh - mockMonitor := &mockETagsClient{changed: true} - mockLoader := &mockKvRefreshClient{} - mockSentinels := &mockKvRefreshClient{} - - // Track actual refresh operations - refreshCount := int32(0) - - // Create a provider with the components needed for refresh - azappcfg := &AzureAppConfiguration{ - kvRefreshTimer: mockTimer, - watchedSettings: []WatchedSetting{{Key: "test", Label: "test"}}, - sentinelETags: make(map[WatchedSetting]*azcore.ETag), - onRefreshSuccess: []func(){ - func() { - // Count each successful refresh - atomic.AddInt32(&refreshCount, 1) - }, - }, - } - - // Override the newKvRefreshClient method to return our mocks - originalNewMethod := azappcfg.newKvRefreshClient - azappcfg.newKvRefreshClient = func() refreshClient { - return refreshClient{ - loader: mockLoader, - monitor: mockMonitor, - sentinels: mockSentinels, - } - } - defer func() { - // Restore original method after test - if originalNewMethod != nil { - azappcfg.newKvRefreshClient = originalNewMethod - } - }() - - // Number of concurrent goroutines to launch - const concurrentCalls = 10 - - // Use a wait group to ensure all goroutines complete - var wg sync.WaitGroup - wg.Add(concurrentCalls) - - // Launch multiple goroutines to call Refresh concurrently - for i := 0; i < concurrentCalls; i++ { - go func(idx int) { - defer wg.Done() - - // Call Refresh with a small delay between calls to increase chance of concurrency - time.Sleep(time.Millisecond * time.Duration(idx)) - err := azappcfg.Refresh(context.Background()) - - // Each call should succeed without error - assert.NoError(t, err, "Refresh call %d should not return error", idx) - }(i) - } - - // Wait for all goroutines to complete - wg.Wait() - - // Only one refresh operation should actually complete successfully - // Since refreshInProgress prevents multiple refreshes - assert.Equal(t, int32(1), refreshCount, "Only one refresh operation should have executed") -} - -// TestRefresh_SequentialCalls tests multiple sequential calls to Refresh -func TestRefresh_SequentialCalls(t *testing.T) { - // Setup mock components - mockTimer := &mockRefreshCondition{shouldRefresh: true} - mockMonitor := &mockETagsClient{changed: true} - mockLoader := &mockKvRefreshClient{} - mockSentinels := &mockKvRefreshClient{} - - // Track actual refresh operations - refreshCount := int32(0) - - // Create a provider with the components needed for refresh - azappcfg := &AzureAppConfiguration{ - kvRefreshTimer: mockTimer, - watchedSettings: []WatchedSetting{{Key: "test", Label: "test"}}, - sentinelETags: make(map[WatchedSetting]*azcore.ETag), - onRefreshSuccess: []func(){ - func() { - // Count each successful refresh - atomic.AddInt32(&refreshCount, 1) - }, - }, - } - - // Override the newKvRefreshClient method to return our mocks - azappcfg.newKvRefreshClient = func() refreshClient { - return refreshClient{ - loader: mockLoader, - monitor: mockMonitor, - sentinels: mockSentinels, - } - } - - // First call should perform a refresh - err1 := azappcfg.Refresh(context.Background()) - assert.NoError(t, err1) - assert.Equal(t, int32(1), refreshCount) - - // Reset the refreshInProgress flag to simulate completion - azappcfg.refreshInProgress.Store(false) - - // Second call should also perform a refresh - err2 := azappcfg.Refresh(context.Background()) - assert.NoError(t, err2) - assert.Equal(t, int32(2), refreshCount) -} From 6e83dbd4130eb3648e81f2a5ea413cdc95239e54 Mon Sep 17 00:00:00 2001 From: "Lingling Ye (from Dev Box)" Date: Wed, 23 Apr 2025 16:17:58 +0800 Subject: [PATCH 12/12] update --- azureappconfiguration/azureappconfiguration.go | 5 ++--- azureappconfiguration/internal/tracing/tracing.go | 1 - 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/azureappconfiguration/azureappconfiguration.go b/azureappconfiguration/azureappconfiguration.go index a9056bc..a25d941 100644 --- a/azureappconfiguration/azureappconfiguration.go +++ b/azureappconfiguration/azureappconfiguration.go @@ -98,10 +98,9 @@ func Load(ctx context.Context, authentication AuthenticationOptions, options *Op if err := azappcfg.load(ctx); err != nil { return nil, err - } else { - // If the initial load was successful, set the initial load finished flag - azappcfg.tracingOptions.InitialLoadFinished = true } + // Set the initial load finished flag + azappcfg.tracingOptions.InitialLoadFinished = true return azappcfg, nil } diff --git a/azureappconfiguration/internal/tracing/tracing.go b/azureappconfiguration/internal/tracing/tracing.go index 95032dd..fdecf93 100644 --- a/azureappconfiguration/internal/tracing/tracing.go +++ b/azureappconfiguration/internal/tracing/tracing.go @@ -32,7 +32,6 @@ const ( // Documentation : https://docs.microsoft.com/en-us/azure/service-fabric/service-fabric-environment-variables-reference EnvVarServiceFabric = "Fabric_NodeName" - TracingKey = "Tracing" RequestTypeKey = "RequestType" HostTypeKey = "Host" KeyVaultConfiguredTag = "UsesKeyVault"