Merge pull request #1999 from stevvooe/fold-target-references

manifest: references should cover all children

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

manifest/schema2/builder_test.go

@@ -203,8 +203,8 @@ func TestBuilder(t *testing.T) {
 	}
 
 	references := manifest.References()
-
+	expected := append([]distribution.Descriptor{manifest.Target()}, descriptors...)
-	if !reflect.DeepEqual(references, descriptors) {
+	if !reflect.DeepEqual(references, expected) {
 		t.Fatal("References() does not match the descriptors added")
 	}
 }

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.

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)
 	}
 }

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
+	// A reference is anything which can be represented by a
-	// is anything which can be represented by a distribution.Descriptor
+	// 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
 }
 

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

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
 		})
 

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 {
+	if skipDependencyVerification {
-		target := mnfst.Target()
+		return nil
-		_, err := ms.repository.Blobs(ctx).Stat(ctx, target.Digest)
+	}
+
+	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})
+			errs = append(errs, distribution.ErrManifestBlobUnknown{Digest: descriptor.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})
-			}
 		}
 	}
+
 	if len(errs) != 0 {
 		return errs
 	}

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,