validate triggers also in the webhook (#4670)

Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>

CHANGELOG.md

@@ -68,6 +68,7 @@ To learn more about active deprecations, we recommend checking [GitHub Discussio
 ### Fixes
 
 - **Admission Webhooks**: Allow to remove the finalizer even if the ScaledObject isn't valid ([#4396](https://github.com/kedacore/keda/issue/4396))
+- **Admission Webhooks**: Check ScaledObjects with multiple triggers with non unique name ([#4664](https://github.com/kedacore/keda/issue/4664))
 - **ScaledJob**: Check if MaxReplicaCount is nil before access to it ([#4568]https://github.com/kedacore/keda/issues/4568)
 - **AWS SQS Scaler**: Respect `scaleOnInFlight` value ([#4276](https://github.com/kedacore/keda/issue/4276))
 - **Azure Pipelines**: Fix for disallowing `$top` on query when using `meta.parentID` method ([#4397])

apis/keda/v1alpha1/scaledobject_types.go

@@ -122,21 +122,6 @@ type ScaleTarget struct {
 	EnvSourceContainerName string `json:"envSourceContainerName,omitempty"`
 }
 
-// ScaleTriggers reference the scaler that will be used
-type ScaleTriggers struct {
-	Type string `json:"type"`
-	// +optional
-	Name string `json:"name,omitempty"`
-
-	UseCachedMetrics bool `json:"useCachedMetrics,omitempty"`
-
-	Metadata map[string]string `json:"metadata"`
-	// +optional
-	AuthenticationRef *ScaledObjectAuthRef `json:"authenticationRef,omitempty"`
-	// +optional
-	MetricType autoscalingv2.MetricTargetType `json:"metricType,omitempty"`
-}
-
 // +k8s:openapi-gen=true
 
 // ScaledObjectStatus is the status for a ScaledObject resource
@@ -173,15 +158,6 @@ type ScaledObjectList struct {
 	Items           []ScaledObject `json:"items"`
 }
 
-// ScaledObjectAuthRef points to the TriggerAuthentication or ClusterTriggerAuthentication object that
-// is used to authenticate the scaler with the environment
-type ScaledObjectAuthRef struct {
-	Name string `json:"name"`
-	// Kind of the resource being referred to. Defaults to TriggerAuthentication.
-	// +optional
-	Kind string `json:"kind,omitempty"`
-}
-
 func init() {
 	SchemeBuilder.Register(&ScaledObject{}, &ScaledObjectList{})
 }

apis/keda/v1alpha1/scaledobject_webhook.go

@@ -92,23 +92,34 @@ func isRemovingFinalizer(so *ScaledObject, old runtime.Object) bool {
 
 func validateWorkload(so *ScaledObject, action string) (admission.Warnings, error) {
 	prommetrics.RecordScaledObjectValidatingTotal(so.Namespace, action)
-	err := verifyCPUMemoryScalers(so, action)
-	if err != nil {
-		return nil, err
-	}
-	err = verifyScaledObjects(so, action)
-	if err != nil {
-		return nil, err
+
+	verifyFunctions := []func(*ScaledObject, string) error{
+		verifyCPUMemoryScalers,
+		verifyTriggers,
+		verifyScaledObjects,
+		verifyHpas,
 	}
-	err = verifyHpas(so, action)
-	if err != nil {
-		return nil, err
+
+	for i := range verifyFunctions {
+		err := verifyFunctions[i](so, action)
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	scaledobjectlog.V(1).Info(fmt.Sprintf("scaledobject %s is valid", so.Name))
 	return nil, nil
 }
 
+func verifyTriggers(incomingSo *ScaledObject, action string) error {
+	err := ValidateTriggers(scaledobjectlog.WithValues("name", incomingSo.Name), incomingSo.Spec.Triggers)
+	if err != nil {
+		scaledobjectlog.WithValues("name", incomingSo.Name).Error(err, "validation error")
+		prommetrics.RecordScaledObjectValidatingErrors(incomingSo.Namespace, action, "incorrect-triggers")
+	}
+	return err
+}
+
 func verifyHpas(incomingSo *ScaledObject, action string) error {
 	hpaList := &autoscalingv2.HorizontalPodAutoscalerList{}
 	opt := &client.ListOptions{

apis/keda/v1alpha1/scaletriggers_types.go

@@ -0,0 +1,85 @@
+/*
+Copyright 2023 The KEDA Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1alpha1
+
+import (
+	"fmt"
+
+	"github.com/go-logr/logr"
+	autoscalingv2 "k8s.io/api/autoscaling/v2"
+)
+
+// ScaleTriggers reference the scaler that will be used
+type ScaleTriggers struct {
+	Type string `json:"type"`
+	// +optional
+	Name string `json:"name,omitempty"`
+
+	UseCachedMetrics bool `json:"useCachedMetrics,omitempty"`
+
+	Metadata map[string]string `json:"metadata"`
+	// +optional
+	AuthenticationRef *AuthenticationRef `json:"authenticationRef,omitempty"`
+	// +optional
+	MetricType autoscalingv2.MetricTargetType `json:"metricType,omitempty"`
+}
+
+// AuthenticationRef points to the TriggerAuthentication or ClusterTriggerAuthentication object that
+// is used to authenticate the scaler with the environment
+type AuthenticationRef struct {
+	Name string `json:"name"`
+	// Kind of the resource being referred to. Defaults to TriggerAuthentication.
+	// +optional
+	Kind string `json:"kind,omitempty"`
+}
+
+// ValidateTriggers checks that general trigger metadata are valid, it checks:
+// - triggerNames in ScaledObject are unique
+// - useCachedMetrics is defined only for a supported triggers
+func ValidateTriggers(logger logr.Logger, triggers []ScaleTriggers) error {
+	triggersCount := len(triggers)
+	if triggers != nil && triggersCount > 0 {
+		triggerNames := make(map[string]bool, triggersCount)
+		for i := 0; i < triggersCount; i++ {
+			trigger := triggers[i]
+
+			if trigger.UseCachedMetrics {
+				if trigger.Type == "cpu" || trigger.Type == "memory" || trigger.Type == "cron" {
+					return fmt.Errorf("property \"useCachedMetrics\" is not supported for %q scaler", trigger.Type)
+				}
+			}
+
+			// FIXME: DEPRECATED to be removed in v2.12
+			_, hasMetricName := trigger.Metadata["metricName"]
+			// aws-cloudwatch and huawei-cloudeye have a meaningful use of metricName
+			if hasMetricName && trigger.Type != "aws-cloudwatch" && trigger.Type != "huawei-cloudeye" {
+				logger.Info("\"metricName\" is deprecated and will be removed in v2.12, please do not set it anymore", "trigger.type", trigger.Type)
+			}
+
+			name := trigger.Name
+			if name != "" {
+				if _, found := triggerNames[name]; found {
+					// found duplicate name
+					return fmt.Errorf("triggerName %q is defined multiple times in the ScaledObject, but it must be unique", name)
+				}
+				triggerNames[name] = true
+			}
+		}
+	}
+
+	return nil
+}

apis/keda/v1alpha1/scaletriggers_types_test.go

@@ -0,0 +1,101 @@
+package v1alpha1
+
+import (
+	"testing"
+
+	"github.com/go-logr/logr"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestValidateTriggers(t *testing.T) {
+	tests := []struct {
+		name           string
+		triggers       []ScaleTriggers
+		expectedErrMsg string
+	}{
+		{
+			name: "valid triggers",
+			triggers: []ScaleTriggers{
+				{
+					Name: "trigger1",
+					Type: "cpu",
+				},
+				{
+					Name: "trigger2",
+					Type: "prometheus",
+				},
+			},
+			expectedErrMsg: "",
+		},
+		{
+			name: "duplicate trigger names",
+			triggers: []ScaleTriggers{
+				{
+					Name: "trigger1",
+					Type: "cpu",
+				},
+				{
+					Name: "trigger1",
+					Type: "prometheus",
+				},
+			},
+			expectedErrMsg: "triggerName \"trigger1\" is defined multiple times in the ScaledObject, but it must be unique",
+		},
+		{
+			name: "unsupported useCachedMetrics property for cpu scaler",
+			triggers: []ScaleTriggers{
+				{
+					Name:             "trigger1",
+					Type:             "cpu",
+					UseCachedMetrics: true,
+				},
+			},
+			expectedErrMsg: "property \"useCachedMetrics\" is not supported for \"cpu\" scaler",
+		},
+		{
+			name: "unsupported useCachedMetrics property for memory scaler",
+			triggers: []ScaleTriggers{
+				{
+					Name:             "trigger2",
+					Type:             "memory",
+					UseCachedMetrics: true,
+				},
+			},
+			expectedErrMsg: "property \"useCachedMetrics\" is not supported for \"memory\" scaler",
+		},
+		{
+			name: "unsupported useCachedMetrics property for cron scaler",
+			triggers: []ScaleTriggers{
+				{
+					Name:             "trigger3",
+					Type:             "cron",
+					UseCachedMetrics: true,
+				},
+			},
+			expectedErrMsg: "property \"useCachedMetrics\" is not supported for \"cron\" scaler",
+		},
+		{
+			name: "supported useCachedMetrics property for kafka scaler",
+			triggers: []ScaleTriggers{
+				{
+					Name:             "trigger4",
+					Type:             "kafka",
+					UseCachedMetrics: true,
+				},
+			},
+			expectedErrMsg: "",
+		},
+	}
+
+	for _, test := range tests {
+		tt := test
+		t.Run(test.name, func(t *testing.T) {
+			err := ValidateTriggers(logr.Discard(), tt.triggers)
+			if test.expectedErrMsg == "" {
+				assert.NoError(t, err)
+			} else {
+				assert.EqualError(t, err, tt.expectedErrMsg)
+			}
+		})
+	}
+}

config/crd/bases/keda.sh_scaledjobs.yaml

@@ -8224,7 +8224,7 @@ spec:
                   description: ScaleTriggers reference the scaler that will be used
                   properties:
                     authenticationRef:
-                      description: ScaledObjectAuthRef points to the TriggerAuthentication
+                      description: AuthenticationRef points to the TriggerAuthentication
                         or ClusterTriggerAuthentication object that is used to authenticate
                         the scaler with the environment
                       properties:

config/crd/bases/keda.sh_scaledobjects.yaml

@@ -252,7 +252,7 @@ spec:
                   description: ScaleTriggers reference the scaler that will be used
                   properties:
                     authenticationRef:
-                      description: ScaledObjectAuthRef points to the TriggerAuthentication
+                      description: AuthenticationRef points to the TriggerAuthentication
                         or ClusterTriggerAuthentication object that is used to authenticate
                         the scaler with the environment
                       properties: