-
Notifications
You must be signed in to change notification settings - Fork 686
Support setting existing app identity on compute resources #9404
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
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 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
AzureResourcePreparer
to 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
resource
shadows 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 disable
to 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 ...
src/Aspire.Hosting.Azure/AzureUserAssignedIdentityExtensions.cs
Outdated
Show resolved
Hide resolved
Should it be possible to add as well as replace? Maybe that can be a separate issue? |
tests/Aspire.Hosting.Azure.Tests/AzureUserAssignedIdentityTests.cs
Outdated
Show resolved
Hide resolved
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. |
b934c53
to
38a343e
Compare
tests/Aspire.Hosting.Azure.Tests/AzureUserAssignedIdentityTests.cs
Outdated
Show resolved
Hide resolved
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.
LGTM. Nice work!
Description
This pull request introduces the ability to associate an
AzureUserAssignedIdentityResource
with a compute resource via theWithAzureUserAssignedIdentity
extension method.Fixes #8441
Checklist
<remarks />
and<code />
elements on your triple slash comments?