From edecc39fd84f6bc30359c96ddb97f0bdb1a10075 Mon Sep 17 00:00:00 2001 From: Bhargav Ravuri Date: Mon, 15 Jan 2024 10:37:53 +0530 Subject: [PATCH] fix(ScaledObject): Check Default Limits from LimitRange for ScaledObject Validations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit KEDA admission webhook rejects ScaledObject requests with CPU or memory triggers when the resource limits (CPU/memory based on triggers) are not set in the pod spec. This is expected behavior. But if default limits are set in the LimitRange object in the same namespace, the admission webhook should allow the ScaledObject request, which currently doesn’t. This change will check if there is a LimitRange with default limits (CPU/memory based on triggers) in the namespace that ScaledObject is in, and allows the request to proceed. Also, added RBAC permissions for list & watch LimitRange. Updated Change Log. Signed-off-by: Bhargav Ravuri --- CHANGELOG.md | 1 + apis/keda/v1alpha1/scaledobject_webhook.go | 55 ++++++++++++- .../v1alpha1/scaledobject_webhook_test.go | 77 +++++++++++++++++++ config/rbac/role.yaml | 7 ++ controllers/keda/scaledobject_controller.go | 1 + 5 files changed, 139 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74b1916bcbb..ed81fe7dd6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -102,6 +102,7 @@ Here is an overview of all new **experimental** features: - **General**: Add CloudEventSource metrics in Prometheus & OpenTelemetry ([#3531](https://github.com/kedacore/keda/issues/3531)) - **General**: Add parameter queryParameters to prometheus-scaler ([#4962](https://github.com/kedacore/keda/issues/4962)) +- **General**: Add RBAC permissions for list & watch LimitRange, and check default limits from LimitRange for validations ([#5377](https://github.com/kedacore/keda/pull/5377)) - **General**: Add validations for replica counts when creating ScaledObjects ([#5288](https://github.com/kedacore/keda/issues/5288)) - **General**: Bubble up AuthRef TriggerAuthentication errors as ScaledObject events ([#5190](https://github.com/kedacore/keda/issues/5190)) - **General**: Enhance podIdentity Role Assumption in AWS by Direct Integration with OIDC/Federation ([#5178](https://github.com/kedacore/keda/issues/5178)) diff --git a/apis/keda/v1alpha1/scaledobject_webhook.go b/apis/keda/v1alpha1/scaledobject_webhook.go index e42ae8ca38e..2c3c919391a 100644 --- a/apis/keda/v1alpha1/scaledobject_webhook.go +++ b/apis/keda/v1alpha1/scaledobject_webhook.go @@ -318,14 +318,20 @@ func verifyCPUMemoryScalers(incomingSo *ScaledObject, action string, dryRun bool } if trigger.Type == cpuString { - if !isWorkloadResourceSet(container.Resources, corev1.ResourceCPU) { + // Fail if neither pod's container spec has a CPU limit specified, nor a default CPU limit is + // specified in LimitRange in the same namespace as the deployment + if !isWorkloadResourceSet(container.Resources, corev1.ResourceCPU) && + !isContainerResourceLimitSet(context.Background(), incomingSo.Namespace, corev1.ResourceCPU) { err := fmt.Errorf("the scaledobject has a cpu trigger but the container %s doesn't have the cpu request defined", container.Name) scaledobjectlog.Error(err, "validation error") metricscollector.RecordScaledObjectValidatingErrors(incomingSo.Namespace, action, "missing-requests") return err } } else if trigger.Type == memoryString { - if !isWorkloadResourceSet(container.Resources, corev1.ResourceMemory) { + // Fail if neither pod's container spec has a memory limit specified, nor a default memory limit is + // specified in LimitRange in the same namespace as the deployment + if !isWorkloadResourceSet(container.Resources, corev1.ResourceMemory) && + !isContainerResourceLimitSet(context.Background(), incomingSo.Namespace, corev1.ResourceMemory) { err := fmt.Errorf("the scaledobject has a memory trigger but the container %s doesn't have the memory request defined", container.Name) scaledobjectlog.Error(err, "validation error") metricscollector.RecordScaledObjectValidatingErrors(incomingSo.Namespace, action, "missing-requests") @@ -463,3 +469,48 @@ func isWorkloadResourceSet(rr corev1.ResourceRequirements, name corev1.ResourceN limits, limitsSet := rr.Limits[name] return (requestsSet || limitsSet) && (requests.AsApproximateFloat64() > 0 || limits.AsApproximateFloat64() > 0) } + +// isContainerResourceSetInLimitRangeObject checks if the LimitRange item has the default limits and requests +// specified for the container type. Returns false if the default limit/request value is not set, or if set to zero, +// for the container. +func isContainerResourceSetInLimitRangeObject(item corev1.LimitRangeItem, resourceName corev1.ResourceName) bool { + request, isRequestSet := item.DefaultRequest[resourceName] + limit, isLimitSet := item.Default[resourceName] + + return (isRequestSet || isLimitSet) && + (request.AsApproximateFloat64() > 0 || limit.AsApproximateFloat64() > 0) && + item.Type == corev1.LimitTypeContainer +} + +// isContainerResourceLimitSet checks if the default limit/request is set for the container type in LimitRanges, +// in the namespace. +func isContainerResourceLimitSet(ctx context.Context, namespace string, triggerType corev1.ResourceName) bool { + limitRangeList := &corev1.LimitRangeList{} + listOps := &client.ListOptions{ + Namespace: namespace, + } + + // List limit ranges in the namespace + if err := kc.List(ctx, limitRangeList, listOps); err != nil { + scaledobjectlog.WithValues("namespace", namespace). + Error(err, "failed to list limitRanges in namespace") + + return false + } + + // Check in the LimitRange's list if at least one item has the default limit/request set + for _, limitRange := range limitRangeList.Items { + for _, limit := range limitRange.Spec.Limits { + if isContainerResourceSetInLimitRangeObject(limit, triggerType) { + return true + } + } + } + + // When no LimitRanges are found in the namespace, or if the default limit/request is not set for container type + // in all of the LimitRanges, return false + scaledobjectlog.WithValues("namespace", namespace). + Error(nil, "no container limit range found in namespace") + + return false +} diff --git a/apis/keda/v1alpha1/scaledobject_webhook_test.go b/apis/keda/v1alpha1/scaledobject_webhook_test.go index c119dba38bf..2edfe24f07a 100644 --- a/apis/keda/v1alpha1/scaledobject_webhook_test.go +++ b/apis/keda/v1alpha1/scaledobject_webhook_test.go @@ -279,6 +279,61 @@ var _ = It("shouldn't validate the so creation with cpu and memory when deployme }).Should(HaveOccurred()) }) +// This test checks whether the validation fails when the CPU and memory resource limits are missing in pod spec (in +// deployment) and there are CPU and memory triggers in ScaledObject. This is a test for already existing behavior. +// See github.com/kedacore/keda/issues/5348 +var _ = It("shouldn't validate the SO creation with CPU and memory when deployment doesn't have CPU and memory", func() { + namespaceName := "resource-default-limits-missing" + namespace := createNamespace(namespaceName) + deployment := createDeployment(namespaceName, false, false) + scaledObject := createScaledObject(soName, namespaceName, workloadName, "apps/v1", "Deployment", true, map[string]string{}, "") + + // Create namespace + err := k8sClient.Create(context.Background(), namespace) + Expect(err).ToNot(HaveOccurred()) + + // Create deployment + err = k8sClient.Create(context.Background(), deployment) + Expect(err).ToNot(HaveOccurred()) + + // Create scaled object, asynchronously + Eventually(func() error { + return k8sClient.Create(context.Background(), scaledObject) + }).Should(HaveOccurred()) +}) + +// This test checks whether the validation fails when the CPU and memory resource limits are missing in pod spec (in +// deployment), but there are default limits specified in LimitRange in the same namespace, and there are CPU and +// memory triggers in ScaledObject. This a test for newly added behavior after fixing the following issue. +// See github.com/kedacore/keda/issues/5348 +var _ = It("should validate the SO creation with CPU and memory when deployment doesn't have CPU and memory, but LimitRange has the limits specified", func() { + namespaceName := "resource-default-limits-in-limitrange" + limitRangeName := "test-limit-range" + cpuLimit := resource.NewMilliQuantity(100, resource.DecimalSI) + memoryLimit := resource.NewMilliQuantity(100, resource.DecimalSI) + namespace := createNamespace(namespaceName) + deployment := createDeployment(namespaceName, false, false) + limitRange := createLimitRange(limitRangeName, namespaceName, v1.LimitTypeContainer, cpuLimit, memoryLimit) + scaledObject := createScaledObject(soName, namespaceName, workloadName, "apps/v1", "Deployment", true, map[string]string{}, "") + + // Create namespace + err := k8sClient.Create(context.Background(), namespace) + Expect(err).ToNot(HaveOccurred()) + + // Create limit range + err = k8sClient.Create(context.Background(), limitRange) + Expect(err).ToNot(HaveOccurred()) + + // Create deployment + err = k8sClient.Create(context.Background(), deployment) + Expect(err).ToNot(HaveOccurred()) + + // Create scaled object, asynchronously + Eventually(func() error { + return k8sClient.Create(context.Background(), scaledObject) + }).ShouldNot(HaveOccurred()) +}) + var _ = It("shouldn't validate the so creation with cpu and memory when deployment hasn't got cpu request", func() { namespaceName := "deployment-no-cpu-request" @@ -1190,3 +1245,25 @@ func createScaledObjectScalingModifiers(namespace string, sm ScalingModifiers, t }, } } + +// createLimitRange creates a LimitRange resource in the specified namespace. The CPU and memory are pointers for easy +// use of constructor methods like NewQuantity, NewMilliQuantity, etc. directly at the caller. +func createLimitRange(name, namespace string, limitType v1.LimitType, cpu, memory *resource.Quantity) *v1.LimitRange { + return &v1.LimitRange{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: v1.LimitRangeSpec{ + Limits: []v1.LimitRangeItem{ + { + Type: limitType, + Default: v1.ResourceList{ + v1.ResourceCPU: *cpu, + v1.ResourceMemory: *memory, + }, + }, + }, + }, + } +} diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index c6cefcaf645..6ca6d10cfe0 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -30,6 +30,13 @@ rules: - get - list - watch +- apiGroups: + - "" + resources: + - limitranges + verbs: + - list + - watch - apiGroups: - "" resources: diff --git a/controllers/keda/scaledobject_controller.go b/controllers/keda/scaledobject_controller.go index 9e79f624acc..dec4c5f0c23 100755 --- a/controllers/keda/scaledobject_controller.go +++ b/controllers/keda/scaledobject_controller.go @@ -62,6 +62,7 @@ import ( // +kubebuilder:rbac:groups="*",resources="*",verbs=get // +kubebuilder:rbac:groups="apps",resources=deployments;statefulsets,verbs=list;watch // +kubebuilder:rbac:groups="coordination.k8s.io",namespace=keda,resources=leases,verbs="*" +// +kubebuilder:rbac:groups="",resources="limitranges",verbs=list;watch // ScaledObjectReconciler reconciles a ScaledObject object type ScaledObjectReconciler struct {