From d849c75069191c79c8a8f35f2dcaa1e49b9272e9 Mon Sep 17 00:00:00 2001 From: Rushen Wang <45029442+dovics@users.noreply.github.com> Date: Tue, 22 Oct 2024 18:50:54 +0800 Subject: [PATCH] Prevent multiple ScaledObjects managing one HPA (#6198) * Prevent multiple ScaledObjects managing one HPA Signed-off-by: wangrushen * fix: e2e test and mv changelog to improvements Signed-off-by: wangrushen * check nil for spec.advanced.horizontalPodAutoscalerConfig Signed-off-by: wangrushen --------- Signed-off-by: wangrushen --- CHANGELOG.md | 1 + apis/keda/v1alpha1/scaledobject_webhook.go | 16 +++++++ .../scaled_object_validation_test.go | 42 +++++++++++++++++++ 3 files changed, 59 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c583cd2b8a2..32f6c393aae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -72,6 +72,7 @@ Here is an overview of all new **experimental** features: ### Improvements +- **General**: Prevent multiple ScaledObjects managing one HPA ([#6130](https://github.com/kedacore/keda/issues/6130)) - **AWS CloudWatch Scaler**: Add support for ignoreNullValues ([#5352](https://github.com/kedacore/keda/issues/5352)) - **Elasticsearch Scaler**: Support Query at the Elasticsearch scaler ([#6216](https://github.com/kedacore/keda/issues/6216)) - **Etcd Scaler**: Add username and password support for etcd ([#6199](https://github.com/kedacore/keda/pull/6199)) diff --git a/apis/keda/v1alpha1/scaledobject_webhook.go b/apis/keda/v1alpha1/scaledobject_webhook.go index b96c984445b..41bd0355596 100644 --- a/apis/keda/v1alpha1/scaledobject_webhook.go +++ b/apis/keda/v1alpha1/scaledobject_webhook.go @@ -295,6 +295,7 @@ func verifyScaledObjects(incomingSo *ScaledObject, action string, _ bool) error return err } + incomingSoHpaName := getHpaName(*incomingSo) for _, so := range soList.Items { if so.Name == incomingSo.Name { continue @@ -315,6 +316,13 @@ func verifyScaledObjects(incomingSo *ScaledObject, action string, _ bool) error metricscollector.RecordScaledObjectValidatingErrors(incomingSo.Namespace, action, "other-scaled-object") return err } + + if getHpaName(so) == incomingSoHpaName { + err = fmt.Errorf("the HPA '%s' is already managed by the ScaledObject '%s'", so.Spec.Advanced.HorizontalPodAutoscalerConfig.Name, so.Name) + scaledobjectlog.Error(err, "validation error") + metricscollector.RecordScaledObjectValidatingErrors(incomingSo.Namespace, action, "other-scaled-object-hpa") + return err + } } // verify ScalingModifiers structure if defined in ScaledObject @@ -572,3 +580,11 @@ func isContainerResourceLimitSet(ctx context.Context, namespace string, triggerT return false } + +func getHpaName(so ScaledObject) string { + if so.Spec.Advanced == nil || so.Spec.Advanced.HorizontalPodAutoscalerConfig == nil || so.Spec.Advanced.HorizontalPodAutoscalerConfig.Name == "" { + return fmt.Sprintf("keda-hpa-%s", so.Name) + } + + return so.Spec.Advanced.HorizontalPodAutoscalerConfig.Name +} diff --git a/tests/internals/scaled_object_validation/scaled_object_validation_test.go b/tests/internals/scaled_object_validation/scaled_object_validation_test.go index 9cdaff34515..2af7f6b81d8 100644 --- a/tests/internals/scaled_object_validation/scaled_object_validation_test.go +++ b/tests/internals/scaled_object_validation/scaled_object_validation_test.go @@ -131,6 +131,27 @@ spec: desiredReplicas: '1' ` + customHpaScaledObjectTemplate = ` +apiVersion: keda.sh/v1alpha1 +kind: ScaledObject +metadata: + name: {{.ScaledObjectName}} + namespace: {{.TestNamespace}} +spec: + scaleTargetRef: + name: {{.DeploymentName}} + advanced: + horizontalPodAutoscalerConfig: + name: {{.HpaName}} + triggers: + - type: cron + metadata: + timezone: Etc/UTC + start: 0 * * * * + end: 1 * * * * + desiredReplicas: '1' + ` + hpaTemplate = ` apiVersion: autoscaling/v2 kind: HorizontalPodAutoscaler @@ -179,6 +200,8 @@ func TestScaledObjectValidations(t *testing.T) { testScaledWorkloadByOtherScaledObject(t, data) + testManagedHpaByOtherScaledObject(t, data) + testScaledWorkloadByOtherHpa(t, data) testScaledWorkloadByOtherHpaWithOwnershipTransfer(t, data) @@ -220,6 +243,25 @@ func testScaledWorkloadByOtherScaledObject(t *testing.T, data templateData) { KubectlDeleteWithTemplate(t, data, "scaledObjectTemplate", scaledObjectTemplate) } +func testManagedHpaByOtherScaledObject(t *testing.T, data templateData) { + t.Log("--- already managed hpa by other scaledobject---") + + data.HpaName = hpaName + + data.ScaledObjectName = scaledObject1Name + err := KubectlApplyWithErrors(t, data, "scaledObjectTemplate", customHpaScaledObjectTemplate) + assert.NoErrorf(t, err, "cannot deploy the scaledObject - %s", err) + + data.ScaledObjectName = scaledObject2Name + data.DeploymentName = fmt.Sprintf("%s-other-deployment", testName) + err = KubectlApplyWithErrors(t, data, "scaledObjectTemplate", customHpaScaledObjectTemplate) + assert.Errorf(t, err, "can deploy the scaledObject - %s", err) + assert.Contains(t, err.Error(), fmt.Sprintf("the HPA '%s' is already managed by the ScaledObject '%s", hpaName, scaledObject1Name)) + + data.ScaledObjectName = scaledObject1Name + KubectlDeleteWithTemplate(t, data, "scaledObjectTemplate", scaledObjectTemplate) +} + func testScaledWorkloadByOtherHpa(t *testing.T, data templateData) { t.Log("--- already scaled workload by other hpa---")