fix(ScaledObject): Check default limits from LimitRange for ScaledObject validations

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 {