From 6078d30236bb40859d3194ae6843ec08a0f1bb9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Guilherme=20Vanz?= Date: Fri, 9 Aug 2024 15:54:47 -0300 Subject: [PATCH] fix: check limits mutation. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The policy adds resource limits when they are missing in the containers. However, depending the requested resources and the policy configuration the users can hit a scenario where the policy mutates the resource adding a limit which is less than the requested resource. Therefore, Kubernetes will complain about it. To avoid this issue, the policy checks if the result of the mutation is valid. If it's not, reject the request to force the user to change the minimum request resource amount or adjust the policy configuration. Signed-off-by: José Guilherme Vanz --- e2e.bats | 11 +- settings.go | 4 +- ..._no_limit_resources_admission_request.json | 178 ++++++++++++++++++ validate.go | 39 +++- validate_test.go | 124 +++++++++--- 5 files changed, 325 insertions(+), 31 deletions(-) create mode 100644 test_data/deployment_with_requests_no_limit_resources_admission_request.json diff --git a/e2e.bats b/e2e.bats index 45fc351..9d764b0 100644 --- a/e2e.bats +++ b/e2e.bats @@ -63,7 +63,7 @@ @test "mutate deployment with limits but no request resources" { run kwctl run annotated-policy.wasm -r test_data/deployment_with_limits_admission_request.json \ - --settings-json '{"cpu": {"maxLimit": "4", "defaultRequest" : "2", "defaultLimit" : "2"}, "memory" : {"maxLimit": "4G", "defaultRequest" : "2G", "defaultLimit" : "2G"}}' + --settings-json '{"cpu": {"maxLimit": "4", "defaultRequest" : "1", "defaultLimit" : "1"}, "memory" : {"maxLimit": "4G", "defaultRequest" : "1G", "defaultLimit" : "1G"}}' [ "$status" -eq 0 ] [ $(expr "$output" : '.*allowed":true') -ne 0 ] @@ -158,3 +158,12 @@ [ $(expr "$output" : '.*invalid cpu settings.*') -ne 0 ] [ $(expr "$output" : '.*invalid memory settings.*') -ne 0 ] } + +@test "policy fails when the request resource is greater than the limit set by the policy" { + run kwctl run annotated-policy.wasm -r test_data/deployment_with_requests_no_limit_resources_admission_request.json \ + --settings-json '{"cpu": {"maxLimit": 2, "defaultRequest": 1, "defaultLimit": 1, "ignoreValues":false}, "memory" : {"maxLimit": "1Gi", "defaultRequest" : "200Mi", "defaultLimit" : "200Mi", "ignoreValues":false}, "ignoreImages": ["image:latest"]}' + + [ "$status" -eq 0 ] + [ $(expr "$output" : '.*allowed":false') -ne 0 ] + [ $(expr "$output" : ".*memory limit '200Mi' is less than the requested '250Mi' value.*") -ne 0 ] +} diff --git a/settings.go b/settings.go index cf8d73a..5d58aa5 100644 --- a/settings.go +++ b/settings.go @@ -74,8 +74,8 @@ func (s *Settings) Valid() error { } if cpuError != nil || memoryError != nil { // user want to validate only one type of resource. The other one should be ignored - if (cpuError == nil && errors.Is(memoryError, AllValuesAreZeroError{})) || (memoryError == nil && errors.Is(cpuError, AllValuesAreZeroError{})) { - return nil + if (cpuError == nil && errors.Is(memoryError, AllValuesAreZeroError{})) || (memoryError == nil && errors.Is(cpuError, AllValuesAreZeroError{})) { + return nil } return errors.Join(cpuError, memoryError) } diff --git a/test_data/deployment_with_requests_no_limit_resources_admission_request.json b/test_data/deployment_with_requests_no_limit_resources_admission_request.json new file mode 100644 index 0000000..980b3e6 --- /dev/null +++ b/test_data/deployment_with_requests_no_limit_resources_admission_request.json @@ -0,0 +1,178 @@ +{ + "dryRun": false, + "kind": { + "group": "apps", + "kind": "Deployment", + "version": "v1" + }, + "name": "nginx", + "namespace": "default", + "object": { + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": { + "annotations": { + "io.kubewarden.policy.echo.create": "true", + "kubectl.kubernetes.io/last-applied-configuration": "{\"apiVersion\":\"apps/v1\",\"kind\":\"Deployment\",\"metadata\":{\"annotations\":{\"io.kubewarden.policy.echo.create\":\"true\"},\"name\":\"nginx\",\"namespace\":\"default\"},\"spec\":{\"replicas\":0,\"selector\":{\"matchLabels\":{\"app\":\"nginx\"}},\"template\":{\"metadata\":{\"labels\":{\"app\":\"nginx\"}},\"spec\":{\"containers\":[{\"image\":\"nginx:latest\",\"name\":\"nginx\",\"ports\":[{\"containerPort\":80}]}]}}}}\n" + }, + "creationTimestamp": "2024-01-11T12:25:50Z", + "generation": 1, + "managedFields": [ + { + "apiVersion": "apps/v1", + "fieldsType": "FieldsV1", + "fieldsV1": { + "f:metadata": { + "f:annotations": { + ".": {}, + "f:io.kubewarden.policy.echo.create": {}, + "f:kubectl.kubernetes.io/last-applied-configuration": {} + } + }, + "f:spec": { + "f:progressDeadlineSeconds": {}, + "f:replicas": {}, + "f:revisionHistoryLimit": {}, + "f:selector": {}, + "f:strategy": { + "f:rollingUpdate": { + ".": {}, + "f:maxSurge": {}, + "f:maxUnavailable": {} + }, + "f:type": {} + }, + "f:template": { + "f:metadata": { + "f:labels": { + ".": {}, + "f:app": {} + } + }, + "f:spec": { + "f:containers": { + "k:{\"name\":\"nginx\"}": { + ".": {}, + "f:image": {}, + "f:imagePullPolicy": {}, + "f:name": {}, + "f:ports": { + ".": {}, + "k:{\"containerPort\":80,\"protocol\":\"TCP\"}": { + ".": {}, + "f:containerPort": {}, + "f:protocol": {} + } + }, + "f:resources": {}, + "f:terminationMessagePath": {}, + "f:terminationMessagePolicy": {} + } + }, + "f:dnsPolicy": {}, + "f:restartPolicy": {}, + "f:schedulerName": {}, + "f:securityContext": {}, + "f:terminationGracePeriodSeconds": {} + } + } + } + }, + "manager": "kubectl-client-side-apply", + "operation": "Update", + "time": "2024-01-11T12:25:50Z" + } + ], + "name": "nginx", + "namespace": "default", + "uid": "0663a366-270c-4d7c-a483-6f59d200fb22" + }, + "spec": { + "progressDeadlineSeconds": 600, + "replicas": 0, + "revisionHistoryLimit": 10, + "selector": { + "matchLabels": { + "app": "nginx" + } + }, + "strategy": { + "rollingUpdate": { + "maxSurge": "25%", + "maxUnavailable": "25%" + }, + "type": "RollingUpdate" + }, + "template": { + "metadata": { + "creationTimestamp": null, + "labels": { + "app": "nginx" + } + }, + "spec": { + "containers": [ + { + "image": "nginx:latest", + "imagePullPolicy": "Always", + "name": "nginx", + "ports": [ + { + "containerPort": 80, + "protocol": "TCP" + } + ], + "resources": { + "requests": { + "cpu": "1", + "memory": "250Mi" + }, + "limits": { + "cpu": "1" + } + }, + "terminationMessagePath": "/dev/termination-log", + "terminationMessagePolicy": "File" + } + ], + "dnsPolicy": "ClusterFirst", + "restartPolicy": "Always", + "schedulerName": "default-scheduler", + "securityContext": {}, + "terminationGracePeriodSeconds": 30 + } + } + }, + "status": {} + }, + "operation": "CREATE", + "options": { + "apiVersion": "meta.k8s.io/v1", + "fieldManager": "kubectl-client-side-apply", + "fieldValidation": "Strict", + "kind": "CreateOptions" + }, + "requestKind": { + "group": "apps", + "kind": "Deployment", + "version": "v1" + }, + "requestResource": { + "group": "apps", + "resource": "deployments", + "version": "v1" + }, + "resource": { + "group": "apps", + "resource": "deployments", + "version": "v1" + }, + "uid": "31403b86-f972-423e-8adb-c4bd79dc15cc", + "userInfo": { + "groups": [ + "system:masters", + "system:authenticated" + ], + "username": "system:admin" + } +} diff --git a/validate.go b/validate.go index 170f69a..a7781ab 100644 --- a/validate.go +++ b/validate.go @@ -2,6 +2,7 @@ package main import ( "encoding/json" + "errors" "fmt" "strings" @@ -66,7 +67,7 @@ func validateContainerResourceRequests(container *corev1.Container, settings *Se // We only check for the presence of the limits/requests, not their values. // Returns an error if the limits/requests are not set and IgnoreValues is set to true. func validateContainerResources(container *corev1.Container, settings *Settings) error { - if container.Resources == nil && ( settings.shouldIgnoreCpuValues() || settings.shouldIgnoreMemoryValues()) { + if container.Resources == nil && (settings.shouldIgnoreCpuValues() || settings.shouldIgnoreMemoryValues()) { missing := fmt.Sprintf("required Cpu:%t, Memory:%t", settings.shouldIgnoreCpuValues(), settings.shouldIgnoreMemoryValues()) return fmt.Errorf("container does not have any resource limits or requests: %s", missing) } @@ -93,6 +94,26 @@ func validateAndAdjustContainerResourceRequests(container *corev1.Container, set return mutated } +// Ensure that the limit is greater than or equal to the request +func isResourceLimitGreaterThanRequest(container *corev1.Container, resourceName string) error { + if !missingResourceQuantity(container.Resources.Requests, resourceName) && !missingResourceQuantity(container.Resources.Limits, resourceName) { + resourceStr := container.Resources.Limits[resourceName] + resourceLimit, err := resource.ParseQuantity(string(*resourceStr)) + if err != nil { + return errors.Join(fmt.Errorf("invalid %s limit", resourceName), err) + } + resourceStr = container.Resources.Requests[resourceName] + resourceRequest, err := resource.ParseQuantity(string(*resourceStr)) + if err != nil { + return errors.Join(fmt.Errorf("invalid %s request", resourceName), err) + } + if resourceLimit.Cmp(resourceRequest) < 0 { + return fmt.Errorf("%s limit '%s' is less than the requested '%s' value. Please, change the resource configuration or change the policy settings to accommodate the requested value.", resourceName, resourceLimit.String(), resourceRequest.String()) + } + } + return nil +} + // validateAndAdjustContainerResourceLimit validates the container against the passed resourceConfig // and mutates it if the validation didn't pass. // Returns true when it mutates the container. func validateAndAdjustContainerResourceLimit(container *corev1.Container, resourceName string, resourceConfig *ResourceConfiguration) (bool, error) { @@ -164,6 +185,22 @@ func validateAndAdjustContainer(container *corev1.Container, settings *Settings) return false, err } requestsMutation := validateAndAdjustContainerResourceRequests(container, settings) + if limitsMutation || requestsMutation { + // If the container has been mutated, we need to check that the limit is greater than the request + // for both CPU and Memory. If the limit is less than the request, we reject the request. + // Because the user need to adjust the resource or change the policy configuration. Otherwise, + // Kubernetes will not accept the resource mutated by the policy. + errorMsg := "There is an issue after resource limits mutation" + if requestsMutation { + errorMsg = "There is an issue after resource requests mutation" + } + if err := isResourceLimitGreaterThanRequest(container, "memory"); err != nil { + return false, errors.Join(errors.New(errorMsg), err) + } + if err := isResourceLimitGreaterThanRequest(container, "cpu"); err != nil { + return false, errors.Join(errors.New(errorMsg), err) + } + } return limitsMutation || requestsMutation, nil } diff --git a/validate_test.go b/validate_test.go index e9dc437..d57a50e 100644 --- a/validate_test.go +++ b/validate_test.go @@ -81,17 +81,18 @@ func TestContainerIsRequiredToHaveLimits(t *testing.T) { "memory": &oneGiMemoryQuantity, }, }, true, ""}, - {"no cpu limit", corev1.Container{ - Resources: &corev1.ResourceRequirements{ - Limits: map[string]*apimachinery_pkg_api_resource.Quantity{ - "memory": &oneGiMemoryQuantity, - }, - Requests: map[string]*apimachinery_pkg_api_resource.Quantity{ - "cpu": &oneCoreCpuQuantity, - "memory": &oneGiMemoryQuantity, + {"no cpu limit", + corev1.Container{ + Resources: &corev1.ResourceRequirements{ + Limits: map[string]*apimachinery_pkg_api_resource.Quantity{ + "memory": &oneGiMemoryQuantity, + }, + Requests: map[string]*apimachinery_pkg_api_resource.Quantity{ + "cpu": &oneCoreCpuQuantity, + "memory": &oneGiMemoryQuantity, + }, }, }, - }, Settings{ Cpu: &ResourceConfiguration{ @@ -359,21 +360,21 @@ func TestContainerIsRequiredToHaveLimits(t *testing.T) { MaxLimit: oneCore, }, Memory: &ResourceConfiguration{ - IgnoreValues: true, + IgnoreValues: true, DefaultLimit: oneGi, DefaultRequest: oneGi, MaxLimit: oneGi, }, }, &corev1.ResourceRequirements{ - Limits: map[string]*apimachinery_pkg_api_resource.Quantity{ - "memory": &twoGiMemoryQuantity, - "cpu": &twoCoreCpuQuantity, - }, - Requests: map[string]*apimachinery_pkg_api_resource.Quantity{ - "memory": &twoGiMemoryQuantity, - "cpu": &twoCoreCpuQuantity, - }, - }, false, "cpu limit '2' exceeds the max allowed value '1'"}, + Limits: map[string]*apimachinery_pkg_api_resource.Quantity{ + "memory": &twoGiMemoryQuantity, + "cpu": &twoCoreCpuQuantity, + }, + Requests: map[string]*apimachinery_pkg_api_resource.Quantity{ + "memory": &twoGiMemoryQuantity, + "cpu": &twoCoreCpuQuantity, + }, + }, false, "cpu limit '2' exceeds the max allowed value '1'"}, {"memory exceeds limit while ignore cpu values", corev1.Container{ Resources: &corev1.ResourceRequirements{ Limits: map[string]*apimachinery_pkg_api_resource.Quantity{ @@ -387,7 +388,7 @@ func TestContainerIsRequiredToHaveLimits(t *testing.T) { }, }, Settings{ Cpu: &ResourceConfiguration{ - IgnoreValues: true, + IgnoreValues: true, DefaultLimit: oneCore, DefaultRequest: oneCore, MaxLimit: oneCore, @@ -398,15 +399,85 @@ func TestContainerIsRequiredToHaveLimits(t *testing.T) { MaxLimit: oneGi, }, }, &corev1.ResourceRequirements{ + Limits: map[string]*apimachinery_pkg_api_resource.Quantity{ + "memory": &twoGiMemoryQuantity, + "cpu": &twoCoreCpuQuantity, + }, + Requests: map[string]*apimachinery_pkg_api_resource.Quantity{ + "memory": &twoGiMemoryQuantity, + "cpu": &twoCoreCpuQuantity, + }, + }, false, "memory limit '2Gi' exceeds the max allowed value '1Gi'"}, + {"no memory limit and request greater then the default limit value", + corev1.Container{ + Resources: &corev1.ResourceRequirements{ + Limits: map[string]*apimachinery_pkg_api_resource.Quantity{ + "cpu": &oneCoreCpuQuantity, + }, + Requests: map[string]*apimachinery_pkg_api_resource.Quantity{ + "cpu": &oneCoreCpuQuantity, + "memory": &twoGiMemoryQuantity, + }, + }, + }, + + Settings{ + Cpu: &ResourceConfiguration{ + DefaultLimit: oneCore, + DefaultRequest: oneCore, + MaxLimit: oneCore, + }, + Memory: &ResourceConfiguration{ + DefaultLimit: oneGi, + DefaultRequest: oneGi, + MaxLimit: oneGi, + }, + IgnoreImages: []string{}, + }, &corev1.ResourceRequirements{ Limits: map[string]*apimachinery_pkg_api_resource.Quantity{ - "memory": &twoGiMemoryQuantity, - "cpu": &twoCoreCpuQuantity, + "cpu": &oneCoreCpuQuantity, + "memory": &oneGiMemoryQuantity, }, Requests: map[string]*apimachinery_pkg_api_resource.Quantity{ + "cpu": &oneCoreCpuQuantity, "memory": &twoGiMemoryQuantity, + }, + }, false, "memory limit '1Gi' is less than the requested '2Gi' value"}, + {"no cpu limit and request greater then the default limit value", + corev1.Container{ + Resources: &corev1.ResourceRequirements{ + Limits: map[string]*apimachinery_pkg_api_resource.Quantity{ + "memory": &oneGiMemoryQuantity, + }, + Requests: map[string]*apimachinery_pkg_api_resource.Quantity{ + "cpu": &twoCoreCpuQuantity, + "memory": &oneGiMemoryQuantity, + }, + }, + }, + + Settings{ + Cpu: &ResourceConfiguration{ + DefaultLimit: oneCore, + DefaultRequest: oneCore, + MaxLimit: oneCore, + }, + Memory: &ResourceConfiguration{ + DefaultLimit: oneGi, + DefaultRequest: oneGi, + MaxLimit: oneGi, + }, + IgnoreImages: []string{}, + }, &corev1.ResourceRequirements{ + Limits: map[string]*apimachinery_pkg_api_resource.Quantity{ + "memory": &oneGiMemoryQuantity, + "cpu": &oneCoreCpuQuantity, + }, + Requests: map[string]*apimachinery_pkg_api_resource.Quantity{ "cpu": &twoCoreCpuQuantity, + "memory": &oneGiMemoryQuantity, }, - }, false, "memory limit '2Gi' exceeds the max allowed value '1Gi'"}, + }, false, "cpu limit '1' is less than the requested '2' value"}, } for _, test := range tests { @@ -420,15 +491,14 @@ func TestContainerIsRequiredToHaveLimits(t *testing.T) { t.Fatalf("expected error message with string '%s'. But no error has been returned", test.expectedErrorMsg) } if !strings.Contains(err.Error(), test.expectedErrorMsg) { - t.Errorf("invalid error message. Expected the string '%s' in the error. Got '%s'", test.expectedErrorMsg, err.Error()) + t.Fatalf("invalid error message. Expected the string '%s' in the error. Got '%s'", test.expectedErrorMsg, err.Error()) } } if mutated != test.shouldMutate { - t.Errorf("validation function does not report mutation flag correctly. Got: %t, expected: %t", mutated, test.shouldMutate) + t.Fatalf("validation function does not report mutation flag correctly. Got: %t, expected: %t", mutated, test.shouldMutate) } if diff := cmp.Diff(test.container.Resources, test.expectedResouceLimits); diff != "" { - t.Logf("%+v", test.container.Resources) - t.Error(diff) + t.Fatalf(diff) } }) }