Adding Managed Identity support for connecting to Cosmos DB, Update dependencies with latest. #71

sandeepsnairms · 2024-06-18T15:02:29Z

Provide a description of what has been changed

Checklist

Commits are signed with Developer Certificate of Origin (DCO)
A PR is opened to update the documentation on our docs repo

Fixes #

JatinSanghvi · 2024-06-27T09:10:24Z

README.md


 - **`databaseId`** - ID of Cosmos DB database that contains the monitored container.

 - **`containerId`** - ID of the monitored container.

- **`leaseConnection`** - Connection string of the Cosmos DB account that contains the lease container. This can be same or different from the value of `connection` metadata.
+- **`leaseEndpoint`** - Endpoint URL of the Cosmos DB account that contains the lease container. This can be same or different from the value of `connection` metadata.


`connection` metadata to `endpoint` metadata.

JatinSanghvi · 2024-06-27T09:13:31Z

images/architecture.pptx

What changes were made to the PowerPoint file?

JatinSanghvi · 2024-06-27T09:24:00Z

src/Scaler.Demo/OrderGenerator/Dockerfile

-COPY src/Scaler.Demo/OrderGenerator/ src/Scaler.Demo/OrderGenerator/
-COPY src/Scaler.Demo/Shared/ src/Scaler.Demo/Shared/
+ARG BUILD_CONFIGURATION=Release
+WORKDIR /src


Should WORKDIR be /src/Scaler.Demo/OrderGenerator? Other files inside /src will not be required to be copied for either build or restore.

On second thought, the earlier Dockerfile was small and used to work. Can you minimize the changes to each Dockerfile, and keep it close to the original?

JatinSanghvi · 2024-06-27T09:29:36Z

src/Scaler.Demo/OrderGenerator/appsettings.json

@@ -1,6 +1,7 @@
 {
  "CosmosDbConfig": {
-    "Connection": "<connection-string-of-monitored-container-account>",
+    "Endpoint": "https://{Cosmos Account Name}.documents.azure.com:443/",


Either keep just the 'Endpoint' config or make it clear that either one of the two properties are required.

In case of Order Generator, we are not deploying it on AKS. It just used locally to generate CosmosDB records and trigger the Change Feed, hence Order Generator is using connection string method to authenticate.

JatinSanghvi · 2024-06-27T09:33:19Z

src/Scaler.Demo/OrderProcessor/Worker.cs

+                var credential = new DefaultAzureCredential();
+
+                leaseDatabase = new Microsoft.Azure.Cosmos.CosmosClient(_cosmosDbConfig.LeaseEndpoint, credential, clientOptions)
+                    .GetDatabase(_cosmosDbConfig.LeaseDatabaseId);


I will check the demo project's Readme but creating the lease database here prevents user from that manual step.

No new database is getting created, the code has been modified to handle AAD based CS based authentications

Can you create the lease database in that case?

JatinSanghvi · 2024-06-27T10:36:38Z

src/Scaler/Services/CosmosDbMetricProvider.cs

 namespace Keda.CosmosDb.Scaler
 {
    internal sealed class CosmosDbMetricProvider : ICosmosDbMetricProvider
    {
        private readonly CosmosDbFactory _factory;
        private readonly ILogger<CosmosDbMetricProvider> _logger;

+		static Meter s_meter = new Meter("OrderProcessor.CFStore", "1.0.0");


Set visibility of properties explicitly. Use standard C# property names and put newlines between property declarations and methods.

JatinSanghvi · 2024-06-27T10:38:10Z

src/Scaler/Services/CosmosDbMetricProvider.cs

-                    .GetContainer(scalerMetadata.DatabaseId, scalerMetadata.ContainerId)
-                    .GetChangeFeedEstimator(scalerMetadata.ProcessorName, leaseContainer);
+                var credential = new DefaultAzureCredential();
+                var cosmosClient = new CosmosClient(scalerMetadata.Endpoint, credential);


Use GetCosmosClient to have a single connection if both endpoints happen to be same.

JatinSanghvi · 2024-06-27T10:40:27Z

src/Scaler/Services/CosmosDbMetricProvider.cs

                }

+                s_CFRecordsReceived.Add(lagCount);


I don't find s_* variables are used anywhere. Can you explain what do they achieve?

JatinSanghvi · 2024-06-27T10:41:49Z

src/Scaler/Services/ScalerMetadata.cs

@@ -39,8 +41,16 @@ private string LeaseAccountHost
        {
            get
            {
-                var builder = new DbConnectionStringBuilder { ConnectionString = this.LeaseConnection };
-                return new Uri((string)builder["AccountEndpoint"]).Host;
+                if(!string.IsNullOrEmpty(this.LeaseConnection))


Do we support connection string based connections or not?

this was a miss

JatinSanghvi · 2024-06-27T10:43:35Z

src/Scaler.Tests/CosmosDbScalerServiceTests.cs

@@ -18,10 +18,10 @@ public CosmosDbScalerServiceTests()
        }

        [Theory]
-        [InlineData("connection")]
+        [InlineData("endpoint")]


Retain support for connection-string based connections and have tests for both scenarios.

JatinSanghvi · 2024-06-28T03:50:28Z

src/Scaler/Services/CosmosDbMetricProvider.cs

                int partitionCount = 0;

-                using (FeedIterator<ChangeFeedProcessorState> iterator = estimator.GetCurrentStateIterator())
+                long lagCount=0;


Put proper spacing.

JatinSanghvi · 2024-06-28T03:58:40Z

src/Scaler.Demo/OrderGenerator/Dockerfile

@@ -1,16 +1,23 @@
-# https://hub.docker.com/_/microsoft-dotnet
+#See https://aka.ms/customizecontainer to learn how to customize your debug container and how Visual Studio uses this Dockerfile to build your images for faster debugging.


Is this Dockerfile generated using Visual Studio?

yes, the original was not working. So, had to overwrite with VS generated Docker.

Sandeep Nair added 2 commits June 18, 2024 19:34

Update Dependencies, Make Cosmos connection using Managed Identity

4dca82e

Updating Readme for Managed Identity configuration steps

6498184

sandeepsnairms requested review from JatinSanghvi and tomkerkhove as code owners June 18, 2024 15:02

JatinSanghvi reviewed Jun 27, 2024

View reviewed changes

JatinSanghvi reviewed Jun 28, 2024

View reviewed changes

Addressing review comments

e941b5f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Managed Identity support for connecting to Cosmos DB, Update dependencies with latest. #71

Adding Managed Identity support for connecting to Cosmos DB, Update dependencies with latest. #71

sandeepsnairms commented Jun 18, 2024

JatinSanghvi Jun 27, 2024

sandeepsnairms Jun 28, 2024

JatinSanghvi Jun 27, 2024

sandeepsnairms Jun 28, 2024

JatinSanghvi Jun 27, 2024 •

edited

Loading

JatinSanghvi Jun 28, 2024

JatinSanghvi Jun 27, 2024

sandeepsnairms Jun 27, 2024

JatinSanghvi Jun 27, 2024

sandeepsnairms Jun 27, 2024

JatinSanghvi Jun 28, 2024

sandeepsnairms Jun 28, 2024

JatinSanghvi Jun 27, 2024

sandeepsnairms Jun 27, 2024

JatinSanghvi Jun 27, 2024

sandeepsnairms Jun 27, 2024

JatinSanghvi Jun 27, 2024

JatinSanghvi Jun 27, 2024

sandeepsnairms Jun 28, 2024

JatinSanghvi Jun 27, 2024

JatinSanghvi Jun 28, 2024 •

edited

Loading

JatinSanghvi Jun 28, 2024

sandeepsnairms Jun 28, 2024

		@@ -1,16 +1,23 @@
		# https://hub.docker.com/_/microsoft-dotnet
		#See https://aka.ms/customizecontainer to learn how to customize your debug container and how Visual Studio uses this Dockerfile to build your images for faster debugging.

Adding Managed Identity support for connecting to Cosmos DB, Update dependencies with latest. #71

Are you sure you want to change the base?

Adding Managed Identity support for connecting to Cosmos DB, Update dependencies with latest. #71

Conversation

sandeepsnairms commented Jun 18, 2024

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

JatinSanghvi Jun 27, 2024 • edited Loading

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

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

JatinSanghvi Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JatinSanghvi Jun 27, 2024 •

edited

Loading

JatinSanghvi Jun 28, 2024 •

edited

Loading