From dfd30a49018b44c89a82d631d010a1af542bf411 Mon Sep 17 00:00:00 2001 From: Chad Roberts Date: Fri, 6 Dec 2024 18:23:31 -0500 Subject: [PATCH] [main/2.10.2] Add resource request and limit validation when creating a namespace (#550) * Add resource request and limit validation when creating a namespace * Update test for number of namespace admitters * cleaning up lint errors * Allow for empty resource limit annotation to be present * Update to allow for partial request/limits --- docs.md | 4 + pkg/resources/core/v1/namespace/Namespace.md | 3 + .../core/v1/namespace/requestlimits.go | 123 +++++++++++++ .../core/v1/namespace/requestlimits_test.go | 161 ++++++++++++++++++ pkg/resources/core/v1/namespace/validator.go | 8 +- .../core/v1/namespace/validator_test.go | 2 +- 6 files changed, 297 insertions(+), 4 deletions(-) create mode 100644 pkg/resources/core/v1/namespace/requestlimits.go create mode 100644 pkg/resources/core/v1/namespace/requestlimits_test.go diff --git a/docs.md b/docs.md index d36df09fc..8d329ae8b 100644 --- a/docs.md +++ b/docs.md @@ -60,6 +60,10 @@ The following labels are considered relevant for PSA enforcement: - pod-security.kubernetes.io/warn - pod-security.kubernetes.io/warn-version +#### Namespace resource limit validation + +Validation ensures that the limits for cpu/memory must not be less than the requests for cpu/memory. + ## Secret ### Validation Checks diff --git a/pkg/resources/core/v1/namespace/Namespace.md b/pkg/resources/core/v1/namespace/Namespace.md index 23fb3879d..b6987df81 100644 --- a/pkg/resources/core/v1/namespace/Namespace.md +++ b/pkg/resources/core/v1/namespace/Namespace.md @@ -20,3 +20,6 @@ The following labels are considered relevant for PSA enforcement: - pod-security.kubernetes.io/warn - pod-security.kubernetes.io/warn-version +### Namespace resource limit validation + +Validation ensures that the limits for cpu/memory must not be less than the requests for cpu/memory. diff --git a/pkg/resources/core/v1/namespace/requestlimits.go b/pkg/resources/core/v1/namespace/requestlimits.go new file mode 100644 index 000000000..822e0400c --- /dev/null +++ b/pkg/resources/core/v1/namespace/requestlimits.go @@ -0,0 +1,123 @@ +package namespace + +import ( + "encoding/json" + "fmt" + + "github.com/rancher/webhook/pkg/admission" + objectsv1 "github.com/rancher/webhook/pkg/generated/objects/core/v1" + admissionv1 "k8s.io/api/admission/v1" + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + "k8s.io/utils/trace" +) + +const resourceLimitAnnotation = "field.cattle.io/containerDefaultResourceLimit" + +type requestLimitAdmitter struct{} + +// Admit ensures that the resource requests are within the limits. +func (r *requestLimitAdmitter) Admit(request *admission.Request) (*admissionv1.AdmissionResponse, error) { + listTrace := trace.New("Namespace Admit", trace.Field{Key: "user", Value: request.UserInfo.Username}) + defer listTrace.LogIfLong(admission.SlowTraceDuration) + + switch request.Operation { + case admissionv1.Create: + ns, err := objectsv1.NamespaceFromRequest(&request.AdmissionRequest) + if err != nil { + return nil, fmt.Errorf("failed to decode namespace from request: %w", err) + } + return r.admitCommonCreateUpdate(nil, ns) + case admissionv1.Update: + oldns, ns, err := objectsv1.NamespaceOldAndNewFromRequest(&request.AdmissionRequest) + if err != nil { + return nil, fmt.Errorf("failed to decode namespace from request: %w", err) + } + return r.admitCommonCreateUpdate(oldns, ns) + } + return admission.ResponseAllowed(), nil +} + +type ResourceLimits struct { + LimitsCPU string `json:"limitsCpu"` + LimitsMemory string `json:"limitsMemory"` + RequestsCPU string `json:"requestsCpu"` + RequestsMemory string `json:"requestsMemory"` +} + +// admitCommonCreateUpdate will extract the annotation values that contain the resource limits and will call +// the validateResourceLimitsWithUnits function to determine whether or not the request is valid. +func (r *requestLimitAdmitter) admitCommonCreateUpdate(_, newNamespace *v1.Namespace) (*admissionv1.AdmissionResponse, error) { + annotations := newNamespace.Annotations + if annotations == nil { + return admission.ResponseAllowed(), nil + } + + resourceLimitJSON, exists := annotations[resourceLimitAnnotation] + if !exists || resourceLimitJSON == "{}" { + return admission.ResponseAllowed(), nil + } + + var resourceLimits ResourceLimits + if err := json.Unmarshal([]byte(resourceLimitJSON), &resourceLimits); err != nil { + return admission.ResponseBadRequest(fmt.Sprintf("invalid resource limits annotation: %v", err)), nil + } + + if err := validateResourceLimitsWithUnits(resourceLimits); err != nil { + return admission.ResponseBadRequest(err.Error()), nil + } + + return admission.ResponseAllowed(), nil +} + +// validateResourceLimitsWithUnits takes a set of CPU/memory requests/limits and validates them. +// It parses all provided values. If both a request and a limit exist for CPU or memory, it ensures +// that the request is not greater than the limit. Missing values are parsed but ignored in comparison. +func validateResourceLimitsWithUnits(limits ResourceLimits) error { + var requestsCPU, limitsCPU resource.Quantity + var err error + if limits.RequestsCPU != "" { + requestsCPU, err = resource.ParseQuantity(limits.RequestsCPU) + if err != nil { + return fmt.Errorf("invalid requestsCpu value: %v", err) + } + } + + if limits.LimitsCPU != "" { + limitsCPU, err = resource.ParseQuantity(limits.LimitsCPU) + if err != nil { + return fmt.Errorf("invalid limitsCpu value: %v", err) + } + } + + // Compare CPU requests and limits if both are provided + if limits.RequestsCPU != "" && limits.LimitsCPU != "" { + if requestsCPU.Cmp(limitsCPU) > 0 { + return fmt.Errorf("requestsCpu (%s) cannot be greater than limitsCpu (%s)", requestsCPU.String(), limitsCPU.String()) + } + } + + var requestsMemory, limitsMemory resource.Quantity + if limits.RequestsMemory != "" { + requestsMemory, err = resource.ParseQuantity(limits.RequestsMemory) + if err != nil { + return fmt.Errorf("invalid requestsMemory value: %v", err) + } + } + + if limits.LimitsMemory != "" { + limitsMemory, err = resource.ParseQuantity(limits.LimitsMemory) + if err != nil { + return fmt.Errorf("invalid limitsMemory value: %v", err) + } + } + + // Compare memory requests and limits if both are provided + if limits.RequestsMemory != "" && limits.LimitsMemory != "" { + if requestsMemory.Cmp(limitsMemory) > 0 { + return fmt.Errorf("requestsMemory (%s) cannot be greater than limitsMemory (%s)", requestsMemory.String(), limitsMemory.String()) + } + } + + return nil +} diff --git a/pkg/resources/core/v1/namespace/requestlimits_test.go b/pkg/resources/core/v1/namespace/requestlimits_test.go new file mode 100644 index 000000000..abbc887f3 --- /dev/null +++ b/pkg/resources/core/v1/namespace/requestlimits_test.go @@ -0,0 +1,161 @@ +package namespace + +import ( + "context" + "encoding/json" + "testing" + + "github.com/rancher/webhook/pkg/admission" + "github.com/stretchr/testify/assert" + v1 "k8s.io/api/admission/v1" + authenticationv1 "k8s.io/api/authentication/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +const testNs = "test-ns" + +func TestRequestLimitAdmitter(t *testing.T) { + tests := []struct { + name string + operationType v1.Operation + limitsAnnotation string + wantAllowed bool + }{ + { + name: "create ns within resource limits", + operationType: v1.Create, + limitsAnnotation: `{"limitsCpu": "500m", "requestsCpu": "100m", "limitsMemory": "128Mi", "requestsMemory": "64Mi"}`, + wantAllowed: true, + }, + { + name: "create ns exceeds resource limits", + operationType: v1.Create, + limitsAnnotation: `{"limitsCpu": "200m", "limitsMemory": "256Mi", "requestsCpu": "1", "requestsMemory": "1Gi"}`, + wantAllowed: false, + }, + { + name: "create ns invalid JSON in annotation", + operationType: v1.Create, + limitsAnnotation: `invalid-json`, + wantAllowed: false, + }, + { + name: "create ns no request annotation", + operationType: v1.Create, + limitsAnnotation: "", + wantAllowed: true, + }, + { + name: "update ns within resource limits", + operationType: v1.Update, + limitsAnnotation: `{"limitsCpu": "500m", "requestsCpu": "100m", "limitsMemory": "128Mi", "requestsMemory": "64Mi"}`, + wantAllowed: true, + }, + { + name: "update ns exceeds resource limits", + operationType: v1.Update, + limitsAnnotation: `{"limitsCpu": "200m", "limitsMemory": "256Mi", "requestsCpu": "1", "requestsMemory": "1Gi"}`, + wantAllowed: false, + }, + { + name: "create ns within only cpu req and limit", + operationType: v1.Create, + limitsAnnotation: `{"limitsCpu": "500m", "requestsCpu": "100m"}`, + wantAllowed: true, + }, + { + name: "create ns within invalid cpu req and limit", + operationType: v1.Create, + limitsAnnotation: `{"limitsCpu": "100m", "requestsCpu": "500m"}`, + wantAllowed: false, + }, + { + name: "update ns within only memory resource req and limit", + operationType: v1.Update, + limitsAnnotation: `{"limitsCpu": "500m", "requestsCpu": "100m"}`, + wantAllowed: true, + }, + { + name: "create ns within only memory req and cpu limit", + operationType: v1.Create, + limitsAnnotation: `{"limitsMemory": "256Mi", "limitssCpu": "100m"}`, + wantAllowed: true, + }, + { + name: "create ns within empty resource limits", + operationType: v1.Create, + limitsAnnotation: `{}`, + wantAllowed: true, + }, + { + name: "create ns within invalid cpu limits", + operationType: v1.Create, + limitsAnnotation: `{"limitsCpu": "25invalid"}`, + wantAllowed: false, + }, + { + name: "update ns within invalid memory limits", + operationType: v1.Update, + limitsAnnotation: `{"limitsMemory": "77foobar"}`, + wantAllowed: false, + }, + } + + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + admitter := requestLimitAdmitter{} + request, err := createRequestLimitRequest(test.limitsAnnotation, test.operationType) + if test.operationType == v1.Update { + request.AdmissionRequest.OldObject.Raw, err = json.Marshal(corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: testNs, + }, + }) + } + assert.NoError(t, err) + response, err := admitter.Admit(request) + assert.NoError(t, err) + assert.Equal(t, test.wantAllowed, response.Allowed) + }) + } +} + +func createRequestLimitRequest(limitsAnnotation string, operation v1.Operation) (*admission.Request, error) { + gvk := metav1.GroupVersionKind{Version: "v1", Kind: "Namespace"} + gvr := metav1.GroupVersionResource{Version: "v1", Resource: "namespace"} + + ns := corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: testNs, + }, + } + if limitsAnnotation != "" { + ns.Annotations = map[string]string{ + resourceLimitAnnotation: limitsAnnotation, + } + } + + req := &admission.Request{ + AdmissionRequest: v1.AdmissionRequest{ + UID: "", + Kind: gvk, + Resource: gvr, + RequestKind: &gvk, + RequestResource: &gvr, + Name: ns.Name, + Operation: operation, + UserInfo: authenticationv1.UserInfo{Username: "test-user", UID: ""}, + }, + Context: context.Background(), + } + + var err error + req.Object.Raw, err = json.Marshal(ns) + if err != nil { + return nil, err + } + return req, nil +} diff --git a/pkg/resources/core/v1/namespace/validator.go b/pkg/resources/core/v1/namespace/validator.go index d4d563d24..e4655f892 100644 --- a/pkg/resources/core/v1/namespace/validator.go +++ b/pkg/resources/core/v1/namespace/validator.go @@ -18,8 +18,9 @@ var projectsGVR = schema.GroupVersionResource{ // Validator validates the namespace admission request. type Validator struct { - psaAdmitter psaLabelAdmitter - projectNamespaceAdmitter projectNamespaceAdmitter + psaAdmitter psaLabelAdmitter + projectNamespaceAdmitter projectNamespaceAdmitter + requestWithinLimitAdmitter requestLimitAdmitter } // NewValidator returns a new validator used for validation of namespace requests. @@ -31,6 +32,7 @@ func NewValidator(sar authorizationv1.SubjectAccessReviewInterface) *Validator { projectNamespaceAdmitter: projectNamespaceAdmitter{ sar: sar, }, + requestWithinLimitAdmitter: requestLimitAdmitter{}, } } @@ -90,5 +92,5 @@ func (v *Validator) ValidatingWebhook(clientConfig admissionv1.WebhookClientConf // Admitters returns the psaAdmitter and the projectNamespaceAdmitter for namespaces. func (v *Validator) Admitters() []admission.Admitter { - return []admission.Admitter{&v.psaAdmitter, &v.projectNamespaceAdmitter} + return []admission.Admitter{&v.psaAdmitter, &v.projectNamespaceAdmitter, &v.requestWithinLimitAdmitter} } diff --git a/pkg/resources/core/v1/namespace/validator_test.go b/pkg/resources/core/v1/namespace/validator_test.go index 62e30cb25..0dcfba11d 100644 --- a/pkg/resources/core/v1/namespace/validator_test.go +++ b/pkg/resources/core/v1/namespace/validator_test.go @@ -28,7 +28,7 @@ func TestOperations(t *testing.T) { func TestAdmitters(t *testing.T) { validator := NewValidator(nil) admitters := validator.Admitters() - assert.Len(t, admitters, 2) + assert.Len(t, admitters, 3) hasPSAAdmitter := false hasProjectNamespaceAdmitter := false for i := range admitters {