diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b10604d825..9c6ae32d48a 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 {