diff --git a/.golangci.yml b/.golangci.yml index d8be386aed3..1f4e8a4228d 100644 --- a/.golangci.yml +++ b/.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: diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cdf500d1de..759a5bfac09 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/controllers/scaledobject_controller.go b/controllers/scaledobject_controller.go index 1d18c42911f..0414fd9a3f5 100644 --- a/controllers/scaledobject_controller.go +++ b/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) diff --git a/controllers/scaledobject_controller_test.go b/controllers/scaledobject_controller_test.go new file mode 100644 index 00000000000..513c4c8c878 --- /dev/null +++ b/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()) + }) + }) + }) +}) diff --git a/pkg/mock/mock_scaling/mock_interface.go b/pkg/mock/mock_scaling/mock_interface.go new file mode 100644 index 00000000000..45540b5307d --- /dev/null +++ b/pkg/mock/mock_scaling/mock_interface.go @@ -0,0 +1,77 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: scale_handler.go + +// Package mock_scaling is a generated GoMock package. +package mock_scaling + +import ( + gomock "github.com/golang/mock/gomock" + scalers "github.com/kedacore/keda/v2/pkg/scalers" + reflect "reflect" +) + +// MockScaleHandler is a mock of ScaleHandler interface +type MockScaleHandler struct { + ctrl *gomock.Controller + recorder *MockScaleHandlerMockRecorder +} + +// MockScaleHandlerMockRecorder is the mock recorder for MockScaleHandler +type MockScaleHandlerMockRecorder struct { + mock *MockScaleHandler +} + +// NewMockScaleHandler creates a new mock instance +func NewMockScaleHandler(ctrl *gomock.Controller) *MockScaleHandler { + mock := &MockScaleHandler{ctrl: ctrl} + mock.recorder = &MockScaleHandlerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockScaleHandler) EXPECT() *MockScaleHandlerMockRecorder { + return m.recorder +} + +// HandleScalableObject mocks base method +func (m *MockScaleHandler) HandleScalableObject(scalableObject interface{}) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HandleScalableObject", scalableObject) + ret0, _ := ret[0].(error) + return ret0 +} + +// HandleScalableObject indicates an expected call of HandleScalableObject +func (mr *MockScaleHandlerMockRecorder) HandleScalableObject(scalableObject interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HandleScalableObject", reflect.TypeOf((*MockScaleHandler)(nil).HandleScalableObject), scalableObject) +} + +// DeleteScalableObject mocks base method +func (m *MockScaleHandler) DeleteScalableObject(scalableObject interface{}) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteScalableObject", scalableObject) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeleteScalableObject indicates an expected call of DeleteScalableObject +func (mr *MockScaleHandlerMockRecorder) DeleteScalableObject(scalableObject interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteScalableObject", reflect.TypeOf((*MockScaleHandler)(nil).DeleteScalableObject), scalableObject) +} + +// GetScalers mocks base method +func (m *MockScaleHandler) GetScalers(scalableObject interface{}) ([]scalers.Scaler, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetScalers", scalableObject) + ret0, _ := ret[0].([]scalers.Scaler) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetScalers indicates an expected call of GetScalers +func (mr *MockScaleHandlerMockRecorder) GetScalers(scalableObject interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetScalers", reflect.TypeOf((*MockScaleHandler)(nil).GetScalers), scalableObject) +}