From ba7fa8b656c4c347d1239fdb1a08112c7c8ee145 Mon Sep 17 00:00:00 2001 From: rickbrouwer Date: Mon, 5 Aug 2024 08:51:19 +0200 Subject: [PATCH] refactor: Remove and deprecate unused variables in IBM MQ scaler Signed-off-by: Rick Brouwer --- CHANGELOG.md | 4 +- pkg/scalers/ibmmq_scaler.go | 217 ++++++++++-------------------- pkg/scalers/ibmmq_scaler_test.go | 54 ++++---- tests/scalers/ibmmq/ibmmq_test.go | 3 +- 4 files changed, 100 insertions(+), 178 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 311e05cd298..6c9416ef90f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,11 +74,13 @@ Here is an overview of all new **experimental** features: ### Deprecations +- TODO ([#XXX](https://github.com/kedacore/keda/issues/XXX)) + You can find all deprecations in [this overview](https://github.com/kedacore/keda/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc+label%3Abreaking-change) and [join the discussion here](https://github.com/kedacore/keda/discussions/categories/deprecations). New deprecation(s): -- TODO ([#XXX](https://github.com/kedacore/keda/issues/XXX)) +- IBM MQ Scaler: Remove and deprecate unused variables in IBM MQ scaler ([#6033](https://github.com/kedacore/keda/issues/6033)) ### Breaking Changes diff --git a/pkg/scalers/ibmmq_scaler.go b/pkg/scalers/ibmmq_scaler.go index efd83d8c968..0b0b4c893f4 100644 --- a/pkg/scalers/ibmmq_scaler.go +++ b/pkg/scalers/ibmmq_scaler.go @@ -8,9 +8,7 @@ import ( "io" "net/http" "net/url" - "strconv" "strings" - "time" "github.com/go-logr/logr" v2 "k8s.io/api/autoscaling/v2" @@ -20,39 +18,28 @@ import ( kedautil "github.com/kedacore/keda/v2/pkg/util" ) -// Default variables and settings -const ( - defaultTargetQueueDepth = 20 - defaultTLSDisabled = false -) - -// IBMMQScaler assigns struct data pointer to metadata variable -type IBMMQScaler struct { - metricType v2.MetricTargetType - metadata *IBMMQMetadata - defaultHTTPTimeout time.Duration - httpClient *http.Client - logger logr.Logger +type ibmmqScaler struct { + metricType v2.MetricTargetType + metadata ibmmqMetadata + httpClient *http.Client + logger logr.Logger } -// IBMMQMetadata Metadata used by KEDA to query IBM MQ queue depth and scale -type IBMMQMetadata struct { - host string - queueManager string - queueName string - username string - password string - queueDepth int64 - activationQueueDepth int64 - tlsDisabled bool - triggerIndex int - - // TLS - ca string - cert string - key string - keyPassword string - unsafeSsl bool +type ibmmqMetadata struct { + Host string `keda:"name=host, order=triggerMetadata"` + QueueName string `keda:"name=queueName, order=triggerMetadata"` + QueueDepth int64 `keda:"name=queueDepth, order=triggerMetadata, default=20"` + ActivationQueueDepth int64 `keda:"name=activationQueueDepth, order=triggerMetadata, default=0"` + Username string `keda:"name=username, order=authParams;resolvedEnv;triggerMetadata"` + Password string `keda:"name=password, order=authParams;resolvedEnv;triggerMetadata"` + UnsafeSsl bool `keda:"name=unsafeSsl, order=triggerMetadata, default=false"` + TLS bool `keda:"name=tls, order=triggerMetadata, default=false"` // , deprecated=use unsafeSsl instead + CA string `keda:"name=ca, order=authParams, optional"` + Cert string `keda:"name=cert, order=authParams, optional"` + Key string `keda:"name=key, order=authParams, optional"` + KeyPassword string `keda:"name=keyPassword, order=authParams, optional"` + + triggerIndex int } // CommandResponse Full structured response from MQ admin REST query @@ -71,142 +58,79 @@ type Parameters struct { Curdepth int `json:"curdepth"` } -// NewIBMMQScaler creates a new IBM MQ scaler +func (m *ibmmqMetadata) Validate() error { + _, err := url.ParseRequestURI(m.Host) + if err != nil { + return fmt.Errorf("invalid URL: %w", err) + } + + if (m.Cert == "") != (m.Key == "") { + return fmt.Errorf("both cert and key must be provided when using TLS") + } + + // TODO: DEPRECATED to be removed in v2.18 + if m.TLS && m.UnsafeSsl { + return fmt.Errorf("'tls' and 'unsafeSsl' are both specified. Please use only 'unsafeSsl'") + } + + return nil +} + func NewIBMMQScaler(config *scalersconfig.ScalerConfig) (Scaler, error) { metricType, err := GetMetricTargetType(config) if err != nil { return nil, fmt.Errorf("error getting scaler metric type: %w", err) } + logger := InitializeLogger(config, "ibm_mq_scaler") + meta, err := parseIBMMQMetadata(config) if err != nil { return nil, fmt.Errorf("error parsing IBM MQ metadata: %w", err) } - httpClient := kedautil.CreateHTTPClient(config.GlobalHTTPTimeout, meta.tlsDisabled) + // TODO: DEPRECATED to be removed in v2.18 + if meta.TLS { + logger.Info("The 'tls' setting is DEPRECATED and will be removed in v2.18 - Use 'unsafeSsl' instead") + meta.UnsafeSsl = meta.TLS + } + + httpClient := kedautil.CreateHTTPClient(config.GlobalHTTPTimeout, meta.UnsafeSsl) - // Configure TLS if cert and key are specified - if meta.cert != "" && meta.key != "" { - tlsConfig, err := kedautil.NewTLSConfigWithPassword(meta.cert, meta.key, meta.keyPassword, meta.ca, meta.unsafeSsl) + if meta.Cert != "" && meta.Key != "" { + tlsConfig, err := kedautil.NewTLSConfigWithPassword(meta.Cert, meta.Key, meta.KeyPassword, meta.CA, meta.UnsafeSsl) if err != nil { return nil, err } httpClient.Transport = kedautil.CreateHTTPTransportWithTLSConfig(tlsConfig) } - return &IBMMQScaler{ - metricType: metricType, - metadata: meta, - defaultHTTPTimeout: config.GlobalHTTPTimeout, - httpClient: httpClient, - logger: InitializeLogger(config, "ibm_mq_scaler"), + return &ibmmqScaler{ + metricType: metricType, + metadata: meta, + httpClient: httpClient, + logger: logger, }, nil } -// Close closes and returns nil -func (s *IBMMQScaler) Close(context.Context) error { +func (s *ibmmqScaler) Close(context.Context) error { if s.httpClient != nil { s.httpClient.CloseIdleConnections() } return nil } -// parseIBMMQMetadata checks the existence of and validates the MQ connection data provided -func parseIBMMQMetadata(config *scalersconfig.ScalerConfig) (*IBMMQMetadata, error) { - meta := IBMMQMetadata{} - - if val, ok := config.TriggerMetadata["host"]; ok { - _, err := url.ParseRequestURI(val) - if err != nil { - return nil, fmt.Errorf("invalid URL: %w", err) - } - meta.host = val - } else { - return nil, fmt.Errorf("no host URI given") - } - - if val, ok := config.TriggerMetadata["queueManager"]; ok { - meta.queueManager = val - } else { - return nil, fmt.Errorf("no queue manager given") - } - - if val, ok := config.TriggerMetadata["queueName"]; ok { - meta.queueName = val - } else { - return nil, fmt.Errorf("no queue name given") +func parseIBMMQMetadata(config *scalersconfig.ScalerConfig) (ibmmqMetadata, error) { + meta := ibmmqMetadata{triggerIndex: config.TriggerIndex} + if err := config.TypedConfig(&meta); err != nil { + return meta, err } - - if val, ok := config.TriggerMetadata["queueDepth"]; ok && val != "" { - queueDepth, err := strconv.ParseInt(val, 10, 64) - if err != nil { - return nil, fmt.Errorf("invalid queueDepth - must be an integer") - } - meta.queueDepth = queueDepth - } else { - fmt.Println("No target depth defined - setting default") - meta.queueDepth = defaultTargetQueueDepth - } - - meta.activationQueueDepth = 0 - if val, ok := config.TriggerMetadata["activationQueueDepth"]; ok && val != "" { - activationQueueDepth, err := strconv.ParseInt(val, 10, 64) - if err != nil { - return nil, fmt.Errorf("invalid activationQueueDepth - must be an integer") - } - meta.activationQueueDepth = activationQueueDepth - } - - if val, ok := config.TriggerMetadata["tls"]; ok { - tlsDisabled, err := strconv.ParseBool(val) - if err != nil { - return nil, fmt.Errorf("invalid tls setting: %w", err) - } - meta.tlsDisabled = tlsDisabled - } else { - fmt.Println("No tls setting defined - setting default") - meta.tlsDisabled = defaultTLSDisabled - } - - if val, ok := config.AuthParams["username"]; ok && val != "" { - meta.username = val - } else if val, ok := config.TriggerMetadata["usernameFromEnv"]; ok && val != "" { - meta.username = config.ResolvedEnv[val] - } else { - return nil, fmt.Errorf("no username given") - } - - if val, ok := config.AuthParams["password"]; ok && val != "" { - meta.password = val - } else if val, ok := config.TriggerMetadata["passwordFromEnv"]; ok && val != "" { - meta.password = config.ResolvedEnv[val] - } else { - return nil, fmt.Errorf("no password given") - } - - // TLS config (optional) - meta.ca = config.AuthParams["ca"] - meta.cert = config.AuthParams["cert"] - meta.key = config.AuthParams["key"] - meta.keyPassword = config.AuthParams["keyPassword"] - - meta.unsafeSsl = false - if val, ok := config.TriggerMetadata["unsafeSsl"]; ok { - boolVal, err := strconv.ParseBool(val) - if err != nil { - return nil, fmt.Errorf("failed to parse unsafeSsl value. Must be either true or false") - } - meta.unsafeSsl = boolVal - } - - meta.triggerIndex = config.TriggerIndex - return &meta, nil + return meta, nil } -// getQueueDepthViaHTTP returns the depth of the MQ Queue from the Admin endpoint -func (s *IBMMQScaler) getQueueDepthViaHTTP(ctx context.Context) (int64, error) { - queue := s.metadata.queueName - url := s.metadata.host +func (s *ibmmqScaler) getQueueDepthViaHTTP(ctx context.Context) (int64, error) { + queue := s.metadata.QueueName + url := s.metadata.Host var requestJSON = []byte(`{"type": "runCommandJSON", "command": "display", "qualifier": "qlocal", "name": "` + queue + `", "responseParameters" : ["CURDEPTH"]}`) req, err := http.NewRequestWithContext(ctx, "POST", url, bytes.NewBuffer(requestJSON)) @@ -216,7 +140,7 @@ func (s *IBMMQScaler) getQueueDepthViaHTTP(ctx context.Context) (int64, error) { req.Header.Set("ibm-mq-rest-csrf-token", "value") req.Header.Set("Content-Type", "application/json") - req.SetBasicAuth(s.metadata.username, s.metadata.password) + req.SetBasicAuth(s.metadata.Username, s.metadata.Password) resp, err := s.httpClient.Do(req) if err != nil { @@ -251,20 +175,19 @@ func (s *IBMMQScaler) getQueueDepthViaHTTP(ctx context.Context) (int64, error) { return int64(response.CommandResponse[0].Parameters.Curdepth), nil } -// GetMetricSpecForScaling returns the MetricSpec for the Horizontal Pod Autoscaler -func (s *IBMMQScaler) GetMetricSpecForScaling(context.Context) []v2.MetricSpec { +func (s *ibmmqScaler) GetMetricSpecForScaling(context.Context) []v2.MetricSpec { + metricName := kedautil.NormalizeString(fmt.Sprintf("ibmmq-%s", s.metadata.QueueName)) externalMetric := &v2.ExternalMetricSource{ Metric: v2.MetricIdentifier{ - Name: GenerateMetricNameWithIndex(s.metadata.triggerIndex, kedautil.NormalizeString(fmt.Sprintf("ibmmq-%s", s.metadata.queueName))), + Name: GenerateMetricNameWithIndex(s.metadata.triggerIndex, metricName), }, - Target: GetMetricTarget(s.metricType, s.metadata.queueDepth), + Target: GetMetricTarget(s.metricType, s.metadata.QueueDepth), } metricSpec := v2.MetricSpec{External: externalMetric, Type: externalMetricType} return []v2.MetricSpec{metricSpec} } -// GetMetricsAndActivity returns value for a supported metric and an error if there is a problem getting the metric -func (s *IBMMQScaler) GetMetricsAndActivity(ctx context.Context, metricName string) ([]external_metrics.ExternalMetricValue, bool, error) { +func (s *ibmmqScaler) GetMetricsAndActivity(ctx context.Context, metricName string) ([]external_metrics.ExternalMetricValue, bool, error) { queueDepth, err := s.getQueueDepthViaHTTP(ctx) if err != nil { return []external_metrics.ExternalMetricValue{}, false, fmt.Errorf("error inspecting IBM MQ queue depth: %w", err) @@ -272,5 +195,5 @@ func (s *IBMMQScaler) GetMetricsAndActivity(ctx context.Context, metricName stri metric := GenerateMetricInMili(metricName, float64(queueDepth)) - return []external_metrics.ExternalMetricValue{metric}, queueDepth > s.metadata.activationQueueDepth, nil + return []external_metrics.ExternalMetricValue{metric}, queueDepth > s.metadata.ActivationQueueDepth, nil } diff --git a/pkg/scalers/ibmmq_scaler_test.go b/pkg/scalers/ibmmq_scaler_test.go index c3e2430043f..30a7fc4b132 100644 --- a/pkg/scalers/ibmmq_scaler_test.go +++ b/pkg/scalers/ibmmq_scaler_test.go @@ -7,7 +7,6 @@ import ( "net/http" "net/http/httptest" "testing" - "time" "github.com/stretchr/testify/assert" @@ -16,8 +15,9 @@ import ( // Test host URLs for validation const ( - testValidMQQueueURL = "https://qmtest.qm2.eu-gb.mq.appdomain.cloud/ibmmq/rest/v2/admin/action/qmgr/QM1/mqsc" - testInvalidMQQueueURL = "testInvalidURL.com" + testValidMQQueueURL = "https://qmtest.qm2.eu-gb.mq.appdomain.cloud/ibmmq/rest/v2/admin/action/qmgr/QM1/mqsc" + testInvalidMQQueueURL = "testInvalidURL.com" + defaultTargetQueueDepth = 20 ) // Test data struct used for TestIBMMQParseMetadata @@ -50,29 +50,29 @@ var testIBMMQMetadata = []parseIBMMQMetadataTestData{ // Nothing passed {map[string]string{}, true, map[string]string{}}, // Properly formed metadata - {map[string]string{"host": testValidMQQueueURL, "queueManager": "testQueueManager", "queueName": "testQueue", "queueDepth": "10"}, false, map[string]string{"username": "testUsername", "password": "Pass123"}}, + {map[string]string{"host": testValidMQQueueURL, "queueName": "testQueue", "queueDepth": "10"}, false, map[string]string{"username": "testUsername", "password": "Pass123"}}, // Invalid queueDepth using a string - {map[string]string{"host": testValidMQQueueURL, "queueManager": "testQueueManager", "queueName": "testQueue", "queueDepth": "AA"}, true, map[string]string{"username": "testUsername", "password": "Pass123"}}, + {map[string]string{"host": testValidMQQueueURL, "queueName": "testQueue", "queueDepth": "AA"}, true, map[string]string{"username": "testUsername", "password": "Pass123"}}, // Invalid activationQueueDepth using a string - {map[string]string{"host": testValidMQQueueURL, "queueManager": "testQueueManager", "queueName": "testQueue", "queueDepth": "1", "activationQueueDepth": "AA"}, true, map[string]string{"username": "testUsername", "password": "Pass123"}}, + {map[string]string{"host": testValidMQQueueURL, "queueName": "testQueue", "queueDepth": "1", "activationQueueDepth": "AA"}, true, map[string]string{"username": "testUsername", "password": "Pass123"}}, // No host provided - {map[string]string{"queueManager": "testQueueManager", "queueName": "testQueue", "queueDepth": "10"}, true, map[string]string{"username": "testUsername", "password": "Pass123"}}, - // Missing queueManager - {map[string]string{"host": testValidMQQueueURL, "queueName": "testQueue", "queueDepth": "10"}, true, map[string]string{"username": "testUsername", "password": "Pass123"}}, + {map[string]string{"queueName": "testQueue", "queueDepth": "10"}, true, map[string]string{"username": "testUsername", "password": "Pass123"}}, // Missing queueName - {map[string]string{"host": testValidMQQueueURL, "queueManager": "testQueueManager", "queueDepth": "10"}, true, map[string]string{"username": "testUsername", "password": "Pass123"}}, + {map[string]string{"host": testValidMQQueueURL, "queueDepth": "10"}, true, map[string]string{"username": "testUsername", "password": "Pass123"}}, // Invalid URL - {map[string]string{"host": testInvalidMQQueueURL, "queueManager": "testQueueManager", "queueName": "testQueue", "queueDepth": "10"}, true, map[string]string{"username": "testUsername", "password": "Pass123"}}, + {map[string]string{"host": testInvalidMQQueueURL, "queueName": "testQueue", "queueDepth": "10"}, true, map[string]string{"username": "testUsername", "password": "Pass123"}}, // Properly formed authParams Basic Auth - {map[string]string{"host": testValidMQQueueURL, "queueManager": "testQueueManager", "queueName": "testQueue", "queueDepth": "10"}, false, map[string]string{"username": "testUsername", "password": "Pass123"}}, + {map[string]string{"host": testValidMQQueueURL, "queueName": "testQueue", "queueDepth": "10"}, false, map[string]string{"username": "testUsername", "password": "Pass123"}}, // Properly formed authParams Basic Auth and TLS - {map[string]string{"host": testValidMQQueueURL, "queueManager": "testQueueManager", "queueName": "testQueue", "queueDepth": "10"}, false, map[string]string{"username": "testUsername", "password": "Pass123", "ca": "cavalue", "cert": "certvalue", "key": "keyvalue"}}, + {map[string]string{"host": testValidMQQueueURL, "queueName": "testQueue", "queueDepth": "10"}, false, map[string]string{"username": "testUsername", "password": "Pass123", "ca": "cavalue", "cert": "certvalue", "key": "keyvalue"}}, + // No key provided + {map[string]string{"host": testValidMQQueueURL, "queueName": "testQueue", "queueDepth": "10"}, true, map[string]string{"username": "testUsername", "password": "Pass123", "ca": "cavalue", "cert": "certvalue"}}, // No username provided - {map[string]string{"host": testValidMQQueueURL, "queueManager": "testQueueManager", "queueName": "testQueue", "queueDepth": "10"}, true, map[string]string{"password": "Pass123"}}, + {map[string]string{"host": testValidMQQueueURL, "queueName": "testQueue", "queueDepth": "10"}, true, map[string]string{"password": "Pass123"}}, // No password provided - {map[string]string{"host": testValidMQQueueURL, "queueManager": "testQueueManager", "queueName": "testQueue", "queueDepth": "10"}, true, map[string]string{"username": "testUsername"}}, + {map[string]string{"host": testValidMQQueueURL, "queueName": "testQueue", "queueDepth": "10"}, true, map[string]string{"username": "testUsername"}}, // Wrong input unsafeSsl - {map[string]string{"host": testValidMQQueueURL, "queueManager": "testQueueManager", "queueName": "testQueue", "queueDepth": "10", "unsafeSsl": "random"}, true, map[string]string{"username": "testUsername", "password": "Pass123"}}, + {map[string]string{"host": testValidMQQueueURL, "queueName": "testQueue", "queueDepth": "10", "unsafeSsl": "random"}, true, map[string]string{"username": "testUsername", "password": "Pass123"}}, } // Test MQ Connection metadata is parsed correctly @@ -89,8 +89,8 @@ func TestIBMMQParseMetadata(t *testing.T) { t.Error("Expected error but got success") fmt.Println(testData) } - if metadata != nil && metadata.password != "" && metadata.password != testData.authParams["password"] { - t.Error("Expected password from configuration but found something else: ", metadata.password) + if metadata != (ibmmqMetadata{}) && metadata.Password != "" && metadata.Password != testData.authParams["password"] { + t.Error("Expected password from configuration but found something else: ", metadata.Password) fmt.Println(testData) } } @@ -98,7 +98,7 @@ func TestIBMMQParseMetadata(t *testing.T) { // Test case for TestParseDefaultQueueDepth test var testDefaultQueueDepth = []parseIBMMQMetadataTestData{ - {map[string]string{"host": testValidMQQueueURL, "queueManager": "testQueueManager", "queueName": "testQueue"}, false, map[string]string{"username": "testUsername", "password": "Pass123"}}, + {map[string]string{"host": testValidMQQueueURL, "queueName": "testQueue"}, false, map[string]string{"username": "testUsername", "password": "Pass123"}}, } // Test that DefaultQueueDepth is set when queueDepth is not provided @@ -110,8 +110,8 @@ func TestParseDefaultQueueDepth(t *testing.T) { t.Error("Expected success but got error", err) case testData.isError && err == nil: t.Error("Expected error but got success") - case metadata.queueDepth != defaultTargetQueueDepth: - t.Error("Expected default queueDepth =", defaultTargetQueueDepth, "but got", metadata.queueDepth) + case metadata.QueueDepth != defaultTargetQueueDepth: + t.Error("Expected default queueDepth =", defaultTargetQueueDepth, "but got", metadata.QueueDepth) } } } @@ -120,14 +120,12 @@ func TestParseDefaultQueueDepth(t *testing.T) { func TestIBMMQGetMetricSpecForScaling(t *testing.T) { for _, testData := range IBMMQMetricIdentifiers { metadata, err := parseIBMMQMetadata(&scalersconfig.ScalerConfig{ResolvedEnv: sampleIBMMQResolvedEnv, TriggerMetadata: testData.metadataTestData.metadata, AuthParams: testData.metadataTestData.authParams, TriggerIndex: testData.triggerIndex}) - httpTimeout := 100 * time.Millisecond if err != nil { t.Fatal("Could not parse metadata:", err) } - mockIBMMQScaler := IBMMQScaler{ - metadata: metadata, - defaultHTTPTimeout: httpTimeout, + mockIBMMQScaler := ibmmqScaler{ + metadata: metadata, } metricSpec := mockIBMMQScaler.GetMetricSpecForScaling(context.Background()) metricName := metricSpec[0].External.Metric.Name @@ -216,9 +214,9 @@ func TestIBMMQScalerGetQueueDepthViaHTTP(t *testing.T) { })) defer server.Close() - scaler := IBMMQScaler{ - metadata: &IBMMQMetadata{ - host: server.URL, + scaler := ibmmqScaler{ + metadata: ibmmqMetadata{ + Host: server.URL, }, httpClient: server.Client(), } diff --git a/tests/scalers/ibmmq/ibmmq_test.go b/tests/scalers/ibmmq/ibmmq_test.go index 82a1c6ccaeb..69bcf459e3b 100644 --- a/tests/scalers/ibmmq/ibmmq_test.go +++ b/tests/scalers/ibmmq/ibmmq_test.go @@ -203,9 +203,8 @@ spec: queueDepth: "3" activationQueueDepth: "{{.ActivationQueueDepth}}" host: {{.MqscAdminRestEndpoint}} - queueManager: {{.QueueManagerName}} queueName: {{.QueueName}} - tls: "true" + unsafeSsl: "true" usernameFromEnv: "" passwordFromEnv: "" authenticationRef: