Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add limits and requests to PolicyServer #708

Merged
merged 6 commits into from
Apr 9, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 22 additions & 0 deletions config/crd/bases/policies.kubewarden.io_policyservers.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1143,6 +1143,16 @@ spec:
items:
type: string
type: array
limits:
additionalProperties:
anyOf:
- type: integer
- type: string
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
x-kubernetes-int-or-string: true
description: Limits describes the maximum amount of compute resources
allowed.
type: object
maxUnavailable:
anyOf:
- type: integer
Expand All @@ -1161,6 +1171,18 @@ spec:
description: Replicas is the number of desired replicas.
format: int32
type: integer
requests:
additionalProperties:
anyOf:
- type: integer
- type: string
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
x-kubernetes-int-or-string: true
description: Requests describes the minimum amount of compute resources
required. If Request is omitted for, it defaults to Limits if that
is explicitly specified, otherwise to an implementation-defined
value
type: object
securityContexts:
description: Security configuration to be used in the Policy Server
workload. The field allows different configurations for the pod
Expand Down
10 changes: 10 additions & 0 deletions pkg/apis/policies/v1/policyserver_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,16 @@ type PolicyServerSpec struct {
// Affinity rules for the associated Policy Server pods.
// +optional
Affinity corev1.Affinity `json:"affinity,omitempty"`

// Limits describes the maximum amount of compute resources allowed.
// +optional
Limits corev1.ResourceList `json:"limits,omitempty"`

// Requests describes the minimum amount of compute resources required.
// If Request is omitted for, it defaults to Limits if that is explicitly specified,
// otherwise to an implementation-defined value
// +optional
Requests corev1.ResourceList `json:"requests,omitempty"`
}

type ReconciliationTransitionReason string
Expand Down
48 changes: 47 additions & 1 deletion pkg/apis/policies/v1/policyserver_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,13 @@ package v1

import (
"context"
"errors"
"fmt"

"github.com/kubewarden/kubewarden-controller/internal/pkg/constants"
"github.com/kubewarden/kubewarden-controller/internal/pkg/policyserver"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/apimachinery/pkg/runtime"
validationutils "k8s.io/apimachinery/pkg/util/validation"
ctrl "sigs.k8s.io/controller-runtime"
Expand Down Expand Up @@ -56,6 +59,16 @@ func (ps *PolicyServer) Default() {
if ps.ObjectMeta.DeletionTimestamp == nil {
controllerutil.AddFinalizer(ps, constants.KubewardenFinalizer)
}

// Default the requests to the limits if not set
for limitName, limitQuantity := range ps.Spec.Limits {
if _, found := ps.Spec.Requests[limitName]; !found {
if ps.Spec.Requests == nil {
ps.Spec.Requests = make(corev1.ResourceList)
}
ps.Spec.Requests[limitName] = limitQuantity
}
}
}

// +kubebuilder:webhook:path=/validate-policies-kubewarden-io-v1-policyserver,mutating=false,failurePolicy=fail,sideEffects=None,groups=policies.kubewarden.io,resources=policyservers,verbs=create;update,versions=v1,name=vpolicyserver.kb.io,admissionReviewVersions=v1
Expand Down Expand Up @@ -86,7 +99,12 @@ func (v *policyServerValidator) validate(ctx context.Context, obj runtime.Object

// Kubernetes does not allow to set both MinAvailable and MaxUnavailable at the same time
if policyServer.Spec.MinAvailable != nil && policyServer.Spec.MaxUnavailable != nil {
return fmt.Errorf("minAvailable and maxUnavailable cannot be both set")
return errors.New("minAvailable and maxUnavailable cannot be both set")
}

err := validateLimitsAndRequests(policyServer.Spec.Limits, policyServer.Spec.Requests)
if err != nil {
return err
}

return nil
Expand All @@ -103,3 +121,31 @@ func (v *policyServerValidator) ValidateUpdate(ctx context.Context, _, obj runti
func (v *policyServerValidator) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) {
return nil, nil
}

func validateLimitsAndRequests(limits, requests corev1.ResourceList) error {
if requests == nil || limits == nil {
return nil
}

for limitName, limitQuantity := range limits {
if limitQuantity.Cmp(resource.Quantity{}) < 0 {
return fmt.Errorf("%s limit must be greater than or equal to 0", limitName)
}
}

for requestName, requestQuantity := range requests {
if requestQuantity.Cmp(resource.Quantity{}) < 0 {
return fmt.Errorf("%s request must be greater than or equal to 0", requestName)
}
fabriziosestito marked this conversation as resolved.
Show resolved Hide resolved

limitQuantity, ok := limits[requestName]
if !ok {
continue
}

if requestQuantity.Cmp(limitQuantity) > 0 {
return fmt.Errorf("request must be less than or equal to %s limit", requestName)
}
}
return nil
}
78 changes: 78 additions & 0 deletions pkg/apis/policies/v1/policyserver_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,41 @@ package v1

import (
"context"
"errors"
"testing"

"github.com/kubewarden/kubewarden-controller/internal/pkg/constants"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
)

func TestDefault(t *testing.T) {
policyServer := &PolicyServer{
ObjectMeta: metav1.ObjectMeta{
Name: "policy-server",
},
Spec: PolicyServerSpec{
Limits: corev1.ResourceList{
"cpu": resource.MustParse("1Gi"),
fabriziosestito marked this conversation as resolved.
Show resolved Hide resolved
"memory": resource.MustParse("1Gi"),
},
},
}

policyServer.Default()

assert.Contains(t, policyServer.Finalizers, constants.KubewardenFinalizer)
assert.Equal(t, corev1.ResourceList{
"cpu": resource.MustParse("1Gi"),
"memory": resource.MustParse("1Gi"),
}, policyServer.Spec.Requests)
}

func TestValidatePolicyServerName(t *testing.T) {
name := make([]byte, 64)
for i := range name {
Expand Down Expand Up @@ -68,3 +95,54 @@ func TestValidateMinAvailable(t *testing.T) {
err := policyServerValidator.validate(context.Background(), policyServer)
require.ErrorContains(t, err, "minAvailable and maxUnavailable cannot be both set")
}

func TestValidateLimitsAndRequests(t *testing.T) {
testCases := []struct {
Name string
Limits corev1.ResourceList
Requests corev1.ResourceList
ExpectedErr error
}{
{
Name: "valid",
Limits: corev1.ResourceList{"cpu": resource.MustParse("1Gi")},
Requests: corev1.ResourceList{"cpu": resource.MustParse("500Mi")},
ExpectedErr: nil,
},
{
Name: "negative limit",
Limits: corev1.ResourceList{"cpu": resource.MustParse("-1Gi")},
Requests: corev1.ResourceList{"cpu": resource.MustParse("500Mi")},
ExpectedErr: errors.New("cpu limit must be greater than or equal to 0"),
},
{
Name: "negative request",
Limits: corev1.ResourceList{"cpu": resource.MustParse("1Gi")},
Requests: corev1.ResourceList{"cpu": resource.MustParse("-500Mi")},
ExpectedErr: errors.New("cpu request must be greater than or equal to 0"),
},
{
Name: "request greater than limit",
Limits: corev1.ResourceList{"cpu": resource.MustParse("1Gi")},
Requests: corev1.ResourceList{"cpu": resource.MustParse("2Gi")},
ExpectedErr: errors.New("request must be less than or equal to cpu limit"),
},
}

for _, test := range testCases {
t.Run(test.Name, func(t *testing.T) {
policyServer := &PolicyServer{
Spec: PolicyServerSpec{
Limits: test.Limits,
Requests: test.Requests,
},
}
policyServerValidator := policyServerValidator{
k8sClient: nil,
deploymentsNamespace: "default",
}
err := policyServerValidator.validate(context.Background(), policyServer)
assert.Equal(t, err, test.ExpectedErr)
})
}
}
14 changes: 14 additions & 0 deletions pkg/apis/policies/v1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.