feat(azure): multi-subscription enumeration parity with AWS Organizations (refs #473) - #591
Merged
cristim merged 3 commits intoMay 20, 2026
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR makes Azure subscription default selection deterministic (config, env, single-subscription fallback), applies it to region lookups and client creation, and adds explicit-account client constructors plus unit tests validating resolution and client behavior. ChangesAzure Subscription Default Selection and Account-Specific Clients
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant GetAccounts as GetAccounts
participant AzureAPI as Azure API
participant resolveDefault as resolveDefaultSubscription
participant GetRegions as GetRegions
participant GetServiceClient as GetServiceClient
Caller->>GetAccounts: GetAccounts(ctx)
GetAccounts->>AzureAPI: list subscriptions
AzureAPI-->>GetAccounts: subscriptions list
GetAccounts->>resolveDefault: resolveDefaultSubscription(subscriptions, config, env)
resolveDefault-->>GetAccounts: accounts with IsDefault set
Caller->>GetRegions: GetRegions(ctx, accounts)
GetRegions->>GetRegions: getDefaultSubscriptionID(accounts)
GetRegions->>AzureAPI: query locations for default subscription
AzureAPI-->>GetRegions: locations list
Caller->>GetServiceClient: GetServiceClient(ctx, service, region)
GetServiceClient->>GetAccounts: GetAccounts(ctx) (if needed)
GetServiceClient->>GetServiceClient: getDefaultSubscriptionID(accounts)
GetServiceClient->>AzureAPI: create service client for subscription
AzureAPI-->>GetServiceClient: service client
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
Poem
🚥 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 unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
providers/azure/provider.go (1)
322-332: 💤 Low valueConsider adding a defensive guard for empty accounts slice.
While all current callers check
len(accounts) == 0before invoking this helper, line 331 will panic ifaccountsis empty and no default is marked. Adding a guard makes the function safer for future callers.🛡️ Defensive guard
func getDefaultSubscriptionID(accounts []common.Account) string { + if len(accounts) == 0 { + return "" + } for _, a := range accounts { if a.IsDefault { return a.ID } } return accounts[0].ID }🤖 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/provider.go` around lines 322 - 332, getDefaultSubscriptionID currently assumes accounts is non-empty and will panic when accounts == nil or len(accounts) == 0; add a defensive guard at the top of getDefaultSubscriptionID to return an empty string when the slice is empty (e.g., if len(accounts) == 0 { return "" }) so callers won't panic, and keep the existing loop that returns a.ID when a.IsDefault or falls back to accounts[0].ID for backward compatibility when non-empty.providers/azure/provider_test.go (1)
1040-1097: ⚡ Quick winAdd an explicit env-fallback test for
GetAccountsdefault marking.Current coverage validates configured ID and no-config paths, but not the
AZURE_SUBSCRIPTION_IDfallback path that this PR introduces.Suggested test case
func TestAzureProvider_GetAccounts_IsDefault(t *testing.T) { + t.Run("env subscriptionID is marked IsDefault when config is empty", func(t *testing.T) { + t.Setenv("AZURE_SUBSCRIPTION_ID", "sub-2") + p := &AzureProvider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(&mockSubscriptionsClient{ + listPagerFunc: func(_ *armsubscriptions.ClientListOptions) SubscriptionsPager { + return makeSubscriptionsPager( + []string{"sub-1", "sub-2", "sub-3"}, + []string{"Sub 1", "Sub 2", "Sub 3"}, + ) + }, + }) + + accounts, err := p.GetAccounts(context.Background()) + require.NoError(t, err) + require.Len(t, accounts, 3) + assert.False(t, accounts[0].IsDefault) + assert.True(t, accounts[1].IsDefault) + assert.False(t, accounts[2].IsDefault) + })🤖 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/provider_test.go` around lines 1040 - 1097, Add a unit test in TestAzureProvider_GetAccounts_IsDefault that verifies the AZURE_SUBSCRIPTION_ID env fallback: create an AzureProvider without subscriptionID, set os.Setenv("AZURE_SUBSCRIPTION_ID","sub-2") (and defer os.Unsetenv), set the mockSubscriptionsClient using makeSubscriptionsPager with multiple IDs ["sub-1","sub-2","sub-3"], call p.GetAccounts(ctx) and assert that only the account with ID "sub-2" has IsDefault true while others are false; use the same mockTokenCredential, mockSubscriptionsClient, makeSubscriptionsPager helpers and require/assert patterns as the existing subtests.
🤖 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.
Inline comments:
In `@providers/azure/provider_test.go`:
- Around line 981-987: The helper makeSubscriptionsPager assumes names and ids
are same length causing an index panic; add a guard at the top of
makeSubscriptionsPager that checks len(ids)==len(names) and fail fast with a
clear message (e.g., panic or test.Fatalf) if they differ so callers get an
informative error instead of an opaque index out of range; modify the function
body around makeSubscriptionsPager to perform this validation before the for
loop that references names[i].
- Around line 1040-1097: The tests in TestAzureProvider_GetAccounts_IsDefault
are flaky because GetAccounts reads the AZURE_SUBSCRIPTION_ID env var; update
each subtest to explicitly control that env var before invoking
AzureProvider.GetAccounts (use os.Setenv to set the desired value or os.Unsetenv
to clear it) and restore the original value after the subtest finishes; apply
this change in the three subtests so the AzureProvider (and its
SetSubscriptionsClient/mockSubscriptionsClient) sees a deterministic
subscriptionID for the assertions.
---
Nitpick comments:
In `@providers/azure/provider_test.go`:
- Around line 1040-1097: Add a unit test in
TestAzureProvider_GetAccounts_IsDefault that verifies the AZURE_SUBSCRIPTION_ID
env fallback: create an AzureProvider without subscriptionID, set
os.Setenv("AZURE_SUBSCRIPTION_ID","sub-2") (and defer os.Unsetenv), set the
mockSubscriptionsClient using makeSubscriptionsPager with multiple IDs
["sub-1","sub-2","sub-3"], call p.GetAccounts(ctx) and assert that only the
account with ID "sub-2" has IsDefault true while others are false; use the same
mockTokenCredential, mockSubscriptionsClient, makeSubscriptionsPager helpers and
require/assert patterns as the existing subtests.
In `@providers/azure/provider.go`:
- Around line 322-332: getDefaultSubscriptionID currently assumes accounts is
non-empty and will panic when accounts == nil or len(accounts) == 0; add a
defensive guard at the top of getDefaultSubscriptionID to return an empty string
when the slice is empty (e.g., if len(accounts) == 0 { return "" }) so callers
won't panic, and keep the existing loop that returns a.ID when a.IsDefault or
falls back to accounts[0].ID for backward compatibility when non-empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08d8d50a-cc76-4df7-a05a-d8df8345683f
📒 Files selected for processing (2)
providers/azure/provider.goproviders/azure/provider_test.go
…ions (refs #473) - Fix GetAccounts to set IsDefault on the correct subscription (priority: explicit config > AZURE_SUBSCRIPTION_ID env var > sole subscription), matching AWS behaviour where the STS-identified account is always default. - Fix GetServiceClient and GetRecommendationsClient to use the default subscription rather than silently picking accounts[0]. - Add GetServiceClientForAccount(ctx, service, region, subscriptionID) so callers iterating all subscriptions from GetAccounts can get a service client per subscription without triggering an extra GetAccounts round-trip. - Add GetRecommendationsClientForAccount(ctx, subscriptionID) for the same fan-out pattern. - Extract newServiceClientForSubscription to eliminate the duplicated switch across the two GetServiceClient variants. - Add 27 tests: resolveDefaultSubscription unit tests, IsDefault integration tests via GetAccounts, and happy/error-path tests for the two new methods.
- Add defensive guard in getDefaultSubscriptionID for empty accounts slice (panic-safe for future callers) - Guard makeSubscriptionsPager against ids/names length mismatch with an explicit panic message instead of an opaque index out of range - Pin AZURE_SUBSCRIPTION_ID env var in each GetAccounts IsDefault subtest so results are deterministic in any shell environment - Add new subtest covering the env-var fallback path in GetAccounts (previously untested via GetAccounts directly)
cristim
force-pushed
the
feat/azure-multi-subscription
branch
from
May 20, 2026 18:41
fdf95f4 to
f1d5ef6
Compare
Member
Author
CodeRabbit review - addressed findingsAll 4 findings actioned in commit f1d5ef6. Summary:
Tests: 380 passed, build clean. @coderabbitai review |
Contributor
|
🐇✨ ✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@providers/azure/provider_test.go`:
- Around line 1029-1040: The tests calling resolveDefaultSubscription(accounts,
"") must be made env-deterministic by ensuring AZURE_SUBSCRIPTION_ID is unset or
controlled during the subtests; update both subtests ("single subscription gets
IsDefault when no explicit ID" and "multiple subscriptions with no explicit ID
stay non-default") to temporarily clear os.Getenv("AZURE_SUBSCRIPTION_ID") (or
set it to empty) for the duration of the test and restore the original value
afterwards so resolveDefaultSubscription sees no inherited subscription id and
assertions on accounts[].IsDefault are deterministic.
In `@providers/azure/provider.go`:
- Around line 425-431: When resolving subscriptionID, don't mask errors from
p.GetAccounts; if p.GetAccounts(ctx) returns a non-nil err, return that error
(or wrap it) instead of converting every failure to "no Azure subscriptions
found"; only return the "no Azure subscriptions found" message when err==nil and
len(accounts)==0 and still use getDefaultSubscriptionID(accounts) when accounts
exist. Apply the same fix to the other lookup block that uses p.GetAccounts(ctx)
(the other occurrence referenced in the review) so auth/API failures are
preserved there as well.
- Around line 322-335: Change getDefaultSubscriptionID to stop returning
accounts[0] when no account is marked default: if no IsDefault is found return
an empty string (keep the existing empty check for len(accounts)==0). Then
update callers (e.g., GetRegions, GetServiceClient, GetRecommendationsClient) to
treat an empty subscription ID as an error and fail fast with a clear message
such as "multiple Azure subscriptions found; set AzureSubscriptionID or
AZURE_SUBSCRIPTION_ID" so the caller surface is deterministic instead of
silently using the first paged account.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4aaf9f1a-3fd4-444a-bb2b-47ea4eeb0f5b
📒 Files selected for processing (2)
providers/azure/provider.goproviders/azure/provider_test.go
Drop the accounts[0] fallback from getDefaultSubscriptionID; return "" when
no account is marked default. Update GetRegions, GetServiceClient, and
GetRecommendationsClient to fail fast with a clear message when the resolved
subscription ID is empty. Preserve GetAccounts errors instead of collapsing
them into "no subscriptions found". Add t.Setenv("AZURE_SUBSCRIPTION_ID", "")
to the two resolveDefaultSubscription subtests that were missing it.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This was referenced May 20, 2026
Merged
4 tasks
2 tasks
This was referenced Jul 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GetAccountsIsDefaultfield: was alwaysfalse; now set totruefor the subscription matching (in priority order) the explicit config,AZURE_SUBSCRIPTION_IDenv var, or the sole visible subscription -- matching AWS behaviour where the STS-identified account is always the default.GetServiceClient/GetRecommendationsClientfallback: both methods previously pickedaccounts[0]silently; they now use the default-marked subscription viagetDefaultSubscriptionID.GetServiceClientForAccount(ctx, service, region, subscriptionID): enables callers that already have an account fromGetAccountsto obtain a service client per subscription without an extra round-trip -- the building block for fan-out across all subscriptions.GetRecommendationsClientForAccount(ctx, subscriptionID): same pattern for recommendations clients.newServiceClientForSubscription: removes the duplicatedswitch service {}block that previously appeared inGetServiceClientand would have been repeated inGetServiceClientForAccount.resolveDefaultSubscription, integration tests forIsDefaultcorrectness throughGetAccounts, and happy/error-path tests for both new methods.Test plan
go test ./providers/azure/...-- 379 tests pass (was 352; 27 new)GetServiceClientForAccountwith empty subscriptionID returns an errorGetRecommendationsClientForAccountwith empty subscriptionID returns an errorIsDefault: trueIsDefault: true, others arefalseRelation to #473
This closes the "multi-account" parity gap row in the issue's table. The remaining gaps (Savings Plans, Redshift/Synapse, MemoryDB, convertible RI exchange, RI utilisation reporting) are tracked in #473 and will land in separate PRs.
Summary by CodeRabbit
New Features
Improvements
Tests