Skip to content

Commit

Permalink
fix: allow resource settings with empty values.
Browse files Browse the repository at this point in the history
When the policy is used in the Rancher UI the policy settings are sent
with empty values when the user does not want to validate a resource
type (memory/cpu). This is causing issues because the policy does not
allow empty values in the settings. This commit fix that by allowing
only a single resource settings to be empty. Not both. Therefore, we fix
the issue in the Rancher UI and does not broken the policy usage.

The Rancher UI cannot be fixed in the questions.yaml file because it
does not have a feature that allow us to remove the boolean flag when
user does not enabled it. Therefore, the settings always sent empty
values.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
  • Loading branch information
jvanz committed Jun 7, 2024
1 parent c51db9f commit 21eae6a
Show file tree
Hide file tree
Showing 6 changed files with 248 additions and 7 deletions.
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ ignoreImages: ["ghcr.io/foo/bar:1.23", "myimage", "otherimages:v1"]
Users can skip the optional parts of the configuration, but an empty configuration is not
allowed. Thus, at least one of the configurations, `cpu`, or `memory` should
be defined including all their sub fields. All CPU and memory configuration
be defined. In other words, users can only remove or keep the values empty for a single
resource configuration. But not for both. All CPU and memory configuration
should be expressed using the [quantity
definitions](https://kubernetes.io/docs/reference/kubernetes-api/common-definitions/quantity/)
of Kubernetes.
Expand Down
37 changes: 37 additions & 0 deletions e2e.bats
Original file line number Diff line number Diff line change
Expand Up @@ -121,3 +121,40 @@
[ $(expr "$output" : '.*allowed":false') -ne 0 ]
[ $(expr "$output" : '.*patch.*') -eq 0 ]
}

@test "reject containers exceeding the expected CPU range ignoring memory values due missing values" {
run kwctl run annotated-policy.wasm -r test_data/pod_exceeding_range.json \
--settings-json '{"cpu": {"maxLimit": "1m", "defaultRequest" : "1m", "defaultLimit" : "1m"}, "memory" : {"ignoreValues": false}, "ignoreImages": ["image:latest"]}'

[ "$status" -eq 0 ]
[ $(expr "$output" : '.*allowed":false') -ne 0 ]
[ $(expr "$output" : '.*cpu limit.*exceeds the max allowed value.*') -ne 0 ]
[ $(expr "$output" : '.*patch.*') -eq 0 ]
}

@test "allow containers exceeding the expected CPU range when the resource should be ignore due missing values" {
run kwctl run annotated-policy.wasm -r test_data/pod_exceeding_cpu_range.json \
--settings-json '{"cpu": {"ignoreValues":false}, "memory" : {"maxLimit": "1Gi", "defaultRequest" : "1Gi", "defaultLimit" : "1Gi", "ignoreValues":false}, "ignoreImages": ["image:latest"]}'

[ "$status" -eq 0 ]
[ $(expr "$output" : '.*allowed":true') -ne 0 ]
[ $(expr "$output" : '.*patch.*') -eq 0 ]
}

@test "allow containers exceeding the expected memory range when the resource should be ignore due missing values" {
run kwctl run annotated-policy.wasm -r test_data/pod_exceeding_memory_range.json \
--settings-json '{"cpu": {"maxLimit": "1m", "defaultRequest" : "1m", "defaultLimit" : "1m", "ignoreVaues": false}, "memory" : {"ignoreValues":false}, "ignoreImages": ["image:latest"]}'

[ "$status" -eq 0 ]
[ $(expr "$output" : '.*allowed":true') -ne 0 ]
[ $(expr "$output" : '.*patch.*') -eq 0 ]
}

@test "invalid settings when both resources have empty values" {
run kwctl run annotated-policy.wasm -r test_data/pod_within_range.json \
--settings-json '{"cpu": {"ignoreVaues": false}, "memory" : {"ignoreValues":false}, "ignoreImages": ["image:latest"]}'

[ "$status" -ne 0 ]
[ $(expr "$output" : '.*invalid cpu settings.*') -ne 0 ]
[ $(expr "$output" : '.*invalid memory settings.*') -ne 0 ]
}
22 changes: 18 additions & 4 deletions settings.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,24 @@ type Settings struct {
IgnoreImages []string `json:"ignoreImages,omitempty"`
}

type AllValuesAreZeroError struct{}

func (e AllValuesAreZeroError) Error() string {
return "all the quantities must be defined"
}

func (s *Settings) shouldIgnoreCpuValues() bool {
return s.Cpu != nil && s.Cpu.IgnoreValues
return s.Cpu != nil && (s.Cpu.IgnoreValues || (!s.Cpu.IgnoreValues && s.Cpu.allValuesAreZero()))
}

func (s *Settings) shouldIgnoreMemoryValues() bool {
return s.Memory != nil && s.Memory.IgnoreValues
return s.Memory != nil && (s.Memory.IgnoreValues || (!s.Memory.IgnoreValues && s.Memory.allValuesAreZero()))
}

func (r *ResourceConfiguration) valid() error {

if r.MaxLimit.IsZero() && r.DefaultLimit.IsZero() && r.DefaultRequest.IsZero() && !r.IgnoreValues {
return fmt.Errorf("all the quantities must be defined")
if r.allValuesAreZero() && !r.IgnoreValues {
return AllValuesAreZeroError{}
}

if r.MaxLimit.Cmp(r.DefaultLimit) < 0 ||
Expand All @@ -45,6 +51,10 @@ func (r *ResourceConfiguration) valid() error {
return nil
}

func (r *ResourceConfiguration) allValuesAreZero() bool {
return r.MaxLimit.IsZero() && r.DefaultLimit.IsZero() && r.DefaultRequest.IsZero()
}

func (s *Settings) Valid() error {
if s.Cpu == nil && s.Memory == nil {
return fmt.Errorf("no settings provided. At least one resource limit or request must be verified")
Expand All @@ -63,6 +73,10 @@ 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
}
return errors.Join(cpuError, memoryError)
}
return nil
Expand Down
7 changes: 5 additions & 2 deletions settings_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,11 @@ func TestParsingSettings(t *testing.T) {
{"valid settings with cpu field only", []byte(`{"cpu": {"maxLimit": "1m", "defaultRequest": "1m", "defaultLimit": "1m"}}`), ""},
{"valid settings with memory fields only", []byte(`{"memory":{ "defaultLimit": "200M", "defaultRequest": "100M", "maxLimit": "500M"}}`), ""},
{"no suffix", []byte(`{"cpu": {"maxLimit": "3", "defaultLimit": "2", "defaultRequest": "1"}, "memory": {"maxLimit": "3", "defaultLimit": "2", "defaultRequest": "1"}, "ignoreImages": []}`), ""},
{"invalid cpu settings", []byte(`{"cpu": {"maxLimit": "2m", "defaultRequest": "3m", "defaultLimit": "4m"}, "memory":{ "defaultLimit": "2G", "defaultRequest": "1G", "maxLimit": "3G"}, "ignoreImages": ["image:latest"]}`), "invalid cpu settings"},
{"invalid memory settings", []byte(`{"cpu": {"maxLimit": "2m", "defaultRequest": "1m", "defaultLimit": "1m"}, "memory":{ "defaultLimit": "2G", "defaultRequest": "3G", "maxLimit": "1G"}, "ignoreImages": ["image:latest"]}`), "invalid memory settings"},
{"invalid cpu settings", []byte(`{"cpu": {"maxLimit": "2m", "defaultRequest": "3m", "defaultLimit": "4m"}, "memory":{ "defaultLimit": "2G", "defaultRequest": "1G", "maxLimit": "3G"}, "ignoreImages": ["image:latest"]}`), "default values cannot be greater than the max limit"},
{"invalid memory settings", []byte(`{"cpu": {"maxLimit": "2m", "defaultRequest": "1m", "defaultLimit": "1m"}, "memory":{ "defaultLimit": "2G", "defaultRequest": "3G", "maxLimit": "1G"}, "ignoreImages": ["image:latest"]}`), "default values cannot be greater than the max limit"},
{"valid settings with empty memory settings", []byte(`{"cpu": {"maxLimit": "1m", "defaultRequest": "1m", "defaultLimit": "1m"}, "memory":{"ignoreValues": false}, "ignoreImages": ["image:latest"]}`), ""},
{"valid settings with empty cpu settings", []byte(`{"cpu": {"ignoreValues": false}, "memory":{ "defaultLimit": "200M", "defaultRequest": "100M", "maxLimit": "500M", "ignoreValues": false}, "ignoreImages": ["image:latest"]}`), ""},
{"invalid settings with empty cpu and memory settings", []byte(`{"cpu": {"ignoreValues": false}, "memory":{"ignoreValues": false}, "ignoreImages": ["image:latest"]}`), "invalid cpu settings\nall the quantities must be defined\ninvalid memory settings\nall the quantities must be defined"},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
Expand Down
93 changes: 93 additions & 0 deletions test_data/pod_exceeding_cpu_range.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
{
"uid": "1299d386-525b-4032-98ae-1949f69f9cfc",
"kind": {
"group": "",
"version": "v1",
"kind": "Pod"
},
"resource": {
"group": "",
"version": "v1",
"resource": "pods"
},
"requestKind": {
"group": "",
"version": "v1",
"kind": "Pod"
},
"requestResource": {
"group": "",
"version": "v1",
"resource": "pods"
},
"name": "nginx",
"namespace": "default",
"operation": "CREATE",
"userInfo": {
"username": "kubernetes-admin",
"groups": [
"system:masters",
"system:authenticated"
]
},
"object": {
"kind": "Pod",
"apiVersion": "v1",
"metadata": {
"name": "nginx",
"namespace": "default",
"uid": "04dc7a5e-e1f1-4e34-8d65-2c9337a43e64",
"labels": {
"env": "test"
},
"annotations": {
"kubectl.kubernetes.io/last-applied-configuration": "{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"metadata\":{\"annotations\":{},\"labels\":{\"env\":\"test\"},\"name\":\"nginx\",\"namespace\":\"default\"},\"spec\":{\"containers\":[{\"image\":\"nginx\",\"imagePullPolicy\":\"IfNotPresent\",\"name\":\"nginx\"}],\"tolerations\":[{\"effect\":\"NoSchedule\",\"key\":\"example-key\",\"operator\":\"Exists\"}]}}\n"
}
},
"spec": {
"containers": [
{
"name": "pause",
"image": "registry.k8s.io/pause",
"resources": {
"limits": {
"cpu": "3m",
"memory": "1Gi"
},
"requests": {
"cpu": "3m",
"memory": "1Gi"
}
}
},
{
"name": "mycontainer",
"image": "image:latest",
"resources": {
"limits": {
"cpu": "2m",
"memory": "1Gi"
},
"requests": {
"cpu": "2m",
"memory": "1Gi"
}
}
}
],
"restartPolicy": "Always",
"terminationGracePeriodSeconds": 30,
"dnsPolicy": "ClusterFirst",
"serviceAccountName": "default",
"serviceAccount": "default",
"securityContext": {},
"schedulerName": "default-scheduler"
}
},
"oldObject": null,
"dryRun": false,
"options": {
"kind": "CreateOptions",
"apiVersion": "meta.k8s.io/v1"
}
}
93 changes: 93 additions & 0 deletions test_data/pod_exceeding_memory_range.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
{
"uid": "1299d386-525b-4032-98ae-1949f69f9cfc",
"kind": {
"group": "",
"version": "v1",
"kind": "Pod"
},
"resource": {
"group": "",
"version": "v1",
"resource": "pods"
},
"requestKind": {
"group": "",
"version": "v1",
"kind": "Pod"
},
"requestResource": {
"group": "",
"version": "v1",
"resource": "pods"
},
"name": "nginx",
"namespace": "default",
"operation": "CREATE",
"userInfo": {
"username": "kubernetes-admin",
"groups": [
"system:masters",
"system:authenticated"
]
},
"object": {
"kind": "Pod",
"apiVersion": "v1",
"metadata": {
"name": "nginx",
"namespace": "default",
"uid": "04dc7a5e-e1f1-4e34-8d65-2c9337a43e64",
"labels": {
"env": "test"
},
"annotations": {
"kubectl.kubernetes.io/last-applied-configuration": "{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"metadata\":{\"annotations\":{},\"labels\":{\"env\":\"test\"},\"name\":\"nginx\",\"namespace\":\"default\"},\"spec\":{\"containers\":[{\"image\":\"nginx\",\"imagePullPolicy\":\"IfNotPresent\",\"name\":\"nginx\"}],\"tolerations\":[{\"effect\":\"NoSchedule\",\"key\":\"example-key\",\"operator\":\"Exists\"}]}}\n"
}
},
"spec": {
"containers": [
{
"name": "pause",
"image": "registry.k8s.io/pause",
"resources": {
"limits": {
"cpu": "1m",
"memory": "3Gi"
},
"requests": {
"cpu": "1m",
"memory": "3Gi"
}
}
},
{
"name": "mycontainer",
"image": "image:latest",
"resources": {
"limits": {
"cpu": "1m",
"memory": "2Gi"
},
"requests": {
"cpu": "1m",
"memory": "2Gi"
}
}
}
],
"restartPolicy": "Always",
"terminationGracePeriodSeconds": 30,
"dnsPolicy": "ClusterFirst",
"serviceAccountName": "default",
"serviceAccount": "default",
"securityContext": {},
"schedulerName": "default-scheduler"
}
},
"oldObject": null,
"dryRun": false,
"options": {
"kind": "CreateOptions",
"apiVersion": "meta.k8s.io/v1"
}
}

0 comments on commit 21eae6a

Please sign in to comment.