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