fix: recover subscription discovery after transient empty results

azuresdk/README.md

@@ -13,6 +13,21 @@
 | `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.
+
+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.

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)

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
+}

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...)
 }