From f6358d574d66e46e8c61923b3e31c2a2f0d49a2c Mon Sep 17 00:00:00 2001 From: Dao Thanh Tung Date: Fri, 6 Sep 2024 15:23:11 +0100 Subject: [PATCH] Refactor aws dynamodb scaler (#5961) Signed-off-by: dttung2905 --- pkg/scalers/aws_dynamodb_scaler.go | 130 ++++++---------------- pkg/scalers/aws_dynamodb_scaler_test.go | 102 +++++++++-------- pkg/scalers/scalersconfig/typed_config.go | 3 +- 3 files changed, 92 insertions(+), 143 deletions(-) diff --git a/pkg/scalers/aws_dynamodb_scaler.go b/pkg/scalers/aws_dynamodb_scaler.go index 72aff9beb46..f3c9bcac1f6 100644 --- a/pkg/scalers/aws_dynamodb_scaler.go +++ b/pkg/scalers/aws_dynamodb_scaler.go @@ -5,7 +5,6 @@ import ( "encoding/json" "errors" "fmt" - "strconv" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/dynamodb" @@ -28,18 +27,18 @@ type awsDynamoDBScaler struct { } type awsDynamoDBMetadata struct { - tableName string - awsRegion string - awsEndpoint string - keyConditionExpression string - expressionAttributeNames map[string]string - expressionAttributeValues map[string]types.AttributeValue - indexName string - targetValue int64 - activationTargetValue int64 awsAuthorization awsutils.AuthorizationMetadata + expressionAttributeValues map[string]types.AttributeValue + expressionAttributeNames map[string]string triggerIndex int metricName string + TableName string `keda:"name=tableName, order=triggerMetadata"` + AwsRegion string `keda:"name=awsRegion, order=triggerMetadata"` + AwsEndpoint string `keda:"name=awsEndpoint, order=triggerMetadata, optional"` + KeyConditionExpression string `keda:"name=keyConditionExpression, order=triggerMetadata"` + IndexName string `keda:"name=indexName, order=triggerMetadata, optional"` + TargetValue int64 `keda:"name=targetValue, order=triggerMetadata, optional, default=-1"` + ActivationTargetValue int64 `keda:"name=activationTargetValue, order=triggerMetadata, default=0"` } func NewAwsDynamoDBScaler(ctx context.Context, config *scalersconfig.ScalerConfig) (Scaler, error) { @@ -65,63 +64,24 @@ func NewAwsDynamoDBScaler(ctx context.Context, config *scalersconfig.ScalerConfi } var ( - // ErrAwsDynamoNoTableName is returned when "tableName" is missing from the config. - ErrAwsDynamoNoTableName = errors.New("no tableName given") - - // ErrAwsDynamoNoAwsRegion is returned when "awsRegion" is missing from the config. - ErrAwsDynamoNoAwsRegion = errors.New("no awsRegion given") - - // ErrAwsDynamoNoKeyConditionExpression is returned when "keyConditionExpression" is missing from the config. - ErrAwsDynamoNoKeyConditionExpression = errors.New("no keyConditionExpression given") - - // ErrAwsDynamoEmptyExpressionAttributeNames is returned when "expressionAttributeNames" is empty. - ErrAwsDynamoEmptyExpressionAttributeNames = errors.New("empty map") - - // ErrAwsDynamoInvalidExpressionAttributeNames is returned when "expressionAttributeNames" is an invalid JSON. - ErrAwsDynamoInvalidExpressionAttributeNames = errors.New("invalid expressionAttributeNames") - - // ErrAwsDynamoNoExpressionAttributeNames is returned when "expressionAttributeNames" is missing from the config. - ErrAwsDynamoNoExpressionAttributeNames = errors.New("no expressionAttributeNames given") - + ErrAwsDynamoNoTargetValue = errors.New("no targetValue given") // ErrAwsDynamoInvalidExpressionAttributeValues is returned when "expressionAttributeNames" is missing an invalid JSON. ErrAwsDynamoInvalidExpressionAttributeValues = errors.New("invalid expressionAttributeValues") - // ErrAwsDynamoNoExpressionAttributeValues is returned when "expressionAttributeValues" is missing from the config. ErrAwsDynamoNoExpressionAttributeValues = errors.New("no expressionAttributeValues given") - - // ErrAwsDynamoNoTargetValue is returned when "targetValue" is missing from the config. - ErrAwsDynamoNoTargetValue = errors.New("no targetValue given") + // ErrAwsDynamoInvalidExpressionAttributeNames is returned when "expressionAttributeNames" is an invalid JSON. + ErrAwsDynamoInvalidExpressionAttributeNames = errors.New("invalid expressionAttributeNames") + // ErrAwsDynamoEmptyExpressionAttributeNames is returned when "expressionAttributeNames" is empty. + ErrAwsDynamoEmptyExpressionAttributeNames = errors.New("empty map") + // ErrAwsDynamoNoExpressionAttributeNames is returned when "expressionAttributeNames" is missing from the config. + ErrAwsDynamoNoExpressionAttributeNames = errors.New("no expressionAttributeNames given") ) func parseAwsDynamoDBMetadata(config *scalersconfig.ScalerConfig) (*awsDynamoDBMetadata, error) { - meta := awsDynamoDBMetadata{} - - if val, ok := config.TriggerMetadata["tableName"]; ok && val != "" { - meta.tableName = val - } else { - return nil, ErrAwsDynamoNoTableName - } - - if val, ok := config.TriggerMetadata["awsRegion"]; ok && val != "" { - meta.awsRegion = val - } else { - return nil, ErrAwsDynamoNoAwsRegion - } - - if val, ok := config.TriggerMetadata["awsEndpoint"]; ok { - meta.awsEndpoint = val - } - - if val, ok := config.TriggerMetadata["indexName"]; ok { - meta.indexName = val - } - - if val, ok := config.TriggerMetadata["keyConditionExpression"]; ok && val != "" { - meta.keyConditionExpression = val - } else { - return nil, ErrAwsDynamoNoKeyConditionExpression + meta := &awsDynamoDBMetadata{} + if err := config.TypedConfig(meta); err != nil { + return nil, fmt.Errorf("error parsing DynamoDb metadata: %w", err) } - if val, ok := config.TriggerMetadata["expressionAttributeNames"]; ok && val != "" { names, err := json2Map(val) @@ -133,7 +93,6 @@ func parseAwsDynamoDBMetadata(config *scalersconfig.ScalerConfig) (*awsDynamoDBM } else { return nil, ErrAwsDynamoNoExpressionAttributeNames } - if val, ok := config.TriggerMetadata["expressionAttributeValues"]; ok && val != "" { values, err := json2DynamoMap(val) @@ -145,31 +104,10 @@ func parseAwsDynamoDBMetadata(config *scalersconfig.ScalerConfig) (*awsDynamoDBM } else { return nil, ErrAwsDynamoNoExpressionAttributeValues } - - if val, ok := config.TriggerMetadata["targetValue"]; ok && val != "" { - n, err := strconv.ParseInt(val, 10, 64) - if err != nil { - return nil, fmt.Errorf("error parsing metadata targetValue: %w", err) - } - - meta.targetValue = n - } else { - if config.AsMetricSource { - meta.targetValue = 0 - } else { - return nil, ErrAwsDynamoNoTargetValue - } - } - - if val, ok := config.TriggerMetadata["activationTargetValue"]; ok && val != "" { - n, err := strconv.ParseInt(val, 10, 64) - if err != nil { - return nil, fmt.Errorf("error parsing metadata activationTargetValue: %w", err) - } - - meta.activationTargetValue = n - } else { - meta.activationTargetValue = 0 + if meta.TargetValue == -1 && config.AsMetricSource { + meta.TargetValue = 0 + } else if meta.TargetValue == -1 && !config.AsMetricSource { + return nil, ErrAwsDynamoNoTargetValue } auth, err := awsutils.GetAwsAuthorization(config.TriggerUniqueKey, config.PodIdentity, config.TriggerMetadata, config.AuthParams, config.ResolvedEnv) @@ -181,20 +119,20 @@ func parseAwsDynamoDBMetadata(config *scalersconfig.ScalerConfig) (*awsDynamoDBM meta.triggerIndex = config.TriggerIndex meta.metricName = GenerateMetricNameWithIndex(config.TriggerIndex, - kedautil.NormalizeString(fmt.Sprintf("aws-dynamodb-%s", meta.tableName))) + kedautil.NormalizeString(fmt.Sprintf("aws-dynamodb-%s", meta.TableName))) - return &meta, nil + return meta, nil } func createDynamoDBClient(ctx context.Context, metadata *awsDynamoDBMetadata) (*dynamodb.Client, error) { - cfg, err := awsutils.GetAwsConfig(ctx, metadata.awsRegion, metadata.awsAuthorization) + cfg, err := awsutils.GetAwsConfig(ctx, metadata.AwsRegion, metadata.awsAuthorization) if err != nil { return nil, err } return dynamodb.NewFromConfig(*cfg, func(options *dynamodb.Options) { - if metadata.awsEndpoint != "" { - options.BaseEndpoint = aws.String(metadata.awsEndpoint) + if metadata.AwsEndpoint != "" { + options.BaseEndpoint = aws.String(metadata.AwsEndpoint) } }), nil } @@ -208,7 +146,7 @@ func (s *awsDynamoDBScaler) GetMetricsAndActivity(ctx context.Context, metricNam metric := GenerateMetricInMili(metricName, metricValue) - return []external_metrics.ExternalMetricValue{metric}, metricValue > float64(s.metadata.activationTargetValue), nil + return []external_metrics.ExternalMetricValue{metric}, metricValue > float64(s.metadata.ActivationTargetValue), nil } func (s *awsDynamoDBScaler) GetMetricSpecForScaling(context.Context) []v2.MetricSpec { @@ -216,7 +154,7 @@ func (s *awsDynamoDBScaler) GetMetricSpecForScaling(context.Context) []v2.Metric Metric: v2.MetricIdentifier{ Name: s.metadata.metricName, }, - Target: GetMetricTarget(s.metricType, s.metadata.targetValue), + Target: GetMetricTarget(s.metricType, s.metadata.TargetValue), } metricSpec := v2.MetricSpec{External: externalMetric, Type: externalMetricType} @@ -232,14 +170,14 @@ func (s *awsDynamoDBScaler) Close(context.Context) error { func (s *awsDynamoDBScaler) GetQueryMetrics(ctx context.Context) (float64, error) { dimensions := dynamodb.QueryInput{ - TableName: aws.String(s.metadata.tableName), - KeyConditionExpression: aws.String(s.metadata.keyConditionExpression), + TableName: aws.String(s.metadata.TableName), + KeyConditionExpression: aws.String(s.metadata.KeyConditionExpression), ExpressionAttributeNames: s.metadata.expressionAttributeNames, ExpressionAttributeValues: s.metadata.expressionAttributeValues, } - if s.metadata.indexName != "" { - dimensions.IndexName = aws.String(s.metadata.indexName) + if s.metadata.IndexName != "" { + dimensions.IndexName = aws.String(s.metadata.IndexName) } res, err := s.dbClient.Query(ctx, &dimensions) diff --git a/pkg/scalers/aws_dynamodb_scaler_test.go b/pkg/scalers/aws_dynamodb_scaler_test.go index a577f561e21..f6ec68314e8 100644 --- a/pkg/scalers/aws_dynamodb_scaler_test.go +++ b/pkg/scalers/aws_dynamodb_scaler_test.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "strconv" "testing" "github.com/aws/aws-sdk-go-v2/service/dynamodb" @@ -38,6 +37,17 @@ type parseDynamoDBMetadataTestData struct { expectedError error } +var ( + // ErrAwsDynamoNoTableName is returned when "tableName" is missing from the config. + ErrAwsDynamoNoTableName = errors.New("missing required parameter \"tableName\"") + + // ErrAwsDynamoNoAwsRegion is returned when "awsRegion" is missing from the config. + ErrAwsDynamoNoAwsRegion = errors.New("missing required parameter \"awsRegion\"") + + // ErrAwsDynamoNoKeyConditionExpression is returned when "keyConditionExpression" is missing from the config. + ErrAwsDynamoNoKeyConditionExpression = errors.New("missing required parameter \"keyConditionExpression\"") +) + var dynamoTestCases = []parseDynamoDBMetadataTestData{ { name: "no tableName given", @@ -104,7 +114,7 @@ var dynamoTestCases = []parseDynamoDBMetadataTestData{ "targetValue": "no-valid", }, authParams: map[string]string{}, - expectedError: strconv.ErrSyntax, + expectedError: errors.New("error parsing DynamoDb metadata: unable to set param \"targetValue\" value"), }, { name: "invalid activationTargetValue given", @@ -118,7 +128,7 @@ var dynamoTestCases = []parseDynamoDBMetadataTestData{ "activationTargetValue": "no-valid", }, authParams: map[string]string{}, - expectedError: strconv.ErrSyntax, + expectedError: errors.New("unable to set param \"activationTargetValue\""), }, { name: "malformed expressionAttributeNames", @@ -185,12 +195,12 @@ var dynamoTestCases = []parseDynamoDBMetadataTestData{ authParams: testAWSDynamoAuthentication, expectedError: nil, expectedMetadata: &awsDynamoDBMetadata{ - tableName: "test", - awsRegion: "eu-west-1", - keyConditionExpression: "#yr = :yyyy", + TableName: "test", + AwsRegion: "eu-west-1", + KeyConditionExpression: "#yr = :yyyy", expressionAttributeNames: map[string]string{"#yr": year}, expressionAttributeValues: map[string]types.AttributeValue{":yyyy": yearAttr}, - targetValue: 3, + TargetValue: 3, triggerIndex: 1, metricName: "s1-aws-dynamodb-test", awsAuthorization: awsutils.AuthorizationMetadata{ @@ -214,13 +224,13 @@ var dynamoTestCases = []parseDynamoDBMetadataTestData{ authParams: testAWSDynamoAuthentication, expectedError: nil, expectedMetadata: &awsDynamoDBMetadata{ - tableName: "test", - awsRegion: "eu-west-1", - awsEndpoint: "http://localhost:4566", - keyConditionExpression: "#yr = :yyyy", + TableName: "test", + AwsRegion: "eu-west-1", + AwsEndpoint: "http://localhost:4566", + KeyConditionExpression: "#yr = :yyyy", expressionAttributeNames: map[string]string{"#yr": year}, expressionAttributeValues: map[string]types.AttributeValue{":yyyy": yearAttr}, - targetValue: 3, + TargetValue: 3, triggerIndex: 1, metricName: "s1-aws-dynamodb-test", awsAuthorization: awsutils.AuthorizationMetadata{ @@ -244,13 +254,13 @@ var dynamoTestCases = []parseDynamoDBMetadataTestData{ authParams: testAWSDynamoAuthentication, expectedError: nil, expectedMetadata: &awsDynamoDBMetadata{ - tableName: "test", - awsRegion: "eu-west-1", - keyConditionExpression: "#yr = :yyyy", + TableName: "test", + AwsRegion: "eu-west-1", + KeyConditionExpression: "#yr = :yyyy", expressionAttributeNames: map[string]string{"#yr": year}, expressionAttributeValues: map[string]types.AttributeValue{":yyyy": yearAttr}, - activationTargetValue: 1, - targetValue: 3, + ActivationTargetValue: 1, + TargetValue: 3, triggerIndex: 1, metricName: "s1-aws-dynamodb-test", awsAuthorization: awsutils.AuthorizationMetadata{ @@ -274,13 +284,13 @@ var dynamoTestCases = []parseDynamoDBMetadataTestData{ authParams: testAWSDynamoAuthentication, expectedError: nil, expectedMetadata: &awsDynamoDBMetadata{ - tableName: "test", - awsRegion: "eu-west-1", - indexName: "test-index", - keyConditionExpression: "#yr = :yyyy", + TableName: "test", + AwsRegion: "eu-west-1", + IndexName: "test-index", + KeyConditionExpression: "#yr = :yyyy", expressionAttributeNames: map[string]string{"#yr": year}, expressionAttributeValues: map[string]types.AttributeValue{":yyyy": yearAttr}, - targetValue: 3, + TargetValue: 3, triggerIndex: 1, metricName: "s1-aws-dynamodb-test", awsAuthorization: awsutils.AuthorizationMetadata{ @@ -346,48 +356,48 @@ var yearAttr = &types.AttributeValueMemberN{Value: target} var awsDynamoDBGetMetricTestData = []awsDynamoDBMetadata{ { - tableName: "ValidTable", - awsRegion: "eu-west-1", - keyConditionExpression: "#yr = :yyyy", + TableName: "ValidTable", + AwsRegion: "eu-west-1", + KeyConditionExpression: "#yr = :yyyy", expressionAttributeNames: map[string]string{"#yr": year}, expressionAttributeValues: map[string]types.AttributeValue{":yyyy": yearAttr}, - targetValue: 3, + TargetValue: 3, }, { - tableName: testAWSDynamoErrorTable, - awsRegion: "eu-west-1", - keyConditionExpression: "#yr = :yyyy", + TableName: testAWSDynamoErrorTable, + AwsRegion: "eu-west-1", + KeyConditionExpression: "#yr = :yyyy", expressionAttributeNames: map[string]string{"#yr": year}, expressionAttributeValues: map[string]types.AttributeValue{":yyyy": yearAttr}, - targetValue: 3, + TargetValue: 3, }, { - tableName: testAWSDynamoNoValueTable, - awsRegion: "eu-west-1", - keyConditionExpression: "#yr = :yyyy", + TableName: testAWSDynamoNoValueTable, + AwsRegion: "eu-west-1", + KeyConditionExpression: "#yr = :yyyy", expressionAttributeNames: map[string]string{"#yr": year}, expressionAttributeValues: map[string]types.AttributeValue{":yyyy": yearAttr}, - targetValue: 3, + TargetValue: 3, }, { - tableName: testAWSDynamoIndexTable, - awsRegion: "eu-west-1", - indexName: "test-index", - keyConditionExpression: "#yr = :yyyy", + TableName: testAWSDynamoIndexTable, + AwsRegion: "eu-west-1", + IndexName: "test-index", + KeyConditionExpression: "#yr = :yyyy", expressionAttributeNames: map[string]string{"#yr": year}, expressionAttributeValues: map[string]types.AttributeValue{":yyyy": yearAttr}, - activationTargetValue: 3, - targetValue: 3, + ActivationTargetValue: 3, + TargetValue: 3, }, } func TestDynamoGetMetrics(t *testing.T) { for _, meta := range awsDynamoDBGetMetricTestData { - t.Run(meta.tableName, func(t *testing.T) { + t.Run(meta.TableName, func(t *testing.T) { scaler := awsDynamoDBScaler{"", &meta, &mockDynamoDB{}, logr.Discard()} value, _, err := scaler.GetMetricsAndActivity(context.Background(), "aws-dynamodb") - switch meta.tableName { + switch meta.TableName { case testAWSDynamoErrorTable: assert.EqualError(t, err, "error", "expect error because of dynamodb api error") case testAWSDynamoNoValueTable: @@ -403,11 +413,11 @@ func TestDynamoGetMetrics(t *testing.T) { func TestDynamoGetQueryMetrics(t *testing.T) { for _, meta := range awsDynamoDBGetMetricTestData { - t.Run(meta.tableName, func(t *testing.T) { + t.Run(meta.TableName, func(t *testing.T) { scaler := awsDynamoDBScaler{"", &meta, &mockDynamoDB{}, logr.Discard()} value, err := scaler.GetQueryMetrics(context.Background()) - switch meta.tableName { + switch meta.TableName { case testAWSDynamoErrorTable: assert.EqualError(t, err, "error", "expect error because of dynamodb api error") case testAWSDynamoNoValueTable: @@ -423,11 +433,11 @@ func TestDynamoGetQueryMetrics(t *testing.T) { func TestDynamoIsActive(t *testing.T) { for _, meta := range awsDynamoDBGetMetricTestData { - t.Run(meta.tableName, func(t *testing.T) { + t.Run(meta.TableName, func(t *testing.T) { scaler := awsDynamoDBScaler{"", &meta, &mockDynamoDB{}, logr.Discard()} _, value, err := scaler.GetMetricsAndActivity(context.Background(), "aws-dynamodb") - switch meta.tableName { + switch meta.TableName { case testAWSDynamoErrorTable: assert.EqualError(t, err, "error", "expect error because of dynamodb api error") case testAWSDynamoNoValueTable: diff --git a/pkg/scalers/scalersconfig/typed_config.go b/pkg/scalers/scalersconfig/typed_config.go index b06e1478b88..1833c49d628 100644 --- a/pkg/scalers/scalersconfig/typed_config.go +++ b/pkg/scalers/scalersconfig/typed_config.go @@ -62,7 +62,8 @@ const ( // separators for map and slice elements const ( - elemSeparator = "," + elemSeparator = "," + // TODO: support custom separator https://github.com/kedacore/keda/pull/5961/files#r1694991497 elemKeyValSeparator = "=" )