From 11d939a646ad697b836c337d508fdd9b3511f7b5 Mon Sep 17 00:00:00 2001 From: Krishna Chaitanya Date: Fri, 23 Oct 2026 10:53:45 +0530 Subject: [PATCH 1/2] fix: bound caching of empty subscription discovery results Cache successful empty subscription lists for at most one minute while preserving shorter positive TTLs and normal non-empty caching. Avoid the cached-list wrapper overwriting the capped expiration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- azuresdk/README.md | 9 + azuresdk/armclient/client.subscriptions.go | 31 ++-- .../armclient/client.subscriptions_test.go | 165 ++++++++++++++++++ 3 files changed, 194 insertions(+), 11 deletions(-) create mode 100644 azuresdk/armclient/client.subscriptions_test.go diff --git a/azuresdk/README.md b/azuresdk/README.md index 1ea22f4..ded2901 100644 --- a/azuresdk/README.md +++ b/azuresdk/README.md @@ -13,6 +13,15 @@ | `AZURE_SERVICEDISCOVERY_SUBSCRIPTION_INCLUDE` | | List of subscription IDs to include, separated by spaces, commas or semicolons | | `AZURE_SERVICEDISCOVERY_SUBSCRIPTION_EXCLUDE` | | List of subscription IDs to exclude, separated by spaces, commas or semicolons | +### Subscription discovery caching + +Successful empty subscription discovery results are cached for at most one minute, +or the configured `AZURE_SERVICEDISCOVERY_CACHE_TTL` when it is positive and shorter. +This allows discovery to recover from transient empty results (for example, during +role-assignment propagation) without querying ARM on every discovery call. +Non-empty subscription results retain the configured cache TTL. Discovery errors +are returned to the caller and are not cached. + ### Authentication Hint: please also check [microsoft azure-sdk documentation](https://docs.microsoft.com/en-us/azure/developer/go/azure-sdk-authentication) for advanced usage. diff --git a/azuresdk/armclient/client.subscriptions.go b/azuresdk/armclient/client.subscriptions.go index 78c60ef..6e7ff61 100644 --- a/azuresdk/armclient/client.subscriptions.go +++ b/azuresdk/armclient/client.subscriptions.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "strings" + "time" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions" "k8s.io/apimachinery/pkg/labels" @@ -13,6 +14,7 @@ import ( const ( CacheIdentifierSubscriptions = "subscriptions" + emptySubscriptionsCacheTtl = time.Minute ) // ListCachedSubscriptionsWithFilter return list of subscription with filter by subscription ids @@ -41,20 +43,18 @@ func (azureClient *ArmClient) ListCachedSubscriptionsWithFilter(ctx context.Cont // ListCachedSubscriptions return cached list of Azure Subscriptions as map (key is subscription id) func (azureClient *ArmClient) ListCachedSubscriptions(ctx context.Context) (map[string]*armsubscriptions.Subscription, error) { - result, err := azureClient.cacheData(CacheIdentifierSubscriptions, func() (interface{}, error) { - azureClient.logger.Debug("updating cached Azure Subscription list") - list, err := azureClient.ListSubscriptions(ctx) - if err != nil { - return nil, err - } - azureClient.logger.Debug(fmt.Sprintf("found %v Azure Subscriptions", len(list))) - return list, nil - }) + if result, ok := azureClient.cache.Get(CacheIdentifierSubscriptions); ok { + return result.(map[string]*armsubscriptions.Subscription), nil + } + + azureClient.logger.Debug("updating cached Azure Subscription list") + list, err := azureClient.ListSubscriptions(ctx) if err != nil { return nil, err } + azureClient.logger.Debug(fmt.Sprintf("found %v Azure Subscriptions", len(list))) - return result.(map[string]*armsubscriptions.Subscription), nil + return list, nil } // ListSubscriptions return list of Azure Subscriptions as map (key is subscription id) @@ -118,11 +118,20 @@ func (azureClient *ArmClient) ListSubscriptions(ctx context.Context) (map[string } // update cache - azureClient.cache.SetDefault(CacheIdentifierSubscriptions, list) + azureClient.cacheSubscriptions(list) return list, nil } +func (azureClient *ArmClient) cacheSubscriptions(list map[string]*armsubscriptions.Subscription) { + ttl := azureClient.cacheTtl + // Empty discovery can be transient; retry sooner without polling ARM on every call. + if len(list) == 0 && (ttl <= 0 || ttl > emptySubscriptionsCacheTtl) { + ttl = emptySubscriptionsCacheTtl + } + azureClient.cache.Set(CacheIdentifierSubscriptions, list, ttl) +} + // GetCachedSubscription returns a cached subscription func (azureClient *ArmClient) GetCachedSubscription(ctx context.Context, subscriptionID string) (*armsubscriptions.Subscription, error) { list, err := azureClient.ListCachedSubscriptions(ctx) diff --git a/azuresdk/armclient/client.subscriptions_test.go b/azuresdk/armclient/client.subscriptions_test.go new file mode 100644 index 0000000..e943ce8 --- /dev/null +++ b/azuresdk/armclient/client.subscriptions_test.go @@ -0,0 +1,165 @@ +package armclient + +import ( + "context" + "errors" + "io" + "log/slog" + "testing" + "testing/synctest" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions" + cache "github.com/patrickmn/go-cache" + + "github.com/webdevops/go-common/azuresdk/cloudconfig" + "github.com/webdevops/go-common/utils/to" +) + +func TestSubscriptionCacheExpiration(t *testing.T) { + tests := []struct { + name string + ttl time.Duration + empty bool + expiration time.Duration + }{ + {name: "empty default", ttl: time.Hour, empty: true, expiration: time.Minute}, + {name: "empty shorter", ttl: 10 * time.Second, empty: true, expiration: 10 * time.Second}, + {name: "empty equal", ttl: time.Minute, empty: true, expiration: time.Minute}, + {name: "empty zero", ttl: 0, empty: true, expiration: time.Minute}, + {name: "empty no expiration", ttl: cache.NoExpiration, empty: true, expiration: time.Minute}, + {name: "nonempty default", ttl: time.Hour, expiration: time.Hour}, + {name: "nonempty shorter", ttl: 10 * time.Second, expiration: 10 * time.Second}, + {name: "nonempty zero", ttl: 0}, + {name: "nonempty no expiration", ttl: cache.NoExpiration}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + client := newSubscriptionCacheTestClient(tt.ttl, nil) + list := map[string]*armsubscriptions.Subscription{} + if !tt.empty { + list["subscription-1"] = &armsubscriptions.Subscription{ + SubscriptionID: to.Ptr("subscription-1"), + } + } + + client.cacheSubscriptions(list) + _, expiration, ok := client.cache.GetWithExpiration(CacheIdentifierSubscriptions) + if !ok { + t.Fatal("successful discovery was not cached") + } + if tt.expiration == 0 { + if !expiration.IsZero() { + t.Fatalf("expected no expiration, got %v", expiration) + } + return + } + if got := time.Until(expiration); got != tt.expiration { + t.Fatalf("expected cache lifetime %v, got %v", tt.expiration, got) + } + + time.Sleep(tt.expiration - time.Nanosecond) + if _, ok := client.cache.Get(CacheIdentifierSubscriptions); !ok { + t.Fatal("subscription cache expired too soon") + } + time.Sleep(2 * time.Nanosecond) + if _, ok := client.cache.Get(CacheIdentifierSubscriptions); ok { + t.Fatal("subscription cache did not expire") + } + }) + }) + } +} + +func TestListCachedSubscriptionsRetriesAfterEmptyExpiration(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + authErr := errors.New("test credential unavailable") + cred := &subscriptionTestCredential{err: authErr} + client := newSubscriptionCacheTestClient(time.Hour, cred) + // Seed the same cache entry as a successful empty ListSubscriptions during Connect. + client.cacheSubscriptions(map[string]*armsubscriptions.Subscription{}) + + for range 3 { + list, err := client.ListCachedSubscriptions(context.Background()) + if err != nil || len(list) != 0 { + t.Fatalf("expected cached empty success, got %v, %v", list, err) + } + time.Sleep(15 * time.Second) + } + if cred.calls != 0 { + t.Fatalf("cached empty results triggered %d credential calls", cred.calls) + } + + time.Sleep(15*time.Second + time.Nanosecond) + for range 2 { + list, err := client.ListCachedSubscriptions(context.Background()) + if list != nil || !errors.Is(err, authErr) { + t.Fatalf("expected discovery retry to surface credential error, got %v, %v", list, err) + } + } + if cred.calls != 2 { + t.Fatalf("expected two uncached discovery retries, got %d", cred.calls) + } + if _, ok := client.cache.Get(CacheIdentifierSubscriptions); ok { + t.Fatal("failed discovery was cached") + } + + // Once discovery succeeds, the normal positive-result cache lifetime applies. + client.cacheSubscriptions(map[string]*armsubscriptions.Subscription{ + "subscription-1": {SubscriptionID: to.Ptr("subscription-1")}, + }) + time.Sleep(2 * time.Minute) + subscription, err := client.GetCachedSubscription(context.Background(), "subscription-1") + if err != nil || subscription == nil || *subscription.SubscriptionID != "subscription-1" { + t.Fatalf("expected recovered cached subscription, got %v, %v", subscription, err) + } + if cred.calls != 2 { + t.Fatal("non-empty results were not retained for the configured TTL") + } + }) +} + +func TestListSubscriptionsErrorPreservesCache(t *testing.T) { + authErr := errors.New("test credential unavailable") + client := newSubscriptionCacheTestClient(time.Hour, &subscriptionTestCredential{err: authErr}) + client.cacheSubscriptions(map[string]*armsubscriptions.Subscription{ + "subscription-1": {SubscriptionID: to.Ptr("subscription-1")}, + }) + + list, err := client.ListSubscriptions(context.Background()) + if list != nil || !errors.Is(err, authErr) { + t.Fatalf("expected discovery error, got %v, %v", list, err) + } + subscription, err := client.GetCachedSubscription(context.Background(), "subscription-1") + if err != nil || subscription == nil { + t.Fatalf("failed refresh replaced successful cache entry: %v", err) + } +} + +func newSubscriptionCacheTestClient(ttl time.Duration, credential *subscriptionTestCredential) *ArmClient { + var cred azcore.TokenCredential = credential + return &ArmClient{ + cloud: cloudconfig.CloudEnvironment{ + Configuration: cloud.AzurePublic, + Name: cloudconfig.AzurePublicCloud, + }, + logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + cache: cache.New(ttl, 0), + cacheTtl: ttl, + cred: &cred, + } +} + +type subscriptionTestCredential struct { + err error + calls int +} + +func (cred *subscriptionTestCredential) GetToken(context.Context, policy.TokenRequestOptions) (azcore.AccessToken, error) { + cred.calls++ + return azcore.AccessToken{}, cred.err +} From 03bc24de41346660c19e77dba3639b75b6b6a0e7 Mon Sep 17 00:00:00 2001 From: Krishna Chaitanya Date: Fri, 23 Oct 2026 11:34:23 +0530 Subject: [PATCH 2/2] fix: refresh filtered subscription iterators through discovery cache Retain a defensive copy of configured subscription IDs instead of a permanent discovery snapshot. Preserve eager discovery and error timing, and refresh filtered results through the existing service-discovery cache. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- azuresdk/README.md | 6 + azuresdk/armclient/iterator.subscriptions.go | 22 +- .../armclient/iterator.subscriptions_test.go | 246 ++++++++++++++++++ 3 files changed, 257 insertions(+), 17 deletions(-) create mode 100644 azuresdk/armclient/iterator.subscriptions_test.go diff --git a/azuresdk/README.md b/azuresdk/README.md index ded2901..afd0357 100644 --- a/azuresdk/README.md +++ b/azuresdk/README.md @@ -22,6 +22,12 @@ role-assignment propagation) without querying ARM on every discovery call. Non-empty subscription results retain the configured cache TTL. Discovery errors are returned to the caller and are not cached. +Subscription iterators retain configured subscription IDs as a filter, not a +snapshot of discovery results. Each iteration uses the service-discovery cache, +so newly visible subscriptions can be discovered after cache expiration while +the configured filter remains in effect. Setting the filter still performs eager +discovery and retains the existing panic-on-error behavior. + ### Authentication Hint: please also check [microsoft azure-sdk documentation](https://docs.microsoft.com/en-us/azure/developer/go/azure-sdk-authentication) for advanced usage. diff --git a/azuresdk/armclient/iterator.subscriptions.go b/azuresdk/armclient/iterator.subscriptions.go index 0b53816..3aa3f10 100644 --- a/azuresdk/armclient/iterator.subscriptions.go +++ b/azuresdk/armclient/iterator.subscriptions.go @@ -14,8 +14,8 @@ import ( type ( SubscriptionsIterator struct { - client *ArmClient - subscriptions *map[string]*armsubscriptions.Subscription + client *ArmClient + subscriptionIDs []string concurrency int } @@ -35,12 +35,12 @@ func NewSubscriptionIterator(client *ArmClient, subscriptionID ...string) *Subsc // SetSubscriptions Set subscription id filter func (i *SubscriptionsIterator) SetSubscriptions(subscriptionID ...string) *SubscriptionsIterator { ctx := context.Background() - list, err := i.client.ListCachedSubscriptionsWithFilter(ctx, subscriptionID...) + _, err := i.client.ListCachedSubscriptionsWithFilter(ctx, subscriptionID...) if err != nil { panic(err.Error()) } - i.subscriptions = &list + i.subscriptionIDs = append([]string(nil), subscriptionID...) return i } @@ -126,17 +126,5 @@ func (i *SubscriptionsIterator) ForEachAsync(logger *slog.Logger, callback func( // ListSubscriptions Returns list of subscriptions for looping func (i *SubscriptionsIterator) ListSubscriptions() (map[string]*armsubscriptions.Subscription, error) { - var list map[string]*armsubscriptions.Subscription - - if i.subscriptions != nil { - list = *i.subscriptions - } else { - if result, err := i.client.ListCachedSubscriptions(context.Background()); err == nil { - list = result - } else { - return list, err - } - } - - return list, nil + return i.client.ListCachedSubscriptionsWithFilter(context.Background(), i.subscriptionIDs...) } diff --git a/azuresdk/armclient/iterator.subscriptions_test.go b/azuresdk/armclient/iterator.subscriptions_test.go new file mode 100644 index 0000000..ce8b2b0 --- /dev/null +++ b/azuresdk/armclient/iterator.subscriptions_test.go @@ -0,0 +1,246 @@ +package armclient + +import ( + "context" + "encoding/json" + "errors" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "sync" + "testing" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud" + cache "github.com/patrickmn/go-cache" + + "github.com/webdevops/go-common/azuresdk/cloudconfig" +) + +const ( + iteratorTestSubscriptionID = "00000000-0000-0000-0000-000000000001" + iteratorTestOtherSubscriptionID = "00000000-0000-0000-0000-000000000002" +) + +func TestSubscriptionIteratorRecoversAfterEmptyDiscovery(t *testing.T) { + for _, preload := range []bool{false, true} { + name := "constructor" + if preload { + name = "direct discovery before constructor" + } + t.Run(name, func(t *testing.T) { + client, mock := newIteratorHTTPTestClient(t) + if preload { + if _, err := client.ListSubscriptions(context.Background()); err != nil { + t.Fatal(err) + } + } + iterator := NewSubscriptionIterator(client, iteratorTestSubscriptionID) + for range 3 { + assertIteratorSubscriptions(t, iterator) + } + assertIteratorRequests(t, mock, 1) + + mock.setSubscriptions(iteratorTestSubscriptionID, iteratorTestOtherSubscriptionID) + waitForIteratorCacheExpiration(t, client) + assertIteratorSubscriptions(t, iterator, iteratorTestSubscriptionID) + assertIteratorRequests(t, mock, 2) + assertIteratorSubscriptions(t, iterator, iteratorTestSubscriptionID) + assertIteratorRequests(t, mock, 2) + }) + } +} + +func TestSubscriptionIteratorFiltersAndCopiesIDs(t *testing.T) { + client, mock := newIteratorHTTPTestClient(t) + mock.setSubscriptions(iteratorTestSubscriptionID, iteratorTestOtherSubscriptionID) + ids := []string{iteratorTestSubscriptionID} + iterator := NewSubscriptionIterator(client, ids...) + ids[0] = iteratorTestOtherSubscriptionID + assertIteratorSubscriptions(t, iterator, iteratorTestSubscriptionID) + + replacement := []string{iteratorTestOtherSubscriptionID} + if iterator.SetSubscriptions(replacement...) != iterator { + t.Fatal("SetSubscriptions did not return the iterator") + } + replacement[0] = iteratorTestSubscriptionID + assertIteratorSubscriptions(t, iterator, iteratorTestOtherSubscriptionID) + + iterator.SetSubscriptions() + assertIteratorSubscriptions(t, iterator, iteratorTestSubscriptionID, iteratorTestOtherSubscriptionID) + assertIteratorRequests(t, mock, 1) +} + +func TestSubscriptionIteratorNormalStartup(t *testing.T) { + client, mock := newIteratorHTTPTestClient(t) + mock.setSubscriptions(iteratorTestSubscriptionID, iteratorTestOtherSubscriptionID) + iterator := NewSubscriptionIterator(client, iteratorTestSubscriptionID) + assertIteratorRequests(t, mock, 1) + assertIteratorSubscriptions(t, iterator, iteratorTestSubscriptionID) + assertIteratorRequests(t, mock, 1) + + unfiltered := NewSubscriptionIterator(client) + assertIteratorSubscriptions(t, unfiltered, iteratorTestSubscriptionID, iteratorTestOtherSubscriptionID) + assertIteratorRequests(t, mock, 1) +} + +func TestSubscriptionIteratorSurfacesRefreshErrors(t *testing.T) { + client, mock := newIteratorHTTPTestClient(t) + mock.setSubscriptions(iteratorTestSubscriptionID) + iterator := NewSubscriptionIterator(client, iteratorTestSubscriptionID) + assertIteratorSubscriptions(t, iterator, iteratorTestSubscriptionID) + waitForIteratorCacheExpiration(t, client) + mock.setForbidden() + + for range 2 { + list, err := iterator.ListSubscriptions() + var responseErr *azcore.ResponseError + if list != nil || !errors.As(err, &responseErr) || + responseErr.StatusCode != http.StatusForbidden || responseErr.ErrorCode != "AuthorizationFailed" { + t.Fatalf("expected surfaced ARM authorization error, got list=%v err=%v", list, err) + } + } + assertIteratorRequests(t, mock, 3) + + mock.setSubscriptions(iteratorTestSubscriptionID, iteratorTestOtherSubscriptionID) + assertIteratorSubscriptions(t, iterator, iteratorTestSubscriptionID) + assertIteratorRequests(t, mock, 4) +} + +func TestSubscriptionIteratorPreservesEagerDiscoveryErrors(t *testing.T) { + for _, constructor := range []bool{false, true} { + name := "setter" + if constructor { + name = "constructor" + } + t.Run(name, func(t *testing.T) { + client, mock := newIteratorHTTPTestClient(t) + mock.setForbidden() + defer func() { + if recovered := recover(); recovered == nil { + t.Error("expected existing eager-discovery panic") + } + assertIteratorRequests(t, mock, 1) + }() + if constructor { + NewSubscriptionIterator(client, iteratorTestSubscriptionID) + } else { + NewSubscriptionIterator(client).SetSubscriptions(iteratorTestSubscriptionID) + } + }) + } +} + +type iteratorHTTPMock struct { + mu sync.Mutex + ids []string + status int + requests int +} + +func (mock *iteratorHTTPMock) setSubscriptions(ids ...string) { + mock.mu.Lock() + defer mock.mu.Unlock() + mock.ids = append([]string(nil), ids...) + mock.status = http.StatusOK +} + +func (mock *iteratorHTTPMock) setForbidden() { + mock.mu.Lock() + defer mock.mu.Unlock() + mock.status = http.StatusForbidden +} + +func (mock *iteratorHTTPMock) ServeHTTP(w http.ResponseWriter, r *http.Request) { + mock.mu.Lock() + defer mock.mu.Unlock() + mock.requests++ + w.Header().Set("Content-Type", "application/json") + if r.Method != http.MethodGet || r.URL.Path != "/subscriptions" || + r.URL.Query().Get("api-version") != "2022-12-01" { + http.Error(w, "unexpected ARM request", http.StatusBadRequest) + return + } + if mock.status == http.StatusForbidden { + w.WriteHeader(http.StatusForbidden) + _, _ = io.WriteString(w, `{"error":{"code":"AuthorizationFailed","message":"mock authorization failure"}}`) + return + } + value := make([]map[string]string, 0, len(mock.ids)) + for _, id := range mock.ids { + value = append(value, map[string]string{ + "subscriptionId": id, + "displayName": "mock subscription", + "state": "Enabled", + }) + } + if err := json.NewEncoder(w).Encode(struct { + Value []map[string]string `json:"value"` + }{Value: value}); err != nil { + panic(err) + } +} + +func newIteratorHTTPTestClient(t *testing.T) (*ArmClient, *iteratorHTTPMock) { + t.Helper() + mock := &iteratorHTTPMock{} + server := httptest.NewServer(mock) + t.Cleanup(server.Close) + // A nil credential permits local HTTP without changing TLS settings or contacting an identity service. + var credential azcore.TokenCredential + ttl := 100 * time.Millisecond + client := &ArmClient{ + cloud: cloudconfig.CloudEnvironment{ + Configuration: cloud.Configuration{ + Services: map[cloud.ServiceName]cloud.ServiceConfiguration{ + cloud.ResourceManager: {Endpoint: server.URL, Audience: server.URL}, + }, + }, + }, + logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + cache: cache.New(ttl, 0), + cacheTtl: ttl, + cred: &credential, + } + return client, mock +} + +func waitForIteratorCacheExpiration(t *testing.T, client *ArmClient) { + t.Helper() + _, expiration, ok := client.cache.GetWithExpiration(CacheIdentifierSubscriptions) + if !ok { + return + } + if expiration.IsZero() { + t.Fatal("expected a finite subscription cache expiration") + } + time.Sleep(time.Until(expiration) + time.Millisecond) +} + +func assertIteratorSubscriptions(t *testing.T, iterator *SubscriptionsIterator, ids ...string) { + t.Helper() + list, err := iterator.ListSubscriptions() + if err != nil { + t.Fatalf("list iterator subscriptions: %v", err) + } + if len(list) != len(ids) { + t.Fatalf("expected subscriptions %v, got %v", ids, list) + } + for _, id := range ids { + subscription, ok := list[id] + if !ok || subscription == nil || subscription.SubscriptionID == nil || *subscription.SubscriptionID != id { + t.Fatalf("expected subscription %s, got %v", id, subscription) + } + } +} + +func assertIteratorRequests(t *testing.T, mock *iteratorHTTPMock, expected int) { + t.Helper() + mock.mu.Lock() + defer mock.mu.Unlock() + if mock.requests != expected { + t.Fatalf("expected %d ARM requests, got %d", expected, mock.requests) + } +}