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

merged 1 commit into from
Mar 10, 2016

Conversation

aveshagarwal
Copy link
Member

No description provided.

@aveshagarwal
Copy link
Member Author

@kubernetes/rh-cluster-infra @derekwaynecarr

@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 10, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 10, 2016

GCE e2e build/test passed for commit a46c5c8.

@k8s-github-robot
Copy link

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

@aveshagarwal aveshagarwal mentioned this pull request Mar 10, 2016
Closed
derekwaynecarr
derekwaynecarr reviewed Mar 10, 2016
View reviewed changes
pkg/api/validation/validation.go
@@ -2044,6 +2055,9 @@ func ValidateLimitRange(limitRange *api.LimitRange) field.ErrorList {
for i := range limitRange.Spec.Limits {
idxPath := fldPath.Index(i)
limit := &limitRange.Spec.Limits[i]
if !validation.IsQualifiedName(string(limit.Type)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this to a separate function and have it check for standard limit types and then require that things that are not standard limit types are qualified (i.e. they have a / ) similar to validateResourceName?

@derekwaynecarr
Copy link
Member

One request then this is LGTM, thanks @aveshagarwal !

@aveshagarwal aveshagarwal force-pushed the master-limitranges-validation-fix branch from a46c5c8 to 4277743 Compare March 10, 2016 16:34
@aveshagarwal
Copy link
Member Author

@derekwaynecarr Updated this PR with validation of standard limit types and non standard limit types (should be fully qualified) and also added a test case. PTAL.

ncdc
ncdc reviewed Mar 10, 2016
View reviewed changes
pkg/api/validation/validation.go
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.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 10, 2016
@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-bot
Copy link

k8s-bot commented Mar 10, 2016

GCE e2e build/test passed for commit 4277743.

@derekwaynecarr
Copy link
Member

LGTM

@derekwaynecarr derekwaynecarr added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2016
@derekwaynecarr derekwaynecarr mentioned this pull request Mar 10, 2016
Closed
85 tasks
@derekwaynecarr derekwaynecarr added this to the v1.2 milestone Mar 10, 2016
@derekwaynecarr
Copy link
Member

I am marking this as a cherry-pick-candidate for release 1.2.

Downstream in OpenShift, we are storing constraints for other resources in our server on the shared limit range in the namespace.

For example, we will let you constraint the size of an Image by adding a openshift.io/image with a max[storage] of a specific value. Putting this in 1.2 will ensure that we can run OpenShift 3.2 on top of an existing Kubernetes 1.2 cluster. This PR just improved the validation when working with third-party limit types.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Mar 10, 2016

GCE e2e build/test passed for commit 4277743.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Mar 10, 2016
Merge pull request #22766 from aveshagarwal/master-limitranges-valida…
7341b0a 
…tion-fix

Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 7341b0a into kubernetes:master Mar 10, 2016
@bgrant0607 bgrant0607 added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Mar 11, 2016
eparis pushed a commit to eparis/kubernetes that referenced this pull request Mar 11, 2016
@eparis
Merge pull request kubernetes#22766 from aveshagarwal/master-limitran…
5a9e134 
…ges-validation-fix

Auto commit by PR queue bot
@eparis eparis mentioned this pull request Mar 11, 2016
Merged
@eparis
Copy link
Contributor

eparis commented Mar 11, 2016

This PR was sucessfully cherry picked in PR #22855
please verify that the release-1.2 branch contains these changes as you would expect and contact @eparis if there appear to be problems.

@eparis eparis removed cherrypick-candidate cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. labels Mar 11, 2016
@aveshagarwal aveshagarwal deleted the master-limitranges-validation-fix branch March 12, 2016 05:38
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@eparis
Merge pull request kubernetes#22766 from aveshagarwal/master-limitran…
a35e665 
…ges-validation-fix

Auto commit by PR queue bot
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@eparis
Merge pull request kubernetes#22766 from aveshagarwal/master-limitran…
ba466e0 
…ges-validation-fix

Auto commit by PR queue bot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@derekwaynecarr derekwaynecarr

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@aveshagarwal @k8s-github-robot @k8s-bot @derekwaynecarr @eparis @ncdc @k8s-teamcity-mesosphere @smarterclayton @googlebot @bgrant0607