Skip to content

Add hosting integration for Azure Container Registry #9059

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
May 1, 2025
Merged

Add hosting integration for Azure Container Registry #9059

merged 4 commits into from
May 1, 2025

Conversation

@captainsafia
Copy link
Member

@captainsafia captainsafia commented May 1, 2025 •
edited
Loading

Description

This PR introduces first-class Azure Container Registry (ACR) support to Aspire. You can now provision a new registry or wire up an existing one and have it automatically injected into every compute resource by linking it to a compute environment via the WithAzureContainerRegistry API.

var acr = builder.AddAzureContainerRegistry("my-acr");

builder.AddAzureContainerAppsEnvironment("env")
       .WithAzureContainerRegistry(acr);

Part of #9005

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

@captainsafia captainsafia requested review from davidfowl and eerhardt and removed request for eerhardt, mitchdenny and radical May 1, 2025 19:18
@github-actions github-actions bot added the area-integrations Issues pertaining to Aspire Integrations packages label May 1, 2025
davidfowl
davidfowl reviewed May 1, 2025
View reviewed changes
davidfowl
davidfowl reviewed May 1, 2025
View reviewed changes
@captainsafia captainsafia merged commit d373c8a into main May 1, 2025
174 checks passed
@captainsafia captainsafia deleted the cs/acr-resource branch May 1, 2025 22:15
radical pushed a commit to radical/aspire that referenced this pull request May 2, 2025
@captainsafia @radical
Add hosting integration for Azure Container Registry (dotnet#9059)
b7af705 
* Add hosting integration for Azure Container Registry

* Revert IComputeResource changes

* Add support for WithRoleAssignments

* Only add ACR resource during publish mode
eerhardt
eerhardt reviewed May 2, 2025
View reviewed changes
src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppsInfrastructure.cs
ContainerRegistryInfo = caes.FirstOrDefault(),
ComputeEnvironment = environment as IComputeEnvironmentResource // will be null if azd
ContainerRegistry = caes.FirstOrDefault(),
ComputeEnvironment = environment as IComputeEnvironmentResource
Copy link
Member

Choose a reason for hiding this comment

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

Why the removal of the comment?

src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppsInfrastructure.cs
// Capture information about the container registry used by the
// container app environment in the deployment target information
// associated with each compute resource that needs an image
#pragma warning disable ASPIRECOMPUTE001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
Copy link
Member

Choose a reason for hiding this comment

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

This can go away now that we are suppressing on the whole file.

src/Aspire.Hosting.Azure.ContainerRegistry/Aspire.Hosting.Azure.ContainerRegistry.csproj
<PropertyGroup>
<TargetFramework>$(DefaultTargetFramework)</TargetFramework>
<IsPackable>true</IsPackable>
<PackageTags>aspire integration hosting azure containerregistry</PackageTags>
Copy link
Member

Choose a reason for hiding this comment

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

Should this be

container registry or containerregistry?

https://www.nuget.org/packages/Azure.Containers.ContainerRegistry has them split apart.

src/Aspire.Hosting.Azure.ContainerRegistry/ContainerRegistryReferenceAnnotation.cs
/// Initializes a new instance of the <see cref="ContainerRegistryReferenceAnnotation"/> class.
/// </remarks>
/// <param name="registry">The container registry resource.</param>
public class ContainerRegistryReferenceAnnotation(IContainerRegistry registry) : IResourceAnnotation
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in the same place IContainerRegistry is defined? (i.e. Aspire.Hosting)

src/Aspire.Hosting.Azure.ContainerRegistry/AzureContainerRegistryResource.cs
/// <summary>
/// The endpoint of the Azure Container Registry.
/// </summary>
public BicepOutputReference RegistryEndpoint => new("loginServer", this);
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want the bicep output to be named loginServer?

src/Aspire.Hosting.Azure.ContainerRegistry/AzureContainerRegistryResource.cs
/// Represents an Azure Container Registry resource.
/// </summary>
public class AzureContainerRegistryResource(string name, Action<AzureResourceInfrastructure> configureInfrastructure)
: AzureProvisioningResource(name, configureInfrastructure), IContainerRegistry
Copy link
Member

Choose a reason for hiding this comment

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

Do we want IResourceWithConnectionString?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to avoid this until we had the chance to figure out what it means to WithReference a container registry from a non-environment resource. Does IResourceWithConnectionString influence anything else outside of references?

Copy link
Member

Choose a reason for hiding this comment

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

Agree we should avoid it for now.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should remove the WithRoleAssignments API then? I don't think there is a scenario for it if we aren't allowing references to the container registry.

src/Aspire.Hosting.Azure.ContainerRegistry/AzureContainerRegistryResource.cs
/// <summary>
/// The name of the Azure Container Registry.
/// </summary>
public BicepOutputReference RegistryName => new("name", this);
Copy link
Member

Choose a reason for hiding this comment

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

Do these need to be prefixed with Registry?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought so otherwise it would conflict with the Name property on Resource.

Copy link
Member

Choose a reason for hiding this comment

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

NameOutputReference is what we call it for keyvault.

This was referenced May 5, 2025
Closed
Merged
@davidfowl davidfowl mentioned this pull request May 13, 2025
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Jun 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@davidfowl davidfowl davidfowl approved these changes

@eerhardt eerhardt eerhardt left review comments

Assignees

No one assigned

Labels

area-integrations Issues pertaining to Aspire Integrations packages

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@captainsafia @davidfowl @eerhardt