From 84df191f4ddb9e0007d83a4ae2db605c496af21f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 14:18:10 +0200 Subject: [PATCH 01/11] feat(azure): org-wide multi-subscription recommendation collection Cache the ARM subscriptions list on AzureProvider (getOrFetchAccounts, double-checked locking, InvalidateAccountsCache for tests) so repeated GetAccounts/GetServiceClient/GetRecommendationsClient calls in one run do not each re-issue the subscriptions.List API call. Add MultiSubscriptionRecommendationsClient, which fans recommendation collection out across every subscription visible to the authenticated principal via errgroup, isolating per-subscription errors and only failing when every subscription fails. GetRecommendationsClient now returns this fan-out client when no subscription is pinned and 2+ are discovered, bringing Azure to parity with the AWS provider's automatic whole-organization recommendation coverage. Pinned-subscription and single-subscription paths are unchanged. Closes #553 --- providers/azure/provider.go | 123 +++++++++-- providers/azure/provider_test.go | 145 +++++++++++++ .../recommendations_multi_subscription.go | 166 +++++++++++++++ ...recommendations_multi_subscription_test.go | 194 ++++++++++++++++++ 4 files changed, 615 insertions(+), 13 deletions(-) create mode 100644 providers/azure/recommendations_multi_subscription.go create mode 100644 providers/azure/recommendations_multi_subscription_test.go diff --git a/providers/azure/provider.go b/providers/azure/provider.go index ed0cb90b1..f93cbf384 100644 --- a/providers/azure/provider.go +++ b/providers/azure/provider.go @@ -96,6 +96,17 @@ type AzureProvider struct { region string // Default region for operations subscriptionsClient SubscriptionsClient credProvider CredentialProvider + + // accountsMu guards cachedAccounts. GetAccounts, GetServiceClient, and + // GetRecommendationsClient all resolve the subscription list on the hot + // path; without caching, a single logical operation (e.g. a + // multi-subscription recommendations sweep) would re-issue the ARM + // subscriptions.List call once per internal caller. cachedAccounts is + // nil until the first successful fetch; InvalidateAccountsCache resets + // it so tests (and long-lived callers that expect subscription + // membership to change) can force a refresh. + accountsMu sync.RWMutex + cachedAccounts []common.Account } // NewAzureProvider creates a new Azure provider instance. @@ -253,7 +264,71 @@ func (p *AzureProvider) GetAccounts(ctx context.Context) ([]common.Account, erro if !p.IsConfigured() { return nil, fmt.Errorf("azure provider is not configured") } + return p.getOrFetchAccounts(ctx) +} +// getOrFetchAccounts returns the cached subscription list, populating it via +// fetchAccountsLocked on first use. Safe for concurrent callers: a read lock +// guards the fast path (cache already populated); a write lock guards the +// fetch-and-populate path, with a re-check after acquiring it so concurrent +// callers that lost the race to the lock don't issue a redundant ARM call. +// +// Callers must have already verified IsConfigured(); this method assumes a +// usable credential is present (mirrors GetAccounts, its only production +// caller alongside GetServiceClient/GetRecommendationsClient which check +// IsConfigured() themselves before resolving accounts). +func (p *AzureProvider) getOrFetchAccounts(ctx context.Context) ([]common.Account, error) { + p.accountsMu.RLock() + cached := p.cachedAccounts + p.accountsMu.RUnlock() + if cached != nil { + return cloneAccounts(cached), nil + } + + p.accountsMu.Lock() + defer p.accountsMu.Unlock() + // Re-check: another goroutine may have populated the cache while this + // one was waiting on the write lock. + if p.cachedAccounts != nil { + return cloneAccounts(p.cachedAccounts), nil + } + + accounts, err := p.fetchAccountsLocked(ctx) + if err != nil { + return nil, err + } + p.cachedAccounts = accounts + return cloneAccounts(accounts), nil +} + +// cloneAccounts returns a shallow copy of accounts backed by a fresh array. +// common.Account has no nested slices/maps, so a shallow per-element copy is +// sufficient to stop a caller mutating a returned slice (e.g. flipping +// IsDefault) from corrupting the shared cache -- the same class of bug +// flagged for getters returning nested state. +func cloneAccounts(accounts []common.Account) []common.Account { + out := make([]common.Account, len(accounts)) + copy(out, accounts) + return out +} + +// InvalidateAccountsCache clears the cached subscription list so the next +// getOrFetchAccounts call re-fetches from the ARM subscriptions API. Exposed +// for tests that need to assert cache-miss behavior; production callers +// currently rely on the cache living for the lifetime of the AzureProvider +// instance (one instance is constructed per collection/purchase run). +func (p *AzureProvider) InvalidateAccountsCache() { + p.accountsMu.Lock() + defer p.accountsMu.Unlock() + p.cachedAccounts = nil +} + +// fetchAccountsLocked performs the actual ARM subscriptions.List call and +// resolves the default subscription. Must only be called while holding +// accountsMu for writing (via getOrFetchAccounts) -- it does not lock itself +// so getOrFetchAccounts can do its cache-populate-and-return in one critical +// section. +func (p *AzureProvider) fetchAccountsLocked(ctx context.Context) ([]common.Account, error) { // Use injected client if available (for testing) var subClient SubscriptionsClient if p.subscriptionsClient != nil { @@ -501,27 +576,49 @@ func (p *AzureProvider) newServiceClientForSubscription(service common.ServiceTy } } -// GetRecommendationsClient returns a recommendations client for the default -// subscription. +// GetRecommendationsClient returns a recommendations client. // -// When operating across multiple subscriptions (fan-out), prefer -// GetRecommendationsClientForAccount. +// When a subscription is pinned (p.subscriptionID set, e.g. by the scheduler +// or purchase-execution paths that always operate on one registered +// account), the returned client is scoped to that single subscription -- +// unchanged from previous behavior. +// +// When no subscription is pinned, GetRecommendationsClient discovers every +// subscription accessible to the authenticated principal (via the cached +// getOrFetchAccounts) and, when 2+ are visible, fans recommendation +// collection out across all of them via +// MultiSubscriptionRecommendationsClient. Azure has no organization-wide +// equivalent of AWS Cost Explorer's AccountScope=Linked -- the Consumption +// Reservation Recommendations and Advisor APIs are subscription-scoped -- so +// this client-side fan-out is what brings Azure to parity with the AWS +// provider's automatic whole-organization coverage. A single discovered +// subscription still returns the plain single-subscription client; no +// fan-out machinery is needed for one subscription. func (p *AzureProvider) GetRecommendationsClient(ctx context.Context) (provider.RecommendationsClient, error) { if !p.IsConfigured() { return nil, fmt.Errorf("azure provider is not configured") } - // Use explicit subscription ID if configured; otherwise resolve from accounts. - subscriptionID := p.subscriptionID - if subscriptionID == "" { - var err error - subscriptionID, err = p.resolveSubscriptionIDFromCtx(ctx) - if err != nil { - return nil, err - } + if p.subscriptionID != "" { + return NewRecommendationsClient(p.cred, p.subscriptionID) } - return NewRecommendationsClient(p.cred, subscriptionID) + accounts, err := p.getOrFetchAccounts(ctx) + if err != nil { + return nil, fmt.Errorf("failed to resolve Azure subscriptions: %w", err) + } + if len(accounts) == 0 { + return nil, fmt.Errorf("no Azure subscriptions found") + } + if len(accounts) == 1 { + return NewRecommendationsClient(p.cred, accounts[0].ID) + } + + client, err := NewMultiSubscriptionRecommendationsClient(p.cred, accounts) + if err != nil { + return nil, err + } + return client, nil } // GetRecommendationsClientForAccount returns a recommendations client scoped to diff --git a/providers/azure/provider_test.go b/providers/azure/provider_test.go index 54f2b1ac3..fdd128955 100644 --- a/providers/azure/provider_test.go +++ b/providers/azure/provider_test.go @@ -1241,3 +1241,148 @@ func TestAzureProvider_GetRecommendationsClientForAccount(t *testing.T) { assert.Contains(t, err.Error(), "azure provider is not configured") }) } + +// countingSubscriptionsClient wraps mockSubscriptionsClient and counts how +// many times NewListPager is invoked, so cache-hit tests can assert the +// underlying ARM API is only called once. +type countingSubscriptionsClient struct { + *mockSubscriptionsClient + calls int +} + +func (c *countingSubscriptionsClient) NewListPager(options *armsubscriptions.ClientListOptions) SubscriptionsPager { + c.calls++ + return c.mockSubscriptionsClient.NewListPager(options) +} + +// twoSubscriptionPages returns a mockSubscriptionsClient listing the same +// two fixed subscriptions ("sub-1"/"sub-2") every test in this file needs; +// none of the cache/fan-out tests care about the actual subscription +// identifiers, so a fixed pair keeps call sites short. +func twoSubscriptionPages() *mockSubscriptionsClient { + sub1ID, sub1Name := "sub-1", "Subscription 1" + sub2ID, sub2Name := "sub-2", "Subscription 2" + return &mockSubscriptionsClient{ + listPagerFunc: func(options *armsubscriptions.ClientListOptions) SubscriptionsPager { + return &mockSubscriptionsPager{ + pages: []armsubscriptions.ClientListResponse{ + { + SubscriptionListResult: armsubscriptions.SubscriptionListResult{ + Value: []*armsubscriptions.Subscription{ + {SubscriptionID: &sub1ID, DisplayName: &sub1Name}, + {SubscriptionID: &sub2ID, DisplayName: &sub2Name}, + }, + }, + }, + }, + } + }, + } +} + +func TestAzureProvider_GetAccounts_CacheHit(t *testing.T) { + counting := &countingSubscriptionsClient{mockSubscriptionsClient: twoSubscriptionPages()} + + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(counting) + + first, err := p.GetAccounts(context.Background()) + require.NoError(t, err) + require.Len(t, first, 2) + assert.Equal(t, 1, counting.calls, "first GetAccounts call should hit the API once") + + second, err := p.GetAccounts(context.Background()) + require.NoError(t, err) + require.Len(t, second, 2) + assert.Equal(t, 1, counting.calls, "second GetAccounts call should be served from cache, not the API") + assert.Equal(t, first, second) +} + +func TestAzureProvider_GetAccounts_CacheHit_ReturnsIndependentCopies(t *testing.T) { + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(twoSubscriptionPages()) + + first, err := p.GetAccounts(context.Background()) + require.NoError(t, err) + first[0].IsDefault = true // mutate the caller's copy + + second, err := p.GetAccounts(context.Background()) + require.NoError(t, err) + assert.False(t, second[0].IsDefault, "mutating a returned slice must not corrupt the cache") +} + +func TestAzureProvider_InvalidateAccountsCache(t *testing.T) { + counting := &countingSubscriptionsClient{mockSubscriptionsClient: twoSubscriptionPages()} + + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(counting) + + _, err := p.GetAccounts(context.Background()) + require.NoError(t, err) + assert.Equal(t, 1, counting.calls) + + p.InvalidateAccountsCache() + + _, err = p.GetAccounts(context.Background()) + require.NoError(t, err) + assert.Equal(t, 2, counting.calls, "GetAccounts after InvalidateAccountsCache should re-hit the API") +} + +func TestAzureProvider_GetRecommendationsClient_MultiSubscriptionFanOut(t *testing.T) { + t.Run("multi-subscription returns MultiSubscriptionRecommendationsClient", func(t *testing.T) { + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(twoSubscriptionPages()) + + client, err := p.GetRecommendationsClient(context.Background()) + require.NoError(t, err) + require.IsType(t, &MultiSubscriptionRecommendationsClient{}, client) + assert.Len(t, client.(*MultiSubscriptionRecommendationsClient).subscriptions, 2) + }) + + t.Run("single discovered subscription returns RecommendationsClientAdapter", func(t *testing.T) { + subID, subName := "sub-solo", "Solo Subscription" + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(&mockSubscriptionsClient{ + listPagerFunc: func(options *armsubscriptions.ClientListOptions) SubscriptionsPager { + return &mockSubscriptionsPager{ + pages: []armsubscriptions.ClientListResponse{ + {SubscriptionListResult: armsubscriptions.SubscriptionListResult{ + Value: []*armsubscriptions.Subscription{{SubscriptionID: &subID, DisplayName: &subName}}, + }}, + }, + } + }, + }) + + client, err := p.GetRecommendationsClient(context.Background()) + require.NoError(t, err) + require.IsType(t, &RecommendationsClientAdapter{}, client) + assert.Equal(t, subID, client.(*RecommendationsClientAdapter).subscriptionID) + }) + + t.Run("pinned subscription always returns single adapter regardless of discovered count", func(t *testing.T) { + p := &AzureProvider{cred: &mockTokenCredential{}, subscriptionID: "pinned-sub"} + // Deliberately do not set a subscriptions client: a pinned subscription + // must never trigger subscription discovery. + client, err := p.GetRecommendationsClient(context.Background()) + require.NoError(t, err) + require.IsType(t, &RecommendationsClientAdapter{}, client) + assert.Equal(t, "pinned-sub", client.(*RecommendationsClientAdapter).subscriptionID) + }) + + // The zero-subscription "no Azure subscriptions found" case is already + // covered by TestAzureProvider_GetRecommendationsClient_WithSubscriptionLookup. + + t.Run("subscription discovery failure is propagated", func(t *testing.T) { + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(&mockSubscriptionsClient{ + listPagerFunc: func(options *armsubscriptions.ClientListOptions) SubscriptionsPager { + return &mockSubscriptionsPager{nextErr: errors.New("boom")} + }, + }) + + _, err := p.GetRecommendationsClient(context.Background()) + assert.Error(t, err) + assert.Contains(t, err.Error(), "failed to resolve Azure subscriptions") + }) +} diff --git a/providers/azure/recommendations_multi_subscription.go b/providers/azure/recommendations_multi_subscription.go new file mode 100644 index 000000000..c5d3e7392 --- /dev/null +++ b/providers/azure/recommendations_multi_subscription.go @@ -0,0 +1,166 @@ +// Package azure provides the org-wide (multi-subscription) recommendations +// fan-out client. +package azure + +import ( + "context" + "fmt" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "golang.org/x/sync/errgroup" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/logging" + "github.com/LeanerCloud/CUDly/pkg/provider" +) + +// newSubscriptionRecommendationsClientFn builds the per-subscription +// recommendations client. Declared as a package-level var (default: +// NewRecommendationsClientAdapter) so tests can substitute a fake +// per-subscription client and exercise fan-out/merge behavior without +// issuing real ARM calls. Mirrors the newComputeClientFn-style injection +// used by RecommendationsClientAdapter in recommendations.go. +var newSubscriptionRecommendationsClientFn = func(cred azcore.TokenCredential, subscriptionID string) (provider.RecommendationsClient, error) { + return NewRecommendationsClientAdapter(cred, subscriptionID) +} + +// subscriptionClient pairs a subscription ID with its recommendations +// client so fan-out logs and error messages can identify which subscription +// a failure came from. +type subscriptionClient struct { + subscriptionID string + client provider.RecommendationsClient +} + +// MultiSubscriptionRecommendationsClient fans recommendation collection out +// across every Azure subscription accessible to the authenticated +// principal. +// +// Azure has no organization-wide equivalent of AWS Cost Explorer's +// AccountScope=Linked: the Consumption Reservation Recommendations and +// Advisor APIs are subscription-scoped. Achieving AWS-parity org-wide +// coverage therefore requires calling the per-subscription +// RecommendationsClientAdapter once per subscription and aggregating the +// results client-side, which is what this type does. +type MultiSubscriptionRecommendationsClient struct { + subscriptions []subscriptionClient +} + +// NewMultiSubscriptionRecommendationsClient builds a fan-out client covering +// every account in accounts. Returns an error when accounts is empty (there +// is nothing to fan out to) or when building the per-subscription client +// fails for any account -- fail loud rather than silently dropping a +// subscription that should have been covered. +func NewMultiSubscriptionRecommendationsClient(cred azcore.TokenCredential, accounts []common.Account) (*MultiSubscriptionRecommendationsClient, error) { + if len(accounts) == 0 { + return nil, fmt.Errorf("azure multi-subscription recommendations: at least one subscription is required") + } + + subscriptions := make([]subscriptionClient, 0, len(accounts)) + for _, account := range accounts { + client, err := newSubscriptionRecommendationsClientFn(cred, account.ID) + if err != nil { + return nil, fmt.Errorf("azure multi-subscription recommendations: failed to build client for subscription %s: %w", account.ID, err) + } + subscriptions = append(subscriptions, subscriptionClient{subscriptionID: account.ID, client: client}) + } + + return &MultiSubscriptionRecommendationsClient{subscriptions: subscriptions}, nil +} + +// GetRecommendations fans params out to every subscription concurrently +// (errgroup) and merges the results. +// +// Error isolation mirrors RecommendationsClientAdapter.GetRecommendations: +// each per-subscription goroutine captures its own error and returns nil to +// the group, so one subscription failing (e.g. the principal lost Reader +// access mid-run, or a subscription-specific throttle) never cancels +// sibling subscriptions. The semaphore that bounds aggregate concurrent ARM +// calls is acquired inside each per-subscription client's own +// GetRecommendations (around the outbound API calls, not around this +// fan-out), so no additional semaphore is needed at this layer. +// +// After g.Wait(), ctx.Err() is checked explicitly: g.Wait() only reports +// errors returned to the group, and every goroutine here returns nil, so a +// parent-context cancellation would otherwise go unnoticed. +// +// If every subscription fails, GetRecommendations returns a wrapped error +// instead of a silently empty, nil-error result -- the same +// all-attempted-failed guard used by mergeServiceResults, ported here so a +// total credential/throttle failure isn't indistinguishable from "no +// savings available across the whole tenant". +func (m *MultiSubscriptionRecommendationsClient) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { + if params == nil { + return nil, fmt.Errorf("params cannot be nil") + } + + results := make([][]common.Recommendation, len(m.subscriptions)) + errs := make([]error, len(m.subscriptions)) + + g, gctx := errgroup.WithContext(ctx) + for i, sub := range m.subscriptions { + i, sub := i, sub + g.Go(func() error { + recs, err := sub.client.GetRecommendations(gctx, params) + results[i] = recs + errs[i] = err + return nil // error isolation: never propagate to errgroup + }) + } + if err := g.Wait(); err != nil { + // Unreachable in practice -- every goroutine above returns nil -- but + // handled explicitly (rather than discarded) so a future change that + // starts propagating a goroutine error isn't silently swallowed. + return nil, err + } + + // Propagate parent-context cancellation explicitly -- see doc comment. + if err := ctx.Err(); err != nil { + return nil, err + } + + return m.mergeResults(results, errs) +} + +// mergeResults concatenates successful per-subscription results, logging a +// warning for each subscription that failed, and applies the +// all-attempted-failed guard described in GetRecommendations' doc comment. +func (m *MultiSubscriptionRecommendationsClient) mergeResults(results [][]common.Recommendation, errs []error) ([]common.Recommendation, error) { + total := 0 + for _, r := range results { + total += len(r) + } + + out := make([]common.Recommendation, 0, total) + failures := 0 + var lastErr error + for i, err := range errs { + if err != nil { + failures++ + lastErr = err + logging.Warnf("Azure subscription %s recommendations: %v", m.subscriptions[i].subscriptionID, err) + continue + } + out = append(out, results[i]...) + } + + if failures > 0 && failures == len(m.subscriptions) { + return nil, fmt.Errorf("all %d Azure subscriptions failed to return recommendations: %w", failures, lastErr) + } + return out, nil +} + +// GetRecommendationsForService retrieves recommendations for a single +// service across every subscription. +func (m *MultiSubscriptionRecommendationsClient) GetRecommendationsForService(ctx context.Context, service common.ServiceType) ([]common.Recommendation, error) { + return m.GetRecommendations(ctx, &common.RecommendationParams{Service: service}) +} + +// GetAllRecommendations retrieves recommendations for every supported +// service across every subscription. +func (m *MultiSubscriptionRecommendationsClient) GetAllRecommendations(ctx context.Context) ([]common.Recommendation, error) { + return m.GetRecommendations(ctx, &common.RecommendationParams{}) +} + +// Compile-time interface compliance check. +var _ provider.RecommendationsClient = (*MultiSubscriptionRecommendationsClient)(nil) diff --git a/providers/azure/recommendations_multi_subscription_test.go b/providers/azure/recommendations_multi_subscription_test.go new file mode 100644 index 000000000..1a8ccb8ab --- /dev/null +++ b/providers/azure/recommendations_multi_subscription_test.go @@ -0,0 +1,194 @@ +package azure + +import ( + "context" + "errors" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/provider" +) + +// fakeRecommendationsClient implements provider.RecommendationsClient for +// fan-out tests, letting each fake subscription's response be controlled +// independently of the others. +type fakeRecommendationsClient struct { + recs []common.Recommendation + err error + gotParams *common.RecommendationParams +} + +func (f *fakeRecommendationsClient) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { + f.gotParams = params + if err := ctx.Err(); err != nil { + // Respect cancellation like a real ARM client would (the SDK's + // underlying HTTP transport checks ctx before issuing the request). + return nil, err + } + return f.recs, f.err +} + +func (f *fakeRecommendationsClient) GetRecommendationsForService(ctx context.Context, service common.ServiceType) ([]common.Recommendation, error) { + return f.GetRecommendations(ctx, &common.RecommendationParams{Service: service}) +} + +func (f *fakeRecommendationsClient) GetAllRecommendations(ctx context.Context) ([]common.Recommendation, error) { + return f.GetRecommendations(ctx, &common.RecommendationParams{}) +} + +// withFakeSubscriptionClients overrides newSubscriptionRecommendationsClientFn +// to hand out the given fakes in order (one per NewMultiSubscriptionRecommendationsClient +// account, in the order accounts are passed) and restores the original on +// cleanup. +func withFakeSubscriptionClients(t *testing.T, fakes map[string]*fakeRecommendationsClient) { + t.Helper() + orig := newSubscriptionRecommendationsClientFn + t.Cleanup(func() { newSubscriptionRecommendationsClientFn = orig }) + newSubscriptionRecommendationsClientFn = func(_ azcore.TokenCredential, subscriptionID string) (provider.RecommendationsClient, error) { + fake, ok := fakes[subscriptionID] + if !ok { + return nil, errors.New("unexpected subscriptionID: " + subscriptionID) + } + return fake, nil + } +} + +func twoTestAccounts() []common.Account { + return []common.Account{ + {Provider: common.ProviderAzure, ID: "sub-1", Name: "Subscription 1"}, + {Provider: common.ProviderAzure, ID: "sub-2", Name: "Subscription 2"}, + } +} + +func TestNewMultiSubscriptionRecommendationsClient_EmptyAccounts(t *testing.T) { + client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, nil) + require.Error(t, err) + assert.Nil(t, client) + assert.Contains(t, err.Error(), "at least one subscription is required") +} + +func TestNewMultiSubscriptionRecommendationsClient_BuildsClientsPerAccount(t *testing.T) { + accounts := twoTestAccounts() + withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ + "sub-1": {recs: []common.Recommendation{{Account: "sub-1"}}}, + "sub-2": {recs: []common.Recommendation{{Account: "sub-2"}}}, + }) + + client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, accounts) + require.NoError(t, err) + require.Len(t, client.subscriptions, 2) + assert.Equal(t, "sub-1", client.subscriptions[0].subscriptionID) + assert.Equal(t, "sub-2", client.subscriptions[1].subscriptionID) +} + +func TestNewMultiSubscriptionRecommendationsClient_ClientConstructionFailurePropagates(t *testing.T) { + orig := newSubscriptionRecommendationsClientFn + t.Cleanup(func() { newSubscriptionRecommendationsClientFn = orig }) + newSubscriptionRecommendationsClientFn = func(_ azcore.TokenCredential, subscriptionID string) (provider.RecommendationsClient, error) { + return nil, errors.New("boom") + } + + client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, twoTestAccounts()) + require.Error(t, err) + assert.Nil(t, client) + assert.Contains(t, err.Error(), "failed to build client for subscription") +} + +func TestMultiSubscriptionRecommendationsClient_GetRecommendations_MergesAcrossSubscriptions(t *testing.T) { + accounts := twoTestAccounts() + withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ + "sub-1": {recs: []common.Recommendation{{Account: "sub-1", Service: common.ServiceCompute}}}, + "sub-2": {recs: []common.Recommendation{{Account: "sub-2", Service: common.ServiceCache}}}, + }) + + client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, accounts) + require.NoError(t, err) + + recs, err := client.GetAllRecommendations(context.Background()) + require.NoError(t, err) + assert.ElementsMatch(t, []common.Recommendation{ + {Account: "sub-1", Service: common.ServiceCompute}, + {Account: "sub-2", Service: common.ServiceCache}, + }, recs) +} + +func TestMultiSubscriptionRecommendationsClient_GetRecommendations_PartialFailureStillSucceeds(t *testing.T) { + accounts := twoTestAccounts() + withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ + "sub-1": {err: errors.New("sub-1 unreachable")}, + "sub-2": {recs: []common.Recommendation{{Account: "sub-2", Service: common.ServiceCache}}}, + }) + + client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, accounts) + require.NoError(t, err) + + recs, err := client.GetAllRecommendations(context.Background()) + require.NoError(t, err, "one subscription failing must not fail the whole fan-out") + assert.Equal(t, []common.Recommendation{{Account: "sub-2", Service: common.ServiceCache}}, recs) +} + +func TestMultiSubscriptionRecommendationsClient_GetRecommendations_AllFail(t *testing.T) { + accounts := twoTestAccounts() + withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ + "sub-1": {err: errors.New("sub-1 unreachable")}, + "sub-2": {err: errors.New("sub-2 unreachable")}, + }) + + client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, accounts) + require.NoError(t, err) + + recs, err := client.GetAllRecommendations(context.Background()) + require.Error(t, err) + assert.Nil(t, recs) + assert.Contains(t, err.Error(), "all 2 Azure subscriptions failed") +} + +func TestMultiSubscriptionRecommendationsClient_GetRecommendations_NilParams(t *testing.T) { + client := &MultiSubscriptionRecommendationsClient{} + recs, err := client.GetRecommendations(context.Background(), nil) + require.EqualError(t, err, "params cannot be nil") + assert.Nil(t, recs) +} + +func TestMultiSubscriptionRecommendationsClient_GetRecommendations_PropagatesContextCancellation(t *testing.T) { + accounts := twoTestAccounts() + withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ + "sub-1": {recs: []common.Recommendation{{Account: "sub-1"}}}, + "sub-2": {recs: []common.Recommendation{{Account: "sub-2"}}}, + }) + + client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, accounts) + require.NoError(t, err) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + recs, err := client.GetAllRecommendations(ctx) + require.Error(t, err, "expected context.Canceled to propagate from GetRecommendations") + assert.ErrorIs(t, err, context.Canceled) + assert.Nil(t, recs) +} + +func TestMultiSubscriptionRecommendationsClient_GetRecommendationsForService_PassesServiceFilter(t *testing.T) { + accounts := twoTestAccounts() + fake1 := &fakeRecommendationsClient{} + fake2 := &fakeRecommendationsClient{} + withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ + "sub-1": fake1, + "sub-2": fake2, + }) + + client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, accounts) + require.NoError(t, err) + + _, err = client.GetRecommendationsForService(context.Background(), common.ServiceCompute) + require.NoError(t, err) + require.NotNil(t, fake1.gotParams) + require.NotNil(t, fake2.gotParams) + assert.Equal(t, common.ServiceCompute, fake1.gotParams.Service) + assert.Equal(t, common.ServiceCompute, fake2.gotParams.Service) +} From 14050890d1415e2e9961f75609e6e18755650409 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 17:23:54 +0200 Subject: [PATCH 02/11] fix(azure): singleflight the accounts cache instead of locking across the ARM fetch getOrFetchAccounts held accountsMu.Lock() for the entire subscriptions.List network round-trip via fetchAccountsLocked, so every reader of the cached subscription list blocked behind a slow or hung ARM call. Move the fetch into golang.org/x/sync/singleflight so concurrent cold-cache callers coalesce into one in-flight request, taking accountsMu only to read or populate cachedAccounts. Add a deterministic regression test that stages N goroutines through a channel-gated fake fetch and asserts exactly one underlying ARM call. --- providers/azure/accounts_cache.go | 189 ++++++++++++++++++++++++++++++ providers/azure/provider.go | 169 ++------------------------ providers/azure/provider_test.go | 105 ++++++++++++++++- 3 files changed, 300 insertions(+), 163 deletions(-) create mode 100644 providers/azure/accounts_cache.go diff --git a/providers/azure/accounts_cache.go b/providers/azure/accounts_cache.go new file mode 100644 index 000000000..42f963e64 --- /dev/null +++ b/providers/azure/accounts_cache.go @@ -0,0 +1,189 @@ +package azure + +import ( + "context" + "fmt" + "os" + + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions" + + "github.com/LeanerCloud/CUDly/pkg/common" +) + +// accountsCacheSFKey is the single singleflight.Group key this cache ever +// uses. There is exactly one cached value per AzureProvider (the org-wide +// subscription list), so a constant key is sufficient to coalesce every +// concurrent cold-cache caller onto the same in-flight fetch. +const accountsCacheSFKey = "accounts" + +// getOrFetchAccounts returns the cached subscription list, populating it via +// fetchAccounts on first use. Safe for concurrent callers: +// +// - Fast path: a read lock serves the already-populated cache. +// - Slow path: singleflight.Group collapses concurrent cold-cache callers +// into a single in-flight ARM subscriptions.List call -- the network +// round-trip runs OUTSIDE accountsMu, so a slow or hung lookup never +// blocks readers of the cache. accountsMu is only ever held briefly to +// read or populate cachedAccounts, never across the fetch. +// - Every caller receives an independent clone so a returned slice can't be +// mutated into the shared cache. +// +// Callers must have already verified IsConfigured(); this method assumes a +// usable credential is present (mirrors GetAccounts, its only production +// caller alongside GetServiceClient/GetRecommendationsClient which check +// IsConfigured() themselves before resolving accounts). +func (p *AzureProvider) getOrFetchAccounts(ctx context.Context) ([]common.Account, error) { + p.accountsMu.RLock() + cached := p.cachedAccounts + p.accountsMu.RUnlock() + if cached != nil { + return cloneAccounts(cached), nil + } + + v, err, _ := p.accountsSF.Do(accountsCacheSFKey, func() (interface{}, error) { + // Re-check: another goroutine's fetch may have populated the cache + // between our RLock check above and this closure actually running + // (e.g. it lost the race to become the singleflight leader). + p.accountsMu.RLock() + cached := p.cachedAccounts + p.accountsMu.RUnlock() + if cached != nil { + return cached, nil + } + + accounts, err := p.fetchAccounts(ctx) + if err != nil { + return nil, err + } + + p.accountsMu.Lock() + p.cachedAccounts = accounts + p.accountsMu.Unlock() + + return accounts, nil + }) + if err != nil { + return nil, err + } + return cloneAccounts(v.([]common.Account)), nil +} + +// cloneAccounts returns a shallow copy of accounts backed by a fresh array. +// common.Account has no nested slices/maps, so a shallow per-element copy is +// sufficient to stop a caller mutating a returned slice (e.g. flipping +// IsDefault) from corrupting the shared cache -- the same class of bug +// flagged for getters returning nested state. +func cloneAccounts(accounts []common.Account) []common.Account { + out := make([]common.Account, len(accounts)) + copy(out, accounts) + return out +} + +// InvalidateAccountsCache clears the cached subscription list so the next +// getOrFetchAccounts call re-fetches from the ARM subscriptions API. Exposed +// for tests that need to assert cache-miss behavior; production callers +// currently rely on the cache living for the lifetime of the AzureProvider +// instance (one instance is constructed per collection/purchase run). +func (p *AzureProvider) InvalidateAccountsCache() { + p.accountsMu.Lock() + defer p.accountsMu.Unlock() + p.cachedAccounts = nil +} + +// fetchAccounts performs the actual ARM subscriptions.List call and resolves +// the default subscription. It holds no lock and issues the network round-trip +// on the caller's goroutine; getOrFetchAccounts serializes concurrent cold-cache +// callers via singleflight so this runs at most once per cache-population window. +func (p *AzureProvider) fetchAccounts(ctx context.Context) ([]common.Account, error) { + // Use injected client if available (for testing) + var subClient SubscriptionsClient + if p.subscriptionsClient != nil { + subClient = p.subscriptionsClient + } else { + client, err := armsubscriptions.NewClient(p.cred, nil) + if err != nil { + return nil, fmt.Errorf("failed to create subscriptions client: %w", err) + } + subClient = &realSubscriptionsClient{client: client} + } + + accounts := make([]common.Account, 0) + pager := subClient.NewListPager(nil) + + for pager.More() { + page, err := pager.NextPage(ctx) + if err != nil { + return nil, fmt.Errorf("failed to list subscriptions: %w", err) + } + + for _, sub := range page.Value { + if sub.SubscriptionID == nil || sub.DisplayName == nil { + continue + } + + accounts = append(accounts, common.Account{ + Provider: common.ProviderAzure, + ID: *sub.SubscriptionID, + Name: *sub.DisplayName, + DisplayName: *sub.DisplayName, + // IsDefault resolved below once the full list is available. + IsDefault: false, + }) + } + } + + // Resolve which subscription is the default. + resolveDefaultSubscription(accounts, p.subscriptionID) + + return accounts, nil +} + +// resolveDefaultSubscription sets IsDefault on the matching account in-place. +// +// Priority: +// 1. explicitSubID (from ProviderConfig.AzureSubscriptionID / Profile). +// 2. AZURE_SUBSCRIPTION_ID environment variable. +// 3. When exactly one subscription is visible, mark it default (mirrors AWS +// behaviour where the STS-identified account is always the default). +func resolveDefaultSubscription(accounts []common.Account, explicitSubID string) { + if len(accounts) == 0 { + return + } + + target := explicitSubID + if target == "" { + target = os.Getenv("AZURE_SUBSCRIPTION_ID") + } + + if target != "" { + for i := range accounts { + if accounts[i].ID == target { + accounts[i].IsDefault = true + return + } + } + // target was configured but not found in the visible subscriptions; + // fall through to the single-subscription rule rather than leaving + // all accounts as non-default. + } + + // Rule 3: single visible subscription. + if len(accounts) == 1 { + accounts[0].IsDefault = true + } +} + +// getDefaultSubscriptionID returns the ID of the default subscription from a +// pre-fetched account list, or an empty string when no account is marked +// default (e.g. ambiguous multi-subscription tenants with no explicit config). +func getDefaultSubscriptionID(accounts []common.Account) string { + if len(accounts) == 0 { + return "" + } + for _, a := range accounts { + if a.IsDefault { + return a.ID + } + } + return "" +} diff --git a/providers/azure/provider.go b/providers/azure/provider.go index f93cbf384..e96230a1e 100644 --- a/providers/azure/provider.go +++ b/providers/azure/provider.go @@ -4,7 +4,6 @@ package azure import ( "context" "fmt" - "os" "sync" "time" @@ -12,6 +11,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions" + "golang.org/x/sync/singleflight" "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/logging" @@ -104,9 +104,19 @@ type AzureProvider struct { // subscriptions.List call once per internal caller. cachedAccounts is // nil until the first successful fetch; InvalidateAccountsCache resets // it so tests (and long-lived callers that expect subscription - // membership to change) can force a refresh. + // membership to change) can force a refresh. The mutex is held only to + // read/write cachedAccounts, never across the ARM network round-trip. + // + // accountsSF collapses concurrent cold-cache callers into a single + // in-flight subscriptions.List: the ARM call runs once (not once per + // caller) and, crucially, outside accountsMu so a slow or hung lookup + // can never block readers of the cache. accountsMu sync.RWMutex cachedAccounts []common.Account + accountsSF singleflight.Group + + // cache subsystem implementation (getOrFetchAccounts, fetchAccounts, + // cloneAccounts, InvalidateAccountsCache) lives in accounts_cache.go. } // NewAzureProvider creates a new Azure provider instance. @@ -267,161 +277,6 @@ func (p *AzureProvider) GetAccounts(ctx context.Context) ([]common.Account, erro return p.getOrFetchAccounts(ctx) } -// getOrFetchAccounts returns the cached subscription list, populating it via -// fetchAccountsLocked on first use. Safe for concurrent callers: a read lock -// guards the fast path (cache already populated); a write lock guards the -// fetch-and-populate path, with a re-check after acquiring it so concurrent -// callers that lost the race to the lock don't issue a redundant ARM call. -// -// Callers must have already verified IsConfigured(); this method assumes a -// usable credential is present (mirrors GetAccounts, its only production -// caller alongside GetServiceClient/GetRecommendationsClient which check -// IsConfigured() themselves before resolving accounts). -func (p *AzureProvider) getOrFetchAccounts(ctx context.Context) ([]common.Account, error) { - p.accountsMu.RLock() - cached := p.cachedAccounts - p.accountsMu.RUnlock() - if cached != nil { - return cloneAccounts(cached), nil - } - - p.accountsMu.Lock() - defer p.accountsMu.Unlock() - // Re-check: another goroutine may have populated the cache while this - // one was waiting on the write lock. - if p.cachedAccounts != nil { - return cloneAccounts(p.cachedAccounts), nil - } - - accounts, err := p.fetchAccountsLocked(ctx) - if err != nil { - return nil, err - } - p.cachedAccounts = accounts - return cloneAccounts(accounts), nil -} - -// cloneAccounts returns a shallow copy of accounts backed by a fresh array. -// common.Account has no nested slices/maps, so a shallow per-element copy is -// sufficient to stop a caller mutating a returned slice (e.g. flipping -// IsDefault) from corrupting the shared cache -- the same class of bug -// flagged for getters returning nested state. -func cloneAccounts(accounts []common.Account) []common.Account { - out := make([]common.Account, len(accounts)) - copy(out, accounts) - return out -} - -// InvalidateAccountsCache clears the cached subscription list so the next -// getOrFetchAccounts call re-fetches from the ARM subscriptions API. Exposed -// for tests that need to assert cache-miss behavior; production callers -// currently rely on the cache living for the lifetime of the AzureProvider -// instance (one instance is constructed per collection/purchase run). -func (p *AzureProvider) InvalidateAccountsCache() { - p.accountsMu.Lock() - defer p.accountsMu.Unlock() - p.cachedAccounts = nil -} - -// fetchAccountsLocked performs the actual ARM subscriptions.List call and -// resolves the default subscription. Must only be called while holding -// accountsMu for writing (via getOrFetchAccounts) -- it does not lock itself -// so getOrFetchAccounts can do its cache-populate-and-return in one critical -// section. -func (p *AzureProvider) fetchAccountsLocked(ctx context.Context) ([]common.Account, error) { - // Use injected client if available (for testing) - var subClient SubscriptionsClient - if p.subscriptionsClient != nil { - subClient = p.subscriptionsClient - } else { - client, err := armsubscriptions.NewClient(p.cred, nil) - if err != nil { - return nil, fmt.Errorf("failed to create subscriptions client: %w", err) - } - subClient = &realSubscriptionsClient{client: client} - } - - accounts := make([]common.Account, 0) - pager := subClient.NewListPager(nil) - - for pager.More() { - page, err := pager.NextPage(ctx) - if err != nil { - return nil, fmt.Errorf("failed to list subscriptions: %w", err) - } - - for _, sub := range page.Value { - if sub.SubscriptionID == nil || sub.DisplayName == nil { - continue - } - - accounts = append(accounts, common.Account{ - Provider: common.ProviderAzure, - ID: *sub.SubscriptionID, - Name: *sub.DisplayName, - DisplayName: *sub.DisplayName, - // IsDefault resolved below once the full list is available. - IsDefault: false, - }) - } - } - - // Resolve which subscription is the default. - resolveDefaultSubscription(accounts, p.subscriptionID) - - return accounts, nil -} - -// resolveDefaultSubscription sets IsDefault on the matching account in-place. -// -// Priority: -// 1. explicitSubID (from ProviderConfig.AzureSubscriptionID / Profile). -// 2. AZURE_SUBSCRIPTION_ID environment variable. -// 3. When exactly one subscription is visible, mark it default (mirrors AWS -// behaviour where the STS-identified account is always the default). -func resolveDefaultSubscription(accounts []common.Account, explicitSubID string) { - if len(accounts) == 0 { - return - } - - target := explicitSubID - if target == "" { - target = os.Getenv("AZURE_SUBSCRIPTION_ID") - } - - if target != "" { - for i := range accounts { - if accounts[i].ID == target { - accounts[i].IsDefault = true - return - } - } - // target was configured but not found in the visible subscriptions; - // fall through to the single-subscription rule rather than leaving - // all accounts as non-default. - } - - // Rule 3: single visible subscription. - if len(accounts) == 1 { - accounts[0].IsDefault = true - } -} - -// getDefaultSubscriptionID returns the ID of the default subscription from a -// pre-fetched account list, or an empty string when no account is marked -// default (e.g. ambiguous multi-subscription tenants with no explicit config). -func getDefaultSubscriptionID(accounts []common.Account) string { - if len(accounts) == 0 { - return "" - } - for _, a := range accounts { - if a.IsDefault { - return a.ID - } - } - return "" -} - // resolveSubscriptionIDFromCtx calls GetAccounts and returns the default // subscription ID, or a descriptive error if none can be resolved. func (p *AzureProvider) resolveSubscriptionIDFromCtx(ctx context.Context) (string, error) { diff --git a/providers/azure/provider_test.go b/providers/azure/provider_test.go index fdd128955..998756167 100644 --- a/providers/azure/provider_test.go +++ b/providers/azure/provider_test.go @@ -3,6 +3,9 @@ package azure import ( "context" "errors" + "runtime" + "sync" + "sync/atomic" "testing" "github.com/Azure/azure-sdk-for-go/sdk/azcore" @@ -1247,11 +1250,11 @@ func TestAzureProvider_GetRecommendationsClientForAccount(t *testing.T) { // underlying ARM API is only called once. type countingSubscriptionsClient struct { *mockSubscriptionsClient - calls int + calls atomic.Int64 } func (c *countingSubscriptionsClient) NewListPager(options *armsubscriptions.ClientListOptions) SubscriptionsPager { - c.calls++ + c.calls.Add(1) return c.mockSubscriptionsClient.NewListPager(options) } @@ -1289,12 +1292,12 @@ func TestAzureProvider_GetAccounts_CacheHit(t *testing.T) { first, err := p.GetAccounts(context.Background()) require.NoError(t, err) require.Len(t, first, 2) - assert.Equal(t, 1, counting.calls, "first GetAccounts call should hit the API once") + assert.Equal(t, int64(1), counting.calls.Load(), "first GetAccounts call should hit the API once") second, err := p.GetAccounts(context.Background()) require.NoError(t, err) require.Len(t, second, 2) - assert.Equal(t, 1, counting.calls, "second GetAccounts call should be served from cache, not the API") + assert.Equal(t, int64(1), counting.calls.Load(), "second GetAccounts call should be served from cache, not the API") assert.Equal(t, first, second) } @@ -1319,13 +1322,103 @@ func TestAzureProvider_InvalidateAccountsCache(t *testing.T) { _, err := p.GetAccounts(context.Background()) require.NoError(t, err) - assert.Equal(t, 1, counting.calls) + assert.Equal(t, int64(1), counting.calls.Load()) p.InvalidateAccountsCache() _, err = p.GetAccounts(context.Background()) require.NoError(t, err) - assert.Equal(t, 2, counting.calls, "GetAccounts after InvalidateAccountsCache should re-hit the API") + assert.Equal(t, int64(2), counting.calls.Load(), "GetAccounts after InvalidateAccountsCache should re-hit the API") +} + +// gatedCountingSubscriptionsClient counts ARM list calls and holds the first +// one open until released, so a test can guarantee the single in-flight fetch +// is genuinely in progress while additional cold-cache callers pile up behind +// it. This is what makes the single-flight assertion deterministic for both a +// correct impl (exactly one call) and a naive per-caller fetch (several calls). +type gatedCountingSubscriptionsClient struct { + inner *mockSubscriptionsClient + calls atomic.Int64 + firstOnce sync.Once + firstInFlight chan struct{} // closed when the first fetch has entered + release chan struct{} // closed by the test to let held fetches proceed +} + +func (c *gatedCountingSubscriptionsClient) NewListPager(options *armsubscriptions.ClientListOptions) SubscriptionsPager { + c.calls.Add(1) + c.firstOnce.Do(func() { close(c.firstInFlight) }) + <-c.release + return c.inner.NewListPager(options) +} + +func (c *gatedCountingSubscriptionsClient) NewListLocationsPager(subscriptionID string, options *armsubscriptions.ClientListLocationsOptions) LocationsPager { + return c.inner.NewListLocationsPager(subscriptionID, options) +} + +// TestAzureProvider_GetAccounts_ConcurrentColdCache_SingleARMCall guards the +// single-flight cold-cache contract: many goroutines hitting an empty cache at +// once must collapse into exactly one ARM subscriptions.List call, not one per +// caller. +// +// The staged launch makes this deterministic: the leader's fetch is held +// in-flight (blocked on release) before the followers start, so the followers +// hit a cold cache while the single fetch is open. A correct impl collapses +// them via single-flight (and the closure's cache re-check), yielding exactly +// one call regardless of scheduling; a naive "fetch outside the lock without +// single-flight" fix lets the followers each issue their own ARM call, which +// this test detects via the atomic counter. Run under -race to also catch any +// unguarded shared-state access on the cold-cache path. +func TestAzureProvider_GetAccounts_ConcurrentColdCache_SingleARMCall(t *testing.T) { + gated := &gatedCountingSubscriptionsClient{ + inner: twoSubscriptionPages(), + firstInFlight: make(chan struct{}), + release: make(chan struct{}), + } + + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(gated) + + const n = 10 + var wg sync.WaitGroup + errs := make(chan error, n) + results := make(chan []common.Account, n) + worker := func() { + defer wg.Done() + accts, err := p.GetAccounts(context.Background()) + errs <- err + results <- accts + } + + // Launch the leader and wait until its ARM fetch is actually in-flight + // before launching the followers, so they observe a cold cache. + wg.Add(1) + go worker() + <-gated.firstInFlight + + for i := 1; i < n; i++ { + wg.Add(1) + go worker() + } + // Nudge the followers onto the cache/single-flight path, then release the + // held fetch so everything can complete. + for i := 0; i < n; i++ { + runtime.Gosched() + } + close(gated.release) + + wg.Wait() + close(errs) + close(results) + + // require.* only from the test goroutine, never the workers. + for err := range errs { + require.NoError(t, err) + } + for accts := range results { + require.Len(t, accts, 2) + } + assert.Equal(t, int64(1), gated.calls.Load(), + "concurrent cold-cache GetAccounts must issue exactly one ARM list call (single-flight)") } func TestAzureProvider_GetRecommendationsClient_MultiSubscriptionFanOut(t *testing.T) { From c73319729a123a3420902c500b5ec3473e748d87 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 19:42:41 +0200 Subject: [PATCH 03/11] fix(azure): keep recommendation scope pinned and fix cache invalidation Adversarial review of the org-wide fan-out. Scoping first. GetRecommendationsClient ignored the resolved default subscription. Before fan-out existed, an unpinned provider resolved the default via resolveSubscriptionIDFromCtx, which honours AZURE_SUBSCRIPTION_ID and errors when the scope is ambiguous. The fan-out path replaced that with "2+ visible subscriptions means fan out", so a caller that pinned a subscription through the environment silently started receiving every other visible subscription's recommendations. Restore the precedence: a resolvable default still yields a single-subscription client, a configured AZURE_SUBSCRIPTION_ID naming a subscription this principal cannot see is an error, and only a genuinely unnamed scope fans out. MultiSubscriptionRecommendationsClient ignored params.AccountFilter. The AWS provider applies that filter to every recommendation it returns (filterByAccounts in providers/aws/service_client.go). It was moot while an Azure client could only ever cover its own subscription, but a client covering every visible subscription has to honour it or a request scoped to one account comes back carrying others'. Filter before the fan-out so subscriptions outside the filter are never queried, and error rather than return an empty slice when the filter matches nothing. Then the subscription-list cache. InvalidateAccountsCache was a no-op against an in-flight fetch: a fetch that started before the invalidation still published its pre-invalidation snapshot afterwards, so the next read was served from cache and handed back exactly the data the caller asked to discard, with no second ARM call. A caller arriving in that window also joined the still in-flight pre-invalidation call through the constant singleflight key. Both are closed by an accountsGen counter that keys the singleflight call and gates the cache write. singleflight hands the leader's error to every waiter, so a caller whose own context was still live inherited a cancellation raised by whichever unrelated caller won the leader election. Retry once as leader when our own context is still healthy; our own cancellation stays terminal, so this cannot spin on a dead context. The unchecked type assertion on singleflight's untyped result now returns an error instead of panicking. Test fixes: the context-cancellation test passed with the guard it named deleted, because fakes that return ctx.Err() make every subscription fail and the all-failed guard produces the same error either way; its fakes now ignore the cancelled context so only the post-Wait ctx.Err() check can produce the error. The new cache tests read AZURE_SUBSCRIPTION_ID from the ambient environment and now neutralise it, the concurrent cold-cache test waits for every follower to enter GetAccounts rather than relying on a runtime.Gosched nudge, and the constructor test asserts the built client is actually stored. Each behaviour has a regression test verified to fail with its fix reverted. Refs #553 --- providers/azure/accounts_cache.go | 106 ++++++++++-- providers/azure/provider.go | 58 +++++-- providers/azure/provider_test.go | 158 +++++++++++++++++- .../recommendations_multi_subscription.go | 78 +++++++-- ...recommendations_multi_subscription_test.go | 114 ++++++++++++- 5 files changed, 466 insertions(+), 48 deletions(-) diff --git a/providers/azure/accounts_cache.go b/providers/azure/accounts_cache.go index 42f963e64..26625ba35 100644 --- a/providers/azure/accounts_cache.go +++ b/providers/azure/accounts_cache.go @@ -2,19 +2,28 @@ package azure import ( "context" + "errors" "fmt" "os" + "strconv" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions" "github.com/LeanerCloud/CUDly/pkg/common" ) -// accountsCacheSFKey is the single singleflight.Group key this cache ever +// accountsCacheSFKeyPrefix prefixes the singleflight.Group key this cache // uses. There is exactly one cached value per AzureProvider (the org-wide -// subscription list), so a constant key is sufficient to coalesce every -// concurrent cold-cache caller onto the same in-flight fetch. -const accountsCacheSFKey = "accounts" +// subscription list), so the key only has to distinguish cache generations -- +// see accountsSFKey. +const accountsCacheSFKeyPrefix = "accounts-gen-" + +// azureSubscriptionIDEnv is the environment variable that pins the default +// subscription when ProviderConfig carries none. Named rather than repeated +// as a literal because both resolveDefaultSubscription (which consumes it) +// and GetRecommendationsClient (which reports it back in an error) depend on +// it being the same variable. +const azureSubscriptionIDEnv = "AZURE_SUBSCRIPTION_ID" // getOrFetchAccounts returns the cached subscription list, populating it via // fetchAccounts on first use. Safe for concurrent callers: @@ -33,14 +42,69 @@ const accountsCacheSFKey = "accounts" // caller alongside GetServiceClient/GetRecommendationsClient which check // IsConfigured() themselves before resolving accounts). func (p *AzureProvider) getOrFetchAccounts(ctx context.Context) ([]common.Account, error) { + if cached := p.readCachedAccounts(); cached != nil { + return cached, nil + } + + accounts, err := p.fetchAccountsShared(ctx) + if err == nil { + return cloneAccounts(accounts), nil + } + + // singleflight hands the leader's error to every waiter, so a caller whose + // own context is still live can inherit a cancellation raised by whichever + // unrelated caller happened to win the leader election. That cancellation + // is not ours to obey: retry once, as leader this time. Our OWN + // cancellation stays terminal -- the ctx.Err() guard short-circuits it -- + // so this can never spin on a dead context, and the single retry bounds + // the work at two attempts. + if ctx.Err() != nil || !isContextError(err) { + return nil, err + } + accounts, err = p.fetchAccountsShared(ctx) + if err != nil { + return nil, err + } + return cloneAccounts(accounts), nil +} + +// isContextError reports whether err was produced by a context being +// cancelled or timing out. +func isContextError(err error) bool { + return errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) +} + +// readCachedAccounts returns an independent clone of the cached subscription +// list, or nil when the cache has not been populated yet. +func (p *AzureProvider) readCachedAccounts() []common.Account { p.accountsMu.RLock() - cached := p.cachedAccounts - p.accountsMu.RUnlock() - if cached != nil { - return cloneAccounts(cached), nil + defer p.accountsMu.RUnlock() + if p.cachedAccounts == nil { + return nil } + return cloneAccounts(p.cachedAccounts) +} - v, err, _ := p.accountsSF.Do(accountsCacheSFKey, func() (interface{}, error) { +// accountsSFKey returns the singleflight key for cache generation gen. +// +// Keying the in-flight call on the generation is what makes +// InvalidateAccountsCache meaningful under concurrency: a caller arriving +// after an invalidation uses a fresh key, so it starts a new ARM call instead +// of joining one that began before the invalidation and being handed back +// exactly the snapshot it just asked to discard. +func accountsSFKey(gen uint64) string { + return accountsCacheSFKeyPrefix + strconv.FormatUint(gen, 10) +} + +// fetchAccountsShared collapses concurrent cold-cache callers onto a single +// in-flight ARM subscriptions.List and returns the SHARED (un-cloned) slice. +// Callers must clone before handing the result outside the package. +func (p *AzureProvider) fetchAccountsShared(ctx context.Context) ([]common.Account, error) { + p.accountsMu.RLock() + gen := p.accountsGen + p.accountsMu.RUnlock() + + v, err, _ := p.accountsSF.Do(accountsSFKey(gen), func() (interface{}, error) { // Re-check: another goroutine's fetch may have populated the cache // between our RLock check above and this closure actually running // (e.g. it lost the race to become the singleflight leader). @@ -57,7 +121,13 @@ func (p *AzureProvider) getOrFetchAccounts(ctx context.Context) ([]common.Accoun } p.accountsMu.Lock() - p.cachedAccounts = accounts + // Publish only when no InvalidateAccountsCache landed while the ARM + // call was in flight. A fetch that started before an invalidation + // carries a pre-invalidation snapshot; writing it now would silently + // resurrect exactly the data the caller asked to discard. + if p.accountsGen == gen { + p.cachedAccounts = accounts + } p.accountsMu.Unlock() return accounts, nil @@ -65,7 +135,13 @@ func (p *AzureProvider) getOrFetchAccounts(ctx context.Context) ([]common.Accoun if err != nil { return nil, err } - return cloneAccounts(v.([]common.Account)), nil + accounts, ok := v.([]common.Account) + if !ok { + // singleflight.Do is untyped, so a future edit to the closure's return + // type would otherwise turn into a panic here. Surface it as an error. + return nil, fmt.Errorf("azure accounts cache: unexpected fetch result type %T", v) + } + return accounts, nil } // cloneAccounts returns a shallow copy of accounts backed by a fresh array. @@ -84,10 +160,16 @@ func cloneAccounts(accounts []common.Account) []common.Account { // for tests that need to assert cache-miss behavior; production callers // currently rely on the cache living for the lifetime of the AzureProvider // instance (one instance is constructed per collection/purchase run). +// +// Bumping accountsGen is what makes this safe against a concurrent in-flight +// fetch: the generation both invalidates that fetch's right to publish its +// result and moves later callers onto a fresh singleflight key, so no caller +// can be served a snapshot taken before this call returned. func (p *AzureProvider) InvalidateAccountsCache() { p.accountsMu.Lock() defer p.accountsMu.Unlock() p.cachedAccounts = nil + p.accountsGen++ } // fetchAccounts performs the actual ARM subscriptions.List call and resolves @@ -152,7 +234,7 @@ func resolveDefaultSubscription(accounts []common.Account, explicitSubID string) target := explicitSubID if target == "" { - target = os.Getenv("AZURE_SUBSCRIPTION_ID") + target = os.Getenv(azureSubscriptionIDEnv) } if target != "" { diff --git a/providers/azure/provider.go b/providers/azure/provider.go index e96230a1e..6565b0683 100644 --- a/providers/azure/provider.go +++ b/providers/azure/provider.go @@ -4,6 +4,7 @@ package azure import ( "context" "fmt" + "os" "sync" "time" @@ -111,8 +112,13 @@ type AzureProvider struct { // in-flight subscriptions.List: the ARM call runs once (not once per // caller) and, crucially, outside accountsMu so a slow or hung lookup // can never block readers of the cache. + // accountsGen is bumped by InvalidateAccountsCache. It keys the + // singleflight call and gates the cache write, so an ARM fetch that + // started before an invalidation can neither publish its stale snapshot + // nor be joined by a caller that arrived after it. accountsMu sync.RWMutex cachedAccounts []common.Account + accountsGen uint64 accountsSF singleflight.Group // cache subsystem implementation (getOrFetchAccounts, fetchAccounts, @@ -440,15 +446,30 @@ func (p *AzureProvider) newServiceClientForSubscription(service common.ServiceTy // // When no subscription is pinned, GetRecommendationsClient discovers every // subscription accessible to the authenticated principal (via the cached -// getOrFetchAccounts) and, when 2+ are visible, fans recommendation -// collection out across all of them via -// MultiSubscriptionRecommendationsClient. Azure has no organization-wide -// equivalent of AWS Cost Explorer's AccountScope=Linked -- the Consumption -// Reservation Recommendations and Advisor APIs are subscription-scoped -- so -// this client-side fan-out is what brings Azure to parity with the AWS -// provider's automatic whole-organization coverage. A single discovered -// subscription still returns the plain single-subscription client; no -// fan-out machinery is needed for one subscription. +// getOrFetchAccounts) and then narrows in the same order the rest of the +// provider does, so widening the scope is never a side effect of adding +// fan-out: +// +// 1. A default subscription resolvable from the discovered list -- the +// AZURE_SUBSCRIPTION_ID environment variable, or a lone visible +// subscription (see resolveDefaultSubscription) -- still scopes the +// client to that single subscription. Env-pinned callers keep the exact +// scope they had before org-wide fan-out existed; broadening them to +// every visible subscription would leak other subscriptions' data into a +// request that named one. +// 2. A configured AZURE_SUBSCRIPTION_ID that names a subscription this +// principal cannot see is an error, not a request for org-wide coverage. +// 3. Only when NO subscription was named at all -- an ambiguous +// multi-subscription principal with nothing configured, which previously +// produced the hard "multiple Azure subscriptions found; set +// AzureSubscriptionID or AZURE_SUBSCRIPTION_ID" error -- does the fan-out +// engage via MultiSubscriptionRecommendationsClient. +// +// Azure has no organization-wide equivalent of AWS Cost Explorer's +// AccountScope=Linked -- the Consumption Reservation Recommendations and +// Advisor APIs are subscription-scoped -- so this client-side fan-out is what +// brings Azure to parity with the AWS provider's automatic whole-organization +// coverage. func (p *AzureProvider) GetRecommendationsClient(ctx context.Context) (provider.RecommendationsClient, error) { if !p.IsConfigured() { return nil, fmt.Errorf("azure provider is not configured") @@ -465,10 +486,25 @@ func (p *AzureProvider) GetRecommendationsClient(ctx context.Context) (provider. if len(accounts) == 0 { return nil, fmt.Errorf("no Azure subscriptions found") } - if len(accounts) == 1 { - return NewRecommendationsClient(p.cred, accounts[0].ID) + // Step 1: an explicitly resolvable default still wins. This also covers + // the single-discovered-subscription case, which resolveDefaultSubscription + // marks as the default. + if defaultID := getDefaultSubscriptionID(accounts); defaultID != "" { + return NewRecommendationsClient(p.cred, defaultID) + } + + // A configured target that resolved to nothing means the caller named a + // subscription this principal cannot see. That is a misconfiguration, not + // a request for org-wide coverage: widening it to every visible + // subscription would answer a narrow question with other subscriptions' + // data. Fail loud, as this path did before fan-out existed. + if target := os.Getenv(azureSubscriptionIDEnv); target != "" { + return nil, fmt.Errorf( + "%s is set to %q, which is not among the %d subscriptions visible to this principal", + azureSubscriptionIDEnv, target, len(accounts)) } + // Step 2: scope is genuinely ambiguous -- fan out across the whole org. client, err := NewMultiSubscriptionRecommendationsClient(p.cred, accounts) if err != nil { return nil, err diff --git a/providers/azure/provider_test.go b/providers/azure/provider_test.go index 998756167..968ad31e3 100644 --- a/providers/azure/provider_test.go +++ b/providers/azure/provider_test.go @@ -3,6 +3,7 @@ package azure import ( "context" "errors" + "fmt" "runtime" "sync" "sync/atomic" @@ -1283,7 +1284,18 @@ func twoSubscriptionPages() *mockSubscriptionsClient { } } +// clearAzureSubscriptionEnv neutralizes AZURE_SUBSCRIPTION_ID for the calling +// test. resolveDefaultSubscription reads it via os.Getenv, so a developer or +// CI runner with it exported would otherwise flip IsDefault on one of the +// fixture subscriptions and change what the cache/fan-out tests observe -- +// failing them for a reason unrelated to the behavior they guard. +func clearAzureSubscriptionEnv(t *testing.T) { + t.Helper() + t.Setenv("AZURE_SUBSCRIPTION_ID", "") +} + func TestAzureProvider_GetAccounts_CacheHit(t *testing.T) { + clearAzureSubscriptionEnv(t) counting := &countingSubscriptionsClient{mockSubscriptionsClient: twoSubscriptionPages()} p := &AzureProvider{cred: &mockTokenCredential{}} @@ -1302,6 +1314,7 @@ func TestAzureProvider_GetAccounts_CacheHit(t *testing.T) { } func TestAzureProvider_GetAccounts_CacheHit_ReturnsIndependentCopies(t *testing.T) { + clearAzureSubscriptionEnv(t) p := &AzureProvider{cred: &mockTokenCredential{}} p.SetSubscriptionsClient(twoSubscriptionPages()) @@ -1315,6 +1328,7 @@ func TestAzureProvider_GetAccounts_CacheHit_ReturnsIndependentCopies(t *testing. } func TestAzureProvider_InvalidateAccountsCache(t *testing.T) { + clearAzureSubscriptionEnv(t) counting := &countingSubscriptionsClient{mockSubscriptionsClient: twoSubscriptionPages()} p := &AzureProvider{cred: &mockTokenCredential{}} @@ -1369,6 +1383,7 @@ func (c *gatedCountingSubscriptionsClient) NewListLocationsPager(subscriptionID // this test detects via the atomic counter. Run under -race to also catch any // unguarded shared-state access on the cold-cache path. func TestAzureProvider_GetAccounts_ConcurrentColdCache_SingleARMCall(t *testing.T) { + clearAzureSubscriptionEnv(t) gated := &gatedCountingSubscriptionsClient{ inner: twoSubscriptionPages(), firstInFlight: make(chan struct{}), @@ -1380,10 +1395,21 @@ func TestAzureProvider_GetAccounts_ConcurrentColdCache_SingleARMCall(t *testing. const n = 10 var wg sync.WaitGroup + // followersReady counts down once per follower that has entered + // GetAccounts. Waiting on it before releasing the held fetch is what makes + // the assertion deterministic: with only a runtime.Gosched() nudge, a + // follower that had not yet reached the cold-cache path when the leader + // finished would find a warm cache and never issue its own ARM call -- + // so a genuinely broken (no single-flight) implementation could still + // report exactly one call and pass. + var followersReady sync.WaitGroup errs := make(chan error, n) results := make(chan []common.Account, n) - worker := func() { + worker := func(signalReady bool) { defer wg.Done() + if signalReady { + followersReady.Done() + } accts, err := p.GetAccounts(context.Background()) errs <- err results <- accts @@ -1392,15 +1418,18 @@ func TestAzureProvider_GetAccounts_ConcurrentColdCache_SingleARMCall(t *testing. // Launch the leader and wait until its ARM fetch is actually in-flight // before launching the followers, so they observe a cold cache. wg.Add(1) - go worker() + go worker(false) <-gated.firstInFlight + followersReady.Add(n - 1) for i := 1; i < n; i++ { wg.Add(1) - go worker() + go worker(true) } - // Nudge the followers onto the cache/single-flight path, then release the - // held fetch so everything can complete. + // Wait until every follower goroutine is running and about to call + // GetAccounts, then give the scheduler a chance to drive them into the + // single-flight path before releasing the held fetch. + followersReady.Wait() for i := 0; i < n; i++ { runtime.Gosched() } @@ -1421,7 +1450,126 @@ func TestAzureProvider_GetAccounts_ConcurrentColdCache_SingleARMCall(t *testing. "concurrent cold-cache GetAccounts must issue exactly one ARM list call (single-flight)") } +// gatedGenerationSubscriptionsClient holds its first ARM list call open (like +// gatedCountingSubscriptionsClient) but serves a DIFFERENT subscription on +// each call, so a test can tell a re-fetched result apart from a resurrected +// pre-invalidation snapshot. +type gatedGenerationSubscriptionsClient struct { + calls atomic.Int64 + firstOnce sync.Once + firstInFlight chan struct{} + release chan struct{} +} + +func (c *gatedGenerationSubscriptionsClient) NewListPager(_ *armsubscriptions.ClientListOptions) SubscriptionsPager { + n := c.calls.Add(1) + c.firstOnce.Do(func() { + close(c.firstInFlight) + <-c.release + }) + id := fmt.Sprintf("sub-gen-%d", n) + name := fmt.Sprintf("Subscription generation %d", n) + return &mockSubscriptionsPager{ + pages: []armsubscriptions.ClientListResponse{ + {SubscriptionListResult: armsubscriptions.SubscriptionListResult{ + Value: []*armsubscriptions.Subscription{{SubscriptionID: &id, DisplayName: &name}}, + }}, + }, + } +} + +func (c *gatedGenerationSubscriptionsClient) NewListLocationsPager(_ string, _ *armsubscriptions.ClientListLocationsOptions) LocationsPager { + return nil +} + +// TestAzureProvider_InvalidateAccountsCache_DuringInFlightFetch guards the +// cache-generation check. +// +// Interleaving: a fetch is in flight (holding the pre-invalidation snapshot) +// when InvalidateAccountsCache lands. Without the generation gate the +// in-flight fetch publishes its now-stale snapshot after the invalidation, so +// the next read is served from cache and the caller is handed back exactly +// the data it asked to discard -- silently, with no second ARM call. +func TestAzureProvider_InvalidateAccountsCache_DuringInFlightFetch(t *testing.T) { + clearAzureSubscriptionEnv(t) + gated := &gatedGenerationSubscriptionsClient{ + firstInFlight: make(chan struct{}), + release: make(chan struct{}), + } + + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(gated) + + type fetchResult struct { + accounts []common.Account + err error + } + done := make(chan fetchResult, 1) + go func() { + accts, err := p.GetAccounts(context.Background()) + done <- fetchResult{accounts: accts, err: err} + }() + + // The first fetch is now blocked inside the ARM call, holding generation-1 + // data. Invalidate while it is still in flight. + <-gated.firstInFlight + p.InvalidateAccountsCache() + close(gated.release) + + first := <-done + require.NoError(t, first.err) + require.Len(t, first.accounts, 1) + assert.Equal(t, "sub-gen-1", first.accounts[0].ID) + + // The invalidated snapshot must not have been published: this read has to + // re-hit ARM and observe the current subscription list. + second, err := p.GetAccounts(context.Background()) + require.NoError(t, err) + assert.Equal(t, int64(2), gated.calls.Load(), + "a fetch that started before InvalidateAccountsCache must not populate the cache") + require.Len(t, second, 1) + assert.Equal(t, "sub-gen-2", second[0].ID, + "read after invalidation must see fresh data, not the resurrected pre-invalidation snapshot") +} + func TestAzureProvider_GetRecommendationsClient_MultiSubscriptionFanOut(t *testing.T) { + clearAzureSubscriptionEnv(t) + + // Regression guard for the scoping rule: fan-out must be the fallback for + // an ambiguous principal, never an upgrade applied to a caller that + // already named its subscription. A principal that can see sub-1 and + // sub-2 but pinned sub-2 via AZURE_SUBSCRIPTION_ID must keep getting + // sub-2 only; returning the fan-out client here would hand that caller + // sub-1's recommendations too. + t.Run("AZURE_SUBSCRIPTION_ID still scopes to one subscription", func(t *testing.T) { + t.Setenv("AZURE_SUBSCRIPTION_ID", "sub-2") + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(twoSubscriptionPages()) + + client, err := p.GetRecommendationsClient(context.Background()) + require.NoError(t, err) + require.IsType(t, &RecommendationsClientAdapter{}, client, + "an env-pinned subscription must not be widened to an org-wide fan-out") + assert.Equal(t, "sub-2", client.(*RecommendationsClientAdapter).subscriptionID) + }) + + // A configured AZURE_SUBSCRIPTION_ID that names a subscription this + // principal cannot see is a misconfiguration. Answering it with an + // org-wide fan-out would hand the caller every OTHER subscription's data + // in response to a request that named one, so this must stay the hard + // error it was before fan-out existed. + t.Run("AZURE_SUBSCRIPTION_ID naming an invisible subscription errors", func(t *testing.T) { + t.Setenv("AZURE_SUBSCRIPTION_ID", "sub-not-visible") + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(twoSubscriptionPages()) + + client, err := p.GetRecommendationsClient(context.Background()) + require.Error(t, err, "an unresolvable explicit subscription must not silently widen to org-wide fan-out") + assert.Nil(t, client) + assert.Contains(t, err.Error(), "sub-not-visible") + assert.Contains(t, err.Error(), "not among the 2 subscriptions visible") + }) + t.Run("multi-subscription returns MultiSubscriptionRecommendationsClient", func(t *testing.T) { p := &AzureProvider{cred: &mockTokenCredential{}} p.SetSubscriptionsClient(twoSubscriptionPages()) diff --git a/providers/azure/recommendations_multi_subscription.go b/providers/azure/recommendations_multi_subscription.go index c5d3e7392..643e40cd1 100644 --- a/providers/azure/recommendations_multi_subscription.go +++ b/providers/azure/recommendations_multi_subscription.go @@ -33,8 +33,9 @@ type subscriptionClient struct { } // MultiSubscriptionRecommendationsClient fans recommendation collection out -// across every Azure subscription accessible to the authenticated -// principal. +// across the Azure subscriptions accessible to the authenticated principal -- +// every one of them by default, or the subset named by +// RecommendationParams.AccountFilter (see selectSubscriptions). // // Azure has no organization-wide equivalent of AWS Cost Explorer's // AccountScope=Linked: the Consumption Reservation Recommendations and @@ -68,8 +69,10 @@ func NewMultiSubscriptionRecommendationsClient(cred azcore.TokenCredential, acco return &MultiSubscriptionRecommendationsClient{subscriptions: subscriptions}, nil } -// GetRecommendations fans params out to every subscription concurrently -// (errgroup) and merges the results. +// GetRecommendations fans params out concurrently (errgroup) to the +// subscriptions selected by selectSubscriptions -- every accessible +// subscription unless params.AccountFilter narrows it -- and merges the +// results. // // Error isolation mirrors RecommendationsClientAdapter.GetRecommendations: // each per-subscription goroutine captures its own error and returns nil to @@ -94,11 +97,16 @@ func (m *MultiSubscriptionRecommendationsClient) GetRecommendations(ctx context. return nil, fmt.Errorf("params cannot be nil") } - results := make([][]common.Recommendation, len(m.subscriptions)) - errs := make([]error, len(m.subscriptions)) + targets, err := m.selectSubscriptions(params.AccountFilter) + if err != nil { + return nil, err + } + + results := make([][]common.Recommendation, len(targets)) + errs := make([]error, len(targets)) g, gctx := errgroup.WithContext(ctx) - for i, sub := range m.subscriptions { + for i, sub := range targets { i, sub := i, sub g.Go(func() error { recs, err := sub.client.GetRecommendations(gctx, params) @@ -119,13 +127,57 @@ func (m *MultiSubscriptionRecommendationsClient) GetRecommendations(ctx context. return nil, err } - return m.mergeResults(results, errs) + return mergeSubscriptionResults(targets, results, errs) +} + +// selectSubscriptions narrows the fan-out to params.AccountFilter. +// +// AccountFilter is a scoping control, not a display convenience: the AWS +// provider applies it to every recommendation it returns (filterByAccounts in +// providers/aws/service_client.go), so a caller that scopes a request to a +// subset of accounts must not be handed another account's data by the Azure +// path either. Before org-wide fan-out existed this was moot -- a +// subscription-scoped client could only ever return its own subscription -- +// but a client covering every visible subscription has to honour the filter +// or it silently widens the caller's scope. +// +// Filtering BEFORE the fan-out (rather than discarding rows afterwards, as +// AWS does) also avoids issuing ARM calls against subscriptions the caller +// never asked about. +// +// An empty filter means "every visible subscription" -- the org-wide default +// this client exists to provide. A non-empty filter that matches nothing is +// an error rather than an empty result: returning zero recommendations would +// be indistinguishable from "these subscriptions have no savings available". +func (m *MultiSubscriptionRecommendationsClient) selectSubscriptions(filter []string) ([]subscriptionClient, error) { + if len(filter) == 0 { + return m.subscriptions, nil + } + + wanted := make(map[string]struct{}, len(filter)) + for _, id := range filter { + wanted[id] = struct{}{} + } + + selected := make([]subscriptionClient, 0, len(m.subscriptions)) + for _, sub := range m.subscriptions { + if _, ok := wanted[sub.subscriptionID]; ok { + selected = append(selected, sub) + } + } + if len(selected) == 0 { + return nil, fmt.Errorf( + "azure multi-subscription recommendations: account filter %v matches none of the %d accessible subscriptions", + filter, len(m.subscriptions)) + } + return selected, nil } -// mergeResults concatenates successful per-subscription results, logging a -// warning for each subscription that failed, and applies the +// mergeSubscriptionResults concatenates successful per-subscription results, +// logging a warning for each subscription that failed, and applies the // all-attempted-failed guard described in GetRecommendations' doc comment. -func (m *MultiSubscriptionRecommendationsClient) mergeResults(results [][]common.Recommendation, errs []error) ([]common.Recommendation, error) { +// subs, results and errs are index-aligned. +func mergeSubscriptionResults(subs []subscriptionClient, results [][]common.Recommendation, errs []error) ([]common.Recommendation, error) { total := 0 for _, r := range results { total += len(r) @@ -138,13 +190,13 @@ func (m *MultiSubscriptionRecommendationsClient) mergeResults(results [][]common if err != nil { failures++ lastErr = err - logging.Warnf("Azure subscription %s recommendations: %v", m.subscriptions[i].subscriptionID, err) + logging.Warnf("Azure subscription %s recommendations: %v", subs[i].subscriptionID, err) continue } out = append(out, results[i]...) } - if failures > 0 && failures == len(m.subscriptions) { + if failures > 0 && failures == len(subs) { return nil, fmt.Errorf("all %d Azure subscriptions failed to return recommendations: %w", failures, lastErr) } return out, nil diff --git a/providers/azure/recommendations_multi_subscription_test.go b/providers/azure/recommendations_multi_subscription_test.go index 1a8ccb8ab..51861a53f 100644 --- a/providers/azure/recommendations_multi_subscription_test.go +++ b/providers/azure/recommendations_multi_subscription_test.go @@ -3,6 +3,7 @@ package azure import ( "context" "errors" + "sync/atomic" "testing" "github.com/Azure/azure-sdk-for-go/sdk/azcore" @@ -17,14 +18,25 @@ import ( // fan-out tests, letting each fake subscription's response be controlled // independently of the others. type fakeRecommendationsClient struct { - recs []common.Recommendation - err error + recs []common.Recommendation + err error + // ignoreCtx makes the fake return its canned response even for a + // cancelled context, modelling a per-subscription client that fails to + // observe cancellation (an SDK layer that answers from its own cache, or + // a call that already completed before the parent context died). This is + // the only shape in which the fan-out's own post-Wait ctx.Err() check is + // load-bearing: a fake that returns ctx.Err() itself makes every + // subscription fail, and the all-subscriptions-failed guard produces a + // context.Canceled error whether or not that check exists. + ignoreCtx bool gotParams *common.RecommendationParams + calls atomic.Int64 } func (f *fakeRecommendationsClient) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { + f.calls.Add(1) f.gotParams = params - if err := ctx.Err(); err != nil { + if err := ctx.Err(); err != nil && !f.ignoreCtx { // Respect cancellation like a real ARM client would (the SDK's // underlying HTTP transport checks ctx before issuing the request). return nil, err @@ -73,9 +85,11 @@ func TestNewMultiSubscriptionRecommendationsClient_EmptyAccounts(t *testing.T) { func TestNewMultiSubscriptionRecommendationsClient_BuildsClientsPerAccount(t *testing.T) { accounts := twoTestAccounts() + fake1 := &fakeRecommendationsClient{recs: []common.Recommendation{{Account: "sub-1"}}} + fake2 := &fakeRecommendationsClient{recs: []common.Recommendation{{Account: "sub-2"}}} withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ - "sub-1": {recs: []common.Recommendation{{Account: "sub-1"}}}, - "sub-2": {recs: []common.Recommendation{{Account: "sub-2"}}}, + "sub-1": fake1, + "sub-2": fake2, }) client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, accounts) @@ -83,6 +97,10 @@ func TestNewMultiSubscriptionRecommendationsClient_BuildsClientsPerAccount(t *te require.Len(t, client.subscriptions, 2) assert.Equal(t, "sub-1", client.subscriptions[0].subscriptionID) assert.Equal(t, "sub-2", client.subscriptions[1].subscriptionID) + // Assert the built client is actually stored against its subscription -- + // without this the test passes even if the client field is never set. + assert.Same(t, fake1, client.subscriptions[0].client) + assert.Same(t, fake2, client.subscriptions[1].client) } func TestNewMultiSubscriptionRecommendationsClient_ClientConstructionFailurePropagates(t *testing.T) { @@ -154,11 +172,20 @@ func TestMultiSubscriptionRecommendationsClient_GetRecommendations_NilParams(t * assert.Nil(t, recs) } +// TestMultiSubscriptionRecommendationsClient_GetRecommendations_PropagatesContextCancellation +// guards the post-Wait ctx.Err() check specifically. +// +// The fakes here deliberately IGNORE the cancelled context and return +// results, so the only thing that can turn this call into an error is the +// fan-out's own ctx.Err() check. Deleting that check makes this test fail +// with a nil error and two merged recommendations -- which is exactly the +// bug it exists to catch: a cancelled request quietly yielding data as if it +// had completed normally. func TestMultiSubscriptionRecommendationsClient_GetRecommendations_PropagatesContextCancellation(t *testing.T) { accounts := twoTestAccounts() withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ - "sub-1": {recs: []common.Recommendation{{Account: "sub-1"}}}, - "sub-2": {recs: []common.Recommendation{{Account: "sub-2"}}}, + "sub-1": {recs: []common.Recommendation{{Account: "sub-1"}}, ignoreCtx: true}, + "sub-2": {recs: []common.Recommendation{{Account: "sub-2"}}, ignoreCtx: true}, }) client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, accounts) @@ -173,6 +200,79 @@ func TestMultiSubscriptionRecommendationsClient_GetRecommendations_PropagatesCon assert.Nil(t, recs) } +// TestMultiSubscriptionRecommendationsClient_GetRecommendations_AccountFilterScopesFanOut +// is the regression test for the cross-subscription bleed: a caller that +// scopes a request to one subscription must not receive another +// subscription's recommendations, and the unasked-for subscription must not +// even be queried. +func TestMultiSubscriptionRecommendationsClient_GetRecommendations_AccountFilterScopesFanOut(t *testing.T) { + accounts := twoTestAccounts() + fake1 := &fakeRecommendationsClient{recs: []common.Recommendation{{Account: "sub-1", Service: common.ServiceCompute}}} + fake2 := &fakeRecommendationsClient{recs: []common.Recommendation{{Account: "sub-2", Service: common.ServiceCache}}} + withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ + "sub-1": fake1, + "sub-2": fake2, + }) + + client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, accounts) + require.NoError(t, err) + + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{ + AccountFilter: []string{"sub-1"}, + }) + require.NoError(t, err) + assert.Equal(t, []common.Recommendation{{Account: "sub-1", Service: common.ServiceCompute}}, recs, + "only the filtered subscription's recommendations may be returned") + assert.Equal(t, int64(1), fake1.calls.Load(), "the filtered-in subscription must be queried") + assert.Equal(t, int64(0), fake2.calls.Load(), "a subscription outside the filter must not be queried at all") +} + +// An AccountFilter naming no accessible subscription must error rather than +// return an empty slice: an empty result is indistinguishable from "these +// subscriptions have no savings available". +func TestMultiSubscriptionRecommendationsClient_GetRecommendations_AccountFilterMatchesNothing(t *testing.T) { + accounts := twoTestAccounts() + fake1 := &fakeRecommendationsClient{recs: []common.Recommendation{{Account: "sub-1"}}} + fake2 := &fakeRecommendationsClient{recs: []common.Recommendation{{Account: "sub-2"}}} + withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ + "sub-1": fake1, + "sub-2": fake2, + }) + + client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, accounts) + require.NoError(t, err) + + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{ + AccountFilter: []string{"sub-not-visible"}, + }) + require.Error(t, err) + assert.Nil(t, recs) + assert.Contains(t, err.Error(), "matches none of the 2 accessible subscriptions") + assert.Equal(t, int64(0), fake1.calls.Load()) + assert.Equal(t, int64(0), fake2.calls.Load()) +} + +// An empty AccountFilter keeps the org-wide default: every visible +// subscription is queried. +func TestMultiSubscriptionRecommendationsClient_GetRecommendations_EmptyAccountFilterFansOutToAll(t *testing.T) { + accounts := twoTestAccounts() + fake1 := &fakeRecommendationsClient{recs: []common.Recommendation{{Account: "sub-1"}}} + fake2 := &fakeRecommendationsClient{recs: []common.Recommendation{{Account: "sub-2"}}} + withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ + "sub-1": fake1, + "sub-2": fake2, + }) + + client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, accounts) + require.NoError(t, err) + + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) + require.NoError(t, err) + assert.ElementsMatch(t, []common.Recommendation{{Account: "sub-1"}, {Account: "sub-2"}}, recs) + assert.Equal(t, int64(1), fake1.calls.Load()) + assert.Equal(t, int64(1), fake2.calls.Load()) +} + func TestMultiSubscriptionRecommendationsClient_GetRecommendationsForService_PassesServiceFilter(t *testing.T) { accounts := twoTestAccounts() fake1 := &fakeRecommendationsClient{} From f1bff826487a89cfe2268d811344859cf3ad4eeb Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 19:42:54 +0200 Subject: [PATCH 04/11] fix(scheduler): reject Azure accounts with no subscription ID collectAzureForAccount tags every recommendation it returns with the registered account's UUID, so the provider has to be pinned to that account's subscription. It built the provider from acct.AzureSubscriptionID without checking it was set, and validateCloudAccountRequest does not require the field, so an azure row created without one produced an unpinned provider. That was harmless while an unpinned provider resolved a single subscription or errored. With the org-wide fan-out it instead collects from every subscription the credential can see, and those results are then filed under this one account, where anyone authorised for it can read them. Reject the misconfigured row by name instead. --- internal/scheduler/scheduler.go | 11 ++++++++++ internal/scheduler/scheduler_test.go | 30 ++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 3697f729b..b021feedb 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -770,6 +770,17 @@ func (s *Scheduler) collectAzureRecommendations(ctx context.Context, _ *config.G } func (s *Scheduler) collectAzureForAccount(ctx context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, error) { + // Every recommendation returned below is tagged with THIS account's UUID + // (see tagAccount at the end of this function), so the provider must be + // pinned to this account's subscription. An empty AzureSubscriptionID + // leaves the provider unpinned, and an unpinned provider now fans out + // across every subscription the credential can see -- which would file + // other subscriptions' recommendations under this account and expose them + // to anyone authorized for it. Fail loud instead; the row is misconfigured. + if acct.AzureSubscriptionID == "" { + return nil, fmt.Errorf("cloud account %s has no azure_subscription_id configured", acct.ID) + } + azCred, err := credentials.ResolveAzureTokenCredentialWithOpts(ctx, &acct, s.credStore, credentials.AzureResolveOptions{ Signer: s.oidcSigner, IssuerURL: s.oidcIssuerURL, diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index a96dd8aab..ba1a4eec6 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -1647,6 +1647,36 @@ func TestScheduler_CollectAzureRecommendations_AllAccountsFailLoud(t *testing.T) "the failed account must not land in SucceededAccountIDs (stale-row eviction guard)") } +// An Azure cloud_accounts row with no azure_subscription_id must be rejected +// before a provider is built. +// +// collectAzureForAccount tags every recommendation it returns with THIS +// account's UUID, so the provider has to be pinned to this account's +// subscription. Leaving AzureSubscriptionID empty leaves the provider +// unpinned, and an unpinned provider fans out across every subscription the +// credential can see -- filing other subscriptions' recommendations under +// this account, where anyone authorized for it can read them. The row is +// misconfigured; fail loud and name the missing field rather than collecting +// data that will be attributed to the wrong account. +func TestScheduler_CollectAzureForAccount_MissingSubscriptionIDFailsLoud(t *testing.T) { + ctx := context.Background() + scheduler := &Scheduler{config: new(MockConfigStore)} + + recs, err := scheduler.collectAzureForAccount(ctx, config.CloudAccount{ + ID: "az-no-sub", + Provider: "azure", + AzureAuthMode: "managed_identity", + Enabled: true, + // AzureSubscriptionID deliberately empty. + }) + + require.Error(t, err) + assert.Nil(t, recs) + assert.Contains(t, err.Error(), "no azure_subscription_id configured", + "an unpinned Azure account must be rejected by name, not fall through to an unscoped org-wide collection") + assert.Contains(t, err.Error(), "az-no-sub", "the error must identify the misconfigured account") +} + // Test GCP recommendations with no accounts — should skip gracefully. func TestScheduler_CollectGCPRecommendations_NoAccounts(t *testing.T) { ctx := context.Background() From dec280698d06042cbfd573d83af8e572a70ec0d1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 19:52:54 +0200 Subject: [PATCH 05/11] fix(azure): drop the accounts cache when the credential is swapped SetCredential is used in production, not only in tests: the scheduler and purchase-execution paths construct the provider and then install per-account federated credentials. The subscription-list cache added in this PR records what the previous credential could see, so leaving it in place across a swap would report subscriptions the new principal may have no access to, and GetRecommendationsClient would fan out across them. Every caller today installs the credential before the first accounts fetch, so this is a guard against a future reordering rather than a live leak. SetSubscriptionsClient gets the same treatment for the same reason. --- providers/azure/provider.go | 19 ++++++++++-- providers/azure/provider_test.go | 53 ++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/providers/azure/provider.go b/providers/azure/provider.go index 6565b0683..ac7b932eb 100644 --- a/providers/azure/provider.go +++ b/providers/azure/provider.go @@ -166,9 +166,14 @@ func resolveAzureSubscriptionID(config *provider.ProviderConfig) string { return config.Profile } -// SetSubscriptionsClient sets the subscriptions client (for testing) +// SetSubscriptionsClient sets the subscriptions client (for testing). +// +// Drops any cached subscription list: the cache holds what the PREVIOUS +// client returned, and serving that after the client is swapped would answer +// with a different source's subscriptions. func (p *AzureProvider) SetSubscriptionsClient(client SubscriptionsClient) { p.subscriptionsClient = client + p.InvalidateAccountsCache() } // SetCredentialProvider sets the credential provider (for testing) @@ -176,9 +181,19 @@ func (p *AzureProvider) SetCredentialProvider(credProvider CredentialProvider) { p.credProvider = credProvider } -// SetCredential sets the credential directly (for testing) +// SetCredential sets the credential directly. +// +// Also used in production (the scheduler and purchase-execution paths +// construct the provider and then install per-account federated credentials), +// so it must drop any cached subscription list: that cache is the set of +// subscriptions the PREVIOUS credential could see. Serving it to the new +// credential would report subscriptions this principal may have no access to, +// and -- via GetRecommendationsClient's fan-out -- fan out across them. +// Today every caller installs the credential before the first accounts fetch, +// so this is a guard against a future reordering rather than a live leak. func (p *AzureProvider) SetCredential(cred azcore.TokenCredential) { p.cred = cred + p.InvalidateAccountsCache() } // Name returns the provider name diff --git a/providers/azure/provider_test.go b/providers/azure/provider_test.go index 968ad31e3..6ba23909c 100644 --- a/providers/azure/provider_test.go +++ b/providers/azure/provider_test.go @@ -1450,6 +1450,59 @@ func TestAzureProvider_GetAccounts_ConcurrentColdCache_SingleARMCall(t *testing. "concurrent cold-cache GetAccounts must issue exactly one ARM list call (single-flight)") } +// Swapping the credential or the subscriptions client must drop the cached +// subscription list. The cache records what the PREVIOUS credential/client +// could see; serving it afterwards would report subscriptions the new +// principal may have no access to, and GetRecommendationsClient would then +// fan out across them. +func TestAzureProvider_CacheDroppedOnCredentialOrClientSwap(t *testing.T) { + clearAzureSubscriptionEnv(t) + + t.Run("SetCredential invalidates", func(t *testing.T) { + counting := &countingSubscriptionsClient{mockSubscriptionsClient: twoSubscriptionPages()} + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(counting) + + _, err := p.GetAccounts(context.Background()) + require.NoError(t, err) + require.Equal(t, int64(1), counting.calls.Load()) + + p.SetCredential(&mockTokenCredential{}) + + _, err = p.GetAccounts(context.Background()) + require.NoError(t, err) + assert.Equal(t, int64(2), counting.calls.Load(), + "a credential swap must re-resolve the subscription list, not reuse the old credential's") + }) + + t.Run("SetSubscriptionsClient invalidates", func(t *testing.T) { + first := &countingSubscriptionsClient{mockSubscriptionsClient: twoSubscriptionPages()} + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(first) + + _, err := p.GetAccounts(context.Background()) + require.NoError(t, err) + + soloID, soloName := "sub-solo", "Solo Subscription" + p.SetSubscriptionsClient(&mockSubscriptionsClient{ + listPagerFunc: func(_ *armsubscriptions.ClientListOptions) SubscriptionsPager { + return &mockSubscriptionsPager{ + pages: []armsubscriptions.ClientListResponse{ + {SubscriptionListResult: armsubscriptions.SubscriptionListResult{ + Value: []*armsubscriptions.Subscription{{SubscriptionID: &soloID, DisplayName: &soloName}}, + }}, + }, + } + }, + }) + + accts, err := p.GetAccounts(context.Background()) + require.NoError(t, err) + require.Len(t, accts, 1, "the swapped-in client's subscriptions must be returned, not the cached ones") + assert.Equal(t, soloID, accts[0].ID) + }) +} + // gatedGenerationSubscriptionsClient holds its first ARM list call open (like // gatedCountingSubscriptionsClient) but serves a DIFFERENT subscription on // each call, so a test can tell a re-fetched result apart from a resurrected From 24b202930548b733f6b15456f9912189210ca9c8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 21:49:28 +0200 Subject: [PATCH 06/11] fix(azure): validate AZURE_SUBSCRIPTION_ID before default resolution CodeRabbit finding. GetRecommendationsClient consulted getDefaultSubscriptionID before validating the configured target, and resolveDefaultSubscription's rule 3 marks a lone visible subscription as the default even when a target was configured and did not match it. So with AZURE_SUBSCRIPTION_ID naming a subscription the credential cannot see and exactly one subscription visible, that one subscription was returned as the default and the fail-loud check further down was unreachable. An operator who typo'd the variable, or whose credential lost access to the intended subscription, silently collected against the wrong subscription instead of getting an error: a misconfiguration answered with a plausible wrong result. Validate the target against the discovered accounts first, via a direct membership check rather than IsDefault, so it fails loud regardless of how many subscriptions are visible. A target that IS visible is honoured directly. The previous test only covered two visible subscriptions, where rule 3 never fires; the new subtest pins the single-visible-subscription case and fails against the old ordering. --- providers/azure/accounts_cache.go | 15 +++++++++++ providers/azure/provider.go | 37 ++++++++++++++++---------- providers/azure/provider_test.go | 43 +++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 13 deletions(-) diff --git a/providers/azure/accounts_cache.go b/providers/azure/accounts_cache.go index 26625ba35..c7c994dd6 100644 --- a/providers/azure/accounts_cache.go +++ b/providers/azure/accounts_cache.go @@ -255,6 +255,21 @@ func resolveDefaultSubscription(accounts []common.Account, explicitSubID string) } } +// accountsContain reports whether id is one of the discovered subscriptions. +// +// Used to validate an explicitly configured subscription against what the +// principal can actually see, without going through IsDefault -- which +// resolveDefaultSubscription may have set on a DIFFERENT subscription via its +// single-visible-subscription rule. +func accountsContain(accounts []common.Account, id string) bool { + for i := range accounts { + if accounts[i].ID == id { + return true + } + } + return false +} + // getDefaultSubscriptionID returns the ID of the default subscription from a // pre-fetched account list, or an empty string when no account is marked // default (e.g. ambiguous multi-subscription tenants with no explicit config). diff --git a/providers/azure/provider.go b/providers/azure/provider.go index ac7b932eb..ad3d5a76d 100644 --- a/providers/azure/provider.go +++ b/providers/azure/provider.go @@ -501,25 +501,36 @@ func (p *AzureProvider) GetRecommendationsClient(ctx context.Context) (provider. if len(accounts) == 0 { return nil, fmt.Errorf("no Azure subscriptions found") } - // Step 1: an explicitly resolvable default still wins. This also covers + // Step 1: an explicitly configured target is validated against the + // discovered subscriptions FIRST, before any default resolution. + // + // This ordering is load-bearing. getDefaultSubscriptionID reads the + // IsDefault flags set by resolveDefaultSubscription, whose rule 3 marks a + // lone visible subscription as the default even when a target was + // configured and did not match it. Consulting that result first would let + // an invisible target (an operator typo, or a credential that lost access + // to the intended subscription) silently resolve to whichever single + // subscription happens to be visible -- a misconfiguration answered with + // a plausible wrong subscription instead of an error. Validating the + // target up front makes it fail loud regardless of how many subscriptions + // are visible. + if target := os.Getenv(azureSubscriptionIDEnv); target != "" { + if !accountsContain(accounts, target) { + return nil, fmt.Errorf( + "%s is set to %q, which is not among the %d subscriptions visible to this principal", + azureSubscriptionIDEnv, target, len(accounts)) + } + return NewRecommendationsClient(p.cred, target) + } + + // Step 2: no explicit target, but a default may still resolve -- notably // the single-discovered-subscription case, which resolveDefaultSubscription // marks as the default. if defaultID := getDefaultSubscriptionID(accounts); defaultID != "" { return NewRecommendationsClient(p.cred, defaultID) } - // A configured target that resolved to nothing means the caller named a - // subscription this principal cannot see. That is a misconfiguration, not - // a request for org-wide coverage: widening it to every visible - // subscription would answer a narrow question with other subscriptions' - // data. Fail loud, as this path did before fan-out existed. - if target := os.Getenv(azureSubscriptionIDEnv); target != "" { - return nil, fmt.Errorf( - "%s is set to %q, which is not among the %d subscriptions visible to this principal", - azureSubscriptionIDEnv, target, len(accounts)) - } - - // Step 2: scope is genuinely ambiguous -- fan out across the whole org. + // Step 3: scope is genuinely ambiguous -- fan out across the whole org. client, err := NewMultiSubscriptionRecommendationsClient(p.cred, accounts) if err != nil { return nil, err diff --git a/providers/azure/provider_test.go b/providers/azure/provider_test.go index 6ba23909c..4216b5a4d 100644 --- a/providers/azure/provider_test.go +++ b/providers/azure/provider_test.go @@ -1623,6 +1623,49 @@ func TestAzureProvider_GetRecommendationsClient_MultiSubscriptionFanOut(t *testi assert.Contains(t, err.Error(), "not among the 2 subscriptions visible") }) + // The single-visible-subscription case is the dangerous one, and the + // reason the target has to be validated BEFORE getDefaultSubscriptionID: + // resolveDefaultSubscription's rule 3 marks a lone subscription as the + // default even when a target was configured and did not match it. Reading + // that default first would resolve an invisible target to whichever one + // subscription happens to be visible and collect against it silently. + t.Run("AZURE_SUBSCRIPTION_ID invisible with one visible subscription errors", func(t *testing.T) { + t.Setenv("AZURE_SUBSCRIPTION_ID", "sub-not-visible") + soloID, soloName := "sub-solo", "Solo Subscription" + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(&mockSubscriptionsClient{ + listPagerFunc: func(_ *armsubscriptions.ClientListOptions) SubscriptionsPager { + return &mockSubscriptionsPager{ + pages: []armsubscriptions.ClientListResponse{ + {SubscriptionListResult: armsubscriptions.SubscriptionListResult{ + Value: []*armsubscriptions.Subscription{{SubscriptionID: &soloID, DisplayName: &soloName}}, + }}, + }, + } + }, + }) + + client, err := p.GetRecommendationsClient(context.Background()) + require.Error(t, err, + "an invisible configured subscription must not silently resolve to the one visible subscription") + assert.Nil(t, client) + assert.Contains(t, err.Error(), "sub-not-visible") + assert.Contains(t, err.Error(), "not among the 1 subscriptions visible") + }) + + // The happy path for the same branch: a target the principal CAN see is + // honoured, and scopes the client to exactly that subscription. + t.Run("AZURE_SUBSCRIPTION_ID matching a visible subscription is honoured", func(t *testing.T) { + t.Setenv("AZURE_SUBSCRIPTION_ID", "sub-1") + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(twoSubscriptionPages()) + + client, err := p.GetRecommendationsClient(context.Background()) + require.NoError(t, err) + require.IsType(t, &RecommendationsClientAdapter{}, client) + assert.Equal(t, "sub-1", client.(*RecommendationsClientAdapter).subscriptionID) + }) + t.Run("multi-subscription returns MultiSubscriptionRecommendationsClient", func(t *testing.T) { p := &AzureProvider{cred: &mockTokenCredential{}} p.SetSubscriptionsClient(twoSubscriptionPages()) From 33bc452b020acc7e4be10b089441d9dc1f511048 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 22:03:29 +0200 Subject: [PATCH 07/11] fix(azure): report partial multi-subscription sweeps as partial mergeSubscriptionResults returned a nil error whenever at least one subscription succeeded, so 3 of 50 subscriptions failing handed back 47 subscriptions' recommendations indistinguishably from a complete sweep. The only signal was a log line, which no programmatic caller reads. On a path whose output is persisted and rendered as a savings opportunity, an under-collected sweep then reads as a shrinking opportunity rather than a failed one. Add PartialSubscriptionFailureError, returned ALONGSIDE the successful subscriptions' recommendations, carrying the attempted/succeeded counts and every failed subscription with its cause. Unwrap exposes the per-subscription causes so errors.Is still matches through it. Failing the whole sweep on one transient error would be worse than a partial result, so the data is still returned; callers opt in to keeping it by inspecting the error with errors.As, and a caller that treats any non-nil error as fatal now fails loud instead of silently under-reporting. Both outcomes beat the previous silent success. Updated the two call sites that can reach the fan-out (the CLI paths, which are the only ones that construct an unpinned Azure provider) to keep the partial data and report the gap. The scheduler's Azure paths always pin a subscription -- collectAzureAmbient via AZURE_SUBSCRIPTION_ID and collectAzureForAccount via the account's subscription -- so they never build the fan-out client and need no change. Tests assert the distinguishing property directly: a sweep where a subscription was never queried and a complete sweep that found nothing both produce an empty slice, and only the error tells them apart. Verified failing against the old nil-error contract. --- cmd/multi_service_helpers.go | 34 ++++++- .../recommendations_multi_subscription.go | 93 +++++++++++++++++-- ...recommendations_multi_subscription_test.go | 87 ++++++++++++++++- 3 files changed, 203 insertions(+), 11 deletions(-) diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index b90a8933f..a7862c968 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "fmt" "log" "sort" @@ -11,6 +12,7 @@ import ( "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/provider" "github.com/LeanerCloud/CUDly/providers/aws/recommendations" + azureprovider "github.com/LeanerCloud/CUDly/providers/azure" "github.com/aws/aws-sdk-go-v2/aws" awsec2 "github.com/aws/aws-sdk-go-v2/service/ec2" ) @@ -99,7 +101,13 @@ func getAllAWSRegionsWithClient(ctx context.Context, ec2Client EC2ClientInterfac // discoverRegionsForService discovers regions that have recommendations for a specific service. func discoverRegionsForService(ctx context.Context, client provider.RecommendationsClient, service common.ServiceType) ([]string, error) { recs, err := client.GetRecommendationsForService(ctx, service) - if err != nil { + if partial := asPartialSubscriptionFailure(err); partial != nil { + // Region discovery is best-effort: the subscriptions that answered + // still tell us where to look. Report the gap rather than dropping + // the discovered regions or failing outright. + AppLogger.Printf(" ⚠️ Region discovery incomplete: %d of %d Azure subscriptions succeeded\n", + partial.Succeeded, partial.Attempted) + } else if err != nil { return nil, err } @@ -439,6 +447,18 @@ func fetchRecommendationsForRegion( } recs, err := recClient.GetRecommendations(ctx, ¶ms) + if partial := asPartialSubscriptionFailure(err); partial != nil { + // Keep the subscriptions that did answer, but say plainly that the + // sweep was incomplete: without this the operator would read a short + // list as "little to buy here" rather than "some subscriptions were + // never queried". + AppLogger.Printf(" ⚠️ Incomplete: %d of %d Azure subscriptions succeeded; %d could not be queried\n", + partial.Succeeded, partial.Attempted, len(partial.Failed)) + for _, f := range partial.Failed { + AppLogger.Printf(" subscription %s: %v\n", f.SubscriptionID, f.Err) + } + return recs + } if err != nil { AppLogger.Printf(" ❌ Failed to fetch recommendations: %v\n", err) return nil @@ -447,6 +467,18 @@ func fetchRecommendationsForRegion( return recs } +// asPartialSubscriptionFailure reports whether err is the Azure fan-out's +// partial-failure signal, which is returned ALONGSIDE the recommendations +// that were collected successfully. Returns nil when err is any other error +// (or nil), so callers keep their normal fail-loud handling for real errors. +func asPartialSubscriptionFailure(err error) *azureprovider.PartialSubscriptionFailureError { + var partial *azureprovider.PartialSubscriptionFailureError + if errors.As(err, &partial) { + return partial + } + return nil +} + // populateRecommendationAccountNames populates account names from account IDs. func populateRecommendationAccountNames(ctx context.Context, recs []common.Recommendation, accountCache *AccountAliasCache) { for i := range recs { diff --git a/providers/azure/recommendations_multi_subscription.go b/providers/azure/recommendations_multi_subscription.go index 643e40cd1..02d4bbd4c 100644 --- a/providers/azure/recommendations_multi_subscription.go +++ b/providers/azure/recommendations_multi_subscription.go @@ -5,6 +5,7 @@ package azure import ( "context" "fmt" + "strings" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "golang.org/x/sync/errgroup" @@ -32,6 +33,68 @@ type subscriptionClient struct { client provider.RecommendationsClient } +// SubscriptionFailure records one subscription that could not be queried +// during an org-wide fan-out. +type SubscriptionFailure struct { + SubscriptionID string + Err error +} + +// PartialSubscriptionFailureError reports that an org-wide fan-out completed +// with some subscriptions queried successfully and others not. +// +// It is returned ALONGSIDE the successful subscriptions' recommendations, so +// a caller can keep the partial data and still know the sweep was +// incomplete. Callers that want the data must inspect the error: +// +// recs, err := client.GetAllRecommendations(ctx) +// var partial *azure.PartialSubscriptionFailureError +// if errors.As(err, &partial) { +// // recs holds partial.Succeeded subscriptions' recommendations; +// // partial.Failed says which subscriptions are missing and why. +// } else if err != nil { +// return err +// } +// +// This exists because the alternative -- returning the partial results with +// a nil error -- makes "these subscriptions have no savings available" +// indistinguishable from "these subscriptions were never successfully +// queried". On a collection path whose output is persisted and rendered as +// a savings opportunity, that reads as a shrinking opportunity rather than a +// failed sweep. A log line is not a programmatic signal; this is. +// +// Failing the whole sweep on one transient subscription error would be worse +// than a partial result, which is why the successful data is still returned. +type PartialSubscriptionFailureError struct { + // Attempted is how many subscriptions the fan-out queried. + Attempted int + // Succeeded is how many returned a result. Always < Attempted and > 0: + // an all-failed sweep is a plain error, not a partial one. + Succeeded int + // Failed carries every subscription that errored, with its cause. + Failed []SubscriptionFailure +} + +func (e *PartialSubscriptionFailureError) Error() string { + ids := make([]string, 0, len(e.Failed)) + for _, f := range e.Failed { + ids = append(ids, f.SubscriptionID) + } + return fmt.Sprintf( + "azure recommendations incomplete: %d of %d subscriptions succeeded; %d failed (%s): %v", + e.Succeeded, e.Attempted, len(e.Failed), strings.Join(ids, ", "), e.Failed[0].Err) +} + +// Unwrap exposes the per-subscription causes so errors.Is/errors.As can match +// against any of them (e.g. checking whether a throttling error is in play). +func (e *PartialSubscriptionFailureError) Unwrap() []error { + errs := make([]error, 0, len(e.Failed)) + for _, f := range e.Failed { + errs = append(errs, f.Err) + } + return errs +} + // MultiSubscriptionRecommendationsClient fans recommendation collection out // across the Azure subscriptions accessible to the authenticated principal -- // every one of them by default, or the subset named by @@ -92,6 +155,13 @@ func NewMultiSubscriptionRecommendationsClient(cred azcore.TokenCredential, acco // all-attempted-failed guard used by mergeServiceResults, ported here so a // total credential/throttle failure isn't indistinguishable from "no // savings available across the whole tenant". +// +// If SOME subscriptions fail, it returns the successful subscriptions' +// recommendations together with a *PartialSubscriptionFailureError. Callers +// that want the partial data must inspect the error with errors.As; a caller +// that treats any non-nil error as fatal gets a loud failure rather than a +// silently incomplete sweep. Either way the incompleteness is visible in the +// return values, not just in a log line. func (m *MultiSubscriptionRecommendationsClient) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { if params == nil { return nil, fmt.Errorf("params cannot be nil") @@ -184,22 +254,31 @@ func mergeSubscriptionResults(subs []subscriptionClient, results [][]common.Reco } out := make([]common.Recommendation, 0, total) - failures := 0 - var lastErr error + failed := make([]SubscriptionFailure, 0, len(subs)) for i, err := range errs { if err != nil { - failures++ - lastErr = err + failed = append(failed, SubscriptionFailure{SubscriptionID: subs[i].subscriptionID, Err: err}) logging.Warnf("Azure subscription %s recommendations: %v", subs[i].subscriptionID, err) continue } out = append(out, results[i]...) } - if failures > 0 && failures == len(subs) { - return nil, fmt.Errorf("all %d Azure subscriptions failed to return recommendations: %w", failures, lastErr) + if len(failed) == 0 { + return out, nil + } + if len(failed) == len(subs) { + return nil, fmt.Errorf("all %d Azure subscriptions failed to return recommendations: %w", len(failed), failed[0].Err) + } + + // Partial sweep: hand back what succeeded AND a typed error saying what + // did not, so the caller can tell an incomplete sweep from a complete one + // that happened to find nothing. See PartialSubscriptionFailureError. + return out, &PartialSubscriptionFailureError{ + Attempted: len(subs), + Succeeded: len(subs) - len(failed), + Failed: failed, } - return out, nil } // GetRecommendationsForService retrieves recommendations for a single diff --git a/providers/azure/recommendations_multi_subscription_test.go b/providers/azure/recommendations_multi_subscription_test.go index 51861a53f..822324187 100644 --- a/providers/azure/recommendations_multi_subscription_test.go +++ b/providers/azure/recommendations_multi_subscription_test.go @@ -134,7 +134,11 @@ func TestMultiSubscriptionRecommendationsClient_GetRecommendations_MergesAcrossS }, recs) } -func TestMultiSubscriptionRecommendationsClient_GetRecommendations_PartialFailureStillSucceeds(t *testing.T) { +// One subscription failing must not discard the others' results. It does, +// however, produce a PartialSubscriptionFailureError so the caller knows the +// sweep was incomplete -- see +// TestMultiSubscriptionRecommendationsClient_PartialFailureIsDistinguishable. +func TestMultiSubscriptionRecommendationsClient_GetRecommendations_PartialFailureStillReturnsData(t *testing.T) { accounts := twoTestAccounts() withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ "sub-1": {err: errors.New("sub-1 unreachable")}, @@ -145,8 +149,85 @@ func TestMultiSubscriptionRecommendationsClient_GetRecommendations_PartialFailur require.NoError(t, err) recs, err := client.GetAllRecommendations(context.Background()) - require.NoError(t, err, "one subscription failing must not fail the whole fan-out") - assert.Equal(t, []common.Recommendation{{Account: "sub-2", Service: common.ServiceCache}}, recs) + var partial *PartialSubscriptionFailureError + require.ErrorAs(t, err, &partial, "a partial sweep must be reported as partial, not as success") + assert.Equal(t, []common.Recommendation{{Account: "sub-2", Service: common.ServiceCache}}, recs, + "one subscription failing must not discard the others' recommendations") +} + +// TestMultiSubscriptionRecommendationsClient_PartialFailureIsDistinguishable +// is the regression test for silent partial success. +// +// The two scenarios below produce IDENTICAL recommendation slices. The only +// thing that can tell them apart is the error: a sweep where a subscription +// was never successfully queried must not look like a complete sweep that +// happened to find nothing there. Persisting the first as if it were the +// second turns a failed collection into an apparently shrinking savings +// opportunity. +func TestMultiSubscriptionRecommendationsClient_PartialFailureIsDistinguishable(t *testing.T) { + newClient := func(t *testing.T, fakes map[string]*fakeRecommendationsClient) *MultiSubscriptionRecommendationsClient { + t.Helper() + withFakeSubscriptionClients(t, fakes) + c, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, twoTestAccounts()) + require.NoError(t, err) + return c + } + + // Scenario A: sub-1 could not be queried, sub-2 returned nothing. + t.Run("partial failure reports which subscriptions were not queried", func(t *testing.T) { + boom := errors.New("sub-1 unreachable") + client := newClient(t, map[string]*fakeRecommendationsClient{ + "sub-1": {err: boom}, + "sub-2": {recs: nil}, + }) + + recs, err := client.GetAllRecommendations(context.Background()) + require.Error(t, err, "an incomplete sweep must not be reported as a complete one") + + var partial *PartialSubscriptionFailureError + require.ErrorAs(t, err, &partial, "the error must be inspectable, not just a log line") + assert.Equal(t, 2, partial.Attempted) + assert.Equal(t, 1, partial.Succeeded) + require.Len(t, partial.Failed, 1) + assert.Equal(t, "sub-1", partial.Failed[0].SubscriptionID) + assert.ErrorIs(t, err, boom, "the underlying cause must remain matchable") + + // The successful subscription's data is still returned, so a caller + // that inspects the error can keep it. + assert.Empty(t, recs) + }) + + // Scenario B: both subscriptions answered, neither had recommendations. + t.Run("complete sweep finding nothing is not an error", func(t *testing.T) { + client := newClient(t, map[string]*fakeRecommendationsClient{ + "sub-1": {recs: nil}, + "sub-2": {recs: nil}, + }) + + recs, err := client.GetAllRecommendations(context.Background()) + require.NoError(t, err, "a complete sweep that found nothing must not look like a failure") + assert.Empty(t, recs) + }) +} + +// A partial failure must still hand back the successful subscriptions' data, +// so a caller that inspects the error loses nothing. +func TestMultiSubscriptionRecommendationsClient_PartialFailureKeepsSuccessfulResults(t *testing.T) { + withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ + "sub-1": {err: errors.New("sub-1 unreachable")}, + "sub-2": {recs: []common.Recommendation{{Account: "sub-2", Service: common.ServiceCache}}}, + }) + + client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, twoTestAccounts()) + require.NoError(t, err) + + recs, err := client.GetAllRecommendations(context.Background()) + + var partial *PartialSubscriptionFailureError + require.ErrorAs(t, err, &partial) + assert.Equal(t, 1, partial.Succeeded) + assert.Equal(t, []common.Recommendation{{Account: "sub-2", Service: common.ServiceCache}}, recs, + "the subscriptions that succeeded must still be returned alongside the partial-failure error") } func TestMultiSubscriptionRecommendationsClient_GetRecommendations_AllFail(t *testing.T) { From a8724c0b405321a10c1c78c08d49445687d212a0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 22:21:31 +0200 Subject: [PATCH 08/11] fix(scheduler): keep partial multi-subscription sweep data Second-order effect of returning a typed partial-failure error. Every recommendation-fetch site in the scheduler has the shape recs, err := recClient.GetAllRecommendations(ctx) if err != nil { return nil, ... } which discards recs. Correct for a real error, but for a partial sweep it turned one flaky subscription out of fifty into a total collection outage: strictly worse than the silent under-collection the typed error exists to prevent. The diff that introduced it looked like a pure improvement in isolation and only misbehaved at callers that were not re-read. Add tolerateIncompleteSweep and route every fetch site through it, so the policy lives in one place instead of relying on each call site to remember it. A partial sweep logs a warning naming the subscriptions that were not queried and returns nil, so the data that was collected is kept and persisted. Any other error is returned unchanged. Also export AsPartialSubscriptionFailure from the azure provider so the scheduler and the CLI share one definition of the check rather than each hand-rolling errors.As, and add FailedSubscriptionIDs for the log lines. Recording the incompleteness in the state table's last_collection_error, so the dashboard shows the sweep as partial rather than merely smaller, needs a partial-note threaded through collectProviderRecommendations and its three per-provider implementations. Left as follow-up rather than bolted on here; the warning names the failed subscriptions in the meantime. Tests drive fetchAndConvert with a client returning recommendations plus a partial-failure error and assert the data survives, that a genuine error still fails loud, and that the helper is narrow. Verified failing with the tolerance removed. --- cmd/multi_service_helpers.go | 17 +-- internal/scheduler/scheduler.go | 40 +++++++ internal/scheduler/scheduler_test.go | 100 ++++++++++++++++++ .../recommendations_multi_subscription.go | 36 ++++++- 4 files changed, 173 insertions(+), 20 deletions(-) diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index a7862c968..6c999afc4 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -2,7 +2,6 @@ package main import ( "context" - "errors" "fmt" "log" "sort" @@ -101,7 +100,7 @@ func getAllAWSRegionsWithClient(ctx context.Context, ec2Client EC2ClientInterfac // discoverRegionsForService discovers regions that have recommendations for a specific service. func discoverRegionsForService(ctx context.Context, client provider.RecommendationsClient, service common.ServiceType) ([]string, error) { recs, err := client.GetRecommendationsForService(ctx, service) - if partial := asPartialSubscriptionFailure(err); partial != nil { + if partial := azureprovider.AsPartialSubscriptionFailure(err); partial != nil { // Region discovery is best-effort: the subscriptions that answered // still tell us where to look. Report the gap rather than dropping // the discovered regions or failing outright. @@ -447,7 +446,7 @@ func fetchRecommendationsForRegion( } recs, err := recClient.GetRecommendations(ctx, ¶ms) - if partial := asPartialSubscriptionFailure(err); partial != nil { + if partial := azureprovider.AsPartialSubscriptionFailure(err); partial != nil { // Keep the subscriptions that did answer, but say plainly that the // sweep was incomplete: without this the operator would read a short // list as "little to buy here" rather than "some subscriptions were @@ -467,18 +466,6 @@ func fetchRecommendationsForRegion( return recs } -// asPartialSubscriptionFailure reports whether err is the Azure fan-out's -// partial-failure signal, which is returned ALONGSIDE the recommendations -// that were collected successfully. Returns nil when err is any other error -// (or nil), so callers keep their normal fail-loud handling for real errors. -func asPartialSubscriptionFailure(err error) *azureprovider.PartialSubscriptionFailureError { - var partial *azureprovider.PartialSubscriptionFailureError - if errors.As(err, &partial) { - return partial - } - return nil -} - // populateRecommendationAccountNames populates account names from account IDs. func populateRecommendationAccountNames(ctx context.Context, recs []common.Recommendation, accountCache *AccountAliasCache) { for i := range recs { diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index b021feedb..e1396653e 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -687,6 +687,7 @@ func (s *Scheduler) collectAzureAmbient(ctx context.Context, subscriptionID stri return nil, fmt.Errorf("get Azure recommendations client: %w", err) } recs, err := recClient.GetAllRecommendations(ctx) + err = tolerateIncompleteSweep("azure", err) if err != nil { return nil, fmt.Errorf("get Azure recommendations: %w", err) } @@ -706,6 +707,7 @@ func (s *Scheduler) collectGCPAmbient(ctx context.Context) ([]config.Recommendat return nil, fmt.Errorf("get GCP recommendations client: %w", err) } recs, err := recClient.GetAllRecommendations(ctx) + err = tolerateIncompleteSweep("gcp", err) if err != nil { return nil, fmt.Errorf("get GCP recommendations: %w", err) } @@ -799,6 +801,7 @@ func (s *Scheduler) collectAzureForAccount(ctx context.Context, acct config.Clou return nil, fmt.Errorf("get recommendations client: %w", err) } recs, err := recClient.GetAllRecommendations(ctx) + err = tolerateIncompleteSweep("azure", err) if err != nil { return nil, fmt.Errorf("get recommendations: %w", err) } @@ -867,6 +870,7 @@ func (s *Scheduler) collectGCPForAccount(ctx context.Context, acct config.CloudA return nil, fmt.Errorf("get recommendations client: %w", err) } recs, err := recClient.GetAllRecommendations(ctx) + err = tolerateIncompleteSweep("gcp", err) if err != nil { return nil, fmt.Errorf("get recommendations: %w", err) } @@ -887,6 +891,40 @@ func (s *Scheduler) enabledAccounts(ctx context.Context, providerName string) [] return accounts } +// tolerateIncompleteSweep converts an org-wide partial-subscription failure +// into a warning and a nil error, so the caller keeps the recommendations that +// WERE collected instead of discarding them. +// +// Every recommendation-fetch site in this file has the shape +// `recs, err := ...; if err != nil { return nil, ... }`, which throws the +// results away. That is correct for a real error, but for a partial sweep it +// would turn one flaky subscription out of fifty into a total collection +// outage -- strictly worse than the silent under-collection that +// PartialSubscriptionFailureError exists to prevent. Routing every site +// through this helper keeps the policy in one place rather than relying on +// each call site to remember it. +// +// The failed subscription IDs are named in the log so an operator can tell an +// under-collected sweep from a genuinely shrinking savings opportunity. Any +// other error is returned unchanged, preserving the existing fail-loud +// behaviour. +// +// NOTE: this records the incompleteness in the log only. Surfacing it in the +// state table's last_collection_error (so the dashboard shows the sweep as +// partial) needs a partial-note threaded through collectProviderRecommendations +// and its three per-provider implementations, which is left as follow-up work. +func tolerateIncompleteSweep(providerName string, err error) error { + partial := azureprovider.AsPartialSubscriptionFailure(err) + if partial == nil { + return err + } + logging.Warnf( + "%s recommendations incomplete: %d of %d subscriptions succeeded; not queried: %s (keeping the %d that did)", + providerName, partial.Succeeded, partial.Attempted, + strings.Join(partial.FailedSubscriptionIDs(), ", "), partial.Succeeded) + return nil +} + // fetchAndConvert is a convenience for the AWS ambient path. func (s *Scheduler) fetchAndConvert(ctx context.Context, prov provider.Provider, providerName string, accountID *string, globalCfg *config.GlobalConfig) ([]config.RecommendationRecord, error) { recClient, err := prov.GetRecommendationsClient(ctx) @@ -894,6 +932,7 @@ func (s *Scheduler) fetchAndConvert(ctx context.Context, prov provider.Provider, return nil, fmt.Errorf("failed to get %s recommendations client: %w", providerName, err) } recs, err := recClient.GetAllRecommendations(ctx) + err = tolerateIncompleteSweep(providerName, err) if err != nil { return nil, fmt.Errorf("failed to get %s recommendations: %w", providerName, err) } @@ -909,6 +948,7 @@ func (s *Scheduler) fetchAndConvert(ctx context.Context, prov provider.Provider, } var recErr error recs, recErr = recClient.GetRecommendations(ctx, ¶ms) + recErr = tolerateIncompleteSweep(providerName, recErr) if recErr != nil { // Fail loud: a misconfigured DefaultPayment/DefaultTerm or a CE // failure on this fallback must surface to the operator instead diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index ba1a4eec6..aeec2f7bd 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -14,6 +14,7 @@ import ( "github.com/LeanerCloud/CUDly/internal/purchase" "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/provider" + azureprovider "github.com/LeanerCloud/CUDly/providers/azure" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/sts" "github.com/stretchr/testify/assert" @@ -703,6 +704,105 @@ func TestSchedulerWithPurchaseManager(t *testing.T) { assert.NotNil(t, scheduler.email) } +// TestScheduler_FetchAndConvert_KeepsPartialSweepData is the regression guard +// for the second-order effect of returning a typed partial-failure error: +// every recommendation-fetch site in the scheduler has the shape +// `recs, err := ...; if err != nil { return nil, ... }`, which DISCARDS the +// results. Without tolerateIncompleteSweep, one flaky subscription out of +// fifty would turn a partial sweep into a total collection outage -- strictly +// worse than the silent under-collection the typed error exists to prevent. +// +// The successful subscriptions' recommendations must survive, and the call +// must not fail. +func TestScheduler_FetchAndConvert_KeepsPartialSweepData(t *testing.T) { + ctx := context.Background() + + // Term/PaymentOption must be present and canonical: convertRecommendations + // deliberately drops rows with an unparseable term rather than defaulting + // one, so an under-specified fixture would pass this test for the wrong + // reason (empty in, empty out). + collected := []common.Recommendation{ + { + Provider: common.ProviderAzure, + Service: common.ServiceCompute, + Account: "sub-2", + ResourceType: "Standard_D2s_v3", + Region: "westeurope", + Term: "1yr", + PaymentOption: "upfront", + Count: 3, + }, + } + partial := &azureprovider.PartialSubscriptionFailureError{ + Attempted: 3, + Succeeded: 2, + Failed: []azureprovider.SubscriptionFailure{ + {SubscriptionID: "sub-1", Err: errors.New("throttled")}, + }, + } + + recClient := new(MockRecommendationsClient) + recClient.On("GetAllRecommendations", mock.Anything).Return(collected, partial) + t.Cleanup(func() { recClient.AssertExpectations(t) }) + + prov := new(MockProvider) + prov.On("GetRecommendationsClient", mock.Anything).Return(recClient, nil) + t.Cleanup(func() { prov.AssertExpectations(t) }) + + s := &Scheduler{config: new(MockConfigStore)} + + // globalCfg nil so the zero-results fallback branch stays out of the way; + // collected is non-empty anyway. + recs, err := s.fetchAndConvert(ctx, prov, "azure", nil, nil) + + require.NoError(t, err, + "a partial multi-subscription sweep must not fail the whole collection") + require.Len(t, recs, 1, + "the subscriptions that succeeded must still be persisted, not discarded") + assert.Equal(t, 3, recs[0].Count, "the surviving subscription's recommendation must be intact") +} + +// The tolerance must be narrow: any error that is NOT a partial-subscription +// failure keeps the existing fail-loud behaviour. +func TestScheduler_FetchAndConvert_RealErrorStillFailsLoud(t *testing.T) { + ctx := context.Background() + + recClient := new(MockRecommendationsClient) + recClient.On("GetAllRecommendations", mock.Anything).Return(nil, errors.New("credentials expired")) + t.Cleanup(func() { recClient.AssertExpectations(t) }) + + prov := new(MockProvider) + prov.On("GetRecommendationsClient", mock.Anything).Return(recClient, nil) + t.Cleanup(func() { prov.AssertExpectations(t) }) + + s := &Scheduler{config: new(MockConfigStore)} + + recs, err := s.fetchAndConvert(ctx, prov, "azure", nil, nil) + + require.Error(t, err, "a genuine error must still fail the collection") + assert.Contains(t, err.Error(), "credentials expired") + assert.Nil(t, recs) +} + +func TestTolerateIncompleteSweep(t *testing.T) { + t.Run("partial failure is swallowed so the caller keeps its data", func(t *testing.T) { + partial := &azureprovider.PartialSubscriptionFailureError{ + Attempted: 2, Succeeded: 1, + Failed: []azureprovider.SubscriptionFailure{{SubscriptionID: "sub-1", Err: errors.New("boom")}}, + } + assert.NoError(t, tolerateIncompleteSweep("azure", partial)) + }) + + t.Run("other errors pass through unchanged", func(t *testing.T) { + boom := errors.New("boom") + assert.Same(t, boom, tolerateIncompleteSweep("azure", boom)) + }) + + t.Run("nil stays nil", func(t *testing.T) { + assert.NoError(t, tolerateIncompleteSweep("azure", nil)) + }) +} + // MockProvider is a mock implementation of provider.Provider. type MockProvider struct { mock.Mock diff --git a/providers/azure/recommendations_multi_subscription.go b/providers/azure/recommendations_multi_subscription.go index 02d4bbd4c..b5ab8ad20 100644 --- a/providers/azure/recommendations_multi_subscription.go +++ b/providers/azure/recommendations_multi_subscription.go @@ -4,6 +4,7 @@ package azure import ( "context" + "errors" "fmt" "strings" @@ -76,13 +77,10 @@ type PartialSubscriptionFailureError struct { } func (e *PartialSubscriptionFailureError) Error() string { - ids := make([]string, 0, len(e.Failed)) - for _, f := range e.Failed { - ids = append(ids, f.SubscriptionID) - } return fmt.Sprintf( "azure recommendations incomplete: %d of %d subscriptions succeeded; %d failed (%s): %v", - e.Succeeded, e.Attempted, len(e.Failed), strings.Join(ids, ", "), e.Failed[0].Err) + e.Succeeded, e.Attempted, len(e.Failed), + strings.Join(e.FailedSubscriptionIDs(), ", "), e.Failed[0].Err) } // Unwrap exposes the per-subscription causes so errors.Is/errors.As can match @@ -95,6 +93,34 @@ func (e *PartialSubscriptionFailureError) Unwrap() []error { return errs } +// FailedSubscriptionIDs lists the subscriptions that could not be queried, for +// log lines and operator-facing messages. +func (e *PartialSubscriptionFailureError) FailedSubscriptionIDs() []string { + ids := make([]string, 0, len(e.Failed)) + for _, f := range e.Failed { + ids = append(ids, f.SubscriptionID) + } + return ids +} + +// AsPartialSubscriptionFailure reports whether err is (or wraps) the org-wide +// fan-out's partial-failure signal, returning it when so and nil otherwise. +// +// Provided so callers do not each hand-roll the errors.As dance, and -- more +// importantly -- so the "a partial sweep must not be treated as a total +// failure" rule is expressed the same way everywhere. A caller that skips this +// check and falls into a plain `if err != nil` discards the recommendations +// that WERE collected, turning one flaky subscription into a total collection +// outage, which is worse than the silent under-collection this error exists to +// prevent. +func AsPartialSubscriptionFailure(err error) *PartialSubscriptionFailureError { + var partial *PartialSubscriptionFailureError + if errors.As(err, &partial) { + return partial + } + return nil +} + // MultiSubscriptionRecommendationsClient fans recommendation collection out // across the Azure subscriptions accessible to the authenticated principal -- // every one of them by default, or the subset named by From 5a0cc4853adcec87a11779bf3ee3efc64656e580 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 27 Jul 2026 23:16:45 +0200 Subject: [PATCH 09/11] chore(scheduler): use US spelling of "behavior" in two comments The Lint Code job pins golangci-lint v2.10.1 and .golangci.yml sets misspell locale to US, so the British spelling in these two comments failed the lint gate and cascaded to the CI Success aggregate: internal/scheduler/scheduler.go:910 internal/scheduler/scheduler_test.go:766 Both are comment prose, so this changes no identifier, no database column name and no string any test asserts on. Scoped deliberately to the two lines the linter flagged. 49 further occurrences of the British spelling remain elsewhere in the tree and were left alone: they are pre-existing, unrelated to this branch, and a repo-wide rewrite here would bury a two-word fix under a diff touching dozens of files. --- internal/scheduler/scheduler.go | 2 +- internal/scheduler/scheduler_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index e1396653e..16e07c28e 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -907,7 +907,7 @@ func (s *Scheduler) enabledAccounts(ctx context.Context, providerName string) [] // The failed subscription IDs are named in the log so an operator can tell an // under-collected sweep from a genuinely shrinking savings opportunity. Any // other error is returned unchanged, preserving the existing fail-loud -// behaviour. +// behavior. // // NOTE: this records the incompleteness in the log only. Surfacing it in the // state table's last_collection_error (so the dashboard shows the sweep as diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index aeec2f7bd..7603f7bcd 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -763,7 +763,7 @@ func TestScheduler_FetchAndConvert_KeepsPartialSweepData(t *testing.T) { } // The tolerance must be narrow: any error that is NOT a partial-subscription -// failure keeps the existing fail-loud behaviour. +// failure keeps the existing fail-loud behavior. func TestScheduler_FetchAndConvert_RealErrorStillFailsLoud(t *testing.T) { ctx := context.Background() From 050c562e071f10e2f8bf0d07831b1741394efd2f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 28 Jul 2026 21:26:45 +0200 Subject: [PATCH 10/11] fix(azure): report filter misses and lock credential swaps in fan-out Three review findings on the org-wide multi-subscription fan-out. 1. A partially-satisfied AccountFilter silently narrowed the sweep. A filter matching NO accessible subscription already errored, but one matching some and missing others returned the matched subscriptions' recommendations with a nil error. An operator scoping a sweep to sub-A and sub-B whose principal has lost Reader on sub-B therefore read sub-B as "no savings available" rather than "never queried" -- the exact confusion PartialSubscriptionFailureError exists to prevent, and which selectSubscriptions' own doc comment says must not happen. selectSubscriptions now iterates the filter rather than the discovered subscriptions, so every requested entry is accounted for as matched or missed, and the misses are folded into the partial-failure error as ErrSubscriptionNotAccessible. They count towards Attempted and Failed; the subscriptions that did answer still return their data, so one revoked subscription does not become a total collection outage. The filter is also deduplicated, so a repeated entry is neither queried twice nor double-counted. 2. PartialSubscriptionFailureError.Error() indexed Failed[0] unguarded. The type and its fields are exported, so a zero-value or externally-constructed instance panicked when formatted -- an error that crashes when logged fails exactly when the message is needed. 3. SetCredential and SetSubscriptionsClient wrote cred/subscriptionsClient as plain assignments and only then took accountsMu to invalidate, while fetchAccounts read both from a goroutine running on behalf of every caller that joined the single-flight. Both fields are now published inside one accountsMu critical section (via invalidateAccountsCacheLocked, since RWMutex is not reentrant) and snapshotted together under RLock, which also closes the window where a fetch pairs the old client with the new credential. The remaining in-package reads go through the same accessors so the guarantee holds rather than covering only one path. No production caller is concurrent today, so this was latent. Both behavioral fixes are covered by tests confirmed to fail on the pre-fix code: the partial-filter test with "An error is expected but got nil", and the swap test with a data race reported by -race. Refs #1652, #1653, #1654 --- providers/azure/accounts_cache.go | 33 +++-- providers/azure/provider.go | 114 +++++++++++++----- providers/azure/provider_test.go | 86 +++++++++++++ .../recommendations_multi_subscription.go | 107 +++++++++++++--- ...recommendations_multi_subscription_test.go | 86 +++++++++++++ 5 files changed, 373 insertions(+), 53 deletions(-) diff --git a/providers/azure/accounts_cache.go b/providers/azure/accounts_cache.go index c7c994dd6..f52e9bcd9 100644 --- a/providers/azure/accounts_cache.go +++ b/providers/azure/accounts_cache.go @@ -168,21 +168,38 @@ func cloneAccounts(accounts []common.Account) []common.Account { func (p *AzureProvider) InvalidateAccountsCache() { p.accountsMu.Lock() defer p.accountsMu.Unlock() + p.invalidateAccountsCacheLocked() +} + +// invalidateAccountsCacheLocked is InvalidateAccountsCache's body, for callers +// that already hold accountsMu for writing. +// +// It exists so SetCredential and SetSubscriptionsClient can publish the new +// field AND invalidate in one critical section: sync.RWMutex is not reentrant, +// so they cannot call InvalidateAccountsCache while holding the lock, and +// releasing it between the two steps would reopen the window where an +// in-flight fetch sees the new credential against the old cache generation. +func (p *AzureProvider) invalidateAccountsCacheLocked() { p.cachedAccounts = nil p.accountsGen++ } // fetchAccounts performs the actual ARM subscriptions.List call and resolves -// the default subscription. It holds no lock and issues the network round-trip -// on the caller's goroutine; getOrFetchAccounts serializes concurrent cold-cache -// callers via singleflight so this runs at most once per cache-population window. +// the default subscription. It holds no lock across the network round-trip and +// runs on the caller's goroutine; getOrFetchAccounts serializes concurrent +// cold-cache callers via singleflight so this runs at most once per +// cache-population window. +// +// The credential and injected client are snapshotted together under a single +// read lock before the call. This runs on behalf of every caller that joined +// the singleflight, so a SetCredential/SetSubscriptionsClient from another +// goroutine would otherwise be an unsynchronized read -- and, worse, could +// pair the old client with the new credential mid-fetch. func (p *AzureProvider) fetchAccounts(ctx context.Context) ([]common.Account, error) { // Use injected client if available (for testing) - var subClient SubscriptionsClient - if p.subscriptionsClient != nil { - subClient = p.subscriptionsClient - } else { - client, err := armsubscriptions.NewClient(p.cred, nil) + cred, subClient := p.credentialAndSubscriptionsClient() + if subClient == nil { + client, err := armsubscriptions.NewClient(cred, nil) if err != nil { return nil, fmt.Errorf("failed to create subscriptions client: %w", err) } diff --git a/providers/azure/provider.go b/providers/azure/provider.go index ad3d5a76d..38b54070c 100644 --- a/providers/azure/provider.go +++ b/providers/azure/provider.go @@ -171,9 +171,15 @@ func resolveAzureSubscriptionID(config *provider.ProviderConfig) string { // Drops any cached subscription list: the cache holds what the PREVIOUS // client returned, and serving that after the client is swapped would answer // with a different source's subscriptions. +// +// The swap and the invalidation happen under a single accountsMu write so a +// concurrent fetch can never observe the new client alongside the old cache +// generation -- see the accountsMu comment on AzureProvider. func (p *AzureProvider) SetSubscriptionsClient(client SubscriptionsClient) { + p.accountsMu.Lock() + defer p.accountsMu.Unlock() p.subscriptionsClient = client - p.InvalidateAccountsCache() + p.invalidateAccountsCacheLocked() } // SetCredentialProvider sets the credential provider (for testing) @@ -191,9 +197,56 @@ func (p *AzureProvider) SetCredentialProvider(credProvider CredentialProvider) { // and -- via GetRecommendationsClient's fan-out -- fan out across them. // Today every caller installs the credential before the first accounts fetch, // so this is a guard against a future reordering rather than a live leak. +// +// The credential is published under accountsMu, the same lock fetchAccounts +// snapshots it under: writing it outside the lock and only then taking the +// lock to invalidate leaves a window in which an in-flight fetch pairs the new +// credential with the old subscriptions client. func (p *AzureProvider) SetCredential(cred azcore.TokenCredential) { + p.accountsMu.Lock() + defer p.accountsMu.Unlock() + p.cred = cred + p.invalidateAccountsCacheLocked() +} + +// credential returns the installed credential. +// +// Reads go through accountsMu -- the same lock SetCredential publishes under +// -- so a credential swap concurrent with client construction is a defined +// handoff rather than a data race. Returns nil when no credential has been +// installed yet; every client-construction caller runs after an IsConfigured() +// check, which is what guarantees a non-nil result there. +func (p *AzureProvider) credential() azcore.TokenCredential { + p.accountsMu.RLock() + defer p.accountsMu.RUnlock() + return p.cred +} + +// credentialAndSubscriptionsClient snapshots both swappable fields under a +// single read lock. +// +// Taking them together matters: reading them under two separate locks could +// pair a credential with a subscriptions client installed by a different +// SetX call, which is precisely the mixed-state hazard the locking exists to +// prevent. The returned client is nil when none was injected, in which case +// the caller builds a real one from cred. +func (p *AzureProvider) credentialAndSubscriptionsClient() (azcore.TokenCredential, SubscriptionsClient) { + p.accountsMu.RLock() + defer p.accountsMu.RUnlock() + return p.cred, p.subscriptionsClient +} + +// publishCredential installs cred under accountsMu without touching the +// accounts cache. +// +// Used by IsConfigured's lazy ambient-credential resolution, which only runs +// when no credential was installed at all. Unlike SetCredential it does not +// invalidate: there is no previous credential whose subscription list could +// have been cached under it. +func (p *AzureProvider) publishCredential(cred azcore.TokenCredential) { + p.accountsMu.Lock() + defer p.accountsMu.Unlock() p.cred = cred - p.InvalidateAccountsCache() } // Name returns the provider name @@ -218,7 +271,7 @@ func (p *AzureProvider) DisplayName() string { // time-bounded cache or single-flight retry. func (p *AzureProvider) IsConfigured() bool { // If credential was injected via SetCredential, skip the Once path. - if p.cred != nil { + if p.credential() != nil { return true } @@ -234,7 +287,7 @@ func (p *AzureProvider) IsConfigured() bool { p.credErr = err return } - p.cred = cred + p.publishCredential(cred) }) return p.credErr == nil } @@ -261,11 +314,9 @@ func (p *AzureProvider) ValidateCredentials(ctx context.Context) error { } // Use injected client if available (for testing) - var subClient SubscriptionsClient - if p.subscriptionsClient != nil { - subClient = p.subscriptionsClient - } else { - client, err := armsubscriptions.NewClient(p.cred, nil) + cred, subClient := p.credentialAndSubscriptionsClient() + if subClient == nil { + client, err := armsubscriptions.NewClient(cred, nil) if err != nil { return fmt.Errorf("failed to create subscriptions client: %w", err) } @@ -324,11 +375,9 @@ func (p *AzureProvider) GetRegions(ctx context.Context) ([]common.Region, error) } // Use injected client if available (for testing) - var subClient SubscriptionsClient - if p.subscriptionsClient != nil { - subClient = p.subscriptionsClient - } else { - client, err := armsubscriptions.NewClient(p.cred, nil) + cred, subClient := p.credentialAndSubscriptionsClient() + if subClient == nil { + client, err := armsubscriptions.NewClient(cred, nil) if err != nil { return nil, fmt.Errorf("failed to create subscriptions client: %w", err) } @@ -430,23 +479,26 @@ func (p *AzureProvider) GetServiceClientForAccount(ctx context.Context, service // the given subscription and region. It is the shared backend for both // GetServiceClient and GetServiceClientForAccount. func (p *AzureProvider) newServiceClientForSubscription(service common.ServiceType, subscriptionID, region string) (provider.ServiceClient, error) { + // Snapshot once so every branch below builds its client from the same + // credential, even if a SetCredential lands mid-call. + cred := p.credential() switch service { case common.ServiceCompute: - return NewComputeClient(p.cred, subscriptionID, region), nil + return NewComputeClient(cred, subscriptionID, region), nil case common.ServiceRelationalDB: - return NewDatabaseClient(p.cred, subscriptionID, region), nil + return NewDatabaseClient(cred, subscriptionID, region), nil case common.ServiceCache: - return NewCacheClient(p.cred, subscriptionID, region), nil + return NewCacheClient(cred, subscriptionID, region), nil case common.ServiceNoSQL: - return NewCosmosDBClient(p.cred, subscriptionID, region), nil + return NewCosmosDBClient(cred, subscriptionID, region), nil case common.ServiceMemoryDB: - return NewManagedRedisClient(p.cred, subscriptionID, region), nil + return NewManagedRedisClient(cred, subscriptionID, region), nil case common.ServiceSavingsPlansAll: - return NewSavingsPlansClient(p.cred, subscriptionID, region), nil + return NewSavingsPlansClient(cred, subscriptionID, region), nil case common.ServiceSearch: - return NewSearchClient(p.cred, subscriptionID, region), nil + return NewSearchClient(cred, subscriptionID, region), nil case common.ServiceDataWarehouse: - return NewSynapseClient(p.cred, subscriptionID, region), nil + return NewSynapseClient(cred, subscriptionID, region), nil default: return nil, fmt.Errorf("unsupported service: %s", service) } @@ -491,7 +543,7 @@ func (p *AzureProvider) GetRecommendationsClient(ctx context.Context) (provider. } if p.subscriptionID != "" { - return NewRecommendationsClient(p.cred, p.subscriptionID) + return NewRecommendationsClient(p.credential(), p.subscriptionID) } accounts, err := p.getOrFetchAccounts(ctx) @@ -501,6 +553,14 @@ func (p *AzureProvider) GetRecommendationsClient(ctx context.Context) (provider. if len(accounts) == 0 { return nil, fmt.Errorf("no Azure subscriptions found") } + + // Snapshot after the accounts resolve, so the three branches below all + // build their client from one credential rather than re-reading it per + // branch. A SetCredential landing between the fetch and this read would + // still pair a fresh credential with a list resolved under the previous + // one; SetCredential invalidates the cache precisely so the NEXT call + // re-resolves, which is the guarantee this layer offers. + cred := p.credential() // Step 1: an explicitly configured target is validated against the // discovered subscriptions FIRST, before any default resolution. // @@ -520,18 +580,18 @@ func (p *AzureProvider) GetRecommendationsClient(ctx context.Context) (provider. "%s is set to %q, which is not among the %d subscriptions visible to this principal", azureSubscriptionIDEnv, target, len(accounts)) } - return NewRecommendationsClient(p.cred, target) + return NewRecommendationsClient(cred, target) } // Step 2: no explicit target, but a default may still resolve -- notably // the single-discovered-subscription case, which resolveDefaultSubscription // marks as the default. if defaultID := getDefaultSubscriptionID(accounts); defaultID != "" { - return NewRecommendationsClient(p.cred, defaultID) + return NewRecommendationsClient(cred, defaultID) } // Step 3: scope is genuinely ambiguous -- fan out across the whole org. - client, err := NewMultiSubscriptionRecommendationsClient(p.cred, accounts) + client, err := NewMultiSubscriptionRecommendationsClient(cred, accounts) if err != nil { return nil, err } @@ -548,7 +608,7 @@ func (p *AzureProvider) GetRecommendationsClientForAccount(ctx context.Context, if subscriptionID == "" { return nil, fmt.Errorf("subscriptionID must not be empty") } - return NewRecommendationsClient(p.cred, subscriptionID) + return NewRecommendationsClient(p.credential(), subscriptionID) } // Register the Azure provider with the global registry diff --git a/providers/azure/provider_test.go b/providers/azure/provider_test.go index 4216b5a4d..f18a97de2 100644 --- a/providers/azure/provider_test.go +++ b/providers/azure/provider_test.go @@ -1450,6 +1450,92 @@ func TestAzureProvider_GetAccounts_ConcurrentColdCache_SingleARMCall(t *testing. "concurrent cold-cache GetAccounts must issue exactly one ARM list call (single-flight)") } +// TestAzureProvider_ConcurrentCredentialSwapAndFetch_NoDataRace guards the +// publication of the two swappable fields. +// +// SetCredential / SetSubscriptionsClient used to write p.cred and +// p.subscriptionsClient as plain assignments and only THEN take accountsMu to +// invalidate, while fetchAccounts read both from a goroutine running on behalf +// of every caller that joined the single-flight. That is an unsynchronized +// read/write pair, and the window it opens can hand the fetch a new credential +// paired with the old subscriptions client. +// +// The hammer below is deliberately free of happens-before edges between the +// swapping goroutines and the fetching ones -- an ordered handshake (e.g. +// waiting on firstInFlight before swapping) would establish exactly the +// ordering that makes the race invisible to -race. Value assertions are +// intentionally absent: this test's assertion IS the race detector, so it must +// run under `go test -race` to be meaningful. +func TestAzureProvider_ConcurrentCredentialSwapAndFetch_NoDataRace(t *testing.T) { + clearAzureSubscriptionEnv(t) + + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(twoSubscriptionPages()) + + const ( + readers = 8 + rounds = 25 + ) + + stop := make(chan struct{}) + var swappers sync.WaitGroup + swappers.Add(2) + go func() { + defer swappers.Done() + for { + select { + case <-stop: + return + default: + } + p.SetCredential(&mockTokenCredential{}) + } + }() + go func() { + defer swappers.Done() + for { + select { + case <-stop: + return + default: + } + p.SetSubscriptionsClient(twoSubscriptionPages()) + } + }() + + var wg sync.WaitGroup + errs := make(chan error, readers*rounds) + for i := 0; i < readers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for r := 0; r < rounds; r++ { + // Both readers of the swapped fields: GetAccounts reaches + // fetchAccounts, GetServiceClientForAccount builds a client + // straight from the credential. + if _, err := p.GetAccounts(context.Background()); err != nil { + errs <- err + return + } + if _, err := p.GetServiceClientForAccount(context.Background(), common.ServiceCompute, "eastus", "sub-1"); err != nil { + errs <- err + return + } + } + }() + } + + wg.Wait() + close(stop) + swappers.Wait() + close(errs) + + // require.* only from the test goroutine, never the workers. + for err := range errs { + require.NoError(t, err) + } +} + // Swapping the credential or the subscriptions client must drop the cached // subscription list. The cache records what the PREVIOUS credential/client // could see; serving it afterwards would report subscriptions the new diff --git a/providers/azure/recommendations_multi_subscription.go b/providers/azure/recommendations_multi_subscription.go index b5ab8ad20..311a29025 100644 --- a/providers/azure/recommendations_multi_subscription.go +++ b/providers/azure/recommendations_multi_subscription.go @@ -41,6 +41,16 @@ type SubscriptionFailure struct { Err error } +// ErrSubscriptionNotAccessible is the cause recorded for a subscription named +// by RecommendationParams.AccountFilter that is not among the subscriptions +// visible to the authenticated principal. +// +// It is a distinct sentinel (rather than a formatted string) so a caller can +// tell "the principal cannot see this subscription" -- a durable access or +// configuration problem -- apart from a transient per-subscription ARM failure +// that a retry might clear. +var ErrSubscriptionNotAccessible = errors.New("subscription is not accessible to the authenticated principal") + // PartialSubscriptionFailureError reports that an org-wide fan-out completed // with some subscriptions queried successfully and others not. // @@ -67,7 +77,11 @@ type SubscriptionFailure struct { // Failing the whole sweep on one transient subscription error would be worse // than a partial result, which is why the successful data is still returned. type PartialSubscriptionFailureError struct { - // Attempted is how many subscriptions the fan-out queried. + // Attempted is how many subscriptions the sweep was supposed to cover: + // the ones actually queried plus any named by AccountFilter that the + // principal cannot see (those are never queried, but they were asked + // for, so leaving them out of the denominator would under-report the + // gap this error exists to surface). Attempted int // Succeeded is how many returned a result. Always < Attempted and > 0: // an all-failed sweep is a plain error, not a partial one. @@ -77,6 +91,15 @@ type PartialSubscriptionFailureError struct { } func (e *PartialSubscriptionFailureError) Error() string { + // Failed is exported and the zero value is constructible, so an + // externally-built or zero-valued instance must format rather than panic + // on Failed[0] -- an error type that panics when logged turns a partial + // sweep into a crash at exactly the moment the operator needs the message. + if len(e.Failed) == 0 { + return fmt.Sprintf( + "azure recommendations incomplete: %d of %d subscriptions succeeded; no failures recorded", + e.Succeeded, e.Attempted) + } return fmt.Sprintf( "azure recommendations incomplete: %d of %d subscriptions succeeded; %d failed (%s): %v", e.Succeeded, e.Attempted, len(e.Failed), @@ -188,12 +211,17 @@ func NewMultiSubscriptionRecommendationsClient(cred azcore.TokenCredential, acco // that treats any non-nil error as fatal gets a loud failure rather than a // silently incomplete sweep. Either way the incompleteness is visible in the // return values, not just in a log line. +// +// The same signal covers params.AccountFilter entries that name no accessible +// subscription: they are reported as ErrSubscriptionNotAccessible failures +// rather than silently dropped from the sweep, so a partially-satisfied filter +// never returns a nil error (see selectSubscriptions). func (m *MultiSubscriptionRecommendationsClient) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { if params == nil { return nil, fmt.Errorf("params cannot be nil") } - targets, err := m.selectSubscriptions(params.AccountFilter) + targets, unmatched, err := m.selectSubscriptions(params.AccountFilter) if err != nil { return nil, err } @@ -223,7 +251,7 @@ func (m *MultiSubscriptionRecommendationsClient) GetRecommendations(ctx context. return nil, err } - return mergeSubscriptionResults(targets, results, errs) + return mergeSubscriptionResults(targets, results, errs, unmatched) } // selectSubscriptions narrows the fan-out to params.AccountFilter. @@ -245,42 +273,77 @@ func (m *MultiSubscriptionRecommendationsClient) GetRecommendations(ctx context. // this client exists to provide. A non-empty filter that matches nothing is // an error rather than an empty result: returning zero recommendations would // be indistinguishable from "these subscriptions have no savings available". -func (m *MultiSubscriptionRecommendationsClient) selectSubscriptions(filter []string) ([]subscriptionClient, error) { +// +// A filter that matches SOME of its entries and misses others is the same +// hazard at smaller scale, so the misses are reported too, via the second +// return value: an operator who scopes a sweep to sub-A and sub-B, and whose +// principal has since lost Reader on sub-B, must not be handed sub-A's +// recommendations with a nil error -- sub-B would read as "no savings +// available" rather than "never queried". The misses are folded into the +// partial-failure error by mergeSubscriptionResults rather than failing the +// call outright, because a stored filter covering many subscriptions must not +// become a total collection outage the moment one of them is deleted or +// access to it is revoked. That is the same trade-off +// PartialSubscriptionFailureError already makes for per-subscription API +// failures: keep the data, but make the gap a programmatic signal. +// +// Returns (selected, unmatchedFilterEntries, error). unmatched is nil when the +// filter is empty or every entry matched. +func (m *MultiSubscriptionRecommendationsClient) selectSubscriptions(filter []string) ([]subscriptionClient, []string, error) { if len(filter) == 0 { - return m.subscriptions, nil + return m.subscriptions, nil, nil } - wanted := make(map[string]struct{}, len(filter)) - for _, id := range filter { - wanted[id] = struct{}{} + available := make(map[string]subscriptionClient, len(m.subscriptions)) + for _, sub := range m.subscriptions { + available[sub.subscriptionID] = sub } - selected := make([]subscriptionClient, 0, len(m.subscriptions)) - for _, sub := range m.subscriptions { - if _, ok := wanted[sub.subscriptionID]; ok { + // Iterate the filter (not m.subscriptions) so every requested entry is + // accounted for as either matched or missed. Duplicate filter entries are + // collapsed via seen so one subscription is neither queried twice nor + // double-counted in Attempted. + seen := make(map[string]struct{}, len(filter)) + selected := make([]subscriptionClient, 0, len(filter)) + var unmatched []string + for _, id := range filter { + if _, dup := seen[id]; dup { + continue + } + seen[id] = struct{}{} + if sub, ok := available[id]; ok { selected = append(selected, sub) + continue } + unmatched = append(unmatched, id) } + if len(selected) == 0 { - return nil, fmt.Errorf( + return nil, nil, fmt.Errorf( "azure multi-subscription recommendations: account filter %v matches none of the %d accessible subscriptions", filter, len(m.subscriptions)) } - return selected, nil + return selected, unmatched, nil } // mergeSubscriptionResults concatenates successful per-subscription results, // logging a warning for each subscription that failed, and applies the // all-attempted-failed guard described in GetRecommendations' doc comment. // subs, results and errs are index-aligned. -func mergeSubscriptionResults(subs []subscriptionClient, results [][]common.Recommendation, errs []error) ([]common.Recommendation, error) { +// +// unmatched carries the AccountFilter entries that named no accessible +// subscription (see selectSubscriptions). They were never queried, so they +// have no results slot, but they count towards both Attempted and Failed: +// a requested subscription that could not be reached is a gap in the sweep +// whether the reason was an ARM error or missing access. +func mergeSubscriptionResults(subs []subscriptionClient, results [][]common.Recommendation, errs []error, unmatched []string) ([]common.Recommendation, error) { total := 0 for _, r := range results { total += len(r) } out := make([]common.Recommendation, 0, total) - failed := make([]SubscriptionFailure, 0, len(subs)) + failed := make([]SubscriptionFailure, 0, len(subs)+len(unmatched)) for i, err := range errs { if err != nil { failed = append(failed, SubscriptionFailure{SubscriptionID: subs[i].subscriptionID, Err: err}) @@ -289,11 +352,19 @@ func mergeSubscriptionResults(subs []subscriptionClient, results [][]common.Reco } out = append(out, results[i]...) } + // Appended after the queried subscriptions' failures so failed[0] keeps + // naming a real API error when there is one -- that is the cause the + // all-failed guard below wraps, and the one an operator can act on. + for _, id := range unmatched { + failed = append(failed, SubscriptionFailure{SubscriptionID: id, Err: ErrSubscriptionNotAccessible}) + logging.Warnf("Azure subscription %s was requested by the account filter but is not accessible; it was not queried", id) + } + attempted := len(subs) + len(unmatched) if len(failed) == 0 { return out, nil } - if len(failed) == len(subs) { + if len(failed) == attempted { return nil, fmt.Errorf("all %d Azure subscriptions failed to return recommendations: %w", len(failed), failed[0].Err) } @@ -301,8 +372,8 @@ func mergeSubscriptionResults(subs []subscriptionClient, results [][]common.Reco // did not, so the caller can tell an incomplete sweep from a complete one // that happened to find nothing. See PartialSubscriptionFailureError. return out, &PartialSubscriptionFailureError{ - Attempted: len(subs), - Succeeded: len(subs) - len(failed), + Attempted: attempted, + Succeeded: attempted - len(failed), Failed: failed, } } diff --git a/providers/azure/recommendations_multi_subscription_test.go b/providers/azure/recommendations_multi_subscription_test.go index 822324187..12a957256 100644 --- a/providers/azure/recommendations_multi_subscription_test.go +++ b/providers/azure/recommendations_multi_subscription_test.go @@ -3,6 +3,7 @@ package azure import ( "context" "errors" + "fmt" "sync/atomic" "testing" @@ -230,6 +231,20 @@ func TestMultiSubscriptionRecommendationsClient_PartialFailureKeepsSuccessfulRes "the subscriptions that succeeded must still be returned alongside the partial-failure error") } +// PartialSubscriptionFailureError and its fields are exported, so a +// zero-value or externally-constructed instance is reachable. Formatting one +// must not panic on Failed[0] -- an error type that crashes when logged fails +// at exactly the moment the operator needs its message. +func TestPartialSubscriptionFailureError_ErrorWithNoFailures(t *testing.T) { + var zero PartialSubscriptionFailureError + + require.NotPanics(t, func() { _ = fmt.Sprintf("%v", &zero) }, + "formatting a zero-value PartialSubscriptionFailureError must not panic") + assert.Contains(t, zero.Error(), "no failures recorded") + assert.Empty(t, zero.FailedSubscriptionIDs()) + assert.Empty(t, zero.Unwrap()) +} + func TestMultiSubscriptionRecommendationsClient_GetRecommendations_AllFail(t *testing.T) { accounts := twoTestAccounts() withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ @@ -333,6 +348,77 @@ func TestMultiSubscriptionRecommendationsClient_GetRecommendations_AccountFilter assert.Equal(t, int64(0), fake2.calls.Load()) } +// TestMultiSubscriptionRecommendationsClient_GetRecommendations_AccountFilterPartiallyMatches +// is the regression test for a silently narrowed sweep. +// +// The all-miss case above errors loudly, but a filter matching SOME of its +// entries used to drop the misses on the floor and return a nil error. The +// scenario that makes that dangerous: an operator scopes a sweep to sub-1 and +// sub-2, the principal has since lost Reader on sub-2 so it is absent from the +// discovered list, and the sweep returns only sub-1's rows with err == nil. +// sub-2 then reads as "no savings available" -- indistinguishable from a +// genuine empty result, and persisted as a shrinking opportunity. +// +// The requested-but-unreachable subscription must therefore appear in the +// partial-failure error, while the subscription that DID answer still returns +// its data. +func TestMultiSubscriptionRecommendationsClient_GetRecommendations_AccountFilterPartiallyMatches(t *testing.T) { + accounts := twoTestAccounts() + fake1 := &fakeRecommendationsClient{recs: []common.Recommendation{{Account: "sub-1", Service: common.ServiceCompute}}} + fake2 := &fakeRecommendationsClient{recs: []common.Recommendation{{Account: "sub-2", Service: common.ServiceCache}}} + withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ + "sub-1": fake1, + "sub-2": fake2, + }) + + client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, accounts) + require.NoError(t, err) + + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{ + AccountFilter: []string{"sub-1", "sub-not-visible"}, + }) + + var partial *PartialSubscriptionFailureError + require.ErrorAs(t, err, &partial, + "a filter entry matching no accessible subscription must not be silently dropped") + assert.Equal(t, 2, partial.Attempted, + "a requested-but-unreachable subscription still counts towards the sweep's intended scope") + assert.Equal(t, 1, partial.Succeeded) + require.Len(t, partial.Failed, 1) + assert.Equal(t, "sub-not-visible", partial.Failed[0].SubscriptionID) + assert.ErrorIs(t, err, ErrSubscriptionNotAccessible, + "the cause must be distinguishable from a transient ARM failure") + + // The subscription that DID match still returns its data, so one revoked + // subscription does not become a total collection outage. + assert.Equal(t, []common.Recommendation{{Account: "sub-1", Service: common.ServiceCompute}}, recs) + assert.Equal(t, int64(1), fake1.calls.Load(), "the matched subscription must be queried") + assert.Equal(t, int64(0), fake2.calls.Load(), "a subscription outside the filter must not be queried") +} + +// A duplicate AccountFilter entry must not query its subscription twice nor +// inflate Attempted -- the filter is a set, and double-counting would +// double-count that subscription's recommendations. +func TestMultiSubscriptionRecommendationsClient_GetRecommendations_AccountFilterDeduplicates(t *testing.T) { + accounts := twoTestAccounts() + fake1 := &fakeRecommendationsClient{recs: []common.Recommendation{{Account: "sub-1", Service: common.ServiceCompute}}} + fake2 := &fakeRecommendationsClient{} + withFakeSubscriptionClients(t, map[string]*fakeRecommendationsClient{ + "sub-1": fake1, + "sub-2": fake2, + }) + + client, err := NewMultiSubscriptionRecommendationsClient(&mockAzureTokenCredential{}, accounts) + require.NoError(t, err) + + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{ + AccountFilter: []string{"sub-1", "sub-1"}, + }) + require.NoError(t, err) + assert.Equal(t, []common.Recommendation{{Account: "sub-1", Service: common.ServiceCompute}}, recs) + assert.Equal(t, int64(1), fake1.calls.Load(), "a duplicated filter entry must be queried once") +} + // An empty AccountFilter keeps the org-wide default: every visible // subscription is queried. func TestMultiSubscriptionRecommendationsClient_GetRecommendations_EmptyAccountFilterFansOutToAll(t *testing.T) { From c8f13b3de20a695146c561a99c4b30f599f89971 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 3 Aug 2026 14:25:24 +0200 Subject: [PATCH 11/11] fix(azure): validate configured subscription on the regions/service path Addresses both CodeRabbit findings from the review round on PR #1520. Both harden invariants that the previous commit established on one path and left off the others, which reads as a global guarantee but is not. 1. Silent subscription retarget (providers/azure/provider.go) resolveSubscriptionIDFromCtx -- which backs GetRegions and GetServiceClient -- called getDefaultSubscriptionID without first validating an explicitly configured subscription against the discovered list. resolveDefaultSubscription deliberately falls through to its single-visible-subscription rule when the configured target is not found, so with an unreachable AZURE_SUBSCRIPTION_ID (or ProviderConfig subscription) and exactly one visible subscription, the request was silently retargeted to whichever subscription happened to be visible. GetRecommendationsClient already guarded this via accountsContain under a comment reading "This ordering is load-bearing"; the same misconfiguration therefore errored on the recommendations path and silently retargeted on the regions/service path. GetServiceClient drives the resource enumeration recommendations are computed from, and is also the purchase-execution path's service-client lookup, so the operator could believe they were scoped to subscription A while data came from B. The guard is extracted into validateConfiguredSubscription and reused by GetRecommendationsClient, so the two paths cannot drift apart again. It covers both configuration axes (ProviderConfig field and the env var), mirroring resolveDefaultSubscription's own priority order. Production Azure flows pin AzureSubscriptionID (scheduler, registration and account handlers) and short-circuit before this path, so the guard engages only on the unpinned/ambient path where the retarget lived. GetRegions' error wrapper no longer claims "no Azure subscriptions found": the new inner error fires precisely when subscriptions were found but the configured one is not among them. 2. Unsynchronized credProvider (providers/azure/provider.go) SetCredentialProvider wrote p.credProvider outside accountsMu while IsConfigured read it inside credOnce.Do -- the one swappable field left outside the locking discipline the rest of the provider follows. Latent today (SetCredentialProvider has no production callers), but a half-kept invariant reads as a kept one. Published and read under accountsMu now, matching cred and subscriptionsClient. It deliberately does not invalidate the accounts cache: credProvider only feeds the lazy resolution that runs when no credential was installed at all, so no subscription list resolved under a different credential can exist yet. Regression tests, both confirmed failing against the pre-fix code: - TestAzureProvider_ResolveSubscription_InvisibleConfiguredTargetErrors uses an invisible configured target with exactly ONE visible subscription -- the case the fall-through hides. With two or more visible subscriptions getDefaultSubscriptionID returns "" and the pre-existing "multiple Azure subscriptions" error fires either way, so such a test would pass with or without the guard. Covers GetRegions and GetServiceClient, both configuration axes, and the honoured visible-target case. Pre-fix: 3 of 4 subtests fail with "An error is expected but got nil". - TestAzureProvider_ConcurrentCredentialProviderSwapAndIsConfigured_NoDataRace loops over fresh providers because credOnce makes the read one-shot per instance. Pre-fix: WARNING: DATA RACE, write at provider.go:187 against read at provider.go:280. TestAzureProvider_GetRegions and TestAzureProvider_GetServiceClient_WithSubscriptionLookup now clear AZURE_SUBSCRIPTION_ID via the existing helper: their fixtures resolve through the newly-guarded path, so an exported value on a developer machine would fail them for an unrelated reason. --- providers/azure/accounts_cache.go | 7 ++ providers/azure/provider.go | 143 +++++++++++++++++++++----- providers/azure/provider_test.go | 165 +++++++++++++++++++++++++++++- 3 files changed, 286 insertions(+), 29 deletions(-) diff --git a/providers/azure/accounts_cache.go b/providers/azure/accounts_cache.go index f52e9bcd9..22396c656 100644 --- a/providers/azure/accounts_cache.go +++ b/providers/azure/accounts_cache.go @@ -264,6 +264,13 @@ func resolveDefaultSubscription(accounts []common.Account, explicitSubID string) // target was configured but not found in the visible subscriptions; // fall through to the single-subscription rule rather than leaving // all accounts as non-default. + // + // This fall-through is why IsDefault alone must never be trusted to + // answer "which subscription did the operator ask for?": with exactly + // one visible subscription it reports that subscription as the + // default even though the configured target does not match it. + // Callers resolving an explicitly configured target must validate it + // via validateConfiguredSubscription BEFORE reading IsDefault. } // Rule 3: single visible subscription. diff --git a/providers/azure/provider.go b/providers/azure/provider.go index 38b54070c..6356d8366 100644 --- a/providers/azure/provider.go +++ b/providers/azure/provider.go @@ -182,11 +182,36 @@ func (p *AzureProvider) SetSubscriptionsClient(client SubscriptionsClient) { p.invalidateAccountsCacheLocked() } -// SetCredentialProvider sets the credential provider (for testing) +// SetCredentialProvider sets the credential provider (for testing). +// +// Published under accountsMu -- the same lock every other swappable field on +// the provider is published under -- because IsConfigured reads credProvider +// on its lazy ambient-credential path. Leaving this one field outside the lock +// would make that read an unsynchronized data race. +// +// Unlike SetCredential it does not invalidate the accounts cache: credProvider +// only feeds IsConfigured's lazy resolution, which runs at most once and only +// when no credential was installed at all, so at the moment it is read there is +// no cached subscription list resolved under a different credential. func (p *AzureProvider) SetCredentialProvider(credProvider CredentialProvider) { + p.accountsMu.Lock() + defer p.accountsMu.Unlock() p.credProvider = credProvider } +// credentialProvider returns the injected credential provider, or nil when +// none was installed -- in which case callers fall back to +// realCredentialProvider. +// +// Reads go through accountsMu, the same lock SetCredentialProvider publishes +// under, so a swap concurrent with IsConfigured's lazy resolution is a defined +// handoff rather than a data race. +func (p *AzureProvider) credentialProvider() CredentialProvider { + p.accountsMu.RLock() + defer p.accountsMu.RUnlock() + return p.credProvider +} + // SetCredential sets the credential directly. // // Also used in production (the scheduler and purchase-execution paths @@ -276,10 +301,8 @@ func (p *AzureProvider) IsConfigured() bool { } p.credOnce.Do(func() { - var credProvider CredentialProvider - if p.credProvider != nil { - credProvider = p.credProvider - } else { + credProvider := p.credentialProvider() + if credProvider == nil { credProvider = &realCredentialProvider{} } cred, err := credProvider.NewDefaultAzureCredential() @@ -349,8 +372,62 @@ func (p *AzureProvider) GetAccounts(ctx context.Context) ([]common.Account, erro return p.getOrFetchAccounts(ctx) } -// resolveSubscriptionIDFromCtx calls GetAccounts and returns the default -// subscription ID, or a descriptive error if none can be resolved. +// configuredSubscriptionTarget returns the subscription this provider was +// explicitly told to operate on, together with a label naming the knob it came +// from so an error can tell the operator what to fix. It mirrors +// resolveDefaultSubscription's priority: the ProviderConfig field first (which +// resolveAzureSubscriptionID may itself have taken from the deprecated Profile +// overload, hence the generic label), then AZURE_SUBSCRIPTION_ID. Both return +// values are empty when nothing was configured. +// +// p.subscriptionID is written once in NewAzureProvider and never mutated +// afterwards, so it needs no lock (unlike cred/subscriptionsClient/credProvider, +// which have SetX swappers). +func (p *AzureProvider) configuredSubscriptionTarget() (target, source string) { + if p.subscriptionID != "" { + return p.subscriptionID, "the configured Azure subscription ID" + } + if env := os.Getenv(azureSubscriptionIDEnv); env != "" { + return env, azureSubscriptionIDEnv + } + return "", "" +} + +// validateConfiguredSubscription checks an explicitly configured subscription +// target against the subscriptions the principal can actually see, returning +// the validated target -- or ("", nil) when nothing was configured at all. +// +// Every caller must run this BEFORE consulting getDefaultSubscriptionID, and +// that ordering is load-bearing. getDefaultSubscriptionID reads the IsDefault +// flags set by resolveDefaultSubscription, whose rule 3 marks a lone visible +// subscription as the default even when a target was configured and did not +// match it. Consulting that result first would let an invisible target -- an +// operator typo, or a credential that lost access to the intended subscription +// -- silently resolve to whichever single subscription happens to be visible, +// answering a misconfiguration with a plausible wrong subscription instead of +// an error. Validating the target up front makes it fail loud regardless of how +// many subscriptions are visible. +func (p *AzureProvider) validateConfiguredSubscription(accounts []common.Account) (string, error) { + target, source := p.configuredSubscriptionTarget() + if target == "" { + return "", nil + } + if !accountsContain(accounts, target) { + return "", fmt.Errorf( + "%s is set to %q, which is not among the %d subscriptions visible to this principal", + source, target, len(accounts)) + } + return target, nil +} + +// resolveSubscriptionIDFromCtx calls GetAccounts and returns the subscription +// to operate on, or a descriptive error if none can be resolved. +// +// This backs GetRegions and GetServiceClient, which means it is on the path +// that enumerates the resources recommendations are computed from -- so it +// applies the same validate-then-default ordering GetRecommendationsClient +// does. Without it the same misconfiguration would error on the +// recommendations path and silently retarget on this one. func (p *AzureProvider) resolveSubscriptionIDFromCtx(ctx context.Context) (string, error) { accounts, err := p.GetAccounts(ctx) if err != nil { @@ -359,6 +436,13 @@ func (p *AzureProvider) resolveSubscriptionIDFromCtx(ctx context.Context) (strin if len(accounts) == 0 { return "", fmt.Errorf("no Azure subscriptions found") } + target, err := p.validateConfiguredSubscription(accounts) + if err != nil { + return "", err + } + if target != "" { + return target, nil + } id := getDefaultSubscriptionID(accounts) if id == "" { return "", fmt.Errorf("multiple Azure subscriptions found; set AzureSubscriptionID or AZURE_SUBSCRIPTION_ID") @@ -368,10 +452,14 @@ func (p *AzureProvider) resolveSubscriptionIDFromCtx(ctx context.Context) (strin // GetRegions returns all available Azure regions using the Subscriptions API func (p *AzureProvider) GetRegions(ctx context.Context) ([]common.Region, error) { - // Resolve the subscription to query available locations. + // Resolve the subscription to query available locations. The wrapper stays + // neutral about WHY resolution failed: resolveSubscriptionIDFromCtx now + // also rejects a configured subscription the principal cannot see, and + // prefixing that with "no Azure subscriptions found" would contradict the + // inner error, which fires precisely when subscriptions were found. subscriptionID, err := p.resolveSubscriptionIDFromCtx(ctx) if err != nil { - return nil, fmt.Errorf("no Azure subscriptions found to query regions: %w", err) + return nil, fmt.Errorf("failed to resolve the Azure subscription to query regions: %w", err) } // Use injected client if available (for testing) @@ -449,7 +537,15 @@ func (p *AzureProvider) GetServiceClient(ctx context.Context, service common.Ser return nil, fmt.Errorf("azure provider is not configured") } - // Use explicit subscription ID if configured; otherwise resolve from accounts. + // Use explicit subscription ID if configured; otherwise resolve from + // accounts. A pinned subscription is taken on trust and NOT validated + // against the visible list -- same contract as GetRecommendationsClient's + // pinned branch. Validating it would force an ARM subscriptions.List on + // every pinned call (this is the purchase-execution path), and a pinned + // subscription the principal cannot reach fails loud at the first ARM call + // anyway. The unpinned branch below is the one that needed a guard, + // because there a bad target resolved to a plausible wrong subscription + // instead of failing. subscriptionID := p.subscriptionID if subscriptionID == "" { var err error @@ -562,24 +658,15 @@ func (p *AzureProvider) GetRecommendationsClient(ctx context.Context) (provider. // re-resolves, which is the guarantee this layer offers. cred := p.credential() // Step 1: an explicitly configured target is validated against the - // discovered subscriptions FIRST, before any default resolution. - // - // This ordering is load-bearing. getDefaultSubscriptionID reads the - // IsDefault flags set by resolveDefaultSubscription, whose rule 3 marks a - // lone visible subscription as the default even when a target was - // configured and did not match it. Consulting that result first would let - // an invisible target (an operator typo, or a credential that lost access - // to the intended subscription) silently resolve to whichever single - // subscription happens to be visible -- a misconfiguration answered with - // a plausible wrong subscription instead of an error. Validating the - // target up front makes it fail loud regardless of how many subscriptions - // are visible. - if target := os.Getenv(azureSubscriptionIDEnv); target != "" { - if !accountsContain(accounts, target) { - return nil, fmt.Errorf( - "%s is set to %q, which is not among the %d subscriptions visible to this principal", - azureSubscriptionIDEnv, target, len(accounts)) - } + // discovered subscriptions FIRST, before any default resolution -- see + // validateConfiguredSubscription for why that ordering is load-bearing. + // p.subscriptionID is empty on this path (the pinned case returned above), + // so the target here is always AZURE_SUBSCRIPTION_ID. + target, err := p.validateConfiguredSubscription(accounts) + if err != nil { + return nil, err + } + if target != "" { return NewRecommendationsClient(cred, target) } diff --git a/providers/azure/provider_test.go b/providers/azure/provider_test.go index f18a97de2..3f58dc4aa 100644 --- a/providers/azure/provider_test.go +++ b/providers/azure/provider_test.go @@ -619,6 +619,13 @@ func TestAzureProvider_GetAccounts(t *testing.T) { } func TestAzureProvider_GetRegions(t *testing.T) { + // These subtests resolve through resolveSubscriptionIDFromCtx, which now + // validates a configured subscription against the fixture's (single) + // subscription list -- so an AZURE_SUBSCRIPTION_ID exported on the + // developer's machine or CI runner would fail them for a reason unrelated + // to the region listing they guard. + clearAzureSubscriptionEnv(t) + t.Run("success with locations", func(t *testing.T) { subID := "test-subscription" subName := "Test Sub" @@ -861,7 +868,7 @@ func TestAzureProvider_SetterMethods(t *testing.T) { p := &AzureProvider{} mockProvider := &mockCredentialProvider{} p.SetCredentialProvider(mockProvider) - assert.NotNil(t, p.credProvider) + assert.NotNil(t, p.credentialProvider()) }) t.Run("SetCredential", func(t *testing.T) { @@ -873,6 +880,11 @@ func TestAzureProvider_SetterMethods(t *testing.T) { } func TestAzureProvider_GetServiceClient_WithSubscriptionLookup(t *testing.T) { + // Same reason as TestAzureProvider_GetRegions: these subtests leave + // subscriptionID unset and so resolve through resolveSubscriptionIDFromCtx, + // which now validates a configured subscription against the fixture list. + clearAzureSubscriptionEnv(t) + t.Run("fetches subscription when subscriptionID not set", func(t *testing.T) { subID := "fetched-subscription" subName := "Fetched Sub" @@ -1809,3 +1821,154 @@ func TestAzureProvider_GetRecommendationsClient_MultiSubscriptionFanOut(t *testi assert.Contains(t, err.Error(), "failed to resolve Azure subscriptions") }) } + +// resolveSubscriptionIDFromCtx feeds GetRegions and GetServiceClient. Like +// GetRecommendationsClient it must validate an explicitly configured target +// against the discovered subscriptions BEFORE consulting the resolved default. +// +// Exactly one visible subscription is the case that hides the bug, and the +// only case this test is worth writing for: resolveDefaultSubscription's rule +// 3 marks a LONE visible subscription as the default even when a configured +// target did not match it, so reading the default first silently retargets the +// operator's request to whichever subscription happens to be visible. With two +// or more visible subscriptions getDefaultSubscriptionID returns "" and the +// pre-existing "multiple Azure subscriptions" error fires either way, so such +// a test would pass with or without the guard and prove nothing. +func TestAzureProvider_ResolveSubscription_InvisibleConfiguredTargetErrors(t *testing.T) { + soloID, soloName := "sub-solo", "Solo Subscription" + soloClient := func() *mockSubscriptionsClient { + return &mockSubscriptionsClient{ + listPagerFunc: func(_ *armsubscriptions.ClientListOptions) SubscriptionsPager { + return &mockSubscriptionsPager{ + pages: []armsubscriptions.ClientListResponse{ + {SubscriptionListResult: armsubscriptions.SubscriptionListResult{ + Value: []*armsubscriptions.Subscription{{SubscriptionID: &soloID, DisplayName: &soloName}}, + }}, + }, + } + }, + listLocationsPagerFunc: func(_ string, _ *armsubscriptions.ClientListLocationsOptions) LocationsPager { + return &mockLocationsPager{} + }, + } + } + + t.Run("GetRegions errors on an env target the principal cannot see", func(t *testing.T) { + t.Setenv("AZURE_SUBSCRIPTION_ID", "sub-not-visible") + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(soloClient()) + + regions, err := p.GetRegions(context.Background()) + require.Error(t, err, + "an invisible configured subscription must not silently retarget to the one visible subscription") + assert.Nil(t, regions) + assert.Contains(t, err.Error(), "sub-not-visible") + assert.Contains(t, err.Error(), "not among the 1 subscriptions visible") + }) + + // GetServiceClient drives the resource enumeration recommendations are + // computed from, so a silent retarget here reports subscription B's + // resources to an operator who asked for subscription A. + t.Run("GetServiceClient errors on an env target the principal cannot see", func(t *testing.T) { + t.Setenv("AZURE_SUBSCRIPTION_ID", "sub-not-visible") + p := &AzureProvider{cred: &mockTokenCredential{}} // subscriptionID unset -> resolves via accounts + p.SetSubscriptionsClient(soloClient()) + + client, err := p.GetServiceClient(context.Background(), common.ServiceCompute, "eastus") + require.Error(t, err) + assert.Nil(t, client) + assert.Contains(t, err.Error(), "sub-not-visible") + assert.Contains(t, err.Error(), "not among the 1 subscriptions visible") + }) + + // The same guard has to cover the ProviderConfig axis, not just the + // environment variable: resolveDefaultSubscription's rule 1 reads + // p.subscriptionID and falls through to rule 3 identically when it misses. + t.Run("GetRegions errors on a config target the principal cannot see", func(t *testing.T) { + clearAzureSubscriptionEnv(t) + p := &AzureProvider{cred: &mockTokenCredential{}, subscriptionID: "sub-not-visible"} + p.SetSubscriptionsClient(soloClient()) + + regions, err := p.GetRegions(context.Background()) + require.Error(t, err) + assert.Nil(t, regions) + assert.Contains(t, err.Error(), "sub-not-visible") + assert.Contains(t, err.Error(), "not among the 1 subscriptions visible") + }) + + // The guard must reject only targets that are genuinely invisible; a + // visible one still resolves, and resolves to itself. + t.Run("a visible configured target is still honoured", func(t *testing.T) { + t.Setenv("AZURE_SUBSCRIPTION_ID", soloID) + var gotSubscriptionID string + client := soloClient() + client.listLocationsPagerFunc = func(subscriptionID string, _ *armsubscriptions.ClientListLocationsOptions) LocationsPager { + gotSubscriptionID = subscriptionID + return &mockLocationsPager{} + } + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(client) + + _, err := p.GetRegions(context.Background()) + require.NoError(t, err) + assert.Equal(t, soloID, gotSubscriptionID) + }) +} + +// SetCredentialProvider publishes credProvider, which IsConfigured reads on its +// lazy ambient-credential path. Both must go through accountsMu -- the lock +// every other swappable field on the provider is published under -- or the +// write is an unsynchronized data race with that read. +// +// credOnce means each provider instance reads credProvider at most once, so +// the race window is one-shot per instance. Looping over fresh providers is +// what makes the detector's chances additive instead of resting on a single +// interleaving. +func TestAzureProvider_ConcurrentCredentialProviderSwapAndIsConfigured_NoDataRace(t *testing.T) { + const ( + instances = 50 + readers = 4 + swaps = 4 + ) + + for i := 0; i < instances; i++ { + // No credential installed, so IsConfigured takes the credOnce path + // that reads credProvider. + p := &AzureProvider{} + // Install one up front so every IsConfigured resolves deterministically + // through the mock rather than depending on ambient Azure credentials + // being present on the machine running the test. + p.SetCredentialProvider(&mockCredentialProvider{cred: &mockTokenCredential{}}) + + start := make(chan struct{}) + results := make(chan bool, readers) + var wg sync.WaitGroup + + wg.Add(1) + go func() { + defer wg.Done() + <-start + for s := 0; s < swaps; s++ { + p.SetCredentialProvider(&mockCredentialProvider{cred: &mockTokenCredential{}}) + } + }() + + for r := 0; r < readers; r++ { + wg.Add(1) + go func() { + defer wg.Done() + <-start + results <- p.IsConfigured() + }() + } + + close(start) + wg.Wait() + close(results) + + // require.* only from the test goroutine, never the workers. + for ok := range results { + require.True(t, ok, "IsConfigured must resolve the injected credential provider") + } + } +}