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

    /// <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

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

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).

Added a flag to only check/create for the resources once.

This seems like it's the only point of contention in the PR. Is it resolved? The other changes look good. The health check creating the database feels so wrong though 😄 .

This is something we did not solve in the Aspire 9.0 timeframe that we should revisit now.

We should have a live chat about it, I tried different options. Right now it does the creation attempt only once then falls back to the standard check. This is just the apphost health check though to report for WaithHealthyAsync, the clients use the standard health check.

I approved the PR, can you open an issue? It's kinda related to #7101

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

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

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

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).

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

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

davidfowl · 2025-01-16T01:38:35Z

Can we decide if we are going to make the breaking change in 9.1? If yes, we should make sure it's easy to get back to the previous state (and we should validate it)

sebastienros · 2025-01-16T03:12:03Z

Can we decide if we are going to make the breaking change in 9.1? If yes, we should make sure it's easy to get back to the previous state (and we should validate it)

This is done with AddAzureCosmosDB("cosmos").WithAccessKeyAuthentication()

Here is the test that verifies that adding it behaves the same way as before in terms of bicep resources. I also validated it in playground.

https://github.com/dotnet/aspire/pull/7092/files#diff-1d15fcad74e27b197d015f339f523828ef1f59378caa471c4bb8f19d133095efR250

davidfowl · 2025-01-16T05:08:22Z

OK let's go with it. Open up a breaking change issue?

davidfowl · 2025-01-16T05:08:31Z

/azp run

azure-pipelines · 2025-01-16T05:08:42Z

Azure Pipelines successfully started running 1 pipeline(s).

eerhardt · 2025-01-22T21:12:30Z

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

    /// <summary>
    /// Gets the "connectionString" reference from the secret outputs of the Azure Cosmos DB resource.
    /// </summary>
    public BicepSecretOutputReference ConnectionString => new("connectionString", this);


Is this property used anymore? I don't see it being used now that we have internal BicepSecretOutputReference? ConnectionStringSecretOutput { get; set; }.

I don't think it is, but it's public so couldn't remove

As a reminder, it started with you asking to change the type to BicepOutputReference, which we couldn't so we don't break, and we created the new one.

Can we make it nullable and log it as a breaking change?

A very minor breaking change that I don't think would affect anyone.

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 eerhardt and radical 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

src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBResource.cs Show resolved Hide resolved

sebastienros added 2 commits January 14, 2025 09:55

Normalize namespaces

9208c29

paulomorgado mentioned this pull request Jan 15, 2025

Add KeycloakRealmResource and AddRealm method #7120

Closed

18 tasks

Create principalTypeParameter

e17cf4a

sebastienros added 7 commits January 15, 2025 10:58

Improve health check logic

8c9b13d

Update package references

29e8624

Add ConnectionStringOutput property

fd462a5

Remove obsolete

c5a2cfc

Merge branch 'main' into sebros/cosmosidentity

ce738f4

Update biceps

357d2f2

Fix tests

3870d36

This was referenced Jan 16, 2025

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Closed

Some tests timing out on helix #5247

Closed

sebastienros mentioned this pull request Jan 16, 2025

[Breaking change]: Aspire.Hosting.Azure.CosmosDB obsolete method and default managed identity support dotnet/docs-aspire#2428

Closed

3 tasks

Bump health check package version

518efec

davidfowl approved these changes Jan 16, 2025

View reviewed changes

sebastienros mentioned this pull request Jan 16, 2025

Make UseAccessKeyAuthentication public so that it is accessible for p… #7135

Closed

17 tasks

sebastienros force-pushed the sebros/cosmosidentity branch from a78e929 to 518efec Compare January 16, 2025 21:19

sebastienros merged commit 6dac55b into main Jan 17, 2025
10 checks passed

sebastienros deleted the sebros/cosmosidentity branch January 17, 2025 01:20

eerhardt reviewed Jan 22, 2025

View reviewed changes

sebastienros mentioned this pull request Jan 27, 2025

AddDatabase doesn't add any database #6949

Closed

eerhardt mentioned this pull request Feb 4, 2025

EventHubs, ServiceBus, and CosmosDB Hosting integrations should create Resources for children #7407

Closed

github-actions bot locked and limited conversation to collaborators Feb 22, 2025

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

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

Uh oh!

Conversation

sebastienros commented Jan 13, 2025

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sebastienros commented Jan 13, 2025

Uh oh!

sebastienros commented Jan 14, 2025

Uh oh!

sebastienros commented Jan 14, 2025

Uh oh!

azure-pipelines bot commented Jan 14, 2025

Uh oh!

Uh oh!

davidfowl commented Jan 14, 2025

Uh oh!

eerhardt commented Jan 14, 2025

Uh oh!

davidfowl commented Jan 16, 2025

Uh oh!

sebastienros commented Jan 16, 2025

Uh oh!

davidfowl commented Jan 16, 2025

Uh oh!

davidfowl commented Jan 16, 2025

Uh oh!

azure-pipelines bot commented Jan 16, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

davidfowl Jan 16, 2025 •

edited

Loading