Prevent multiple ScaledObjects managing one HPA (#6198)

* Prevent multiple ScaledObjects managing one HPA Signed-off-by: wangrushen <[email protected]> * fix: e2e test and mv changelog to improvements Signed-off-by: wangrushen <[email protected]> * check nil for spec.advanced.horizontalPodAutoscalerConfig Signed-off-by: wangrushen <[email protected]> --------- Signed-off-by: wangrushen <[email protected]>
kedacore · Oct 22, 2024 · d849c75 · d849c75
1 parent a390a91
commit d849c75
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 0 deletions.
diff --git 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
@@ -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
@@ -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---")