Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix validation of resources (cpu, memory, storage) for limit range types. #22766

Merged
merged 1 commit into from Mar 10, 2016
Merged

Fix validation of resources (cpu, memory, storage) for limit range types. #22766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions pkg/api/helpers.go
View file
Open in desktop
Expand Up @@ -127,6 +127,16 @@ func IsStandardContainerResourceName(str string) bool {
return standardContainerResources.Has(str)
}

var standardLimitRangeTypes = sets.NewString(
string(LimitTypePod),
string(LimitTypeContainer),
)

// IsStandardLimitRangeType returns true if the type is Pod or Container
func IsStandardLimitRangeType(str string) bool {
return standardLimitRangeTypes.Has(str)
}

var standardQuotaResources = sets.NewString(
string(ResourceCPU),
string(ResourceMemory),
Expand Down
39 changes: 34 additions & 5 deletions pkg/api/validation/validation.go
View file
Open in desktop
Expand Up @@ -2034,6 +2034,33 @@ func validateResourceQuotaResourceName(value string, fldPath *field.Path) field.
return field.ErrorList{}
}

// Validate limit range types
func validateLimitRangeTypeName(value string, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
if !validation.IsQualifiedName(value) {
return append(allErrs, field.Invalid(fldPath, value, qualifiedNameErrorMsg))
}

if len(strings.Split(value, "/")) == 1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use strings.Contains?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do you really want exactly 1 slash?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that in the function IsQualifiedName and also as per this document:
https://github.com/kubernetes/kubernetes/blob/master/docs/design/resources.md
only one slash is expected to be a qualified name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With more than one slash, IsQualifiedName fails.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, i am biased to stay consistent, so this lgtm.

if !api.IsStandardLimitRangeType(value) {
return append(allErrs, field.Invalid(fldPath, value, "must be a standard limit type or fully qualified"))
}
}

return allErrs
}

// Validate limit range resource name
// limit types (other than Pod/Container) could contain storage not just cpu or memory
func validateLimitRangeResourceName(limitType api.LimitType, value string, fldPath *field.Path) field.ErrorList {
switch limitType {
case api.LimitTypePod, api.LimitTypeContainer:
return validateContainerResourceName(value, fldPath)
default:
return validateResourceName(value, fldPath)
}
}

// ValidateLimitRange tests if required fields in the LimitRange are set.
func ValidateLimitRange(limitRange *api.LimitRange) field.ErrorList {
allErrs := ValidateObjectMeta(&limitRange.ObjectMeta, true, ValidateLimitRangeName, field.NewPath("metadata"))
Expand All @@ -2044,6 +2071,8 @@ func ValidateLimitRange(limitRange *api.LimitRange) field.ErrorList {
for i := range limitRange.Spec.Limits {
idxPath := fldPath.Index(i)
limit := &limitRange.Spec.Limits[i]
allErrs = append(allErrs, validateLimitRangeTypeName(string(limit.Type), idxPath.Child("type"))...)

_, found := limitTypeSet[limit.Type]
if found {
allErrs = append(allErrs, field.Duplicate(idxPath.Child("type"), limit.Type))
Expand All @@ -2058,12 +2087,12 @@ func ValidateLimitRange(limitRange *api.LimitRange) field.ErrorList {
maxLimitRequestRatios := map[string]resource.Quantity{}

for k, q := range limit.Max {
allErrs = append(allErrs, validateContainerResourceName(string(k), idxPath.Child("max").Key(string(k)))...)
allErrs = append(allErrs, validateLimitRangeResourceName(limit.Type, string(k), idxPath.Child("max").Key(string(k)))...)
keys.Insert(string(k))
max[string(k)] = q
}
for k, q := range limit.Min {
allErrs = append(allErrs, validateContainerResourceName(string(k), idxPath.Child("min").Key(string(k)))...)
allErrs = append(allErrs, validateLimitRangeResourceName(limit.Type, string(k), idxPath.Child("min").Key(string(k)))...)
keys.Insert(string(k))
min[string(k)] = q
}
Expand All @@ -2077,19 +2106,19 @@ func ValidateLimitRange(limitRange *api.LimitRange) field.ErrorList {
}
} else {
for k, q := range limit.Default {
allErrs = append(allErrs, validateContainerResourceName(string(k), idxPath.Child("default").Key(string(k)))...)
allErrs = append(allErrs, validateLimitRangeResourceName(limit.Type, string(k), idxPath.Child("default").Key(string(k)))...)
keys.Insert(string(k))
defaults[string(k)] = q
}
for k, q := range limit.DefaultRequest {
allErrs = append(allErrs, validateContainerResourceName(string(k), idxPath.Child("defaultRequest").Key(string(k)))...)
allErrs = append(allErrs, validateLimitRangeResourceName(limit.Type, string(k), idxPath.Child("defaultRequest").Key(string(k)))...)
keys.Insert(string(k))
defaultRequests[string(k)] = q
}
}

for k, q := range limit.MaxLimitRequestRatio {
allErrs = append(allErrs, validateContainerResourceName(string(k), idxPath.Child("maxLimitRequestRatio").Key(string(k)))...)
allErrs = append(allErrs, validateLimitRangeResourceName(limit.Type, string(k), idxPath.Child("maxLimitRequestRatio").Key(string(k)))...)
keys.Insert(string(k))
maxLimitRequestRatios[string(k)] = q
}
Expand Down
53 changes: 53 additions & 0 deletions pkg/api/validation/validation_test.go
View file
Open in desktop
Expand Up @@ -3864,6 +3864,14 @@ func getResourceList(cpu, memory string) api.ResourceList {
return res
}

func getStorageResourceList(storage string) api.ResourceList {
res := api.ResourceList{}
if storage != "" {
res[api.ResourceStorage] = resource.MustParse(storage)
}
return res
}

func TestValidateLimitRange(t *testing.T) {
successCases := []struct {
name string
Expand Down Expand Up @@ -3905,6 +3913,36 @@ func TestValidateLimitRange(t *testing.T) {
},
},
},
{
name: "thirdparty-fields-all-valid-standard-container-resources",
spec: api.LimitRangeSpec{
Limits: []api.LimitRangeItem{
{
Type: "thirdparty.com/foo",
Max: getResourceList("100m", "10000T"),
Min: getResourceList("5m", "100Mi"),
Default: getResourceList("50m", "500Mi"),
DefaultRequest: getResourceList("10m", "200Mi"),
MaxLimitRequestRatio: getResourceList("10", ""),
},
},
},
},
{
name: "thirdparty-fields-all-valid-storage-resources",
spec: api.LimitRangeSpec{
Limits: []api.LimitRangeItem{
{
Type: "thirdparty.com/foo",
Max: getStorageResourceList("10000T"),
Min: getStorageResourceList("100Mi"),
Default: getStorageResourceList("500Mi"),
DefaultRequest: getStorageResourceList("200Mi"),
MaxLimitRequestRatio: getStorageResourceList(""),
},
},
},
},
}

for _, successCase := range successCases {
Expand Down Expand Up @@ -4052,6 +4090,21 @@ func TestValidateLimitRange(t *testing.T) {
}},
"ratio 10 is greater than max/min = 4.000000",
},
"invalid non standard limit type": {
api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: api.LimitRangeSpec{
Limits: []api.LimitRangeItem{
{
Type: "foo",
Max: getStorageResourceList("10000T"),
Min: getStorageResourceList("100Mi"),
Default: getStorageResourceList("500Mi"),
DefaultRequest: getStorageResourceList("200Mi"),
MaxLimitRequestRatio: getStorageResourceList(""),
},
},
}},
"must be a standard limit type or fully qualified",
},
}

for k, v := range errorCases {
Expand Down