feat(providers/azure): enumerate subscriptions in GetServiceClient (closes #553) - #561
feat(providers/azure): enumerate subscriptions in GetServiceClient (closes #553)#561cristim wants to merge 2 commits into
Conversation
…ionsClient (closes #553) GetAccounts now caches the ARM subscriptions list after the first successful call so that GetServiceClient, GetRecommendationsClient, and GetRegions do not each trigger a separate network round-trip in the same request. The cache is keyed to the provider lifetime; InvalidateAccountsCache() clears it for tests that need a fresh fetch. GetRecommendationsClient is brought to parity with the AWS provider: when no subscription is pinned (ProviderConfig.AzureSubscriptionID is empty), all accessible subscriptions are discovered via GetAccounts and a MultiSubscriptionRecommendationsClient is returned so recommendations are collected across every subscription the authenticated principal can see. When exactly one subscription is found the existing single-adapter path is taken to avoid unnecessary overhead. MultiSubscriptionRecommendationsClient fans out GetRecommendations to each per-subscription RecommendationsClientAdapter concurrently (errgroup), isolates per-subscription errors (logs warnings, keeps successful results), and propagates parent context cancellation after the fan-out completes. GetServiceClient continues to use the first discovered subscription when no subscription is pinned -- purchase execution (internal/purchase) and the scheduler (internal/scheduler) already pin a subscription per call via ProviderConfig.AzureSubscriptionID, so the multi-sub behaviour is not needed there. 9 new unit tests cover: multi-sub client returned when 2+ subscriptions found, single adapter returned for exactly 1 subscription, pinned subscription always returns single adapter, cache hit after second GetAccounts call, cache invalidation forces re-fetch, constructor rejects empty account list, adapter count and subscriptionID assignment, all-fail error propagation, and interface compliance. Refs #473
📝 WalkthroughWalkthroughAzure provider now discovers and caches all accessible subscriptions, returning either single- or multi-subscription recommendation clients depending on account count. Multi-subscription recommendations aggregate results across subscriptions concurrently with graceful handling of partial failures. ChangesMulti-subscription Azure provider support
Sequence DiagramsequenceDiagram
participant Code
participant GetRecClient as GetRecommendationsClient
participant Cache as getOrFetchAccounts
participant ARM as Azure ARM API
participant MultiClient as MultiSubscriptionRecommendationsClient
participant ErrGroup as concurrent adapters
Code->>GetRecClient: call (no subscription pinned)
GetRecClient->>Cache: fetch all subscriptions
Cache->>Cache: check local cache
alt cache hit
Cache-->>GetRecClient: return cached accounts
else cache miss
Cache->>ARM: list subscriptions
ARM-->>Cache: subscription list
Cache->>Cache: store in cache
Cache-->>GetRecClient: return accounts
end
alt single account
GetRecClient-->>Code: return RecommendationsClientAdapter
else multiple accounts
GetRecClient->>MultiClient: NewMultiSubscriptionRecommendationsClient
MultiClient->>ErrGroup: fan out GetRecommendations per adapter
ErrGroup-->>MultiClient: results from all subscriptions
MultiClient-->>GetRecClient: aggregated recommendations
GetRecClient-->>Code: return MultiSubscriptionRecommendationsClient
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
providers/azure/recommendations_test.go (1)
402-421: 💤 Low valueClarify test name to match implementation.
The test name suggests it verifies "all sub-adapters fail during fan-out," but the implementation uses a pre-cancelled context to trigger the parent context error check (lines 418-420 acknowledge this). These are distinct code paths: testing context cancellation vs. testing the error-aggregation logic when all per-subscription API calls fail but the context remains valid.
Consider renaming to
TestMultiSubscriptionRecommendationsClient_CancelledContextor similar to accurately reflect what the test covers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/recommendations_test.go` around lines 402 - 421, Rename the test TestMultiSubscriptionRecommendationsClient_AllFail to reflect it exercises a pre-cancelled context path (e.g., TestMultiSubscriptionRecommendationsClient_CancelledContext) and update its comment/description accordingly; locate the test function named TestMultiSubscriptionRecommendationsClient_AllFail (which constructs accounts, calls NewMultiSubscriptionRecommendationsClient and invokes GetAllRecommendations with a cancelled ctx) and change the function name and top comment to indicate "cancelled context" behavior so the name matches the implementation and intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@providers/azure/recommendations_test.go`:
- Around line 402-421: Rename the test
TestMultiSubscriptionRecommendationsClient_AllFail to reflect it exercises a
pre-cancelled context path (e.g.,
TestMultiSubscriptionRecommendationsClient_CancelledContext) and update its
comment/description accordingly; locate the test function named
TestMultiSubscriptionRecommendationsClient_AllFail (which constructs accounts,
calls NewMultiSubscriptionRecommendationsClient and invokes
GetAllRecommendations with a cancelled ctx) and change the function name and top
comment to indicate "cancelled context" behavior so the name matches the
implementation and intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5559bd50-4402-452e-93ea-9bc28cfe18e5
📒 Files selected for processing (4)
providers/azure/provider.goproviders/azure/provider_test.goproviders/azure/recommendations.goproviders/azure/recommendations_test.go
Rename TestMultiSubscriptionRecommendationsClient_AllFail to TestMultiSubscriptionRecommendationsClient_CancelledContext and update the comment to accurately reflect that the test exercises a pre-cancelled context path, not the "all adapters fail" error-aggregation scenario. This clarifies the test's intent and prevents confusion about which code path is actually being exercised. Addresses CodeRabbit nitpick on PR #561.
CodeRabbit Review ResponseNitpick addressed:
Verification:
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
providers/azure/recommendations_test.go (1)
417-421:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the specific cancellation error, not just any error.
At Line 420,
require.Error(t, err)is too broad for this test’s stated intent. Please also assertcontext.Canceledso the test fails on unrelated errors.Suggested patch
_, err = client.GetAllRecommendations(ctx) // With a cancelled context GetRecommendations propagates context.Canceled // before the sub-adapter fan-out error-accounting path runs. require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/recommendations_test.go` around lines 417 - 421, The test currently asserts any error from client.GetAllRecommendations(ctx) but intends to ensure the context cancellation is propagated; replace the broad require.Error(t, err) check with an assertion that the error is specifically context.Canceled (e.g., using require.ErrorIs or errors.Is) so the test fails on unrelated errors; update the assertion near the call to GetAllRecommendations in the test to assert errors.Is(err, context.Canceled) or require.ErrorIs(t, err, context.Canceled).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@providers/azure/recommendations_test.go`:
- Around line 417-421: The test currently asserts any error from
client.GetAllRecommendations(ctx) but intends to ensure the context cancellation
is propagated; replace the broad require.Error(t, err) check with an assertion
that the error is specifically context.Canceled (e.g., using require.ErrorIs or
errors.Is) so the test fails on unrelated errors; update the assertion near the
call to GetAllRecommendations in the test to assert errors.Is(err,
context.Canceled) or require.ErrorIs(t, err, context.Canceled).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c6deb81-bbcf-4e41-94ba-8061d5528de1
📒 Files selected for processing (1)
providers/azure/recommendations_test.go
|
Superseded by PR #591 (merged: feat(azure): multi-subscription enumeration parity with AWS Organizations). PR #591 shipped GetServiceClientForAccount + GetRecommendationsClientForAccount fan-out building blocks for the same #553 ask. The subscription-list caching this PR adds is a useful enhancement but the conflict surface is too large to rebase cleanly. If caching is still needed, file a follow-up issue scoped to just the cache layer on top of the already-merged GetAccounts. |
Summary
AzureProviderafter the first successfulGetAccountscall so thatGetServiceClient,GetRecommendationsClient, andGetRegionsdo not each trigger a separate network round-trip in the same requestMultiSubscriptionRecommendationsClientthat fans out recommendation collection across all accessible subscriptions concurrently (errgroup) with per-subscription error isolationGetRecommendationsClientto return the multi-subscription client when no subscription is pinned, bringing Azure to parity with the AWS provider which automatically spans the whole organisationWhat changed (per file)
providers/azure/provider.gocachedAccounts/accountsMufields onAzureProviderfor thread-safe subscription cachinggetOrFetchAccounts(double-checked-lock cache) andfetchAccountsLocked(ARM API) helpersGetAccountsdelegates togetOrFetchAccounts-- identical behaviour, now cache-backedGetServiceClientusesgetOrFetchAccountsinstead ofGetAccounts(avoids duplicate call)GetRecommendationsClientreturnsMultiSubscriptionRecommendationsClientwhen 2+ subscriptions are discovered (1-sub path unchanged; pinned-subscription path unchanged)InvalidateAccountsCache()for testsproviders/azure/recommendations.goMultiSubscriptionRecommendationsClientstruct withNewMultiSubscriptionRecommendationsClientconstructorprovider.RecommendationsClient(GetRecommendations,GetRecommendationsForService,GetAllRecommendations)providers/azure/provider_test.goTestAzureProvider_GetRecommendationsClient_WithSubscriptionLookupextended with 3 new sub-tests: multi-sub returns*MultiSubscriptionRecommendationsClient; single-sub returns*RecommendationsClientAdapter; pinned subscription always returns single adapterTestAzureProvider_GetAccounts_CacheHit: verifies secondGetAccountscall does not invoke the subscriptions API againTestAzureProvider_InvalidateAccountsCache: verifies cache invalidation forces a fresh API callproviders/azure/recommendations_test.goTestNewMultiSubscriptionRecommendationsClient_EmptyAccounts: constructor rejects empty account listTestNewMultiSubscriptionRecommendationsClient_BuildsAdaptersPerAccount: adapter slice has correct length and subscriptionIDsTestMultiSubscriptionRecommendationsClient_AllFail: cancelled context propagates errorTestMultiSubscriptionRecommendationsClient_InterfaceCompliance: compile-time interface assertionAWS-vs-Azure parity narrative
AWS Cost Explorer spans the whole organisation automatically (via
AccountScope: Linked). Before this PR, Azure'sGetRecommendationsClientwould silently use only the first discovered subscription when no subscription was pinned, meaning multi-subscription tenants would see incomplete recommendations. After this PR,GetRecommendationsClientfans out across every accessible subscription the authenticated principal can see, matching the AWS behaviour.The purchase execution path (
internal/purchase/execution.go) and the scheduler (internal/scheduler/scheduler.go) always pin a subscription viaProviderConfig.AzureSubscriptionID, so the multi-sub code path is not triggered there -- those callers already handle per-account fan-out externally.Test plan
go test ./providers/azure/... -count=1 -shortpasses (113 tests, up from 104)go test ./providers/aws/... -count=1 -shortpasses (647 tests, no regression)go build ./...succeeds (no downstream compile breaks)GetRecommendationsClientwithout pinning a subscription -- verify both subscriptions appear in the returned recommendationsCloses #553
Refs #473
Summary by CodeRabbit
New Features
Performance Improvements
Tests