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

prevent slashes in object names #1847

Merged
merged 2 commits into from
Apr 23, 2015
Merged

prevent slashes in object names #1847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
741d271
add imagestream validation
deads2k Apr 21, 2015
6e359df
more tests
deads2k Apr 22, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/api/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ func ValidateObject(obj runtime.Object) (errors []error) {
errors = validation.ValidateMinion(t)

case *imageapi.Image:
t.Namespace = ""
errors = imagev.ValidateImage(t)
case *imageapi.ImageRepository:
s := &imageapi.ImageStream{}
Expand Down
24 changes: 15 additions & 9 deletions pkg/image/api/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package validation

import (
"fmt"
"strings"

"github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation"
"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
Expand All @@ -10,13 +11,21 @@ import (
"github.com/openshift/origin/pkg/image/api"
)

func MinimalNameValidation(name string, prefix bool) (bool, string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need this for every other resource type as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sort of. All the name validators need to at least limit those characters. The others I know about are more restrictive.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see a lot are using ValidatePodName and others are using custom funcs that handle % and / and other characters. It'd be nice to have a single common func that all validations could use to check the bare minimum, and then custom logic could be added after that. But I don't think it's required for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'd be nice to have a single common func that all validations could use to check the bare minimum

My first attempt as rejected because it was too broad. My second attempt failed because our main validation package would become cyclic. I decided I'd rather get enough in to stop us from failing before attempting a more consistent change again.

for _, illegal := range []string{"%", "/"} {
if strings.Contains(name, illegal) {
return false, fmt.Sprintf(`may not contain "%s"`, illegal)
}
}
return true, ""
}

// ValidateImage tests required fields for an Image.
func ValidateImage(image *api.Image) fielderrors.ValidationErrorList {
result := fielderrors.ValidationErrorList{}

if len(image.Name) == 0 {
result = append(result, fielderrors.NewFieldRequired("name"))
}
result = append(result, validation.ValidateObjectMeta(&image.ObjectMeta, false, MinimalNameValidation).Prefix("metadata")...)

if len(image.DockerImageReference) == 0 {
result = append(result, fielderrors.NewFieldRequired("dockerImageReference"))
} else {
Expand All @@ -32,15 +41,12 @@ func ValidateImage(image *api.Image) fielderrors.ValidationErrorList {
func ValidateImageStream(stream *api.ImageStream) fielderrors.ValidationErrorList {
result := fielderrors.ValidationErrorList{}

result = append(result, validation.ValidateObjectMeta(&stream.ObjectMeta, true, MinimalNameValidation).Prefix("metadata")...)

if stream.Spec.Tags == nil {
stream.Spec.Tags = make(map[string]api.TagReference)
}
if len(stream.Name) == 0 {
result = append(result, fielderrors.NewFieldRequired("name"))
}
if !util.IsDNS1123Subdomain(stream.Namespace) {
result = append(result, fielderrors.NewFieldInvalid("namespace", stream.Namespace, ""))
}

if len(stream.Spec.DockerImageRepository) != 0 {
if _, err := api.ParseDockerImageReference(stream.Spec.DockerImageRepository); err != nil {
result = append(result, fielderrors.NewFieldInvalid("spec.dockerImageRepository", stream.Spec.DockerImageRepository, err.Error()))
Expand Down
39 changes: 30 additions & 9 deletions pkg/image/api/validation/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (

func TestValidateImageOK(t *testing.T) {
errs := ValidateImage(&api.Image{
ObjectMeta: kapi.ObjectMeta{Name: "foo", Namespace: "default"},
ObjectMeta: kapi.ObjectMeta{Name: "foo"},
DockerImageReference: "openshift/ruby-19-centos",
})
if len(errs) > 0 {
Expand All @@ -29,7 +29,17 @@ func TestValidateImageMissingFields(t *testing.T) {
"missing Name": {
api.Image{DockerImageReference: "ref"},
fielderrors.ValidationErrorTypeRequired,
"name",
"metadata.name",
},
"no slash in Name": {
api.Image{ObjectMeta: kapi.ObjectMeta{Name: "foo/bar"}},
fielderrors.ValidationErrorTypeInvalid,
"metadata.name",
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Test % too?

"no percent in Name": {
api.Image{ObjectMeta: kapi.ObjectMeta{Name: "foo%%bar"}},
fielderrors.ValidationErrorTypeInvalid,
"metadata.name",
},
"missing DockerImageReference": {
api.Image{ObjectMeta: kapi.ObjectMeta{Name: "foo"}},
Expand Down Expand Up @@ -122,14 +132,11 @@ func TestValidateImageStreamMappingNotOK(t *testing.T) {
DockerImageRepository: "openshift/ruby-19-centos",
Tag: "latest",
Image: api.Image{
ObjectMeta: kapi.ObjectMeta{
Namespace: "default",
},
DockerImageReference: "openshift/ruby-19-centos",
},
},
fielderrors.ValidationErrorTypeRequired,
"image.name",
"image.metadata.name",
},
"invalid repository pull spec": {
api.ImageStreamMapping{
Expand Down Expand Up @@ -183,21 +190,35 @@ func TestValidateImageStream(t *testing.T) {
namespace: "foo",
name: "",
expected: fielderrors.ValidationErrorList{
fielderrors.NewFieldRequired("name"),
fielderrors.NewFieldRequired("metadata.name"),
},
},
"no slash in Name": {
namespace: "foo",
name: "foo/bar",
expected: fielderrors.ValidationErrorList{
fielderrors.NewFieldInvalid("metadata.name", "foo/bar", `may not contain "/"`),
},
},
"no percent in Name": {
namespace: "foo",
name: "foo%%bar",
expected: fielderrors.ValidationErrorList{
fielderrors.NewFieldInvalid("metadata.name", "foo%%bar", `may not contain "%"`),
},
},
"missing namespace": {
namespace: "",
name: "foo",
expected: fielderrors.ValidationErrorList{
fielderrors.NewFieldInvalid("namespace", "", ""),
fielderrors.NewFieldRequired("metadata.namespace"),
},
},
"invalid namespace": {
namespace: "!$",
name: "foo",
expected: fielderrors.ValidationErrorList{
fielderrors.NewFieldInvalid("namespace", "!$", ""),
fielderrors.NewFieldInvalid("metadata.namespace", "!$", `must have at most 253 characters and match regex [a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*`),
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding tests for names with % and / in them?

"invalid dockerImageRepository": {
Expand Down
6 changes: 2 additions & 4 deletions pkg/image/registry/imagerepositorymapping/rest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,8 +356,7 @@ func TestAddExistingImageWithNewTag(t *testing.T) {

existingImage := &api.Image{
ObjectMeta: kapi.ObjectMeta{
Name: imageID,
Namespace: "default",
Name: imageID,
},
DockerImageReference: "localhost:5000/someproject/somerepo:" + imageID,
DockerImageMetadata: api.DockerImage{
Expand Down Expand Up @@ -483,8 +482,7 @@ func TestAddExistingImageAndTag(t *testing.T) {

existingImage := &api.Image{
ObjectMeta: kapi.ObjectMeta{
Name: "existingImage",
Namespace: "default",
Name: "existingImage",
},
DockerImageReference: "localhost:5000/someproject/somerepo:imageID1",
DockerImageMetadata: api.DockerImage{
Expand Down