Merge pull request #13688 from derekwaynecarr/fix_validate_limitrange

Auto commit by PR queue bot

pkg/api/resource/quantity.go

@@ -301,6 +301,27 @@ func (q *Quantity) String() string {
 	return number + string(suffix)
 }
 
+// Cmp compares q and y and returns:
+//
+//   -1 if q <  y
+//    0 if q == y
+//   +1 if q >  y
+//
+func (q *Quantity) Cmp(y Quantity) int {
+	num1 := q.Value()
+	num2 := y.Value()
+	if num1 < MaxMilliValue && num2 < MaxMilliValue {
+		num1 = q.MilliValue()
+		num2 = y.MilliValue()
+	}
+	if num1 < num2 {
+		return -1
+	} else if num1 > num2 {
+		return 1
+	}
+	return 0
+}
+
 func (q *Quantity) Add(y Quantity) error {
 	if q.Format != y.Format {
 		return fmt.Errorf("format mismatch: %v vs. %v", q.Format, y.Format)

pkg/api/resource/quantity_test.go

@@ -77,6 +77,26 @@ func TestQuantityCanocicalizeZero(t *testing.T) {
 	}
 }
 
+func TestQuantityCmp(t *testing.T) {
+	table := []struct {
+		x      string
+		y      string
+		expect int
+	}{
+		{"0", "0", 0},
+		{"100m", "50m", 1},
+		{"50m", "100m", -1},
+		{"10000T", "100Gi", 1},
+	}
+	for _, testCase := range table {
+		q1 := MustParse(testCase.x)
+		q2 := MustParse(testCase.y)
+		if result := q1.Cmp(q2); result != testCase.expect {
+			t.Errorf("X: %v, Y: %v, Expected: %v, Actual: %v", testCase.x, testCase.y, testCase.expect, result)
+		}
+	}
+}
+
 func TestQuantityParse(t *testing.T) {
 	table := []struct {
 		input  string

pkg/api/validation/validation.go

@@ -1442,79 +1442,75 @@ func ValidateLimitRange(limitRange *api.LimitRange) errs.ValidationErrorList {
 		limitTypeSet[limit.Type] = true
 
 		keys := util.StringSet{}
-		min := map[string]int64{}
+		min := map[string]resource.Quantity{}
-		max := map[string]int64{}
+		max := map[string]resource.Quantity{}
-		defaults := map[string]int64{}
+		defaults := map[string]resource.Quantity{}
-		defaultRequests := map[string]int64{}
+		defaultRequests := map[string]resource.Quantity{}
 
-		for k := range limit.Max {
+		for k, q := range limit.Max {
 			allErrs = append(allErrs, validateResourceName(string(k), fmt.Sprintf("spec.limits[%d].max[%s]", i, k))...)
 			keys.Insert(string(k))
-			q := limit.Max[k]
+			max[string(k)] = q
-			max[string(k)] = q.Value()
 		}
-		for k := range limit.Min {
+		for k, q := range limit.Min {
 			allErrs = append(allErrs, validateResourceName(string(k), fmt.Sprintf("spec.limits[%d].min[%s]", i, k))...)
 			keys.Insert(string(k))
-			q := limit.Min[k]
+			min[string(k)] = q
-			min[string(k)] = q.Value()
 		}
-		for k := range limit.Default {
+		for k, q := range limit.Default {
 			allErrs = append(allErrs, validateResourceName(string(k), fmt.Sprintf("spec.limits[%d].default[%s]", i, k))...)
 			keys.Insert(string(k))
-			q := limit.Default[k]
+			defaults[string(k)] = q
-			defaults[string(k)] = q.Value()
 		}
-		for k := range limit.DefaultRequest {
+		for k, q := range limit.DefaultRequest {
 			allErrs = append(allErrs, validateResourceName(string(k), fmt.Sprintf("spec.limits[%d].defaultRequest[%s]", i, k))...)
 			keys.Insert(string(k))
-			q := limit.DefaultRequest[k]
+			defaultRequests[string(k)] = q
-			defaultRequests[string(k)] = q.Value()
 		}
 		for k := range limit.MaxLimitRequestRatio {
 			allErrs = append(allErrs, validateResourceName(string(k), fmt.Sprintf("spec.limits[%d].maxLimitRequestRatio[%s]", i, k))...)
 		}
 
 		for k := range keys {
-			minValue, minValueFound := min[k]
+			minQuantity, minQuantityFound := min[k]
-			maxValue, maxValueFound := max[k]
+			maxQuantity, maxQuantityFound := max[k]
-			defaultValue, defaultValueFound := defaults[k]
+			defaultQuantity, defaultQuantityFound := defaults[k]
-			defaultRequestValue, defaultRequestValueFound := defaultRequests[k]
+			defaultRequestQuantity, defaultRequestQuantityFound := defaultRequests[k]
 
-			if minValueFound && maxValueFound && minValue > maxValue {
+			if minQuantityFound && maxQuantityFound && minQuantity.Cmp(maxQuantity) > 0 {
 				minQuantity := limit.Min[api.ResourceName(k)]
 				maxQuantity := limit.Max[api.ResourceName(k)]
-				allErrs = append(allErrs, errs.NewFieldInvalid(fmt.Sprintf("spec.limits[%d].min[%s]", i, k), minValue, fmt.Sprintf("min value %s is greater than max value %s", minQuantity.String(), maxQuantity.String())))
+				allErrs = append(allErrs, errs.NewFieldInvalid(fmt.Sprintf("spec.limits[%d].min[%s]", i, k), minQuantity, fmt.Sprintf("min value %s is greater than max value %s", minQuantity.String(), maxQuantity.String())))
 			}
 
-			if defaultRequestValueFound && minValueFound && minValue > defaultRequestValue {
+			if defaultRequestQuantityFound && minQuantityFound && minQuantity.Cmp(defaultRequestQuantity) > 0 {
 				minQuantity := limit.Min[api.ResourceName(k)]
 				defaultRequestQuantity := limit.DefaultRequest[api.ResourceName(k)]
-				allErrs = append(allErrs, errs.NewFieldInvalid(fmt.Sprintf("spec.limits[%d].defaultRequest[%s]", i, k), defaultRequestValue, fmt.Sprintf("min value %s is greater than default request value %s", minQuantity.String(), defaultRequestQuantity.String())))
+				allErrs = append(allErrs, errs.NewFieldInvalid(fmt.Sprintf("spec.limits[%d].defaultRequest[%s]", i, k), defaultRequestQuantity, fmt.Sprintf("min value %s is greater than default request value %s", minQuantity.String(), defaultRequestQuantity.String())))
 			}
 
-			if defaultRequestValueFound && maxValueFound && defaultRequestValue > maxValue {
+			if defaultRequestQuantityFound && maxQuantityFound && defaultRequestQuantity.Cmp(maxQuantity) > 0 {
 				maxQuantity := limit.Max[api.ResourceName(k)]
 				defaultRequestQuantity := limit.DefaultRequest[api.ResourceName(k)]
-				allErrs = append(allErrs, errs.NewFieldInvalid(fmt.Sprintf("spec.limits[%d].defaultRequest[%s]", i, k), defaultRequestValue, fmt.Sprintf("default request value %s is greater than max value %s", defaultRequestQuantity.String(), maxQuantity.String())))
+				allErrs = append(allErrs, errs.NewFieldInvalid(fmt.Sprintf("spec.limits[%d].defaultRequest[%s]", i, k), defaultRequestQuantity, fmt.Sprintf("default request value %s is greater than max value %s", defaultRequestQuantity.String(), maxQuantity.String())))
 			}
 
-			if defaultRequestValueFound && defaultValueFound && defaultRequestValue > defaultValue {
+			if defaultRequestQuantityFound && defaultQuantityFound && defaultRequestQuantity.Cmp(defaultQuantity) > 0 {
 				defaultQuantity := limit.Default[api.ResourceName(k)]
 				defaultRequestQuantity := limit.DefaultRequest[api.ResourceName(k)]
-				allErrs = append(allErrs, errs.NewFieldInvalid(fmt.Sprintf("spec.limits[%d].defaultRequest[%s]", i, k), defaultRequestValue, fmt.Sprintf("default request value %s is greater than default limit value %s", defaultRequestQuantity.String(), defaultQuantity.String())))
+				allErrs = append(allErrs, errs.NewFieldInvalid(fmt.Sprintf("spec.limits[%d].defaultRequest[%s]", i, k), defaultRequestQuantity, fmt.Sprintf("default request value %s is greater than default limit value %s", defaultRequestQuantity.String(), defaultQuantity.String())))
 			}
 
-			if defaultValueFound && minValueFound && minValue > defaultValue {
+			if defaultQuantityFound && minQuantityFound && minQuantity.Cmp(defaultQuantity) > 0 {
 				minQuantity := limit.Min[api.ResourceName(k)]
 				defaultQuantity := limit.Default[api.ResourceName(k)]
-				allErrs = append(allErrs, errs.NewFieldInvalid(fmt.Sprintf("spec.limits[%d].default[%s]", i, k), minValue, fmt.Sprintf("min value %s is greater than default value %s", minQuantity.String(), defaultQuantity.String())))
+				allErrs = append(allErrs, errs.NewFieldInvalid(fmt.Sprintf("spec.limits[%d].default[%s]", i, k), minQuantity, fmt.Sprintf("min value %s is greater than default value %s", minQuantity.String(), defaultQuantity.String())))
 			}
 
-			if defaultValueFound && maxValueFound && defaultValue > maxValue {
+			if defaultQuantityFound && maxQuantityFound && defaultQuantity.Cmp(maxQuantity) > 0 {
 				maxQuantity := limit.Max[api.ResourceName(k)]
 				defaultQuantity := limit.Default[api.ResourceName(k)]
-				allErrs = append(allErrs, errs.NewFieldInvalid(fmt.Sprintf("spec.limits[%d].default[%s]", i, k), maxValue, fmt.Sprintf("default value %s is greater than max value %s", defaultQuantity.String(), maxQuantity.String())))
+				allErrs = append(allErrs, errs.NewFieldInvalid(fmt.Sprintf("spec.limits[%d].default[%s]", i, k), maxQuantity, fmt.Sprintf("default value %s is greater than max value %s", defaultQuantity.String(), maxQuantity.String())))
 			}
 		}
 	}

pkg/api/validation/validation_test.go

@@ -2886,138 +2886,58 @@ func TestValidateResourceNames(t *testing.T) {
 	}
 }
 
+func getResourceList(cpu, memory string) api.ResourceList {
+	res := api.ResourceList{}
+	if cpu != "" {
+		res[api.ResourceCPU] = resource.MustParse(cpu)
+	}
+	if memory != "" {
+		res[api.ResourceMemory] = resource.MustParse(memory)
+	}
+	return res
+}
+
 func TestValidateLimitRange(t *testing.T) {
-	spec := api.LimitRangeSpec{
+	successCases := []struct {
-		Limits: []api.LimitRangeItem{
+		name string
-			{
+		spec api.LimitRangeSpec
-				Type: api.LimitTypePod,
+	}{
-				Max: api.ResourceList{
-					api.ResourceCPU:    resource.MustParse("100"),
-					api.ResourceMemory: resource.MustParse("10000"),
-				},
-				Min: api.ResourceList{
-					api.ResourceCPU:    resource.MustParse("5"),
-					api.ResourceMemory: resource.MustParse("100"),
-				},
-				Default: api.ResourceList{
-					api.ResourceCPU:    resource.MustParse("50"),
-					api.ResourceMemory: resource.MustParse("500"),
-				},
-				DefaultRequest: api.ResourceList{
-					api.ResourceCPU:    resource.MustParse("10"),
-					api.ResourceMemory: resource.MustParse("200"),
-				},
-				MaxLimitRequestRatio: api.ResourceList{
-					api.ResourceCPU: resource.MustParse("20"),
-				},
-			},
-		},
-	}
-
-	invalidSpecDuplicateType := api.LimitRangeSpec{
-		Limits: []api.LimitRangeItem{
-			{
-				Type: api.LimitTypePod,
-				Max: api.ResourceList{
-					api.ResourceCPU:    resource.MustParse("100"),
-					api.ResourceMemory: resource.MustParse("10000"),
-				},
-				Min: api.ResourceList{
-					api.ResourceCPU:    resource.MustParse("0"),
-					api.ResourceMemory: resource.MustParse("100"),
-				},
-			},
-			{
-				Type: api.LimitTypePod,
-				Min: api.ResourceList{
-					api.ResourceCPU:    resource.MustParse("0"),
-					api.ResourceMemory: resource.MustParse("100"),
-				},
-			},
-		},
-	}
-
-	invalidSpecRangeMaxLessThanMin := api.LimitRangeSpec{
-		Limits: []api.LimitRangeItem{
-			{
-				Type: api.LimitTypePod,
-				Max: api.ResourceList{
-					api.ResourceCPU: resource.MustParse("10"),
-				},
-				Min: api.ResourceList{
-					api.ResourceCPU: resource.MustParse("1000"),
-				},
-			},
-		},
-	}
-
-	invalidSpecRangeDefaultOutsideRange := api.LimitRangeSpec{
-		Limits: []api.LimitRangeItem{
-			{
-				Type: api.LimitTypePod,
-				Max: api.ResourceList{
-					api.ResourceCPU: resource.MustParse("1000"),
-				},
-				Min: api.ResourceList{
-					api.ResourceCPU: resource.MustParse("100"),
-				},
-				Default: api.ResourceList{
-					api.ResourceCPU: resource.MustParse("2000"),
-				},
-			},
-		},
-	}
-
-	invalidSpecRangeDefaultRequestOutsideRange := api.LimitRangeSpec{
-		Limits: []api.LimitRangeItem{
-			{
-				Type: api.LimitTypePod,
-				Max: api.ResourceList{
-					api.ResourceCPU: resource.MustParse("1000"),
-				},
-				Min: api.ResourceList{
-					api.ResourceCPU: resource.MustParse("100"),
-				},
-				DefaultRequest: api.ResourceList{
-					api.ResourceCPU: resource.MustParse("2000"),
-				},
-			},
-		},
-	}
-
-	invalidSpecRangeRequestMoreThanDefaultRange := api.LimitRangeSpec{
-		Limits: []api.LimitRangeItem{
-			{
-				Type: api.LimitTypePod,
-				Max: api.ResourceList{
-					api.ResourceCPU: resource.MustParse("1000"),
-				},
-				Min: api.ResourceList{
-					api.ResourceCPU: resource.MustParse("100"),
-				},
-				Default: api.ResourceList{
-					api.ResourceCPU: resource.MustParse("500"),
-				},
-				DefaultRequest: api.ResourceList{
-					api.ResourceCPU: resource.MustParse("800"),
-				},
-			},
-		},
-	}
-
-	successCases := []api.LimitRange{
 		{
-			ObjectMeta: api.ObjectMeta{
+			name: "all-fields-valid",
-				Name:      "abc",
+			spec: api.LimitRangeSpec{
-				Namespace: "foo",
+				Limits: []api.LimitRangeItem{
+					{
+						Type:                 api.LimitTypePod,
+						Max:                  getResourceList("100m", "10000Mi"),
+						Min:                  getResourceList("5m", "100Mi"),
+						Default:              getResourceList("50m", "500Mi"),
+						DefaultRequest:       getResourceList("10m", "200Mi"),
+						MaxLimitRequestRatio: getResourceList("10", ""),
+					},
+				},
+			},
+		},
+		{
+			name: "all-fields-valid-big-numbers",
+			spec: api.LimitRangeSpec{
+				Limits: []api.LimitRangeItem{
+					{
+						Type:                 api.LimitTypePod,
+						Max:                  getResourceList("100m", "10000T"),
+						Min:                  getResourceList("5m", "100Mi"),
+						Default:              getResourceList("50m", "500Mi"),
+						DefaultRequest:       getResourceList("10m", "200Mi"),
+						MaxLimitRequestRatio: getResourceList("10", ""),
+					},
+				},
 			},
-			Spec: spec,
 		},
 	}
 
 	for _, successCase := range successCases {
-		if errs := ValidateLimitRange(&successCase); len(errs) != 0 {
+		limitRange := &api.LimitRange{ObjectMeta: api.ObjectMeta{Name: successCase.name, Namespace: "foo"}, Spec: successCase.spec}
-			t.Errorf("expected success: %v", errs)
+		if errs := ValidateLimitRange(limitRange); len(errs) != 0 {
+			t.Errorf("Case %v, unexpected error: %v", successCase.name, errs)
 		}
 	}
 
@@ -3025,43 +2945,92 @@ func TestValidateLimitRange(t *testing.T) {
 		R api.LimitRange
 		D string
 	}{
-		"zero-length Name": {
+		"zero-length-name": {
-			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "", Namespace: "foo"}, Spec: spec},
+			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "", Namespace: "foo"}, Spec: api.LimitRangeSpec{}},
 			"name or generateName is required",
 		},
 		"zero-length-namespace": {
-			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: ""}, Spec: spec},
+			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: ""}, Spec: api.LimitRangeSpec{}},
 			"",
 		},
-		"invalid Name": {
+		"invalid-name": {
-			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "^Invalid", Namespace: "foo"}, Spec: spec},
+			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "^Invalid", Namespace: "foo"}, Spec: api.LimitRangeSpec{}},
 			DNSSubdomainErrorMsg,
 		},
-		"invalid Namespace": {
+		"invalid-namespace": {
-			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "^Invalid"}, Spec: spec},
+			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "^Invalid"}, Spec: api.LimitRangeSpec{}},
 			DNS1123LabelErrorMsg,
 		},
-		"duplicate limit type": {
+		"duplicate-limit-type": {
-			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: invalidSpecDuplicateType},
+			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: api.LimitRangeSpec{
+				Limits: []api.LimitRangeItem{
+					{
+						Type: api.LimitTypePod,
+						Max:  getResourceList("100m", "10000m"),
+						Min:  getResourceList("0m", "100m"),
+					},
+					{
+						Type: api.LimitTypePod,
+						Min:  getResourceList("0m", "100m"),
+					},
+				},
+			}},
 			"",
 		},
-		"min value 1k is greater than max value 10": {
+		"min value 100m is greater than max value 10m": {
-			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: invalidSpecRangeMaxLessThanMin},
+			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: api.LimitRangeSpec{
-			"min value 1k is greater than max value 10",
+				Limits: []api.LimitRangeItem{
+					{
+						Type: api.LimitTypePod,
+						Max:  getResourceList("10m", ""),
+						Min:  getResourceList("100m", ""),
+					},
+				},
+			}},
+			"min value 100m is greater than max value 10m",
 		},
 		"invalid spec default outside range": {
-			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: invalidSpecRangeDefaultOutsideRange},
+			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: api.LimitRangeSpec{
-			"default value 2k is greater than max value 1k",
+				Limits: []api.LimitRangeItem{
+					{
+						Type:    api.LimitTypePod,
+						Max:     getResourceList("1", ""),
+						Min:     getResourceList("100m", ""),
+						Default: getResourceList("2000m", ""),
+					},
+				},
+			}},
+			"default value 2 is greater than max value 1",
 		},
 		"invalid spec defaultrequest outside range": {
-			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: invalidSpecRangeDefaultRequestOutsideRange},
+			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: api.LimitRangeSpec{
-			"default request value 2k is greater than max value 1k",
+				Limits: []api.LimitRangeItem{
+					{
+						Type:           api.LimitTypePod,
+						Max:            getResourceList("1", ""),
+						Min:            getResourceList("100m", ""),
+						DefaultRequest: getResourceList("2000m", ""),
+					},
+				},
+			}},
+			"default request value 2 is greater than max value 1",
 		},
 		"invalid spec defaultrequest more than default": {
-			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: invalidSpecRangeRequestMoreThanDefaultRange},
+			api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: api.LimitRangeSpec{
-			"default request value 800 is greater than default limit value 500",
+				Limits: []api.LimitRangeItem{
+					{
+						Type:           api.LimitTypeContainer,
+						Max:            getResourceList("2", ""),
+						Min:            getResourceList("100m", ""),
+						Default:        getResourceList("500m", ""),
+						DefaultRequest: getResourceList("800m", ""),
+					},
+				},
+			}},
+			"default request value 800m is greater than default limit value 500m",
 		},
 	}
+
 	for k, v := range errorCases {
 		errs := ValidateLimitRange(&v.R)
 		if len(errs) == 0 {
@@ -3074,6 +3043,7 @@ func TestValidateLimitRange(t *testing.T) {
 			}
 		}
 	}
+
 }
 
 func TestValidateResourceQuota(t *testing.T) {