Merge pull request #2142 from dmcgowan/reference-enforce-canonical-pa…

…rsing

reference: ParseNamed updated to enforce canonical format

manifest/schema1/config_builder_test.go

@@ -197,10 +197,14 @@ func TestConfigBuilder(t *testing.T) {
 
 	bs := &mockBlobService{descriptors: make(map[digest.Digest]distribution.Descriptor)}
 
-	ref, err := reference.ParseNamed("testrepo:testtag")
+	ref, err := reference.WithName("testrepo")
 	if err != nil {
 		t.Fatalf("could not parse reference: %v", err)
 	}
+	ref, err = reference.WithTag(ref, "testtag")
+	if err != nil {
+		t.Fatalf("could not add tag: %v", err)
+	}
 
 	builder := NewConfigManifestBuilder(bs, pk, ref, []byte(imgJSON))
 

manifest/schema1/reference_builder_test.go

@@ -55,7 +55,7 @@ func TestReferenceBuilder(t *testing.T) {
 
 	handCrafted := makeSignedManifest(t, pk, []Reference{r1, r2})
 
-	ref, err := reference.ParseNamed(handCrafted.Manifest.Name)
+	ref, err := reference.WithName(handCrafted.Manifest.Name)
 	if err != nil {
 		t.Fatalf("could not parse reference: %v", err)
 	}

notifications/bridge_test.go

@@ -43,7 +43,7 @@ func TestEventBridgeManifestPulled(t *testing.T) {
 		return nil
 	}))
 
-	repoRef, _ := reference.ParseNamed(repo)
+	repoRef, _ := reference.WithName(repo)
 	if err := l.ManifestPulled(repoRef, sm); err != nil {
 		t.Fatalf("unexpected error notifying manifest pull: %v", err)
 	}
@@ -56,7 +56,7 @@ func TestEventBridgeManifestPushed(t *testing.T) {
 		return nil
 	}))
 
-	repoRef, _ := reference.ParseNamed(repo)
+	repoRef, _ := reference.WithName(repo)
 	if err := l.ManifestPushed(repoRef, sm); err != nil {
 		t.Fatalf("unexpected error notifying manifest pull: %v", err)
 	}
@@ -72,7 +72,7 @@ func TestEventBridgeManifestPushedWithTag(t *testing.T) {
 		return nil
 	}))
 
-	repoRef, _ := reference.ParseNamed(repo)
+	repoRef, _ := reference.WithName(repo)
 	if err := l.ManifestPushed(repoRef, sm, distribution.WithTag(m.Tag)); err != nil {
 		t.Fatalf("unexpected error notifying manifest pull: %v", err)
 	}
@@ -88,7 +88,7 @@ func TestEventBridgeManifestPulledWithTag(t *testing.T) {
 		return nil
 	}))
 
-	repoRef, _ := reference.ParseNamed(repo)
+	repoRef, _ := reference.WithName(repo)
 	if err := l.ManifestPulled(repoRef, sm, distribution.WithTag(m.Tag)); err != nil {
 		t.Fatalf("unexpected error notifying manifest pull: %v", err)
 	}
@@ -100,7 +100,7 @@ func TestEventBridgeManifestDeleted(t *testing.T) {
 		return nil
 	}))
 
-	repoRef, _ := reference.ParseNamed(repo)
+	repoRef, _ := reference.WithName(repo)
 	if err := l.ManifestDeleted(repoRef, dgst); err != nil {
 		t.Fatalf("unexpected error notifying manifest pull: %v", err)
 	}
@@ -160,7 +160,7 @@ func checkCommonManifest(t *testing.T, action string, events ...Event) {
 		t.Fatalf("unexpected event action: %q != %q", event.Action, action)
 	}
 
-	repoRef, _ := reference.ParseNamed(repo)
+	repoRef, _ := reference.WithName(repo)
 	ref, _ := reference.WithDigest(repoRef, dgst)
 	u, err := ub.BuildManifestURL(ref)
 	if err != nil {

notifications/listener_test.go

@@ -33,7 +33,7 @@ func TestListener(t *testing.T) {
 		ops: make(map[string]int),
 	}
 
-	repoRef, _ := reference.ParseNamed("foo/bar")
+	repoRef, _ := reference.WithName("foo/bar")
 	repository, err := registry.Repository(ctx, repoRef)
 	if err != nil {
 		t.Fatalf("unexpected error getting repo: %v", err)

reference/normalize_test.go

@@ -208,6 +208,13 @@ func TestParseRepositoryInfo(t *testing.T) {
 			AmbiguousName: "index.docker.io/library/ubuntu-12.04-base",
 			Domain:        "docker.io",
 		},
+		{
+			RemoteName:    "library/foo",
+			FamiliarName:  "foo",
+			FullName:      "docker.io/library/foo",
+			AmbiguousName: "docker.io/foo",
+			Domain:        "docker.io",
+		},
 		{
 			RemoteName:    "library/foo/bar",
 			FamiliarName:  "library/foo/bar",

reference/reference.go

@@ -55,6 +55,9 @@ var (
 
 	// ErrNameTooLong is returned when a repository name is longer than NameTotalLengthMax.
 	ErrNameTooLong = fmt.Errorf("repository name must not be more than %v characters", NameTotalLengthMax)
+
+	// ErrNameNotCanonical is returned when a name is not canonical.
+	ErrNameNotCanonical = errors.New("repository name must be canonical")
 )
 
 // Reference is an opaque object reference identifier that may include
@@ -231,18 +234,17 @@ func Parse(s string) (Reference, error) {
 }
 
 // ParseNamed parses s and returns a syntactically valid reference implementing
-// the Named interface. The reference must have a name, otherwise an error is
-// returned.
+// the Named interface. The reference must have a name and be in the canonical
+// form, otherwise an error is returned.
 // If an error was encountered it is returned, along with a nil Reference.
 // NOTE: ParseNamed will not handle short digests.
 func ParseNamed(s string) (Named, error) {
-	ref, err := Parse(s)
+	named, err := ParseNormalizedNamed(s)
 	if err != nil {
 		return nil, err
 	}
-	named, isNamed := ref.(Named)
-	if !isNamed {
-		return nil, fmt.Errorf("reference %s has no name", ref.String())
+	if named.String() != s {
+		return nil, ErrNameNotCanonical
 	}
 	return named, nil
 }

reference/reference_test.go

@@ -583,3 +583,77 @@ func TestWithDigest(t *testing.T) {
 		}
 	}
 }
+
+func TestParseNamed(t *testing.T) {
+	testcases := []struct {
+		input  string
+		domain string
+		name   string
+		err    error
+	}{
+		{
+			input:  "test.com/foo",
+			domain: "test.com",
+			name:   "foo",
+		},
+		{
+			input:  "test:8080/foo",
+			domain: "test:8080",
+			name:   "foo",
+		},
+		{
+			input: "test_com/foo",
+			err:   ErrNameNotCanonical,
+		},
+		{
+			input: "test.com",
+			err:   ErrNameNotCanonical,
+		},
+		{
+			input: "foo",
+			err:   ErrNameNotCanonical,
+		},
+		{
+			input: "library/foo",
+			err:   ErrNameNotCanonical,
+		},
+		{
+			input:  "docker.io/library/foo",
+			domain: "docker.io",
+			name:   "library/foo",
+		},
+		// Ambiguous case, parser will add "library/" to foo
+		{
+			input: "docker.io/foo",
+			err:   ErrNameNotCanonical,
+		},
+	}
+	for _, testcase := range testcases {
+		failf := func(format string, v ...interface{}) {
+			t.Logf(strconv.Quote(testcase.input)+": "+format, v...)
+			t.Fail()
+		}
+
+		named, err := ParseNamed(testcase.input)
+		if err != nil && testcase.err == nil {
+			failf("error parsing name: %s", err)
+			continue
+		} else if err == nil && testcase.err != nil {
+			failf("parsing succeded: expected error %v", testcase.err)
+			continue
+		} else if err != testcase.err {
+			failf("unexpected error %v, expected %v", err, testcase.err)
+			continue
+		} else if err != nil {
+			continue
+		}
+
+		domain, name := SplitHostname(named)
+		if domain != testcase.domain {
+			failf("unexpected domain: got %q, expected %q", domain, testcase.domain)
+		}
+		if name != testcase.name {
+			failf("unexpected name: got %q, expected %q", name, testcase.name)
+		}
+	}
+}

registry/api/v2/urls_test.go

@@ -15,7 +15,7 @@ type urlBuilderTestCase struct {
 }
 
 func makeURLBuilderTestCases(urlBuilder *URLBuilder) []urlBuilderTestCase {
-	fooBarRef, _ := reference.ParseNamed("foo/bar")
+	fooBarRef, _ := reference.WithName("foo/bar")
 	return []urlBuilderTestCase{
 		{
 			description:  "test base url",

registry/client/repository_test.go

@@ -100,7 +100,7 @@ func addTestCatalog(route string, content []byte, link string, m *testutil.Reque
 func TestBlobDelete(t *testing.T) {
 	dgst, _ := newRandomBlob(1024)
 	var m testutil.RequestResponseMap
-	repo, _ := reference.ParseNamed("test.example.com/repo1")
+	repo, _ := reference.WithName("test.example.com/repo1")
 	m = append(m, testutil.RequestResponseMapping{
 		Request: testutil.Request{
 			Method: "DELETE",
@@ -139,7 +139,7 @@ func TestBlobFetch(t *testing.T) {
 	defer c()
 
 	ctx := context.Background()
-	repo, _ := reference.ParseNamed("test.example.com/repo1")
+	repo, _ := reference.WithName("test.example.com/repo1")
 	r, err := NewRepository(ctx, repo, e, nil)
 	if err != nil {
 		t.Fatal(err)
@@ -160,7 +160,7 @@ func TestBlobFetch(t *testing.T) {
 func TestBlobExistsNoContentLength(t *testing.T) {
 	var m testutil.RequestResponseMap
 
-	repo, _ := reference.ParseNamed("biff")
+	repo, _ := reference.WithName("biff")
 	dgst, content := newRandomBlob(1024)
 	m = append(m, testutil.RequestResponseMapping{
 		Request: testutil.Request{
@@ -219,7 +219,7 @@ func TestBlobExists(t *testing.T) {
 	defer c()
 
 	ctx := context.Background()
-	repo, _ := reference.ParseNamed("test.example.com/repo1")
+	repo, _ := reference.WithName("test.example.com/repo1")
 	r, err := NewRepository(ctx, repo, e, nil)
 	if err != nil {
 		t.Fatal(err)
@@ -251,7 +251,7 @@ func TestBlobUploadChunked(t *testing.T) {
 		b1[512:513],
 		b1[513:1024],
 	}
-	repo, _ := reference.ParseNamed("test.example.com/uploadrepo")
+	repo, _ := reference.WithName("test.example.com/uploadrepo")
 	uuids := []string{uuid.Generate().String()}
 	m = append(m, testutil.RequestResponseMapping{
 		Request: testutil.Request{
@@ -366,7 +366,7 @@ func TestBlobUploadChunked(t *testing.T) {
 func TestBlobUploadMonolithic(t *testing.T) {
 	dgst, b1 := newRandomBlob(1024)
 	var m testutil.RequestResponseMap
-	repo, _ := reference.ParseNamed("test.example.com/uploadrepo")
+	repo, _ := reference.WithName("test.example.com/uploadrepo")
 	uploadID := uuid.Generate().String()
 	m = append(m, testutil.RequestResponseMapping{
 		Request: testutil.Request{
@@ -474,9 +474,9 @@ func TestBlobUploadMonolithic(t *testing.T) {
 func TestBlobMount(t *testing.T) {
 	dgst, content := newRandomBlob(1024)
 	var m testutil.RequestResponseMap
-	repo, _ := reference.ParseNamed("test.example.com/uploadrepo")
+	repo, _ := reference.WithName("test.example.com/uploadrepo")
 
-	sourceRepo, _ := reference.ParseNamed("test.example.com/sourcerepo")
+	sourceRepo, _ := reference.WithName("test.example.com/sourcerepo")
 	canonicalRef, _ := reference.WithDigest(sourceRepo, dgst)
 
 	m = append(m, testutil.RequestResponseMapping{
@@ -678,7 +678,7 @@ func checkEqualManifest(m1, m2 *schema1.SignedManifest) error {
 
 func TestV1ManifestFetch(t *testing.T) {
 	ctx := context.Background()
-	repo, _ := reference.ParseNamed("test.example.com/repo")
+	repo, _ := reference.WithName("test.example.com/repo")
 	m1, dgst, _ := newRandomSchemaV1Manifest(repo, "latest", 6)
 	var m testutil.RequestResponseMap
 	_, pl, err := m1.Payload()
@@ -755,7 +755,7 @@ func TestV1ManifestFetch(t *testing.T) {
 }
 
 func TestManifestFetchWithEtag(t *testing.T) {
-	repo, _ := reference.ParseNamed("test.example.com/repo/by/tag")
+	repo, _ := reference.WithName("test.example.com/repo/by/tag")
 	_, d1, p1 := newRandomSchemaV1Manifest(repo, "latest", 6)
 	var m testutil.RequestResponseMap
 	addTestManifestWithEtag(repo, "latest", p1, &m, d1.String())
@@ -785,7 +785,7 @@ func TestManifestFetchWithEtag(t *testing.T) {
 }
 
 func TestManifestDelete(t *testing.T) {
-	repo, _ := reference.ParseNamed("test.example.com/repo/delete")
+	repo, _ := reference.WithName("test.example.com/repo/delete")
 	_, dgst1, _ := newRandomSchemaV1Manifest(repo, "latest", 6)
 	_, dgst2, _ := newRandomSchemaV1Manifest(repo, "latest", 6)
 	var m testutil.RequestResponseMap
@@ -825,7 +825,7 @@ func TestManifestDelete(t *testing.T) {
 }
 
 func TestManifestPut(t *testing.T) {
-	repo, _ := reference.ParseNamed("test.example.com/repo/delete")
+	repo, _ := reference.WithName("test.example.com/repo/delete")
 	m1, dgst, _ := newRandomSchemaV1Manifest(repo, "other", 6)
 
 	_, payload, err := m1.Payload()
@@ -890,7 +890,7 @@ func TestManifestPut(t *testing.T) {
 }
 
 func TestManifestTags(t *testing.T) {
-	repo, _ := reference.ParseNamed("test.example.com/repo/tags/list")
+	repo, _ := reference.WithName("test.example.com/repo/tags/list")
 	tagsList := []byte(strings.TrimSpace(`
 {
 	"name": "test.example.com/repo/tags/list",
@@ -952,7 +952,7 @@ func TestManifestTags(t *testing.T) {
 }
 
 func TestObtainsErrorForMissingTag(t *testing.T) {
-	repo, _ := reference.ParseNamed("test.example.com/repo")
+	repo, _ := reference.WithName("test.example.com/repo")
 
 	var m testutil.RequestResponseMap
 	var errors errcode.Errors
@@ -998,7 +998,7 @@ func TestManifestTagsPaginated(t *testing.T) {
 	s := httptest.NewServer(http.NotFoundHandler())
 	defer s.Close()
 
-	repo, _ := reference.ParseNamed("test.example.com/repo/tags/list")
+	repo, _ := reference.WithName("test.example.com/repo/tags/list")
 	tagsList := []string{"tag1", "tag2", "funtag"}
 	var m testutil.RequestResponseMap
 	for i := 0; i < 3; i++ {
@@ -1067,7 +1067,7 @@ func TestManifestTagsPaginated(t *testing.T) {
 }
 
 func TestManifestUnauthorized(t *testing.T) {
-	repo, _ := reference.ParseNamed("test.example.com/repo")
+	repo, _ := reference.WithName("test.example.com/repo")
 	_, dgst, _ := newRandomSchemaV1Manifest(repo, "latest", 6)
 	var m testutil.RequestResponseMap