Skip to content
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

Custom Manifest Output & Custom Identities for Projects #3339

Closed
wants to merge 6 commits into from

Conversation

rudiv
Copy link

@rudiv rudiv commented Apr 2, 2024

I'm not sure if this is helpful to the wider project, but the ability to support different Managed Identities for a deployed Project is super useful.

Some semi-related issues: #2730 #1864

What is this

For example, consider 4 apps (A, B, C, D). We may want to configure that A and B can access C, but only B can access D. Within ACA by default it's a free for all so we have to rely on our own auth implementation.

The best way to do this is with specific managed identities for each application and use MSAL for request authorisation, yet right now there is no System Assigned Identity and only the User Assigned Identity that's used for the Container Registry.

This change introduces 2 core concepts:

  • Aspire.Hosting: Custom Manifest Output (via Annotation)
  • Aspire.Hosting.Azure: User Assigned Identity support against the project resource

The former allows anyone to define a custom output in to the manifest that may be useful for other tools in the future, but is basically groundwork to support this Azure specific extension.

The latter can be used either with hardcoded paths or through a Bicep/AzureConstruct (#2429)* Resource.

Usage

var projectResource = builder.AddProject<Projects.ServiceA>("servicea")
    .WithUserAssignedIdentity("IDEN1", "00000000-0000-0000-0000-000000000000", "/subscriptions/<subscription_id>/resourcegroups/my-rg/providers/Microsoft.ManagedIdentity/userAssignedIdentities/my-user")
    .WithUserAssignedIdentity("IDEN2", bicepResource.GetOutput("clientId"), bicepResource.GetOutput("resourceId"));

This outputs the following to the manifest:

"userAssignedIdentities": [
 {
   "clientId": "00000000-0000-0000-0000-000000000000",
   "resourceId": "/subscriptions/\u003Csubscription_id\u003E/resourcegroups/my-rg/providers/Microsoft.ManagedIdentity/userAssignedIdentities/my-user",
   "env": "IDEN1"
 }
 {
   "clientId": "{identities.outputs.clientId}",
   "resourceId": "{identities.outputs.resourceId}",
   "env": "IDEN2"
 }
]

Which can then be consumed by azd (See Azure/azure-dev#3634) that add in the appropriate Client IDs for the app to use, in this case IDEN1_CLIENT_ID and IDEN2_CLIENT_ID.

Other approaches considered

Originally, I'd planned to just output the Resource ID from here that would be consumed by azd to wire up the userAssignedIdentity on the container as that's the only thing really required that can't be done outside of Bicep. The WithUserAssignedIdentity would then wire up both the output of this Resource ID and the Environment Variable separately.

Whilst this reduces the amount of work that azd would have to do, it feels a bit less "general purpose".

What I mean by that is two fold, in our scenario we'd like both the Client ID for use in external apps (done as part of this) and the Principal ID (can be easily added to this - right now we just look up using the resource ID) for assignment of this identity to an app registration to use in the Graph API.

Basically, having more information in the manifest feels preferable to less, even if it means slightly more work for azd to perform.


As I said at the start, I'm not sure how this really fits in to Aspire as a whole as it's definitively deployment related, but it's helped me to wire things up much easier in a "proper" way as the alternative is modifying the generated Bicep and .yaml files for each container, whereas with this we only need to provide the Bicep for the Identity creation (and in the future, hopefully use the new Construct methods).

This as usual with my stuff relates to Azure/azure-dev#3184, and whilst I'm aware that changes are incoming I didn't see anything about managed identity per-app support in any other discussions.

* There probably needs Aspire.Hosting.Azure.ManagedServiceIdentities or similar which I'd be happy to contribute as part of this PR if you confirm this is a right path to take. It's obviously a relatively simple resource:

var bicepResource = builder.AddAzureConstruct("identity", construct =>
{
    var uai = new UserAssignedIdentity(construct, name: "testIdentity");
    uai.AddOutput("resourceId", sa => sa.Id);
    uai.AddOutput("clientId", sa => sa.ClientId);
});
Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Apr 2, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 2, 2024
@davidfowl
Copy link
Member

This work is going to be post GA, but I've been working on this long-term branch that takes over bicep generation from azd https://github.com/dotnet/aspire/tree/davidfowl/gen-compute-infra. Instead of building up a bicep string, this will move to use the azure provisioning libraries for container apps (these don't exist yet). The way we extend or describe container app is something like Azure/azure-dev#3292 (comment). A callback that lets you describe the container app companion resource for the project/container resource.

Given that I think this scenario would look like:

var builder = DistributedApplication.CreateBuilder(args);

var identity = builder.AddAzureUserAssignedIdentity("identity");

var storage = builder.AddAzureStorage("storage").AddBlob("blob");

// Dynamically create a role assignment between the identity and the storage account
builder.AddAzureRoleAssignment(storage, identity, RoleAssignment.StorageContributor);

var apiService = builder.AddProject<Projects.AspireAppWithHttps_ApiService>("apiservice")
                                      // AZURE_CLIENT_ID=identity.clientid
                                      .WithUserAssignedManagedIdentity(identity) 
                                      // Associate the identity with the container app
                                      .PublishAsAzureContainerApp(c => c.AddUserAssignedIdentity(identity)); 

builder.Build().Run();

I made up a ton of these APIs, I'm sure there will be higher and lower level in the fullness of time to represent what you want to.

PS: Right now we're super busy cleaning up APIs and doing lots of fit and finish to get to GA. Soon after we will be looking at this in more detail.

Thanks for sticking in there 😄

@ellismg
Copy link

ellismg commented Apr 8, 2024

@davidfowl - Happy to take the PR on the azd to support this if the manifest if you wanted to do something here before GA and you move all this logic into Aspire proper post GA.

@davidfowl
Copy link
Member

I'm not a fan of putting this in the manifest. It's too tied to azure in its current form. I understand the scenario we are trying to unblock but I rather not go down this route.

@rudiv
Copy link
Author

rudiv commented Apr 11, 2024

I'm not a fan of putting this in the manifest. It's too tied to azure in its current form. I understand the scenario we are trying to unblock but I rather not go down this route.

Understand this to an extent, and it's specifically why I added this in as a concept of a custom manifest output so that it's more general purpose than just tying everything to Azure / azd.

That is to say that custom manifest output is general purpose and could be used for much more than managed identity in the future. In my opinion it also opens up the manifest to other tools that are more specific to different environments (or even requirements, consider a scenario where you'd want to provide metadata to another app that isn't even deployment related, but you still want to be configured through your Aspire app from a developer perspective).

The Azure specific stuff is in the .Azure project (where it should be for that exact purpose?) and of course the changes I've made in azd are Azure specific too because, well it's an Azure Developer CLI 😃

I think what I'm getting at is that, whilst this specific implementation of the custom manifest output would be Azure specific, I don't feel that the concept actually is. I do understand that keeping the manifest as simple and generic as possible appears to be the goal here, but as old developers we all know that only goes so far.

This is quite useful to us now (using a custom build of azd) for deploying what we require without stepping down to Bicep too much or manually adjusting the YAML. Even if most Aspire users would just bung their services up using connection strings rather than identities, or stick with the default one that has Registry access etc etc, this is a very low-cost way to give even more flexibility.

PS. On your other reply @davidfowl I get where this is heading, it's just really hard to picture without the associated libs.

@davidfowl
Copy link
Member

I understand and appreciate the thinking and the PR but it's not the direction we want to go in.

@rudiv
Copy link
Author

rudiv commented Apr 13, 2024

OK, since the needle on support for this won't move until after GA. I've created a package that adds support for anyone else that might want more control.

Only MI / KV support at the moment, but will add more as we need it. Trying to move away from the plethora of custom bicep and towards the world I think we all envisage of Aspire being able to do everything in C#.

Feel free to close this if it's never going to be merged.

@davidfowl
Copy link
Member

Very cool package!

@davidfowl davidfowl closed this Apr 14, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants