Fix ACR pull role assignments for existing registries - #17992
Draft
David Fowler (davidfowl) wants to merge 9 commits into
Draft
Fix ACR pull role assignments for existing registries#17992David Fowler (davidfowl) wants to merge 9 commits into
David Fowler (davidfowl) wants to merge 9 commits into
Conversation
Move ACA and App Service environment ACR pull grants into generated AzureRoleAssignmentResource modules so existing registries in other resource groups are scoped correctly. Generalize AzureResourcePreparer so aggregate resources can declare role assignments for internal Azure resources, and expand regression coverage across ACA, App Service, AKS/Foundry, and generated artifacts. 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 -- 17992Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 17992" |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This reverts commit 17429bb.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
❓ CLI E2E Tests unknown — 113 passed, 0 failed, 2 unknown (commit View all recordings
📹 Recordings uploaded automatically from CI run #27107572727 |
Member
|
Make sure we have an E2E deployment test covering this scenario. We already have some test cases in the K8S tests which pre-provision ACR, we should do a similar thing for this change to make sure we catch changes which break it in the future. |
The ACR pull managed identity is now a real {env}-acr-pull-identity model
resource with a separate {env}-roles-{env}-acr role-assignment module
(instead of an inline env_mi identity + inline AcrPull role). That caused
four categories of test failure:
- Race: azure-prepare-resources mutates the model (adds role-assignment
resources/annotations) while validate-compute-environments enumerates it.
Both were only ordered against BeforeStart, so they could run concurrently
("Collection was modified"). Make prepare depend on validate so validation
completes (and fails fast) before any mutation.
- Test fake: the separate role module reads the identity's principalId output
at provision time, but the test provisioner returned empty outputs ("No
output for principalId"). Synthesize id/clientId/principalId/principalName/
name for {env}-acr-pull-identity deployments to mirror real Azure UAIs.
- Counts: the env now adds a real AzureUserAssignedIdentityResource plus a
role module, so AzureUserAssignedIdentityTests collection/count assertions
were updated to the new topology.
- Snapshots: regenerated 17 Deployer/Environment/Sql .verified snapshots to
reflect the separate identity + role modules.
All 9 affected classes pass locally (254/254).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #11256 where ACR pull role assignments failed with Bicep error BCP139 when the target Azure Container Registry existed in a different resource group. The root cause was that the generated Bicep emitted role assignments inline in the environment module, but Bicep requires extension resources to be deployed in a separate module when they target a different scope.
Changes:
- Moves the managed identity creation and AcrPull role assignment out of the environment Bicep module into separate
AzureUserAssignedIdentityResourceandAzureRoleAssignmentResourcemodules, allowing cross-resource-group existing registries to work with Bicep's scope rules. - Adds a
ContainerRegistryRoleAssignmentAnnotationandGeneratedContainerRegistryAnnotationto track auto-generated ACR infrastructure, and updatesWithAcrPullIdentityto cleanly remove generated resources when a BYO identity replaces them. - Adds a pipeline ordering edge (
DependsOnSteps = [ValidateComputeEnvironments]) toAzureResourcePreparer's step to prevent concurrent model mutation during validation.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Hosting.Azure/RoleAssignmentAnnotation.cs |
Converts from primary constructor to explicit constructor with null checks |
src/Aspire.Hosting.Azure/ContainerRegistryRoleAssignmentAnnotation.cs |
New annotation declaring ACR role needs for compute environments |
src/Shared/GeneratedContainerRegistryAnnotation.cs |
New shared annotation to track auto-generated container registries |
src/Aspire.Hosting.Azure/AzureEnvironmentResource.cs |
Adds DependsOnSteps on ValidateComputeEnvironments to prevent concurrent mutation |
src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppExtensions.cs |
Major refactoring: removes inline identity/role creation, adds identity & role as separate resources |
src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppEnvironmentResource.cs |
Replaces DefaultContainerRegistry with annotation-based registry tracking and adds deployment prerequisite helpers |
src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppEnvironmentAcrPullIdentityAnnotation.cs |
Adds assignAcrPullRole flag to distinguish generated vs BYO identities |
src/Aspire.Hosting.Azure.AppContainers/Aspire.Hosting.Azure.AppContainers.csproj |
Adds shared file include |
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentExtensions.cs |
Parallel refactoring to AppContainers |
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentResource.cs |
Parallel refactoring to AppContainers |
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentAcrPullIdentityAnnotation.cs |
Parallel to AppContainers annotation |
src/Aspire.Hosting.Azure.AppService/Aspire.Hosting.Azure.AppService.csproj |
Adds shared file include |
src/Aspire.Hosting.Azure.Kubernetes/AzureKubernetesEnvironmentResource.cs |
Replaces DefaultContainerRegistry with annotation-based lookup |
src/Aspire.Hosting.Azure.Kubernetes/AzureKubernetesEnvironmentExtensions.cs |
Uses GeneratedContainerRegistryAnnotation for registry replacement |
src/Aspire.Hosting.Azure.Kubernetes/Aspire.Hosting.Azure.Kubernetes.csproj |
Adds shared file include |
tests/Aspire.Hosting.Azure.Tests/AzureResourcePreparerTests.cs |
Adds 6 new tests for aggregate owner role assignments |
tests/Aspire.Hosting.Azure.Tests/AzureUserAssignedIdentityTests.cs |
Updates assertions for new auto-generated identity/role resources |
tests/Aspire.Hosting.Azure.Tests/AzureContainerAppEnvironmentExtensionsTests.cs |
Updates test to verify role assignment scope for existing registries |
tests/Aspire.Hosting.Azure.Tests/AzureAppServiceTests.cs |
Updates test to verify role assignment scope for existing registries |
tests/Aspire.Hosting.Azure.Tests/ProvisioningTestHelpers.cs |
Synthesizes identity outputs for test deployments |
tests/Aspire.Hosting.Azure.Kubernetes.Tests/AzureKubernetesEnvironmentExtensionsTests.cs |
Updates tests for annotation-based registry tracking |
tests/.../*.verified.bicep (many) |
Snapshot updates reflecting the refactored Bicep structure |
tests/.../*.verified.json (many) |
Snapshot updates reflecting new identity/role resources in manifest |
tests/.../*.verified.txt (several) |
Pipeline dependency graph snapshots reflecting new provisioning steps |
Copilot's findings
- Files reviewed: 85/85 changed files
- Comments generated: 2
Comment on lines
+234
to
+267
| private IReadOnlySet<AzureBicepResource> GetEnvironmentDeploymentPrerequisites() | ||
| { | ||
| if (!this.TryGetAnnotationsOfType<DeploymentPrerequisitesAnnotation>(out var prerequisiteAnnotations)) | ||
| { | ||
| return new HashSet<AzureBicepResource>(); | ||
| } | ||
|
|
||
| // The preparer writes environment-owned prerequisites after it materializes generated | ||
| // role-assignment modules. Collapse all annotations here so future environment-owned | ||
| // infrastructure can participate in the same deployment ordering contract. | ||
| return prerequisiteAnnotations.SelectMany(a => a.Resources).ToHashSet(); | ||
| } | ||
|
|
||
| private static void AddDeploymentPrerequisites(IResource resource, IReadOnlySet<AzureBicepResource> prerequisites) | ||
| { | ||
| if (prerequisites.Count == 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var newPrerequisites = prerequisites.ToHashSet(); | ||
| if (resource.TryGetAnnotationsOfType<DeploymentPrerequisitesAnnotation>(out var existingAnnotations)) | ||
| { | ||
| // A resource can already have prerequisites from direct Azure references. Only add the | ||
| // environment-level resources it does not already wait on to avoid duplicate References | ||
| // when deployment targets are generated. | ||
| newPrerequisites.ExceptWith(existingAnnotations.SelectMany(a => a.Resources)); | ||
| } | ||
|
|
||
| if (newPrerequisites.Count > 0) | ||
| { | ||
| resource.Annotations.Add(new DeploymentPrerequisitesAnnotation(newPrerequisites)); | ||
| } | ||
| } |
Comment on lines
+717
to
+790
| private static void RemoveGeneratedAcrPullIdentity(IResourceBuilder<AzureContainerAppEnvironmentResource> builder) | ||
| { | ||
| if (!builder.Resource.TryGetLastAnnotation<AzureContainerAppEnvironmentAcrPullIdentityAnnotation>(out var identityAnnotation) || | ||
| !identityAnnotation.AssignAcrPullRole) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // This environment owns the generated ACR-pull identity only while Aspire is also responsible | ||
| // for granting AcrPull. A BYO identity means the caller owns both the identity and permission, | ||
| // so remove only Aspire's generated AppIdentityAnnotation(s) and model resource. | ||
| foreach (var appIdentityAnnotation in builder.Resource.Annotations.OfType<AppIdentityAnnotation>() | ||
| .Where(a => a.IdentityResource == identityAnnotation.Identity) | ||
| .ToArray()) | ||
| { | ||
| builder.Resource.Annotations.Remove(appIdentityAnnotation); | ||
| } | ||
|
|
||
| // WithAcrPullIdentity means the caller owns the identity and its AcrPull permission. Remove the | ||
| // hidden identity from the model; RemoveGeneratedAcrPullRoleAssignment removes the matching | ||
| // generated RBAC intent. | ||
| builder.ApplicationBuilder.Resources.Remove(identityAnnotation.Identity); | ||
| } | ||
|
|
||
| private static void RemoveGeneratedAcrPullRoleAssignment(AzureContainerAppEnvironmentResource environment) | ||
| { | ||
| foreach (var annotation in environment.Annotations.OfType<ContainerRegistryRoleAssignmentAnnotation>().ToArray()) | ||
| { | ||
| environment.Annotations.Remove(annotation); | ||
| } | ||
| } | ||
|
|
||
| private static AzureProvisioningResource GetContainerRegistryForAcrPullRole(AzureContainerAppEnvironmentResource environment) | ||
| { | ||
| if (environment.TryGetLastAnnotation<ContainerRegistryReferenceAnnotation>(out var registryReferenceAnnotation)) | ||
| { | ||
| if (registryReferenceAnnotation.Registry is AzureProvisioningResource explicitRegistry) | ||
| { | ||
| return explicitRegistry; | ||
| } | ||
|
|
||
| throw new InvalidOperationException($"The container registry associated with environment '{environment.Name}' is not an Azure Container Registry."); | ||
| } | ||
|
|
||
| throw new InvalidOperationException($"No container registry associated with environment '{environment.Name}'. This should have been added automatically."); | ||
| } | ||
|
|
||
| private static AzureUserAssignedIdentityResource CreateDefaultAcrPullIdentity(IDistributedApplicationBuilder builder, string environmentName) | ||
| { | ||
| var identity = new AzureUserAssignedIdentityResource(GetUniqueAcrPullIdentityName(builder, environmentName)); | ||
| // The identity is a first-class resource so the preparer can order it before the environment | ||
| // module and pass its id into the environment Bicep as an input parameter. | ||
| builder.AddResource(identity); | ||
|
|
||
| return identity; | ||
| } | ||
|
|
||
| private static string GetUniqueAcrPullIdentityName(IDistributedApplicationBuilder builder, string environmentName) | ||
| { | ||
| var baseName = $"{environmentName}-acr-pull-identity"; | ||
| if (!builder.Resources.TryGetByName(baseName, out _)) | ||
| { | ||
| return baseName; | ||
| } | ||
|
|
||
| for (var i = 2; ; i++) | ||
| { | ||
| var candidate = $"{baseName}-{i}"; | ||
| if (!builder.Resources.TryGetByName(candidate, out _)) | ||
| { | ||
| return candidate; | ||
| } | ||
| } | ||
| } |
14 tasks
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
Existing ACA and App Service environment Bicep emitted the AcrPull role assignment inline in the environment module. That fails when the selected ACR is an existing registry in another resource group because Bicep extension resources must be deployed at the target scope.
This moves those grants into generated
AzureRoleAssignmentResourcemodules and teachesAzureResourcePreparerto materialize role-assignment annotations from aggregate owner resources. Environments now own the ACR-pull identity and deployment prerequisites, whileWithAcrPullIdentitykeeps the BYO identity path from generating extra identity or role resources.User-facing usage
Existing registry scenarios continue to use the existing APIs:
The generated Bicep now emits the AcrPull grant as a separate role-assignment module scoped with the registry, so cross-resource-group existing registries no longer fail Bicep validation.
Security considerations
This change preserves the existing AcrPull permission shape but moves where the grant is emitted. It also keeps BYO identities as an explicit opt-out: callers using
WithAcrPullIdentityremain responsible for granting AcrPull themselves. Tests cover that aggregate and implied role assignments do not broaden defaults or grant non-referencing resources.Validation
dotnet test --project tests/Aspire.Hosting.Azure.Tests/Aspire.Hosting.Azure.Tests.csproj --no-launch-profile -- --filter-class "*.AzureResourcePreparerTests" --filter-class "*.AzureContainerAppEnvironmentExtensionsTests" --filter-class "*.AzureAppServiceTests" --filter-class "*.AzureContainerAppsTests" --filter-not-trait "quarantined=true" --filter-not-trait "outerloop=true"- 191 passeddotnet test --project tests/Aspire.Hosting.Azure.Kubernetes.Tests/Aspire.Hosting.Azure.Kubernetes.Tests.csproj --no-launch-profile -- --filter-class "*.AzureKubernetesFoundryReferenceTests" --filter-not-trait "quarantined=true" --filter-not-trait "outerloop=true"- 1 passeddotnet test --project tests/Aspire.Hosting.Foundry.Tests/Aspire.Hosting.Foundry.Tests.csproj --no-launch-profile -- --filter-class "*.HostedAgentExtensionTests" --filter-not-trait "quarantined=true" --filter-not-trait "outerloop=true"- 27 passedgit diff --check.received.*snapshot files remainFixes #11256
Checklist
<remarks />and<code />elements on your triple slash comments?