Add resource resolution check for all maxAllowed

kubernetes · Apr 22, 2022 · 9a80be0 · 9a80be0
1 parent 48b4e45
commit 9a80be0
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 15 deletions.
diff --git a/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go b/vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go
@@ -136,19 +136,22 @@ func validateVPA(vpa *vpa_types.VerticalPodAutoscaler, isCreate bool) error {
 				}
 			}
 			for resource, min := range policy.MinAllowed {
-				if err := validate(resource, min); err != nil {
-					return err
+				if err := validateResourceResolution(resource, min); err != nil {
+					return fmt.Errorf("MinAllowed: %v", err)
 				}
 				max, found := policy.MaxAllowed[resource]
 				if found {
-					if err := validate(resource, max); err != nil {
-						return err
-					}
 					if max.Cmp(min) < 0 {
 						return fmt.Errorf("max resource for %v is lower than min", resource)
 					}
 				}
 			}
+
+			for resource, max := range policy.MaxAllowed {
+				if err := validateResourceResolution(resource, max); err != nil {
+					return fmt.Errorf("MaxAllowed: %v", err)
+				}
+			}
 			ControlledValues := policy.ControlledValues
 			if mode != nil && ControlledValues != nil {
 				if *mode == vpa_types.ContainerScalingModeOff && *ControlledValues == vpa_types.ContainerControlledValuesRequestsAndLimits {
@@ -169,7 +172,7 @@ func validateVPA(vpa *vpa_types.VerticalPodAutoscaler, isCreate bool) error {
 	return nil
 }
 
-func validate(name corev1.ResourceName, val apires.Quantity) error {
+func validateResourceResolution(name corev1.ResourceName, val apires.Quantity) error {
 	switch name {
 	case corev1.ResourceCPU:
 		return validateCPUResolution(val)

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
@@ -37,7 +37,7 @@ func TestValidateVPA(t *testing.T) {
 	badMinReplicas := int32(0)
 	validMinReplicas := int32(1)
 	badScalingMode := vpa_types.ContainerScalingMode("bad")
-	badCPUValue := resource.MustParse("187500u")
+	badCPUResource := resource.MustParse("187500u")
 	validScalingMode := vpa_types.ContainerScalingModeAuto
 	scalingModeOff := vpa_types.ContainerScalingModeOff
 	controlledValuesRequestsAndLimits := vpa_types.ContainerControlledValuesRequestsAndLimits
@@ -161,7 +161,7 @@ func TestValidateVPA(t *testing.T) {
 							{
 								ContainerName: "loot box",
 								MinAllowed: apiv1.ResourceList{
-									cpu: badCPUValue,
+									cpu: resource.MustParse("187500u"),
 								},
 								MaxAllowed: apiv1.ResourceList{
 									cpu: resource.MustParse("275m"),
@@ -171,7 +171,7 @@ func TestValidateVPA(t *testing.T) {
 					},
 				},
 			},
-			expectError: fmt.Errorf("CPU [%v] must be a whole number of milli CPUs", badCPUValue.String()),
+			expectError: fmt.Errorf("MinAllowed: CPU [%v] must be a whole number of milli CPUs", badCPUResource.String()),
 		},
 		{
 			name: "bad minAllowed memory value",
@@ -194,28 +194,47 @@ func TestValidateVPA(t *testing.T) {
 					},
 				},
 			},
-			expectError: fmt.Errorf("Memory [%v] must be a whole number of bytes", resource.MustParse("100m")),
+			expectError: fmt.Errorf("MinAllowed: Memory [%v] must be a whole number of bytes", resource.MustParse("100m")),
 		},
 		{
-			name: "bad manAllowed cpu value",
+			name: "bad maxAllowed cpu value",
 			vpa: vpa_types.VerticalPodAutoscaler{
 				Spec: vpa_types.VerticalPodAutoscalerSpec{
 					ResourcePolicy: &vpa_types.PodResourcePolicy{
 						ContainerPolicies: []vpa_types.ContainerResourcePolicy{
 							{
 								ContainerName: "loot box",
-								MinAllowed: apiv1.ResourceList{
-									cpu: resource.MustParse("1m"),
+								MinAllowed:    apiv1.ResourceList{},
+								MaxAllowed: apiv1.ResourceList{
+									cpu: resource.MustParse("187500u"),
 								},
+							},
+						},
+					},
+				},
+			},
+			expectError: fmt.Errorf("MaxAllowed: CPU [%s] must be a whole number of milli CPUs", badCPUResource.String()),
+		},
+		{
+			name: "bad maxAllowed memory value",
+			vpa: vpa_types.VerticalPodAutoscaler{
+				Spec: vpa_types.VerticalPodAutoscalerSpec{
+					ResourcePolicy: &vpa_types.PodResourcePolicy{
+						ContainerPolicies: []vpa_types.ContainerResourcePolicy{
+							{
+								ContainerName: "loot box",
+								MinAllowed: apiv1.ResourceList{
+									cpu: resource.MustParse("1m")},
 								MaxAllowed: apiv1.ResourceList{
-									cpu: badCPUValue,
+									cpu:    resource.MustParse("275m"),
+									memory: resource.MustParse("500m"),
 								},
 							},
 						},
 					},
 				},
 			},
-			expectError: fmt.Errorf("CPU [%s] must be a whole number of milli CPUs", badCPUValue.String()),
+			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",