-
Notifications
You must be signed in to change notification settings - Fork 735
Fix Azure Container App Environment to respect AsExisting and WithComputeEnvironment #12978
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.
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
base: main
Are you sure you want to change the base?
Conversation
Adds logic to filter resources based on their compute environment annotation in AzureContainerAppsInfrastructure. Updates tests to verify correct binding of containers to existing environments and multiple environments using WithComputeEnvironment.
Adds logic to detect and handle existing Azure Container App environments by referencing them and skipping child resource creation. Also makes ComputeEnvironmentAnnotation public and adds documentation comments.
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12978Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12978" |
|
@dotnet-policy-service agree |
There was a problem hiding this 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 PR fixes two critical bugs in Azure Container App deployments that prevented proper use of existing Container App Environments. The first bug caused infrastructure generation to create new resources even when AsExisting() was specified. The second bug caused resources to be assigned to all environments instead of only their specified environment when using WithComputeEnvironment().
Key changes:
- Added early-return logic to detect existing resources and skip creation of child infrastructure
- Added environment binding checks to ensure resources are only assigned to their correct environment
- Made
ComputeEnvironmentAnnotationpublic to enable cross-assembly access - Added comprehensive test coverage for both single and multiple existing environment scenarios
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/Aspire.Hosting.Azure.Tests/AzureContainerAppsTests.cs | Added two new tests validating that existing environments bind correctly with AsExisting() and WithComputeEnvironment() for single and multiple environment scenarios |
| src/Aspire.Hosting/ApplicationModel/ComputeEnvironmentAnnotation.cs | Changed visibility from internal to public and added XML documentation to enable cross-assembly usage from AppContainers package |
| src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppsInfrastructure.cs | Added logic to check ComputeEnvironmentAnnotation and only assign resources to their matching environment, preventing resources from being deployed to wrong environments |
| src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppExtensions.cs | Added early-return when ExistingAzureResourceAnnotation is detected to reference existing infrastructure instead of creating new resources |
Co-authored-by: Copilot <[email protected]>
captainsafia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start but we'll probably want to consider a different approach. This issue impacts the App Service environment as well so we'll need to make sure that works here.
Also, we'll want to test this to validate the behavior when we push new images to an existing registry and see whether everything works well with the managed identity.
Since the container app environment encapsulates multiple resources, we should figure out a way to emit the "existing" reference for each of the underlying resources. That might require us to split how we model this under the hood.
Summary
Fixes two critical bugs preventing Azure Container App deployments from using existing Container App Environments, causing permission failures when the deployment attempted to create new infrastructure instead of referencing existing resources.
Problems Fixed
Issue #12977
AsExistingAnnotationWhen a Container App Environment was marked with
.AsExisting(), the infrastructure callback still created a brand new environment with all child resources (Container Registry, Log Analytics Workspace, User Assigned Identity, Dashboard, etc.) instead of referencing the existing environment. This caused deployment failures due to ACR access permission issues.Location:
src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppExtensions.cs:60-324WithComputeEnvironmentBindingWhen multiple Container App Environments existed, resources were assigned to ALL environments instead of only their specified environment via
WithComputeEnvironment(). The last environment processed would win, causing resources to be deployed to the wrong environment.Location:
src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppsInfrastructure.cs:39-52Changes Made
AzureContainerAppExtensions.cs- Added early-return logic in the infrastructure callback to detectExistingAzureResourceAnnotationand callAddAsExistingResource()instead of creating new infrastructureAzureContainerAppsInfrastructure.cs- Added logic to checkComputeEnvironmentAnnotationand only assign resources to their matching environment- Made the class public (was internal) with XML documentation to allow access from theAppContainers` packageAzureContainerAppsTests.cs- Added comprehensive tests:AsExistingEnvironmentWithWithComputeEnvironmentBindsCorrectly- Validates single existing environment scenarioMultipleEnvironmentsWithAsExistingAndWithComputeEnvironmentBindsCorrectly- Validates multiple existing environments with correct bindingTest Plan
Breaking Changes
None - this is a bug fix that makes existing functionality work as documented.
Related Issues
Fixes scenarios where:
.AsExisting(environmentName, sharedResourceGroupName)creates new infrastructure.WithComputeEnvironment(env)doesn't bind resources to the correct environmentNote: The
ComputeEnvironmentAnnotationvisibility change from internal to public is intentional and necessary for the fix to work across assembly boundaries.Checklist
<remarks />and<code />elements on your triple slash comments?doc-ideatemplatebreaking-changetemplatediagnostictemplate