-
Couldn't load subscription status.
- Fork 712
Introduce AzureKeyVaultSecretReference #8171
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
Conversation
0d4156b to
94ceb65
Compare
- This is a replacement for the magic secret outputs implementation. Resources that support keys push those keys into an explicitly defined per resoure key vault and now into any keyvault that is provided. The IKeyVaultSecretReference is a new primitive reference type that can is understood natively by compute environments like ACA and the azure preparer.
94ceb65 to
b66df54
Compare
…g references in Bicep manifests
…ource and related classes
…ion in BicepProvisioner
…ust related comments
| { | ||
| "type": "azure.bicep.v0", | ||
| "connectionString": "{postgres-data.secretOutputs.connectionString}", | ||
| "connectionString": "{postgres-data-kv.secrets.postgres-data--connectionString}", |
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.
Is there a change queued up on the azd side to process .secrets instead of secret outputs?
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.
cc @vhvb1989
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 looking good - a few minor questions/comments.
I think it's worth going through all the playgrounds and regenerating the manifest publisher output, so we have a new baseline for comparison. The outputs of that command all make sense.
…nal access to secrets and secret client
…ey vault references
…names and improve authentication handling
…nt prefix across Azure resources
…ix across Azure resources
| SetKnownParameterValue(r, AzureBicepResource.KnownParameters.PrincipalId, _ => environment.ContainerRegistryManagedIdentityId); | ||
| SetKnownParameterValue(r, AzureBicepResource.KnownParameters.PrincipalType, _ => "ServicePrincipal"); | ||
| SetKnownParameterValue(r, AzureBicepResource.KnownParameters.PrincipalName, _ => environment.PrincipalName); |
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.
It feels wrong that we still have PrincipalName on the environment. Shouldn't it be ContainerRegistryManagedIdentityPrincipalName?
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 almost half wonder if we should no longer support these "KnownParameters" for PrincipalId, Type, and Name in ACA. Our Azure resources will never use them (or at least, if they do it is a mistake that needs to be fixed).
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.
Separate change. Not in this PR, it's too big already 😄 . I agree.
src/Aspire.Hosting.Azure.KeyVault/AzureKeyVaultSecretReference.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Azure/Provisioning/Provisioners/BicepProvisioner.cs
Outdated
Show resolved
Hide resolved
Refactor Azure Key Vault integration to use a secret resolver function for runtime secret resolution

Description
Fixes #8134
Checklist
<remarks />and<code />elements on your triple slash comments?breaking-changetemplate): [Breaking change]: WithAccessKeyAuthenticaton and WithPasswordAuthentication create a keyvault resource in the app model docs-aspire#2889doc-ideatemplate):