Skip to content

Add user-assigned managed identity support for ACR cache rules#62

Merged
mabelegba merged 3 commits into
feature/artifactcachefrom
mabe/assign-identity-parameter-clean
Nov 26, 2025
Merged

Add user-assigned managed identity support for ACR cache rules#62
mabelegba merged 3 commits into
feature/artifactcachefrom
mabe/assign-identity-parameter-clean

Conversation

@mabelegba
Copy link
Copy Markdown

Summary

Adds support for user-assigned managed identities in ACR cache rules, enabling secure cross-registry authentication without credential sets.

Changes

  • Added --assign-identity parameter to az acr cache create and az acr cache update
  • Added validation to ensure --sync-referrers enabled requires --sync activesync
  • Updated help documentation and README with examples
  • Added comprehensive unit tests

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

Related command

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally? (pip install wheel==0.30.0 required)
  • My extension version conforms to the Extension version schema

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.json automatically.
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.

…pdate operations

Add user-assigned managed identity support to cache rule create and update operations
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request adds user-assigned managed identity support to Azure Container Registry cache rules, enabling secure cross-registry authentication without requiring credential sets. It also improves validation to ensure --sync-referrers enabled can only be used with --sync activesync.

Key Changes

  • Added --assign-identity parameter to az acr cache create and az acr cache update commands with resource ID validation
  • Enhanced validation logic to prevent enabling sync referrers without activesync mode
  • Comprehensive documentation updates including help text, README examples, and authentication method guidelines

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
setup.py Version bumped from 1.0.0c6 to 1.0.0c8
cache.py Added identity processing functions (process_assign_identity_parameter, is_valid_user_assigned_managed_identity_resource_id) and integrated identity support into create/update operations; improved sync_referrers validation
_params.py Added --assign-identity parameter definition with help text
_help.py Updated help documentation for create/update commands with identity parameter details and usage examples
README.rst Added comprehensive authentication methods section, managed identity setup examples, and clarified sync mode requirements
HISTORY.rst Added changelog entries for versions 1.0.0c7 and 1.0.0c8 documenting the new features and bug fixes
test_cache.py Added extensive unit tests for identity processing, validation logic, and create/update operations with identity support
Comments suppressed due to low confidence (1)

src/acrcache/azext_acrcache/tests/latest/test_cache.py:197

  • Missing test coverage for an important edge case: There is no test case that validates the scenario where a cache rule already has sync_referrers set to "Enabled" and a user tries to update sync to "PassiveSync" or remove it. This edge case is related to the validation gap identified in cache.py lines 310-312. Consider adding a test case like:
def test_acr_cache_update_disable_sync_with_enabled_referrers(self, mock_get_registry):
    # Set up cache rule with sync_referrers already enabled
    self.dummy_rule.properties.sync_referrers = "Enabled"
    self.dummy_rule.properties.sync_mode = "ActiveSync"
    mock_get_registry.return_value = (mock.Mock(id="id"), "mockrg")
    self.client.get.return_value = self.dummy_rule
    
    with self.assertRaises(CLIError):
        cache.acr_cache_update_custom(
            self.cmd, self.client, "mockRegistry", "mockCacheRule1",
            sync="passivesync"  # Trying to disable activesync while referrers are enabled
        )
class TestCacheUpdateValidation(unittest.TestCase):
    """Test cache update validation logic"""
    
    def setUp(self):
        """Set up test fixtures"""
        self.cmd = mock.Mock()
        self.cmd.cli_ctx = mock.Mock()
        self.client = mock.Mock()
        
        # Set up mock cache rule
        self.dummy_rule = mock.Mock()
        self.dummy_rule.properties = mock.Mock()
        self.dummy_rule.properties.sync_mode = "activeSync"
        self.dummy_rule.properties.sync_referrers = "Disabled"
        self.dummy_rule.properties.artifact_sync_filters = None
        self.dummy_rule.properties.credential_set_resource_id = None
        
    @mock.patch('azext_acrcache.cache.get_registry_by_name')
    def test_acr_cache_update_custom_mutually_exclusive_artifact_types(self, mock_get_registry):
        """Test that cache update raises error for mutually exclusive artifact type filters"""
        mock_get_registry.return_value = (mock.Mock(id="id"), "mockrg")
        self.client.get.return_value = self.dummy_rule
        
        with self.assertRaises(CLIError):
            cache.acr_cache_update_custom(
                self.cmd, self.client, "mockRegistry", "mockCacheRule1",
                include_artifact_types="a", exclude_artifact_types="b"
            )

    @mock.patch('azext_acrcache.cache.get_registry_by_name')
    def test_acr_cache_update_custom_mutually_exclusive_image_types(self, mock_get_registry):
        """Test that cache update raises error for mutually exclusive image type filters"""
        mock_get_registry.return_value = (mock.Mock(id="id"), "mockrg")
        self.client.get.return_value = self.dummy_rule
        
        with self.assertRaises(CLIError):
            cache.acr_cache_update_custom(
                self.cmd, self.client, "mockRegistry", "mockCacheRule1",
                include_image_types="a", exclude_image_types="b"
            )

Comment thread src/acrcache/README.rst Outdated
Comment thread src/acrcache/azext_acrcache/cache.py Outdated
Comment thread src/acrcache/azext_acrcache/cache.py Outdated
Comment thread src/acrcache/azext_acrcache/cache.py Outdated
Comment thread src/acrcache/HISTORY.rst Outdated
Comment thread src/acrcache/azext_acrcache/cache.py Outdated
Comment thread src/acrcache/azext_acrcache/cache.py Outdated
Comment thread src/acrcache/azext_acrcache/cache.py
Comment thread src/acrcache/azext_acrcache/tests/latest/test_cache.py Outdated
Comment thread src/acrcache/azext_acrcache/_help.py
Copy link
Copy Markdown

@cegraybl cegraybl left a comment

Choose a reason for hiding this comment

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

LGTM, some minor comments

Comment thread src/acrcache/HISTORY.rst Outdated
Comment thread src/acrcache/azext_acrcache/cache.py Outdated
Comment thread src/acrcache/azext_acrcache/cache.py
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

# --------------------------------------------------------------------------------------------
# pylint: disable=line-too-long

import re
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

The re module is imported at the top level but is not used until line 172, where it is imported again within a function scope. Since the top-level import already exists, remove the redundant import statement at line 171.

Suggested change
import re

Copilot uses AI. Check for mistakes.

# These are accepted by Azure's parsing utilities (which is by design)
for accepted_id in potentially_invalid_but_accepted_ids:
result = cache.is_valid_user_assigned_managed_identity_resource_id(accepted_id)
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

The test computes the result but does not assert anything about it. This test case should include assertions to verify the expected behavior. Based on the comment at line 305, these IDs are 'accepted by Azure's parsing utilities', so the test should likely verify that the function returns True for these cases, or document the expected behavior more explicitly.

Copilot uses AI. Check for mistakes.
Comment thread src/acrcache/HISTORY.rst

Release History
===============
1.0.0c8
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

The version 1.0.0c8 changelog includes both the managed identity feature (which should be in c7) and the sync-referrers bugfix. According to the version history structure, version 1.0.0c7 (lines 11-19) already documents the --assign-identity feature. The c8 entry should focus only on the bugfix for sync-referrers validation, not re-document the identity feature implementation details from line 8.

Copilot uses AI. Check for mistakes.
@mabelegba mabelegba merged commit 6ae6acb into feature/artifactcache Nov 26, 2025
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants