Feature/validate metric name uniqueness (kedacore#1390)

* add metricName uniqueness check

Signed-off-by: ycabrer <43866176+ycabrer@users.noreply.github.com>

.golangci.yml

@@ -56,6 +56,10 @@ issues:
     - path: scale_handler.go
       linters:
         - gocyclo
+  # Excluding interfacer for finalizers, reason: https://github.com/kedacore/keda/pull/1390
+    - path: _finalizer.go
+      linters:
+        - interfacer
 
     # https://github.com/go-critic/go-critic/issues/926
     - linters:

CHANGELOG.md

@@ -40,6 +40,7 @@
 - Improve error reporting in prometheus scaler ([PR #1497](https://github.com/kedacore/keda/pull/1497))
 
 ### Breaking Changes
+- Require metricNames be unique in scaled objects ([#1390](https://github.com/kedacore/keda/pull/1390))
 
 ### Other
 - Bump go module version to v2 ([#1324](https://github.com/kedacore/keda/pull/1324))

controllers/scaledobject_controller.go

@@ -190,6 +190,11 @@ func (r *ScaledObjectReconciler) reconcileScaledObject(logger logr.Logger, scale
 		return "ScaledObject doesn't have correct scaleTargetRef specification", err
 	}
 
+	err = r.validateMetricNameUniqueness(logger, scaledObject)
+	if err != nil {
+		return "Error checking metric name uniqueness", err
+	}
+
 	// Create a new HPA or update existing one according to ScaledObject
 	newHPACreated, err := r.ensureHPAForScaledObjectExists(logger, scaledObject, &gvkr)
 	if err != nil {
@@ -235,6 +240,34 @@ func (r *ScaledObjectReconciler) ensureScaledObjectLabel(logger logr.Logger, sca
 	return r.Client.Update(context.TODO(), scaledObject)
 }
 
+func (r *ScaledObjectReconciler) validateMetricNameUniqueness(logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject) error {
+	scalers, err := r.scaleHandler.GetScalers(scaledObject)
+	if err != nil {
+		logger.Error(err, "Unable to fetch scalers in metric name uniqueness check")
+		return err
+	}
+
+	observedMetricNames := make(map[string]struct{})
+	for _, scaler := range scalers {
+		for _, metric := range scaler.GetMetricSpecForScaling() {
+			// Only validate external metricNames
+			if metric.External == nil {
+				continue
+			}
+
+			metricName := metric.External.Metric.Name
+			if _, ok := observedMetricNames[metricName]; ok {
+				return fmt.Errorf("metricName %s defined multiple times in ScaledObject %s, please refer the documentation how to define metircName manually", metricName, scaledObject.Name)
+			}
+
+			observedMetricNames[metricName] = struct{}{}
+		}
+	}
+
+	logger.V(1).Info("All metric names are unique in ScaledObject", "value", scaledObject.Name)
+	return nil
+}
+
 // checkTargetResourceIsScalable checks if resource targeted for scaling exists and exposes /scale subresource
 func (r *ScaledObjectReconciler) checkTargetResourceIsScalable(logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject) (kedav1alpha1.GroupVersionKindResource, error) {
 	gvkr, err := kedautil.ParseGVKR(r.restMapper, scaledObject.Spec.ScaleTargetRef.APIVersion, scaledObject.Spec.ScaleTargetRef.Kind)

controllers/scaledobject_controller_test.go

@@ -0,0 +1,124 @@
+package controllers
+
+import (
+	"fmt"
+
+	"github.com/golang/mock/gomock"
+	kedav1alpha1 "github.com/kedacore/keda/v2/api/v1alpha1"
+	"github.com/kedacore/keda/v2/pkg/mock/mock_scaling"
+	"github.com/kedacore/keda/v2/pkg/scalers"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"sigs.k8s.io/controller-runtime/pkg/log/zap"
+)
+
+type GinkgoTestReporter struct{}
+
+func (g GinkgoTestReporter) Errorf(format string, args ...interface{}) {
+	Fail(fmt.Sprintf(format, args...))
+}
+
+func (g GinkgoTestReporter) Fatalf(format string, args ...interface{}) {
+	Fail(fmt.Sprintf(format, args...))
+}
+
+var _ = Describe("ScaledObjectController", func() {
+	var (
+		testLogger = zap.LoggerTo(GinkgoWriter, true)
+	)
+
+	Describe("Metric Names", func() {
+		var (
+			metricNameTestReconciler ScaledObjectReconciler
+			mockScaleHandler         *mock_scaling.MockScaleHandler
+		)
+
+		var triggerMeta []map[string]string = []map[string]string{
+			{"serverAddress": "http://localhost:9090", "metricName": "http_requests_total", "threshold": "100", "query": "up", "disableScaleToZero": "true"},
+			{"serverAddress": "http://localhost:9090", "metricName": "http_requests_total2", "threshold": "100", "query": "up"},
+		}
+
+		BeforeEach(func() {
+			mockScaleHandler = mock_scaling.NewMockScaleHandler(gomock.NewController(GinkgoTestReporter{}))
+
+			metricNameTestReconciler = ScaledObjectReconciler{
+				scaleHandler: mockScaleHandler,
+			}
+		})
+
+		Context("With Unique Values", func() {
+			var uniqueNamedScaledObjectTrigger = &kedav1alpha1.ScaledObject{}
+
+			It("should pass metric name validation", func() {
+				testScalers := make([]scalers.Scaler, 0)
+				for i, tm := range triggerMeta {
+					config := &scalers.ScalerConfig{
+						Name:            fmt.Sprintf("test.%d", i),
+						Namespace:       "test",
+						TriggerMetadata: tm,
+						ResolvedEnv:     nil,
+						AuthParams:      nil,
+					}
+
+					s, err := scalers.NewPrometheusScaler(config)
+					if err != nil {
+						Fail(err.Error())
+					}
+
+					testScalers = append(testScalers, s)
+				}
+
+				mockScaleHandler.EXPECT().GetScalers(uniqueNamedScaledObjectTrigger).Return(testScalers, nil)
+
+				Ω(metricNameTestReconciler.validateMetricNameUniqueness(testLogger, uniqueNamedScaledObjectTrigger)).Should(BeNil())
+			})
+
+			It("should pass metric name validation with single value", func() {
+				config := &scalers.ScalerConfig{
+					Name:            "test",
+					Namespace:       "test",
+					TriggerMetadata: triggerMeta[0],
+					ResolvedEnv:     nil,
+					AuthParams:      nil,
+				}
+
+				s, err := scalers.NewPrometheusScaler(config)
+				if err != nil {
+					Fail(err.Error())
+				}
+
+				mockScaleHandler.EXPECT().GetScalers(uniqueNamedScaledObjectTrigger).Return([]scalers.Scaler{s}, nil)
+
+				Ω(metricNameTestReconciler.validateMetricNameUniqueness(testLogger, uniqueNamedScaledObjectTrigger)).Should(BeNil())
+			})
+		})
+
+		Context("With Duplicate Values", func() {
+			var duplicateNamedScaledObjectTrigger = &kedav1alpha1.ScaledObject{}
+
+			It("should pass metric name validation", func() {
+				testScalers := make([]scalers.Scaler, 0)
+				for i := 0; i < 4; i++ {
+					config := &scalers.ScalerConfig{
+						Name:            fmt.Sprintf("test.%d", i),
+						Namespace:       "test",
+						TriggerMetadata: triggerMeta[0],
+						ResolvedEnv:     nil,
+						AuthParams:      nil,
+					}
+
+					s, err := scalers.NewPrometheusScaler(config)
+					if err != nil {
+						Fail(err.Error())
+					}
+
+					testScalers = append(testScalers, s)
+				}
+
+				mockScaleHandler.EXPECT().GetScalers(duplicateNamedScaledObjectTrigger).Return(testScalers, nil)
+
+				Ω(metricNameTestReconciler.validateMetricNameUniqueness(testLogger, duplicateNamedScaledObjectTrigger)).ShouldNot(BeNil())
+			})
+		})
+	})
+})