fix(ScaledObject): Check Default Limits from LimitRange for ScaledObj…

…ect Validations (#5377)

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 <[email protected]>

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))

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
+}

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,
+					},
+				},
+			},
+		},
+	}
+}

config/rbac/role.yaml

@@ -30,6 +30,13 @@ rules:
   - get
   - list
   - watch
+- apiGroups:
+  - ""
+  resources:
+  - limitranges
+  verbs:
+  - list
+  - watch
 - apiGroups:
   - ""
   resources:

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 {