diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index b90a8933f..6c999afc4 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -11,6 +11,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 +100,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 := 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. + 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 +446,18 @@ func fetchRecommendationsForRegion( } recs, err := recClient.GetRecommendations(ctx, ¶ms) + 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 + // 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 diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 3697f729b..16e07c28e 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) } @@ -770,6 +772,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, @@ -788,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) } @@ -856,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) } @@ -876,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 +// 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 +// 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) @@ -883,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) } @@ -898,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 a96dd8aab..7603f7bcd 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 behavior. +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 @@ -1647,6 +1747,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() diff --git a/providers/azure/accounts_cache.go b/providers/azure/accounts_cache.go new file mode 100644 index 000000000..22396c656 --- /dev/null +++ b/providers/azure/accounts_cache.go @@ -0,0 +1,310 @@ +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" +) + +// accountsCacheSFKeyPrefix prefixes the singleflight.Group key this cache +// uses. There is exactly one cached value per AzureProvider (the org-wide +// 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: +// +// - 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) { + 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() + defer p.accountsMu.RUnlock() + if p.cachedAccounts == nil { + return nil + } + return cloneAccounts(p.cachedAccounts) +} + +// 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). + 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() + // 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 + }) + if err != nil { + return nil, err + } + 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. +// 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). +// +// 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.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 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) + 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) + } + 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(azureSubscriptionIDEnv) + } + + 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. + // + // 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. + if len(accounts) == 1 { + accounts[0].IsDefault = true + } +} + +// 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). +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 ed0cb90b1..6356d8366 100644 --- a/providers/azure/provider.go +++ b/providers/azure/provider.go @@ -12,6 +12,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" @@ -96,6 +97,32 @@ 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. 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. + // 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, + // cloneAccounts, InvalidateAccountsCache) lives in accounts_cache.go. } // NewAzureProvider creates a new Azure provider instance. @@ -139,18 +166,111 @@ 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. +// +// 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.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 } -// SetCredential sets the credential directly (for testing) +// 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 +// 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. +// +// 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 } @@ -176,15 +296,13 @@ 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 } 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() @@ -192,7 +310,7 @@ func (p *AzureProvider) IsConfigured() bool { p.credErr = err return } - p.cred = cred + p.publishCredential(cred) }) return p.credErr == nil } @@ -219,11 +337,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) } @@ -253,102 +369,65 @@ func (p *AzureProvider) GetAccounts(ctx context.Context) ([]common.Account, erro if !p.IsConfigured() { return nil, fmt.Errorf("azure provider is not configured") } - - // 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 + return p.getOrFetchAccounts(ctx) } -// resolveDefaultSubscription sets IsDefault on the matching account in-place. +// 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. // -// 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 +// 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" } - - 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 + if env := os.Getenv(azureSubscriptionIDEnv); env != "" { + return env, azureSubscriptionIDEnv } + return "", "" } -// 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 "" +// 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 } - for _, a := range accounts { - if a.IsDefault { - return a.ID - } + 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 "" + return target, nil } -// resolveSubscriptionIDFromCtx calls GetAccounts and returns the default -// subscription ID, or a descriptive error if none can be resolved. +// 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 { @@ -357,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") @@ -366,18 +452,20 @@ 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) - 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) } @@ -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 @@ -479,49 +575,114 @@ 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) } } -// 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 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") } - // 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.credential(), p.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") + } + + // 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 -- 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) + } + + // 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(cred, defaultID) } - return NewRecommendationsClient(p.cred, subscriptionID) + // Step 3: scope is genuinely ambiguous -- fan out across the whole org. + client, err := NewMultiSubscriptionRecommendationsClient(cred, accounts) + if err != nil { + return nil, err + } + return client, nil } // GetRecommendationsClientForAccount returns a recommendations client scoped to @@ -534,7 +695,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 54f2b1ac3..3f58dc4aa 100644 --- a/providers/azure/provider_test.go +++ b/providers/azure/provider_test.go @@ -3,6 +3,10 @@ package azure import ( "context" "errors" + "fmt" + "runtime" + "sync" + "sync/atomic" "testing" "github.com/Azure/azure-sdk-for-go/sdk/azcore" @@ -615,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" @@ -857,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) { @@ -869,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" @@ -1241,3 +1257,718 @@ 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 atomic.Int64 +} + +func (c *countingSubscriptionsClient) NewListPager(options *armsubscriptions.ClientListOptions) SubscriptionsPager { + c.calls.Add(1) + 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}, + }, + }, + }, + }, + } + }, + } +} + +// 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{}} + p.SetSubscriptionsClient(counting) + + first, err := p.GetAccounts(context.Background()) + require.NoError(t, err) + require.Len(t, first, 2) + 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, int64(1), counting.calls.Load(), "second GetAccounts call should be served from cache, not the API") + assert.Equal(t, first, second) +} + +func TestAzureProvider_GetAccounts_CacheHit_ReturnsIndependentCopies(t *testing.T) { + clearAzureSubscriptionEnv(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) { + clearAzureSubscriptionEnv(t) + counting := &countingSubscriptionsClient{mockSubscriptionsClient: twoSubscriptionPages()} + + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(counting) + + _, err := p.GetAccounts(context.Background()) + require.NoError(t, err) + assert.Equal(t, int64(1), counting.calls.Load()) + + p.InvalidateAccountsCache() + + _, err = p.GetAccounts(context.Background()) + require.NoError(t, err) + 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) { + clearAzureSubscriptionEnv(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 + // 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(signalReady bool) { + defer wg.Done() + if signalReady { + followersReady.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(false) + <-gated.firstInFlight + + followersReady.Add(n - 1) + for i := 1; i < n; i++ { + wg.Add(1) + go worker(true) + } + // 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() + } + 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)") +} + +// 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 +// 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 +// 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") + }) + + // 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()) + + 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") + }) +} + +// 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") + } + } +} diff --git a/providers/azure/recommendations_multi_subscription.go b/providers/azure/recommendations_multi_subscription.go new file mode 100644 index 000000000..311a29025 --- /dev/null +++ b/providers/azure/recommendations_multi_subscription.go @@ -0,0 +1,394 @@ +// Package azure provides the org-wide (multi-subscription) recommendations +// fan-out client. +package azure + +import ( + "context" + "errors" + "fmt" + "strings" + + "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 +} + +// SubscriptionFailure records one subscription that could not be queried +// during an org-wide fan-out. +type SubscriptionFailure struct { + SubscriptionID string + 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. +// +// 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 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. + Succeeded int + // Failed carries every subscription that errored, with its cause. + Failed []SubscriptionFailure +} + +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), + strings.Join(e.FailedSubscriptionIDs(), ", "), 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 +} + +// 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 +// RecommendationParams.AccountFilter (see selectSubscriptions). +// +// 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 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 +// 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". +// +// 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. +// +// 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, unmatched, 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 targets { + 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 mergeSubscriptionResults(targets, results, errs, unmatched) +} + +// 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". +// +// 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, nil + } + + available := make(map[string]subscriptionClient, len(m.subscriptions)) + for _, sub := range m.subscriptions { + available[sub.subscriptionID] = sub + } + + // 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, nil, fmt.Errorf( + "azure multi-subscription recommendations: account filter %v matches none of the %d accessible subscriptions", + filter, len(m.subscriptions)) + } + 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. +// +// 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)+len(unmatched)) + for i, err := range errs { + if err != nil { + 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]...) + } + // 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) == attempted { + 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: attempted, + Succeeded: attempted - len(failed), + Failed: failed, + } +} + +// 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..12a957256 --- /dev/null +++ b/providers/azure/recommendations_multi_subscription_test.go @@ -0,0 +1,461 @@ +package azure + +import ( + "context" + "errors" + "fmt" + "sync/atomic" + "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 + // 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 && !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 + } + 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() + 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) + 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) { + 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) +} + +// 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")}, + "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()) + 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") +} + +// 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{ + "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) +} + +// 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"}}, ignoreCtx: true}, + "sub-2": {recs: []common.Recommendation{{Account: "sub-2"}}, ignoreCtx: true}, + }) + + 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) +} + +// 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()) +} + +// 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) { + 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{} + 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) +}