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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 22 commits into from
Jan 17, 2025
Merged

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

merged 22 commits into from
Jan 17, 2025

Conversation

sebastienros
Copy link
Member

Description

Fixes #3792

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?

sebastienros
sebastienros commented Jan 13, 2025
View reviewed changes
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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@danmoseley danmoseley added the area-integrations Issues pertaining to Aspire Integrations packages label Jan 13, 2025
eerhardt
eerhardt reviewed Jan 13, 2025
View reviewed changes
playground/CosmosEndToEnd/CosmosEndToEnd.AppHost/Program.cs Outdated Show resolved Hide resolved
src/Components/Aspire.Microsoft.Azure.Cosmos/MicrosoftAzureCosmosSettings.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBResource.cs Show resolved Hide resolved
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@davidfowl davidfowl Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBEmulatorHealthCheck.cs Outdated Show resolved Hide resolved
tests/Aspire.Hosting.Azure.Tests/AzureCosmosDBEmulatorFunctionalTests.cs Outdated Show resolved Hide resolved
tests/Aspire.Hosting.Azure.Tests/AzureCosmosDBEmulatorFunctionalTests.cs Outdated Show resolved Hide resolved
tests/Aspire.Hosting.Azure.Tests/AzureCosmosDBEmulatorFunctionalTests.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBExtensions.cs Show resolved Hide resolved
src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBExtensions.cs Outdated Show resolved Hide resolved
@sebastienros
Copy link
Member Author

/azd run

This was referenced Jan 14, 2025
Open
Open
@sebastienros
Copy link
Member Author

/azd run

@sebastienros
Copy link
Member Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidfowl
Copy link
Member

Do we want this to be a breaking change?

@eerhardt
Copy link
Member

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.

@sebastienros
Merge remote-tracking branch 'origin/main' into sebros/cosmosidentity
3ab8c03 
# 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
Normalize namespaces
9208c29
@paulomorgado paulomorgado mentioned this pull request Jan 15, 2025
Open
18 tasks
@davidfowl
Copy link
Member

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 was referenced Jan 16, 2025
Open
Open
@sebastienros
Copy link
Member Author

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
Copy link
Member

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

@davidfowl
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sebastienros sebastienros mentioned this pull request Jan 16, 2025
Open
3 tasks
@sebastienros sebastienros mentioned this pull request Jan 16, 2025
Draft
17 tasks
@sebastienros sebastienros force-pushed the sebros/cosmosidentity branch from a78e929 to 518efec Compare January 16, 2025 21:19
@sebastienros sebastienros merged commit 6dac55b into main Jan 17, 2025
10 checks passed
@sebastienros sebastienros deleted the sebros/cosmosidentity branch January 17, 2025 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eerhardt eerhardt eerhardt left review comments

@davidfowl davidfowl davidfowl approved these changes

@radical radical Awaiting requested review from radical radical is a code owner

Assignees
No one assigned
Labels
area-integrations Issues pertaining to Aspire Integrations packages
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Cosmos .NET SDK] - Introduce Managed Identity authentication for Cosmos
4 participants
@sebastienros @davidfowl @eerhardt @danmoseley