-
Notifications
You must be signed in to change notification settings - Fork 668
Hook up Azure Deployer to BicepProvisioner #10845
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
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 connects the Azure Deployer to the BicepProvisioner, enabling deployment-time provisioning of Azure resources. The changes focus on extending the existing provisioning infrastructure to handle both run-time and deploy-time scenarios.
Key changes:
- Enhanced ProvisioningContext to include execution context and output path information
- Modified BicepProvisioner to handle both subscription-scoped and resource group-scoped deployments
- Connected AzureEnvironmentResource to use BicepProvisioner during deployment phase
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/Aspire.Hosting.Azure/Provisioning/ProvisioningContext.cs | Added execution context and output path to provisioning context |
src/Aspire.Hosting.Azure/Provisioning/Provisioners/IBicepProvisioner.cs | New interface for Bicep provisioning operations |
src/Aspire.Hosting.Azure/Provisioning/Provisioners/BicepProvisioner.cs | Enhanced to support both run-time and deploy-time provisioning modes |
src/Aspire.Hosting.Azure/AzureEnvironmentResource.cs | Changed inheritance to AzureBicepResource and integrated deployment logic |
src/Aspire.Hosting.Azure/AzureDeployingContext.cs | Added comprehensive deployment orchestration with parameter mapping |
src/Aspire.Hosting.Azure/AzureBicepResource.cs | Fixed template file path handling for custom output directories |
tests/Aspire.Hosting.Azure.Tests/* | Updated test infrastructure to support new interfaces and parameters |
playground/deployers/Deployers.AppHost/* | New playground project demonstrating Azure deployment capabilities |
Comments suppressed due to low confidence (1)
tests/Aspire.Hosting.Azure.Tests/ProvisioningTestHelpers.cs:187
- The TestArmDeploymentCollection implementation is referenced but not defined in this file. This could cause test failures if the implementation is missing or incomplete.
public IArmDeploymentCollection GetArmDeployments()
if (context.ExecutionContext.IsRunMode) | ||
{ | ||
template.Dispose(); | ||
} |
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.
The disposal of the template should happen regardless of execution mode. Moving the template.Dispose() call outside the if block would ensure proper resource cleanup in all scenarios.
if (context.ExecutionContext.IsRunMode) | |
{ | |
template.Dispose(); | |
} | |
template.Dispose(); |
Copilot uses AI. Check for mistakes.
// TODO: Prompt here. | ||
await deployingStep.FailAsync("Deployment contains unresolvable parameters.", cancellationToken).ConfigureAwait(false); |
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.
The TODO comment indicates incomplete functionality for handling unresolvable parameters. This could leave the deployment in an inconsistent state without proper parameter resolution.
// TODO: Prompt here. | |
await deployingStep.FailAsync("Deployment contains unresolvable parameters.", cancellationToken).ConfigureAwait(false); | |
throw new InvalidOperationException( | |
$"Deployment contains unresolvable parameter: '{provisioningParameter.BicepIdentifier}'. " + | |
"Please ensure all required parameters are provided or handled before deployment."); |
Copilot uses AI. Check for mistakes.
@@ -217,14 +220,14 @@ public async Task<ProvisioningContext> CreateProvisioningContextAsync(JsonObject | |||
// Create a unique resource group name and save it in user secrets |
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.
We never create a resource group via the ProvisioningContextProvider when in publish mode and always rely on it to be provisoned via the declaration in main.bicep.
@@ -259,6 +262,7 @@ public async Task<ProvisioningContext> CreateProvisioningContextAsync(JsonObject | |||
} | |||
|
|||
var principal = await userPrincipalProvider.GetUserPrincipalAsync(cancellationToken).ConfigureAwait(false); | |||
var outputPath = _publishingOptions.OutputPath is { } outputPathValue ? Path.GetFullPath(outputPathValue) : null; |
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.
We past the output path to the ProvisioningContext during deployment so that we can reuse the bicep templates that were generated by the publish step during deployment.
}), | ||
cancellationToken).ConfigureAwait(false); | ||
}) | ||
{ Location = context.Location }, |
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.
ARM requires a location when doing a subscription-scoped deployment so we pass it to ArmDeploymentContent
here. There's no consequence to setting the location for RG-based deployments so setting this up conditionally.
var deployments = resourceGroup.GetArmDeployments(); | ||
// Deploy-time provisioning should target the subscription scope while run-time | ||
// provisioning should target the resource group scope. | ||
var deployments = context.ExecutionContext.IsPublishMode |
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 one of the places where we need to differentiate between run-mode and publish-mode in the provisioner. Deployments during publish-mode are subscription scoped and need to be resolved differently.
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 shouldn't be based on the mode. This should be part of the Scope property on the AzureBicepResource. We'll need to do this #7514 and I suspect it'll look similar to this.
@@ -198,48 +205,54 @@ await notificationService.PublishUpdateAsync(resource, state => | |||
|
|||
if (deployment.Data.Properties.ProvisioningState == ResourcesProvisioningState.Succeeded) | |||
{ | |||
template.Dispose(); | |||
if (context.ExecutionContext.IsRunMode) |
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.
We want to keep the template files used to do the deployment around during deploy mode (for now: see #10642).
/// <summary> | ||
/// Provides functionality for provisioning Azure Bicep resources. | ||
/// </summary> | ||
internal interface IBicepProvisioner |
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 interface exists to support testability. We need an IBicepProvisioner implementation that no-ops in tests.
/// <param name="context">The provisioning context containing Azure subscription, resource group, and other deployment details.</param> | ||
/// <param name="cancellationToken">A cancellation token to cancel the operation.</param> | ||
/// <returns>A task that represents the asynchronous operation.</returns> | ||
Task GetOrCreateResourceAsync(AzureBicepResource resource, ProvisioningContext context, CancellationToken cancellationToken); |
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 method currently returns no value but I can see it evolving to return a type that we can use to poll the underlying ArmOperation for their state so we can get granular state updates on each resource deployed from main.bicep in the future.
@@ -17,7 +19,7 @@ namespace Aspire.Hosting.Azure; | |||
/// Emits a <c>main.bicep</c> that aggregates all provisionable resources. | |||
/// </summary> | |||
[Experimental("ASPIREAZURE001", UrlFormat = "https://aka.ms/dotnet/aspire/diagnostics#{0}")] | |||
public sealed class AzureEnvironmentResource : Resource | |||
public sealed class AzureEnvironmentResource : AzureBicepResource |
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.
AzureEnvironmentResource
needs to implement AzureBicepResource
so we can set parameters and store outputs from the ARM deployment, like the ACR instance associated with a container apps environment for future image pushes.
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.
So this represents main.bicep now? Should this be an AzureProvisoningResource instead?
@@ -0,0 +1,22 @@ | |||
var builder = DistributedApplication.CreateBuilder(args); | |||
|
|||
builder.AddAzureContainerAppEnvironment("env"); |
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.
Its kind of strange that we add AddAzureContainerAppEnvironment(...)
when we aren't using anything other than Azure resources. I realize this is probably just the case for this playground but I wonder if we will see cases of people using apphosts to deploy things without any compute (containers/projects) because the resources they deploy have some built-in compute functionality that they are leveraging.
In which case - does that API name make sense?
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.
I could see a world where you only only use Aspire deploy to deploy Azure resources and not any actual compute resources. For example, I saw a scenario recently on a static front-end deployed on an Azure Storage instance with Azure Front Door for routing.
We've set up the APIs for AddAcaEnv such that you don't really "discover" you need them until you try to publish or deploy a project with compute resources (or role assignments).
We've really overloaded the term "environment" in our APIs and I'm not sure how we dig ourselves out of it. Although it won't really be exposed in public API to the user until the add compute.
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.
Today we need a compute environment or we'll generate invalid bicep. If you provision an azure resource that depends on managed identity (which we do by default) but don't provide a compute environment, we'll generate invalid bicep (with a missing user assigned managed identity).
So I jumped straight into doing aspire deploy ... I didn't run it first - so the parameters were not set. |
So it looked to me like it was building the storage roles and storage templates simultaneously in the dashboard. That is probably OK - but I'm wondering if it tries to start deploying them simultaneously as well which would be an issue. |
await provisioningContextProvider.CreateProvisioningContextAsync(userSecrets, cancellationToken).ConfigureAwait(false); | ||
var provisioningContext = await provisioningContextProvider.CreateProvisioningContextAsync(userSecrets, cancellationToken).ConfigureAwait(false); | ||
|
||
if (resource.PublishingContext is null) |
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.
How does this happen?
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.
Currently -- never. I left this as an explicit check for when we do #10620.
@mitchdenny b8beec3 includes fixes for the two issues you ran into. The exception you got during Adding some more conditional logic based on modes to the context provider was annoying but I realized that resource group creation is idempotent since we use The other issue was due to the fact that setting the |
Contributes towards #10448.
The next step here is to revise the APIs for IBicepProvisioner so that it is possible to query the ARM deployment to get the status on each subresource that is deployed from
main.bicep
. The currentBicepProvisioner
implementation assumes that only one resource is deployed and that status is set through theResourceNotificationService
.