Validate project and image stream namespaces and names #2351

liggitt · 2015-05-19T15:17:22Z

To ensure we end up with valid image repository paths derived from ImageStreams, add the following validation:

Require Project names to be >= 2 characters on creation
Require ImageStream namespaces to be >= 2 characters
Require ImageStream namespace+'/'+name to be <= 255 characters
Require ImageStream names to be valid docker image repository names

To ensure we don't break updating an existing project/namespace that was created with a single character name:

Allow updating projects with names < 2 characters

Fixes #1232

liggitt · 2015-05-19T15:18:54Z

@derekwaynecarr @ncdc PTAL

ncdc · 2015-05-19T15:22:36Z

pkg/image/api/validation/validation.go

+		result = append(result, fielderrors.NewFieldInvalid("metadata.namespace", stream.Namespace, "must be at least 2 characters long"))
+	}
+	if len(stream.Namespace+"/"+stream.Name) > 255 {
+		result = append(result, fielderrors.NewFieldInvalid("metadata.name", stream.Name, "'namespace/name' must be less than 255 characters long"))


<= 255, < 256

liggitt · 2015-05-19T15:56:32Z

@ncdc imagestream comments addressed, ParseDockerImageReference validation added as well

ncdc · 2015-05-19T16:01:17Z

pkg/image/api/helper.go

+	repo := ns + "/" + ref.Name
+
+	// Length checking
+	if len(ns) < DockerMinNamespaceLength {


Each component must be at least 2 chars - meaning both ns and ref.Name

You can call this if you want https://github.com/docker/distribution/blob/master/registry/api/v2/names.go#L78

liggitt · 2015-05-19T16:16:56Z

@ncdc updated to use docker constants/validation

ncdc · 2015-05-19T16:20:20Z

pkg/image/api/validation/validation.go

@@ -43,6 +44,17 @@ func ValidateImageStream(stream *api.ImageStream) fielderrors.ValidationErrorLis

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

+	// Ensure we can generate a valid docker image repository from namespace/name


This is technically only relevant if the stream is using our internal registry. If spec.dockerImageRepository is set, it's ok to have a 1-char ns or name, since they won't be used in the push/pull spec. Not sure if it's worth making that distinction in the code, however.

Since they can switch between internal/external post-creation, I'd rather keep this "strict" for now

ncdc · 2015-05-19T16:29:49Z

ImageStream & DockerImageReference changes LGTM

derekwaynecarr · 2015-05-19T16:52:09Z

pkg/project/api/validation/validation.go

@@ -12,12 +11,14 @@ import (
 )

 // ValidateProject tests required fields for a Project.
-func ValidateProject(project *api.Project) fielderrors.ValidationErrorList {
+func ValidateProject(project *api.Project, create bool) fielderrors.ValidationErrorList {


I don't think you need to change this signature.

On create we call:
https://github.com/openshift/origin/blob/master/pkg/project/registry/project/rest.go#L57

On update we call:
https://github.com/openshift/origin/blob/master/pkg/project/registry/project/rest.go#L67

This was calling ValidateProject(), and I couldn't tell how it was used: https://github.com/openshift/origin/pull/2351/files#diff-f19a7777a36882029db668a427f950feL71

It was only used in an examples test, removed the signature change

liggitt · 2015-05-19T18:55:09Z

[test]

openshift-bot · 2015-05-19T18:55:45Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/2330/)

smarterclayton · 2015-05-19T19:36:59Z

pkg/image/api/helper.go

 	default:
 		return ref, fmt.Errorf("the docker pull spec %q must be two or three segments separated by slashes", spec)
 	}
+
+	if len(ref.Namespace) == 0 {


Very scary.

I think this code is used by things where this is not a safe assumption.

We had the defaulting for registry/name but not for name. What code do we have that expects no namespace in a pull spec?

----- Original Message -----

default: return ref, fmt.Errorf("the docker pull spec %q must be two or three segments separated by slashes", spec) }

if len(ref.Namespace) == 0 {

We had the defaulting for registry/name but not for name. What code do we
have that expects no namespace in a pull spec?

Is library even present in v2?

I'm putting back the "library" defaulting where we had it before, and only validating what they passed.

Library isn't a strict requirement in v2, but the Docker daemon still prepends library if you only specify foo

I'm less concerned if they give us an invalid spec and it fails. The crux of this PR was preventing them from giving us namespace/imagestream pieces we would assemble into an invalid image repository

I'm fine with keeping the library default the way it was originally

liggitt · 2015-05-21T08:34:16Z

@smarterclayton, you ok with this now?

liggitt · 2015-05-21T15:42:54Z

@ncdc @smarterclayton backed out the changes in ParseDockerImageReference to limit the scope of this PR. PTAL

ncdc · 2015-05-21T15:49:44Z

LGTM

liggitt · 2015-05-21T17:54:09Z

[merge]

openshift-bot · 2015-05-21T17:55:30Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/2052/) (Image: devenv-fedora_1595)

openshift-bot · 2015-05-21T19:55:32Z

Evaluated for origin up to de47827

Merged by openshift-bot

ncdc reviewed May 19, 2015
View reviewed changes

derekwaynecarr reviewed May 19, 2015
View reviewed changes

smarterclayton reviewed May 19, 2015
View reviewed changes

Validate project and image stream namespaces and names

de47827

openshift-bot pushed a commit that referenced this pull request May 22, 2015

Merge pull request #2351 from liggitt/name_length_validation

ccdb67d

Merged by openshift-bot

openshift-bot merged commit ccdb67d into openshift:master May 22, 2015

liggitt deleted the name_length_validation branch May 23, 2015 02:01

ghost mentioned this pull request May 26, 2015

Project create page should be updated #2483

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate project and image stream namespaces and names #2351

Validate project and image stream namespaces and names #2351

liggitt commented May 19, 2015

liggitt commented May 19, 2015

ncdc May 19, 2015

liggitt commented May 19, 2015

ncdc May 19, 2015

ncdc May 19, 2015

liggitt commented May 19, 2015

ncdc May 19, 2015

liggitt May 19, 2015

ncdc commented May 19, 2015

derekwaynecarr May 19, 2015

liggitt May 19, 2015

liggitt May 19, 2015

liggitt commented May 19, 2015

openshift-bot commented May 19, 2015

smarterclayton May 19, 2015

smarterclayton May 19, 2015

ncdc May 19, 2015

smarterclayton May 19, 2015

liggitt May 19, 2015

ncdc May 19, 2015

liggitt May 19, 2015

ncdc May 19, 2015

liggitt commented May 21, 2015

liggitt commented May 21, 2015

ncdc commented May 21, 2015

liggitt commented May 21, 2015

openshift-bot commented May 21, 2015

openshift-bot commented May 21, 2015

		@@ -43,6 +44,17 @@ func ValidateImageStream(stream *api.ImageStream) fielderrors.ValidationErrorLis

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

		// Ensure we can generate a valid docker image repository from namespace/name

Validate project and image stream namespaces and names #2351

Validate project and image stream namespaces and names #2351

Conversation

liggitt commented May 19, 2015

liggitt commented May 19, 2015

Choose a reason for hiding this comment

liggitt commented May 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented May 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented May 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented May 19, 2015

openshift-bot commented May 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented May 21, 2015

liggitt commented May 21, 2015

ncdc commented May 21, 2015

liggitt commented May 21, 2015

openshift-bot commented May 21, 2015

openshift-bot commented May 21, 2015