Change URLBuilder methods to use references for tags and digests

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

docs/api/v2/urls.go

@@ -5,7 +5,6 @@ import (
 	"net/url"
 	"strings"
 
-	"github.com/docker/distribution/digest"
 	"github.com/docker/distribution/reference"
 	"github.com/gorilla/mux"
 )
@@ -127,10 +126,18 @@ func (ub *URLBuilder) BuildTagsURL(name reference.Named) (string, error) {
 
 // BuildManifestURL constructs a url for the manifest identified by name and
 // reference. The argument reference may be either a tag or digest.
-func (ub *URLBuilder) BuildManifestURL(name reference.Named, reference string) (string, error) {
+func (ub *URLBuilder) BuildManifestURL(ref reference.Named) (string, error) {
 	route := ub.cloneRoute(RouteNameManifest)
 
-	manifestURL, err := route.URL("name", name.Name(), "reference", reference)
+	tagOrDigest := ""
+	switch v := ref.(type) {
+	case reference.Tagged:
+		tagOrDigest = v.Tag()
+	case reference.Digested:
+		tagOrDigest = v.Digest().String()
+	}
+
+	manifestURL, err := route.URL("name", ref.Name(), "reference", tagOrDigest)
 	if err != nil {
 		return "", err
 	}
@@ -139,10 +146,10 @@ func (ub *URLBuilder) BuildManifestURL(name reference.Named, reference string) (
 }
 
 // BuildBlobURL constructs the url for the blob identified by name and dgst.
-func (ub *URLBuilder) BuildBlobURL(name reference.Named, dgst digest.Digest) (string, error) {
+func (ub *URLBuilder) BuildBlobURL(ref reference.Canonical) (string, error) {
 	route := ub.cloneRoute(RouteNameBlob)
 
-	layerURL, err := route.URL("name", name.Name(), "digest", dgst.String())
+	layerURL, err := route.URL("name", ref.Name(), "digest", ref.Digest().String())
 	if err != nil {
 		return "", err
 	}

docs/api/v2/urls_test.go

@@ -33,14 +33,16 @@ func makeURLBuilderTestCases(urlBuilder *URLBuilder) []urlBuilderTestCase {
 			description:  "test manifest url",
 			expectedPath: "/v2/foo/bar/manifests/tag",
 			build: func() (string, error) {
-				return urlBuilder.BuildManifestURL(fooBarRef, "tag")
+				ref, _ := reference.WithTag(fooBarRef, "tag")
+				return urlBuilder.BuildManifestURL(ref)
 			},
 		},
 		{
 			description:  "build blob url",
 			expectedPath: "/v2/foo/bar/blobs/sha256:3b3692957d439ac1928219a83fac91e7bf96c153725526874673ae1f2023f8d5",
 			build: func() (string, error) {
-				return urlBuilder.BuildBlobURL(fooBarRef, "sha256:3b3692957d439ac1928219a83fac91e7bf96c153725526874673ae1f2023f8d5")
+				ref, _ := reference.WithDigest(fooBarRef, "sha256:3b3692957d439ac1928219a83fac91e7bf96c153725526874673ae1f2023f8d5")
+				return urlBuilder.BuildBlobURL(ref)
 			},
 		},
 		{

docs/client/repository.go

@@ -249,7 +249,11 @@ func descriptorFromResponse(response *http.Response) (distribution.Descriptor, e
 // to construct a descriptor for the tag.  If the registry doesn't support HEADing
 // a manifest, fallback to GET.
 func (t *tags) Get(ctx context.Context, tag string) (distribution.Descriptor, error) {
-	u, err := t.ub.BuildManifestURL(t.name, tag)
+	ref, err := reference.WithTag(t.name, tag)
+	if err != nil {
+		return distribution.Descriptor{}, err
+	}
+	u, err := t.ub.BuildManifestURL(ref)
 	if err != nil {
 		return distribution.Descriptor{}, err
 	}
@@ -296,7 +300,11 @@ type manifests struct {
 }
 
 func (ms *manifests) Exists(ctx context.Context, dgst digest.Digest) (bool, error) {
-	u, err := ms.ub.BuildManifestURL(ms.name, dgst.String())
+	ref, err := reference.WithDigest(ms.name, dgst)
+	if err != nil {
+		return false, err
+	}
+	u, err := ms.ub.BuildManifestURL(ref)
 	if err != nil {
 		return false, err
 	}
@@ -333,11 +341,19 @@ func (o etagOption) Apply(ms distribution.ManifestService) error {
 }
 
 func (ms *manifests) Get(ctx context.Context, dgst digest.Digest, options ...distribution.ManifestServiceOption) (distribution.Manifest, error) {
+	var (
+		digestOrTag string
+		ref         reference.Named
+		err         error
+	)
 
-	var tag string
 	for _, option := range options {
 		if opt, ok := option.(withTagOption); ok {
-			tag = opt.tag
+			digestOrTag = opt.tag
+			ref, err = reference.WithTag(ms.name, opt.tag)
+			if err != nil {
+				return nil, err
+			}
 		} else {
 			err := option.Apply(ms)
 			if err != nil {
@@ -346,14 +362,15 @@ func (ms *manifests) Get(ctx context.Context, dgst digest.Digest, options ...dis
 		}
 	}
 
-	var ref string
+	if digestOrTag == "" {
-	if tag != "" {
+		digestOrTag = dgst.String()
-		ref = tag
+		ref, err = reference.WithDigest(ms.name, dgst)
-	} else {
+		if err != nil {
-		ref = dgst.String()
+			return nil, err
+		}
 	}
 
-	u, err := ms.ub.BuildManifestURL(ms.name, ref)
+	u, err := ms.ub.BuildManifestURL(ref)
 	if err != nil {
 		return nil, err
 	}
@@ -367,8 +384,8 @@ func (ms *manifests) Get(ctx context.Context, dgst digest.Digest, options ...dis
 		req.Header.Add("Accept", t)
 	}
 
-	if _, ok := ms.etags[ref]; ok {
+	if _, ok := ms.etags[digestOrTag]; ok {
-		req.Header.Set("If-None-Match", ms.etags[ref])
+		req.Header.Set("If-None-Match", ms.etags[digestOrTag])
 	}
 
 	resp, err := ms.client.Do(req)
@@ -412,11 +429,15 @@ func (o withTagOption) Apply(m distribution.ManifestService) error {
 // Put puts a manifest.  A tag can be specified using an options parameter which uses some shared state to hold the
 // tag name in order to build the correct upload URL.  This state is written and read under a lock.
 func (ms *manifests) Put(ctx context.Context, m distribution.Manifest, options ...distribution.ManifestServiceOption) (digest.Digest, error) {
-	var tag string
+	ref := ms.name
 
 	for _, option := range options {
 		if opt, ok := option.(withTagOption); ok {
-			tag = opt.tag
+			var err error
+			ref, err = reference.WithTag(ref, opt.tag)
+			if err != nil {
+				return "", err
+			}
 		} else {
 			err := option.Apply(ms)
 			if err != nil {
@@ -425,7 +446,7 @@ func (ms *manifests) Put(ctx context.Context, m distribution.Manifest, options .
 		}
 	}
 
-	manifestURL, err := ms.ub.BuildManifestURL(ms.name, tag)
+	manifestURL, err := ms.ub.BuildManifestURL(ref)
 	if err != nil {
 		return "", err
 	}
@@ -462,7 +483,11 @@ func (ms *manifests) Put(ctx context.Context, m distribution.Manifest, options .
 }
 
 func (ms *manifests) Delete(ctx context.Context, dgst digest.Digest) error {
-	u, err := ms.ub.BuildManifestURL(ms.name, dgst.String())
+	ref, err := reference.WithDigest(ms.name, dgst)
+	if err != nil {
+		return err
+	}
+	u, err := ms.ub.BuildManifestURL(ref)
 	if err != nil {
 		return err
 	}
@@ -527,7 +552,11 @@ func (bs *blobs) Get(ctx context.Context, dgst digest.Digest) ([]byte, error) {
 }
 
 func (bs *blobs) Open(ctx context.Context, dgst digest.Digest) (distribution.ReadSeekCloser, error) {
-	blobURL, err := bs.ub.BuildBlobURL(bs.name, dgst)
+	ref, err := reference.WithDigest(bs.name, dgst)
+	if err != nil {
+		return nil, err
+	}
+	blobURL, err := bs.ub.BuildBlobURL(ref)
 	if err != nil {
 		return nil, err
 	}
@@ -668,7 +697,11 @@ type blobStatter struct {
 }
 
 func (bs *blobStatter) Stat(ctx context.Context, dgst digest.Digest) (distribution.Descriptor, error) {
-	u, err := bs.ub.BuildBlobURL(bs.name, dgst)
+	ref, err := reference.WithDigest(bs.name, dgst)
+	if err != nil {
+		return distribution.Descriptor{}, err
+	}
+	u, err := bs.ub.BuildBlobURL(ref)
 	if err != nil {
 		return distribution.Descriptor{}, err
 	}
@@ -716,7 +749,11 @@ func buildCatalogValues(maxEntries int, last string) url.Values {
 }
 
 func (bs *blobStatter) Clear(ctx context.Context, dgst digest.Digest) error {
-	blobURL, err := bs.ub.BuildBlobURL(bs.name, dgst)
+	ref, err := reference.WithDigest(bs.name, dgst)
+	if err != nil {
+		return err
+	}
+	blobURL, err := bs.ub.BuildBlobURL(ref)
 	if err != nil {
 		return err
 	}

docs/handlers/api_test.go

@@ -301,7 +301,8 @@ func TestBlobDeleteDisabled(t *testing.T) {
 
 	imageName := args.imageName
 	layerDigest := args.layerDigest
-	layerURL, err := env.builder.BuildBlobURL(imageName, layerDigest)
+	ref, _ := reference.WithDigest(imageName, layerDigest)
+	layerURL, err := env.builder.BuildBlobURL(ref)
 	if err != nil {
 		t.Fatalf("error building url: %v", err)
 	}
@@ -324,7 +325,8 @@ func testBlobAPI(t *testing.T, env *testEnv, args blobArgs) *testEnv {
 
 	// -----------------------------------
 	// Test fetch for non-existent content
-	layerURL, err := env.builder.BuildBlobURL(imageName, layerDigest)
+	ref, _ := reference.WithDigest(imageName, layerDigest)
+	layerURL, err := env.builder.BuildBlobURL(ref)
 	if err != nil {
 		t.Fatalf("error building url: %v", err)
 	}
@@ -534,7 +536,8 @@ func testBlobDelete(t *testing.T, env *testEnv, args blobArgs) {
 	layerFile := args.layerFile
 	layerDigest := args.layerDigest
 
-	layerURL, err := env.builder.BuildBlobURL(imageName, layerDigest)
+	ref, _ := reference.WithDigest(imageName, layerDigest)
+	layerURL, err := env.builder.BuildBlobURL(ref)
 	if err != nil {
 		t.Fatalf(err.Error())
 	}
@@ -617,7 +620,8 @@ func TestDeleteDisabled(t *testing.T) {
 		t.Fatalf("error creating random layer file: %v", err)
 	}
 
-	layerURL, err := env.builder.BuildBlobURL(imageName, layerDigest)
+	ref, _ := reference.WithDigest(imageName, layerDigest)
+	layerURL, err := env.builder.BuildBlobURL(ref)
 	if err != nil {
 		t.Fatalf("Error building blob URL")
 	}
@@ -642,7 +646,8 @@ func TestDeleteReadOnly(t *testing.T) {
 		t.Fatalf("error creating random layer file: %v", err)
 	}
 
-	layerURL, err := env.builder.BuildBlobURL(imageName, layerDigest)
+	ref, _ := reference.WithDigest(imageName, layerDigest)
+	layerURL, err := env.builder.BuildBlobURL(ref)
 	if err != nil {
 		t.Fatalf("Error building blob URL")
 	}
@@ -737,7 +742,8 @@ func TestManifestDeleteDisabled(t *testing.T) {
 }
 
 func testManifestDeleteDisabled(t *testing.T, env *testEnv, imageName reference.Named) {
-	manifestURL, err := env.builder.BuildManifestURL(imageName, digest.DigestSha256EmptyTar)
+	ref, _ := reference.WithDigest(imageName, digest.DigestSha256EmptyTar)
+	manifestURL, err := env.builder.BuildManifestURL(ref)
 	if err != nil {
 		t.Fatalf("unexpected error getting manifest url: %v", err)
 	}
@@ -755,7 +761,8 @@ func testManifestAPISchema1(t *testing.T, env *testEnv, imageName reference.Name
 	tag := "thetag"
 	args := manifestArgs{imageName: imageName}
 
-	manifestURL, err := env.builder.BuildManifestURL(imageName, tag)
+	tagRef, _ := reference.WithTag(imageName, tag)
+	manifestURL, err := env.builder.BuildManifestURL(tagRef)
 	if err != nil {
 		t.Fatalf("unexpected error getting manifest url: %v", err)
 	}
@@ -879,7 +886,8 @@ func testManifestAPISchema1(t *testing.T, env *testEnv, imageName reference.Name
 	args.manifest = signedManifest
 	args.dgst = dgst
 
-	manifestDigestURL, err := env.builder.BuildManifestURL(imageName, dgst.String())
+	digestRef, _ := reference.WithDigest(imageName, dgst)
+	manifestDigestURL, err := env.builder.BuildManifestURL(digestRef)
 	checkErr(t, err, "building manifest url")
 
 	resp = putManifest(t, "putting signed manifest no error", manifestURL, "", signedManifest)
@@ -1075,7 +1083,8 @@ func testManifestAPISchema2(t *testing.T, env *testEnv, imageName reference.Name
 		mediaType: schema2.MediaTypeManifest,
 	}
 
-	manifestURL, err := env.builder.BuildManifestURL(imageName, tag)
+	tagRef, _ := reference.WithTag(imageName, tag)
+	manifestURL, err := env.builder.BuildManifestURL(tagRef)
 	if err != nil {
 		t.Fatalf("unexpected error getting manifest url: %v", err)
 	}
@@ -1219,7 +1228,8 @@ func testManifestAPISchema2(t *testing.T, env *testEnv, imageName reference.Name
 	args.dgst = dgst
 	args.manifest = deserializedManifest
 
-	manifestDigestURL, err := env.builder.BuildManifestURL(imageName, dgst.String())
+	digestRef, _ := reference.WithDigest(imageName, dgst)
+	manifestDigestURL, err := env.builder.BuildManifestURL(digestRef)
 	checkErr(t, err, "building manifest url")
 
 	resp = putManifest(t, "putting manifest no error", manifestURL, schema2.MediaTypeManifest, manifest)
@@ -1415,7 +1425,8 @@ func testManifestAPIManifestList(t *testing.T, env *testEnv, args manifestArgs)
 	imageName := args.imageName
 	tag := "manifestlisttag"
 
-	manifestURL, err := env.builder.BuildManifestURL(imageName, tag)
+	tagRef, _ := reference.WithTag(imageName, tag)
+	manifestURL, err := env.builder.BuildManifestURL(tagRef)
 	if err != nil {
 		t.Fatalf("unexpected error getting manifest url: %v", err)
 	}
@@ -1468,7 +1479,8 @@ func testManifestAPIManifestList(t *testing.T, env *testEnv, args manifestArgs)
 	}
 	dgst := digest.FromBytes(canonical)
 
-	manifestDigestURL, err := env.builder.BuildManifestURL(imageName, dgst.String())
+	digestRef, _ := reference.WithDigest(imageName, dgst)
+	manifestDigestURL, err := env.builder.BuildManifestURL(digestRef)
 	checkErr(t, err, "building manifest url")
 
 	resp = putManifest(t, "putting manifest list no error", manifestURL, manifestlist.MediaTypeManifestList, deserializedManifestList)
@@ -1637,8 +1649,9 @@ func testManifestDelete(t *testing.T, env *testEnv, args manifestArgs) {
 	imageName := args.imageName
 	dgst := args.dgst
 	manifest := args.manifest
-	manifestDigestURL, err := env.builder.BuildManifestURL(imageName, dgst.String())
 
+	ref, _ := reference.WithDigest(imageName, dgst)
+	manifestDigestURL, err := env.builder.BuildManifestURL(ref)
 	// ---------------
 	// Delete by digest
 	resp, err := httpDelete(manifestDigestURL)
@@ -1686,8 +1699,9 @@ func testManifestDelete(t *testing.T, env *testEnv, args manifestArgs) {
 
 	// ---------------
 	// Attempt to delete an unknown manifest
-	unknownDigest := "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+	unknownDigest := digest.Digest("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")
-	unknownManifestDigestURL, err := env.builder.BuildManifestURL(imageName, unknownDigest)
+	unknownRef, _ := reference.WithDigest(imageName, unknownDigest)
+	unknownManifestDigestURL, err := env.builder.BuildManifestURL(unknownRef)
 	checkErr(t, err, "building unknown manifest url")
 
 	resp, err = httpDelete(unknownManifestDigestURL)
@@ -1695,11 +1709,12 @@ func testManifestDelete(t *testing.T, env *testEnv, args manifestArgs) {
 	checkResponse(t, "fetching deleted manifest", resp, http.StatusNotFound)
 
 	// --------------------
-	// Uupload manifest by tag
+	// Upload manifest by tag
 	tag := "atag"
-	manifestTagURL, err := env.builder.BuildManifestURL(imageName, tag)
+	tagRef, _ := reference.WithTag(imageName, tag)
-	resp = putManifest(t, "putting signed manifest by tag", manifestTagURL, args.mediaType, manifest)
+	manifestTagURL, err := env.builder.BuildManifestURL(tagRef)
-	checkResponse(t, "putting signed manifest by tag", resp, http.StatusCreated)
+	resp = putManifest(t, "putting manifest by tag", manifestTagURL, args.mediaType, manifest)
+	checkResponse(t, "putting manifest by tag", resp, http.StatusCreated)
 	checkHeaders(t, resp, http.Header{
 		"Location":              []string{manifestDigestURL},
 		"Docker-Content-Digest": []string{dgst.String()},
@@ -1943,7 +1958,8 @@ func pushLayer(t *testing.T, ub *v2.URLBuilder, name reference.Named, dgst diges
 
 	sha256Dgst := digester.Digest()
 
-	expectedLayerURL, err := ub.BuildBlobURL(name, sha256Dgst)
+	ref, _ := reference.WithDigest(name, sha256Dgst)
+	expectedLayerURL, err := ub.BuildBlobURL(ref)
 	if err != nil {
 		t.Fatalf("error building expected layer url: %v", err)
 	}
@@ -1966,7 +1982,8 @@ func finishUpload(t *testing.T, ub *v2.URLBuilder, name reference.Named, uploadU
 
 	checkResponse(t, "putting monolithic chunk", resp, http.StatusCreated)
 
-	expectedLayerURL, err := ub.BuildBlobURL(name, dgst)
+	ref, _ := reference.WithDigest(name, dgst)
+	expectedLayerURL, err := ub.BuildBlobURL(ref)
 	if err != nil {
 		t.Fatalf("error building expected layer url: %v", err)
 	}
@@ -2189,10 +2206,12 @@ func createRepository(env *testEnv, t *testing.T, imageName string, tag string)
 	dgst := digest.FromBytes(signedManifest.Canonical)
 
 	// Create this repository by tag to ensure the tag mapping is made in the registry
-	manifestDigestURL, err := env.builder.BuildManifestURL(imageNameRef, tag)
+	tagRef, _ := reference.WithTag(imageNameRef, tag)
+	manifestDigestURL, err := env.builder.BuildManifestURL(tagRef)
 	checkErr(t, err, "building manifest url")
 
-	location, err := env.builder.BuildManifestURL(imageNameRef, dgst.String())
+	digestRef, _ := reference.WithDigest(imageNameRef, dgst)
+	location, err := env.builder.BuildManifestURL(digestRef)
 	checkErr(t, err, "building location URL")
 
 	resp := putManifest(t, "putting signed manifest", manifestDigestURL, "", signedManifest)
@@ -2212,7 +2231,8 @@ func TestRegistryAsCacheMutationAPIs(t *testing.T) {
 
 	imageName, _ := reference.ParseNamed("foo/bar")
 	tag := "latest"
-	manifestURL, err := env.builder.BuildManifestURL(imageName, tag)
+	tagRef, _ := reference.WithTag(imageName, tag)
+	manifestURL, err := env.builder.BuildManifestURL(tagRef)
 	if err != nil {
 		t.Fatalf("unexpected error building base url: %v", err)
 	}
@@ -2255,7 +2275,8 @@ func TestRegistryAsCacheMutationAPIs(t *testing.T) {
 	checkResponse(t, fmt.Sprintf("starting layer push to cache %v", imageName), resp, errcode.ErrorCodeUnsupported.Descriptor().HTTPStatusCode)
 
 	// Blob Delete
-	blobURL, err := env.builder.BuildBlobURL(imageName, digest.DigestSha256EmptyTar)
+	ref, _ := reference.WithDigest(imageName, digest.DigestSha256EmptyTar)
+	blobURL, err := env.builder.BuildBlobURL(ref)
 	resp, err = httpDelete(blobURL)
 	checkResponse(t, "deleting blob from cache", resp, errcode.ErrorCodeUnsupported.Descriptor().HTTPStatusCode)
 
@@ -2316,14 +2337,16 @@ func TestProxyManifestGetByTag(t *testing.T) {
 
 	proxyEnv := newTestEnvWithConfig(t, &proxyConfig)
 
-	manifestDigestURL, err := proxyEnv.builder.BuildManifestURL(imageName, dgst.String())
+	digestRef, _ := reference.WithDigest(imageName, dgst)
+	manifestDigestURL, err := proxyEnv.builder.BuildManifestURL(digestRef)
 	checkErr(t, err, "building manifest url")
 
 	resp, err := http.Get(manifestDigestURL)
 	checkErr(t, err, "fetching manifest from proxy by digest")
 	defer resp.Body.Close()
 
-	manifestTagURL, err := proxyEnv.builder.BuildManifestURL(imageName, tag)
+	tagRef, _ := reference.WithTag(imageName, tag)
+	manifestTagURL, err := proxyEnv.builder.BuildManifestURL(tagRef)
 	checkErr(t, err, "building manifest url")
 
 	resp, err = http.Get(manifestTagURL)

docs/handlers/blobupload.go

@@ -372,7 +372,11 @@ func (buh *blobUploadHandler) createBlobMountOption(fromRepo, mountDigest string
 // created blob. A 201 Created is written as well as the canonical URL and
 // blob digest.
 func (buh *blobUploadHandler) writeBlobCreatedHeaders(w http.ResponseWriter, desc distribution.Descriptor) error {
-	blobURL, err := buh.urlBuilder.BuildBlobURL(buh.Repository.Name(), desc.Digest)
+	ref, err := reference.WithDigest(buh.Repository.Name(), desc.Digest)
+	if err != nil {
+		return err
+	}
+	blobURL, err := buh.urlBuilder.BuildBlobURL(ref)
 	if err != nil {
 		return err
 	}

docs/handlers/images.go

@@ -289,7 +289,13 @@ func (imh *imageManifestHandler) PutImageManifest(w http.ResponseWriter, r *http
 	}
 
 	// Construct a canonical url for the uploaded manifest.
-	location, err := imh.urlBuilder.BuildManifestURL(imh.Repository.Name(), imh.Digest.String())
+	ref, err := reference.WithDigest(imh.Repository.Name(), imh.Digest)
+	if err != nil {
+		imh.Errors = append(imh.Errors, errcode.ErrorCodeUnknown.WithDetail(err))
+		return
+	}
+
+	location, err := imh.urlBuilder.BuildManifestURL(ref)
 	if err != nil {
 		// NOTE(stevvooe): Given the behavior above, this absurdly unlikely to
 		// happen. We'll log the error here but proceed as if it worked. Worst