Add support for creating and using user-assigned identities #9130
Conversation
github-actions
bot
added
the
area-integrations
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the AppIdentityResource with a new AzureUserAssignedIdentityResource to improve support for Azure user-assigned managed identities, while also enhancing role assignment logic and adding extension methods and tests.
- Replaces AppIdentityResource with AzureUserAssignedIdentityResource
- Adds new extension methods for simplified identity resource integration
- Enhances role assignment logic and introduces comprehensive tests
Reviewed Changes
Copilot reviewed 5 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Azure.Tests/AzureUserAssignedIdentityTests.cs | New unit tests and snapshot tests verifying identity resources and role assignments. |
| src/Aspire.Hosting.Azure/AzureUserAssignedIdentityResource.cs | Introduces the new identity resource with updated provisioning outputs. |
| src/Aspire.Hosting.Azure/AzureUserAssignedIdentityExtensions.cs | Adds extension methods for integrating Azure user-assigned identities into the application model. |
| src/Aspire.Hosting.Azure/AzureResourcePreparer.cs | Updates role assignment validation logic and resource addition to support the new identity resource. |
| src/Aspire.Hosting.Azure/AppIdentityResource.cs | Removed to replace with the new AzureUserAssignedIdentityResource. |
Files not reviewed (5)
- tests/Aspire.Hosting.Azure.Tests/Snapshots/AzureUserAssignedIdentityTests.AddAzureUserAssignedIdentity_GeneratesExpectedResourcesAndBicep.verified.bicep: Language not supported
- tests/Aspire.Hosting.Azure.Tests/Snapshots/AzureUserAssignedIdentityTests.AddAzureUserAssignedIdentity_PublishAsExisting_Works.verified.bicep: Language not supported
- tests/Aspire.Hosting.Azure.Tests/Snapshots/AzureUserAssignedIdentityTests.AddAzureUserAssignedIdentity_WithRoleAssignments_Works#00.verified.bicep: Language not supported
- tests/Aspire.Hosting.Azure.Tests/Snapshots/AzureUserAssignedIdentityTests.AddAzureUserAssignedIdentity_WithRoleAssignments_Works#01.verified.bicep: Language not supported
- tests/Aspire.Hosting.Azure.Tests/Snapshots/AzureUserAssignedIdentityTests.AddAzureUserAssignedIdentity_WithRoleAssignments_Works#02.verified.bicep: Language not supported
| public override ProvisionableResource AddAsExistingResource(AzureResourceInfrastructure infra) | ||
| { | ||
| var store = UserAssignedIdentity.FromExisting(this.GetBicepIdentifier()); | ||
| store.Name = PrincipalName.AsProvisioningParameter(infra); |
There was a problem hiding this comment.
Does PrincipalName always work? Is that always the same as Name? Oh, we assign "Name" to the "principalName" output.
|
|
||
| await Verifier.Verify(bicep, extension: "bicep") | ||
| .UseHelixAwareDirectory("Snapshots") | ||
| .AutoVerify(); |
There was a problem hiding this comment.
I don't think this is something we should be doing; auto-accepting changes in the tests undermines the value of the tests - we won't be alerted when the snapshot changes, and thus, may inadvertently let a regression slip through.
For local dev there are tools to accept all the pending snapshots, which is useful when the snapshots first created --> https://github.com/VerifyTests/Verify/blob/main/readme.md#snapshot-management (I've been using DiffEngineTray).
There was a problem hiding this comment.
Yes @captainsafia mentioned that she forgot to revert it
There was a problem hiding this comment.
Yes, I noticed a branch in the trunk :)
To add - there's an optional parameter to autoaccept locally, but fail on a CI (https://github.com/VerifyTests/DiffEngine/blob/main/src/DiffEngine/BuildServerDetector.cs), but I don't think we should consider that either - some things we assert may contain random/generated information (like guid, port numbers, etc.). And auto-acceptance may hide those until after the changes are pushed.
There was a problem hiding this comment.
Yes, AutoVerify and I have a long history of getting in trouble in the ASP.NET Core repo. 😅
One thing we were able to do in the manual snapshot strategy we used for RDG is use a boolean that would regenerate the files but fail the test (see https://github.com/dotnet/aspnetcore/blob/754e56eea0d88f36653af0da1f9c00186d93cec8/src/Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateCreationTestBase.cs#L363). This makes it easy to bootstrap or iterate on new snapshots without getting into trouble with accidentally merging. It seems like the BuildServerDetector helps with that.
but I don't think we should consider that either - some things we assert may contain random/generated information (like guid, port numbers, etc.).
I think for the most part, the GUIDs/port numbers we rely on might be consistent across tests, like a GUID associated with a role identity. For everything else, we probably want to use scrubbers.
Revert PR is #9134.
There was a problem hiding this comment.
One thing we were able to do in the manual snapshot strategy we used for RDG is use a boolean that would regenerate the files but fail the test (see dotnet/aspnetcore@
754e56e/src/Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateCreationTestBase.cs#L363). This makes it easy to bootstrap or iterate on new snapshots without getting into trouble with accidentally merging. It seems like theBuildServerDetectorhelps with that.
DiffEngineTray can accept all pending snapshots in one go 😉
And yes for scrubbers.
Description
This pull request introduces significant changes to the Azure hosting infrastructure by replacing the
AppIdentityResourcewith the more robustAzureUserAssignedIdentityResource. It also adds new extension methods, updates role assignment logic, and includes comprehensive tests to verify the new functionality.Replacement of
AppIdentityResourcewithAzureUserAssignedIdentityResource:AppIdentityResourceclass and replaced it with the newAzureUserAssignedIdentityResourceclass, which provides improved support for Azure user-assigned managed identities. (src/Aspire.Hosting.Azure/AppIdentityResource.csremoved,src/Aspire.Hosting.Azure/AzureUserAssignedIdentityResource.csadded) [1] [2]AzureResourcePreparerto useAzureUserAssignedIdentityResourceinstead ofAppIdentityResource. [1] [2] [3] [4]Enhancements to Role Assignment Logic:
IsResourceValidForRoleAssignmentsmethod to streamline the validation of resources eligible for role assignments.New Extension Methods:
AzureUserAssignedIdentityExtensionsclass to provide a convenient method for adding Azure user-assigned identities to the application model. This includes support for configuring infrastructure and handling execution contexts. (src/Aspire.Hosting.Azure/AzureUserAssignedIdentityExtensions.cs)Comprehensive Testing:
AzureUserAssignedIdentityTeststo verify the behavior of theAzureUserAssignedIdentityResourceand its integration with role assignments and other resources. (tests/Aspire.Hosting.Azure.Tests/AzureUserAssignedIdentityTests.cs)tests/Aspire.Hosting.Azure.Tests/Snapshots) [1] [2] [3] [4]Contributes towards #8441.
Checklist
<remarks />and<code />elements on your triple slash comments?