From ac0c64b3c733f9d27491c73aa48dee3d6468c982 Mon Sep 17 00:00:00 2001 From: Piotr Karpala Date: Tue, 11 Jun 2024 15:48:02 -0400 Subject: [PATCH] Fixing tests --- README.md | 14 +- .../CosmosDbScalerServiceTests.cs | 130 ++++++++++++------ src/Scaler/Services/CosmosDbMetricProvider.cs | 4 +- src/Scaler/Services/ScalerMetadata.cs | 24 ++++ 4 files changed, 124 insertions(+), 48 deletions(-) diff --git a/README.md b/README.md index 25876d3..e957432 100644 --- a/README.md +++ b/README.md @@ -68,10 +68,12 @@ The specification below describes the `trigger` metadata in `ScaledObject` resou - type: external metadata: scalerAddress: external-scaler-azure-cosmos-db.keda:4050 # Mandatory. Address of the external scaler service. - connection: # Mandatory. Connection string of Cosmos DB account with monitored container. + connection: # Optional. Connection string of Cosmos DB account with monitored container. Either `connection` or `endpoint` has to be provided. + endpoint: # Optional. Cosmos DB endpoint with monitored container. Either `connection` or `endpoint` has to be provided. databaseId: # Mandatory. ID of Cosmos DB database containing monitored container. containerId: # Mandatory. ID of monitored container. - leaseConnection: # Mandatory. Connection string of Cosmos DB account with lease container. + leaseConnection: # Optional. Connection string of Cosmos DB account with lease container. Either `leaseConnection` or `leaseEndpoint` has to be provided. + leaseEndpoint: # Optional. Cosmos DB endpoint with lease container. Either `leaseConnection` or `leaseEndpoint` has to be provided. leaseDatabaseId: # Mandatory. ID of Cosmos DB database containing lease container. leaseContainerId: # Mandatory. ID of lease container. processorName: # Mandatory. Name of change-feed processor used by listener application. @@ -81,13 +83,13 @@ The specification below describes the `trigger` metadata in `ScaledObject` resou - **`scalerAddress`** - Address of the external scaler service. This would be in format `.:`. If you installed Azure Cosmos DB external scaler Helm chart in `keda` namespace and did not specify custom values, the metadata value would be `external-scaler-azure-cosmos-db.keda:4050`. -- **`connection`** - Connection string of the Cosmos DB account that contains the monitored container. +- **`connection`** or **`endpoint`** - Connection string of the Cosmos DB account or Cosmos DB endpoint that contains the monitored container. - **`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. +- **`leaseConnection`** or **`leaseEndpoint`**- Connection string of the Cosmos DB account or Cosmos DB endpoint that contains the lease container. This can be same or different from the value of `connection` metadata. - **`leaseDatabaseId`** - ID of Cosmos DB database that contains the lease container. This can be same or different from the value of `databaseId` metadata. @@ -95,4 +97,6 @@ The specification below describes the `trigger` metadata in `ScaledObject` resou - **`processorName`** - Name of change-feed processor used by listener application. For more information on this, you can refer to [Implementing the change feed processor](https://docs.microsoft.com/azure/cosmos-db/sql/change-feed-processor#implementing-the-change-feed-processor) section. -> **Note** Ideally, we would have created `TriggerAuthentication` resource that would have prevented us from adding the connection strings in plain text in the `ScaledObject` trigger metadata. However, this is not possible since at the moment, the triggers of `external` type do not support referencing a `TriggerAuthentication` resource ([link](https://keda.sh/docs/scalers/external/#authentication-parameters)). +### Workload Identity support + +To utilize Azure Workload Identity via Default Azure Credential use **`endpoint`** and **`leaseEndpoint`** parameters. \ No newline at end of file diff --git a/src/Scaler.Tests/CosmosDbScalerServiceTests.cs b/src/Scaler.Tests/CosmosDbScalerServiceTests.cs index 4e2dc47..2156b0e 100644 --- a/src/Scaler.Tests/CosmosDbScalerServiceTests.cs +++ b/src/Scaler.Tests/CosmosDbScalerServiceTests.cs @@ -1,3 +1,4 @@ +using System; using System.Threading.Tasks; using Google.Protobuf.Collections; using Moq; @@ -18,10 +19,8 @@ public CosmosDbScalerServiceTests() } [Theory] - [InlineData("connection")] [InlineData("databaseId")] [InlineData("containerId")] - [InlineData("leaseConnection")] [InlineData("leaseDatabaseId")] [InlineData("leaseContainerId")] [InlineData("processorName")] @@ -31,29 +30,31 @@ await Assert.ThrowsAsync( () => _cosmosDbScalerService.IsActive(GetScaledObjectRefWithoutMetadata(metadataKey), null)); } - [Fact] - public async Task IsActive_ReturnsFalseOnZeroPartitions() + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task IsActive_ReturnsFalseOnZeroPartitions(bool workloadIdentity) { _metricProviderMock.Setup(provider => provider.GetPartitionCountAsync(It.IsAny())).ReturnsAsync(0L); - IsActiveResponse response = await _cosmosDbScalerService.IsActive(GetScaledObjectRef(), null); + IsActiveResponse response = await _cosmosDbScalerService.IsActive(GetScaledObjectRef(workloadIdentity), null); Assert.False(response.Result); } [Theory] - [InlineData(1L)] - [InlineData(100L)] - public async Task IsActive_ReturnsFalseOnNonZeroPartitions(long partitionCount) + [InlineData(1L, true)] + [InlineData(1L, false)] + [InlineData(100L, true)] + [InlineData(100L, false)] + public async Task IsActive_ReturnsFalseOnNonZeroPartitions(long partitionCount, bool workloadIdentity) { _metricProviderMock.Setup(provider => provider.GetPartitionCountAsync(It.IsAny())).ReturnsAsync(partitionCount); - IsActiveResponse response = await _cosmosDbScalerService.IsActive(GetScaledObjectRef(), null); + IsActiveResponse response = await _cosmosDbScalerService.IsActive(GetScaledObjectRef(workloadIdentity), null); Assert.True(response.Result); } [Theory] - [InlineData("connection")] [InlineData("databaseId")] [InlineData("containerId")] - [InlineData("leaseConnection")] [InlineData("leaseDatabaseId")] [InlineData("leaseContainerId")] [InlineData("processorName")] @@ -64,13 +65,16 @@ await Assert.ThrowsAsync( } [Theory] - [InlineData(0L)] - [InlineData(1L)] - [InlineData(100L)] - public async Task GetMetrics_ReturnsPartitionCount(long partitionCount) + [InlineData(0L, true)] + [InlineData(0L, false)] + [InlineData(1L, true)] + [InlineData(1L, false)] + [InlineData(100L, true)] + [InlineData(100L, false)] + public async Task GetMetrics_ReturnsPartitionCount(long partitionCount, bool workloadIdentity) { _metricProviderMock.Setup(provider => provider.GetPartitionCountAsync(It.IsAny())).ReturnsAsync(partitionCount); - GetMetricsResponse response = await _cosmosDbScalerService.GetMetrics(GetGetMetricsRequest(), null); + GetMetricsResponse response = await _cosmosDbScalerService.GetMetrics(GetGetMetricsRequest(workloadIdentity), null); Assert.Single(response.MetricValues); @@ -82,14 +86,16 @@ public async Task GetMetrics_ReturnsPartitionCount(long partitionCount) } [Theory] - [InlineData("")] - [InlineData("custom-metric-name")] - public async Task GetMetrics_ReturnsSameMetricNameIfPassed(string requestMetricName) + [InlineData("", true)] + [InlineData("", false)] + [InlineData("custom-metric-name", true)] + [InlineData("custom-metric-name", false)] + public async Task GetMetrics_ReturnsSameMetricNameIfPassed(string requestMetricName, bool workloadIdentity) { _metricProviderMock.Setup(provider => provider.GetPartitionCountAsync(It.IsAny())).ReturnsAsync(1L); // No assertion with request.MetricName since it is ignored. - GetMetricsRequest request = GetGetMetricsRequest(); + GetMetricsRequest request = GetGetMetricsRequest(workloadIdentity); request.ScaledObjectRef.ScalerMetadata["metricName"] = requestMetricName; GetMetricsResponse response = await _cosmosDbScalerService.GetMetrics(request, null); @@ -99,10 +105,8 @@ public async Task GetMetrics_ReturnsSameMetricNameIfPassed(string requestMetricN } [Theory] - [InlineData("connection")] [InlineData("databaseId")] [InlineData("containerId")] - [InlineData("leaseConnection")] [InlineData("leaseDatabaseId")] [InlineData("leaseContainerId")] [InlineData("processorName")] @@ -112,10 +116,30 @@ await Assert.ThrowsAsync( () => _cosmosDbScalerService.GetMetricSpec(GetScaledObjectRefWithoutMetadata(metadataKey), null)); } - [Fact] - public async Task GetMetricSpec_ReturnsMetricSpec() + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task GetMetricSpec_DoesNotThrowsOnOptionalMetadata(bool workloadIdentity) + { + await _cosmosDbScalerService.GetMetricSpec(GetScaledObjectRef(workloadIdentity), null); + } + + [Theory] + [InlineData("endpoint", "connection")] + [InlineData("leaseEndpoint", "leaseConnection")] + public async Task GetMetricSpec_ThrowsOnMissingConnections(string firstMetadataKey, string secondMetadataKey) + { + var exception = await Assert.ThrowsAnyAsync( + () => _cosmosDbScalerService.GetMetricSpec(GetScaledObjectRefWithoutMetadata(firstMetadataKey, secondMetadataKey), null)); + Assert.IsType(exception.InnerException); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task GetMetricSpec_ReturnsMetricSpec(bool workloadIdentity) { - GetMetricSpecResponse response = await _cosmosDbScalerService.GetMetricSpec(GetScaledObjectRef(), null); + GetMetricSpecResponse response = await _cosmosDbScalerService.GetMetricSpec(GetScaledObjectRef(workloadIdentity), null); Assert.Single(response.MetricSpecs); @@ -127,11 +151,13 @@ public async Task GetMetricSpec_ReturnsMetricSpec() } [Theory] - [InlineData("")] - [InlineData("custom-metric-name")] - public async Task GetMetricSpec_ReturnsSameMetricNameIfPassed(string requestMetricName) + [InlineData("", true)] + [InlineData("", false)] + [InlineData("custom-metric-name", true)] + [InlineData("custom-metric-name", false)] + public async Task GetMetricSpec_ReturnsSameMetricNameIfPassed(string requestMetricName, bool workloadIdentity) { - ScaledObjectRef request = GetScaledObjectRef(); + ScaledObjectRef request = GetScaledObjectRef(workloadIdentity); request.ScalerMetadata["metricName"] = requestMetricName; GetMetricSpecResponse response = await _cosmosDbScalerService.GetMetricSpec(request, null); @@ -140,11 +166,20 @@ public async Task GetMetricSpec_ReturnsSameMetricNameIfPassed(string requestMetr Assert.Equal(requestMetricName, response.MetricSpecs[0].MetricName); } - [Fact] - public async Task GetMetricSpec_ReturnsNormalizedMetricName() + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task GetMetricSpec_ReturnsNormalizedMetricName(bool workloadIdentity) { - ScaledObjectRef request = GetScaledObjectRef(); - request.ScalerMetadata["leaseConnection"] = "AccountEndpoint=https://example.com:443/;AccountKey=ZHVtbXky"; + ScaledObjectRef request = GetScaledObjectRef(workloadIdentity); + if (workloadIdentity) + { + request.ScalerMetadata["leaseEndpoint"] = "https://example.com:443"; + } + else + { + request.ScalerMetadata["leaseConnection"] = "AccountEndpoint=https://example.com:443/;AccountKey=ZHVtbXky"; + } request.ScalerMetadata["leaseDatabaseId"] = "Dummy.Lease.Database.Id"; request.ScalerMetadata["leaseContainerId"] = "Dummy:Lease:Container:Id"; request.ScalerMetadata["processorName"] = "Dummy%Processor%Name"; @@ -158,12 +193,12 @@ public async Task GetMetricSpec_ReturnsNormalizedMetricName() response.MetricSpecs[0].MetricName); } - private static GetMetricsRequest GetGetMetricsRequest() + private static GetMetricsRequest GetGetMetricsRequest(bool workloadIdentity) { return new GetMetricsRequest { MetricName = "dummy-metric-name", - ScaledObjectRef = GetScaledObjectRef(), + ScaledObjectRef = GetScaledObjectRef(workloadIdentity), }; } @@ -176,15 +211,20 @@ private static GetMetricsRequest GetGetMetricsRequestWithoutMetadata(string meta }; } - private static ScaledObjectRef GetScaledObjectRefWithoutMetadata(string metadataKey) + private static ScaledObjectRef GetScaledObjectRefWithoutMetadata(params string[] metadataKeys) { - var scaledObjectRef = GetScaledObjectRef(); - scaledObjectRef.ScalerMetadata.Remove(metadataKey); + var scaledObjectRef = GetScaledObjectRef(workloadIdentity: true); + // this is not technically correct but for sake of the test we need both connection and endpoint to be present + scaledObjectRef.ScalerMetadata["endpoint"] = "https://example1.com:443"; + scaledObjectRef.ScalerMetadata["leaseEndpoint"] = "https://example2.com:443"; + + foreach (string metadataKey in metadataKeys) + scaledObjectRef.ScalerMetadata.Remove(metadataKey); return scaledObjectRef; } - private static ScaledObjectRef GetScaledObjectRef() + private static ScaledObjectRef GetScaledObjectRef(bool workloadIdentity = false) { var scaledObjectRef = new ScaledObjectRef { @@ -194,10 +234,18 @@ private static ScaledObjectRef GetScaledObjectRef() MapField scalerMetadata = scaledObjectRef.ScalerMetadata; - scalerMetadata["connection"] = "AccountEndpoint=https://example1.com:443/;AccountKey=ZHVtbXkx"; + if (workloadIdentity) + { + scalerMetadata["endpoint"] = "https://example1.com:443"; + scalerMetadata["leaseEndpoint"] = "https://example2.com:443"; + } + else + { + scalerMetadata["connection"] = "AccountEndpoint=https://example1.com:443/;AccountKey=ZHVtbXkx"; + scalerMetadata["leaseConnection"] = "AccountEndpoint=https://example2.com:443/;AccountKey=ZHVtbXky"; + } scalerMetadata["databaseId"] = "dummy-database-id"; scalerMetadata["containerId"] = "dummy-container-id"; - scalerMetadata["leaseConnection"] = "AccountEndpoint=https://example2.com:443/;AccountKey=ZHVtbXky"; scalerMetadata["leaseDatabaseId"] = "dummy-lease-database-id"; scalerMetadata["leaseContainerId"] = "dummy-lease-container-id"; scalerMetadata["processorName"] = "dummy-processor-name"; diff --git a/src/Scaler/Services/CosmosDbMetricProvider.cs b/src/Scaler/Services/CosmosDbMetricProvider.cs index db9cd85..8ca242b 100644 --- a/src/Scaler/Services/CosmosDbMetricProvider.cs +++ b/src/Scaler/Services/CosmosDbMetricProvider.cs @@ -25,11 +25,11 @@ public async Task GetPartitionCountAsync(ScalerMetadata scalerMetadata) try { Container leaseContainer = _factory - .GetCosmosClient(scalerMetadata.LeaseConnection) + .GetCosmosClient(scalerMetadata.LeaseConnection ?? scalerMetadata.LeaseEndpoint) .GetContainer(scalerMetadata.LeaseDatabaseId, scalerMetadata.LeaseContainerId); ChangeFeedEstimator estimator = _factory - .GetCosmosClient(scalerMetadata.Connection) + .GetCosmosClient(scalerMetadata.Connection ?? scalerMetadata.Endpoint) .GetContainer(scalerMetadata.DatabaseId, scalerMetadata.ContainerId) .GetChangeFeedEstimator(scalerMetadata.ProcessorName, leaseContainer); diff --git a/src/Scaler/Services/ScalerMetadata.cs b/src/Scaler/Services/ScalerMetadata.cs index a2b6d74..05a03a1 100644 --- a/src/Scaler/Services/ScalerMetadata.cs +++ b/src/Scaler/Services/ScalerMetadata.cs @@ -1,5 +1,6 @@ using System; using System.Data.Common; +using System.Runtime.Serialization; using Newtonsoft.Json; namespace Keda.CosmosDb.Scaler @@ -9,10 +10,16 @@ internal sealed class ScalerMetadata { private string _metricName; + [JsonProperty(Required = Required.Default)] public string Connection { get; set; } + [JsonProperty(Required = Required.Default)] + public string Endpoint { get; set; } public string DatabaseId { get; set; } public string ContainerId { get; set; } + [JsonProperty(Required = Required.Default)] public string LeaseConnection { get; set; } + [JsonProperty(Required = Required.Default)] + public string LeaseEndpoint { get; set; } public string LeaseDatabaseId { get; set; } public string LeaseContainerId { get; set; } public string ProcessorName { get; set; } @@ -39,6 +46,10 @@ private string LeaseAccountHost { get { + if (string.IsNullOrEmpty(LeaseConnection)) + { + return new Uri(LeaseEndpoint).Host; + } var builder = new DbConnectionStringBuilder { ConnectionString = this.LeaseConnection }; return new Uri((string)builder["AccountEndpoint"]).Host; } @@ -48,5 +59,18 @@ public static ScalerMetadata Create(ScaledObjectRef scaledObjectRef) { return JsonConvert.DeserializeObject(scaledObjectRef.ScalerMetadata.ToString()); } + + [OnDeserialized] + internal void OnDeserializedMethod(StreamingContext context) + { + if (string.IsNullOrEmpty(LeaseConnection) && string.IsNullOrEmpty(LeaseEndpoint)) + { + throw new JsonSerializationException("Both LeaseConnection and LeaseEndpoint are missing."); + } + if(string.IsNullOrEmpty(Connection) && string.IsNullOrEmpty(Endpoint)) + { + throw new JsonSerializationException("Both Connection and Endpoint are missing."); + } + } } }