From ea919f6d1e716a350d8db9530193574a05370aa6 Mon Sep 17 00:00:00 2001 From: derekwaynecarr Date: Tue, 8 Sep 2015 14:49:54 -0400 Subject: [PATCH] Fix precision handling in validating LimitRange --- pkg/api/resource/quantity.go | 21 +++ pkg/api/resource/quantity_test.go | 20 ++ pkg/api/validation/validation.go | 60 +++--- pkg/api/validation/validation_test.go | 256 ++++++++++++-------------- 4 files changed, 182 insertions(+), 175 deletions(-) diff --git a/pkg/api/resource/quantity.go b/pkg/api/resource/quantity.go index ef8eaef9e6b..577d5b6093c 100644 --- a/pkg/api/resource/quantity.go +++ b/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) diff --git a/pkg/api/resource/quantity_test.go b/pkg/api/resource/quantity_test.go index 70f4836bb1d..da8858ea97d 100644 --- a/pkg/api/resource/quantity_test.go +++ b/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 diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 43ea1e7e1be..d5965bce358 100644 --- a/pkg/api/validation/validation.go +++ b/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{} - max := map[string]int64{} - defaults := map[string]int64{} - defaultRequests := map[string]int64{} + min := map[string]resource.Quantity{} + max := map[string]resource.Quantity{} + defaults := map[string]resource.Quantity{} + 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.Value() + max[string(k)] = q } - 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.Value() + min[string(k)] = q } - 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.Value() + defaults[string(k)] = q } - 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.Value() + defaultRequests[string(k)] = q } 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] - maxValue, maxValueFound := max[k] - defaultValue, defaultValueFound := defaults[k] - defaultRequestValue, defaultRequestValueFound := defaultRequests[k] + minQuantity, minQuantityFound := min[k] + maxQuantity, maxQuantityFound := max[k] + defaultQuantity, defaultQuantityFound := defaults[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()))) } } } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 3f71b703950..7a1a0167899 100644 --- a/pkg/api/validation/validation_test.go +++ b/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{ - 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("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{ + successCases := []struct { + name string + spec api.LimitRangeSpec + }{ { - ObjectMeta: api.ObjectMeta{ - Name: "abc", - Namespace: "foo", + name: "all-fields-valid", + spec: api.LimitRangeSpec{ + 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 { - t.Errorf("expected success: %v", errs) + limitRange := &api.LimitRange{ObjectMeta: api.ObjectMeta{Name: successCase.name, Namespace: "foo"}, Spec: successCase.spec} + 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": { - api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "", Namespace: "foo"}, Spec: spec}, + "zero-length-name": { + 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": { - api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "^Invalid", Namespace: "foo"}, Spec: spec}, + "invalid-name": { + api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "^Invalid", Namespace: "foo"}, Spec: api.LimitRangeSpec{}}, DNSSubdomainErrorMsg, }, - "invalid Namespace": { - api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "^Invalid"}, Spec: spec}, + "invalid-namespace": { + api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "^Invalid"}, Spec: api.LimitRangeSpec{}}, DNS1123LabelErrorMsg, }, - "duplicate limit type": { - api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: invalidSpecDuplicateType}, + "duplicate-limit-type": { + 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": { - api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: invalidSpecRangeMaxLessThanMin}, - "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: api.LimitRangeSpec{ + 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}, - "default value 2k is greater than max value 1k", + api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: api.LimitRangeSpec{ + 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}, - "default request value 2k is greater than max value 1k", + api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: api.LimitRangeSpec{ + 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}, - "default request value 800 is greater than default limit value 500", + api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: api.LimitRangeSpec{ + 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) {