General: validate replica counts when creating scaled objects (#5289)

Signed-off-by: Bojan Zelic <[email protected]>

CHANGELOG.md

@@ -62,6 +62,7 @@ Here is an overview of all new **experimental** features:
 ### Improvements
 
 - **General**: Add parameter queryParameters to prometheus-scaler ([#4962](https://github.com/kedacore/keda/issues/4962))
+- **General**: Add validations for replica counts when creating ScaledObjects ([#5288](https://github.com/kedacore/keda/issues/5288))
 - **General**: Support TriggerAuthentication properties from ConfigMap ([#4830](https://github.com/kedacore/keda/issues/4830))
 - **General**: Use client-side round-robin load balancing for grpc calls ([#5224](https://github.com/kedacore/keda/issues/5224))
 - **GCP pubsub scaler**: Support distribution-valued metrics and metrics from topics ([#5070](https://github.com/kedacore/keda/issues/5070))

apis/keda/v1alpha1/scaledobject_types.go

@@ -17,6 +17,7 @@ limitations under the License.
 package v1alpha1
 
 import (
+	"fmt"
 	"reflect"
 
 	autoscalingv2 "k8s.io/api/autoscaling/v2"
@@ -74,6 +75,9 @@ const (
 
 	// Composite metric name used for scalingModifiers composite metric
 	CompositeMetricName string = "composite-metric"
+
+	defaultHPAMinReplicas int32 = 1
+	defaultHPAMaxReplicas int32 = 100
 )
 
 // ScaledObjectSpec is the spec for a ScaledObject resource
@@ -211,3 +215,40 @@ func (so *ScaledObject) NeedToBePausedByAnnotation() bool {
 func (so *ScaledObject) IsUsingModifiers() bool {
 	return so.Spec.Advanced != nil && !reflect.DeepEqual(so.Spec.Advanced.ScalingModifiers, ScalingModifiers{})
 }
+
+// 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 {
+		return so.Spec.MinReplicaCount
+	}
+	tmp := defaultHPAMinReplicas
+	return &tmp
+}
+
+// getHPAMaxReplicas returns MaxReplicas based on definition in ScaledObject or default value if not defined
+func (so *ScaledObject) GetHPAMaxReplicas() int32 {
+	if so.Spec.MaxReplicaCount != nil {
+		return *so.Spec.MaxReplicaCount
+	}
+	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()
+	}
+	max := scaledObject.GetHPAMaxReplicas()
+
+	if min > max {
+		return fmt.Errorf("MinReplicaCount=%d must be less than MaxReplicaCount=%d", min, max)
+	}
+
+	if scaledObject.Spec.IdleReplicaCount != nil && *scaledObject.Spec.IdleReplicaCount >= min {
+		return fmt.Errorf("IdleReplicaCount=%d must be less than MinReplicaCount=%d", *scaledObject.Spec.IdleReplicaCount, min)
+	}
+
+	return nil
+}

apis/keda/v1alpha1/scaledobject_webhook.go

@@ -102,6 +102,7 @@ func validateWorkload(so *ScaledObject, action string) (admission.Warnings, erro
 		verifyTriggers,
 		verifyScaledObjects,
 		verifyHpas,
+		verifyReplicaCount,
 	}
 
 	for i := range verifyFunctions {
@@ -115,6 +116,15 @@ func validateWorkload(so *ScaledObject, action string) (admission.Warnings, erro
 	return nil, nil
 }
 
+func verifyReplicaCount(incomingSo *ScaledObject, action string) error {
+	err := CheckReplicaCountBoundsAreValid(incomingSo)
+	if err != nil {
+		scaledobjectlog.WithValues("name", incomingSo.Name).Error(err, "validation error")
+		metricscollector.RecordScaledObjectValidatingErrors(incomingSo.Namespace, action, "incorrect-replicas")
+	}
+	return nil
+}
+
 func verifyTriggers(incomingSo *ScaledObject, action string) error {
 	err := ValidateTriggers(incomingSo.Spec.Triggers)
 	if err != nil {

apis/keda/v1alpha1/scaledobject_webhook_test.go

@@ -142,6 +142,21 @@ var _ = It("shouldn't validate the so creation when there is another unmanaged h
 	}).Should(HaveOccurred())
 })
 
+var _ = It("shouldn't validate the so creation when the replica counts are wrong", func() {
+	namespaceName := "wrong-replica-count"
+	namespace := createNamespace(namespaceName)
+	so := createScaledObject(soName, namespaceName, workloadName, "apps/v1", "Deployment", false, map[string]string{}, "")
+	so.Spec.MinReplicaCount = ptr.To[int32](10)
+	so.Spec.MaxReplicaCount = ptr.To[int32](5)
+
+	err := k8sClient.Create(context.Background(), namespace)
+	Expect(err).ToNot(HaveOccurred())
+
+	Eventually(func() error {
+		return k8sClient.Create(context.Background(), so)
+	}).Should(HaveOccurred())
+})
+
 var _ = It("shouldn't validate the so creation when there is another unmanaged hpa and so has transfer-hpa-ownership activated", func() {
 
 	hpaName := "test-unmanaged-hpa-ownership"

controllers/keda/hpa.go

@@ -38,11 +38,6 @@ import (
 	version "github.com/kedacore/keda/v2/version"
 )
 
-const (
-	defaultHPAMinReplicas int32 = 1
-	defaultHPAMaxReplicas int32 = 100
-)
-
 // createAndDeployNewHPA creates and deploy HPA in the cluster for specified ScaledObject
 func (r *ScaledObjectReconciler) createAndDeployNewHPA(ctx context.Context, logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject, gvkr *kedav1alpha1.GroupVersionKindResource) error {
 	hpaName := getHPAName(scaledObject)
@@ -104,8 +99,8 @@ func (r *ScaledObjectReconciler) newHPAForScaledObject(ctx context.Context, logg
 		labels[key] = value
 	}
 
-	minReplicas := getHPAMinReplicas(scaledObject)
-	maxReplicas := getHPAMaxReplicas(scaledObject)
+	minReplicas := scaledObject.GetHPAMinReplicas()
+	maxReplicas := scaledObject.GetHPAMaxReplicas()
 
 	pausedCount, err := executor.GetPausedReplicaCount(scaledObject)
 	if err != nil {
@@ -340,20 +335,3 @@ func getHPAName(scaledObject *kedav1alpha1.ScaledObject) string {
 func getDefaultHpaName(scaledObject *kedav1alpha1.ScaledObject) string {
 	return fmt.Sprintf("keda-hpa-%s", scaledObject.Name)
 }
-
-// getHPAMinReplicas returns MinReplicas based on definition in ScaledObject or default value if not defined
-func getHPAMinReplicas(scaledObject *kedav1alpha1.ScaledObject) *int32 {
-	if scaledObject.Spec.MinReplicaCount != nil && *scaledObject.Spec.MinReplicaCount > 0 {
-		return scaledObject.Spec.MinReplicaCount
-	}
-	tmp := defaultHPAMinReplicas
-	return &tmp
-}
-
-// getHPAMaxReplicas returns MaxReplicas based on definition in ScaledObject or default value if not defined
-func getHPAMaxReplicas(scaledObject *kedav1alpha1.ScaledObject) int32 {
-	if scaledObject.Spec.MaxReplicaCount != nil {
-		return *scaledObject.Spec.MaxReplicaCount
-	}
-	return defaultHPAMaxReplicas
-}

controllers/keda/scaledobject_controller.go

@@ -257,7 +257,7 @@ func (r *ScaledObjectReconciler) reconcileScaledObject(ctx context.Context, logg
 		return message.ScaleTargetErrMsg, err
 	}
 
-	err = r.checkReplicaCountBoundsAreValid(scaledObject)
+	err = kedav1alpha1.CheckReplicaCountBoundsAreValid(scaledObject)
 	if err != nil {
 		return "ScaledObject doesn't have correct Idle/Min/Max Replica Counts specification", err
 	}
@@ -411,26 +411,6 @@ func (r *ScaledObjectReconciler) checkTargetResourceIsScalable(ctx context.Conte
 	return gvkr, nil
 }
 
-// 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 (r *ScaledObjectReconciler) checkReplicaCountBoundsAreValid(scaledObject *kedav1alpha1.ScaledObject) error {
-	min := int32(0)
-	if scaledObject.Spec.MinReplicaCount != nil {
-		min = *getHPAMinReplicas(scaledObject)
-	}
-	max := getHPAMaxReplicas(scaledObject)
-
-	if min > max {
-		return fmt.Errorf("MinReplicaCount=%d must be less than MaxReplicaCount=%d", min, max)
-	}
-
-	if scaledObject.Spec.IdleReplicaCount != nil && *scaledObject.Spec.IdleReplicaCount >= min {
-		return fmt.Errorf("IdleReplicaCount=%d must be less than MinReplicaCount=%d", *scaledObject.Spec.IdleReplicaCount, min)
-	}
-
-	return nil
-}
-
 // ensureHPAForScaledObjectExists ensures that in cluster exist up-to-date HPA for specified ScaledObject, returns true if a new HPA was created
 func (r *ScaledObjectReconciler) ensureHPAForScaledObjectExists(ctx context.Context, logger logr.Logger, scaledObject *kedav1alpha1.ScaledObject, gvkr *kedav1alpha1.GroupVersionKindResource) (bool, error) {
 	hpaName := getHPANameOnEnsure(scaledObject)