From 6314664806cb80637c1661e07b339b153ddee7c7 Mon Sep 17 00:00:00 2001 From: Omer Aplatony Date: Thu, 2 Jan 2025 17:33:42 +0200 Subject: [PATCH] refactor gcp stackdriver scaler Signed-off-by: Omer Aplatony --- pkg/scalers/gcp_stackdriver_scaler.go | 88 ++------ pkg/scalers/gcp_stackdriver_scaler_test.go | 247 +++++++++++++++------ 2 files changed, 200 insertions(+), 135 deletions(-) diff --git a/pkg/scalers/gcp_stackdriver_scaler.go b/pkg/scalers/gcp_stackdriver_scaler.go index 7fee64ffa8a..d39c3f2cced 100644 --- a/pkg/scalers/gcp_stackdriver_scaler.go +++ b/pkg/scalers/gcp_stackdriver_scaler.go @@ -15,10 +15,6 @@ import ( kedautil "github.com/kedacore/keda/v2/pkg/util" ) -const ( - defaultStackdriverTargetValue = 5 -) - type stackdriverScaler struct { client *gcp.StackDriverClient metricType v2.MetricTargetType @@ -27,13 +23,13 @@ type stackdriverScaler struct { } type stackdriverMetadata struct { - projectID string - filter string - targetValue float64 - activationTargetValue float64 + ProjectID string `keda:"name=projectID, order=triggerMetadata"` + Filter string `keda:"name=filter, order=triggerMetadata"` + TargetValue float64 `keda:"name=targetValue, order=triggerMetadata, optional, default=5"` + ActivationTargetValue float64 `keda:"name=activationTargetValue, order=triggerMetadata, optional, default=0"` metricName string - valueIfNull *float64 - filterDuration int64 + ValueIfNull *float64 `keda:"name=valueIfNull, order=triggerMetadata, optional"` + FilterDuration int64 `keda:"name=filterDuration, order=triggerMetadata, optional"` gcpAuthorization *gcp.AuthorizationMetadata aggregation *monitoringpb.Aggregation @@ -68,67 +64,15 @@ func NewStackdriverScaler(ctx context.Context, config *scalersconfig.ScalerConfi } func parseStackdriverMetadata(config *scalersconfig.ScalerConfig, logger logr.Logger) (*stackdriverMetadata, error) { - meta := stackdriverMetadata{} - meta.targetValue = defaultStackdriverTargetValue - - if val, ok := config.TriggerMetadata["projectId"]; ok { - if val == "" { - return nil, fmt.Errorf("no projectId name given") - } - - meta.projectID = val - } else { - return nil, fmt.Errorf("no projectId name given") - } + meta := &stackdriverMetadata{} - if val, ok := config.TriggerMetadata["filter"]; ok { - if val == "" { - return nil, fmt.Errorf("no filter given") - } - - meta.filter = val - } else { - return nil, fmt.Errorf("no filter given") + if err := config.TypedConfig(meta); err != nil { + return nil, fmt.Errorf("error parsing Stackdriver metadata: %w", err) } - name := kedautil.NormalizeString(fmt.Sprintf("gcp-stackdriver-%s", meta.projectID)) + name := kedautil.NormalizeString(fmt.Sprintf("gcp-stackdriver-%s", meta.ProjectID)) meta.metricName = GenerateMetricNameWithIndex(config.TriggerIndex, name) - if val, ok := config.TriggerMetadata["targetValue"]; ok { - targetValue, err := strconv.ParseFloat(val, 64) - if err != nil { - logger.Error(err, "Error parsing targetValue") - return nil, fmt.Errorf("error parsing targetValue: %w", err) - } - - meta.targetValue = targetValue - } - - meta.activationTargetValue = 0 - if val, ok := config.TriggerMetadata["activationTargetValue"]; ok { - activationTargetValue, err := strconv.ParseFloat(val, 64) - if err != nil { - return nil, fmt.Errorf("activationTargetValue parsing error %w", err) - } - meta.activationTargetValue = activationTargetValue - } - - if val, ok := config.TriggerMetadata["valueIfNull"]; ok && val != "" { - valueIfNull, err := strconv.ParseFloat(val, 64) - if err != nil { - return nil, fmt.Errorf("valueIfNull parsing error %w", err) - } - meta.valueIfNull = &valueIfNull - } - - if val, ok := config.TriggerMetadata["filterDuration"]; ok { - filterDuration, err := strconv.ParseInt(val, 10, 64) - if err != nil { - return nil, fmt.Errorf("filterDuration parsing error %w", err) - } - meta.filterDuration = filterDuration - } - auth, err := gcp.GetGCPAuthorization(config) if err != nil { return nil, err @@ -141,7 +85,7 @@ func parseStackdriverMetadata(config *scalersconfig.ScalerConfig, logger logr.Lo } meta.aggregation = aggregation - return &meta, nil + return meta, nil } func parseAggregation(config *scalersconfig.ScalerConfig, logger logr.Logger) (*monitoringpb.Aggregation, error) { @@ -199,7 +143,7 @@ func (s *stackdriverScaler) GetMetricSpecForScaling(context.Context) []v2.Metric Metric: v2.MetricIdentifier{ Name: s.metadata.metricName, }, - Target: GetMetricTargetMili(s.metricType, s.metadata.targetValue), + Target: GetMetricTargetMili(s.metricType, s.metadata.TargetValue), } // Create the metric spec for the HPA @@ -221,17 +165,17 @@ func (s *stackdriverScaler) GetMetricsAndActivity(ctx context.Context, metricNam metric := GenerateMetricInMili(metricName, value) - return []external_metrics.ExternalMetricValue{metric}, value > s.metadata.activationTargetValue, nil + return []external_metrics.ExternalMetricValue{metric}, value > s.metadata.ActivationTargetValue, nil } // getMetrics gets metric type value from stackdriver api func (s *stackdriverScaler) getMetrics(ctx context.Context) (float64, error) { - val, err := s.client.GetMetrics(ctx, s.metadata.filter, s.metadata.projectID, s.metadata.aggregation, s.metadata.valueIfNull, s.metadata.filterDuration) + val, err := s.client.GetMetrics(ctx, s.metadata.Filter, s.metadata.ProjectID, s.metadata.aggregation, s.metadata.ValueIfNull, s.metadata.FilterDuration) if err == nil { s.logger.V(1).Info( fmt.Sprintf("Getting metrics for project %s, filter %s and aggregation %v. Result: %f", - s.metadata.projectID, - s.metadata.filter, + s.metadata.ProjectID, + s.metadata.Filter, s.metadata.aggregation, val)) } diff --git a/pkg/scalers/gcp_stackdriver_scaler_test.go b/pkg/scalers/gcp_stackdriver_scaler_test.go index bcf078749c3..f4346a42420 100644 --- a/pkg/scalers/gcp_stackdriver_scaler_test.go +++ b/pkg/scalers/gcp_stackdriver_scaler_test.go @@ -2,10 +2,11 @@ package scalers import ( "context" + "reflect" "testing" "github.com/go-logr/logr" - + "github.com/kedacore/keda/v2/pkg/scalers/gcp" "github.com/kedacore/keda/v2/pkg/scalers/scalersconfig" ) @@ -13,85 +14,205 @@ var testStackdriverResolvedEnv = map[string]string{ "SAMPLE_CREDS": "{}", } +func float64Ptr(v float64) *float64 { + return &v +} + type parseStackdriverMetadataTestData struct { authParams map[string]string metadata map[string]string isError bool -} - -type gcpStackdriverMetricIdentifier struct { - metadataTestData *parseStackdriverMetadataTestData - triggerIndex int - name string + expected *stackdriverMetadata + comment string } var sdFilter = "metric.type=\"storage.googleapis.com/storage/object_count\" resource.type=\"gcs_bucket\"" var testStackdriverMetadata = []parseStackdriverMetadataTestData{ - {map[string]string{}, map[string]string{}, true}, - // all properly formed - {nil, map[string]string{"projectId": "myProject", "filter": sdFilter, "targetValue": "7", "credentialsFromEnv": "SAMPLE_CREDS", "activationTargetValue": "5"}, false}, - // all required properly formed - {nil, map[string]string{"projectId": "myProject", "filter": sdFilter, "credentialsFromEnv": "SAMPLE_CREDS"}, false}, - // missing projectId - {nil, map[string]string{"filter": sdFilter, "targetValue": "7", "credentialsFromEnv": "SAMPLE_CREDS"}, true}, - // missing filter - {nil, map[string]string{"projectId": "myProject", "targetValue": "7", "credentialsFromEnv": "SAMPLE_CREDS"}, true}, - // missing credentials - {nil, map[string]string{"projectId": "myProject", "filter": sdFilter, "targetValue": "7"}, true}, - // malformed targetValue - {nil, map[string]string{"projectId": "myProject", "filter": sdFilter, "targetValue": "aa", "credentialsFromEnv": "SAMPLE_CREDS"}, true}, - // malformed activationTargetValue - {nil, map[string]string{"projectId": "myProject", "filter": sdFilter, "credentialsFromEnv": "SAMPLE_CREDS", "activationTargetValue": "a"}, true}, - // Credentials from AuthParams - {map[string]string{"GoogleApplicationCredentials": "Creds"}, map[string]string{"projectId": "myProject", "filter": sdFilter}, false}, - // Credentials from AuthParams with empty creds - {map[string]string{"GoogleApplicationCredentials": ""}, map[string]string{"projectId": "myProject", "filter": sdFilter}, true}, - // With aggregation info - {nil, map[string]string{"projectId": "myProject", "filter": sdFilter, "credentialsFromEnv": "SAMPLE_CREDS", "alignmentPeriodSeconds": "120", "alignmentAligner": "sum", "alignmentReducer": "percentile_99"}, false}, - // With minimal aggregation info - {nil, map[string]string{"projectId": "myProject", "filter": sdFilter, "credentialsFromEnv": "SAMPLE_CREDS", "alignmentPeriodSeconds": "120"}, false}, - // With too short alignment period - {nil, map[string]string{"projectId": "myProject", "filter": sdFilter, "credentialsFromEnv": "SAMPLE_CREDS", "alignmentPeriodSeconds": "30"}, true}, - // With bad alignment period - {nil, map[string]string{"projectId": "myProject", "filter": sdFilter, "credentialsFromEnv": "SAMPLE_CREDS", "alignmentPeriodSeconds": "a"}, true}, - // properly formed float targetValue and activationTargetValue - {nil, map[string]string{"projectId": "myProject", "filter": sdFilter, "credentialsFromEnv": "SAMPLE_CREDS", "targetValue": "1.1", "activationTargetValue": "2.1"}, false}, - // properly formed float valueIfNull - {nil, map[string]string{"projectId": "myProject", "filter": sdFilter, "credentialsFromEnv": "SAMPLE_CREDS", "targetValue": "1.1", "activationTargetValue": "2.1", "valueIfNull": "1.0"}, false}, - // With bad valueIfNull - {nil, map[string]string{"projectId": "myProject", "filter": sdFilter, "credentialsFromEnv": "SAMPLE_CREDS", "targetValue": "1.1", "activationTargetValue": "2.1", "valueIfNull": "toto"}, true}, -} -var gcpStackdriverMetricIdentifiers = []gcpStackdriverMetricIdentifier{ - {&testStackdriverMetadata[1], 0, "s0-gcp-stackdriver-myProject"}, - {&testStackdriverMetadata[1], 1, "s1-gcp-stackdriver-myProject"}, + { + authParams: map[string]string{}, + metadata: map[string]string{}, + isError: true, + expected: nil, + comment: "error case - empty metadata", + }, + { + authParams: nil, + metadata: map[string]string{ + "projectID": "myProject", + "filter": sdFilter, + "targetValue": "7", + "credentialsFromEnv": "SAMPLE_CREDS", + "activationTargetValue": "5", + }, + isError: false, + expected: &stackdriverMetadata{ + ProjectID: "myProject", + Filter: sdFilter, + TargetValue: 7, + ActivationTargetValue: 5, + metricName: "s0-gcp-stackdriver-myProject", + gcpAuthorization: &gcp.AuthorizationMetadata{ + GoogleApplicationCredentials: "{}", + PodIdentityProviderEnabled: false, + }, + }, + comment: "all properly formed", + }, + { + authParams: nil, + metadata: map[string]string{ + "projectID": "myProject", + "filter": sdFilter, + "credentialsFromEnv": "SAMPLE_CREDS", + }, + isError: false, + expected: &stackdriverMetadata{ + ProjectID: "myProject", + Filter: sdFilter, + TargetValue: 5, + ActivationTargetValue: 0, + metricName: "s0-gcp-stackdriver-myProject", + gcpAuthorization: &gcp.AuthorizationMetadata{ + GoogleApplicationCredentials: "{}", + PodIdentityProviderEnabled: false, + }, + }, + comment: "required fields only with defaults", + }, + { + authParams: nil, + metadata: map[string]string{ + "projectId": "myProject", + "filter": sdFilter, + "credentialsFromEnv": "SAMPLE_CREDS", + "alignmentPeriodSeconds": "120", + "alignmentAligner": "sum", + "alignmentReducer": "percentile_99", + }, + isError: true, + expected: nil, + comment: "with aggregation configuration", + }, + { + authParams: nil, + metadata: map[string]string{ + "projectId": "myProject", + "filter": sdFilter, + "credentialsFromEnv": "SAMPLE_CREDS", + "valueIfNull": "1.5", + }, + isError: true, + expected: nil, + comment: "with valueIfNull configuration", + }, + { + authParams: nil, + metadata: map[string]string{ + "filter": sdFilter, + "credentialsFromEnv": "SAMPLE_CREDS", + }, + isError: true, + expected: nil, + comment: "error case - missing projectId", + }, + { + authParams: nil, + metadata: map[string]string{ + "projectId": "myProject", + "credentialsFromEnv": "SAMPLE_CREDS", + }, + isError: true, + expected: nil, + comment: "error case - missing filter", + }, + { + authParams: nil, + metadata: map[string]string{ + "projectID": "myProject", + "filter": sdFilter, + }, + isError: true, + expected: nil, + comment: "error case - missing credentials", + }, } func TestStackdriverParseMetadata(t *testing.T) { for _, testData := range testStackdriverMetadata { - _, err := parseStackdriverMetadata(&scalersconfig.ScalerConfig{AuthParams: testData.authParams, TriggerMetadata: testData.metadata, ResolvedEnv: testStackdriverResolvedEnv}, logr.Discard()) - if err != nil && !testData.isError { - t.Error("Expected success but got error", err) - } - if testData.isError && err == nil { - t.Error("Expected error but got success") - } + t.Run(testData.comment, func(t *testing.T) { + metadata, err := parseStackdriverMetadata(&scalersconfig.ScalerConfig{ + AuthParams: testData.authParams, + TriggerMetadata: testData.metadata, + ResolvedEnv: testStackdriverResolvedEnv, + }, logr.Discard()) + + if err != nil && !testData.isError { + t.Errorf("Expected success but got error") + } + + if testData.isError && err == nil { + t.Errorf("Expected error but got success") + } + + if !testData.isError && !reflect.DeepEqual(testData.expected, metadata) { + t.Fatalf("Expected %#v but got %+#v", testData.expected, metadata) + } + }) } } +var gcpStackdriverMetricIdentifiers = []struct { + comment string + triggerIndex int + metadata map[string]string + expectedName string +}{ + { + comment: "basic metric name", + triggerIndex: 0, + metadata: map[string]string{ + "projectID": "myProject", + "filter": sdFilter, + "credentialsFromEnv": "SAMPLE_CREDS", + }, + expectedName: "s0-gcp-stackdriver-myProject", + }, + { + comment: "metric name with different index", + triggerIndex: 1, + metadata: map[string]string{ + "projectID": "myProject", + "filter": sdFilter, + "credentialsFromEnv": "SAMPLE_CREDS", + }, + expectedName: "s1-gcp-stackdriver-myProject", + }, +} + func TestGcpStackdriverGetMetricSpecForScaling(t *testing.T) { - for _, testData := range gcpStackdriverMetricIdentifiers { - meta, err := parseStackdriverMetadata(&scalersconfig.ScalerConfig{TriggerMetadata: testData.metadataTestData.metadata, ResolvedEnv: testStackdriverResolvedEnv, TriggerIndex: testData.triggerIndex}, logr.Discard()) - if err != nil { - t.Fatal("Could not parse metadata:", err) - } - mockGcpStackdriverScaler := stackdriverScaler{nil, "", meta, logr.Discard()} - - metricSpec := mockGcpStackdriverScaler.GetMetricSpecForScaling(context.Background()) - metricName := metricSpec[0].External.Metric.Name - if metricName != testData.name { - t.Error("Wrong External metric source name:", metricName) - } + for _, test := range gcpStackdriverMetricIdentifiers { + t.Run(test.comment, func(t *testing.T) { + meta, err := parseStackdriverMetadata(&scalersconfig.ScalerConfig{ + TriggerMetadata: test.metadata, + ResolvedEnv: testStackdriverResolvedEnv, + TriggerIndex: test.triggerIndex, + }, logr.Discard()) + if err != nil { + t.Fatal("Could not parse metadata:", err) + } + + mockScaler := stackdriverScaler{ + metadata: meta, + logger: logr.Discard(), + } + + metricSpec := mockScaler.GetMetricSpecForScaling(context.Background()) + metricName := metricSpec[0].External.Metric.Name + if metricName != test.expectedName { + t.Errorf("Wrong metric name - got %s, want %s", metricName, test.expectedName) + } + }) } }