fix: optimize all checkRepliacas code and fix the problem does not take effect

CHANGELOG.md

@@ -61,6 +61,7 @@ To learn more about active deprecations, we recommend checking [GitHub Discussio
 - **General**: Enable OpenSSF Scorecard to enhance security practices across the project ([#5913](https://github.com/kedacore/keda/issues/5913))
 - **General**: Introduce new NSQ scaler ([#3281](https://github.com/kedacore/keda/issues/3281))
 - **General**: Operator flag to control patching of webhook resources certificates ([#6184](https://github.com/kedacore/keda/issues/6184))
+- **General**: Optimize all webhook checkReplicas code ([#6344](https://github.com/kedacore/keda/pull/6344))
 
 #### Experimental
 
@@ -584,7 +585,7 @@ New deprecation(s):
 - **General**: Drop a transitive dependency on bou.ke/monkey ([#4364](https://github.com/kedacore/keda/issues/4364))
 - **General**: Fix odd number of arguments passed as key-value pairs for logging ([#4368](https://github.com/kedacore/keda/issues/4368))
 - **General**: Refactor several functions for Status & Conditions handling into pkg util functions ([#2906](https://github.com/kedacore/keda/pull/2906))
-- **General**: Stop logging errors for paused ScaledObject (with `autoscaling.keda.sh/paused-replicas` annotation) by skipping reconciliation loop for the object (stop the scale loop and delete the HPA) ([#4253](https://github.com/kedacore/keda/pull/4253))
+- **General**: Stop logging errors for paused ScaledObject (with `autoscaling.keda.sh/paused-` annotation) by skipping reconciliation loop for the object (stop the scale loop and delete the HPA) ([#4253](https://github.com/kedacore/keda/pull/4253))
 - **General**: Trying to prevent operator crash when accessing `ScaledObject.Status.ScaleTargetGVKR` ([#4389](https://github.com/kedacore/keda/issues/4389))
 - **General**: Use default metrics provider from `sigs.k8s.io/custom-metrics-apiserver` ([#4473](https://github.com/kedacore/keda/pull/4473))
 
@@ -905,7 +906,7 @@ None.
 
 ### New
 
-- **General**: Introduce annotation `"autoscaling.keda.sh/paused-replicas"` for ScaledObjects to pause scaling at a fixed replica count. ([#944](https://github.com/kedacore/keda/issues/944))
+- **General**: Introduce annotation `"autoscaling.keda.sh/paused-"` for ScaledObjects to pause scaling at a fixed replica count. ([#944](https://github.com/kedacore/keda/issues/944))
 - **General**: Introduce ARM-based container image for KEDA ([#2263](https://github.com/kedacore/keda/issues/2263)|[#2262](https://github.com/kedacore/keda/issues/2262))
 - **General**: Introduce new AWS DynamoDB Scaler ([#2482](https://github.com/kedacore/keda/issues/2482))
 - **General**: Introduce new Azure Data Explorer Scaler ([#1488](https://github.com/kedacore/keda/issues/1488)|[#2734](https://github.com/kedacore/keda/issues/2734))

apis/keda/v1alpha1/scaledobject_types.go

@@ -239,7 +239,7 @@ func (so *ScaledObject) IsUsingModifiers() bool {
 
 // getHPAMinReplicas returns MinReplicas based on definition in ScaledObject or default value if not defined
 func (so *ScaledObject) GetHPAMinReplicas() *int32 {
-	if so.Spec.MinReplicaCount != nil && *so.Spec.MinReplicaCount > 0 {
+	if so.Spec.MinReplicaCount != nil {
 		return so.Spec.MinReplicaCount
 	}
 	tmp := defaultHPAMinReplicas
@@ -254,21 +254,39 @@ func (so *ScaledObject) GetHPAMaxReplicas() int32 {
 	return defaultHPAMaxReplicas
 }
 
+// GetDefaultHPAMinReplicas returns defaultHPAMinReplicas
+func GetDefaultHPAMinReplicas() *int32 {
+	tmp := defaultHPAMinReplicas
+	return &tmp
+}
+
+// GetDefaultHPAMaxReplicas returns defaultHPAMaxReplicas
+func GetDefaultHPAMaxReplicas() int32 {
+	return defaultHPAMaxReplicas
+}
+
 // checkReplicaCountBoundsAreValid checks that Idle/Min/Max ReplicaCount defined in ScaledObject are correctly specified
 // i.e. that Min is not greater than Max or Idle greater or equal to Min
 func CheckReplicaCountBoundsAreValid(scaledObject *ScaledObject) error {
-	min := int32(0)
-	if scaledObject.Spec.MinReplicaCount != nil {
-		min = *scaledObject.GetHPAMinReplicas()
+	var idleReplicas *int32
+	minReplicas := *scaledObject.GetHPAMinReplicas()
+	maxReplicas := scaledObject.GetHPAMaxReplicas()
+	idleReplicas = scaledObject.Spec.IdleReplicaCount
+
+	if scaledObject.Spec.IdleReplicaCount != nil && *idleReplicas < 0 {
+		return fmt.Errorf("IdleReplicaCount=%d must not be negative", *idleReplicas)
+	}
+
+	if minReplicas < 0 {
+		return fmt.Errorf("MinReplicaCount=%d must not be negative", minReplicas)
 	}
-	max := scaledObject.GetHPAMaxReplicas()
 
-	if min > max {
-		return fmt.Errorf("MinReplicaCount=%d must be less than MaxReplicaCount=%d", min, max)
+	if minReplicas > maxReplicas {
+		return fmt.Errorf("MinReplicaCount=%d must not be greater than MaxReplicaCount=%d", minReplicas, maxReplicas)
 	}
 
-	if scaledObject.Spec.IdleReplicaCount != nil && *scaledObject.Spec.IdleReplicaCount >= min {
-		return fmt.Errorf("IdleReplicaCount=%d must be less than MinReplicaCount=%d", *scaledObject.Spec.IdleReplicaCount, min)
+	if scaledObject.Spec.IdleReplicaCount != nil && *idleReplicas >= minReplicas {
+		return fmt.Errorf("IdleReplicaCount=%d must be less than MinReplicaCount=%d", *scaledObject.Spec.IdleReplicaCount, minReplicas)
 	}
 
 	return nil

apis/keda/v1alpha1/scaledobject_webhook.go

@@ -184,7 +184,7 @@ func verifyReplicaCount(incomingSo *ScaledObject, action string, _ bool) error {
 		scaledobjectlog.WithValues("name", incomingSo.Name).Error(err, "validation error")
 		metricscollector.RecordScaledObjectValidatingErrors(incomingSo.Namespace, action, "incorrect-replicas")
 	}
-	return nil
+	return err
 }
 
 func verifyFallback(incomingSo *ScaledObject, action string, _ bool) error {

apis/keda/v1alpha1/scaledobject_webhook_test.go

@@ -1048,6 +1048,92 @@ var _ = It("should validate the so creation with ScalingModifiers.Formula - doub
 	}).ShouldNot(HaveOccurred())
 })
 
+var _ = It("shouldn't validate negative minreplicacount", func() {
+
+	namespaceName := "negative-minreplicas"
+	namespace := createNamespace(namespaceName)
+
+	err := k8sClient.Create(context.Background(), namespace)
+	Expect(err).ToNot(HaveOccurred())
+
+	workload := createDeployment(namespaceName, false, false)
+
+	err = k8sClient.Create(context.Background(), workload)
+	Expect(err).ToNot(HaveOccurred())
+
+	so := createScaledObject(soName, namespaceName, workloadName, "apps/v1", "Deployment", false, map[string]string{}, "")
+	so.Spec.MinReplicaCount = ptr.To[int32](-5)
+
+	Eventually(func() error {
+		return k8sClient.Create(context.Background(), so)
+	}).Should(HaveOccurred())
+})
+
+var _ = It("shouldn't validate minreplicacount greater than maxreplicacount", func() {
+
+	namespaceName := "minreplicas-greater-than-maxreplicas"
+	namespace := createNamespace(namespaceName)
+
+	err := k8sClient.Create(context.Background(), namespace)
+	Expect(err).ToNot(HaveOccurred())
+
+	workload := createDeployment(namespaceName, false, false)
+
+	err = k8sClient.Create(context.Background(), workload)
+	Expect(err).ToNot(HaveOccurred())
+
+	so := createScaledObject(soName, namespaceName, workloadName, "apps/v1", "Deployment", false, map[string]string{}, "")
+	so.Spec.MinReplicaCount = ptr.To[int32](11)
+
+	Eventually(func() error {
+		return k8sClient.Create(context.Background(), so)
+	}).Should(HaveOccurred())
+})
+
+var _ = It("should validate minreplicacount and maxreplicacount are all equal to zero", func() {
+
+	namespaceName := "minreplicas-maxreplicas-zero"
+	namespace := createNamespace(namespaceName)
+
+	err := k8sClient.Create(context.Background(), namespace)
+	Expect(err).ToNot(HaveOccurred())
+
+	workload := createDeployment(namespaceName, false, false)
+
+	err = k8sClient.Create(context.Background(), workload)
+	Expect(err).ToNot(HaveOccurred())
+
+	so := createScaledObject(soName, namespaceName, workloadName, "apps/v1", "Deployment", false, map[string]string{}, "")
+	so.Spec.MinReplicaCount = ptr.To[int32](0)
+	so.Spec.MaxReplicaCount = ptr.To[int32](0)
+	so.Spec.IdleReplicaCount = nil
+
+	Eventually(func() error {
+		return k8sClient.Create(context.Background(), so)
+	}).ShouldNot(HaveOccurred())
+})
+
+var _ = It("shouldn't validate negative idlereplicacount", func() {
+
+	namespaceName := "negative-idlereplicas"
+	namespace := createNamespace(namespaceName)
+
+	err := k8sClient.Create(context.Background(), namespace)
+	Expect(err).ToNot(HaveOccurred())
+
+	workload := createDeployment(namespaceName, false, false)
+
+	err = k8sClient.Create(context.Background(), workload)
+	Expect(err).ToNot(HaveOccurred())
+
+	so := createScaledObject(soName, namespaceName, workloadName, "apps/v1", "Deployment", false, map[string]string{}, "")
+	so.Spec.IdleReplicaCount = ptr.To[int32](-1)
+
+	Eventually(func() error {
+		return k8sClient.Create(context.Background(), so)
+	}).Should(HaveOccurred())
+})
+
 var _ = AfterSuite(func() {
 	cancel()
 	By("tearing down the test environment")

controllers/keda/hpa.go

@@ -102,6 +102,19 @@ func (r *ScaledObjectReconciler) newHPAForScaledObject(ctx context.Context, logg
 	minReplicas := scaledObject.GetHPAMinReplicas()
 	maxReplicas := scaledObject.GetHPAMaxReplicas()
 
+	if *minReplicas == 0 {
+		minReplicas = kedav1alpha1.GetDefaultHPAMinReplicas()
+	}
+
+	if maxReplicas == 0 {
+		maxReplicas = kedav1alpha1.GetDefaultHPAMaxReplicas()
+	}
+
+	if *minReplicas < 0 || maxReplicas < 0 {
+		err := fmt.Errorf("MinReplicaCount=%d and MaxReplicaCount=%d must not be negative", minReplicas, maxReplicas)
+		return nil, err
+	}
+
 	pausedCount, err := executor.GetPausedReplicaCount(scaledObject)
 	if err != nil {
 		return nil, err