Feature/validate metric name uniqueness

.golangci.yml

@@ -56,6 +56,9 @@ issues:
  - path: scale_handler.go
  linters:
  - gocyclo
+ - 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.
 
 ### 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())
+ })
+ })
+ })
+})