[Compute] Add Managed Identity Support in Azure Disk Encryption #30457
Conversation
️✔️AzureCLI-FullTest
|
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| vm create | cmd vm create added parameter encryption_identity |
||
| vm encryption enable | cmd vm encryption enable added parameter encryption_identity |
|
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
anshuljain26
requested review from
zhoxing-ms,
jsntcy,
wangzelin007 and
yanzhudd
as code owners
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| if not cmd.supported_api_version(min_api='2023-07-01', resource_type=ResourceType.MGMT_COMPUTE): | ||
| raise CLIError("Usage error: Encryption identity is not available under current profile." | ||
| "You can set the cloud's profile to latest with:" | ||
| "az cloud set --profile latest --name <cloud name>") |
There was a problem hiding this comment.
| if not cmd.supported_api_version(min_api='2023-07-01', resource_type=ResourceType.MGMT_COMPUTE): | |
| raise CLIError("Usage error: Encryption identity is not available under current profile." | |
| "You can set the cloud's profile to latest with:" | |
| "az cloud set --profile latest --name <cloud name>") | |
| if not cmd.supported_api_version(min_api='2023-07-01', resource_type=ResourceType.MGMT_COMPUTE): | |
| raise CLIError("Usage error: Encryption identity requires API version 2023-07-01 or higher." | |
| "You can set the cloud's profile to use the required API version with:" | |
| "az cloud set --profile latest --name <cloud name>") |
There was a problem hiding this comment.
Addressed suggestion
| if not cmd.supported_api_version(min_api='2023-07-01', resource_type=ResourceType.MGMT_COMPUTE): | ||
| raise CLIInternalError("Usage error: Encryption identity is not available under current profile." | ||
| "You can set the cloud's profile to latest with:" | ||
| "az cloud set --profile latest --name <cloud name>") |
There was a problem hiding this comment.
| if not cmd.supported_api_version(min_api='2023-07-01', resource_type=ResourceType.MGMT_COMPUTE): | |
| raise CLIInternalError("Usage error: Encryption identity is not available under current profile." | |
| "You can set the cloud's profile to latest with:" | |
| "az cloud set --profile latest --name <cloud name>") | |
| if not cmd.supported_api_version(min_api='2023-07-01', resource_type=ResourceType.MGMT_COMPUTE): | |
| raise CLIInternalError("Usage error: Encryption identity requires API version 2023-07-01 or higher." | |
| "You can set the cloud's profile to use the required API version with:" | |
| "az cloud set --profile latest --name <cloud name>") |
There was a problem hiding this comment.
Addressed suggestion
|
/azp run |
|
Commenter does not have sufficient privileges for PR 30457 in repo Azure/azure-cli |
anshuljain26
changed the title
anshuljain26
changed the title
| if 'encryptionIdentity' not in vm_resource['properties']['securityProfile']: | ||
| vm_resource['properties']['securityProfile']['encryptionIdentity'] = {} | ||
| if 'userAssignedIdentityResourceId' not \ | ||
| in vm_resource['properties']['securityProfile']['encryptionIdentity'] or \ | ||
| vm_resource['properties']['securityProfile']['encryptionIdentity']['userAssignedIdentityResourceId'] \ | ||
| != encryption_identity: | ||
| vm_resource['properties']['securityProfile']['encryptionIdentity']['userAssignedIdentityResourceId'] \ | ||
| = encryption_identity |
There was a problem hiding this comment.
This judgment condition is too long. Could we assign vm_resource['properties']['securityProfile']['encryptionIdentity'] to a variable to simplify its condition
There was a problem hiding this comment.
Addressed suggestion
| raise CLIError("Encryption Identity should be an ARM Resource ID of one of the " | ||
| "user assigned identities associated to the resource") |
There was a problem hiding this comment.
| raise CLIError("Encryption Identity should be an ARM Resource ID of one of the " | |
| "user assigned identities associated to the resource") | |
| raise ArgumentUsageError("Encryption Identity should be an ARM Resource ID of one of the " | |
| "user assigned identities associated to the resource") |
There was a problem hiding this comment.
During VM creation if there is any exception, it will throw cliError exception by default, that's why used that cliError exception here
| raise CLIError("Encryption Identity should be an ARM Resource ID of one of the " | ||
| "user assigned identities associated to the resource") |
There was a problem hiding this comment.
| raise CLIError("Encryption Identity should be an ARM Resource ID of one of the " | |
| "user assigned identities associated to the resource") | |
| raise ArgumentUsageError("Encryption Identity should be an ARM Resource ID of one of the " | |
| "user assigned identities associated to the resource") |
There was a problem hiding this comment.
Addressed suggestion
|
Please refer to this guideline https://github.com/Azure/azure-cli/tree/dev/doc/authoring_command_modules#submitting-pull-requests to add relevant history note, which should include specific new parameter explanations |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@wangzelin007 All the check are working fine and addressed all the comments, could you please help to review the PR? |
|
LGTM. |
|
@zhoxing-ms Could you please help to review and approve the PR? |
please address this comment, as to specify the impacted commands/parameters in the History Notes section, e.g., |
|
There is a guideline about adding support for managed identity in Azure CLI: https://github.com/Azure/azure-cli/blob/dev/doc/managed_identity_command_guideline.md |
|
Please address these comments from Yan, I don't have any other comments anymore~ |
We have added a new Parameter encryption identity to authenticate to customer's keyvault. |
Done |
|
please follow the specifications in the guideline to submit future PRs and use `` character to quote the command/parameter in the History Notes/title section: |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Could someone please run /azp run |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@yanzhudd All checks are passing, please have a look |
Related command
Description
Azure Disk Encryption (ADE) is adding support for using managed identity to authenticate to customer's keyvault.
As part of it, a new field (EncryptionIdentity) has been added to the VM model. By setting this field customer will be notifying ADE to use that managed identity for keyvault operations. The identity should also be explicitly assigned to the VM.
This PR adds a new parameter (EncryptionIdentity) to Set-AzVMDiskEncryptionExtension cmdlet. If the parameter is present then the cmdlet will update the EncryptionIdentity field.
Encryption Identity field is also updated during VM creation if the encryption identity is a part of the identities assigned to the vm
Testing Guide
History Notes
[Compute]
az vm create: Add--encryption-identityparameter to use that managed identity for Azure disk encryption[Compute]
az vm encryption enable: Add--encryption-identityparameter to update or set encryption identity for Azure disk encryptionThis 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.