diff --git a/CHANGELOG.md b/CHANGELOG.md index dbba7b57a05..3887dac7287 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,7 +58,7 @@ To learn more about active deprecations, we recommend checking [GitHub Discussio ### New -- TODO ([#XXX](https://github.com/kedacore/keda/issues/XXX)) +- **General**: Fix and optimize all webhook checkReplicas code ([#6344](https://github.com/kedacore/keda/pull/6344)) #### Experimental diff --git a/apis/keda/v1alpha1/scaledobject_types.go b/apis/keda/v1alpha1/scaledobject_types.go index 37c5e640963..e40da6c51b2 100644 --- a/apis/keda/v1alpha1/scaledobject_types.go +++ b/apis/keda/v1alpha1/scaledobject_types.go @@ -239,13 +239,19 @@ 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 return &tmp } +// GetDefaultHPAMinReplicas returns defaultHPAMinReplicas +func (so *ScaledObject) GetDefaultHPAMinReplicas() *int32 { + 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 { @@ -254,21 +260,49 @@ func (so *ScaledObject) GetHPAMaxReplicas() int32 { return defaultHPAMaxReplicas } +// GetDefaultHPAMaxReplicas returns defaultHPAMaxReplicas +func (so *ScaledObject) GetDefaultHPAMaxReplicas() int32 { + return defaultHPAMaxReplicas +} + +// CheckReplicasNotNegative checks that Min/Max ReplicaCount defined in ScaledObject are not negative +func (so *ScaledObject) CheckReplicasNotNegative(replicas ...int32) bool { + for _, replica := range replicas { + if replica < 0 { + return false + } + } + return true +} + +// GetIdleReplicasIfDefined returns bool based on whether idleRelicas is defined +func (so *ScaledObject) GetIdleReplicasIfDefined() bool { + return so.Spec.IdleReplicaCount != 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 CheckReplicaCountBoundsAreValid(scaledObject *ScaledObject) error { - min := int32(0) - if scaledObject.Spec.MinReplicaCount != nil { - min = *scaledObject.GetHPAMinReplicas() + var idleReplicas *int32 + minReplicas := *scaledObject.GetHPAMinReplicas() + maxReplicas := scaledObject.GetHPAMaxReplicas() + idleReplicasDefined := scaledObject.GetIdleReplicasIfDefined() + idleReplicas = scaledObject.Spec.IdleReplicaCount + + if !scaledObject.CheckReplicasNotNegative(minReplicas, maxReplicas) { + return fmt.Errorf("MinReplicaCount=%d, MaxReplicaCount=%d must not be negative", minReplicas, maxReplicas) + } + + if idleReplicasDefined && *idleReplicas < 0 { + return fmt.Errorf("IdleReplicaCount=%d must not be negative", *idleReplicas) } - 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 idleReplicasDefined && *idleReplicas >= minReplicas { + return fmt.Errorf("IdleReplicaCount=%d must be less than MinReplicaCount=%d", *scaledObject.Spec.IdleReplicaCount, minReplicas) } return nil diff --git a/apis/keda/v1alpha1/scaledobject_webhook.go b/apis/keda/v1alpha1/scaledobject_webhook.go index 41bd0355596..e4583730b15 100644 --- a/apis/keda/v1alpha1/scaledobject_webhook.go +++ b/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 { diff --git a/controllers/keda/hpa.go b/controllers/keda/hpa.go index 5fb2c835eb7..88f794955f2 100644 --- a/controllers/keda/hpa.go +++ b/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 = scaledObject.GetDefaultHPAMinReplicas() + } + + if maxReplicas == 0 { + maxReplicas = scaledObject.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