From 6f540b1195bbb780821a06707ba5d6612cf80ef2 Mon Sep 17 00:00:00 2001 From: Bojan Zelic Date: Fri, 15 Dec 2023 06:20:46 -0700 Subject: [PATCH] General: validate replica counts when creating scaled objects (#5289) Signed-off-by: Bojan Zelic --- CHANGELOG.md | 1 + apis/keda/v1alpha1/scaledobject_types.go | 41 +++++++++++++++++++ apis/keda/v1alpha1/scaledobject_webhook.go | 10 +++++ .../v1alpha1/scaledobject_webhook_test.go | 15 +++++++ controllers/keda/hpa.go | 26 +----------- controllers/keda/scaledobject_controller.go | 22 +--------- 6 files changed, 70 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2aabe09dc4e..508ce44ade3 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/apis/keda/v1alpha1/scaledobject_types.go b/apis/keda/v1alpha1/scaledobject_types.go index 08cc0187c20..a7357f5f804 100644 --- a/apis/keda/v1alpha1/scaledobject_types.go +++ b/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 +} diff --git a/apis/keda/v1alpha1/scaledobject_webhook.go b/apis/keda/v1alpha1/scaledobject_webhook.go index bc3cd48216e..812877b92f0 100644 --- a/apis/keda/v1alpha1/scaledobject_webhook.go +++ b/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 { diff --git a/apis/keda/v1alpha1/scaledobject_webhook_test.go b/apis/keda/v1alpha1/scaledobject_webhook_test.go index c02361ac432..6ff6f0b513b 100644 --- a/apis/keda/v1alpha1/scaledobject_webhook_test.go +++ b/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" diff --git a/controllers/keda/hpa.go b/controllers/keda/hpa.go index 4dc9d3d08d9..0c13926b924 100644 --- a/controllers/keda/hpa.go +++ b/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 -} diff --git a/controllers/keda/scaledobject_controller.go b/controllers/keda/scaledobject_controller.go index b0420c1e9a3..8ff5acd98d1 100755 --- a/controllers/keda/scaledobject_controller.go +++ b/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)