From 53ea3fd6d3e4b11b82fb0070ec478cc9afd5a322 Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Thu, 28 Mar 2024 14:59:04 +0000 Subject: [PATCH 01/23] add errorWhenMetricValueEmpty Signed-off-by: Rob Pickerill --- pkg/scalers/aws_cloudwatch_scaler.go | 31 +++++ pkg/scalers/aws_cloudwatch_scaler_test.go | 160 +++++++++++++++++++++- 2 files changed, 190 insertions(+), 1 deletion(-) diff --git a/pkg/scalers/aws_cloudwatch_scaler.go b/pkg/scalers/aws_cloudwatch_scaler.go index 10eb10b260e..13014cd26d2 100644 --- a/pkg/scalers/aws_cloudwatch_scaler.go +++ b/pkg/scalers/aws_cloudwatch_scaler.go @@ -42,6 +42,7 @@ type awsCloudwatchMetadata struct { targetMetricValue float64 activationTargetMetricValue float64 minMetricValue float64 + errorWhenMetricValuesEmpty bool metricCollectionTime int64 metricStat string @@ -113,6 +114,22 @@ func getFloatMetadataValue(metadata map[string]string, key string, required bool return defaultValue, nil } +func getBoolMetadataValue(metadata map[string]string, key string, required bool, defaultValue bool) (bool, error) { + if val, ok := metadata[key]; ok && val != "" { + value, err := strconv.ParseBool(val) + if err != nil { + return false, fmt.Errorf("error parsing %s metadata: %w", key, err) + } + return value, nil + } + + if required { + return false, fmt.Errorf("metadata %s not given", key) + } + + return defaultValue, nil +} + func createCloudwatchClient(ctx context.Context, metadata *awsCloudwatchMetadata) (*cloudwatch.Client, error) { cfg, err := awsutils.GetAwsConfig(ctx, metadata.awsRegion, metadata.awsAuthorization) @@ -189,6 +206,12 @@ func parseAwsCloudwatchMetadata(config *scalersconfig.ScalerConfig) (*awsCloudwa } meta.minMetricValue = minMetricValue + errorWhenMetricValuesEmpty, err := getBoolMetadataValue(config.TriggerMetadata, "errorWhenMetricValuesEmpty", false, false) + if err != nil { + return nil, err + } + meta.errorWhenMetricValuesEmpty = errorWhenMetricValuesEmpty + meta.metricStat = defaultMetricStat if val, ok := config.TriggerMetadata["metricStat"]; ok && val != "" { meta.metricStat = val @@ -383,6 +406,14 @@ func (s *awsCloudwatchScaler) GetCloudwatchMetrics(ctx context.Context) (float64 s.logger.V(1).Info("Received Metric Data", "data", output) var metricValue float64 + + // If no metric data is received and errorWhenMetricValuesEmpty is set to true, return error, + // otherwise continue with either the metric value recieved, or the minMetricValue + if len(output.MetricDataResults) == 0 && s.metadata.errorWhenMetricValuesEmpty { + s.logger.Error(err, "empty metric data received and errorWhenMetricValuesEmpty is set to true") + return -1, fmt.Errorf("empty metric data received and errorWhenMetricValuesEmpty is set to true") + } + if len(output.MetricDataResults) > 0 && len(output.MetricDataResults[0].Values) > 0 { metricValue = output.MetricDataResults[0].Values[0] } else { diff --git a/pkg/scalers/aws_cloudwatch_scaler_test.go b/pkg/scalers/aws_cloudwatch_scaler_test.go index 0564f205b88..9dff5245011 100644 --- a/pkg/scalers/aws_cloudwatch_scaler_test.go +++ b/pkg/scalers/aws_cloudwatch_scaler_test.go @@ -3,6 +3,7 @@ package scalers import ( "context" "errors" + "strings" "testing" "time" @@ -356,6 +357,48 @@ var testAWSCloudwatchMetadata = []parseAWSCloudwatchMetadataTestData{ "awsRegion": "eu-west-1"}, testAWSAuthentication, true, "unsupported metricUnit"}, + // test errorWhenMetricValuesEmpty is false + {map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStatPeriod": "60", + "metricStat": "Average", + "errorWhenMetricValuesEmpty": "false", + "awsRegion": "eu-west-1"}, + testAWSAuthentication, false, + "with errorWhenMetricValuesEmpty set to false"}, + // test errorWhenMetricValuesEmpty is true + {map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStatPeriod": "60", + "metricStat": "Average", + "errorWhenMetricValuesEmpty": "true", + "awsRegion": "eu-west-1"}, + testAWSAuthentication, false, + "with errorWhenMetricValuesEmpty set to true"}, + // test errorWhenMetricValuesEmpty is incorrect + {map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStatPeriod": "60", + "metricStat": "Average", + "errorWhenMetricValuesEmpty": "maybe", + "awsRegion": "eu-west-1"}, + testAWSAuthentication, true, + "unsupported value for errorWhenMetricValuesEmpty"}, } var awsCloudwatchMetricIdentifiers = []awsCloudwatchMetricIdentifier{ @@ -444,6 +487,42 @@ var awsCloudwatchGetMetricTestData = []awsCloudwatchMetadata{ awsAuthorization: awsutils.AuthorizationMetadata{PodIdentityOwner: false}, triggerIndex: 0, }, + // Test for metric with no data + { + namespace: "Custom", + metricsName: testAWSCloudwatchNoValueMetric, + dimensionName: []string{"DIM"}, + dimensionValue: []string{"DIM_VALUE"}, + targetMetricValue: 100, + minMetricValue: 0, + errorWhenMetricValuesEmpty: false, + metricCollectionTime: 60, + metricStat: "Average", + metricUnit: "", + metricStatPeriod: 60, + metricEndTimeOffset: 60, + awsRegion: "us-west-2", + awsAuthorization: awsutils.AuthorizationMetadata{PodIdentityOwner: false}, + triggerIndex: 0, + }, + // Test for metric with no data, and the scaler errors when metric data values are empty + { + namespace: "Custom", + metricsName: testAWSCloudwatchNoValueMetric, + dimensionName: []string{"DIM"}, + dimensionValue: []string{"DIM_VALUE"}, + targetMetricValue: 100, + minMetricValue: 0, + errorWhenMetricValuesEmpty: true, + metricCollectionTime: 60, + metricStat: "Average", + metricUnit: "", + metricStatPeriod: 60, + metricEndTimeOffset: 60, + awsRegion: "us-west-2", + awsAuthorization: awsutils.AuthorizationMetadata{PodIdentityOwner: false}, + triggerIndex: 0, + }, } type mockCloudwatch struct { @@ -507,7 +586,12 @@ func TestAWSCloudwatchScalerGetMetrics(t *testing.T) { case testAWSCloudwatchErrorMetric: assert.Error(t, err, "expect error because of cloudwatch api error") case testAWSCloudwatchNoValueMetric: - assert.NoError(t, err, "dont expect error when returning empty metric list from cloudwatch") + // if errorWhenMetricValuesEmpty is defined, then an error is expected + if meta.errorWhenMetricValuesEmpty { + assert.Error(t, err, "expected an error when returning empty metric list from cloudwatch") + } else { + assert.NoError(t, err, "dont expect error when returning empty metric list from cloudwatch") + } default: assert.EqualValues(t, int64(10.0), value[0].Value.Value()) } @@ -556,3 +640,77 @@ func TestComputeQueryWindow(t *testing.T) { assert.Equal(t, testData.expectedEndTime, endTime.UTC().Format(time.RFC3339Nano), "unexpected endTime", "name", testData.name) } } + +func TestGetBoolMetadataValue(t *testing.T) { + testCases := []struct { + name string + metadata map[string]string + key string + required bool + defaultValue bool + expectedValue bool + expectedError string + }{ + { + name: "valid true value", + metadata: map[string]string{"key": "true"}, + key: "key", + required: true, + defaultValue: false, + expectedValue: true, + expectedError: "", + }, + { + name: "valid false value", + metadata: map[string]string{"key": "false"}, + key: "key", + required: true, + defaultValue: true, + expectedValue: false, + expectedError: "", + }, + { + name: "invalid value", + metadata: map[string]string{"key": "invalid"}, + key: "key", + required: true, + defaultValue: false, + expectedValue: false, + expectedError: "error parsing key metadata: strconv.ParseBool: parsing \"invalid\": invalid syntax", + }, + { + name: "key not present, required", + metadata: map[string]string{}, + key: "key", + required: true, + defaultValue: false, + expectedValue: false, + expectedError: "metadata key not given", + }, + { + name: "key not present, not required", + metadata: map[string]string{}, + key: "key", + required: false, + defaultValue: true, + expectedValue: true, + expectedError: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + value, err := getBoolMetadataValue(tc.metadata, tc.key, tc.required, tc.defaultValue) + + if tc.expectedError == "" && err != nil { + t.Errorf("unexpected error: %v", err) + } else if tc.expectedError != "" && (err == nil || !strings.Contains(err.Error(), tc.expectedError)) { + t.Errorf("expected error containing %q, got %v", tc.expectedError, err) + } + + if value != tc.expectedValue { + t.Errorf("expected value %v, got %v", tc.expectedValue, value) + } + }) + } +} From 4174e93d8685897b45385e1f60566f61b1a92cca Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Thu, 28 Mar 2024 19:09:09 +0000 Subject: [PATCH 02/23] fix golangci-lint Signed-off-by: Rob Pickerill --- pkg/scalers/aws_cloudwatch_scaler.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/pkg/scalers/aws_cloudwatch_scaler.go b/pkg/scalers/aws_cloudwatch_scaler.go index 13014cd26d2..38e42d10593 100644 --- a/pkg/scalers/aws_cloudwatch_scaler.go +++ b/pkg/scalers/aws_cloudwatch_scaler.go @@ -236,8 +236,8 @@ func parseAwsCloudwatchMetadata(config *scalersconfig.ScalerConfig) (*awsCloudwa } meta.metricCollectionTime = metricCollectionTime - if meta.metricCollectionTime < 0 || meta.metricCollectionTime%meta.metricStatPeriod != 0 { - return nil, fmt.Errorf("metricCollectionTime must be greater than 0 and a multiple of metricStatPeriod(%d), %d is given", meta.metricStatPeriod, meta.metricCollectionTime) + if err = checkMetricCollectionTime(meta.metricCollectionTime, meta.metricStatPeriod); err != nil { + return nil, err } metricEndTimeOffset, err := getIntMetadataValue(config.TriggerMetadata, "metricEndTimeOffset", false, defaultMetricEndTimeOffset) @@ -307,6 +307,14 @@ func checkMetricStatPeriod(period int64) error { return nil } +func checkMetricCollectionTime(metricCollectionTime, metricStatPeriod int64) error { + if metricCollectionTime < 0 || metricCollectionTime%metricStatPeriod != 0 { + return fmt.Errorf("metricCollectionTime must be greater than 0 and a multiple of metricStatPeriod(%d), %d is given", metricStatPeriod, metricCollectionTime) + } + + return nil +} + func computeQueryWindow(current time.Time, metricPeriodSec, metricEndTimeOffsetSec, metricCollectionTimeSec int64) (startTime, endTime time.Time) { endTime = current.Add(time.Second * -1 * time.Duration(metricEndTimeOffsetSec)).Truncate(time.Duration(metricPeriodSec) * time.Second) startTime = endTime.Add(time.Second * -1 * time.Duration(metricCollectionTimeSec)) @@ -408,7 +416,7 @@ func (s *awsCloudwatchScaler) GetCloudwatchMetrics(ctx context.Context) (float64 var metricValue float64 // If no metric data is received and errorWhenMetricValuesEmpty is set to true, return error, - // otherwise continue with either the metric value recieved, or the minMetricValue + // otherwise continue with either the metric value received, or the minMetricValue if len(output.MetricDataResults) == 0 && s.metadata.errorWhenMetricValuesEmpty { s.logger.Error(err, "empty metric data received and errorWhenMetricValuesEmpty is set to true") return -1, fmt.Errorf("empty metric data received and errorWhenMetricValuesEmpty is set to true") From 8e41311c2716ca519c99e8e094485ce0dd66f2c2 Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Fri, 29 Mar 2024 16:32:47 +0000 Subject: [PATCH 03/23] improve error message for empty results Signed-off-by: Rob Pickerill --- pkg/scalers/aws_cloudwatch_scaler.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/scalers/aws_cloudwatch_scaler.go b/pkg/scalers/aws_cloudwatch_scaler.go index 38e42d10593..97d84c77439 100644 --- a/pkg/scalers/aws_cloudwatch_scaler.go +++ b/pkg/scalers/aws_cloudwatch_scaler.go @@ -418,8 +418,9 @@ func (s *awsCloudwatchScaler) GetCloudwatchMetrics(ctx context.Context) (float64 // If no metric data is received and errorWhenMetricValuesEmpty is set to true, return error, // otherwise continue with either the metric value received, or the minMetricValue if len(output.MetricDataResults) == 0 && s.metadata.errorWhenMetricValuesEmpty { - s.logger.Error(err, "empty metric data received and errorWhenMetricValuesEmpty is set to true") - return -1, fmt.Errorf("empty metric data received and errorWhenMetricValuesEmpty is set to true") + emptyMetricsErrMessage := "empty result of metric values received, and errorWhenMetricValuesEmpty is set to true" + s.logger.Error(err, emptyMetricsErrMessage) + return -1, fmt.Errorf(emptyMetricsErrMessage) } if len(output.MetricDataResults) > 0 && len(output.MetricDataResults[0].Values) > 0 { From b0d4e84371043fcc51ab1f9f15d129f9f3b95e76 Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Fri, 29 Mar 2024 18:38:37 +0000 Subject: [PATCH 04/23] add error when empty metric values to changelog Signed-off-by: Rob Pickerill --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0fb04c9e6e..000bd3a2273 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,7 @@ Here is an overview of all new **experimental** features: - **General**: Add GRPC Healthchecks ([#5590](https://github.com/kedacore/keda/issues/5590)) - **General**: Add OPENTELEMETRY flag in e2e test YAML ([#5375](https://github.com/kedacore/keda/issues/5375)) - **General**: Add support for cross tenant/cloud authentication when using Azure Workload Identity for TriggerAuthentication ([#5441](https://github.com/kedacore/keda/issues/5441)) +- **AWS CloudWatch Scaler**: Add support for errorWhenMetricValuesEmpty ([#5352](https://github.com/kedacore/keda/issues/5352)) - **Metrics API Scaler**: Add support for various formats: json, xml, yaml, prometheus ([#2633](https://github.com/kedacore/keda/issues/2633)) - **MongoDB Scaler**: Add scheme field support srv record ([#5544](https://github.com/kedacore/keda/issues/5544)) From 7a5f617925a3578c2ef92b5037b29cfad994812a Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Sun, 31 Mar 2024 16:39:38 +0100 Subject: [PATCH 05/23] rename errorWhenMetricValuesEmpty -> errorWhenNullValues Signed-off-by: Rob Pickerill --- pkg/scalers/aws_cloudwatch_scaler.go | 10 +- pkg/scalers/aws_cloudwatch_scaler_test.go | 134 +++++++++++----------- 2 files changed, 72 insertions(+), 72 deletions(-) diff --git a/pkg/scalers/aws_cloudwatch_scaler.go b/pkg/scalers/aws_cloudwatch_scaler.go index 97d84c77439..264061d3061 100644 --- a/pkg/scalers/aws_cloudwatch_scaler.go +++ b/pkg/scalers/aws_cloudwatch_scaler.go @@ -42,7 +42,7 @@ type awsCloudwatchMetadata struct { targetMetricValue float64 activationTargetMetricValue float64 minMetricValue float64 - errorWhenMetricValuesEmpty bool + errorWhenNullValues bool metricCollectionTime int64 metricStat string @@ -206,11 +206,11 @@ func parseAwsCloudwatchMetadata(config *scalersconfig.ScalerConfig) (*awsCloudwa } meta.minMetricValue = minMetricValue - errorWhenMetricValuesEmpty, err := getBoolMetadataValue(config.TriggerMetadata, "errorWhenMetricValuesEmpty", false, false) + errorWhenNullValues, err := getBoolMetadataValue(config.TriggerMetadata, "errorWhenNullValues", false, false) if err != nil { return nil, err } - meta.errorWhenMetricValuesEmpty = errorWhenMetricValuesEmpty + meta.errorWhenNullValues = errorWhenNullValues meta.metricStat = defaultMetricStat if val, ok := config.TriggerMetadata["metricStat"]; ok && val != "" { @@ -417,8 +417,8 @@ func (s *awsCloudwatchScaler) GetCloudwatchMetrics(ctx context.Context) (float64 // If no metric data is received and errorWhenMetricValuesEmpty is set to true, return error, // otherwise continue with either the metric value received, or the minMetricValue - if len(output.MetricDataResults) == 0 && s.metadata.errorWhenMetricValuesEmpty { - emptyMetricsErrMessage := "empty result of metric values received, and errorWhenMetricValuesEmpty is set to true" + if len(output.MetricDataResults) == 0 && s.metadata.errorWhenNullValues { + emptyMetricsErrMessage := "empty result of metric values received, and errorWhenNullValues is set to true" s.logger.Error(err, emptyMetricsErrMessage) return -1, fmt.Errorf(emptyMetricsErrMessage) } diff --git a/pkg/scalers/aws_cloudwatch_scaler_test.go b/pkg/scalers/aws_cloudwatch_scaler_test.go index 9dff5245011..1475dbd3b17 100644 --- a/pkg/scalers/aws_cloudwatch_scaler_test.go +++ b/pkg/scalers/aws_cloudwatch_scaler_test.go @@ -357,48 +357,48 @@ var testAWSCloudwatchMetadata = []parseAWSCloudwatchMetadataTestData{ "awsRegion": "eu-west-1"}, testAWSAuthentication, true, "unsupported metricUnit"}, - // test errorWhenMetricValuesEmpty is false + // test errorWhenNullValues is false {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricStatPeriod": "60", - "metricStat": "Average", - "errorWhenMetricValuesEmpty": "false", - "awsRegion": "eu-west-1"}, + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStatPeriod": "60", + "metricStat": "Average", + "errorWhenNullValues": "false", + "awsRegion": "eu-west-1"}, testAWSAuthentication, false, - "with errorWhenMetricValuesEmpty set to false"}, - // test errorWhenMetricValuesEmpty is true + "with errorWhenNullValues set to false"}, + // test errorWhenNullValues is true {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricStatPeriod": "60", - "metricStat": "Average", - "errorWhenMetricValuesEmpty": "true", - "awsRegion": "eu-west-1"}, + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStatPeriod": "60", + "metricStat": "Average", + "errorWhenNullValues": "true", + "awsRegion": "eu-west-1"}, testAWSAuthentication, false, - "with errorWhenMetricValuesEmpty set to true"}, - // test errorWhenMetricValuesEmpty is incorrect + "with errorWhenNullValues set to true"}, + // test errorWhenNullValues is incorrect {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricStatPeriod": "60", - "metricStat": "Average", - "errorWhenMetricValuesEmpty": "maybe", - "awsRegion": "eu-west-1"}, + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStatPeriod": "60", + "metricStat": "Average", + "errorWhenNullValues": "maybe", + "awsRegion": "eu-west-1"}, testAWSAuthentication, true, - "unsupported value for errorWhenMetricValuesEmpty"}, + "unsupported value for errorWhenNullValues"}, } var awsCloudwatchMetricIdentifiers = []awsCloudwatchMetricIdentifier{ @@ -489,39 +489,39 @@ var awsCloudwatchGetMetricTestData = []awsCloudwatchMetadata{ }, // Test for metric with no data { - namespace: "Custom", - metricsName: testAWSCloudwatchNoValueMetric, - dimensionName: []string{"DIM"}, - dimensionValue: []string{"DIM_VALUE"}, - targetMetricValue: 100, - minMetricValue: 0, - errorWhenMetricValuesEmpty: false, - metricCollectionTime: 60, - metricStat: "Average", - metricUnit: "", - metricStatPeriod: 60, - metricEndTimeOffset: 60, - awsRegion: "us-west-2", - awsAuthorization: awsutils.AuthorizationMetadata{PodIdentityOwner: false}, - triggerIndex: 0, + namespace: "Custom", + metricsName: testAWSCloudwatchNoValueMetric, + dimensionName: []string{"DIM"}, + dimensionValue: []string{"DIM_VALUE"}, + targetMetricValue: 100, + minMetricValue: 0, + errorWhenNullValues: false, + metricCollectionTime: 60, + metricStat: "Average", + metricUnit: "", + metricStatPeriod: 60, + metricEndTimeOffset: 60, + awsRegion: "us-west-2", + awsAuthorization: awsutils.AuthorizationMetadata{PodIdentityOwner: false}, + triggerIndex: 0, }, // Test for metric with no data, and the scaler errors when metric data values are empty { - namespace: "Custom", - metricsName: testAWSCloudwatchNoValueMetric, - dimensionName: []string{"DIM"}, - dimensionValue: []string{"DIM_VALUE"}, - targetMetricValue: 100, - minMetricValue: 0, - errorWhenMetricValuesEmpty: true, - metricCollectionTime: 60, - metricStat: "Average", - metricUnit: "", - metricStatPeriod: 60, - metricEndTimeOffset: 60, - awsRegion: "us-west-2", - awsAuthorization: awsutils.AuthorizationMetadata{PodIdentityOwner: false}, - triggerIndex: 0, + namespace: "Custom", + metricsName: testAWSCloudwatchNoValueMetric, + dimensionName: []string{"DIM"}, + dimensionValue: []string{"DIM_VALUE"}, + targetMetricValue: 100, + minMetricValue: 0, + errorWhenNullValues: true, + metricCollectionTime: 60, + metricStat: "Average", + metricUnit: "", + metricStatPeriod: 60, + metricEndTimeOffset: 60, + awsRegion: "us-west-2", + awsAuthorization: awsutils.AuthorizationMetadata{PodIdentityOwner: false}, + triggerIndex: 0, }, } @@ -587,7 +587,7 @@ func TestAWSCloudwatchScalerGetMetrics(t *testing.T) { assert.Error(t, err, "expect error because of cloudwatch api error") case testAWSCloudwatchNoValueMetric: // if errorWhenMetricValuesEmpty is defined, then an error is expected - if meta.errorWhenMetricValuesEmpty { + if meta.errorWhenNullValues { assert.Error(t, err, "expected an error when returning empty metric list from cloudwatch") } else { assert.NoError(t, err, "dont expect error when returning empty metric list from cloudwatch") From 96a2254c7eaedac394ebbc90633f37bfdac3fb72 Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Sun, 31 Mar 2024 16:56:42 +0100 Subject: [PATCH 06/23] use getParameterFromConfigV2 to read config for errorWhenNullValues Signed-off-by: Rob Pickerill --- pkg/scalers/aws_cloudwatch_scaler.go | 23 ++----- pkg/scalers/aws_cloudwatch_scaler_test.go | 77 +---------------------- 2 files changed, 5 insertions(+), 95 deletions(-) diff --git a/pkg/scalers/aws_cloudwatch_scaler.go b/pkg/scalers/aws_cloudwatch_scaler.go index 264061d3061..b9ce7e3ddda 100644 --- a/pkg/scalers/aws_cloudwatch_scaler.go +++ b/pkg/scalers/aws_cloudwatch_scaler.go @@ -3,6 +3,7 @@ package scalers import ( "context" "fmt" + "reflect" "strconv" "strings" "time" @@ -114,22 +115,6 @@ func getFloatMetadataValue(metadata map[string]string, key string, required bool return defaultValue, nil } -func getBoolMetadataValue(metadata map[string]string, key string, required bool, defaultValue bool) (bool, error) { - if val, ok := metadata[key]; ok && val != "" { - value, err := strconv.ParseBool(val) - if err != nil { - return false, fmt.Errorf("error parsing %s metadata: %w", key, err) - } - return value, nil - } - - if required { - return false, fmt.Errorf("metadata %s not given", key) - } - - return defaultValue, nil -} - func createCloudwatchClient(ctx context.Context, metadata *awsCloudwatchMetadata) (*cloudwatch.Client, error) { cfg, err := awsutils.GetAwsConfig(ctx, metadata.awsRegion, metadata.awsAuthorization) @@ -206,11 +191,11 @@ func parseAwsCloudwatchMetadata(config *scalersconfig.ScalerConfig) (*awsCloudwa } meta.minMetricValue = minMetricValue - errorWhenNullValues, err := getBoolMetadataValue(config.TriggerMetadata, "errorWhenNullValues", false, false) + errorWhenNullValues, err := getParameterFromConfigV2(config, "errorWhenNullValues", reflect.TypeOf(false), IsOptional(true), UseMetadata(true), WithDefaultVal(false)) if err != nil { return nil, err } - meta.errorWhenNullValues = errorWhenNullValues + meta.errorWhenNullValues, _ = errorWhenNullValues.(bool) meta.metricStat = defaultMetricStat if val, ok := config.TriggerMetadata["metricStat"]; ok && val != "" { @@ -415,7 +400,7 @@ func (s *awsCloudwatchScaler) GetCloudwatchMetrics(ctx context.Context) (float64 s.logger.V(1).Info("Received Metric Data", "data", output) var metricValue float64 - // If no metric data is received and errorWhenMetricValuesEmpty is set to true, return error, + // If no metric data is received and errorWhenNullValues is set to true, return error, // otherwise continue with either the metric value received, or the minMetricValue if len(output.MetricDataResults) == 0 && s.metadata.errorWhenNullValues { emptyMetricsErrMessage := "empty result of metric values received, and errorWhenNullValues is set to true" diff --git a/pkg/scalers/aws_cloudwatch_scaler_test.go b/pkg/scalers/aws_cloudwatch_scaler_test.go index 1475dbd3b17..473318a8250 100644 --- a/pkg/scalers/aws_cloudwatch_scaler_test.go +++ b/pkg/scalers/aws_cloudwatch_scaler_test.go @@ -3,7 +3,6 @@ package scalers import ( "context" "errors" - "strings" "testing" "time" @@ -586,7 +585,7 @@ func TestAWSCloudwatchScalerGetMetrics(t *testing.T) { case testAWSCloudwatchErrorMetric: assert.Error(t, err, "expect error because of cloudwatch api error") case testAWSCloudwatchNoValueMetric: - // if errorWhenMetricValuesEmpty is defined, then an error is expected + // if errorWhenNullValues is defined, then an error is expected if meta.errorWhenNullValues { assert.Error(t, err, "expected an error when returning empty metric list from cloudwatch") } else { @@ -640,77 +639,3 @@ func TestComputeQueryWindow(t *testing.T) { assert.Equal(t, testData.expectedEndTime, endTime.UTC().Format(time.RFC3339Nano), "unexpected endTime", "name", testData.name) } } - -func TestGetBoolMetadataValue(t *testing.T) { - testCases := []struct { - name string - metadata map[string]string - key string - required bool - defaultValue bool - expectedValue bool - expectedError string - }{ - { - name: "valid true value", - metadata: map[string]string{"key": "true"}, - key: "key", - required: true, - defaultValue: false, - expectedValue: true, - expectedError: "", - }, - { - name: "valid false value", - metadata: map[string]string{"key": "false"}, - key: "key", - required: true, - defaultValue: true, - expectedValue: false, - expectedError: "", - }, - { - name: "invalid value", - metadata: map[string]string{"key": "invalid"}, - key: "key", - required: true, - defaultValue: false, - expectedValue: false, - expectedError: "error parsing key metadata: strconv.ParseBool: parsing \"invalid\": invalid syntax", - }, - { - name: "key not present, required", - metadata: map[string]string{}, - key: "key", - required: true, - defaultValue: false, - expectedValue: false, - expectedError: "metadata key not given", - }, - { - name: "key not present, not required", - metadata: map[string]string{}, - key: "key", - required: false, - defaultValue: true, - expectedValue: true, - expectedError: "", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - value, err := getBoolMetadataValue(tc.metadata, tc.key, tc.required, tc.defaultValue) - - if tc.expectedError == "" && err != nil { - t.Errorf("unexpected error: %v", err) - } else if tc.expectedError != "" && (err == nil || !strings.Contains(err.Error(), tc.expectedError)) { - t.Errorf("expected error containing %q, got %v", tc.expectedError, err) - } - - if value != tc.expectedValue { - t.Errorf("expected value %v, got %v", tc.expectedValue, value) - } - }) - } -} From 0b61f9740343b4567cb5c6ba4f897b7d6522c271 Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Wed, 3 Apr 2024 20:02:01 +0100 Subject: [PATCH 07/23] add e2e for error state for cw, and improve e2e for min values for cw Signed-off-by: Rob Pickerill --- pkg/scalers/aws_cloudwatch_scaler.go | 12 +- pkg/scalers/aws_cloudwatch_scaler_test.go | 674 ++++++++++-------- tests/helper/helper.go | 62 +- .../aws_cloudwatch_error_null_values_test.go | 245 +++++++ ...s_cloudwatch_min_value_null_values_test.go | 243 +++++++ 5 files changed, 925 insertions(+), 311 deletions(-) create mode 100644 tests/scalers/aws/aws_cloudwatch_error_null_values/aws_cloudwatch_error_null_values_test.go create mode 100644 tests/scalers/aws/aws_cloudwatch_min_value_null_values/aws_cloudwatch_min_value_null_values_test.go diff --git a/pkg/scalers/aws_cloudwatch_scaler.go b/pkg/scalers/aws_cloudwatch_scaler.go index b9ce7e3ddda..9a78a9d8924 100644 --- a/pkg/scalers/aws_cloudwatch_scaler.go +++ b/pkg/scalers/aws_cloudwatch_scaler.go @@ -117,7 +117,6 @@ func getFloatMetadataValue(metadata map[string]string, key string, required bool func createCloudwatchClient(ctx context.Context, metadata *awsCloudwatchMetadata) (*cloudwatch.Client, error) { cfg, err := awsutils.GetAwsConfig(ctx, metadata.awsRegion, metadata.awsAuthorization) - if err != nil { return nil, err } @@ -308,7 +307,6 @@ func computeQueryWindow(current time.Time, metricPeriodSec, metricEndTimeOffsetS func (s *awsCloudwatchScaler) GetMetricsAndActivity(ctx context.Context, metricName string) ([]external_metrics.ExternalMetricValue, bool, error) { metricValue, err := s.GetCloudwatchMetrics(ctx) - if err != nil { s.logger.Error(err, "Error getting metric value") return []external_metrics.ExternalMetricValue{}, false, err @@ -391,7 +389,6 @@ func (s *awsCloudwatchScaler) GetCloudwatchMetrics(ctx context.Context) (float64 } output, err := s.cwClient.GetMetricData(ctx, &input) - if err != nil { s.logger.Error(err, "Failed to get output") return -1, err @@ -400,10 +397,11 @@ func (s *awsCloudwatchScaler) GetCloudwatchMetrics(ctx context.Context) (float64 s.logger.V(1).Info("Received Metric Data", "data", output) var metricValue float64 - // If no metric data is received and errorWhenNullValues is set to true, return error, - // otherwise continue with either the metric value received, or the minMetricValue - if len(output.MetricDataResults) == 0 && s.metadata.errorWhenNullValues { - emptyMetricsErrMessage := "empty result of metric values received, and errorWhenNullValues is set to true" + fmt.Printf("output.MetricDataResults: %+v", output.MetricDataResults) + + // If no metric data is received and errorWhenNullValues is set to true, return an error + if len(output.MetricDataResults) > 0 && len(output.MetricDataResults[0].Values) == 0 && s.metadata.errorWhenNullValues { + emptyMetricsErrMessage := "empty metric data received, and errorWhenNullValues is set to true, returning error" s.logger.Error(err, emptyMetricsErrMessage) return -1, fmt.Errorf(emptyMetricsErrMessage) } diff --git a/pkg/scalers/aws_cloudwatch_scaler_test.go b/pkg/scalers/aws_cloudwatch_scaler_test.go index 473318a8250..99149541d50 100644 --- a/pkg/scalers/aws_cloudwatch_scaler_test.go +++ b/pkg/scalers/aws_cloudwatch_scaler_test.go @@ -22,6 +22,7 @@ const ( testAWSCloudwatchSessionToken = "none" testAWSCloudwatchErrorMetric = "Error" testAWSCloudwatchNoValueMetric = "NoValue" + testAWSCloudwatchEmptyValues = "EmptyValues" ) var testAWSCloudwatchResolvedEnv = map[string]string{ @@ -50,354 +51,433 @@ type awsCloudwatchMetricIdentifier struct { var testAWSCloudwatchMetadata = []parseAWSCloudwatchMetadataTestData{ {map[string]string{}, testAWSAuthentication, true, "Empty structures"}, // properly formed cloudwatch query and awsRegion - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "activationTargetMetricValue": "0", - "minMetricValue": "0", - "awsRegion": "eu-west-1"}, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "activationTargetMetricValue": "0", + "minMetricValue": "0", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, false, - "properly formed cloudwatch query and awsRegion"}, + "properly formed cloudwatch query and awsRegion", + }, // properly formed cloudwatch expression query and awsRegion - {map[string]string{ - "namespace": "AWS/SQS", - "expression": "SELECT MIN(MessageCount) FROM \"AWS/AmazonMQ\" WHERE Broker = 'production' and Queue = 'worker'", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "activationTargetMetricValue": "0", - "minMetricValue": "0", - "awsRegion": "eu-west-1"}, + { + map[string]string{ + "namespace": "AWS/SQS", + "expression": "SELECT MIN(MessageCount) FROM \"AWS/AmazonMQ\" WHERE Broker = 'production' and Queue = 'worker'", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "activationTargetMetricValue": "0", + "minMetricValue": "0", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, false, - "properly formed cloudwatch expression query and awsRegion"}, + "properly formed cloudwatch expression query and awsRegion", + }, // Properly formed cloudwatch query with optional parameters - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "activationTargetMetricValue": "0", - "minMetricValue": "0", - "metricCollectionTime": "300", - "metricStat": "Average", - "metricStatPeriod": "300", - "awsRegion": "eu-west-1", - "awsEndpoint": "http://localhost:4566"}, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "activationTargetMetricValue": "0", + "minMetricValue": "0", + "metricCollectionTime": "300", + "metricStat": "Average", + "metricStatPeriod": "300", + "awsRegion": "eu-west-1", + "awsEndpoint": "http://localhost:4566", + }, testAWSAuthentication, false, - "Properly formed cloudwatch query with optional parameters"}, + "Properly formed cloudwatch query with optional parameters", + }, // properly formed cloudwatch query but Region is empty - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "activationTargetMetricValue": "0", - "minMetricValue": "0", - "awsRegion": ""}, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "activationTargetMetricValue": "0", + "minMetricValue": "0", + "awsRegion": "", + }, testAWSAuthentication, true, - "properly formed cloudwatch query but Region is empty"}, + "properly formed cloudwatch query but Region is empty", + }, // Missing namespace - {map[string]string{"dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "awsRegion": "eu-west-1"}, + { + map[string]string{ + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, true, - "Missing namespace"}, + "Missing namespace", + }, // Missing dimensionName - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "awsRegion": "eu-west-1"}, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, true, - "Missing dimensionName"}, + "Missing dimensionName", + }, // Missing dimensionValue - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "awsRegion": "eu-west-1"}, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, true, - "Missing dimensionValue"}, + "Missing dimensionValue", + }, // Missing metricName - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "targetMetricValue": "2", - "minMetricValue": "0", - "awsRegion": "eu-west-1"}, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "targetMetricValue": "2", + "minMetricValue": "0", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, true, - "Missing metricName"}, + "Missing metricName", + }, // with static "aws_credentials" from TriggerAuthentication - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricCollectionTime": "300", - "metricStat": "Average", - "metricStatPeriod": "300", - "awsRegion": "eu-west-1"}, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricCollectionTime": "300", + "metricStat": "Average", + "metricStatPeriod": "300", + "awsRegion": "eu-west-1", + }, map[string]string{ "awsAccessKeyId": testAWSCloudwatchAccessKeyID, "awsSecretAccessKey": testAWSCloudwatchSecretAccessKey, }, false, - "with AWS Credentials from TriggerAuthentication"}, + "with AWS Credentials from TriggerAuthentication", + }, // with temporary "aws_credentials" from TriggerAuthentication - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricCollectionTime": "300", - "metricStat": "Average", - "metricStatPeriod": "300", - "awsRegion": "eu-west-1"}, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricCollectionTime": "300", + "metricStat": "Average", + "metricStatPeriod": "300", + "awsRegion": "eu-west-1", + }, map[string]string{ "awsAccessKeyId": testAWSCloudwatchAccessKeyID, "awsSecretAccessKey": testAWSCloudwatchSecretAccessKey, "awsSessionToken": testAWSCloudwatchSessionToken, }, false, - "with AWS Credentials from TriggerAuthentication"}, + "with AWS Credentials from TriggerAuthentication", + }, // with "aws_role" from TriggerAuthentication - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricCollectionTime": "300", - "metricStat": "Average", - "metricStatPeriod": "300", - "awsRegion": "eu-west-1"}, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricCollectionTime": "300", + "metricStat": "Average", + "metricStatPeriod": "300", + "awsRegion": "eu-west-1", + }, map[string]string{ "awsRoleArn": testAWSCloudwatchRoleArn, }, false, - "with AWS Role from TriggerAuthentication"}, - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricCollectionTime": "300", - "metricStat": "Average", - "metricStatPeriod": "300", - "awsRegion": "eu-west-1", - "identityOwner": "operator"}, + "with AWS Role from TriggerAuthentication", + }, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricCollectionTime": "300", + "metricStat": "Average", + "metricStatPeriod": "300", + "awsRegion": "eu-west-1", + "identityOwner": "operator", + }, map[string]string{}, false, - "with AWS Role assigned on KEDA operator itself"}, - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricCollectionTime": "a", - "metricStat": "Average", - "metricStatPeriod": "300", - "awsRegion": "eu-west-1", - "identityOwner": "operator"}, + "with AWS Role assigned on KEDA operator itself", + }, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricCollectionTime": "a", + "metricStat": "Average", + "metricStatPeriod": "300", + "awsRegion": "eu-west-1", + "identityOwner": "operator", + }, map[string]string{}, true, - "if metricCollectionTime assigned with a string, need to be a number"}, - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricCollectionTime": "300", - "metricStat": "Average", - "metricStatPeriod": "a", - "awsRegion": "eu-west-1", - "identityOwner": "operator"}, + "if metricCollectionTime assigned with a string, need to be a number", + }, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricCollectionTime": "300", + "metricStat": "Average", + "metricStatPeriod": "a", + "awsRegion": "eu-west-1", + "identityOwner": "operator", + }, map[string]string{}, true, - "if metricStatPeriod assigned with a string, need to be a number"}, - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricStat": "Average", - "metricStatPeriod": "300", - "awsRegion": "eu-west-1"}, + "if metricStatPeriod assigned with a string, need to be a number", + }, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStat": "Average", + "metricStatPeriod": "300", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, false, - "Missing metricCollectionTime not generate error because will get the default value"}, - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricCollectionTime": "300", - "metricStatPeriod": "300", - "awsRegion": "eu-west-1"}, + "Missing metricCollectionTime not generate error because will get the default value", + }, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricCollectionTime": "300", + "metricStatPeriod": "300", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, false, - "Missing metricStat not generate error because will get the default value"}, - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricCollectionTime": "300", - "metricStat": "Average", - "awsRegion": "eu-west-1"}, + "Missing metricStat not generate error because will get the default value", + }, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricCollectionTime": "300", + "metricStat": "Average", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, false, - "Missing metricStatPeriod not generate error because will get the default value"}, - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricStat": "Average", - "metricUnit": "Count", - "metricEndTimeOffset": "60", - "awsRegion": "eu-west-1"}, + "Missing metricStatPeriod not generate error because will get the default value", + }, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStat": "Average", + "metricUnit": "Count", + "metricEndTimeOffset": "60", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, false, - "set a supported metricUnit"}, - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricCollectionTime": "300", - "metricStat": "SomeStat", - "awsRegion": "eu-west-1"}, + "set a supported metricUnit", + }, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricCollectionTime": "300", + "metricStat": "SomeStat", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, true, - "metricStat is not supported"}, - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricStatPeriod": "300", - "metricCollectionTime": "100", - "metricStat": "Average", - "awsRegion": "eu-west-1"}, + "metricStat is not supported", + }, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStatPeriod": "300", + "metricCollectionTime": "100", + "metricStat": "Average", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, true, - "metricCollectionTime smaller than metricStatPeriod"}, - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricStatPeriod": "250", - "metricStat": "Average", - "awsRegion": "eu-west-1"}, + "metricCollectionTime smaller than metricStatPeriod", + }, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStatPeriod": "250", + "metricStat": "Average", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, true, - "unsupported metricStatPeriod"}, - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricStatPeriod": "25", - "metricStat": "Average", - "awsRegion": "eu-west-1"}, + "unsupported metricStatPeriod", + }, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStatPeriod": "25", + "metricStat": "Average", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, true, - "unsupported metricStatPeriod"}, - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricStatPeriod": "25", - "metricStat": "Average", - "metricUnit": "Hour", - "awsRegion": "eu-west-1"}, + "unsupported metricStatPeriod", + }, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStatPeriod": "25", + "metricStat": "Average", + "metricUnit": "Hour", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, true, - "unsupported metricUnit"}, + "unsupported metricUnit", + }, // test errorWhenNullValues is false - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricStatPeriod": "60", - "metricStat": "Average", - "errorWhenNullValues": "false", - "awsRegion": "eu-west-1"}, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStatPeriod": "60", + "metricStat": "Average", + "errorWhenNullValues": "false", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, false, - "with errorWhenNullValues set to false"}, + "with errorWhenNullValues set to false", + }, // test errorWhenNullValues is true - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricStatPeriod": "60", - "metricStat": "Average", - "errorWhenNullValues": "true", - "awsRegion": "eu-west-1"}, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStatPeriod": "60", + "metricStat": "Average", + "errorWhenNullValues": "true", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, false, - "with errorWhenNullValues set to true"}, + "with errorWhenNullValues set to true", + }, // test errorWhenNullValues is incorrect - {map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricStatPeriod": "60", - "metricStat": "Average", - "errorWhenNullValues": "maybe", - "awsRegion": "eu-west-1"}, + { + map[string]string{ + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStatPeriod": "60", + "metricStat": "Average", + "errorWhenNullValues": "maybe", + "awsRegion": "eu-west-1", + }, testAWSAuthentication, true, - "unsupported value for errorWhenNullValues"}, + "unsupported value for errorWhenNullValues", + }, } var awsCloudwatchMetricIdentifiers = []awsCloudwatchMetricIdentifier{ @@ -489,7 +569,7 @@ var awsCloudwatchGetMetricTestData = []awsCloudwatchMetadata{ // Test for metric with no data { namespace: "Custom", - metricsName: testAWSCloudwatchNoValueMetric, + metricsName: testAWSCloudwatchEmptyValues, dimensionName: []string{"DIM"}, dimensionValue: []string{"DIM_VALUE"}, targetMetricValue: 100, @@ -507,7 +587,7 @@ var awsCloudwatchGetMetricTestData = []awsCloudwatchMetadata{ // Test for metric with no data, and the scaler errors when metric data values are empty { namespace: "Custom", - metricsName: testAWSCloudwatchNoValueMetric, + metricsName: testAWSCloudwatchEmptyValues, dimensionName: []string{"DIM"}, dimensionValue: []string{"DIM_VALUE"}, targetMetricValue: 100, @@ -524,8 +604,7 @@ var awsCloudwatchGetMetricTestData = []awsCloudwatchMetadata{ }, } -type mockCloudwatch struct { -} +type mockCloudwatch struct{} func (m *mockCloudwatch) GetMetricData(_ context.Context, input *cloudwatch.GetMetricDataInput, _ ...func(*cloudwatch.Options)) (*cloudwatch.GetMetricDataOutput, error) { if input.MetricDataQueries[0].MetricStat != nil { @@ -536,6 +615,14 @@ func (m *mockCloudwatch) GetMetricData(_ context.Context, input *cloudwatch.GetM return &cloudwatch.GetMetricDataOutput{ MetricDataResults: []types.MetricDataResult{}, }, nil + case testAWSCloudwatchEmptyValues: + return &cloudwatch.GetMetricDataOutput{ + MetricDataResults: []types.MetricDataResult{ + { + Values: []float64{}, + }, + }, + }, nil } } @@ -585,9 +672,10 @@ func TestAWSCloudwatchScalerGetMetrics(t *testing.T) { case testAWSCloudwatchErrorMetric: assert.Error(t, err, "expect error because of cloudwatch api error") case testAWSCloudwatchNoValueMetric: - // if errorWhenNullValues is defined, then an error is expected + assert.NoError(t, err, "dont expect error when returning empty metric list from cloudwatch") + case testAWSCloudwatchEmptyValues: if meta.errorWhenNullValues { - assert.Error(t, err, "expected an error when returning empty metric list from cloudwatch") + assert.Error(t, err, "expect error when returning empty metric list from cloudwatch, because errorWhenNullValues is true") } else { assert.NoError(t, err, "dont expect error when returning empty metric list from cloudwatch") } diff --git a/tests/helper/helper.go b/tests/helper/helper.go index bbb3b8124b1..4cee58ca0af 100644 --- a/tests/helper/helper.go +++ b/tests/helper/helper.go @@ -38,6 +38,7 @@ import ( "k8s.io/client-go/tools/remotecommand" "sigs.k8s.io/controller-runtime/pkg/client/config" + v1alpha1Api "github.com/kedacore/keda/v2/apis/keda/v1alpha1" "github.com/kedacore/keda/v2/pkg/generated/clientset/versioned/typed/keda/v1alpha1" ) @@ -309,17 +310,20 @@ func WaitForNamespaceDeletion(t *testing.T, nsName string) bool { } func WaitForScaledJobCount(t *testing.T, kc *kubernetes.Clientset, scaledJobName, namespace string, - target, iterations, intervalSeconds int) bool { + target, iterations, intervalSeconds int, +) bool { return waitForJobCount(t, kc, fmt.Sprintf("scaledjob.keda.sh/name=%s", scaledJobName), namespace, target, iterations, intervalSeconds) } func WaitForJobCount(t *testing.T, kc *kubernetes.Clientset, namespace string, - target, iterations, intervalSeconds int) bool { + target, iterations, intervalSeconds int, +) bool { return waitForJobCount(t, kc, "", namespace, target, iterations, intervalSeconds) } func waitForJobCount(t *testing.T, kc *kubernetes.Clientset, selector, namespace string, - target, iterations, intervalSeconds int) bool { + target, iterations, intervalSeconds int, +) bool { for i := 0; i < iterations; i++ { jobList, _ := kc.BatchV1().Jobs(namespace).List(context.Background(), metav1.ListOptions{ LabelSelector: selector, @@ -340,8 +344,9 @@ func waitForJobCount(t *testing.T, kc *kubernetes.Clientset, selector, namespace } func WaitForJobCountUntilIteration(t *testing.T, kc *kubernetes.Clientset, namespace string, - target, iterations, intervalSeconds int) bool { - var isTargetAchieved = false + target, iterations, intervalSeconds int, +) bool { + isTargetAchieved := false for i := 0; i < iterations; i++ { jobList, _ := kc.BatchV1().Jobs(namespace).List(context.Background(), metav1.ListOptions{}) @@ -364,7 +369,8 @@ func WaitForJobCountUntilIteration(t *testing.T, kc *kubernetes.Clientset, names // Waits until deployment count hits target or number of iterations are done. func WaitForPodCountInNamespace(t *testing.T, kc *kubernetes.Clientset, namespace string, - target, iterations, intervalSeconds int) bool { + target, iterations, intervalSeconds int, +) bool { for i := 0; i < iterations; i++ { pods, _ := kc.CoreV1().Pods(namespace).List(context.Background(), metav1.ListOptions{}) @@ -410,7 +416,8 @@ func WaitForAllPodRunningInNamespace(t *testing.T, kc *kubernetes.Clientset, nam // Waits until the Horizontal Pod Autoscaler for the scaledObject reports that it has metrics available // to calculate, or until the number of iterations are done, whichever happens first. func WaitForHPAMetricsToPopulate(t *testing.T, kc *kubernetes.Clientset, name, namespace string, - iterations, intervalSeconds int) bool { + iterations, intervalSeconds int, +) bool { totalWaitDuration := time.Duration(iterations) * time.Duration(intervalSeconds) * time.Second startedWaiting := time.Now() for i := 0; i < iterations; i++ { @@ -436,7 +443,8 @@ func WaitForHPAMetricsToPopulate(t *testing.T, kc *kubernetes.Clientset, name, n // Waits until deployment ready replica count hits target or number of iterations are done. func WaitForDeploymentReplicaReadyCount(t *testing.T, kc *kubernetes.Clientset, name, namespace string, - target, iterations, intervalSeconds int) bool { + target, iterations, intervalSeconds int, +) bool { for i := 0; i < iterations; i++ { deployment, _ := kc.AppsV1().Deployments(namespace).Get(context.Background(), name, metav1.GetOptions{}) replicas := deployment.Status.ReadyReplicas @@ -456,7 +464,8 @@ func WaitForDeploymentReplicaReadyCount(t *testing.T, kc *kubernetes.Clientset, // Waits until statefulset count hits target or number of iterations are done. func WaitForStatefulsetReplicaReadyCount(t *testing.T, kc *kubernetes.Clientset, name, namespace string, - target, iterations, intervalSeconds int) bool { + target, iterations, intervalSeconds int, +) bool { for i := 0; i < iterations; i++ { statefulset, _ := kc.AppsV1().StatefulSets(namespace).Get(context.Background(), name, metav1.GetOptions{}) replicas := statefulset.Status.ReadyReplicas @@ -518,7 +527,8 @@ func AssertReplicaCountNotChangeDuringTimePeriod(t *testing.T, kc *kubernetes.Cl } func WaitForHpaCreation(t *testing.T, kc *kubernetes.Clientset, name, namespace string, - iterations, intervalSeconds int) (*autoscalingv2.HorizontalPodAutoscaler, error) { + iterations, intervalSeconds int, +) (*autoscalingv2.HorizontalPodAutoscaler, error) { hpa := &autoscalingv2.HorizontalPodAutoscaler{} var err error for i := 0; i < iterations; i++ { @@ -754,7 +764,8 @@ func DeletePodsInNamespaceBySelector(t *testing.T, kc *kubernetes.Clientset, sel // Wait for Pods identified by selector to complete termination func WaitForPodsTerminated(t *testing.T, kc *kubernetes.Clientset, selector, namespace string, - iterations, intervalSeconds int) bool { + iterations, intervalSeconds int, +) bool { for i := 0; i < iterations; i++ { pods, err := kc.CoreV1().Pods(namespace).List(context.Background(), metav1.ListOptions{LabelSelector: selector}) if (err != nil && errors.IsNotFound(err)) || len(pods.Items) == 0 { @@ -910,3 +921,32 @@ func CheckKubectlGetResult(t *testing.T, kind string, name string, namespace str unqoutedOutput := strings.ReplaceAll(string(output), "\"", "") assert.Equal(t, expected, unqoutedOutput) } + +// FailIfScaledObjectStatusNotReachedWithTimeout waits for the scaledobject to reach the desired state, within the timeout +// or fails the test if the timeout is reached. +func FailIfScaledObjectStatusNotReachedWithTimeout(t *testing.T, kedaClient *v1alpha1.KedaV1alpha1Client, namespace, scaledObjectName string, timeout time.Duration, desiredState v1alpha1Api.ConditionType) { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + for { + select { + case <-ctx.Done(): + t.Fatalf("timeout waiting for scaledobject to be in %s state", desiredState) + default: + scaledObject, err := kedaClient.ScaledObjects(namespace).Get(context.Background(), scaledObjectName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("error getting scaledobject: %s", err) + } + + conditions := scaledObject.Status.Conditions + + t.Logf("scaledobject status: %+v", conditions) + + if len(conditions) > 0 && conditions[len(conditions)-1].Type == desiredState { + return + } + + time.Sleep(10 * time.Second) + } + } +} diff --git a/tests/scalers/aws/aws_cloudwatch_error_null_values/aws_cloudwatch_error_null_values_test.go b/tests/scalers/aws/aws_cloudwatch_error_null_values/aws_cloudwatch_error_null_values_test.go new file mode 100644 index 00000000000..55ae762867a --- /dev/null +++ b/tests/scalers/aws/aws_cloudwatch_error_null_values/aws_cloudwatch_error_null_values_test.go @@ -0,0 +1,245 @@ +//go:build e2e +// +build e2e + +package aws_cloudwatch_error_null_metric_values_test + +import ( + "context" + "encoding/base64" + "fmt" + "os" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/config" + "github.com/aws/aws-sdk-go-v2/credentials" + "github.com/aws/aws-sdk-go-v2/service/cloudwatch" + "github.com/aws/aws-sdk-go-v2/service/cloudwatch/types" + "github.com/joho/godotenv" + v1alpha1Api "github.com/kedacore/keda/v2/apis/keda/v1alpha1" + "github.com/stretchr/testify/assert" + + . "github.com/kedacore/keda/v2/tests/helper" +) + +// Load environment variables from .env file +var _ = godotenv.Load("../../../.env") + +const ( + testName = "aws-cloudwatch-error-null-metrics-test" +) + +type templateData struct { + TestNamespace string + DeploymentName string + ScaledObjectName string + SecretName string + AwsAccessKeyID string + AwsSecretAccessKey string + AwsRegion string + CloudWatchMetricName string + CloudWatchMetricNamespace string + CloudWatchMetricDimensionName string + CloudWatchMetricDimensionValue string +} + +const ( + secretTemplate = `apiVersion: v1 +kind: Secret +metadata: + name: {{.SecretName}} + namespace: {{.TestNamespace}} +data: + AWS_ACCESS_KEY_ID: {{.AwsAccessKeyID}} + AWS_SECRET_ACCESS_KEY: {{.AwsSecretAccessKey}} +` + + triggerAuthenticationTemplate = `apiVersion: keda.sh/v1alpha1 +kind: TriggerAuthentication +metadata: + name: keda-trigger-auth-aws-credentials + namespace: {{.TestNamespace}} +spec: + secretTargetRef: + - parameter: awsAccessKeyID # Required. + name: {{.SecretName}} # Required. + key: AWS_ACCESS_KEY_ID # Required. + - parameter: awsSecretAccessKey # Required. + name: {{.SecretName}} # Required. + key: AWS_SECRET_ACCESS_KEY # Required. +` + + deploymentTemplate = ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: {{.DeploymentName}} + namespace: {{.TestNamespace}} + labels: + app: {{.DeploymentName}} +spec: + replicas: 0 + selector: + matchLabels: + app: {{.DeploymentName}} + template: + metadata: + labels: + app: {{.DeploymentName}} + spec: + containers: + - name: nginx + image: nginxinc/nginx-unprivileged + ports: + - containerPort: 80 +` + + scaledObjectTemplate = ` +apiVersion: keda.sh/v1alpha1 +kind: ScaledObject +metadata: + name: {{.ScaledObjectName}} + namespace: {{.TestNamespace}} + labels: + app: {{.DeploymentName}} +spec: + scaleTargetRef: + name: {{.DeploymentName}} + maxReplicaCount: 2 + minReplicaCount: 0 + cooldownPeriod: 1 + triggers: + - type: aws-cloudwatch + authenticationRef: + name: keda-trigger-auth-aws-credentials + metadata: + awsRegion: {{.AwsRegion}} + namespace: {{.CloudWatchMetricNamespace}} + dimensionName: {{.CloudWatchMetricDimensionName}} + dimensionValue: {{.CloudWatchMetricDimensionValue}} + metricName: {{.CloudWatchMetricName}} + targetMetricValue: "1" + minMetricValue: "1" + errorWhenNullValues: "true" + metricCollectionTime: "120" + metricStatPeriod: "60" +` +) + +var ( + testNamespace = fmt.Sprintf("%s-ns", testName) + deploymentName = fmt.Sprintf("%s-deployment", testName) + scaledObjectName = fmt.Sprintf("%s-so", testName) + secretName = fmt.Sprintf("%s-secret", testName) + cloudwatchMetricName = fmt.Sprintf("cw-%d", GetRandomNumber()) + awsAccessKeyID = os.Getenv("TF_AWS_ACCESS_KEY") + awsSecretAccessKey = os.Getenv("TF_AWS_SECRET_KEY") + awsRegion = os.Getenv("TF_AWS_REGION") + cloudwatchMetricNamespace = "DoesNotExist" + cloudwatchMetricDimensionName = "dimensionName" + cloudwatchMetricDimensionValue = "dimensionValue" + maxReplicaCount = 2 + minReplicaCount = 0 +) + +func TestCloudWatchScalerWithErrorWhenNullValues(t *testing.T) { + // setup cloudwatch + cloudwatchClient := createCloudWatchClient() + + // check that the metric in question is not already present, and is returning + // an empty set of values. + checkCloudWatchCustomMetric(t, cloudwatchClient) + + // Create kubernetes resources + kc := GetKubernetesClient(t) + kedaClient := GetKedaKubernetesClient(t) + data, templates := getTemplateData() + CreateKubernetesResources(t, kc, testNamespace, data, templates) + defer DeleteKubernetesResources(t, testNamespace, data, templates) + + assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, minReplicaCount, 60, 1), + "replica count should be %d after 1 minute", minReplicaCount) + + // check that the scaledobject is in paused state + FailIfScaledObjectStatusNotReachedWithTimeout(t, kedaClient, testNamespace, scaledObjectName, 2*time.Minute, v1alpha1Api.ConditionPaused) + + // check that the deployment did not scale, as the metric query is returning + // null values and the scaledobject is receiving errors, the deployment + // should not scale. + assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, minReplicaCount, 60, 1), + "replica count should be %d after 1 minute", minReplicaCount) +} + +// checkCloudWatchCustomMetric will evaluate the custom metric for any metric values, if any +// values are found the test will be failed. +func checkCloudWatchCustomMetric(t *testing.T, cloudwatchClient *cloudwatch.Client) { + metricData, err := cloudwatchClient.GetMetricData(context.Background(), &cloudwatch.GetMetricDataInput{ + MetricDataQueries: []types.MetricDataQuery{ + { + Id: aws.String("m1"), + ReturnData: aws.Bool(true), + MetricStat: &types.MetricStat{ + Metric: &types.Metric{ + Namespace: aws.String(cloudwatchMetricNamespace), + MetricName: aws.String(cloudwatchMetricName), + Dimensions: []types.Dimension{ + { + Name: aws.String(cloudwatchMetricDimensionName), + Value: aws.String(cloudwatchMetricDimensionValue), + }, + }, + }, + Period: aws.Int32(60), + Stat: aws.String("Average"), + }, + }, + }, + // evaluate +/- 5 minutes from now to be sure we cover the query window + // leading into the e2e test. + EndTime: aws.Time(time.Now().Add(time.Minute * 5)), + StartTime: aws.Time(time.Now().Add(-time.Minute * 5)), + }) + if err != nil { + t.Fatalf("error checking cloudwatch metric: %s", err) + return + } + + // This is a e2e preflight check for returning an error when there are no + // metric values. If there are any metric values, then the test should fail + // here, as the scaler will never enter an error state if there are metric + // values in the query window. + if len(metricData.MetricDataResults) != 1 || len(metricData.MetricDataResults[0].Values) > 0 { + t.Fatalf("found unexpected metric data results for namespace: %s: %+v", cloudwatchMetricNamespace, metricData.MetricDataResults) + return + } +} + +func createCloudWatchClient() *cloudwatch.Client { + configOptions := make([]func(*config.LoadOptions) error, 0) + configOptions = append(configOptions, config.WithRegion(awsRegion)) + cfg, _ := config.LoadDefaultConfig(context.TODO(), configOptions...) + cfg.Credentials = credentials.NewStaticCredentialsProvider(awsAccessKeyID, awsSecretAccessKey, "") + return cloudwatch.NewFromConfig(cfg) +} + +func getTemplateData() (templateData, []Template) { + return templateData{ + TestNamespace: testNamespace, + DeploymentName: deploymentName, + ScaledObjectName: scaledObjectName, + SecretName: secretName, + AwsAccessKeyID: base64.StdEncoding.EncodeToString([]byte(awsAccessKeyID)), + AwsSecretAccessKey: base64.StdEncoding.EncodeToString([]byte(awsSecretAccessKey)), + AwsRegion: awsRegion, + CloudWatchMetricName: cloudwatchMetricName, + CloudWatchMetricNamespace: cloudwatchMetricNamespace, + CloudWatchMetricDimensionName: cloudwatchMetricDimensionName, + CloudWatchMetricDimensionValue: cloudwatchMetricDimensionValue, + }, []Template{ + {Name: "secretTemplate", Config: secretTemplate}, + {Name: "triggerAuthenticationTemplate", Config: triggerAuthenticationTemplate}, + {Name: "deploymentTemplate", Config: deploymentTemplate}, + {Name: "scaledObjectTemplate", Config: scaledObjectTemplate}, + } +} diff --git a/tests/scalers/aws/aws_cloudwatch_min_value_null_values/aws_cloudwatch_min_value_null_values_test.go b/tests/scalers/aws/aws_cloudwatch_min_value_null_values/aws_cloudwatch_min_value_null_values_test.go new file mode 100644 index 00000000000..7729075c4e9 --- /dev/null +++ b/tests/scalers/aws/aws_cloudwatch_min_value_null_values/aws_cloudwatch_min_value_null_values_test.go @@ -0,0 +1,243 @@ +//go:build e2e +// +build e2e + +package aws_cloudwatch_min_value_null_values_test + +import ( + "context" + "encoding/base64" + "fmt" + "os" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/config" + "github.com/aws/aws-sdk-go-v2/credentials" + "github.com/aws/aws-sdk-go-v2/service/cloudwatch" + "github.com/aws/aws-sdk-go-v2/service/cloudwatch/types" + "github.com/joho/godotenv" + v1alpha1Api "github.com/kedacore/keda/v2/apis/keda/v1alpha1" + "github.com/stretchr/testify/assert" + + . "github.com/kedacore/keda/v2/tests/helper" +) + +// Load environment variables from .env file +var _ = godotenv.Load("../../../.env") + +const ( + testName = "aws-cloudwatch-min-value-null-metrics-test" +) + +type templateData struct { + TestNamespace string + DeploymentName string + ScaledObjectName string + SecretName string + AwsAccessKeyID string + AwsSecretAccessKey string + AwsRegion string + CloudWatchMetricName string + CloudWatchMetricNamespace string + CloudWatchMetricDimensionName string + CloudWatchMetricDimensionValue string +} + +const ( + secretTemplate = `apiVersion: v1 +kind: Secret +metadata: + name: {{.SecretName}} + namespace: {{.TestNamespace}} +data: + AWS_ACCESS_KEY_ID: {{.AwsAccessKeyID}} + AWS_SECRET_ACCESS_KEY: {{.AwsSecretAccessKey}} +` + + triggerAuthenticationTemplate = `apiVersion: keda.sh/v1alpha1 +kind: TriggerAuthentication +metadata: + name: keda-trigger-auth-aws-credentials + namespace: {{.TestNamespace}} +spec: + secretTargetRef: + - parameter: awsAccessKeyID # Required. + name: {{.SecretName}} # Required. + key: AWS_ACCESS_KEY_ID # Required. + - parameter: awsSecretAccessKey # Required. + name: {{.SecretName}} # Required. + key: AWS_SECRET_ACCESS_KEY # Required. +` + + deploymentTemplate = ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: {{.DeploymentName}} + namespace: {{.TestNamespace}} + labels: + app: {{.DeploymentName}} +spec: + replicas: 0 + selector: + matchLabels: + app: {{.DeploymentName}} + template: + metadata: + labels: + app: {{.DeploymentName}} + spec: + containers: + - name: nginx + image: nginxinc/nginx-unprivileged + ports: + - containerPort: 80 +` + + scaledObjectTemplate = ` +apiVersion: keda.sh/v1alpha1 +kind: ScaledObject +metadata: + name: {{.ScaledObjectName}} + namespace: {{.TestNamespace}} + labels: + app: {{.DeploymentName}} +spec: + scaleTargetRef: + name: {{.DeploymentName}} + maxReplicaCount: 2 + minReplicaCount: 0 + cooldownPeriod: 1 + triggers: + - type: aws-cloudwatch + authenticationRef: + name: keda-trigger-auth-aws-credentials + metadata: + awsRegion: {{.AwsRegion}} + namespace: {{.CloudWatchMetricNamespace}} + dimensionName: {{.CloudWatchMetricDimensionName}} + dimensionValue: {{.CloudWatchMetricDimensionValue}} + metricName: {{.CloudWatchMetricName}} + targetMetricValue: "1" + minMetricValue: "1" + metricCollectionTime: "120" + metricStatPeriod: "60" +` +) + +var ( + testNamespace = fmt.Sprintf("%s-ns", testName) + deploymentName = fmt.Sprintf("%s-deployment", testName) + scaledObjectName = fmt.Sprintf("%s-so", testName) + secretName = fmt.Sprintf("%s-secret", testName) + cloudwatchMetricName = fmt.Sprintf("cw-%d", GetRandomNumber()) + awsAccessKeyID = os.Getenv("TF_AWS_ACCESS_KEY") + awsSecretAccessKey = os.Getenv("TF_AWS_SECRET_KEY") + awsRegion = os.Getenv("TF_AWS_REGION") + cloudwatchMetricNamespace = "DoesNotExist" + cloudwatchMetricDimensionName = "dimensionName" + cloudwatchMetricDimensionValue = "dimensionValue" + maxReplicaCount = 2 + minReplicaCount = 0 + minMetricValueReplicaCount = 1 +) + +func TestCloudWatchScalerWithMinValueWhenNullValues(t *testing.T) { + // setup cloudwatch + cloudwatchClient := createCloudWatchClient() + + // check that the metric in question is not already present, and is returning + // an empty set of values. + checkCloudWatchCustomMetric(t, cloudwatchClient) + + // Create kubernetes resources + kc := GetKubernetesClient(t) + kedaClient := GetKedaKubernetesClient(t) + data, templates := getTemplateData() + CreateKubernetesResources(t, kc, testNamespace, data, templates) + defer DeleteKubernetesResources(t, testNamespace, data, templates) + + assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, minReplicaCount, 60, 1), + "replica count should be %d after 1 minute", minReplicaCount) + + // check that the scaledobject is in paused state + FailIfScaledObjectStatusNotReachedWithTimeout(t, kedaClient, testNamespace, scaledObjectName, 2*time.Minute, v1alpha1Api.ConditionPaused) + + // check that the deployment scaled up to the minMetricValueReplicaCount + assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, minMetricValueReplicaCount, 60, 1), + "replica count should be %d after 1 minute", minMetricValueReplicaCount) +} + +func createCloudWatchClient() *cloudwatch.Client { + configOptions := make([]func(*config.LoadOptions) error, 0) + configOptions = append(configOptions, config.WithRegion(awsRegion)) + cfg, _ := config.LoadDefaultConfig(context.TODO(), configOptions...) + cfg.Credentials = credentials.NewStaticCredentialsProvider(awsAccessKeyID, awsSecretAccessKey, "") + return cloudwatch.NewFromConfig(cfg) +} + +// checkCloudWatchCustomMetric will evaluate the custom metric for any metric values, if any +// values are found the test will be failed. +func checkCloudWatchCustomMetric(t *testing.T, cloudwatchClient *cloudwatch.Client) { + metricData, err := cloudwatchClient.GetMetricData(context.Background(), &cloudwatch.GetMetricDataInput{ + MetricDataQueries: []types.MetricDataQuery{ + { + Id: aws.String("m1"), + ReturnData: aws.Bool(true), + MetricStat: &types.MetricStat{ + Metric: &types.Metric{ + Namespace: aws.String(cloudwatchMetricNamespace), + MetricName: aws.String(cloudwatchMetricName), + Dimensions: []types.Dimension{ + { + Name: aws.String(cloudwatchMetricDimensionName), + Value: aws.String(cloudwatchMetricDimensionValue), + }, + }, + }, + Period: aws.Int32(60), + Stat: aws.String("Average"), + }, + }, + }, + // evaluate +/- 5 minutes from now to be sure we cover the query window + // leading into the e2e test. + EndTime: aws.Time(time.Now().Add(time.Minute * 5)), + StartTime: aws.Time(time.Now().Add(-time.Minute * 5)), + }) + if err != nil { + t.Fatalf("error checking cloudwatch metric: %s", err) + return + } + + // This is a e2e preflight check for returning an error when there are no + // metric values. If there are any metric values, then the test should fail + // here, as the scaler will never enter an error state if there are metric + // values in the query window. + if len(metricData.MetricDataResults) != 1 || len(metricData.MetricDataResults[0].Values) > 0 { + t.Fatalf("found unexpected metric data results for namespace: %s: %+v", cloudwatchMetricNamespace, metricData.MetricDataResults) + return + } +} + +func getTemplateData() (templateData, []Template) { + return templateData{ + TestNamespace: testNamespace, + DeploymentName: deploymentName, + ScaledObjectName: scaledObjectName, + SecretName: secretName, + AwsAccessKeyID: base64.StdEncoding.EncodeToString([]byte(awsAccessKeyID)), + AwsSecretAccessKey: base64.StdEncoding.EncodeToString([]byte(awsSecretAccessKey)), + AwsRegion: awsRegion, + CloudWatchMetricName: cloudwatchMetricName, + CloudWatchMetricNamespace: cloudwatchMetricNamespace, + CloudWatchMetricDimensionName: cloudwatchMetricDimensionName, + CloudWatchMetricDimensionValue: cloudwatchMetricDimensionValue, + }, []Template{ + {Name: "secretTemplate", Config: secretTemplate}, + {Name: "triggerAuthenticationTemplate", Config: triggerAuthenticationTemplate}, + {Name: "deploymentTemplate", Config: deploymentTemplate}, + {Name: "scaledObjectTemplate", Config: scaledObjectTemplate}, + } +} From c6dc5245eae185a3da338d91777ee8d78714de1e Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Thu, 4 Apr 2024 18:34:14 +0100 Subject: [PATCH 08/23] remove erroneous print statement Signed-off-by: Rob Pickerill --- pkg/scalers/aws_cloudwatch_scaler.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/scalers/aws_cloudwatch_scaler.go b/pkg/scalers/aws_cloudwatch_scaler.go index 9a78a9d8924..fa4b9d6db4b 100644 --- a/pkg/scalers/aws_cloudwatch_scaler.go +++ b/pkg/scalers/aws_cloudwatch_scaler.go @@ -397,8 +397,6 @@ func (s *awsCloudwatchScaler) GetCloudwatchMetrics(ctx context.Context) (float64 s.logger.V(1).Info("Received Metric Data", "data", output) var metricValue float64 - fmt.Printf("output.MetricDataResults: %+v", output.MetricDataResults) - // If no metric data is received and errorWhenNullValues is set to true, return an error if len(output.MetricDataResults) > 0 && len(output.MetricDataResults[0].Values) == 0 && s.metadata.errorWhenNullValues { emptyMetricsErrMessage := "empty metric data received, and errorWhenNullValues is set to true, returning error" From 0dc9b7735b019f63eec8d761a6caf99cda048fde Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Thu, 4 Apr 2024 18:43:51 +0100 Subject: [PATCH 09/23] remove unused vars Signed-off-by: Rob Pickerill --- .../aws_cloudwatch_error_null_values_test.go | 3 +-- .../aws_cloudwatch_min_value_null_values_test.go | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/scalers/aws/aws_cloudwatch_error_null_values/aws_cloudwatch_error_null_values_test.go b/tests/scalers/aws/aws_cloudwatch_error_null_values/aws_cloudwatch_error_null_values_test.go index 55ae762867a..59d3e9e2781 100644 --- a/tests/scalers/aws/aws_cloudwatch_error_null_values/aws_cloudwatch_error_null_values_test.go +++ b/tests/scalers/aws/aws_cloudwatch_error_null_values/aws_cloudwatch_error_null_values_test.go @@ -17,9 +17,9 @@ import ( "github.com/aws/aws-sdk-go-v2/service/cloudwatch" "github.com/aws/aws-sdk-go-v2/service/cloudwatch/types" "github.com/joho/godotenv" - v1alpha1Api "github.com/kedacore/keda/v2/apis/keda/v1alpha1" "github.com/stretchr/testify/assert" + v1alpha1Api "github.com/kedacore/keda/v2/apis/keda/v1alpha1" . "github.com/kedacore/keda/v2/tests/helper" ) @@ -139,7 +139,6 @@ var ( cloudwatchMetricNamespace = "DoesNotExist" cloudwatchMetricDimensionName = "dimensionName" cloudwatchMetricDimensionValue = "dimensionValue" - maxReplicaCount = 2 minReplicaCount = 0 ) diff --git a/tests/scalers/aws/aws_cloudwatch_min_value_null_values/aws_cloudwatch_min_value_null_values_test.go b/tests/scalers/aws/aws_cloudwatch_min_value_null_values/aws_cloudwatch_min_value_null_values_test.go index 7729075c4e9..5f085b3a590 100644 --- a/tests/scalers/aws/aws_cloudwatch_min_value_null_values/aws_cloudwatch_min_value_null_values_test.go +++ b/tests/scalers/aws/aws_cloudwatch_min_value_null_values/aws_cloudwatch_min_value_null_values_test.go @@ -17,9 +17,9 @@ import ( "github.com/aws/aws-sdk-go-v2/service/cloudwatch" "github.com/aws/aws-sdk-go-v2/service/cloudwatch/types" "github.com/joho/godotenv" - v1alpha1Api "github.com/kedacore/keda/v2/apis/keda/v1alpha1" "github.com/stretchr/testify/assert" + v1alpha1Api "github.com/kedacore/keda/v2/apis/keda/v1alpha1" . "github.com/kedacore/keda/v2/tests/helper" ) @@ -138,7 +138,6 @@ var ( cloudwatchMetricNamespace = "DoesNotExist" cloudwatchMetricDimensionName = "dimensionName" cloudwatchMetricDimensionValue = "dimensionValue" - maxReplicaCount = 2 minReplicaCount = 0 minMetricValueReplicaCount = 1 ) From 1f7073622a6ff22a75eda5b166707624c77021e5 Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Thu, 11 Apr 2024 21:26:23 +0100 Subject: [PATCH 10/23] rename errorWhenMetricValuesEmpty -> ignoreNullValues Signed-off-by: Rob Pickerill --- CHANGELOG.md | 2 +- pkg/scalers/aws_cloudwatch_scaler.go | 21 ++--- pkg/scalers/aws_cloudwatch_scaler_test.go | 85 ++++++++++--------- ...oudwatch_ignore_null_values_false_test.go} | 10 ++- .../aws_cloudwatch_min_metric_value_test.go} | 8 +- 5 files changed, 66 insertions(+), 60 deletions(-) rename tests/scalers/aws/{aws_cloudwatch_error_null_values/aws_cloudwatch_error_null_values_test.go => aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go} (95%) rename tests/scalers/aws/{aws_cloudwatch_min_value_null_values/aws_cloudwatch_min_value_null_values_test.go => aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go} (96%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2dd901ea406..61d181daaf6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,7 +67,7 @@ Here is an overview of all new **experimental** features: - **General**: Add GRPC Healthchecks ([#5590](https://github.com/kedacore/keda/issues/5590)) - **General**: Add OPENTELEMETRY flag in e2e test YAML ([#5375](https://github.com/kedacore/keda/issues/5375)) - **General**: Add support for cross tenant/cloud authentication when using Azure Workload Identity for TriggerAuthentication ([#5441](https://github.com/kedacore/keda/issues/5441)) -- **AWS CloudWatch Scaler**: Add support for errorWhenNullValues ([#5352](https://github.com/kedacore/keda/issues/5352)) +- **AWS CloudWatch Scaler**: Add support for ignoreNullValues ([#5352](https://github.com/kedacore/keda/issues/5352)) - **GCP Stackdriver Scaler**: Add missing parameters 'rate' and 'count' for GCP Stackdriver Scaler alignment ([#5633](https://github.com/kedacore/keda/issues/5633)) - **Metrics API Scaler**: Add support for various formats: json, xml, yaml, prometheus ([#2633](https://github.com/kedacore/keda/issues/2633)) - **MongoDB Scaler**: Add scheme field support srv record ([#5544](https://github.com/kedacore/keda/issues/5544)) diff --git a/pkg/scalers/aws_cloudwatch_scaler.go b/pkg/scalers/aws_cloudwatch_scaler.go index fa4b9d6db4b..43f9711a9dc 100644 --- a/pkg/scalers/aws_cloudwatch_scaler.go +++ b/pkg/scalers/aws_cloudwatch_scaler.go @@ -43,7 +43,7 @@ type awsCloudwatchMetadata struct { targetMetricValue float64 activationTargetMetricValue float64 minMetricValue float64 - errorWhenNullValues bool + ignoreNullValues bool metricCollectionTime int64 metricStat string @@ -190,11 +190,11 @@ func parseAwsCloudwatchMetadata(config *scalersconfig.ScalerConfig) (*awsCloudwa } meta.minMetricValue = minMetricValue - errorWhenNullValues, err := getParameterFromConfigV2(config, "errorWhenNullValues", reflect.TypeOf(false), IsOptional(true), UseMetadata(true), WithDefaultVal(false)) + ignoreNullValues, err := getParameterFromConfigV2(config, "ignoreNullValues", reflect.TypeOf(true), IsOptional(true), UseMetadata(true), WithDefaultVal(true)) if err != nil { return nil, err } - meta.errorWhenNullValues, _ = errorWhenNullValues.(bool) + meta.ignoreNullValues, _ = ignoreNullValues.(bool) meta.metricStat = defaultMetricStat if val, ok := config.TriggerMetadata["metricStat"]; ok && val != "" { @@ -395,21 +395,22 @@ func (s *awsCloudwatchScaler) GetCloudwatchMetrics(ctx context.Context) (float64 } s.logger.V(1).Info("Received Metric Data", "data", output) - var metricValue float64 - // If no metric data is received and errorWhenNullValues is set to true, return an error - if len(output.MetricDataResults) > 0 && len(output.MetricDataResults[0].Values) == 0 && s.metadata.errorWhenNullValues { - emptyMetricsErrMessage := "empty metric data received, and errorWhenNullValues is set to true, returning error" - s.logger.Error(err, emptyMetricsErrMessage) - return -1, fmt.Errorf(emptyMetricsErrMessage) + // If no metric data results or the first result has no values, and ignoreNullValues is false, + // the scaler should return an error to prevent any further scaling actions. + if len(output.MetricDataResults) > 0 && len(output.MetricDataResults[0].Values) == 0 && !s.metadata.ignoreNullValues { + emptyMetricsErrMsg := "empty metric data received, ignoreNullValues is false, returning error" + s.logger.Error(nil, emptyMetricsErrMsg) + return -1, fmt.Errorf(emptyMetricsErrMsg) } + var metricValue float64 + if len(output.MetricDataResults) > 0 && len(output.MetricDataResults[0].Values) > 0 { metricValue = output.MetricDataResults[0].Values[0] } else { s.logger.Info("empty metric data received, returning minMetricValue") metricValue = s.metadata.minMetricValue } - return metricValue, nil } diff --git a/pkg/scalers/aws_cloudwatch_scaler_test.go b/pkg/scalers/aws_cloudwatch_scaler_test.go index 99149541d50..571a912d05d 100644 --- a/pkg/scalers/aws_cloudwatch_scaler_test.go +++ b/pkg/scalers/aws_cloudwatch_scaler_test.go @@ -427,56 +427,56 @@ var testAWSCloudwatchMetadata = []parseAWSCloudwatchMetadataTestData{ testAWSAuthentication, true, "unsupported metricUnit", }, - // test errorWhenNullValues is false + // test ignoreNullValues is false { map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricStatPeriod": "60", - "metricStat": "Average", - "errorWhenNullValues": "false", - "awsRegion": "eu-west-1", + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStatPeriod": "60", + "metricStat": "Average", + "ignoreNullValues": "false", + "awsRegion": "eu-west-1", }, testAWSAuthentication, false, - "with errorWhenNullValues set to false", + "with ignoreNullValues set to false", }, - // test errorWhenNullValues is true + // test ignoreNullValues is true { map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricStatPeriod": "60", - "metricStat": "Average", - "errorWhenNullValues": "true", - "awsRegion": "eu-west-1", + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStatPeriod": "60", + "metricStat": "Average", + "ignoreNullValues": "true", + "awsRegion": "eu-west-1", }, testAWSAuthentication, false, - "with errorWhenNullValues set to true", + "with ignoreNullValues set to true", }, - // test errorWhenNullValues is incorrect + // test ignoreNullValues is incorrect { map[string]string{ - "namespace": "AWS/SQS", - "dimensionName": "QueueName", - "dimensionValue": "keda", - "metricName": "ApproximateNumberOfMessagesVisible", - "targetMetricValue": "2", - "minMetricValue": "0", - "metricStatPeriod": "60", - "metricStat": "Average", - "errorWhenNullValues": "maybe", - "awsRegion": "eu-west-1", + "namespace": "AWS/SQS", + "dimensionName": "QueueName", + "dimensionValue": "keda", + "metricName": "ApproximateNumberOfMessagesVisible", + "targetMetricValue": "2", + "minMetricValue": "0", + "metricStatPeriod": "60", + "metricStat": "Average", + "ignoreNullValues": "maybe", + "awsRegion": "eu-west-1", }, testAWSAuthentication, true, - "unsupported value for errorWhenNullValues", + "unsupported value for ignoreNullValues", }, } @@ -542,6 +542,7 @@ var awsCloudwatchGetMetricTestData = []awsCloudwatchMetadata{ dimensionValue: []string{"DIM_VALUE"}, targetMetricValue: 100, minMetricValue: 0, + ignoreNullValues: true, metricCollectionTime: 60, metricStat: "Average", metricUnit: "", @@ -566,7 +567,7 @@ var awsCloudwatchGetMetricTestData = []awsCloudwatchMetadata{ awsAuthorization: awsutils.AuthorizationMetadata{PodIdentityOwner: false}, triggerIndex: 0, }, - // Test for metric with no data + // Test for metric with no data, no error expected as we are ignoring null values { namespace: "Custom", metricsName: testAWSCloudwatchEmptyValues, @@ -574,7 +575,7 @@ var awsCloudwatchGetMetricTestData = []awsCloudwatchMetadata{ dimensionValue: []string{"DIM_VALUE"}, targetMetricValue: 100, minMetricValue: 0, - errorWhenNullValues: false, + ignoreNullValues: true, metricCollectionTime: 60, metricStat: "Average", metricUnit: "", @@ -592,7 +593,7 @@ var awsCloudwatchGetMetricTestData = []awsCloudwatchMetadata{ dimensionValue: []string{"DIM_VALUE"}, targetMetricValue: 100, minMetricValue: 0, - errorWhenNullValues: true, + ignoreNullValues: false, metricCollectionTime: 60, metricStat: "Average", metricUnit: "", @@ -674,10 +675,10 @@ func TestAWSCloudwatchScalerGetMetrics(t *testing.T) { case testAWSCloudwatchNoValueMetric: assert.NoError(t, err, "dont expect error when returning empty metric list from cloudwatch") case testAWSCloudwatchEmptyValues: - if meta.errorWhenNullValues { - assert.Error(t, err, "expect error when returning empty metric list from cloudwatch, because errorWhenNullValues is true") - } else { + if meta.ignoreNullValues { assert.NoError(t, err, "dont expect error when returning empty metric list from cloudwatch") + } else { + assert.Error(t, err, "expect error when returning empty metric list from cloudwatch, because ignoreNullValues is false") } default: assert.EqualValues(t, int64(10.0), value[0].Value.Value()) diff --git a/tests/scalers/aws/aws_cloudwatch_error_null_values/aws_cloudwatch_error_null_values_test.go b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go similarity index 95% rename from tests/scalers/aws/aws_cloudwatch_error_null_values/aws_cloudwatch_error_null_values_test.go rename to tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go index 59d3e9e2781..24fd81b3e0c 100644 --- a/tests/scalers/aws/aws_cloudwatch_error_null_values/aws_cloudwatch_error_null_values_test.go +++ b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go @@ -1,7 +1,7 @@ //go:build e2e // +build e2e -package aws_cloudwatch_error_null_metric_values_test +package aws_cloudwatch_ignore_null_values_false import ( "context" @@ -27,7 +27,7 @@ import ( var _ = godotenv.Load("../../../.env") const ( - testName = "aws-cloudwatch-error-null-metrics-test" + testName = "aws-cloudwatch-ignore-null-values-false-test" ) type templateData struct { @@ -121,7 +121,7 @@ spec: metricName: {{.CloudWatchMetricName}} targetMetricValue: "1" minMetricValue: "1" - errorWhenNullValues: "true" + ignoreNullValues: "false" metricCollectionTime: "120" metricStatPeriod: "60" ` @@ -142,7 +142,9 @@ var ( minReplicaCount = 0 ) -func TestCloudWatchScalerWithErrorWhenNullValues(t *testing.T) { +// This test is to verify that the scaler results in an error state when +// the metric query returns null values and the ignoreNullValues is set to false. +func TestCloudWatchScalerWithIgnoreNullValuesFalse(t *testing.T) { // setup cloudwatch cloudwatchClient := createCloudWatchClient() diff --git a/tests/scalers/aws/aws_cloudwatch_min_value_null_values/aws_cloudwatch_min_value_null_values_test.go b/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go similarity index 96% rename from tests/scalers/aws/aws_cloudwatch_min_value_null_values/aws_cloudwatch_min_value_null_values_test.go rename to tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go index 5f085b3a590..d531f58fbaf 100644 --- a/tests/scalers/aws/aws_cloudwatch_min_value_null_values/aws_cloudwatch_min_value_null_values_test.go +++ b/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go @@ -1,7 +1,7 @@ //go:build e2e // +build e2e -package aws_cloudwatch_min_value_null_values_test +package aws_cloudwatch_min_metric_value import ( "context" @@ -27,7 +27,7 @@ import ( var _ = godotenv.Load("../../../.env") const ( - testName = "aws-cloudwatch-min-value-null-metrics-test" + testName = "aws-cloudwatch-min-metric-value-test" ) type templateData struct { @@ -142,7 +142,9 @@ var ( minMetricValueReplicaCount = 1 ) -func TestCloudWatchScalerWithMinValueWhenNullValues(t *testing.T) { +// This test is to verify that the scaler returns the minMetricValue when the metric +// value is null and ignoreNullValues is set to true. +func TestCloudWatchScalerWithMinMetricValue(t *testing.T) { // setup cloudwatch cloudwatchClient := createCloudWatchClient() From 185db2140de025ee825e17ddb173005aa9004691 Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Sat, 13 Apr 2024 17:17:50 +0100 Subject: [PATCH 11/23] move towards shared package for e2e aws Signed-off-by: Rob Pickerill --- ...loudwatch_ignore_null_values_false_test.go | 68 +++---------------- .../aws_cloudwatch_min_metric_value_test.go | 68 +++---------------- .../aws/helpers/cloudwatch/cloudwatch.go | 67 ++++++++++++++++++ 3 files changed, 85 insertions(+), 118 deletions(-) create mode 100644 tests/scalers/aws/helpers/cloudwatch/cloudwatch.go diff --git a/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go index 24fd81b3e0c..b54ca1b988e 100644 --- a/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go +++ b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go @@ -11,16 +11,12 @@ import ( "testing" "time" - "github.com/aws/aws-sdk-go-v2/aws" - "github.com/aws/aws-sdk-go-v2/config" - "github.com/aws/aws-sdk-go-v2/credentials" - "github.com/aws/aws-sdk-go-v2/service/cloudwatch" - "github.com/aws/aws-sdk-go-v2/service/cloudwatch/types" "github.com/joho/godotenv" "github.com/stretchr/testify/assert" v1alpha1Api "github.com/kedacore/keda/v2/apis/keda/v1alpha1" . "github.com/kedacore/keda/v2/tests/helper" + "github.com/kedacore/keda/v2/tests/scalers/aws/helpers/cloudwatch" ) // Load environment variables from .env file @@ -145,12 +141,18 @@ var ( // This test is to verify that the scaler results in an error state when // the metric query returns null values and the ignoreNullValues is set to false. func TestCloudWatchScalerWithIgnoreNullValuesFalse(t *testing.T) { + ctx := context.Background() + // setup cloudwatch - cloudwatchClient := createCloudWatchClient() + cloudwatchClient, err := cloudwatch.NewClient(ctx, awsRegion, awsAccessKeyID, awsSecretAccessKey, "") + assert.Nil(t, err, "error creating cloudwatch client") // check that the metric in question is not already present, and is returning // an empty set of values. - checkCloudWatchCustomMetric(t, cloudwatchClient) + metricQuery := cloudwatch.CreateMetricDataInputForEmptyMetricValues(cloudwatchMetricNamespace, cloudwatchMetricName, cloudwatchMetricDimensionName, cloudwatchMetricDimensionValue) + metricData, err := cloudwatch.GetMetricData(ctx, cloudwatchClient, metricQuery) + assert.Nil(t, err, "error getting metric data") + assert.Nil(t, cloudwatch.ExpectEmptyMetricDataResults(metricData), "metric data should be empty") // Create kubernetes resources kc := GetKubernetesClient(t) @@ -172,58 +174,6 @@ func TestCloudWatchScalerWithIgnoreNullValuesFalse(t *testing.T) { "replica count should be %d after 1 minute", minReplicaCount) } -// checkCloudWatchCustomMetric will evaluate the custom metric for any metric values, if any -// values are found the test will be failed. -func checkCloudWatchCustomMetric(t *testing.T, cloudwatchClient *cloudwatch.Client) { - metricData, err := cloudwatchClient.GetMetricData(context.Background(), &cloudwatch.GetMetricDataInput{ - MetricDataQueries: []types.MetricDataQuery{ - { - Id: aws.String("m1"), - ReturnData: aws.Bool(true), - MetricStat: &types.MetricStat{ - Metric: &types.Metric{ - Namespace: aws.String(cloudwatchMetricNamespace), - MetricName: aws.String(cloudwatchMetricName), - Dimensions: []types.Dimension{ - { - Name: aws.String(cloudwatchMetricDimensionName), - Value: aws.String(cloudwatchMetricDimensionValue), - }, - }, - }, - Period: aws.Int32(60), - Stat: aws.String("Average"), - }, - }, - }, - // evaluate +/- 5 minutes from now to be sure we cover the query window - // leading into the e2e test. - EndTime: aws.Time(time.Now().Add(time.Minute * 5)), - StartTime: aws.Time(time.Now().Add(-time.Minute * 5)), - }) - if err != nil { - t.Fatalf("error checking cloudwatch metric: %s", err) - return - } - - // This is a e2e preflight check for returning an error when there are no - // metric values. If there are any metric values, then the test should fail - // here, as the scaler will never enter an error state if there are metric - // values in the query window. - if len(metricData.MetricDataResults) != 1 || len(metricData.MetricDataResults[0].Values) > 0 { - t.Fatalf("found unexpected metric data results for namespace: %s: %+v", cloudwatchMetricNamespace, metricData.MetricDataResults) - return - } -} - -func createCloudWatchClient() *cloudwatch.Client { - configOptions := make([]func(*config.LoadOptions) error, 0) - configOptions = append(configOptions, config.WithRegion(awsRegion)) - cfg, _ := config.LoadDefaultConfig(context.TODO(), configOptions...) - cfg.Credentials = credentials.NewStaticCredentialsProvider(awsAccessKeyID, awsSecretAccessKey, "") - return cloudwatch.NewFromConfig(cfg) -} - func getTemplateData() (templateData, []Template) { return templateData{ TestNamespace: testNamespace, diff --git a/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go b/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go index d531f58fbaf..a0173d29989 100644 --- a/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go +++ b/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go @@ -11,16 +11,12 @@ import ( "testing" "time" - "github.com/aws/aws-sdk-go-v2/aws" - "github.com/aws/aws-sdk-go-v2/config" - "github.com/aws/aws-sdk-go-v2/credentials" - "github.com/aws/aws-sdk-go-v2/service/cloudwatch" - "github.com/aws/aws-sdk-go-v2/service/cloudwatch/types" "github.com/joho/godotenv" "github.com/stretchr/testify/assert" v1alpha1Api "github.com/kedacore/keda/v2/apis/keda/v1alpha1" . "github.com/kedacore/keda/v2/tests/helper" + "github.com/kedacore/keda/v2/tests/scalers/aws/helpers/cloudwatch" ) // Load environment variables from .env file @@ -145,12 +141,18 @@ var ( // This test is to verify that the scaler returns the minMetricValue when the metric // value is null and ignoreNullValues is set to true. func TestCloudWatchScalerWithMinMetricValue(t *testing.T) { + ctx := context.Background() + // setup cloudwatch - cloudwatchClient := createCloudWatchClient() + cloudwatchClient, err := cloudwatch.NewClient(ctx, awsRegion, awsAccessKeyID, awsSecretAccessKey, "") + assert.Nil(t, err, "error creating cloudwatch client") // check that the metric in question is not already present, and is returning // an empty set of values. - checkCloudWatchCustomMetric(t, cloudwatchClient) + metricQuery := cloudwatch.CreateMetricDataInputForEmptyMetricValues(cloudwatchMetricNamespace, cloudwatchMetricName, cloudwatchMetricDimensionName, cloudwatchMetricDimensionValue) + metricData, err := cloudwatch.GetMetricData(ctx, cloudwatchClient, metricQuery) + assert.Nil(t, err, "error getting metric data") + assert.Nil(t, cloudwatch.ExpectEmptyMetricDataResults(metricData), "metric data should be empty") // Create kubernetes resources kc := GetKubernetesClient(t) @@ -170,58 +172,6 @@ func TestCloudWatchScalerWithMinMetricValue(t *testing.T) { "replica count should be %d after 1 minute", minMetricValueReplicaCount) } -func createCloudWatchClient() *cloudwatch.Client { - configOptions := make([]func(*config.LoadOptions) error, 0) - configOptions = append(configOptions, config.WithRegion(awsRegion)) - cfg, _ := config.LoadDefaultConfig(context.TODO(), configOptions...) - cfg.Credentials = credentials.NewStaticCredentialsProvider(awsAccessKeyID, awsSecretAccessKey, "") - return cloudwatch.NewFromConfig(cfg) -} - -// checkCloudWatchCustomMetric will evaluate the custom metric for any metric values, if any -// values are found the test will be failed. -func checkCloudWatchCustomMetric(t *testing.T, cloudwatchClient *cloudwatch.Client) { - metricData, err := cloudwatchClient.GetMetricData(context.Background(), &cloudwatch.GetMetricDataInput{ - MetricDataQueries: []types.MetricDataQuery{ - { - Id: aws.String("m1"), - ReturnData: aws.Bool(true), - MetricStat: &types.MetricStat{ - Metric: &types.Metric{ - Namespace: aws.String(cloudwatchMetricNamespace), - MetricName: aws.String(cloudwatchMetricName), - Dimensions: []types.Dimension{ - { - Name: aws.String(cloudwatchMetricDimensionName), - Value: aws.String(cloudwatchMetricDimensionValue), - }, - }, - }, - Period: aws.Int32(60), - Stat: aws.String("Average"), - }, - }, - }, - // evaluate +/- 5 minutes from now to be sure we cover the query window - // leading into the e2e test. - EndTime: aws.Time(time.Now().Add(time.Minute * 5)), - StartTime: aws.Time(time.Now().Add(-time.Minute * 5)), - }) - if err != nil { - t.Fatalf("error checking cloudwatch metric: %s", err) - return - } - - // This is a e2e preflight check for returning an error when there are no - // metric values. If there are any metric values, then the test should fail - // here, as the scaler will never enter an error state if there are metric - // values in the query window. - if len(metricData.MetricDataResults) != 1 || len(metricData.MetricDataResults[0].Values) > 0 { - t.Fatalf("found unexpected metric data results for namespace: %s: %+v", cloudwatchMetricNamespace, metricData.MetricDataResults) - return - } -} - func getTemplateData() (templateData, []Template) { return templateData{ TestNamespace: testNamespace, diff --git a/tests/scalers/aws/helpers/cloudwatch/cloudwatch.go b/tests/scalers/aws/helpers/cloudwatch/cloudwatch.go new file mode 100644 index 00000000000..56d75effbc9 --- /dev/null +++ b/tests/scalers/aws/helpers/cloudwatch/cloudwatch.go @@ -0,0 +1,67 @@ +package cloudwatch + +import ( + "context" + "fmt" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/config" + "github.com/aws/aws-sdk-go-v2/credentials" + "github.com/aws/aws-sdk-go-v2/service/cloudwatch" + "github.com/aws/aws-sdk-go-v2/service/cloudwatch/types" +) + +// NewClient will provision a new Cloudwatch Client. +func NewClient(ctx context.Context, region, accessKeyID, secretAccessKey, sessionToken string) (*cloudwatch.Client, error) { + cfg, err := config.LoadDefaultConfig(ctx, config.WithRegion(region)) + if err != nil { + return nil, err + } + + cfg.Credentials = credentials.NewStaticCredentialsProvider(accessKeyID, secretAccessKey, sessionToken) + return cloudwatch.NewFromConfig(cfg), nil +} + +// CreateMetricDataInputForEmptyMetricValues will return a GetMetricDataInput with a single metric query +// that is expected to return no metric values. +func CreateMetricDataInputForEmptyMetricValues(metricNamespace, metricName, dimensionName, dimensionValue string) *cloudwatch.GetMetricDataInput { + return &cloudwatch.GetMetricDataInput{ + MetricDataQueries: []types.MetricDataQuery{ + { + Id: aws.String("m1"), + MetricStat: &types.MetricStat{ + Metric: &types.Metric{ + Namespace: &metricNamespace, + MetricName: &metricName, + Dimensions: []types.Dimension{ + { + Name: &dimensionName, + Value: &dimensionValue, + }, + }, + }, Period: aws.Int32(60), Stat: aws.String("Average"), + }, + }, + }, + // evaluate +/- 10 minutes from now to be sure we cover the query window + // as all e2e tests use a 300 second query window. + EndTime: aws.Time(time.Now().Add(time.Minute * 10)), + StartTime: aws.Time(time.Now().Add(-time.Minute * 10)), + } +} + +// GetMetricData will return the metric data for the given input. +func GetMetricData(ctx context.Context, cloudwatchClient *cloudwatch.Client, metricDataInput *cloudwatch.GetMetricDataInput) (*cloudwatch.GetMetricDataOutput, error) { + return cloudwatchClient.GetMetricData(ctx, metricDataInput) +} + +// ExpectEmptyMetricDataResults will evaluate the custom metric for any metric values, if any +// values are an error will be returned. +func ExpectEmptyMetricDataResults(metricData *cloudwatch.GetMetricDataOutput) error { + if len(metricData.MetricDataResults) != 1 || len(metricData.MetricDataResults[0].Values) > 0 { + return fmt.Errorf("found unexpected metric data results for metricData: %+v", metricData.MetricDataResults) + } + + return nil +} From 9f5bbf53ffdaee4b738cb89703a3ceaa7a1f4bbb Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Sun, 14 Apr 2024 14:25:52 +0100 Subject: [PATCH 12/23] minMetricValue optionality based on ignoreNullValues Signed-off-by: Rob Pickerill --- pkg/scalers/aws_cloudwatch_scaler.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/scalers/aws_cloudwatch_scaler.go b/pkg/scalers/aws_cloudwatch_scaler.go index 43f9711a9dc..e232eb6f73f 100644 --- a/pkg/scalers/aws_cloudwatch_scaler.go +++ b/pkg/scalers/aws_cloudwatch_scaler.go @@ -184,17 +184,22 @@ func parseAwsCloudwatchMetadata(config *scalersconfig.ScalerConfig) (*awsCloudwa } meta.activationTargetMetricValue = activationTargetMetricValue - minMetricValue, err := getFloatMetadataValue(config.TriggerMetadata, "minMetricValue", true, 0) + ignoreNullValues, err := getParameterFromConfigV2(config, "ignoreNullValues", reflect.TypeOf(true), IsOptional(true), UseMetadata(true), WithDefaultVal(true)) if err != nil { return nil, err } - meta.minMetricValue = minMetricValue + meta.ignoreNullValues, _ = ignoreNullValues.(bool) - ignoreNullValues, err := getParameterFromConfigV2(config, "ignoreNullValues", reflect.TypeOf(true), IsOptional(true), UseMetadata(true), WithDefaultVal(true)) + // Note here that optionality of minMetricValue, is dependent on + // ignoreNullValues. If ignoreNullValues is true, then minMetricValue is + // required, otherwise it is optional. This is because if ignoreNullValues + // is true, then the scaler will return minMetricValue when the metric value + // is null. + minMetricValue, err := getFloatMetadataValue(config.TriggerMetadata, "minMetricValue", meta.ignoreNullValues, 0) if err != nil { return nil, err } - meta.ignoreNullValues, _ = ignoreNullValues.(bool) + meta.minMetricValue = minMetricValue meta.metricStat = defaultMetricStat if val, ok := config.TriggerMetadata["metricStat"]; ok && val != "" { From 97f47a035a34542aade44fda965f0e47fb14d914 Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Fri, 26 Apr 2024 18:14:20 +0100 Subject: [PATCH 13/23] fail fast Co-authored-by: Jorge Turrado Ferrero Signed-off-by: Rob Pickerill --- .../aws_cloudwatch_ignore_null_values_false_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go index b54ca1b988e..f6ba2697070 100644 --- a/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go +++ b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go @@ -151,8 +151,8 @@ func TestCloudWatchScalerWithIgnoreNullValuesFalse(t *testing.T) { // an empty set of values. metricQuery := cloudwatch.CreateMetricDataInputForEmptyMetricValues(cloudwatchMetricNamespace, cloudwatchMetricName, cloudwatchMetricDimensionName, cloudwatchMetricDimensionValue) metricData, err := cloudwatch.GetMetricData(ctx, cloudwatchClient, metricQuery) - assert.Nil(t, err, "error getting metric data") - assert.Nil(t, cloudwatch.ExpectEmptyMetricDataResults(metricData), "metric data should be empty") + require.Nil(t, err, "error getting metric data") + require.Nil(t, cloudwatch.ExpectEmptyMetricDataResults(metricData), "metric data should be empty") // Create kubernetes resources kc := GetKubernetesClient(t) From 9d0ce60c425b2d71dc95cb6628b47d7468c22057 Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Fri, 26 Apr 2024 18:14:50 +0100 Subject: [PATCH 14/23] Update tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go Co-authored-by: Jorge Turrado Ferrero Signed-off-by: Rob Pickerill --- .../aws_cloudwatch_ignore_null_values_false_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go index f6ba2697070..54103cab41a 100644 --- a/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go +++ b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go @@ -13,6 +13,7 @@ import ( "github.com/joho/godotenv" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" v1alpha1Api "github.com/kedacore/keda/v2/apis/keda/v1alpha1" . "github.com/kedacore/keda/v2/tests/helper" From a8959f2c1d4387fd51e3fdc921e929d687b9cad4 Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Fri, 26 Apr 2024 18:15:59 +0100 Subject: [PATCH 15/23] Apply suggestions from code review Co-authored-by: Jorge Turrado Ferrero Signed-off-by: Rob Pickerill --- .../aws_cloudwatch_ignore_null_values_false_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go index 54103cab41a..5e7d74c81fb 100644 --- a/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go +++ b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go @@ -146,7 +146,7 @@ func TestCloudWatchScalerWithIgnoreNullValuesFalse(t *testing.T) { // setup cloudwatch cloudwatchClient, err := cloudwatch.NewClient(ctx, awsRegion, awsAccessKeyID, awsSecretAccessKey, "") - assert.Nil(t, err, "error creating cloudwatch client") + require.Nil(t, err, "error creating cloudwatch client") // check that the metric in question is not already present, and is returning // an empty set of values. From 702817f3cac117d7a9415ab3d2407ef1e1d01e29 Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Fri, 26 Apr 2024 18:18:17 +0100 Subject: [PATCH 16/23] fail fast Signed-off-by: Rob Pickerill --- .../aws_cloudwatch_min_metric_value_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go b/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go index a0173d29989..1741ea9f301 100644 --- a/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go +++ b/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go @@ -13,6 +13,7 @@ import ( "github.com/joho/godotenv" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" v1alpha1Api "github.com/kedacore/keda/v2/apis/keda/v1alpha1" . "github.com/kedacore/keda/v2/tests/helper" @@ -151,8 +152,8 @@ func TestCloudWatchScalerWithMinMetricValue(t *testing.T) { // an empty set of values. metricQuery := cloudwatch.CreateMetricDataInputForEmptyMetricValues(cloudwatchMetricNamespace, cloudwatchMetricName, cloudwatchMetricDimensionName, cloudwatchMetricDimensionValue) metricData, err := cloudwatch.GetMetricData(ctx, cloudwatchClient, metricQuery) - assert.Nil(t, err, "error getting metric data") - assert.Nil(t, cloudwatch.ExpectEmptyMetricDataResults(metricData), "metric data should be empty") + require.Nil(t, err, "error getting metric data") + require.Nil(t, cloudwatch.ExpectEmptyMetricDataResults(metricData), "metric data should be empty") // Create kubernetes resources kc := GetKubernetesClient(t) From 2862e9cf1df522f655c2d37362ab0066f3284799 Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Fri, 26 Apr 2024 18:19:08 +0100 Subject: [PATCH 17/23] fix broken new line Signed-off-by: Rob Pickerill --- tests/helper/helper.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/helper/helper.go b/tests/helper/helper.go index 4cee58ca0af..7f6004250a9 100644 --- a/tests/helper/helper.go +++ b/tests/helper/helper.go @@ -415,9 +415,7 @@ func WaitForAllPodRunningInNamespace(t *testing.T, kc *kubernetes.Clientset, nam // Waits until the Horizontal Pod Autoscaler for the scaledObject reports that it has metrics available // to calculate, or until the number of iterations are done, whichever happens first. -func WaitForHPAMetricsToPopulate(t *testing.T, kc *kubernetes.Clientset, name, namespace string, - iterations, intervalSeconds int, -) bool { +func WaitForHPAMetricsToPopulate(t *testing.T, kc *kubernetes.Clientset, name, namespace string, iterations, intervalSeconds int) bool { totalWaitDuration := time.Duration(iterations) * time.Duration(intervalSeconds) * time.Second startedWaiting := time.Now() for i := 0; i < iterations; i++ { From c6846762b28b5bb5a403ae5640c106dedca030e1 Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Fri, 26 Apr 2024 18:29:38 +0100 Subject: [PATCH 18/23] fix broken new lines Signed-off-by: Rob Pickerill --- tests/helper/helper.go | 36 +++++++++--------------------------- 1 file changed, 9 insertions(+), 27 deletions(-) diff --git a/tests/helper/helper.go b/tests/helper/helper.go index 7f6004250a9..e79b2b48261 100644 --- a/tests/helper/helper.go +++ b/tests/helper/helper.go @@ -309,21 +309,15 @@ func WaitForNamespaceDeletion(t *testing.T, nsName string) bool { return false } -func WaitForScaledJobCount(t *testing.T, kc *kubernetes.Clientset, scaledJobName, namespace string, - target, iterations, intervalSeconds int, -) bool { +func WaitForScaledJobCount(t *testing.T, kc *kubernetes.Clientset, scaledJobName, namespace string, target, iterations, intervalSeconds int) bool { return waitForJobCount(t, kc, fmt.Sprintf("scaledjob.keda.sh/name=%s", scaledJobName), namespace, target, iterations, intervalSeconds) } -func WaitForJobCount(t *testing.T, kc *kubernetes.Clientset, namespace string, - target, iterations, intervalSeconds int, -) bool { +func WaitForJobCount(t *testing.T, kc *kubernetes.Clientset, namespace string, target, iterations, intervalSeconds int) bool { return waitForJobCount(t, kc, "", namespace, target, iterations, intervalSeconds) } -func waitForJobCount(t *testing.T, kc *kubernetes.Clientset, selector, namespace string, - target, iterations, intervalSeconds int, -) bool { +func waitForJobCount(t *testing.T, kc *kubernetes.Clientset, selector, namespace string, target, iterations, intervalSeconds int) bool { for i := 0; i < iterations; i++ { jobList, _ := kc.BatchV1().Jobs(namespace).List(context.Background(), metav1.ListOptions{ LabelSelector: selector, @@ -343,9 +337,7 @@ func waitForJobCount(t *testing.T, kc *kubernetes.Clientset, selector, namespace return false } -func WaitForJobCountUntilIteration(t *testing.T, kc *kubernetes.Clientset, namespace string, - target, iterations, intervalSeconds int, -) bool { +func WaitForJobCountUntilIteration(t *testing.T, kc *kubernetes.Clientset, namespace string, target, iterations, intervalSeconds int) bool { isTargetAchieved := false for i := 0; i < iterations; i++ { @@ -368,9 +360,7 @@ func WaitForJobCountUntilIteration(t *testing.T, kc *kubernetes.Clientset, names } // Waits until deployment count hits target or number of iterations are done. -func WaitForPodCountInNamespace(t *testing.T, kc *kubernetes.Clientset, namespace string, - target, iterations, intervalSeconds int, -) bool { +func WaitForPodCountInNamespace(t *testing.T, kc *kubernetes.Clientset, namespace string, target, iterations, intervalSeconds int) bool { for i := 0; i < iterations; i++ { pods, _ := kc.CoreV1().Pods(namespace).List(context.Background(), metav1.ListOptions{}) @@ -440,9 +430,7 @@ func WaitForHPAMetricsToPopulate(t *testing.T, kc *kubernetes.Clientset, name, n } // Waits until deployment ready replica count hits target or number of iterations are done. -func WaitForDeploymentReplicaReadyCount(t *testing.T, kc *kubernetes.Clientset, name, namespace string, - target, iterations, intervalSeconds int, -) bool { +func WaitForDeploymentReplicaReadyCount(t *testing.T, kc *kubernetes.Clientset, name, namespace string, target, iterations, intervalSeconds int) bool { for i := 0; i < iterations; i++ { deployment, _ := kc.AppsV1().Deployments(namespace).Get(context.Background(), name, metav1.GetOptions{}) replicas := deployment.Status.ReadyReplicas @@ -461,9 +449,7 @@ func WaitForDeploymentReplicaReadyCount(t *testing.T, kc *kubernetes.Clientset, } // Waits until statefulset count hits target or number of iterations are done. -func WaitForStatefulsetReplicaReadyCount(t *testing.T, kc *kubernetes.Clientset, name, namespace string, - target, iterations, intervalSeconds int, -) bool { +func WaitForStatefulsetReplicaReadyCount(t *testing.T, kc *kubernetes.Clientset, name, namespace string, target, iterations, intervalSeconds int) bool { for i := 0; i < iterations; i++ { statefulset, _ := kc.AppsV1().StatefulSets(namespace).Get(context.Background(), name, metav1.GetOptions{}) replicas := statefulset.Status.ReadyReplicas @@ -524,9 +510,7 @@ func AssertReplicaCountNotChangeDuringTimePeriod(t *testing.T, kc *kubernetes.Cl } } -func WaitForHpaCreation(t *testing.T, kc *kubernetes.Clientset, name, namespace string, - iterations, intervalSeconds int, -) (*autoscalingv2.HorizontalPodAutoscaler, error) { +func WaitForHpaCreation(t *testing.T, kc *kubernetes.Clientset, name, namespace string, iterations, intervalSeconds int) (*autoscalingv2.HorizontalPodAutoscaler, error) { hpa := &autoscalingv2.HorizontalPodAutoscaler{} var err error for i := 0; i < iterations; i++ { @@ -761,9 +745,7 @@ func DeletePodsInNamespaceBySelector(t *testing.T, kc *kubernetes.Clientset, sel } // Wait for Pods identified by selector to complete termination -func WaitForPodsTerminated(t *testing.T, kc *kubernetes.Clientset, selector, namespace string, - iterations, intervalSeconds int, -) bool { +func WaitForPodsTerminated(t *testing.T, kc *kubernetes.Clientset, selector, namespace string, iterations, intervalSeconds int) bool { for i := 0; i < iterations; i++ { pods, err := kc.CoreV1().Pods(namespace).List(context.Background(), metav1.ListOptions{LabelSelector: selector}) if (err != nil && errors.IsNotFound(err)) || len(pods.Items) == 0 { From 5aceb727d986dc0adf71c9d517d2d413984f9491 Mon Sep 17 00:00:00 2001 From: robpickerill Date: Sat, 4 May 2024 21:03:45 +0100 Subject: [PATCH 19/23] assert no scaling changes in e2e, and set false for required in minMetricValue Signed-off-by: robpickerill --- pkg/scalers/aws_cloudwatch_scaler.go | 7 +---- tests/helper/helper.go | 30 ------------------- ...loudwatch_ignore_null_values_false_test.go | 8 +---- .../aws_cloudwatch_min_metric_value_test.go | 8 +---- 4 files changed, 3 insertions(+), 50 deletions(-) diff --git a/pkg/scalers/aws_cloudwatch_scaler.go b/pkg/scalers/aws_cloudwatch_scaler.go index e232eb6f73f..a288ce784ad 100644 --- a/pkg/scalers/aws_cloudwatch_scaler.go +++ b/pkg/scalers/aws_cloudwatch_scaler.go @@ -190,12 +190,7 @@ func parseAwsCloudwatchMetadata(config *scalersconfig.ScalerConfig) (*awsCloudwa } meta.ignoreNullValues, _ = ignoreNullValues.(bool) - // Note here that optionality of minMetricValue, is dependent on - // ignoreNullValues. If ignoreNullValues is true, then minMetricValue is - // required, otherwise it is optional. This is because if ignoreNullValues - // is true, then the scaler will return minMetricValue when the metric value - // is null. - minMetricValue, err := getFloatMetadataValue(config.TriggerMetadata, "minMetricValue", meta.ignoreNullValues, 0) + minMetricValue, err := getFloatMetadataValue(config.TriggerMetadata, "minMetricValue", false, 0) if err != nil { return nil, err } diff --git a/tests/helper/helper.go b/tests/helper/helper.go index e79b2b48261..96240c9d63e 100644 --- a/tests/helper/helper.go +++ b/tests/helper/helper.go @@ -38,7 +38,6 @@ import ( "k8s.io/client-go/tools/remotecommand" "sigs.k8s.io/controller-runtime/pkg/client/config" - v1alpha1Api "github.com/kedacore/keda/v2/apis/keda/v1alpha1" "github.com/kedacore/keda/v2/pkg/generated/clientset/versioned/typed/keda/v1alpha1" ) @@ -901,32 +900,3 @@ func CheckKubectlGetResult(t *testing.T, kind string, name string, namespace str unqoutedOutput := strings.ReplaceAll(string(output), "\"", "") assert.Equal(t, expected, unqoutedOutput) } - -// FailIfScaledObjectStatusNotReachedWithTimeout waits for the scaledobject to reach the desired state, within the timeout -// or fails the test if the timeout is reached. -func FailIfScaledObjectStatusNotReachedWithTimeout(t *testing.T, kedaClient *v1alpha1.KedaV1alpha1Client, namespace, scaledObjectName string, timeout time.Duration, desiredState v1alpha1Api.ConditionType) { - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() - - for { - select { - case <-ctx.Done(): - t.Fatalf("timeout waiting for scaledobject to be in %s state", desiredState) - default: - scaledObject, err := kedaClient.ScaledObjects(namespace).Get(context.Background(), scaledObjectName, metav1.GetOptions{}) - if err != nil { - t.Fatalf("error getting scaledobject: %s", err) - } - - conditions := scaledObject.Status.Conditions - - t.Logf("scaledobject status: %+v", conditions) - - if len(conditions) > 0 && conditions[len(conditions)-1].Type == desiredState { - return - } - - time.Sleep(10 * time.Second) - } - } -} diff --git a/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go index 5e7d74c81fb..b0f742c0d9c 100644 --- a/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go +++ b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go @@ -9,13 +9,11 @@ import ( "fmt" "os" "testing" - "time" "github.com/joho/godotenv" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - v1alpha1Api "github.com/kedacore/keda/v2/apis/keda/v1alpha1" . "github.com/kedacore/keda/v2/tests/helper" "github.com/kedacore/keda/v2/tests/scalers/aws/helpers/cloudwatch" ) @@ -165,14 +163,10 @@ func TestCloudWatchScalerWithIgnoreNullValuesFalse(t *testing.T) { assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, minReplicaCount, 60, 1), "replica count should be %d after 1 minute", minReplicaCount) - // check that the scaledobject is in paused state - FailIfScaledObjectStatusNotReachedWithTimeout(t, kedaClient, testNamespace, scaledObjectName, 2*time.Minute, v1alpha1Api.ConditionPaused) - // check that the deployment did not scale, as the metric query is returning // null values and the scaledobject is receiving errors, the deployment // should not scale. - assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, minReplicaCount, 60, 1), - "replica count should be %d after 1 minute", minReplicaCount) + AssertReplicaCountNotChangeDuringTimePeriod(t, kc, deploymentName, testNamespace, minReplicaCount, 60) } func getTemplateData() (templateData, []Template) { diff --git a/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go b/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go index 1741ea9f301..688b1ad8e5d 100644 --- a/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go +++ b/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go @@ -9,13 +9,11 @@ import ( "fmt" "os" "testing" - "time" "github.com/joho/godotenv" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - v1alpha1Api "github.com/kedacore/keda/v2/apis/keda/v1alpha1" . "github.com/kedacore/keda/v2/tests/helper" "github.com/kedacore/keda/v2/tests/scalers/aws/helpers/cloudwatch" ) @@ -165,12 +163,8 @@ func TestCloudWatchScalerWithMinMetricValue(t *testing.T) { assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, minReplicaCount, 60, 1), "replica count should be %d after 1 minute", minReplicaCount) - // check that the scaledobject is in paused state - FailIfScaledObjectStatusNotReachedWithTimeout(t, kedaClient, testNamespace, scaledObjectName, 2*time.Minute, v1alpha1Api.ConditionPaused) - // check that the deployment scaled up to the minMetricValueReplicaCount - assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, minMetricValueReplicaCount, 60, 1), - "replica count should be %d after 1 minute", minMetricValueReplicaCount) + AssertReplicaCountNotChangeDuringTimePeriod(t, kc, deploymentName, testNamespace, minReplicaCount, 60) } func getTemplateData() (templateData, []Template) { From 9374c265f853a94b055c3290cb0e70a21e3b7495 Mon Sep 17 00:00:00 2001 From: robpickerill Date: Tue, 21 May 2024 07:07:21 +0100 Subject: [PATCH 20/23] fix ci checks Signed-off-by: robpickerill --- CHANGELOG.md | 3 +-- .../aws_cloudwatch_ignore_null_values_false_test.go | 1 - .../aws_cloudwatch_min_metric_value_test.go | 3 +-- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 241a0342b00..09c0baa097b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,11 +69,10 @@ Here is an overview of all new **experimental** features: ### Improvements +- **AWS CloudWatch Scaler**: Add support for ignoreNullValues ([#5352](https://github.com/kedacore/keda/issues/5352)) - **GCP Scalers**: Added custom time horizon in GCP scalers ([#5778](https://github.com/kedacore/keda/issues/5778)) - **GitHub Scaler**: Fixed pagination, fetching repository list ([#5738](https://github.com/kedacore/keda/issues/5738)) - **Kafka**: Fix logic to scale to zero on invalid offset even with earliest offsetResetPolicy ([#5689](https://github.com/kedacore/keda/issues/5689)) -- **AWS CloudWatch Scaler**: Add support for ignoreNullValues ([#5352](https://github.com/kedacore/keda/issues/5352)) - ### Fixes diff --git a/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go index b0f742c0d9c..aecd55b9164 100644 --- a/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go +++ b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go @@ -155,7 +155,6 @@ func TestCloudWatchScalerWithIgnoreNullValuesFalse(t *testing.T) { // Create kubernetes resources kc := GetKubernetesClient(t) - kedaClient := GetKedaKubernetesClient(t) data, templates := getTemplateData() CreateKubernetesResources(t, kc, testNamespace, data, templates) defer DeleteKubernetesResources(t, testNamespace, data, templates) diff --git a/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go b/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go index 688b1ad8e5d..b388d9e9a73 100644 --- a/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go +++ b/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go @@ -155,7 +155,6 @@ func TestCloudWatchScalerWithMinMetricValue(t *testing.T) { // Create kubernetes resources kc := GetKubernetesClient(t) - kedaClient := GetKedaKubernetesClient(t) data, templates := getTemplateData() CreateKubernetesResources(t, kc, testNamespace, data, templates) defer DeleteKubernetesResources(t, testNamespace, data, templates) @@ -164,7 +163,7 @@ func TestCloudWatchScalerWithMinMetricValue(t *testing.T) { "replica count should be %d after 1 minute", minReplicaCount) // check that the deployment scaled up to the minMetricValueReplicaCount - AssertReplicaCountNotChangeDuringTimePeriod(t, kc, deploymentName, testNamespace, minReplicaCount, 60) + AssertReplicaCountNotChangeDuringTimePeriod(t, kc, deploymentName, testNamespace, minMetricValueReplicaCount, 60) } func getTemplateData() (templateData, []Template) { From 1898924dccd7f5d293238d672fcd718854e61e3a Mon Sep 17 00:00:00 2001 From: Rob Pickerill Date: Tue, 21 May 2024 09:37:13 +0100 Subject: [PATCH 21/23] Update tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go fix invalid check Co-authored-by: Jorge Turrado Ferrero Signed-off-by: Rob Pickerill --- .../aws_cloudwatch_min_metric_value_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go b/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go index b388d9e9a73..620694f113d 100644 --- a/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go +++ b/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go @@ -160,7 +160,7 @@ func TestCloudWatchScalerWithMinMetricValue(t *testing.T) { defer DeleteKubernetesResources(t, testNamespace, data, templates) assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, minReplicaCount, 60, 1), - "replica count should be %d after 1 minute", minReplicaCount) + "replica count should be %d after 1 minute", minMetricValueReplicaCount) // check that the deployment scaled up to the minMetricValueReplicaCount AssertReplicaCountNotChangeDuringTimePeriod(t, kc, deploymentName, testNamespace, minMetricValueReplicaCount, 60) From 2e76a9b9ee59374a1ac74f072ff8add1c2978e05 Mon Sep 17 00:00:00 2001 From: robpickerill Date: Sat, 8 Jun 2024 16:03:07 +0100 Subject: [PATCH 22/23] fix merge conflicts Signed-off-by: robpickerill --- pkg/scalers/aws_cloudwatch_scaler.go | 11 ++--------- pkg/scalers/aws_cloudwatch_scaler_test.go | 2 +- .../aws_cloudwatch_ignore_null_values_false_test.go | 2 +- .../aws_cloudwatch_min_metric_value_test.go | 13 ++++++++++++- tests/scalers/aws/helpers/cloudwatch/cloudwatch.go | 3 +++ 5 files changed, 19 insertions(+), 12 deletions(-) diff --git a/pkg/scalers/aws_cloudwatch_scaler.go b/pkg/scalers/aws_cloudwatch_scaler.go index 10c1c2f9504..a07db246f80 100644 --- a/pkg/scalers/aws_cloudwatch_scaler.go +++ b/pkg/scalers/aws_cloudwatch_scaler.go @@ -37,6 +37,7 @@ type awsCloudwatchMetadata struct { TargetMetricValue float64 `keda:"name=targetMetricValue, order=triggerMetadata"` ActivationTargetMetricValue float64 `keda:"name=activationTargetMetricValue, order=triggerMetadata, optional"` MinMetricValue float64 `keda:"name=minMetricValue, order=triggerMetadata"` + IgnoreNullValues bool `keda:"name=ignoreNullValues, order=triggerMetadata, optional, default=true"` MetricCollectionTime int64 `keda:"name=metricCollectionTime, order=triggerMetadata, optional, default=300"` MetricStat string `keda:"name=metricStat, order=triggerMetadata, optional, default=Average"` @@ -183,14 +184,6 @@ func checkMetricStatPeriod(period int64) error { return nil } -func checkMetricCollectionTime(metricCollectionTime, metricStatPeriod int64) error { - if metricCollectionTime < 0 || metricCollectionTime%metricStatPeriod != 0 { - return fmt.Errorf("metricCollectionTime must be greater than 0 and a multiple of metricStatPeriod(%d), %d is given", metricStatPeriod, metricCollectionTime) - } - - return nil -} - func computeQueryWindow(current time.Time, metricPeriodSec, metricEndTimeOffsetSec, metricCollectionTimeSec int64) (startTime, endTime time.Time) { endTime = current.Add(time.Second * -1 * time.Duration(metricEndTimeOffsetSec)).Truncate(time.Duration(metricPeriodSec) * time.Second) startTime = endTime.Add(time.Second * -1 * time.Duration(metricCollectionTimeSec)) @@ -290,7 +283,7 @@ func (s *awsCloudwatchScaler) GetCloudwatchMetrics(ctx context.Context) (float64 // If no metric data results or the first result has no values, and ignoreNullValues is false, // the scaler should return an error to prevent any further scaling actions. - if len(output.MetricDataResults) > 0 && len(output.MetricDataResults[0].Values) == 0 && !s.metadata.ignoreNullValues { + if len(output.MetricDataResults) > 0 && len(output.MetricDataResults[0].Values) == 0 && !s.metadata.IgnoreNullValues { emptyMetricsErrMsg := "empty metric data received, ignoreNullValues is false, returning error" s.logger.Error(nil, emptyMetricsErrMsg) return -1, fmt.Errorf(emptyMetricsErrMsg) diff --git a/pkg/scalers/aws_cloudwatch_scaler_test.go b/pkg/scalers/aws_cloudwatch_scaler_test.go index 7a6842b4fc9..da6e85fc2c7 100644 --- a/pkg/scalers/aws_cloudwatch_scaler_test.go +++ b/pkg/scalers/aws_cloudwatch_scaler_test.go @@ -656,7 +656,7 @@ func TestAWSCloudwatchScalerGetMetrics(t *testing.T) { case testAWSCloudwatchNoValueMetric: assert.NoError(t, err, "dont expect error when returning empty metric list from cloudwatch") case testAWSCloudwatchEmptyValues: - if meta.ignoreNullValues { + if meta.IgnoreNullValues { assert.NoError(t, err, "dont expect error when returning empty metric list from cloudwatch") } else { assert.Error(t, err, "expect error when returning empty metric list from cloudwatch, because ignoreNullValues is false") diff --git a/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go index aecd55b9164..575bbfb2bff 100644 --- a/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go +++ b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go @@ -164,7 +164,7 @@ func TestCloudWatchScalerWithIgnoreNullValuesFalse(t *testing.T) { // check that the deployment did not scale, as the metric query is returning // null values and the scaledobject is receiving errors, the deployment - // should not scale. + // should not scale, even though the minMetricValue is set to 1. AssertReplicaCountNotChangeDuringTimePeriod(t, kc, deploymentName, testNamespace, minReplicaCount, 60) } diff --git a/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go b/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go index 620694f113d..8247da604f5 100644 --- a/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go +++ b/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go @@ -9,6 +9,7 @@ import ( "fmt" "os" "testing" + "time" "github.com/joho/godotenv" "github.com/stretchr/testify/assert" @@ -162,7 +163,17 @@ func TestCloudWatchScalerWithMinMetricValue(t *testing.T) { assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, minReplicaCount, 60, 1), "replica count should be %d after 1 minute", minMetricValueReplicaCount) - // check that the deployment scaled up to the minMetricValueReplicaCount + // Allow a small amount of grace for stabilization, otherwise we will see the + // minMetricValue of 1 scale up the deployment from 0 to 1, as the deployment + // starts at a minReplicaCount of 0. The reason for this is to ensure that the + // scaler is still functioning when the metric value is null, as opposed to + // returning an error, and not scaling the workload at all. + time.Sleep(5 * time.Second) + + // Then check that the deployment did not scale further, as the metric query + // is returning null values, the scaler should evaluate the metric value as + // the minMetricValue of 1, and not scale the deployment further beyond this + // point. AssertReplicaCountNotChangeDuringTimePeriod(t, kc, deploymentName, testNamespace, minMetricValueReplicaCount, 60) } diff --git a/tests/scalers/aws/helpers/cloudwatch/cloudwatch.go b/tests/scalers/aws/helpers/cloudwatch/cloudwatch.go index 56d75effbc9..d30434e47f1 100644 --- a/tests/scalers/aws/helpers/cloudwatch/cloudwatch.go +++ b/tests/scalers/aws/helpers/cloudwatch/cloudwatch.go @@ -1,3 +1,6 @@ +//go:build e2e +// +build e2e + package cloudwatch import ( From 43a2c3f7e148dfff65dd0ca79c69990969cf44fa Mon Sep 17 00:00:00 2001 From: robpickerill Date: Mon, 10 Jun 2024 09:35:27 +0100 Subject: [PATCH 23/23] fix e2e package names Signed-off-by: robpickerill --- .../aws_cloudwatch_ignore_null_values_false_test.go | 2 +- .../aws_cloudwatch_min_metric_value_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go index 575bbfb2bff..ddf4e1f08e3 100644 --- a/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go +++ b/tests/scalers/aws/aws_cloudwatch_ignore_null_values_false/aws_cloudwatch_ignore_null_values_false_test.go @@ -1,7 +1,7 @@ //go:build e2e // +build e2e -package aws_cloudwatch_ignore_null_values_false +package aws_cloudwatch_ignore_null_values_false_test import ( "context" diff --git a/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go b/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go index 8247da604f5..341cead869c 100644 --- a/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go +++ b/tests/scalers/aws/aws_cloudwatch_min_metric_value/aws_cloudwatch_min_metric_value_test.go @@ -1,7 +1,7 @@ //go:build e2e // +build e2e -package aws_cloudwatch_min_metric_value +package aws_cloudwatch_min_metric_value_test import ( "context"