Implement subscription filter on az login#33122
Implement subscription filter on az login#33122xuming-ms wants to merge 1 commit intoAzure:feat/subscription-filteringfrom
Conversation
xuming-ms
requested review from
DanielMicrosoft,
ReaNAiveD,
bebound,
jiasli,
necusjz,
teresaritorto and
yonzhan
as code owners
|
Hi @xuming-ms, |
️✔️AzureCLI-FullTest
|
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| login | cmd login added parameter default_subscription |
||
| login | cmd login added parameter skip_subscription_discovery |
microsoft-github-policy-service
bot
requested review from
calvinhzy and
evelyn-ys
microsoft-github-policy-service
bot
added
Account
xuming-ms
force-pushed
the
ming/implement-subscription-filter-on-az-login
branch
from
9666e63 to
4c236dd
Compare
|
az login |
isra-fel
left a comment
isra-fel
left a comment
There was a problem hiding this comment.
Please check the inline comments. Love the decent test coverage!
|
|
||
|
|
||
| # pylint: disable=too-many-branches, too-many-locals | ||
| def login(cmd, username=None, password=None, tenant=None, scopes=None, allow_no_subscriptions=False, |
There was a problem hiding this comment.
Would allow_no_subscriptions block -skip-subscription-discovery (bare mode)?
I think the answer is no as it's covered by test_skip_discovery_bare_mode
There was a problem hiding this comment.
Correct, allow_no_subscriptions does not block --skip-subscription-discovery (bare mode).
| set_login_experience_v2(login_experience_v2) | ||
|
|
||
| select_subscription = interactive and sys.stdin.isatty() and sys.stdout.isatty() and login_experience_v2 | ||
| # When --subscription or --skip-subscription-discovery is provided, bypass the interactive selector |
There was a problem hiding this comment.
There's an edge case: Customers can delegate subscriptions and resource groups to specified users and roles in the managing tenant
meaning the same subscription can appear in multiple tenants, and we might still need to display the selector.
I think a good criteria could be: after we apply all the filters, if there are still multiple subscription candidates, then we display the selector.
There was a problem hiding this comment.
Good point — thanks for calling that out. To address this, I'm
thinking about consolidating the default subscription selection
and the interactive selector into the same place. Would it be
better to move the default selection logic out of _profile.login
and colocate it with the existing interactive selector (I prefer this since this is currently where default is set and reduce the chance to breaking existing code), or to
move the interactive selector into _profile.login instead?
| @@ -60,6 +60,18 @@ def load_arguments(self, command): | |||
| "WWW-Authenticate header.") | |||
| c.ignore('_subscription') # hide the global subscription parameter | |||
There was a problem hiding this comment.
I wonder if it makes sense to enable this global parameter.
There was a problem hiding this comment.
I believe we have to ignore this global parameter to accomodate for the new one that we will create in this command.
There was a problem hiding this comment.
Pull request overview
Adds support for filtering/bypassing subscription discovery in az login to improve login performance for tenants with very large numbers of subscriptions, including a “bare” tenant-level login mode and a fast path that retrieves a single subscription by ID.
Changes:
- Wire
--skip-subscription-discoveryand--subscriptionthrough the profile command module to coreProfile.login. - Implement core login behavior for (1) skip discovery + specific subscription, (2) skip discovery bare tenant-level account, and (3) full discovery with a chosen default subscription.
- Add/update help examples and unit tests covering the new login modes and selector bypass.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/azure-cli/azure/cli/command_modules/profile/tests/latest/test_profile_custom.py | Adds command-module-level tests for tenant requirement and interactive selector bypass behavior. |
| src/azure-cli/azure/cli/command_modules/profile/custom.py | Passes new parameters into core profile login and bypasses the interactive selector when applicable. |
| src/azure-cli/azure/cli/command_modules/profile/_help.py | Documents new az login usage patterns with examples. |
| src/azure-cli/azure/cli/command_modules/profile/init.py | Registers new az login CLI arguments for skipping discovery and selecting a subscription. |
| src/azure-cli-core/azure/cli/core/tests/test_profile.py | Adds core tests for fast-path GET, bare mode, error/warn behavior, and merge behavior. |
| src/azure-cli-core/azure/cli/core/_profile.py | Implements skip discovery / specific subscription logic and default subscription validation/selection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| subscriptions = subscription_finder.find_specific_subscriptions( | ||
| tenant, credential, [default_subscription]) | ||
| elif is_bare_mode: | ||
| # Bare mode: no ARM subscription calls. Tenant-level account will be creatd below |
There was a problem hiding this comment.
Typo in comment: "creatd" should be "created".
| # Bare mode: no ARM subscription calls. Tenant-level account will be creatd below | |
| # Bare mode: no ARM subscription calls. Tenant-level account will be created below |
| is_bare_mode = skip_subscription_discovery and not default_subscription | ||
|
|
||
| if skip_subscription_discovery and default_subscription: | ||
| # Fast path: fetch only the specified subscription (1 API call) | ||
| subscriptions = subscription_finder.find_specific_subscriptions( | ||
| tenant, credential, [default_subscription]) | ||
| elif is_bare_mode: | ||
| # Bare mode: no ARM subscription calls. Tenant-level account will be creatd below | ||
| subscriptions = [] | ||
| subscription_finder.tenants.append(tenant) | ||
| elif tenant: |
There was a problem hiding this comment.
Profile.login accepts skip_subscription_discovery=True but doesn't validate that tenant is provided. If tenant is None, the bare-mode branch appends None to subscription_finder.tenants and _build_tenant_level_accounts will be called with [None], and the fast-path branch will call find_specific_subscriptions(None, ...). Add a core-level usage check (similar to command module custom.login) to raise a CLIError when skip_subscription_discovery is set without tenant to keep behavior consistent for programmatic callers.
| subscriptions = subscription_finder.find_using_common_tenant(username, credential) | ||
|
|
||
| if not subscriptions and not allow_no_subscriptions: | ||
| if not subscriptions and not allow_no_subscriptions and not is_bare_mode: |
There was a problem hiding this comment.
When skip_subscription_discovery=True and a default_subscription is provided but cannot be retrieved, find_specific_subscriptions returns an empty list and this code raises CLIError("No subscriptions found for <username>."). This is misleading because the user asked for a specific subscription. Consider special-casing the fast-path to raise an error that mentions the requested subscription ID (and ideally distinguishes not-found/unauthorized vs transient errors), rather than the generic "No subscriptions found" message.
| if not subscriptions and not allow_no_subscriptions and not is_bare_mode: | |
| if not subscriptions and not allow_no_subscriptions and not is_bare_mode: | |
| if skip_subscription_discovery and default_subscription: | |
| # Fast-path error: a specific subscription was requested but could not be retrieved | |
| raise CLIError( | |
| "The subscription '{}' could not be retrieved for '{}'. " | |
| "Ensure the subscription exists and that you have access to it.".format( | |
| default_subscription, username)) |
| cmd = mock.MagicMock() | ||
| cmd.cli_ctx = DummyCli() | ||
| cmd.cli_ctx.config = mock.MagicMock() | ||
| cmd.cli_ctx.config.getboolean.return_value = True # login_experience_v2 = True | ||
|
|
||
| result = login(cmd, tenant=tenant_id, skip_subscription_discovery=True) | ||
|
|
There was a problem hiding this comment.
These selector-bypass tests depend on sys.stdin.isatty()/sys.stdout.isatty() being true for the selector to be eligible. In many test runners those are false (captured IO), which would make this test pass even if the bypass logic regresses. Patch sys.stdin.isatty and sys.stdout.isatty to return True so the test actually exercises the selector-bypass condition.
| cmd = mock.MagicMock() | ||
| cmd.cli_ctx = DummyCli() | ||
| cmd.cli_ctx.config = mock.MagicMock() | ||
| cmd.cli_ctx.config.getboolean.return_value = False | ||
|
|
||
| # Interactive login (no username) with --subscription | ||
| result = login(cmd, tenant='tenant1', default_subscription=sub_id) | ||
|
|
||
| # Assert selector was never instantiated | ||
| selector_mock.assert_not_called() |
There was a problem hiding this comment.
cmd.cli_ctx.config.getboolean.return_value is set to False here, which disables the interactive selector regardless of --subscription. This makes the test unable to verify that --subscription is what bypasses the selector. Set login_experience_v2 to True (and patch isatty() to True) so the selector would run without --subscription, then assert it is bypassed when default_subscription is provided.
| raise CLIError("usage error: '--use-sn-issuer' is only applicable with a service principal") | ||
| if service_principal and not username: | ||
| raise CLIError('usage error: --service-principal --username NAME --password SECRET --tenant TENANT') | ||
| if skip_subscription_discovery and not tenant: |
There was a problem hiding this comment.
There should be another check for subscription here?
There was a problem hiding this comment.
Could you please clarify what check to add?
Related command
az loginDescription
Wires
--skip-subscription-discoveryand--subscriptionparameters intoaz login. This enables fast login for tenants with 1M+ subscriptions by skipping full subscription enumeration.Three login modes are now supported:
--skip-subscription-discovery --subscription S: Fast path: fetches only the specified subscription viaGET /subscriptions/{id}(1 API call)--skip-subscription-discovery(bare mode): No ARM calls and creates a tenant-level account for tenant-level operations (e.g.,az ad)--subscription S(without skip): Full discovery, then sets S as the defaultBoth
--subscriptionand--skip-subscription-discoverybypass the interactive subscription selector.Related issues: #31939, #14933, #13285
Testing Guide
8 unit tests added in
TestLoginSubscriptionFilter:--skip-subscription-discovery --subscriptionverifiesGETcalled, notLIST--allow-no-subscriptionsverifies warning, tenant-level fallback--subscriptionwithout skip verifies full discovery + default set--subscriptionnot found without skip verifies error3 tests added in
test_profile_custom.py:-
Add validation that --skip-subscription-discovery requires --tenant--skip-subscription-discoverybypasses interactive selector--subscriptionbypasses interactive selectorRun tests:
History Notes
[Core]
az login: Add--skip-subscription-discoveryand--subscriptionparameters to enable fast login for tenants with many subscriptionsThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.