From c9aaff00f89c8b67330fa80a7d7fc89162f3540b Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Wed, 12 Oct 2016 17:20:27 -0700 Subject: [PATCH] manifest: references should cover all children To allow generic manifest walking, we define an interface method of `References` that returns the referenced items in the manifest. The current implementation does not return the config target from schema2, making this useless for most applications. The garbage collector has been modified to show the utility of this correctly formed `References` method. We may be able to make more generic traversal methods with this, as well. Signed-off-by: Stephen J Day --- manifest/schema1/config_builder.go | 7 +- manifest/schema2/builder_test.go | 4 +- manifest/schema2/manifest.go | 5 +- manifest/schema2/manifest_test.go | 14 ++- manifests.go | 12 ++- registry/handlers/images.go | 2 +- registry/storage/garbagecollect.go | 9 -- registry/storage/schema2manifesthandler.go | 88 +++++++++++-------- .../storage/schema2manifesthandler_test.go | 3 +- 9 files changed, 81 insertions(+), 63 deletions(-) diff --git a/manifest/schema1/config_builder.go b/manifest/schema1/config_builder.go index 5cdd76796..be0123731 100644 --- a/manifest/schema1/config_builder.go +++ b/manifest/schema1/config_builder.go @@ -9,11 +9,10 @@ import ( "github.com/docker/distribution" "github.com/docker/distribution/context" - "github.com/docker/distribution/reference" - "github.com/docker/libtrust" - "github.com/docker/distribution/digest" "github.com/docker/distribution/manifest" + "github.com/docker/distribution/reference" + "github.com/docker/libtrust" ) type diffID digest.Digest @@ -95,7 +94,7 @@ func (mb *configManifestBuilder) Build(ctx context.Context) (m distribution.Mani } if len(img.RootFS.DiffIDs) != len(mb.descriptors) { - return nil, errors.New("number of descriptors and number of layers in rootfs must match") + return nil, fmt.Errorf("number of descriptors and number of layers in rootfs must match: len(%v) != len(%v)", img.RootFS.DiffIDs, mb.descriptors) } // Generate IDs for each layer diff --git a/manifest/schema2/builder_test.go b/manifest/schema2/builder_test.go index 02ed401bf..851f917cb 100644 --- a/manifest/schema2/builder_test.go +++ b/manifest/schema2/builder_test.go @@ -203,8 +203,8 @@ func TestBuilder(t *testing.T) { } references := manifest.References() - - if !reflect.DeepEqual(references, descriptors) { + expected := append([]distribution.Descriptor{manifest.Target()}, descriptors...) + if !reflect.DeepEqual(references, expected) { t.Fatal("References() does not match the descriptors added") } } diff --git a/manifest/schema2/manifest.go b/manifest/schema2/manifest.go index dd2ed114c..1a1282709 100644 --- a/manifest/schema2/manifest.go +++ b/manifest/schema2/manifest.go @@ -69,7 +69,10 @@ type Manifest struct { // References returnes the descriptors of this manifests references. func (m Manifest) References() []distribution.Descriptor { - return m.Layers + references := make([]distribution.Descriptor, 0, 1+len(m.Layers)) + references = append(references, m.Config) + references = append(references, m.Layers...) + return references } // Target returns the target of this signed manifest. diff --git a/manifest/schema2/manifest_test.go b/manifest/schema2/manifest_test.go index 459d614cd..f0003d18b 100644 --- a/manifest/schema2/manifest_test.go +++ b/manifest/schema2/manifest_test.go @@ -90,16 +90,22 @@ func TestManifest(t *testing.T) { } references := deserialized.References() - if len(references) != 1 { + if len(references) != 2 { t.Fatalf("unexpected number of references: %d", len(references)) } - if references[0].Digest != "sha256:62d8908bee94c202b2d35224a221aaa2058318bfa9879fa541efaecba272331b" { + + if !reflect.DeepEqual(references[0], target) { + t.Fatalf("first reference should be target: %v != %v", references[0], target) + } + + // Test the second reference + if references[1].Digest != "sha256:62d8908bee94c202b2d35224a221aaa2058318bfa9879fa541efaecba272331b" { t.Fatalf("unexpected digest in reference: %s", references[0].Digest.String()) } - if references[0].MediaType != MediaTypeLayer { + if references[1].MediaType != MediaTypeLayer { t.Fatalf("unexpected media type in reference: %s", references[0].MediaType) } - if references[0].Size != 153263 { + if references[1].Size != 153263 { t.Fatalf("unexpected size in reference: %d", references[0].Size) } } diff --git a/manifests.go b/manifests.go index 2ac7c8f21..c4fb63450 100644 --- a/manifests.go +++ b/manifests.go @@ -12,8 +12,13 @@ import ( // references and an optional target type Manifest interface { // References returns a list of objects which make up this manifest. - // The references are strictly ordered from base to head. A reference - // is anything which can be represented by a distribution.Descriptor + // A reference is anything which can be represented by a + // distribution.Descriptor. These can consist of layers, resources or other + // manifests. + // + // While no particular order is required, implementations should return + // them from highest to lowest priority. For example, one might want to + // return the base layer before the top layer. References() []Descriptor // Payload provides the serialized format of the manifest, in addition to @@ -36,6 +41,9 @@ type ManifestBuilder interface { // AppendReference includes the given object in the manifest after any // existing dependencies. If the add fails, such as when adding an // unsupported dependency, an error may be returned. + // + // The destination of the reference is dependent on the manifest type and + // the dependency type. AppendReference(dependency Describable) error } diff --git a/registry/handlers/images.go b/registry/handlers/images.go index df7f869be..96316348c 100644 --- a/registry/handlers/images.go +++ b/registry/handlers/images.go @@ -205,7 +205,7 @@ func (imh *imageManifestHandler) convertSchema2Manifest(schema2Manifest *schema2 } builder := schema1.NewConfigManifestBuilder(imh.Repository.Blobs(imh), imh.Context.App.trustKey, ref, configJSON) - for _, d := range schema2Manifest.References() { + for _, d := range schema2Manifest.Layers { if err := builder.AppendReference(d); err != nil { imh.Errors = append(imh.Errors, v2.ErrorCodeManifestInvalid.WithDetail(err)) return nil, err diff --git a/registry/storage/garbagecollect.go b/registry/storage/garbagecollect.go index fe3bd9b92..e982c994c 100644 --- a/registry/storage/garbagecollect.go +++ b/registry/storage/garbagecollect.go @@ -6,7 +6,6 @@ import ( "github.com/docker/distribution" "github.com/docker/distribution/context" "github.com/docker/distribution/digest" - "github.com/docker/distribution/manifest/schema2" "github.com/docker/distribution/reference" "github.com/docker/distribution/registry/storage/driver" ) @@ -63,14 +62,6 @@ func MarkAndSweep(ctx context.Context, storageDriver driver.StorageDriver, regis emit("%s: marking blob %s", repoName, descriptor.Digest) } - switch manifest.(type) { - case *schema2.DeserializedManifest: - config := manifest.(*schema2.DeserializedManifest).Config - emit("%s: marking configuration %s", repoName, config.Digest) - markSet[config.Digest] = struct{}{} - break - } - return nil }) diff --git a/registry/storage/schema2manifesthandler.go b/registry/storage/schema2manifesthandler.go index 1d221410e..9fe71bb4d 100644 --- a/registry/storage/schema2manifesthandler.go +++ b/registry/storage/schema2manifesthandler.go @@ -9,6 +9,7 @@ import ( "github.com/docker/distribution" "github.com/docker/distribution/context" "github.com/docker/distribution/digest" + "github.com/docker/distribution/manifest/schema1" "github.com/docker/distribution/manifest/schema2" ) @@ -71,53 +72,62 @@ func (ms *schema2ManifestHandler) Put(ctx context.Context, manifest distribution func (ms *schema2ManifestHandler) verifyManifest(ctx context.Context, mnfst schema2.DeserializedManifest, skipDependencyVerification bool) error { var errs distribution.ErrManifestVerification - if !skipDependencyVerification { - target := mnfst.Target() - _, err := ms.repository.Blobs(ctx).Stat(ctx, target.Digest) + if skipDependencyVerification { + return nil + } + + manifestService, err := ms.repository.Manifests(ctx) + if err != nil { + return err + } + + blobsService := ms.repository.Blobs(ctx) + + for _, descriptor := range mnfst.References() { + var err error + + switch descriptor.MediaType { + case schema2.MediaTypeForeignLayer: + // Clients download this layer from an external URL, so do not check for + // its presense. + if len(descriptor.URLs) == 0 { + err = errMissingURL + } + allow := ms.manifestURLs.allow + deny := ms.manifestURLs.deny + for _, u := range descriptor.URLs { + var pu *url.URL + pu, err = url.Parse(u) + if err != nil || (pu.Scheme != "http" && pu.Scheme != "https") || pu.Fragment != "" || (allow != nil && !allow.MatchString(u)) || (deny != nil && deny.MatchString(u)) { + err = errInvalidURL + break + } + } + case schema2.MediaTypeManifest, schema1.MediaTypeManifest: + var exists bool + exists, err = manifestService.Exists(ctx, descriptor.Digest) + if err != nil || !exists { + err = distribution.ErrBlobUnknown // just coerce to unknown. + } + + fallthrough // double check the blob store. + default: + // forward all else to blob storage + if len(descriptor.URLs) == 0 { + _, err = blobsService.Stat(ctx, descriptor.Digest) + } + } + if err != nil { if err != distribution.ErrBlobUnknown { errs = append(errs, err) } // On error here, we always append unknown blob errors. - errs = append(errs, distribution.ErrManifestBlobUnknown{Digest: target.Digest}) - } - - for _, fsLayer := range mnfst.References() { - var err error - if fsLayer.MediaType != schema2.MediaTypeForeignLayer { - if len(fsLayer.URLs) == 0 { - _, err = ms.repository.Blobs(ctx).Stat(ctx, fsLayer.Digest) - } else { - err = errUnexpectedURL - } - } else { - // Clients download this layer from an external URL, so do not check for - // its presense. - if len(fsLayer.URLs) == 0 { - err = errMissingURL - } - allow := ms.manifestURLs.allow - deny := ms.manifestURLs.deny - for _, u := range fsLayer.URLs { - var pu *url.URL - pu, err = url.Parse(u) - if err != nil || (pu.Scheme != "http" && pu.Scheme != "https") || pu.Fragment != "" || (allow != nil && !allow.MatchString(u)) || (deny != nil && deny.MatchString(u)) { - err = errInvalidURL - break - } - } - } - if err != nil { - if err != distribution.ErrBlobUnknown { - errs = append(errs, err) - } - - // On error here, we always append unknown blob errors. - errs = append(errs, distribution.ErrManifestBlobUnknown{Digest: fsLayer.Digest}) - } + errs = append(errs, distribution.ErrManifestBlobUnknown{Digest: descriptor.Digest}) } } + if len(errs) != 0 { return errs } diff --git a/registry/storage/schema2manifesthandler_test.go b/registry/storage/schema2manifesthandler_test.go index 73a7e336a..5051fa3de 100644 --- a/registry/storage/schema2manifesthandler_test.go +++ b/registry/storage/schema2manifesthandler_test.go @@ -57,9 +57,10 @@ func TestVerifyManifestForeignLayer(t *testing.T) { errMissingURL, }, { + // regular layers may have foreign urls layer, []string{"http://foo/bar"}, - errUnexpectedURL, + nil, }, { foreignLayer,