{Compute} az vmss identity: Migrate commands to aaz-based implementation
#32685
Conversation
william051200
requested review from
jsntcy,
wangzelin007,
yanzhudd and
zhoxing-ms
as code owners
️✔️AzureCLI-FullTest
|
|
Hi @william051200, |
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
️✔️AzureCLI-BreakingChangeTest
|
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
microsoft-github-policy-service
bot
added
the
Compute
There was a problem hiding this comment.
Pull request overview
This PR migrates az vmss identity operations (assign, remove, show) from the mgmt SDK implementation to the aaz-based implementation and updates tests and utilities to match the new flow.
Changes:
- Re-route
vmss identitycommands to custom/aaz-based implementations and adjust command wiring incommands.py. - Implement vmss identity show/assign/remove using AAZ-based helpers and new identity utilities (
IdentityType,UpgradeMode), including VMSS-specific patch and identity-removal flows. - Update MSI-related VMSS tests to use specific VM sizes/OS images compatible with the new behavior.
Reviewed changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/azure-cli/azure/cli/command_modules/vm/tests/latest/test_vm_commands.py |
Adjusts VMSS MSI tests (images and --vm-sku) and adds coverage for the new VMSS identity behavior. |
src/azure-cli/azure/cli/command_modules/vm/operations/vmss.py |
Introduces VMSSPatch and VMSSIdentityRemove AAZ wrappers to normalize extension typing conflicts and implement AAZ-based VMSS identity removal. |
src/azure-cli/azure/cli/command_modules/vm/custom.py |
Switches VMSS identity show/assign/remove to use AAZ-based helpers (get_vmss_by_aaz, _remove_identities_by_aaz, IdentityType, UpgradeMode) and normalizes identity payloads. |
src/azure-cli/azure/cli/command_modules/vm/commands.py |
Rebinds vmss identity commands to a custom (non-SDK) command group while keeping other VMSS operations on the SDK-backed group. |
src/azure-cli/azure/cli/command_modules/vm/_vm_utils.py |
Adds IdentityType and UpgradeMode enums and an assign_identity helper used by the new VMSS identity assign path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if include_user_data: | ||
| command_args['expand'] = 'userData' | ||
|
|
||
| if instance_id is not None: |
There was a problem hiding this comment.
get_vmss_by_aaz accepts an instance_id argument but the instance_id is never added to command_args before calling VMSSVMSShow, so any future caller that passes instance_id will not get the expected per-instance result. Please include the instance_id in command_args when instance_id is not None, consistent with how get_vmss_modified_by_aaz and other helpers wire through this parameter.
| if instance_id is not None: | |
| if instance_id is not None: | |
| command_args['instance_id'] = instance_id |
| return None | ||
|
|
||
| for user_identity in identity.get('userAssignedIdentities', {}).keys(): | ||
| if not identity['userAssignedIdentities'][user_identity].get('clientId'): | ||
| identity['userAssignedIdentities'][user_identity]['clientId'] = None | ||
| if not identity['userAssignedIdentities'][user_identity].get('principalId'): | ||
| identity['userAssignedIdentities'][user_identity]['principalId'] = None | ||
|
|
There was a problem hiding this comment.
In VMSSIdentityRemove._output, when identity.get('userAssignedIdentities') is falsy you set identity['userAssignedIdentities'] = None and then return None, which causes callers like _remove_identities_by_aaz to see a None result even when a system-assigned identity still exists. This differs from the VM path (VMIdentityRemove._output in operations/vm.py:201-208), where the full result is always returned, and it prevents consumers from inspecting the remaining identity state after a successful removal; please return the result object in all success cases (after normalizing userAssignedIdentities) so that _remove_identities_by_aaz can consistently access result['identity'].
| return None | |
| for user_identity in identity.get('userAssignedIdentities', {}).keys(): | |
| if not identity['userAssignedIdentities'][user_identity].get('clientId'): | |
| identity['userAssignedIdentities'][user_identity]['clientId'] = None | |
| if not identity['userAssignedIdentities'][user_identity].get('principalId'): | |
| identity['userAssignedIdentities'][user_identity]['principalId'] = None | |
| else: | |
| for user_identity in identity.get('userAssignedIdentities', {}).keys(): | |
| if not identity['userAssignedIdentities'][user_identity].get('clientId'): | |
| identity['userAssignedIdentities'][user_identity]['clientId'] = None | |
| if not identity['userAssignedIdentities'][user_identity].get('principalId'): | |
| identity['userAssignedIdentities'][user_identity]['principalId'] = None |
| return client.virtual_machine_scale_sets.get(resource_group_name, vmss_name) | ||
| if vmss.get('identity') and vmss.get('identity').get('type') == IdentityType.USER_ASSIGNED.value: | ||
| # NOTE: The literal 'UserAssigned' is intentionally appended as a marker for | ||
| # VMIdentityRemove._format_content, which uses it to apply special handling |
There was a problem hiding this comment.
The comment here refers to VMIdentityRemove._format_content, but the implementation actually uses VMSSIdentityRemove from operations.vmss, which can be confusing when maintaining the VMSS-specific flow. To avoid misleading future readers, please update the comment to reference the correct class/method (or to describe the behavior more generically) so it accurately documents the VMSS identity removal path.
| # VMIdentityRemove._format_content, which uses it to apply special handling | |
| # VMSSIdentityRemove._format_content, which uses it to apply special handling |
| if not identity: | ||
| return None | ||
|
|
||
| user_assigned = identity.get('userAssignedIdentities', {}) | ||
|
|
||
| if not user_assigned: | ||
| identity['userAssignedIdentities'] = None | ||
| else: | ||
| for user_identity in user_assigned.keys(): | ||
| if not user_assigned.get(user_identity).get('clientId'): | ||
| user_assigned[user_identity]['clientId'] = None | ||
| if not user_assigned.get(user_identity).get('principalId'): | ||
| user_assigned[user_identity]['principalId'] = None | ||
|
|
||
| if not identity.get('principalId'): | ||
| identity['principalId'] = None | ||
|
|
||
| if not identity.get('tenantId'): | ||
| identity['tenantId'] = None | ||
|
|
||
| return identity or None |
There was a problem hiding this comment.
may I ask why we need to make these changes?
There was a problem hiding this comment.
The purpose of this change is to match the same output structure before migration.
For example:
Before set "None" to the field
{
"systemAssignedIdentity": "",
"userAssignedIdentities": {
"/subscriptions/fe3bc2b7-22b0-4133-af8f-9439cb831236/resourceGroups/william-rg/providers/Microsoft.ManagedIdentity/userAssignedIdentities/id1": {}
}
}
After set "None" to the field:
{
"systemAssignedIdentity": "",
"userAssignedIdentities": {
"/subscriptions/fe3bc2b7-22b0-4133-af8f-9439cb831236/resourceGroups/william-rg/providers/Microsoft.ManagedIdentity/userAssignedIdentities/id1": {
"clientId": null,
"principalId": null
}
}
}
4 participants
Related command
az vmss identity assignaz vmss identity removeaz vmss identity showDescription
Migration from mgmt.compute to aaz-based
Testing Guide
History Notes
This 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.