Storage Container Endpoint Identity Patch #9510
Conversation
️✔️Azure CLI Extensions Breaking Change Test
|
|
Hi @kartikkhullar, |
|
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>
|
CodeGen Tools Feedback CollectionThank you for using our CodeGen tool. We value your feedback, and we would like to know how we can improve our product. Please take a few minutes to fill our codegen survey |
Release SuggestionsModule: storage-mover
Notes
|
There was a problem hiding this comment.
Pull request overview
This PR adds automatic system-assigned managed identity configuration to storage blob container endpoints in the Azure CLI storage-mover extension.
Key Changes:
- Adds
"mi_system_assigned": "True"to the arguments dictionary returned by_get_args_for_endpoint_for_storage_container - This automatically enables system-assigned managed identity when creating or updating storage container endpoints
| "storage_account_resource_id": storage_account_resource_id, | ||
| } | ||
| }, | ||
| "mi_system_assigned": "True" |
There was a problem hiding this comment.
The system-assigned managed identity is being set in the helper function that is used by both create and update operations. This means that every update operation will attempt to set "mi_system_assigned": "True", which may not be the intended behavior. Consider whether the identity assignment should only occur during creation, not during updates.
| "storage_account_resource_id": storage_account_resource_id, | ||
| } | ||
| }, | ||
| "mi_system_assigned": "True" |
There was a problem hiding this comment.
System-assigned managed identity is only being applied to storage blob container endpoints but not to NFS file share endpoints (lines 77-88) or SMB file share endpoints (lines 47-58). This inconsistency could be intentional based on Azure Storage Mover requirements, but if all Azure Storage endpoints require managed identity for authentication, the same pattern should be applied to NFS and SMB file share endpoint creation functions as well.
| "storage_account_resource_id": storage_account_resource_id, | ||
| } | ||
| }, | ||
| "mi_system_assigned": "True" |
There was a problem hiding this comment.
The new system-assigned managed identity feature for storage container endpoints lacks test coverage. The existing test at line 118-127 creates a storage container endpoint but doesn't verify that the identity is properly set. Consider adding a JMESPathCheck to verify that 'identity.type' equals 'SystemAssigned' and that 'identity.principalId' exists, similar to the verification done for NFS file share endpoints at lines 172-174.
3 participants
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally? (pip install wheel==0.30.0required)For new extensions:
About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update
src/index.jsonautomatically.You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify
src/index.json.