[Profile] Drop support for old-style managed identity account #30321
Conversation
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
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
Collaborator
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
microsoft-github-policy-service
bot
requested review from
yonzhan and
zhoxing-ms
microsoft-github-policy-service
bot
added
the
ARM
Member
Author
|
CI failed: I don't think this detection is correct. |
Member
Author
|
I am temporarily altering the history notes to bypass the incorrect detection logic. After the PR is merged, I will change it back:
|
jiasli
commented
Nov 14, 2024
| if parts[0] in MsiAccountTypes.valid_msi_account_types(): | ||
| return parts[0], (None if len(parts) <= 1 else parts[1]) | ||
| def _parse_managed_identity_account(account): | ||
| user_name = account[_USER_ENTITY][_USER_NAME] |
Member
Author
There was a problem hiding this comment.
_USER_NAME must exist:
azure-cli/src/azure-cli-core/azure/cli/core/_profile.py
Lines 491 to 494 in 77f14ba
There is no need to use get().
jiasli
commented
Nov 14, 2024
| # The account contains: | ||
| # "assignedIdentityInfo": "MSIClient-xxx"/"MSIObject-xxx"/"MSIResource-xxx", | ||
| # "name": "userAssignedIdentity", | ||
| return tuple(account[_USER_ENTITY][_ASSIGNED_IDENTITY_INFO].split('-', maxsplit=1)) |
Member
Author
There was a problem hiding this comment.
If _USER_NAME is _SYSTEM_ASSIGNED_IDENTITY or _USER_ASSIGNED_IDENTITY, _ASSIGNED_IDENTITY_INFO must exist:
azure-cli/src/azure-cli-core/azure/cli/core/_profile.py
Lines 276 to 285 in 77f14ba
evelyn-ys
approved these changes
Nov 14, 2024
jiasli
requested review from
bebound,
calvinhzy,
jsntcy,
kairu-ms and
necusjz
as code owners
jiasli
commented
Nov 20, 2024
| test_subscription_id = '12345678-1bf0-4dda-aec3-cb9272f09590' | ||
| test_tenant_id = '12345678-38d6-4fb2-bad9-b7b93a3e1234' | ||
| test_user = 'systemAssignedIdentity' | ||
| msi_subscription = SubscriptionStub('/subscriptions/' + test_subscription_id, 'MSI', self.state1, test_tenant_id) |
Member
Author
There was a problem hiding this comment.
Test cases still use subscription name to store the managed identity info.
bebound
approved these changes
Nov 20, 2024
yanzhudd
pushed a commit
to yanzhudd/azure-cli
that referenced
this pull request
Nov 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Related command
az loginDescription
This PR is separated from #25959 (comment)
In versions <= 2.0.50 (released on November 6, 2018),
_SUBSCRIPTION_NAMEis used to denote the managed identity ID info (_ASSIGNED_IDENTITY_INFO). This is a bad design, as an Azure subscription has its own name. So,_ASSIGNED_IDENTITY_INFOwas added in #7744 to give way to the real subscription name and_try_parse_msi_account_name()acts as an adaptor. However, such logic is difficult to maintain. Even its creator admits:As it has been 6 years since 2.0.50, and Azure CLI is self-consistent, this should not be considered a breaking change.
Testing Guide
History Notes
[Profile] Drop support for old-style managed identity account created by Azure CLI <= 2.0.50. If you upgrade from one of these versions, please run
az login --identityagain.