Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Compute] Add Managed Identity Support in Azure Disk Encryption for VMSS #30657

Merged
merged 10 commits into from
Jan 26, 2025

Conversation

anshuljain26
Copy link
Contributor

@anshuljain26 anshuljain26 commented Jan 14, 2025

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 VMSS 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 VMSS.

This PR adds a new parameter (EncryptionIdentity) to az vmss encryption enable cmdlet. If the parameter is present then the cmdlet will update the EncryptionIdentity field.

Encryption Identity field is also updated during VMSS creation if the encryption identity is a part of the identities assigned to the vmss

Testing Guide

History Notes
[Compute] az vmss create: Add --encryption-identity parameter to use that managed identity for Azure disk encryption
[Compute] az vmss encryption enable: Add --encryption-identity parameter to update or set encryption identity for Azure disk encryption


This checklist is used to make sure that common guidelines for a pull request are followed.

Copy link

azure-client-tools-bot-prd bot commented Jan 14, 2025

️✔️AzureCLI-FullTest
️✔️acr
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️acs
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️advisor
️✔️latest
️✔️3.12
️✔️3.9
️✔️ams
️✔️latest
️✔️3.12
️✔️3.9
️✔️apim
️✔️latest
️✔️3.12
️✔️3.9
️✔️appconfig
️✔️latest
️✔️3.12
️✔️3.9
️✔️appservice
️✔️latest
️✔️3.12
️✔️3.9
️✔️aro
️✔️latest
️✔️3.12
️✔️3.9
️✔️backup
️✔️latest
️✔️3.12
️✔️3.9
️✔️batch
️✔️latest
️✔️3.12
️✔️3.9
️✔️batchai
️✔️latest
️✔️3.12
️✔️3.9
️✔️billing
️✔️latest
️✔️3.12
️✔️3.9
️✔️botservice
️✔️latest
️✔️3.12
️✔️3.9
️✔️cdn
️✔️latest
️✔️3.12
️✔️3.9
️✔️cloud
️✔️latest
️✔️3.12
️✔️3.9
️✔️cognitiveservices
️✔️latest
️✔️3.12
️✔️3.9
️✔️compute_recommender
️✔️latest
️✔️3.12
️✔️3.9
️✔️computefleet
️✔️latest
️✔️3.12
️✔️3.9
️✔️config
️✔️latest
️✔️3.12
️✔️3.9
️✔️configure
️✔️latest
️✔️3.12
️✔️3.9
️✔️consumption
️✔️latest
️✔️3.12
️✔️3.9
️✔️container
️✔️latest
️✔️3.12
️✔️3.9
️✔️containerapp
️✔️latest
️✔️3.12
️✔️3.9
️✔️core
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️cosmosdb
️✔️latest
️✔️3.12
️✔️3.9
️✔️databoxedge
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️dls
️✔️latest
️✔️3.12
️✔️3.9
️✔️dms
️✔️latest
️✔️3.12
️✔️3.9
️✔️eventgrid
️✔️latest
️✔️3.12
️✔️3.9
️✔️eventhubs
️✔️latest
️✔️3.12
️✔️3.9
️✔️feedback
️✔️latest
️✔️3.12
️✔️3.9
️✔️find
️✔️latest
️✔️3.12
️✔️3.9
️✔️hdinsight
️✔️latest
️✔️3.12
️✔️3.9
️✔️identity
️✔️latest
️✔️3.12
️✔️3.9
️✔️iot
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️keyvault
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️lab
️✔️latest
️✔️3.12
️✔️3.9
️✔️managedservices
️✔️latest
️✔️3.12
️✔️3.9
️✔️maps
️✔️latest
️✔️3.12
️✔️3.9
️✔️marketplaceordering
️✔️latest
️✔️3.12
️✔️3.9
️✔️monitor
️✔️latest
️✔️3.12
️✔️3.9
️✔️mysql
️✔️latest
️✔️3.12
️✔️3.9
️✔️netappfiles
️✔️latest
️✔️3.12
️✔️3.9
️✔️network
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️policyinsights
️✔️latest
️✔️3.12
️✔️3.9
️✔️privatedns
️✔️latest
️✔️3.12
️✔️3.9
️✔️profile
️✔️latest
️✔️3.12
️✔️3.9
️✔️rdbms
️✔️latest
️✔️3.12
️✔️3.9
️✔️redis
️✔️latest
️✔️3.12
️✔️3.9
️✔️relay
️✔️latest
️✔️3.12
️✔️3.9
️✔️resource
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️role
️✔️latest
️✔️3.12
️✔️3.9
️✔️search
️✔️latest
️✔️3.12
️✔️3.9
️✔️security
️✔️latest
️✔️3.12
️✔️3.9
️✔️servicebus
️✔️latest
️✔️3.12
️✔️3.9
️✔️serviceconnector
️✔️latest
️✔️3.12
️✔️3.9
️✔️servicefabric
️✔️latest
️✔️3.12
️✔️3.9
️✔️signalr
️✔️latest
️✔️3.12
️✔️3.9
️✔️sql
️✔️latest
️✔️3.12
️✔️3.9
️✔️sqlvm
️✔️latest
️✔️3.12
️✔️3.9
️✔️storage
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️synapse
️✔️latest
️✔️3.12
️✔️3.9
️✔️telemetry
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️util
️✔️latest
️✔️3.12
️✔️3.9
️✔️vm
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9

Copy link

azure-client-tools-bot-prd bot commented Jan 14, 2025

⚠️AzureCLI-BreakingChangeTest
⚠️vm
rule cmd_name rule_message suggest_message
⚠️ 1006 - ParaAdd vmss create cmd vmss create added parameter encryption_identity
⚠️ 1006 - ParaAdd vmss encryption enable cmd vmss encryption enable added parameter encryption_identity

@anshuljain26
Copy link
Contributor Author

@vimish Please review the PR

@yonzhan
Copy link
Collaborator

yonzhan commented Jan 14, 2025

Compute

@evelyn-ys
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@zhoxing-ms
Copy link
Contributor

Could you please refer to this document https://github.com/Azure/azure-cli/blob/dev/doc/managed_identity_command_guideline.md to design commands and parameters related to managed identity?

@anshuljain26
Copy link
Contributor Author

anshuljain26 commented Jan 21, 2025

Could you please refer to this document https://github.com/Azure/azure-cli/blob/dev/doc/managed_identity_command_guideline.md to design commands and parameters related to managed identity?

We have added a new Parameter encryption identity to authenticate to customer's keyvault.
As part of it, a new field (EncryptionIdentity) has been added to the VMSS model. By setting this field customer will be notifying ADE to use that managed identity for keyvault operations. And we want to set this field during vmss creation or update it during the encryption. As VMSS can have multiple identities assigned, we will set encryption identity to one of the identity that will be used to authenticate the key Vault for ADE operation

@@ -2643,6 +2643,9 @@
- name: Create a Debian11 VM scaleset with a user assigned identity.
text: >
az vmss create -n MyVmss -g rg1 --image Debian11 --assign-identity /subscriptions/99999999-1bf0-4dda-aec3-cb9272f09590/resourcegroups/myRG/providers/Microsoft.ManagedIdentity/userAssignedIdentities/myID
- name: Create a vmss with user assigned identity and add encryption identity for Azure disk encryption
text: >
az vmss create -n MyVm -g rg1 --image Debian11 --assign-identity myID --encryption-identity /subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/myRG/providers/Microsoft.ManagedIdentity/userAssignedIdentities/myID --orchestration-mode Uniform --lb-sku Standard
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask what is the difference between the --assign-identity and --encryption-identity?

Copy link
Contributor Author

@anshuljain26 anshuljain26 Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--assign-identity params is used to assign system or user assigned identities associated with the Virtual Machine Scale set. There can be multiple user assigned identities associated with the virtual machine scale set.

--encryption-identity params is used to set which Identity used by ADE to get access token for keyvault operations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks~

Comment on lines 3536 to 3539
if not cmd.supported_api_version(min_api='2023-09-01', resource_type=ResourceType.MGMT_COMPUTE):
raise CLIError("Usage error: Encryption Identity required API version 2023-09-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>")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please set the min_api='2023-09-01' in the parameter definition instead of adding checking here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed Suggestion

Comment on lines 3562 to 3563
raise CLIError("Encryption Identity should be an ARM Resource ID of one of the "
"user assigned identities associated to the resource")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the specific error type ArgumentUsageError in stead of CLIError

Suggested change
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")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During VMSS creation if there is any exception, it will throw cliError exception by default, that's why used that cliError exception here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, we usually recommend using more specific error type if they can be clearly classified, as this will help us in future Telemetry data analysis

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed Suggestion

@yanzhudd yanzhudd merged commit 8a499ff into Azure:dev Jan 26, 2025
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Assign Auto assign by bot Compute az vm/vmss/image/disk/snapshot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants