Skip to content

Commit

Permalink
fix: check limits mutation.
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
jvanz committed Aug 9, 2024
1 parent f427370 commit c36574d
Show file tree
Hide file tree
Showing 5 changed files with 320 additions and 30 deletions.
9 changes: 9 additions & 0 deletions e2e.bats
Original file line number Diff line number Diff line change
Expand Up @@ -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 (mutated by the policy) \'200Mi\' is less than the request \'250Mi\'.*') -ne 0 ]
}
4 changes: 2 additions & 2 deletions settings.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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"
}
}
35 changes: 34 additions & 1 deletion validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package main

import (
"encoding/json"
"errors"
"fmt"
"strings"

Expand Down Expand Up @@ -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)
}
Expand All @@ -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 (mutated by the policy) '%s' is less than the request '%s'. Please, change the requested resource amount 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) {
Expand Down Expand Up @@ -164,6 +185,18 @@ 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.
if err := isResourceLimitGreaterThanRequest(container, "memory"); err != nil {
return false, err
}
if err := isResourceLimitGreaterThanRequest(container, "cpu"); err != nil {
return false, err
}
}
return limitsMutation || requestsMutation, nil
}

Expand Down
Loading

0 comments on commit c36574d

Please sign in to comment.