Migrate Admission Controller Validation to CEL

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,13 +371,20 @@ 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:
                           - Auto
                           - "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,6 +461,7 @@ 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:
                     description: |-

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,15 +986,14 @@ var _ = AdmissionControllerE2eDescribe("Admission-controller", func() {
 					"name":"hamster"
 				},
 		   	"resourcePolicy": {
-		  		"containerPolicies": [{"containerName": "*", "minAllowed":{"requests":{"cpu":"50m"}}}]
+		  		"containerPolicies": [{"containerName": "cert-vpa-invalid", "minAllowed":{"requests":{"cpu":"50m"}}}]
 		  	}
 		  }
 		}`)
 		err = InstallRawVPA(f, invalidVPA)
 		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 {

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
 }
 

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{