Support setting existing app identity on compute resources#9404
Support setting existing app identity on compute resources#9404captainsafia merged 4 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for associating an existing AzureUserAssignedIdentityResource with compute resources via a new WithAzureUserAssignedIdentity extension and updates the provisioning logic to handle identity annotations and role assignments. It also includes comprehensive unit tests and updated Bicep snapshots to verify the new scenarios.
- Introduced
WithAzureUserAssignedIdentity<T>extension method for compute resource builders. - Enhanced
AzureResourcePreparerto attach identity annotations without duplication and create fall-back identities for role assignments. - Expanded unit tests and updated Bicep snapshots for various identity and role-assignment scenarios.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Aspire.Hosting.Azure/AzureUserAssignedIdentityExtensions.cs | Added WithAzureUserAssignedIdentity extension and XML docs |
| src/Aspire.Hosting.Azure/AzureResourcePreparer.cs | Adjusted role-assignment logic to attach/copy identity resources |
| tests/Aspire.Hosting.Azure.Tests/AzureUserAssignedIdentityTests.cs | New tests for WithAzureUserAssignedIdentity methods |
| tests/.../Snapshots/*.verified.bicep | Updated Bicep snapshots to include identity settings |
Comments suppressed due to low confidence (3)
src/Aspire.Hosting.Azure/AzureResourcePreparer.cs:181
- [nitpick] The local variable
resourceshadows the method parameter; consider renaming it (e.g.,targetResource) to improve clarity and reduce confusion.
if (resource != identityResource)
src/Aspire.Hosting.Azure/AzureResourcePreparer.cs:187
- [nitpick] Indentation here is inconsistent with the surrounding block; aligning this line will improve readability.
resource.Annotations.Add(new AppIdentityAnnotation(identityResource));
tests/Aspire.Hosting.Azure.Tests/AzureUserAssignedIdentityTests.cs:2
- [nitpick] Consider scoping this
#pragma warning disableto the specific code region or pairing it with a restore directive, so the suppression doesn’t apply to the entire file unintentionally.
#pragma warning disable ASPIRECOMPUTE001 // Type is for evaluation purposes ...
|
Should it be possible to add as well as replace? Maybe that can be a separate issue? |
Hmmm....how does this function in practice? Do role assignments apply on both identities or do we need a way to specify the identity that a role assignment should target? I worry that the API might get too confusing with that many permutations. |
To try to enable multiple identities would take more work in the client libraries as well, since I assume we would want to specify which identity to use to connect to an Azure resource. If someone wants to use multiple identities, I think they would need to do the heavy lifting themselves. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
b934c53 to
38a343e
Compare
Description
This pull request introduces the ability to associate an
AzureUserAssignedIdentityResourcewith a compute resource via theWithAzureUserAssignedIdentityextension method.Fixes #8441
Checklist
<remarks />and<code />elements on your triple slash comments?