diff --git a/vertical-pod-autoscaler/deploy/vpa-v1-crd-gen.yaml b/vertical-pod-autoscaler/deploy/vpa-v1-crd-gen.yaml index 6499413aa751..07e4219cc5c0 100644 --- a/vertical-pod-autoscaler/deploy/vpa-v1-crd-gen.yaml +++ b/vertical-pod-autoscaler/deploy/vpa-v1-crd-gen.yaml @@ -301,6 +301,7 @@ spec: required: - name type: object + maxItems: 1 type: array resourcePolicy: description: |- @@ -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 @@ -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: @@ -373,6 +379,12 @@ spec: - "Off" type: string type: object + x-kubernetes-validations: + - message: ControlledValues shouldn't be specified if container + scaling mode is off + rule: '!has(self.mode) || !has(self.controlledValues) || self.mode + != ''Off'' || self.controlledValues != ''RequestsAndLimits''' + maxItems: 100 type: array type: object targetRef: @@ -449,8 +461,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'. diff --git a/vertical-pod-autoscaler/e2e/v1/admission_controller.go b/vertical-pod-autoscaler/e2e/v1/admission_controller.go index d4e79c778283..ab0f2e0912d5 100644 --- a/vertical-pod-autoscaler/e2e/v1/admission_controller.go +++ b/vertical-pod-autoscaler/e2e/v1/admission_controller.go @@ -824,33 +824,121 @@ 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" + } + } + }`), + } + + 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 certificate", func(ctx ginkgo.SpecContext) { @@ -898,7 +986,7 @@ var _ = AdmissionControllerE2eDescribe("Admission-controller", func() { "name":"hamster" }, "resourcePolicy": { - "containerPolicies": [{"containerName": "*", "minAllowed":{"requests":{"cpu":"50m"}}}] + "containerPolicies": [{"containerName": "cert-vpa-invalid", "minAllowed":{"requests":{"cpu":"50m"}}}] } } }`) @@ -906,7 +994,6 @@ var _ = AdmissionControllerE2eDescribe("Admission-controller", func() { gomega.Expect(err).To(gomega.HaveOccurred(), "Invalid VPA object accepted") gomega.Expect(err.Error()).To(gomega.MatchRegexp(`.*admission webhook .*vpa.* denied the request: .*`), "Admission controller did not inspect the object") }) - }) func startDeploymentPods(f *framework.Framework, deployment *appsv1.Deployment) *apiv1.PodList { diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go index f2f2a29c4d34..061cc21ee51e 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go @@ -112,25 +112,8 @@ func parseVPA(raw []byte) (*vpa_types.VerticalPodAutoscaler, error) { // ValidateVPA checks the correctness of VPA Spec and returns an error if there is a problem. func ValidateVPA(vpa *vpa_types.VerticalPodAutoscaler, isCreate bool) error { - if vpa.Spec.UpdatePolicy != nil { - mode := vpa.Spec.UpdatePolicy.UpdateMode - if mode == nil { - return fmt.Errorf("UpdateMode is required if UpdatePolicy is used") - } - if _, found := possibleUpdateModes[*mode]; !found { - return fmt.Errorf("unexpected UpdateMode value %s", *mode) - } - - 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 { @@ -152,23 +135,8 @@ func ValidateVPA(vpa *vpa_types.VerticalPodAutoscaler, isCreate bool) error { return fmt.Errorf("MaxAllowed: %v", err) } } - ControlledValues := policy.ControlledValues - 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.") - } - } } } - - 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 } diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler_test.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler_test.go index a5d496de6fe4..02f5087d87af 100644 --- a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler_test.go +++ b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler_test.go @@ -32,74 +32,17 @@ const ( ) func TestValidateVPA(t *testing.T) { - badUpdateMode := vpa_types.UpdateMode("bad") validUpdateMode := vpa_types.UpdateModeOff - badMinReplicas := int32(0) validMinReplicas := int32(1) badScalingMode := vpa_types.ContainerScalingMode("bad") badCPUResource := resource.MustParse("187500u") validScalingMode := vpa_types.ContainerScalingModeAuto - scalingModeOff := vpa_types.ContainerScalingModeOff - controlledValuesRequestsAndLimits := vpa_types.ContainerControlledValuesRequestsAndLimits tests := []struct { name string vpa vpa_types.VerticalPodAutoscaler isCreate bool expectError error }{ - { - name: "empty update", - vpa: vpa_types.VerticalPodAutoscaler{}, - }, - { - name: "empty create", - vpa: vpa_types.VerticalPodAutoscaler{}, - isCreate: true, - expectError: fmt.Errorf("TargetRef is required. If you're using v1beta1 version of the API, please migrate to v1"), - }, - { - name: "no update mode", - vpa: vpa_types.VerticalPodAutoscaler{ - Spec: vpa_types.VerticalPodAutoscalerSpec{ - UpdatePolicy: &vpa_types.PodUpdatePolicy{}, - }, - }, - expectError: fmt.Errorf("UpdateMode is required if UpdatePolicy is used"), - }, - { - name: "bad update mode", - vpa: vpa_types.VerticalPodAutoscaler{ - Spec: vpa_types.VerticalPodAutoscalerSpec{ - UpdatePolicy: &vpa_types.PodUpdatePolicy{ - UpdateMode: &badUpdateMode, - }, - }, - }, - expectError: fmt.Errorf("unexpected UpdateMode value bad"), - }, - { - name: "zero minReplicas", - vpa: vpa_types.VerticalPodAutoscaler{ - Spec: vpa_types.VerticalPodAutoscalerSpec{ - UpdatePolicy: &vpa_types.PodUpdatePolicy{ - MinReplicas: &badMinReplicas, - UpdateMode: &validUpdateMode, - }, - }, - }, - expectError: fmt.Errorf("MinReplicas has to be positive, got 0"), - }, - { - name: "no policy name", - vpa: vpa_types.VerticalPodAutoscaler{ - Spec: vpa_types.VerticalPodAutoscalerSpec{ - ResourcePolicy: &vpa_types.PodResourcePolicy{ - ContainerPolicies: []vpa_types.ContainerResourcePolicy{{}}, - }, - }, - }, - expectError: fmt.Errorf("ContainerPolicies.ContainerName is required"), - }, { name: "invalid scaling mode", vpa: vpa_types.VerticalPodAutoscaler{ @@ -116,21 +59,6 @@ func TestValidateVPA(t *testing.T) { }, expectError: fmt.Errorf("unexpected Mode value bad"), }, - { - name: "more than one recommender", - vpa: vpa_types.VerticalPodAutoscaler{ - Spec: vpa_types.VerticalPodAutoscalerSpec{ - UpdatePolicy: &vpa_types.PodUpdatePolicy{ - UpdateMode: &validUpdateMode, - }, - Recommenders: []*vpa_types.VerticalPodAutoscalerRecommenderSelector{ - {Name: "test1"}, - {Name: "test2"}, - }, - }, - }, - expectError: fmt.Errorf("The current version of VPA object shouldn't specify more than one recommenders."), - }, { name: "bad limits", vpa: vpa_types.VerticalPodAutoscaler{ @@ -236,23 +164,6 @@ func TestValidateVPA(t *testing.T) { }, expectError: fmt.Errorf("MaxAllowed: Memory [%v] must be a whole number of bytes", resource.MustParse("500m")), }, - { - name: "scaling off with controlled values requests and limits", - vpa: vpa_types.VerticalPodAutoscaler{ - Spec: vpa_types.VerticalPodAutoscalerSpec{ - ResourcePolicy: &vpa_types.PodResourcePolicy{ - ContainerPolicies: []vpa_types.ContainerResourcePolicy{ - { - ContainerName: "loot box", - Mode: &scalingModeOff, - ControlledValues: &controlledValuesRequestsAndLimits, - }, - }, - }, - }, - }, - expectError: fmt.Errorf("ControlledValues shouldn't be specified if container scaling mode is off."), - }, { name: "all valid", vpa: vpa_types.VerticalPodAutoscaler{ diff --git a/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go b/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go index 5ede0fe4810a..2627e5e32c18 100644 --- a/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go +++ b/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go @@ -106,6 +106,7 @@ type VerticalPodAutoscalerSpec struct { // Recommender responsible for generating recommendation for this object. // List should be empty (then the default recommender will generate the // recommendation) or contain exactly one recommender. + // +kubebuilder:validation:MaxItems=1 // +optional Recommenders []*VerticalPodAutoscalerRecommenderSelector `json:"recommenders,omitempty" protobuf:"bytes,4,opt,name=recommenders"` } @@ -136,12 +137,14 @@ type PodUpdatePolicy struct { // Controls when autoscaler applies changes to the pod resources. // The default is 'Auto'. // +optional + // +kubebuilder:default="Auto" UpdateMode *UpdateMode `json:"updateMode,omitempty" protobuf:"bytes,1,opt,name=updateMode"` // Minimal number of replicas which need to be alive for Updater to attempt // pod eviction (pending other checks like PDB). Only positive values are // allowed. Overrides global '--min-replicas' flag. // +optional + // +kubebuilder:validation:Minimum=1 MinReplicas *int32 `json:"minReplicas,omitempty" protobuf:"varint,2,opt,name=minReplicas"` // EvictionRequirements is a list of EvictionRequirements that need to @@ -183,18 +186,23 @@ type PodResourcePolicy struct { // +optional // +patchMergeKey=containerName // +patchStrategy=merge + // +kubebuilder:validation:MaxItems=100 ContainerPolicies []ContainerResourcePolicy `json:"containerPolicies,omitempty" patchStrategy:"merge" patchMergeKey:"containerName" protobuf:"bytes,1,rep,name=containerPolicies"` } // ContainerResourcePolicy controls how autoscaler computes the recommended // resources for a specific container. +// +kubebuilder:validation:XValidation:rule="!has(self.mode) || !has(self.controlledValues) || self.mode != 'Off' || self.controlledValues != 'RequestsAndLimits'",message="ControlledValues shouldn't be specified if container scaling mode is off" type ContainerResourcePolicy struct { // Name of the container or DefaultContainerResourcePolicy, in which // case the policy is used by the containers that don't have their own // policy specified. + // +kubebuilder:validation:Pattern=`(^[a-zA-Z0-9-_]+$)|(^\*$)` + // +kubebuilder:validation:XValidation:rule="size(self) > 0",message="ContainerName cannot be empty" ContainerName string `json:"containerName,omitempty" protobuf:"bytes,1,opt,name=containerName"` // Whether autoscaler is enabled for the container. The default is "Auto". // +optional + // +kubebuilder:default="Auto" Mode *ContainerScalingMode `json:"mode,omitempty" protobuf:"bytes,2,opt,name=mode"` // Specifies the minimal amount of resources that will be recommended // for the container. The default is no minimum.