From bf214d8ec67c9d724f1844d4f12aa6c7c3b1fe61 Mon Sep 17 00:00:00 2001 From: Claudio Netto Date: Sun, 16 Jul 2023 13:00:45 -0300 Subject: [PATCH 1/6] fix: don't validate workload with only resource limits set Signed-off-by: Claudio Netto --- apis/keda/v1alpha1/scaledobject_webhook.go | 11 ++-- .../v1alpha1/scaledobject_webhook_test.go | 62 +++++++++++++++++++ 2 files changed, 67 insertions(+), 6 deletions(-) diff --git a/apis/keda/v1alpha1/scaledobject_webhook.go b/apis/keda/v1alpha1/scaledobject_webhook.go index b487024b013..ced857ab75b 100644 --- a/apis/keda/v1alpha1/scaledobject_webhook.go +++ b/apis/keda/v1alpha1/scaledobject_webhook.go @@ -316,19 +316,18 @@ func verifyCPUMemoryScalers(incomingSo *ScaledObject, action string, dryRun bool if conainerName != "" && container.Name != conainerName { continue } + if trigger.Type == cpuString { - if container.Resources.Requests == nil || - container.Resources.Requests.Cpu() == nil || - container.Resources.Requests.Cpu().AsApproximateFloat64() == 0 { + if container.Resources.Requests.Cpu().AsApproximateFloat64() == 0 && + container.Resources.Limits.Cpu().AsApproximateFloat64() == 0 { 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 container.Resources.Requests == nil || - container.Resources.Requests.Memory() == nil || - container.Resources.Requests.Memory().AsApproximateFloat64() == 0 { + if container.Resources.Requests.Memory().AsApproximateFloat64() == 0 && + container.Resources.Limits.Memory().AsApproximateFloat64() == 0 { 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") diff --git a/apis/keda/v1alpha1/scaledobject_webhook_test.go b/apis/keda/v1alpha1/scaledobject_webhook_test.go index 8c9dfe00df5..a9f5543e818 100644 --- a/apis/keda/v1alpha1/scaledobject_webhook_test.go +++ b/apis/keda/v1alpha1/scaledobject_webhook_test.go @@ -416,6 +416,68 @@ var _ = It("should validate so creation when min replicas is > 0 with only cpu s }) +var _ = It("should not validate ScaledObject creation when deployment only provides cpu resource limits", func() { + + namespaceName := "only-cpu-resource-limits-set" + namespace := createNamespace(namespaceName) + + err := k8sClient.Create(context.Background(), namespace) + Expect(err).ToNot(HaveOccurred()) + + workload := createDeployment(namespaceName, false, false) + workload.Spec.Template.Spec.Containers[0].Resources.Limits = v1.ResourceList{ + v1.ResourceCPU: *resource.NewMilliQuantity(100, resource.DecimalSI), + } + + err = k8sClient.Create(context.Background(), workload) + Expect(err).ToNot(HaveOccurred()) + + so := createScaledObject(soName, namespaceName, workloadName, "apps/v1", "Deployment", false, map[string]string{}, "") + so.Spec.Triggers = []ScaleTriggers{ + { + Type: "cpu", + Metadata: map[string]string{ + "cpu": "10", + }, + }, + } + + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).ShouldNot(HaveOccurred()) +}) + +var _ = It("should not validate ScaledObject creation when deployment only provides memory resource limits", func() { + + namespaceName := "only-memory-resource-limits-set" + namespace := createNamespace(namespaceName) + + err := k8sClient.Create(context.Background(), namespace) + Expect(err).ToNot(HaveOccurred()) + + workload := createDeployment(namespaceName, false, false) + workload.Spec.Template.Spec.Containers[0].Resources.Limits = v1.ResourceList{ + v1.ResourceMemory: *resource.NewMilliQuantity(1024, resource.DecimalSI), + } + + err = k8sClient.Create(context.Background(), workload) + Expect(err).ToNot(HaveOccurred()) + + so := createScaledObject(soName, namespaceName, workloadName, "apps/v1", "Deployment", false, map[string]string{}, "") + so.Spec.Triggers = []ScaleTriggers{ + { + Type: "memory", + Metadata: map[string]string{ + "memory": "512Mi", + }, + }, + } + + Eventually(func() error { + return k8sClient.Create(context.Background(), so) + }).ShouldNot(HaveOccurred()) +}) + var _ = It("should validate the so update if it's removing the finalizer even if it's invalid", func() { namespaceName := "removing-finalizers" From 11a7abf15994d89c159adba1406cc02a2e8ce8df Mon Sep 17 00:00:00 2001 From: Claudio Netto Date: Mon, 17 Jul 2023 17:12:00 -0300 Subject: [PATCH 2/6] docs: add resolved issue in changelog Signed-off-by: Claudio Netto --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 24bda4a24df..d7f30e2273f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,6 +78,7 @@ Here is an overview of all new **experimental** features: ### Fixes +- **General**: Admission webhook does not reject workloads with only resource limits provided ([#4802](https://github.com/kedacore/keda/issues/4802)) - **General**: Fix CVE-2023-39325 in golang.org/x/net ([#5122](https://github.com/kedacore/keda/issues/5122)) - **General**: Fix otelgrpc DoS vulnerability ([#5208](https://github.com/kedacore/keda/issues/5208)) - **General**: Prevented memory leak generated by not correctly cleaning http connections ([#5248](https://github.com/kedacore/keda/issues/5248)) From 4a4316321ac0811b5979804a08ff84fc5d431f28 Mon Sep 17 00:00:00 2001 From: Claudio Netto Date: Tue, 1 Aug 2023 19:03:15 -0300 Subject: [PATCH 3/6] fix: preventing possible panic due to nil pointer dereferencing Signed-off-by: Claudio Netto --- apis/keda/v1alpha1/scaledobject_webhook.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/apis/keda/v1alpha1/scaledobject_webhook.go b/apis/keda/v1alpha1/scaledobject_webhook.go index ced857ab75b..af97736e2d3 100644 --- a/apis/keda/v1alpha1/scaledobject_webhook.go +++ b/apis/keda/v1alpha1/scaledobject_webhook.go @@ -318,16 +318,16 @@ func verifyCPUMemoryScalers(incomingSo *ScaledObject, action string, dryRun bool } if trigger.Type == cpuString { - if container.Resources.Requests.Cpu().AsApproximateFloat64() == 0 && - container.Resources.Limits.Cpu().AsApproximateFloat64() == 0 { + if (container.Resources.Requests == nil || container.Resources.Requests.Cpu() == nil || container.Resources.Requests.Cpu().AsApproximateFloat64() == 0) && + (container.Resources.Limits == nil || container.Resources.Limits.Cpu() == nil || container.Resources.Limits.Cpu().AsApproximateFloat64() == 0) { 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 container.Resources.Requests.Memory().AsApproximateFloat64() == 0 && - container.Resources.Limits.Memory().AsApproximateFloat64() == 0 { + if (container.Resources.Requests == nil || container.Resources.Requests.Memory() == nil || container.Resources.Requests.Memory().AsApproximateFloat64() == 0) && + (container.Resources.Limits == nil || container.Resources.Limits.Memory() == nil || container.Resources.Limits.Memory().AsApproximateFloat64() == 0) { 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") From 13226a63f9aba7f4097061556e19f11f3d0e5406 Mon Sep 17 00:00:00 2001 From: Claudio Netto Date: Tue, 1 Aug 2023 19:05:07 -0300 Subject: [PATCH 4/6] test: set CPU/memory scaler specs properly Signed-off-by: Claudio Netto --- apis/keda/v1alpha1/scaledobject_webhook_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apis/keda/v1alpha1/scaledobject_webhook_test.go b/apis/keda/v1alpha1/scaledobject_webhook_test.go index a9f5543e818..c119dba38bf 100644 --- a/apis/keda/v1alpha1/scaledobject_webhook_test.go +++ b/apis/keda/v1alpha1/scaledobject_webhook_test.go @@ -437,7 +437,7 @@ var _ = It("should not validate ScaledObject creation when deployment only provi { Type: "cpu", Metadata: map[string]string{ - "cpu": "10", + "value": "10", }, }, } @@ -468,7 +468,7 @@ var _ = It("should not validate ScaledObject creation when deployment only provi { Type: "memory", Metadata: map[string]string{ - "memory": "512Mi", + "value": "512Mi", }, }, } From 1c655d8782556a0164bf0f9ebdb406250fd4d2b0 Mon Sep 17 00:00:00 2001 From: Claudio Netto Date: Fri, 4 Aug 2023 10:57:55 -0300 Subject: [PATCH 5/6] refactor: moving the check of resource validation to helper function Signed-off-by: Claudio Netto --- apis/keda/v1alpha1/scaledobject_webhook.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/apis/keda/v1alpha1/scaledobject_webhook.go b/apis/keda/v1alpha1/scaledobject_webhook.go index af97736e2d3..66d2c76d60a 100644 --- a/apis/keda/v1alpha1/scaledobject_webhook.go +++ b/apis/keda/v1alpha1/scaledobject_webhook.go @@ -318,16 +318,14 @@ func verifyCPUMemoryScalers(incomingSo *ScaledObject, action string, dryRun bool } if trigger.Type == cpuString { - if (container.Resources.Requests == nil || container.Resources.Requests.Cpu() == nil || container.Resources.Requests.Cpu().AsApproximateFloat64() == 0) && - (container.Resources.Limits == nil || container.Resources.Limits.Cpu() == nil || container.Resources.Limits.Cpu().AsApproximateFloat64() == 0) { + if !isWorkloadResourceSet(container.Resources, 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 (container.Resources.Requests == nil || container.Resources.Requests.Memory() == nil || container.Resources.Requests.Memory().AsApproximateFloat64() == 0) && - (container.Resources.Limits == nil || container.Resources.Limits.Memory() == nil || container.Resources.Limits.Memory().AsApproximateFloat64() == 0) { + if !isWorkloadResourceSet(container.Resources, 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") @@ -459,3 +457,9 @@ func castToFloatIfNecessary(formula string) string { } return "float(" + formula + ")" } + +func isWorkloadResourceSet(rr corev1.ResourceRequirements, name corev1.ResourceName) bool { + requests, requestsSet := rr.Requests[name] + limits, limitsSet := rr.Limits[name] + return (requestsSet || limitsSet) && (requests.AsApproximateFloat64() > 0 || limits.AsApproximateFloat64() > 0) +} From 465aebb78703b5a29e47d8e10341139be6169b01 Mon Sep 17 00:00:00 2001 From: Claudio Netto Date: Fri, 22 Dec 2023 10:07:14 -0300 Subject: [PATCH 6/6] test: +e2e to cover that webhook dont validate workload with only limits set Signed-off-by: Claudio Netto --- .../scaled_object_validation_test.go | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/tests/internals/scaled_object_validation/scaled_object_validation_test.go b/tests/internals/scaled_object_validation/scaled_object_validation_test.go index ad1f0fc9c69..ca1bfbdc944 100644 --- a/tests/internals/scaled_object_validation/scaled_object_validation_test.go +++ b/tests/internals/scaled_object_validation/scaled_object_validation_test.go @@ -173,6 +173,8 @@ func TestScaledObjectValidations(t *testing.T) { testMissingMemory(t, data) + testWorkloadWithOnlyLimits(t, data) + DeleteKubernetesResources(t, testNamespace, data, templates) } @@ -249,6 +251,66 @@ func testMissingMemory(t *testing.T, data templateData) { assert.Contains(t, err.Error(), fmt.Sprintf("the scaledobject has a memory trigger but the container %s doesn't have the memory request defined", deploymentName)) } +func testWorkloadWithOnlyLimits(t *testing.T, data templateData) { + t.Log("--- workload with only resource limits set ---") + + data.DeploymentName = fmt.Sprintf("%s-deploy-only-limits", testName) + + customDeploymentTemplate := ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: {{.DeploymentName}} + namespace: {{.TestNamespace}} + labels: + app: {{.DeploymentName}} +spec: + replicas: 1 + selector: + matchLabels: + app: {{.DeploymentName}} + template: + metadata: + labels: + app: {{.DeploymentName}} + spec: + containers: + - name: {{.DeploymentName}} + image: nginxinc/nginx-unprivileged + resources: + limits: + cpu: 50m +` + + KubectlApplyWithTemplate(t, data, "deploymentTemplate", customDeploymentTemplate) + WaitForDeploymentReplicaReadyCount(t, GetKubernetesClient(t), data.DeploymentName, data.TestNamespace, 1, 10, 5) + + t.Log("deployment was updated with resource limits") + + data.ScaledObjectName = fmt.Sprintf("%s-so-only-limits", testName) + + customScaledObjectTemplate := ` +apiVersion: keda.sh/v1alpha1 +kind: ScaledObject +metadata: + name: {{.ScaledObjectName}} + namespace: {{.TestNamespace}} +spec: + scaleTargetRef: + name: {{.DeploymentName}} + minReplicaCount: 1 + maxReplicaCount: 1 + triggers: + - type: cpu + metadata: + type: Utilization + value: "50" +` + + err := KubectlApplyWithErrors(t, data, "scaledObjectTemplate", customScaledObjectTemplate) + assert.NoError(t, err, "Deployment with only resource limits set should be validated") +} + func getTemplateData() (templateData, []Template) { return templateData{ TestNamespace: testNamespace,