{Compute} az vm identity: Migrate commands to aaz-based implementation#32572
{Compute} az vm identity: Migrate commands to aaz-based implementation#32572
az vm identity: Migrate commands to aaz-based implementation#32572Conversation
william051200
requested review from
jsntcy,
wangzelin007,
yanzhudd and
zhoxing-ms
as code owners
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
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
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR migrates the az vm identity commands (assign, remove, and show) from the older mgmt.compute SDK to the AAZ (Azure AutoRest) framework. The migration modernizes the command implementation while maintaining backward compatibility.
Key Changes
- Migrated
show_vm_identity,assign_vm_identity, andremove_vm_identityto use AAZ-based operations instead of mgmt.compute SDK - Added helper functions
assign_identityandresolve_role_idto_vm_utils.pyto support the new implementation - Introduced
IdentityTypeenum for better type management - Created
VMIdentityRemoveclass inoperations/vm.pyto handle identity removal logic - Updated test commands to include
--sizeparameter for VM creation
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| test_vm_commands.py | Updated test cases with explicit VM size specifications and reformatted command strings for better readability. No functional test changes. |
| operations/vm.py | Added VMIdentityRemove class extending _VMPatch to handle identity removal with custom content formatting logic. |
| custom.py | Migrated three identity functions to use AAZ operations: show_vm_identity, assign_vm_identity, and remove_vm_identity. Added get_vm_by_aaz helper and _remove_identities_by_aaz function. |
| _vm_utils.py | Added utility functions (assign_identity, resolve_role_id, _gen_guid) and IdentityType enum to support AAZ-based identity operations. Consolidated imports at the top. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| from ._vm_utils import IdentityType | ||
| if vm.get('identity') and vm.get('identity').get('type') == IdentityType.USER_ASSIGNED.value: |
There was a problem hiding this comment.
There's a redundant 'UserAssigned' literal string being added to the mi_user_assigned list. This appears to be a workaround for special handling in VMIdentityRemove._format_content (line 194), but it creates confusion since 'UserAssigned' is not a valid identity resource ID. Consider documenting this special case with a comment explaining why 'UserAssigned' is added.
| if vm.get('identity') and vm.get('identity').get('type') == IdentityType.USER_ASSIGNED.value: | |
| if vm.get('identity') and vm.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 | |
| # for purely user-assigned identities. It is not a real identity resource ID. |
| ([key for key in list(vm.get('identity', {}).get('userAssignedIdentities', {}).keys())] + | ||
| ['UserAssigned']) | ||
| elif vm.get('identity') and vm.get('identity').get('type') == IdentityType.SYSTEM_ASSIGNED.value: | ||
| command_args['mi_user_assigned'] = [] | ||
| command_args['mi_system_assigned'] = 'True' | ||
| elif vm.get('identity') and vm.get('identity').get('type') == IdentityType.SYSTEM_ASSIGNED_USER_ASSIGNED.value: | ||
| command_args['mi_user_assigned'] = \ | ||
| [key for key in list(vm.get('identity', {}).get('userAssignedIdentities', {}).keys())] |
There was a problem hiding this comment.
The list() call is unnecessary here as dict.keys() already returns an iterable that can be directly used in the list comprehension. This creates an extra intermediate list.
| ([key for key in list(vm.get('identity', {}).get('userAssignedIdentities', {}).keys())] + | |
| ['UserAssigned']) | |
| elif vm.get('identity') and vm.get('identity').get('type') == IdentityType.SYSTEM_ASSIGNED.value: | |
| command_args['mi_user_assigned'] = [] | |
| command_args['mi_system_assigned'] = 'True' | |
| elif vm.get('identity') and vm.get('identity').get('type') == IdentityType.SYSTEM_ASSIGNED_USER_ASSIGNED.value: | |
| command_args['mi_user_assigned'] = \ | |
| [key for key in list(vm.get('identity', {}).get('userAssignedIdentities', {}).keys())] | |
| (list(vm.get('identity', {}).get('userAssignedIdentities', {}).keys()) + | |
| ['UserAssigned']) | |
| elif vm.get('identity') and vm.get('identity').get('type') == IdentityType.SYSTEM_ASSIGNED.value: | |
| command_args['mi_user_assigned'] = [] | |
| command_args['mi_system_assigned'] = 'True' | |
| elif vm.get('identity') and vm.get('identity').get('type') == IdentityType.SYSTEM_ASSIGNED_USER_ASSIGNED.value: | |
| command_args['mi_user_assigned'] = \ | |
| list(vm.get('identity', {}).get('userAssignedIdentities', {}).keys()) |
| ([key for key in list(vm.get('identity', {}).get('userAssignedIdentities', {}).keys())] + | ||
| ['UserAssigned']) | ||
| elif vm.get('identity') and vm.get('identity').get('type') == IdentityType.SYSTEM_ASSIGNED.value: | ||
| command_args['mi_user_assigned'] = [] | ||
| command_args['mi_system_assigned'] = 'True' | ||
| elif vm.get('identity') and vm.get('identity').get('type') == IdentityType.SYSTEM_ASSIGNED_USER_ASSIGNED.value: | ||
| command_args['mi_user_assigned'] = \ | ||
| [key for key in list(vm.get('identity', {}).get('userAssignedIdentities', {}).keys())] |
There was a problem hiding this comment.
The list() call is unnecessary here as dict.keys() already returns an iterable that can be directly used in the list comprehension. This creates an extra intermediate list.
| ([key for key in list(vm.get('identity', {}).get('userAssignedIdentities', {}).keys())] + | |
| ['UserAssigned']) | |
| elif vm.get('identity') and vm.get('identity').get('type') == IdentityType.SYSTEM_ASSIGNED.value: | |
| command_args['mi_user_assigned'] = [] | |
| command_args['mi_system_assigned'] = 'True' | |
| elif vm.get('identity') and vm.get('identity').get('type') == IdentityType.SYSTEM_ASSIGNED_USER_ASSIGNED.value: | |
| command_args['mi_user_assigned'] = \ | |
| [key for key in list(vm.get('identity', {}).get('userAssignedIdentities', {}).keys())] | |
| (list(vm.get('identity', {}).get('userAssignedIdentities', {}).keys()) + | |
| ['UserAssigned']) | |
| elif vm.get('identity') and vm.get('identity').get('type') == IdentityType.SYSTEM_ASSIGNED.value: | |
| command_args['mi_user_assigned'] = [] | |
| command_args['mi_system_assigned'] = 'True' | |
| elif vm.get('identity') and vm.get('identity').get('type') == IdentityType.SYSTEM_ASSIGNED_USER_ASSIGNED.value: | |
| command_args['mi_user_assigned'] = \ | |
| list(vm.get('identity', {}).get('userAssignedIdentities', {}).keys()) |
| for key in list(identities.keys()): | ||
| identities[key] = None | ||
|
|
||
| if len(content.get('identity', {}).get('userAssignedIdentities', {}).keys()) < 1: |
There was a problem hiding this comment.
This code checks if the length is less than 1, but this is equivalent to checking if the dictionary is empty. Using if not content.get('identity', {}).get('userAssignedIdentities', {}) would be more idiomatic and readable.
| if len(content.get('identity', {}).get('userAssignedIdentities', {}).keys()) < 1: | |
| if not content.get('identity', {}).get('userAssignedIdentities', {}): |
|
|
||
| # create role assignment: | ||
| if identity_scope: | ||
| principal_id = resource.get('identity', {}).get('principal_id') |
There was a problem hiding this comment.
The key name 'principal_id' should be 'principalId' to match the camelCase format used in AAZ-based responses. Throughout the codebase (such as in assign_vm_identity at line 900), 'principalId' is used consistently when accessing AAZ response dictionaries.
| principal_id = resource.get('identity', {}).get('principal_id') | |
| principal_id = resource.get('identity', {}).get('principalId') or \ | |
| resource.get('identity', {}).get('principal_id') |
william051200
force-pushed
the
vm-identity-migration
branch
from
e21034b to
7e32359
Compare
| if identity and not identity.get('userAssignedIdentities'): | ||
| identity['userAssignedIdentities'] = None | ||
|
|
||
| return identity or None |
There was a problem hiding this comment.
We may need to consider whether this should return an empty dictionary ({}) or None when there is no identity on the VM. We can refer to the SDK behavior for guidance.
There was a problem hiding this comment.
Here is the output from SDK behavior, it returns None instead of of ( { } ) for empty result.
| identity_types = ResourceIdentityType.system_assigned_user_assigned | ||
| elif vm.identity and vm.identity.type == ResourceIdentityType.user_assigned and enable_local_identity: | ||
| identity_types = ResourceIdentityType.system_assigned_user_assigned | ||
| if vm.get('identity') and vm.get('identity').get('type') == system_assigned_user_assigned: |
There was a problem hiding this comment.
just for your reference, adding a default value to the get() function of dict might be more concise in this case
| if vm.get('identity') and vm.get('identity').get('type') == system_assigned_user_assigned: | |
| if vm.get('identity', {}).get('type', None) == system_assigned_user_assigned: |
| system_assigned = "SystemAssigned" | ||
| user_assigned = "UserAssigned" | ||
| system_assigned_user_assigned = "SystemAssigned, UserAssigned" |
There was a problem hiding this comment.
since they are already defined in _vm_utils.py file, it would be better to leverage the definition across all the places that need to read the values
| if not identity.get('principalId'): | ||
| identity['principalId'] = None | ||
|
|
||
| if not identity.get('tenantId'): | ||
| identity['tenantId'] = None |
There was a problem hiding this comment.
may I ask why we need to add the None value to the result?
There was a problem hiding this comment.
I have just validated, I was using them when running some test, it is now redundant. I will be removing those lines. Thank you.
| if 'UserAssigned' in identities.keys(): | ||
| identities.pop('UserAssigned') |
There was a problem hiding this comment.
May I ask what the identities look like in this case?
There was a problem hiding this comment.
The reason I have implemented this solution is because:
- VM Patch will always set userAssigned and systemAssigned action flag to
create. - VM Patch only accept a list of userAssigned identities, then
AAZIdentityObjectwill always format the identity to create based on thecreateaction. - Therefore, I will only pass identity to be removed to the list as I did a little twist by inheriting the original VM Patch, then set the identities to be removed into this format
{'identity':None}. (I found this is the only format to remove userAssigned identity from the list) - It works until I need to remove systemAssigned identity from systemAssigned + userAssigned identities, this is because passing userAssigned as empty list (because I am not removing any userAssigned identity) and
AAZIdentityObjectwill be converting the identity toNoneinstead ofUserAssigned, cause the removal of systemAssigned + userAssigned identities. - The workaround is, I have added a placeholder into the userAssigned identity list and send to customized aaz in this format
{'resource_group': 'william-rg', 'vm_name': 'william-vm1', 'mi_user_assigned': ['UserAssigned']}. This way, AAZIdentityObject will set the type of identity toUserAssigned, hence retain the userAssigned identities. - Then I remove the placeholder afterwards in the inherited function.
Result:
|
|
||
| return json.dumps(content) | ||
|
|
||
| def __call__(self, *args, **kwargs): |
There was a problem hiding this comment.
Why do we need to manually write this __call__() function here, as I remember it is already defined in the parent class
There was a problem hiding this comment.
Yes, the call function exists but is it not workable for 'removing identity using vm patch class'. Reason as:
This is _update.py, it does support pre_instance_update, which modifies the content of request before calling the update API.
This is _patch.py, it does not support pre_instance_update, and I do not find any other function to perform the same function as pre_instance_update
And that is why I have created __call__ function and added one function to modify the content
yanzhudd
changed the title
az vm identity: Migrate commands to aaz-based implementation
|
since the changes are not visible to users, the title should be started with |
|
could you please double check the changes to the recording yaml files? We need to ensure there is no difference for the command behavior after migrating to aaz-based |
william051200
force-pushed
the
vm-identity-migration
branch
from
874946d to
29a399e
Compare
Related command
az vm identity assignaz vm identity removeaz vm 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.