[ACR] Support Custom ACR Scope for Disconnected Clouds (ALDO)#33294
[ACR] Support Custom ACR Scope for Disconnected Clouds (ALDO)#33294
Conversation
RohanPawarMSFT
requested review from
NoriZC,
teresaritorto and
yanzhudd
as code owners
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
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>
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
microsoft-github-policy-service
Bot
requested review from
jiasli,
northtyphoon,
terencet-dev,
wangzelin007 and
yonzhan
@microsoft-github-policy-service agree company="Microsoft" |
There was a problem hiding this comment.
Pull request overview
Adds configurability for the ACR AAD token “audience/resource” used during az acr login, enabling disconnected cloud environments to override the default https://containerregistry.azure.net.
Changes:
- Introduces
_resolve_acr_scope(cli_ctx)to compute the ACR audience fromaz config(with a default fallback). - Switches AAD token acquisition to use the resolved audience instead of the hard-coded default.
- Adds unit tests validating default behavior and basic override forms.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/azure-cli/azure/cli/command_modules/acr/_docker_utils.py |
Adds audience resolution helper and wires it into AAD token acquisition flow. |
src/azure-cli/azure/cli/command_modules/acr/tests/latest/test_acr_commands_mock.py |
Adds unit tests for the new audience resolution helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| configured = cli_ctx.config.get('acr', 'audience_resource', fallback=None) | ||
| except Exception: # pylint: disable=broad-except | ||
| configured = None | ||
| if configured: | ||
| return configured if configured.startswith('https://') else "https://{}.azure.net".format(configured) | ||
| return "https://{}.azure.net".format(ACR_AUDIENCE_RESOURCE_NAME) |
There was a problem hiding this comment.
The config parsing here treats any override that doesn’t start with https:// as a short name and appends .azure.net. This will produce an invalid audience for common inputs like containerregistry.azure.net (no scheme) or non-HTTPS schemes like http://... / api://... (it becomes https://http://....azure.net). Consider detecting “already a URL/host” more robustly (e.g., if it contains :// use verbatim; else if it contains a dot treat it as a host and just prefix https://; else treat it as a short name and append .azure.net).
There was a problem hiding this comment.
@copilot apply changes based on this feedback
|
|
||
|
|
There was a problem hiding this comment.
There’s trailing whitespace on the blank line after the return statement. Please remove it to keep the file clean and avoid lint noise.
| def test_short_name_is_expanded(self): | ||
| self.assertEqual( | ||
| _resolve_acr_scope(self._ctx("containerregistry")), | ||
| "https://containerregistry.azure.net", | ||
| ) | ||
|
|
||
| def test_full_url_is_used_verbatim(self): | ||
| self.assertEqual( | ||
| _resolve_acr_scope(self._ctx("https://customregistry.example.com")), | ||
| "https://customregistry.example.com", | ||
| ) | ||
|
|
||
| def test_config_exception_falls_back_to_default(self): | ||
| cli_ctx = mock.MagicMock() | ||
| cli_ctx.config.get.side_effect = RuntimeError("no config") | ||
| self.assertEqual( | ||
| _resolve_acr_scope(cli_ctx), | ||
| "https://{}.azure.net".format(ACR_AUDIENCE_RESOURCE_NAME), | ||
| ) |
There was a problem hiding this comment.
The new tests don’t cover overrides that are likely user inputs but currently mis-parsed, such as containerregistry.azure.net (no scheme) or a non-HTTPS scheme (http://.../api://...). Adding coverage for these cases will help prevent regressions once the parsing logic is made more robust.
|
|
@yanzhudd Can you trigger the validation run for this PR? |
|
@microsoft-github-policy-service agree company="Microsoft" |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
RohanPawarMSFT
requested review from
DanielMicrosoft,
FumingZhang,
ReaNAiveD and
necusjz
as code owners
RohanPawarMSFT
force-pushed
the
rohanpawar/aldo
branch
from
3ff4acb to
1aec167
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Related command
az acr login
Description
Support override ACR audience (https://containerregistry.azure.net/) when acquiring aad token to access container registry instance
Testing Guide
az acr login --debugHistory Notes
[ACR]
az acr login: Make ACR audience customizable in AAD token acquisitionThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.