Add support for managed identity in Azure Cosmos DB hosting component #7092

sebastienros · 2025-01-13T20:01:24Z

Description

Checklist

Is this feature complete?
- Yes. Ready to ship.
- No. Follow-up changes expected.
Are you including unit tests for the changes and scenario tests if relevant?
- Yes
- No
Did you add public API?
- Yes
  - If yes, did you have an API Review for it?
    - Yes
    - No
  - Did you add <remarks /> and <code /> elements on your triple slash comments?
    - Yes
    - No
- No
Does the change make any security assumptions or guarantees?
- Yes
  - If yes, have you done a threat model and had a security review?
    - Yes
    - No
- No
Does the change require an update in our Aspire docs?
- Yes
  - Is this introducing a breaking change?
    - Yes
      - Link to aspire-docs issue (please use this breaking-change template):
    - No
      - Link to aspire-docs issue (please use this doc-idea template):
- No

sebastienros · 2025-01-13T20:03:10Z

src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBExtensions.cs

@@ -205,7 +156,7 @@ public static IResourceBuilder<AzureCosmosDBEmulatorResource> WithGatewayPort(th
    /// <param name="builder">Builder for the Cosmos emulator container</param>
    /// <param name="count">Desired partition count.</param>
    /// <returns>Cosmos emulator resource builder.</returns>
-    /// <remarks>Not calling this method will result in the default of 25 partitions. The actual started partitions is always one more than specified.
+    /// <remarks>Not calling this method will result in the default of 10 partitions. The actual started partitions is always one more than specified.


11 (actual) is the default number of partitions when nothing is set, so updated to 10 since the actual is argument + 1

src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBExtensions.cs

playground/CosmosEndToEnd/CosmosEndToEnd.AppHost/Program.cs

src/Components/Aspire.Microsoft.Azure.Cosmos/MicrosoftAzureCosmosSettings.cs

eerhardt · 2025-01-13T22:27:28Z

src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBResource.cs


    internal EndpointReference EmulatorEndpoint => new(this, "emulator");

    /// <summary>
    /// Gets the "connectionString" reference from the secret outputs of the Azure Cosmos DB resource.
+    ///
+    /// This is used when Entra ID authentication is used. The connection string is an output of the bicep template.
    /// </summary>
    public BicepSecretOutputReference ConnectionString => new("connectionString", this);


Isn't this supposed to be:

Suggested change

public BicepSecretOutputReference ConnectionString => new("connectionString", this);

public BicepOutputReference ConnectionString => new("connectionString", this);

It's not a secret reference anymore - it is just a normal bicep output.

That would be a breaking change. How do you suggest we deal with it?

This is why I opt to not make things public unless we absolutely need to make them public.

I would suggest just making the breaking change, and document it. The fact that we are switching to managed identity by default is going to be an even bigger breaking change.

This should be public for sure, we'll need to make this a reference expression.

Actually, we can just expose another property

eerhardt · 2025-01-13T22:29:31Z

src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBEmulatorHealthCheck.cs

+namespace Aspire.Hosting.Azure.CosmosDB;
+
+/// <summary>
+/// This health check also creates default databases and containers for the Azure CosmosDB Emulator.


This feels like too much responsibility. Everytime we are checking for "health" we are rechecking all the dbs and containers.

I think it would be better if the health check was only a health check. And then we "waited" for it to be healthy and only ensure the db and containers are created once.

The default health check is checking only one db and its containers (because its state is only for one db, I assume one could create more instances for other dbs). At least we could do the same, pick the first db.

But I did what you suggested at first, though this could introduce undesired effects that it's stated as healthy but we haven't created the db/containers yet. So while we are creating them other services could have started using them.

Another option would be to change the implementation to do like it does right now, but then store a bool and only check for the first db afterwards (not even the containers).

src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBEmulatorHealthCheck.cs

tests/Aspire.Hosting.Azure.Tests/AzureCosmosDBEmulatorFunctionalTests.cs

eerhardt · 2025-01-13T22:37:21Z

src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBExtensions.cs

        var healthCheckKey = $"{builder.Resource.Name}_check";
-        builder.ApplicationBuilder.Services.AddHealthChecks().AddAzureCosmosDB(sp =>


Do we still need a reference on AspNetCore.HealthChecks.CosmosDb if we aren't using it?

eerhardt · 2025-01-13T22:44:57Z

src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBExtensions.cs

+            var principalIdParameter = new ProvisioningParameter(AzureBicepResource.KnownParameters.PrincipalId, typeof(string));
+            infrastructure.Add(principalIdParameter);
+
+            cosmosAccount.CreateRoleAssignment(CosmosDBBuiltInRole.DocumentDBAccountContributor, RoleManagementPrincipalType.ServicePrincipal, principalIdParameter);


Why isn't this generating a prinicpalType like all the other Azure integrations?

aspire/src/Aspire.Hosting.Azure.AppConfiguration/AzureAppConfigurationExtensions.cs

Lines 38 to 43 in 882ae2b

var principalTypeParameter = new ProvisioningParameter(AzureBicepResource.KnownParameters.PrincipalType, typeof(string));

infrastructure.Add(principalTypeParameter);

var principalIdParameter = new ProvisioningParameter(AzureBicepResource.KnownParameters.PrincipalId, typeof(string));

infrastructure.Add(principalIdParameter);

infrastructure.Add(store.CreateRoleAssignment(AppConfigurationBuiltInRole.AppConfigurationDataOwner, principalTypeParameter, principalIdParameter));

The method to create the role on cosmos doesn't take any. It's working though, you think it must still be missing?

Yes this should be a parameter, @tg-msft?

sebastienros · 2025-01-13T23:43:50Z

/azd run

sebastienros · 2025-01-14T00:53:51Z

/azd run

sebastienros · 2025-01-14T00:58:43Z

/azp run

azure-pipelines · 2025-01-14T00:58:56Z

Azure Pipelines successfully started running 1 pipeline(s).

davidfowl · 2025-01-14T03:53:05Z

src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBResource.cs

+    internal BicepSecretOutputReference? ConnectionStringSecretOutput { get; set; }
+
+    [MemberNotNullWhen(true, nameof(ConnectionStringSecretOutput))]
+    internal bool UseAccessKeyAuthentication => ConnectionStringSecretOutput is not null;


Maybe this should be public too.

davidfowl · 2025-01-14T03:54:16Z

Do we want this to be a breaking change?

eerhardt · 2025-01-14T17:17:21Z

Do we want this to be a breaking change?

At some point, yes. We want managed identity to be the default everywhere, and this is the last integration that still uses secrets by default.

So the question in my mind is: "When do we want to make the breaking change?". I think now is a good time. But I can see an argument to wait until the next major version.

# Conflicts: # playground/CosmosEndToEnd/CosmosEndToEnd.ApiService/Program.cs # playground/CosmosEndToEnd/CosmosEndToEnd.AppHost/Program.cs # src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBExtensions.cs # src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBResource.cs # src/Aspire.Hosting.Azure.CosmosDB/PublicAPI.Unshipped.txt # tests/Aspire.Hosting.Azure.Tests/AzureCosmosDBEmulatorFunctionalTests.cs

sebastienros added 4 commits January 10, 2025 09:40

Fix CosmosDb sample endpoint in xml docs

6d86e13

Add support for managed identity

347c49e

Seed Emulator with custom databases and containers

e337c22

Add tests

b2761eb

sebastienros requested review from radical and eerhardt as code owners January 13, 2025 20:01

sebastienros commented Jan 13, 2025

View reviewed changes

Typo

49b3309

davidfowl reviewed Jan 13, 2025

View reviewed changes

src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBExtensions.cs Outdated Show resolved Hide resolved

sebastienros added 2 commits January 13, 2025 12:29

Fix connection string creation

e897a42

Replace obsolete method

90c6d91

danmoseley added the area-integrations Issues pertaining to Aspire Integrations packages label Jan 13, 2025

eerhardt reviewed Jan 13, 2025

View reviewed changes

sebastienros added 3 commits January 13, 2025 14:50

Fix test and update manifests

e093f46

Merge remote-tracking branch 'origin/main' into sebros/cosmosidentity

28654fa

PR feedback

9107115

This was referenced Jan 14, 2025

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

Unable to checkout dotnet/dnceng#4115

Open

Fix build

8dd206e

davidfowl reviewed Jan 14, 2025

View reviewed changes

sebastienros added 2 commits January 14, 2025 09:55

Normalize namespaces

9208c29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for managed identity in Azure Cosmos DB hosting component #7092

Add support for managed identity in Azure Cosmos DB hosting component #7092

sebastienros commented Jan 13, 2025

sebastienros Jan 13, 2025

eerhardt Jan 13, 2025

sebastienros Jan 13, 2025

eerhardt Jan 13, 2025

davidfowl Jan 14, 2025

davidfowl Jan 14, 2025

eerhardt Jan 13, 2025

sebastienros Jan 13, 2025

eerhardt Jan 13, 2025

eerhardt Jan 13, 2025

sebastienros Jan 13, 2025

davidfowl Jan 14, 2025

sebastienros commented Jan 13, 2025

sebastienros commented Jan 14, 2025

sebastienros commented Jan 14, 2025

azure-pipelines bot commented Jan 14, 2025

davidfowl Jan 14, 2025

davidfowl commented Jan 14, 2025

eerhardt commented Jan 14, 2025

	public BicepSecretOutputReference ConnectionString => new("connectionString", this);
	public BicepOutputReference ConnectionString => new("connectionString", this);

		var healthCheckKey = $"{builder.Resource.Name}_check";
		builder.ApplicationBuilder.Services.AddHealthChecks().AddAzureCosmosDB(sp =>

	var principalTypeParameter = new ProvisioningParameter(AzureBicepResource.KnownParameters.PrincipalType, typeof(string));
	infrastructure.Add(principalTypeParameter);
	var principalIdParameter = new ProvisioningParameter(AzureBicepResource.KnownParameters.PrincipalId, typeof(string));
	infrastructure.Add(principalIdParameter);

	infrastructure.Add(store.CreateRoleAssignment(AppConfigurationBuiltInRole.AppConfigurationDataOwner, principalTypeParameter, principalIdParameter));

Add support for managed identity in Azure Cosmos DB hosting component #7092

Are you sure you want to change the base?

Add support for managed identity in Azure Cosmos DB hosting component #7092

Conversation

sebastienros commented Jan 13, 2025

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienros commented Jan 13, 2025

sebastienros commented Jan 14, 2025

sebastienros commented Jan 14, 2025

azure-pipelines bot commented Jan 14, 2025

Choose a reason for hiding this comment

davidfowl commented Jan 14, 2025

eerhardt commented Jan 14, 2025