Skip to content

Migrate Admission Controller Validation to CEL #7690

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.

Sign up for GitHub

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
+147 −153
Open

Migrate Admission Controller Validation to CEL #7690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c03f11d
Migrate Admission Controller Validation to CEL
omerap12 Jan 14, 2025
81ff1c7
Add e2e tests and remove unit tests
omerap12 Jan 15, 2025
e6f487c
Add default for UpdateMode
omerap12 Jan 15, 2025
a8a6e96
Fixed * in containerPolicy.ContainerName
omerap12 Jan 16, 2025
77e0377
remove check & maxItems
omerap12 Apr 4, 2025
e75d2bb
revert unit test
omerap12 Apr 4, 2025
24c8a8e
lint
omerap12 Apr 4, 2025
549ce7b
merged master
omerap12 May 13, 2025
40b2576
removed unused struct
omerap12 May 13, 2025
2ce86ab
moved test to e2e
omerap12 May 13, 2025
d503ba2
fixed lint
omerap12 May 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions vertical-pod-autoscaler/deploy/vpa-v1-crd-gen.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ kind: CustomResourceDefinition
metadata:
annotations:
api-approved.kubernetes.io: https://github.com/kubernetes/kubernetes/pull/63797
controller-gen.kubebuilder.io/version: v0.16.5
controller-gen.kubebuilder.io/version: v0.17.2
name: verticalpodautoscalercheckpoints.autoscaling.k8s.io
spec:
group: autoscaling.k8s.io
Expand Down Expand Up @@ -225,7 +225,7 @@ kind: CustomResourceDefinition
metadata:
annotations:
api-approved.kubernetes.io: https://github.com/kubernetes/kubernetes/pull/63797
controller-gen.kubebuilder.io/version: v0.16.5
controller-gen.kubebuilder.io/version: v0.17.2
name: verticalpodautoscalers.autoscaling.k8s.io
spec:
group: autoscaling.k8s.io
Expand Down Expand Up @@ -301,6 +301,7 @@ spec:
required:
- name
type: object
maxItems: 1
type: array
resourcePolicy:
description: |-
Expand All @@ -324,7 +325,11 @@ spec:
Name of the container or DefaultContainerResourcePolicy, in which
case the policy is used by the containers that don't have their own
policy specified.
pattern: (^[a-zA-Z0-9-_]+$)|(^\*$)
type: string
x-kubernetes-validations:
- message: ContainerName cannot be empty
rule: size(self) > 0
controlledResources:
description: |-
Specifies the type of recommendations that will be computed
Expand Down Expand Up @@ -366,6 +371,7 @@ spec:
for the container. The default is no minimum.
type: object
mode:
default: Auto
description: Whether autoscaler is enabled for the container.
The default is "Auto".
enum:
Expand Down Expand Up @@ -449,8 +455,10 @@ spec:
pod eviction (pending other checks like PDB). Only positive values are
allowed. Overrides global '--min-replicas' flag.
format: int32
minimum: 1
type: integer
updateMode:
default: Auto
description: |-
Controls when autoscaler applies changes to the pod resources.
The default is 'Auto'.
Expand Down
150 changes: 128 additions & 22 deletions vertical-pod-autoscaler/e2e/v1/admission_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -876,33 +876,139 @@ var _ = AdmissionControllerE2eDescribe("Admission-controller", func() {
"name":"hamster"
},
"resourcePolicy": {
"containerPolicies": [{"containerName": "*", "minAllowed":{"cpu":"50m"}}]
"containerPolicies": [{"containerName": "hamster-vpa-valid", "minAllowed":{"cpu":"50m"}}]
}
}
}`)
err := InstallRawVPA(f, validVPA)
gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Valid VPA object rejected")

ginkgo.By("Setting up invalid VPA object")
// The invalid object differs by name and minAllowed - there is an invalid "requests" field.
invalidVPA := []byte(`{
"kind": "VerticalPodAutoscaler",
"apiVersion": "autoscaling.k8s.io/v1",
"metadata": {"name": "hamster-vpa-invalid"},
"spec": {
"targetRef": {
"apiVersion": "apps/v1",
"kind": "Deployment",
"name":"hamster"
},
"resourcePolicy": {
"containerPolicies": [{"containerName": "*", "minAllowed":{"requests":{"cpu":"50m"}}}]
}
}
}`)
err2 := InstallRawVPA(f, invalidVPA)
gomega.Expect(err2).To(gomega.HaveOccurred(), "Invalid VPA object accepted")
gomega.Expect(err2.Error()).To(gomega.MatchRegexp(`.*admission webhook .*vpa.* denied the request: .*`))
invalidVPAs := map[string][]byte{
`spec\.resourcePolicy\.containerPolicies\[0\]\.containerName: Invalid value: "\*": spec\.resourcePolicy\.containerPolicies\[0\]\.containerName in body should match '^[a-zA-Z0-9-_]+$'`: []byte(`{
"kind": "VerticalPodAutoscaler",
"apiVersion": "autoscaling.k8s.io/v1",
"metadata": {"name": "basic-vpa"},
"spec": {
"targetRef": {
"apiVersion": "apps/v1",
"kind": "Deployment",
"name": "example-deployment"
},
"resourcePolicy": {
"containerPolicies": [{
"containerName": "*",
"mode": "Auto"
}]
},
"updatePolicy": {
"updateMode": "Auto"
}
}
}`),
`spec.resourcePolicy.containerPolicies[0].containerName: Invalid value: "string": ContainerName cannot be empty`: []byte(`{
"kind": "VerticalPodAutoscaler",
"apiVersion": "autoscaling.k8s.io/v1",
"metadata": {"name": "example-vpa-containername-empty"},
"spec": {
"targetRef": {
"apiVersion": "apps/v1",
"kind": "Deployment",
"name": "example-deployment"
},
"resourcePolicy": {
"containerPolicies": [{
"containerName": ""
"mode": "Auto",
"controlledResources": ["RequestsAndLimits"]
}]
},
"updatePolicy": {
"updateMode": "Recreate",
"minReplicas": 2
}
}
}`),
`spec.updatePolicy.minReplicas: Invalid value: -1: spec.updatePolicy.minReplicas in body should be greater than or equal to 1`: []byte(`{
"kind": "VerticalPodAutoscaler",
"apiVersion": "autoscaling.k8s.io/v1",
"metadata": {"name": "vpa-minreplicas-negative"},
"spec": {
"targetRef": {
"apiVersion": "apps/v1",
"kind": "Deployment",
"name": "nginx"
},
"resourcePolicy": {
"containerPolicies": [{
"containerName": "nginx",
"mode": "Auto",
"minAllowed": {
"memory": "200Mi"
},
"maxAllowed": {
"memory": "500Mi"
}
}]
},
"updatePolicy": {
"updateMode": "Auto",
"minReplicas": -1
}
}
}`),
`spec.updatePolicy.updateMode: Unsupported value: "InvalidMode": supported values: "Off", "Initial", "Recreate", "Auto"`: []byte(`{
"kind": "VerticalPodAutoscaler",
"apiVersion": "autoscaling.k8s.io/v1",
"metadata": {"name": "vpa-updatemode-invalid"},
"spec": {
"targetRef": {
"apiVersion": "apps/v1",
"kind": "Deployment",
"name": "nginx"
},
"resourcePolicy": {
"containerPolicies": [{
"containerName": "nginx",
"mode": "Auto",
"minAllowed": {
"memory": "200Mi"
},
"maxAllowed": {
"memory": "500Mi"
}
}]
},
"updatePolicy": {
"updateMode": "InvalidMode"
}
}
}`),
`spec.resourcePolicy.containerPolicies[0].mode: Unsupported value: "bad": supported values: "Auto", "Off"`: []byte(`{
"kind": "VerticalPodAutoscaler",
"apiVersion": "autoscaling.k8s.io/v1",
"metadata": {"name": "hamster-vpa-invalid-mode"},
"spec": {
"targetRef": {
"apiVersion": "apps/v1",
"kind": "Deployment",
"name": "hamster"
},
"resourcePolicy": {
"containerPolicies": [{
"containerName": "loot box",
"mode": "bad"
}]
},
}
}`),
}

var err2 error
for regexExp, invalidVPA := range invalidVPAs {
err2 = InstallRawVPA(f, invalidVPA)
gomega.Expect(err2).To(gomega.HaveOccurred(), "Invalid VPA object accepted")
gomega.Expect(err2.Error()).To(gomega.MatchRegexp(regexExp))
}
})

ginkgo.It("reloads the webhook leaf and CA certificate", func(ctx ginkgo.SpecContext) {
Expand Down Expand Up @@ -951,7 +1057,7 @@ var _ = AdmissionControllerE2eDescribe("Admission-controller", func() {
"name":"hamster"
},
"resourcePolicy": {
"containerPolicies": [{"containerName": "*", "minAllowed":{"requests":{"cpu":"50m"}}}]
"containerPolicies": [{"containerName": "cert-vpa-invalid", "minAllowed":{"requests":{"cpu":"50m"}}}]
}
}
}`)
Expand Down
43 changes: 2 additions & 41 deletions vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,6 @@ import (
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/metrics/admission"
)

var (
possibleUpdateModes = map[vpa_types.UpdateMode]interface{}{
vpa_types.UpdateModeOff: struct{}{},
vpa_types.UpdateModeInitial: struct{}{},
vpa_types.UpdateModeRecreate: struct{}{},
vpa_types.UpdateModeAuto: struct{}{},
vpa_types.UpdateModeInPlaceOrRecreate: struct{}{},
}

possibleScalingModes = map[vpa_types.ContainerScalingMode]interface{}{
vpa_types.ContainerScalingModeAuto: struct{}{},
vpa_types.ContainerScalingModeOff: struct{}{},
}
)

// resourceHandler builds patches for VPAs.
type resourceHandler struct {
preProcessor PreProcessor
Expand Down Expand Up @@ -120,29 +105,13 @@ func ValidateVPA(vpa *vpa_types.VerticalPodAutoscaler, isCreate bool) error {
if mode == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any checks added regarding updatePolicy.updateMode – is this intentional and those checks are implicitly done somewhere else now? Or do we need to add them as CEL validations as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the CRD itself I saw this:
https://github.com/kubernetes/autoscaler/blob/master/vertical-pod-autoscaler/deploy/vpa-v1-crd-gen.yaml#L456

So added in here: ea90c23

return fmt.Errorf("UpdateMode is required if UpdatePolicy is used")
}
if _, found := possibleUpdateModes[*mode]; !found {
return fmt.Errorf("unexpected UpdateMode value %s", *mode)
}
if (*mode == vpa_types.UpdateModeInPlaceOrRecreate) && !features.Enabled(features.InPlaceOrRecreate) && isCreate {
return fmt.Errorf("in order to use UpdateMode %s, you must enable feature gate %s in the admission-controller args", vpa_types.UpdateModeInPlaceOrRecreate, features.InPlaceOrRecreate)
}

if minReplicas := vpa.Spec.UpdatePolicy.MinReplicas; minReplicas != nil && *minReplicas <= 0 {
return fmt.Errorf("MinReplicas has to be positive, got %v", *minReplicas)
}
}

if vpa.Spec.ResourcePolicy != nil {
for _, policy := range vpa.Spec.ResourcePolicy.ContainerPolicies {
if policy.ContainerName == "" {
return fmt.Errorf("ContainerPolicies.ContainerName is required")
}
mode := policy.Mode
if mode != nil {
if _, found := possibleScalingModes[*mode]; !found {
return fmt.Errorf("unexpected Mode value %s", *mode)
}
}
for resource, min := range policy.MinAllowed {
if err := validateResourceResolution(resource, min); err != nil {
return fmt.Errorf("MinAllowed: %v", err)
Expand All @@ -159,22 +128,14 @@ func ValidateVPA(vpa *vpa_types.VerticalPodAutoscaler, isCreate bool) error {
}
}
ControlledValues := policy.ControlledValues
mode := policy.Mode
if mode != nil && ControlledValues != nil {
if *mode == vpa_types.ContainerScalingModeOff && *ControlledValues == vpa_types.ContainerControlledValuesRequestsAndLimits {
return fmt.Errorf("ControlledValues shouldn't be specified if container scaling mode is off.")
return fmt.Errorf("controlledValues shouldn't be specified if container scaling mode is off.")
}
}
}
}

if isCreate && vpa.Spec.TargetRef == nil {
return fmt.Errorf("TargetRef is required. If you're using v1beta1 version of the API, please migrate to v1")
}

if len(vpa.Spec.Recommenders) > 1 {
return fmt.Errorf("The current version of VPA object shouldn't specify more than one recommenders.")
}

return nil
}

Expand Down
Loading
Loading