Skip to content

{ACR} az acr check-health: fix check-health command by removing outdated API version gate for CMK check #33117

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.

Sign up for GitHub

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

Open
lizMSFT wants to merge 1 commit into
base: dev
Choose a base branch
from
+18 −20
Open

{ACR} az acr check-health: fix check-health command by removing outdated API version gate for CMK check #33117

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 18 additions & 20 deletions src/azure-cli/azure/cli/command_modules/acr/check_health.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,6 @@ def _get_endpoint_and_token_status(cmd, login_server, registry_abac_enabled, rep


def _check_registry_health(cmd, registry_name, repository, ignore_errors):
from azure.cli.core.profiles import ResourceType
if registry_name is None:
logger.warning("Registry name must be provided to check connectivity.")
return
Expand Down Expand Up @@ -349,25 +348,24 @@ def _check_registry_health(cmd, registry_name, repository, ignore_errors):
registry and registry.role_assignment_mode == RoleAssignmentMode.ABAC_REPOSITORY_PERMISSIONS
_get_endpoint_and_token_status(cmd, login_server, registry_abac_enabled, repository, ignore_errors)

if cmd.supported_api_version(min_api='2020-11-01-preview', resource_type=ResourceType.MGMT_CONTAINERREGISTRY): # pylint: disable=too-many-nested-blocks
# CMK settings
if registry and registry.encryption and registry.encryption.key_vault_properties: # pylint: disable=too-many-nested-blocks
client_id = registry.encryption.key_vault_properties.identity
valid_identity = False
if registry.identity:
valid_identity = ((client_id == 'system') and
bool(registry.identity.principal_id)) # use system identity?
if not valid_identity and registry.identity.user_assigned_identities:
for k, v in registry.identity.user_assigned_identities.items():
if v.client_id == client_id:
from azure.core.exceptions import HttpResponseError
try:
valid_identity = resolve_identity_client_id(cmd.cli_ctx, k) == client_id
except HttpResponseError:
pass
if not valid_identity:
from ._errors import CMK_MANAGED_IDENTITY_ERROR
_handle_error(CMK_MANAGED_IDENTITY_ERROR.format_error_message(registry_name), ignore_errors)
# CMK settings
if registry and registry.encryption and registry.encryption.key_vault_properties: # pylint: disable=too-many-nested-blocks
client_id = registry.encryption.key_vault_properties.identity
Comment on lines +352 to +353
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

After removing the API-version gate, this code path can run under profiles/SDK shapes where the registry model may not define an encryption attribute. Accessing registry.encryption directly would then raise AttributeError and reintroduce a crash in check-health. To make this robust across model versions, use attribute-safe access (e.g., getattr(registry, 'encryption', None) and similar for key_vault_properties) before dereferencing nested properties.

Suggested change
if registry and registry.encryption and registry.encryption.key_vault_properties: # pylint: disable=too-many-nested-blocks
client_id = registry.encryption.key_vault_properties.identity
encryption = getattr(registry, 'encryption', None) if registry is not None else None
key_vault_properties = getattr(encryption, 'key_vault_properties', None) if encryption is not None else None
if registry and key_vault_properties: # pylint: disable=too-many-nested-blocks
client_id = key_vault_properties.identity

Copilot uses AI. Check for mistakes.
valid_identity = False
if registry.identity:
valid_identity = ((client_id == 'system') and
bool(registry.identity.principal_id)) # use system identity?
if not valid_identity and registry.identity.user_assigned_identities:
for k, v in registry.identity.user_assigned_identities.items():
if v.client_id == client_id:
from azure.core.exceptions import HttpResponseError
try:
valid_identity = resolve_identity_client_id(cmd.cli_ctx, k) == client_id
except HttpResponseError:
pass
Comment on lines +359 to +365
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

Importing HttpResponseError inside the loop is unnecessary repeated work and makes the control flow harder to read. Import it once at function scope (or at least before the loop) and keep the loop focused on the identity resolution logic.

Copilot uses AI. Check for mistakes.
if not valid_identity:
from ._errors import CMK_MANAGED_IDENTITY_ERROR
_handle_error(CMK_MANAGED_IDENTITY_ERROR.format_error_message(registry_name), ignore_errors)


def _check_private_endpoint(cmd, registry_name, vnet_of_private_endpoint): # pylint: disable=too-many-locals, too-many-statements
Expand Down
Loading