Fix BCP139 cross-resource-group ACR AcrPull role in compute environments#18118
Draft
davidfowl wants to merge 2 commits into
Draft
Fix BCP139 cross-resource-group ACR AcrPull role in compute environments#18118davidfowl wants to merge 2 commits into
davidfowl wants to merge 2 commits into
Conversation
In publish mode, AddAzureContainerAppEnvironment and AddAzureAppServiceEnvironment
inlined an AcrPull role assignment into the environment's bicep file scoped to the
container registry. When the registry is an existing resource in a different resource
group (via PublishAsExisting), Bicep rejected the cross-scope role assignment with
BCP139 ("A resource's scope must match the scope of the Bicep file").
The fix reuses the existing model-resource + role-module machinery that the
user-supplied identity path (WithAcrPullIdentity) already uses, so the unmodified
AzureResourcePreparer emits a separate {identity}-roles-{registry} module and sets
its scope to the registry's resource group for an existing cross-RG registry.
- Add an internal AssignAcrPullRole flag to the existing per-package
AcrPullIdentityAnnotation types to mark Aspire-generated default identities.
- In publish mode, create a default AzureUserAssignedIdentityResource ({env}-mi)
and attach it via the existing annotation so the env callback reads the identity
id as a parameter and skips the inline identity + inline AcrPull role. Run mode
is unchanged.
- Add a per-package pre-prepare pipeline step that resolves the environment's final
configured registry (honoring a later WithAzureContainerRegistry swap) and adds a
RoleAssignmentAnnotation(registry, AcrPull) to the generated identity.
- WithAcrPullIdentity removes the generated identity when the user supplies their own.
No public API changes, no AzureResourcePreparer changes, no AKS changes. Adds
regression tests for both ACA and App Service asserting the inline cross-scope role
is gone and a role module scoped to the existing registry's resource group is emitted.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 18118Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18118" |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes #11256 — a BCP139 Bicep deployment error triggered when an Azure compute environment (ACA or App Service) is configured to pull from an existing Azure Container Registry in a different resource group. The inline AcrPull role assignment in env.bicep failed because Bicep requires modules for cross-scope deployments. The fix extracts the managed identity and its role assignment into separately deployable modules, reusing the existing AzureResourcePreparer cross-resource-group scoping machinery.
Changes:
- The ACR-pull identity (
env-mi) is now materialized as a first-classAzureUserAssignedIdentityResourcein publish mode, with the AcrPull role assignment emitted as a scoped module (env-mi-roles-env-acr) instead of being inlined inenv.bicep. - A new pipeline step (per compute package) runs before
azure-prepare-resourcesto resolve the final registry and attach theRoleAssignmentAnnotationto the generated identity, ensuring correct last-writer-wins semantics forWithAzureContainerRegistry. - Two new regression tests (ACA + App Service) verify cross-resource-group ACR scenarios produce correctly scoped modules.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppExtensions.cs |
Adds pipeline step, helper methods for identity creation, role assignment, and registry resolution (ACA path) |
src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppEnvironmentAcrPullIdentityAnnotation.cs |
Adds assignAcrPullRole constructor parameter and AssignAcrPullRole property |
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentExtensions.cs |
Same pipeline step and helper methods (App Service path, near-identical to ACA) |
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentAcrPullIdentityAnnotation.cs |
Adds assignAcrPullRole constructor parameter and AssignAcrPullRole property |
tests/Aspire.Hosting.Azure.Tests/AzureContainerAppEnvironmentExtensionsTests.cs |
New regression test for cross-RG ACR with ACA |
tests/Aspire.Hosting.Azure.Tests/AzureAppServiceEnvironmentExtensionsTests.cs |
New regression test for cross-RG ACR with App Service |
tests/Aspire.Hosting.Azure.Tests/AzureUserAssignedIdentityTests.cs |
Updated resource collection assertions to account for new env-mi identity |
tests/Aspire.Hosting.Azure.Tests/AzureDeployerTests.cs |
Fake provisioner updated with env-mi module outputs |
tests/Aspire.Hosting.Azure.Tests/AzureContainerAppsTests.cs |
Updated resource count and collection assertions |
tests/Aspire.Hosting.Azure.Tests/AzureAppServiceTests.cs |
Updated resource count and collection assertions |
~63 .verified.bicep/.verified.json/.verified.txt snapshots |
Reflect identity moved to standalone module, role assignment moved to scoped module |
Copilot's findings
- Files reviewed: 72/72 changed files
- Comments generated: 1
14 tasks
The AcrPull role helpers and pipeline step are intentionally duplicated across the AppContainers and AppService packages (they don't share internals). Add reciprocal keep-in-sync notes so future maintainers know the two copies must evolve together. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
❓ CLI E2E Tests unknown — 115 passed, 0 failed, 2 unknown (commit View all recordings
📹 Recordings uploaded automatically from CI run #27402601210 |
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
Fixes #11256
When a compute environment (Azure Container Apps or Azure App Service) is configured to pull from an existing Azure Container Registry that lives in a different resource group (via
PublishAsExisting("myacr", "my-existing-resource-group")+env.WithAzureContainerRegistry(acr)), the generatedenv.bicepinlined anAcrPullrole assignment scoped directly to that cross-resource-group registry. Bicep rejects a resource whose scope doesn't match the file's scope with BCP139 ("A resource's scope must match the scope of the Bicep file ... use modules to deploy resources to a different scope"), soaspire publishproduced bicep that fails to deploy.Relationship to #17988
This is the companion fix for #17988. Live PR-build testing of #17988's Azure scope APIs found that the shared organization ACR scenario from #7514 still fails end to end for Azure Container Apps unless the
AcrPullrole assignment is moved out of the ACA environment module. #17988 provides the scope model/APIs, and this PR makes the ACA/App Service generated Bicep composition deployable for cross-resource-group existing registries.What changed for users
Publishing an ACA or App Service environment that pulls from a cross-resource-group existing ACR now succeeds: the
AcrPullrole assignment is emitted as a separately-scoped module instead of being inlined inenv.bicep, so it deploys without BCP139. The granted role is unchanged (AcrPullon the same identity and registry) — only where the assignment is declared moves. Run mode and same-resource-group scenarios are unaffected.Approach (minimal)
In publish mode, the ACR-pull identity is materialized as a first-class
AzureUserAssignedIdentityResource({env}-mi) added to the model, and the existing internal...AcrPullIdentityAnnotationis flagged with a new internalAssignAcrPullRole. This flips the environment's infrastructure callback onto its existing "bring-your-own identity" branch (the env module reads the identity id, and for App Service the client id, from a parameter), so the callback no longer emits the inline_miidentity or the inlineAcrPullrole.A per-package pre-prepare pipeline step (
requiredBy: PrepareResourcesStepName) resolves the environment's final registry — honoring any laterWithAzureContainerRegistryswap, last-writer-wins — and adds aRoleAssignmentAnnotation(registry, AcrPull)to the generated identity. The unchangedAzureResourcePreparerthen turns that annotation on the standalone identity into a separately-scoped{identity}-roles-{registry}module, settingScope = resourceGroup(...)when the registry is an existing cross-resource-group resource. That scoped module is the BCP139 fix.WithAcrPullIdentity(bring-your-own identity) removes the generated identity and skips the role assignment, preserving today's behavior where the caller owns the role via.WithRoleAssignments(acr, AcrPull).Constraints honored
api/*.cschanges).AzureResourcePreparer.csuntouched — the fix reuses its existing cross-resource-group role-module machinery.InternalsVisibleTo/.csprojchanges. Reuses the existing publicRoleAssignmentAnnotation.Validation
env.bicepcontains no inlineAcrPullrole assignment and thatmain.bicepemits a{env}-mi-roles-acrmodule scoped toresourceGroup('my-existing-resource-group').AzureDeployerTestsfake provisioner (now suppliesid/principalId/clientIdoutputs for the generated*-miidentity modules)._miidentity + inlineAcrPullrole removed fromenv.bicep; the env module now takes the identity id (and client id, for App Service) as a parameter; the role moves to a scoped module.Aspire.Hosting.Azure.Tests: 1477 passed, 5 skipped. The only failures (15) are pre-existing Docker-emulator functional tests (EventHubs/Storage/SignalR/Cosmos) unrelated to this change.Minimal alternative to #17992 (which refactors
AzureResourcePreparerand adds public API); opening this so both approaches can be compared. Not closing #17992.Checklist
<remarks />and<code />elements on your triple slash comments?