From 6149a8c6343f01352876e2c91cc0281547abc823 Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Tue, 15 Dec 2015 16:43:13 -0800 Subject: [PATCH] Change URLBuilder methods to use references for tags and digests Signed-off-by: Aaron Lehmann --- docs/api/v2/urls.go | 17 +++++--- docs/api/v2/urls_test.go | 6 ++- docs/client/repository.go | 75 +++++++++++++++++++++++++++--------- docs/handlers/api_test.go | 77 ++++++++++++++++++++++++------------- docs/handlers/blobupload.go | 6 ++- docs/handlers/images.go | 8 +++- 6 files changed, 134 insertions(+), 55 deletions(-) diff --git a/docs/api/v2/urls.go b/docs/api/v2/urls.go index 5b63ccaa..408c7b74 100644 --- a/docs/api/v2/urls.go +++ b/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 } diff --git a/docs/api/v2/urls_test.go b/docs/api/v2/urls_test.go index 7dab00fc..1af1f261 100644 --- a/docs/api/v2/urls_test.go +++ b/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) }, }, { diff --git a/docs/client/repository.go b/docs/client/repository.go index 43826907..1f777add 100644 --- a/docs/client/repository.go +++ b/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 tag != "" { - ref = tag - } else { - ref = dgst.String() + if digestOrTag == "" { + digestOrTag = dgst.String() + ref, err = reference.WithDigest(ms.name, dgst) + if err != nil { + 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 { - req.Header.Set("If-None-Match", ms.etags[ref]) + if _, ok := ms.etags[digestOrTag]; ok { + 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 } diff --git a/docs/handlers/api_test.go b/docs/handlers/api_test.go index b59db6cc..5fffaa5a 100644 --- a/docs/handlers/api_test.go +++ b/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" - unknownManifestDigestURL, err := env.builder.BuildManifestURL(imageName, unknownDigest) + unknownDigest := digest.Digest("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") + 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) - resp = putManifest(t, "putting signed manifest by tag", manifestTagURL, args.mediaType, manifest) - checkResponse(t, "putting signed manifest by tag", resp, http.StatusCreated) + tagRef, _ := reference.WithTag(imageName, tag) + manifestTagURL, err := env.builder.BuildManifestURL(tagRef) + 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) diff --git a/docs/handlers/blobupload.go b/docs/handlers/blobupload.go index 56403dd9..a42e57f6 100644 --- a/docs/handlers/blobupload.go +++ b/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 } diff --git a/docs/handlers/images.go b/docs/handlers/images.go index 9b4e5399..808ead54 100644 --- a/docs/handlers/images.go +++ b/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