[App Service] Fix container identity ACR pull and Key Vault reference protection#33064
Open
[App Service] Fix container identity ACR pull and Key Vault reference protection#33064
Conversation
…managed identity ACR pull support Add managed identity support for ACR image pulling in `az webapp config container set`: - Add --assign-identity, --scope, --role params for identity assignment - Add --acr-use-identity flag to enable/disable ACR pull via managed identity - Add --acr-identity param to specify which identity to use for ACR pull - Update update_container_settings() to call assign_identity() and update_site_configs() with ACR identity parameters - Add unit tests for system identity, user identity, custom role, disable identity, and no-identity scenarios - Update help text with managed identity example This follows the same patterns used by `az webapp create` for ACR identity support, using the existing assign_identity() and update_site_configs() infrastructure. Fixes Azure#28858 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
seligj95
requested review from
NoriZC,
teresaritorto and
yanzhudd
as code owners
❌AzureCLI-FullTest
|
|
Hi @seligj95, |
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| webapp config container set | cmd webapp config container set added parameter acr_identity |
||
| webapp config container set | cmd webapp config container set added parameter acr_use_identity |
||
| webapp config container set | cmd webapp config container set added parameter assign_identities |
||
| webapp config container set | cmd webapp config container set added parameter role |
||
| webapp config container set | cmd webapp config container set added parameter scope |
Collaborator
|
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
requested review from
jsntcy,
terencet-dev,
wangzelin007,
yonzhan and
zhoxing-ms
microsoft-github-policy-service
bot
added
Web Apps
Contributor
There was a problem hiding this comment.
Pull request overview
Adds managed identity support for pulling container images from Azure Container Registry (ACR) when running az webapp config container set, aligning container-update behavior with existing managed-identity ACR pull support elsewhere in App Service.
Changes:
- Adds new CLI parameters to
webapp config container setfor assigning managed identities and controlling ACR pull via managed identity (--assign-identity,--scope,--role,--acr-use-identity,--acr-identity). - Extends
update_container_settingsto optionally assign identities and update site config ACR identity settings. - Adds help example and new mock-based unit tests for the identity-enabled flows.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/azure-cli/azure/cli/command_modules/appservice/custom.py |
Extends update_container_settings to optionally assign identities and set ACR MI site-config flags/identity. |
src/azure-cli/azure/cli/command_modules/appservice/_params.py |
Wires new arguments into webapp config container set. |
src/azure-cli/azure/cli/command_modules/appservice/_help.py |
Adds a managed-identity ACR pull example for webapp config container set. |
src/azure-cli/azure/cli/command_modules/appservice/tests/latest/test_webapp_commands_thru_mock.py |
Adds unit tests validating identity-related calls made by update_container_settings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...zure-cli/azure/cli/command_modules/appservice/tests/latest/test_webapp_commands_thru_mock.py
Show resolved
Hide resolved
- Short-circuit _get_acr_cred when acr_use_identity is set to avoid persisting admin credentials that defeat MI-based pulls - Add regression test asserting _get_acr_cred is not called in MI flow Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…erve Key Vault references When running `az webapp config container set`, existing app settings containing Key Vault references (e.g., @Microsoft.KeyVault(...)) were being overwritten. This happened because: 1. ACR credential auto-detection replaced KV-ref credentials with raw values when a .azurecr.io URL was provided without explicit username/password. 2. The settings update used a read-modify-write cycle through update_app_settings that could interfere with non-container settings. This fix: - Adds _is_key_vault_reference() helper to detect KV reference values. - Skips ACR credential auto-detection when existing username/password settings are Key Vault references. - Merges only container-specific keys directly into the existing properties dict, preserving all other app settings as-is. - Adds unit tests verifying KV references survive container set operations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
seligj95
changed the title
az webapp config container set: Add managed identity ACR pull support…alls, strengthen test assertions - When acr_use_identity is set, short-circuit credential lookup entirely (explicit early branch instead of relying on compound condition) - Accumulate all site-config changes (multicontainer, replicas, ACR identity) into a single update_site_configs call to reduce latency and failure surface - Add _get_acr_cred mock to all MI tests and assert it is never called - Add update_site_configs value assertions to custom-role test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
seligj95
changed the title
Extract _build_container_updates helper to reduce branch count in update_container_settings from 22 to 18 (max 20). Fix continuation line indentation for visual alignment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR consolidates two related fixes for
az webapp config container set:Fix #28858: Managed identity ACR pull support
--assign-identity,--role,--scope,--acr-use-identity, and--acr-identityparametersFix #21907: Preserve Key Vault references
@Microsoft.KeyVault(...)) in existing registry credentials and skips automatic ACR credential lookup when presentTesting
test_webapp_commands_thru_mock.pyKnown Limitations
--docker-custom-image-nameflag is not compatible with the new--acr-use-identityparameter due to argparse group conflicts. Users should use the current--container-image-nameflag instead. This only affects the deprecated code path and is not a regression.Fixes #28858
Fixes #21907
Previously #33071, consolidated here.