Fix missing sizes for manifest schema 1 images

Instead of filling metadata on image object in the registry, send manifest and config blobs to the master API and let it do the job. Signed-off-by: Michal Minář <[email protected]>
openshift · Sep 28, 2017 · 5ac2762 · 5ac2762
1 parent 848776c
commit 5ac2762
Show file tree

Hide file tree

Showing 13 changed files with 820 additions and 306 deletions.
diff --git a/pkg/dockerregistry/server/manifesthandler.go b/pkg/dockerregistry/server/manifesthandler.go
@@ -14,11 +14,12 @@ import (
 
 // A ManifestHandler defines a common set of operations on all versions of manifest schema.
 type ManifestHandler interface {
-	// FillImageMetadata fills a given image with metadata parsed from manifest. It also corrects layer sizes
-	// with blob sizes. Newer Docker client versions don't set layer sizes in the manifest schema 1 at all.
-	// Origin master needs correct layer sizes for proper image quota support. That's why we need to fill the
-	// metadata in the registry.
-	FillImageMetadata(ctx context.Context, image *imageapiv1.Image) error
+	// Config returns a blob with image configuration associated with the manifest. This applies only to
+	// manifet schema 2.
+	Config(ctx context.Context) ([]byte, error)
+
+	// Digest returns manifest's digest.
+	Digest() (manifestDigest digest.Digest, err error)
 
 	// Manifest returns a deserialized manifest object.
 	Manifest() distribution.Manifest
@@ -29,9 +30,6 @@ type ManifestHandler interface {
 
 	// Verify returns an error if the contained manifest is not valid or has missing dependencies.
 	Verify(ctx context.Context, skipDependencyVerification bool) error
-
-	// Digest returns manifest's digest
-	Digest() (manifestDigest digest.Digest, err error)
 }
 
 // NewManifestHandler creates a manifest handler for the given manifest.

diff --git a/pkg/dockerregistry/server/manifestschema1handler.go b/pkg/dockerregistry/server/manifestschema1handler.go
@@ -11,11 +11,6 @@ import (
 	"github.com/docker/distribution/manifest/schema1"
 	"github.com/docker/distribution/reference"
 	"github.com/docker/libtrust"
-
-	"k8s.io/apimachinery/pkg/util/sets"
-
-	imageapi "github.com/openshift/origin/pkg/image/apis/image"
-	imageapiv1 "github.com/openshift/origin/pkg/image/apis/image/v1"
 )
 
 func unmarshalManifestSchema1(content []byte, signatures [][]byte) (distribution.Manifest, error) {
@@ -52,57 +47,12 @@ type manifestSchema1Handler struct {
 
 var _ ManifestHandler = &manifestSchema1Handler{}
 
-func (h *manifestSchema1Handler) FillImageMetadata(ctx context.Context, image *imageapiv1.Image) error {
-	signatures, err := h.manifest.Signatures()
-	if err != nil {
-		return err
-	}
-
-	for _, signDigest := range signatures {
-		image.DockerImageSignatures = append(image.DockerImageSignatures, signDigest)
-	}
-
-	refs := h.manifest.References()
-
-	if err := imageMetadataFromManifest(image); err != nil {
-		return fmt.Errorf("unable to fill image %s metadata: %v", image.Name, err)
-	}
-
-	blobSet := sets.NewString()
-	meta, ok := image.DockerImageMetadata.Object.(*imageapi.DockerImage)
-	if !ok {
-		return fmt.Errorf("image %q does not have metadata", image.Name)
-	}
-	meta.Size = int64(0)
-
-	blobs := h.repo.Blobs(ctx)
-	for i := range image.DockerImageLayers {
-		layer := &image.DockerImageLayers[i]
-		// DockerImageLayers represents h.manifest.Manifest.FSLayers in reversed order
-		desc, err := blobs.Stat(ctx, refs[len(image.DockerImageLayers)-i-1].Digest)
-		if err != nil {
-			context.GetLogger(ctx).Errorf("failed to stat blob %s of image %s", layer.Name, image.DockerImageReference)
-			return err
-		}
-		// The MediaType appeared in manifest schema v2. We need to fill it
-		// manually in the old images if it is not already filled.
-		if len(layer.MediaType) == 0 {
-			if len(desc.MediaType) > 0 {
-				layer.MediaType = desc.MediaType
-			} else {
-				layer.MediaType = schema1.MediaTypeManifestLayer
-			}
-		}
-		layer.LayerSize = desc.Size
-		// count empty layer just once (empty layer may actually have non-zero size)
-		if !blobSet.Has(layer.Name) {
-			meta.Size += desc.Size
-			blobSet.Insert(layer.Name)
-		}
-	}
-	image.DockerImageMetadata.Object = meta
+func (h *manifestSchema1Handler) Config(ctx context.Context) ([]byte, error) {
+	return nil, nil
+}
 
-	return nil
+func (h *manifestSchema1Handler) Digest() (digest.Digest, error) {
+	return digest.FromBytes(h.manifest.Canonical), nil
 }
 
 func (h *manifestSchema1Handler) Manifest() distribution.Manifest {
@@ -183,7 +133,3 @@ func (h *manifestSchema1Handler) Verify(ctx context.Context, skipDependencyVerif
 	}
 	return nil
 }
-
-func (h *manifestSchema1Handler) Digest() (digest.Digest, error) {
-	return digest.FromBytes(h.manifest.Canonical), nil
-}
diff --git a/pkg/dockerregistry/server/manifestschema1handler_test.go b/pkg/dockerregistry/server/manifestschema1handler_test.go
diff --git a/pkg/dockerregistry/server/manifestschema2handler.go b/pkg/dockerregistry/server/manifestschema2handler.go
@@ -3,13 +3,13 @@ package server
 import (
 	"encoding/json"
 	"errors"
+	"fmt"
+	"reflect"
 
 	"github.com/docker/distribution"
 	"github.com/docker/distribution/context"
 	"github.com/docker/distribution/digest"
 	"github.com/docker/distribution/manifest/schema2"
-
-	imageapiv1 "github.com/openshift/origin/pkg/image/apis/image/v1"
 )
 
 var (
@@ -23,28 +23,42 @@ func unmarshalManifestSchema2(content []byte) (distribution.Manifest, error) {
 		return nil, err
 	}
 
+	if !reflect.DeepEqual(deserializedManifest.Versioned, schema2.SchemaVersion) {
+		return nil, fmt.Errorf("unexpected manifest schema version=%d, mediaType=%q",
+			deserializedManifest.SchemaVersion,
+			deserializedManifest.MediaType)
+	}
+
 	return &deserializedManifest, nil
 }
 
 type manifestSchema2Handler struct {
-	repo     *repository
-	manifest *schema2.DeserializedManifest
+	repo         *repository
+	manifest     *schema2.DeserializedManifest
+	cachedConfig []byte
 }
 
 var _ ManifestHandler = &manifestSchema2Handler{}
 
-func (h *manifestSchema2Handler) FillImageMetadata(ctx context.Context, image *imageapiv1.Image) error {
-	// The manifest.Config references a configuration object for a container by its digest.
-	// It needs to be fetched in order to fill an image object metadata below.
-	configBytes, err := h.repo.Blobs(ctx).Get(ctx, h.manifest.Config.Digest)
-	if err != nil {
-		context.GetLogger(ctx).Errorf("failed to get image config %s: %v", h.manifest.Config.Digest.String(), err)
-		return err
+func (h *manifestSchema2Handler) Config(ctx context.Context) ([]byte, error) {
+	if h.cachedConfig == nil {
+		blob, err := h.repo.Blobs(ctx).Get(ctx, h.manifest.Config.Digest)
+		if err != nil {
+			context.GetLogger(ctx).Errorf("failed to get manifest config: %v", err)
+			return nil, err
+		}
+		h.cachedConfig = blob
 	}
-	image.DockerImageConfig = string(configBytes)
 
-	// We need to populate the image metadata using the manifest.
-	return imageMetadataFromManifest(image)
+	return h.cachedConfig, nil
+}
+
+func (h *manifestSchema2Handler) Digest() (digest.Digest, error) {
+	_, p, err := h.manifest.Payload()
+	if err != nil {
+		return "", err
+	}
+	return digest.FromBytes(p), nil
 }
 
 func (h *manifestSchema2Handler) Manifest() distribution.Manifest {
@@ -112,11 +126,3 @@ func (h *manifestSchema2Handler) Verify(ctx context.Context, skipDependencyVerif
 	}
 	return nil
 }
-
-func (h *manifestSchema2Handler) Digest() (digest.Digest, error) {
-	_, p, err := h.manifest.Payload()
-	if err != nil {
-		return "", err
-	}
-	return digest.FromBytes(p), nil
-}
diff --git a/pkg/dockerregistry/server/manifestschema2handler_test.go b/pkg/dockerregistry/server/manifestschema2handler_test.go
@@ -0,0 +1,138 @@
+package server
+
+import (
+	"reflect"
+	"strings"
+	"testing"
+
+	"k8s.io/apimachinery/pkg/util/diff"
+
+	"github.com/docker/distribution"
+	"github.com/docker/distribution/digest"
+	"github.com/docker/distribution/manifest/schema2"
+)
+
+func TestUnmarshalManifestSchema2(t *testing.T) {
+	for _, tc := range []struct {
+		name                   string
+		manifestString         string
+		expectedConfig         distribution.Descriptor
+		expectedReferences     []distribution.Descriptor
+		expectedErrorSubstring string
+	}{
+		{
+			name:           "valid nginx image with sizes in manifest",
+			manifestString: manifestSchema2,
+			expectedConfig: manifestSchema2ConfigDescriptor,
+			expectedReferences: []distribution.Descriptor{
+				manifestSchema2ConfigDescriptor,
+				manifestSchema2LayerDescriptors[0],
+				manifestSchema2LayerDescriptors[1],
+				manifestSchema2LayerDescriptors[2],
+			},
+		},
+
+		{
+			name:                   "invalid schema2 image",
+			manifestString:         manifestSchema2Invalid,
+			expectedErrorSubstring: "invalid character",
+		},
+
+		{
+			name:                   "manifest schema1 image",
+			manifestString:         manifestSchema1,
+			expectedErrorSubstring: "unexpected manifest schema version",
+		},
+	} {
+
+		t.Run(tc.name, func(t *testing.T) {
+			manifest, err := unmarshalManifestSchema2([]byte(tc.manifestString))
+			if err != nil {
+				if len(tc.expectedErrorSubstring) == 0 {
+					t.Fatalf("got unexpected error: (%T) %v", err, err)
+				}
+				if !strings.Contains(err.Error(), tc.expectedErrorSubstring) {
+					t.Fatalf("expected error with string %q, instead got: %v", tc.expectedErrorSubstring, err)
+				}
+				return
+			}
+			if err == nil && len(tc.expectedErrorSubstring) > 0 {
+				t.Fatalf("got non-error while expecting: %s", tc.expectedErrorSubstring)
+			}
+
+			dm, ok := manifest.(*schema2.DeserializedManifest)
+			if !ok {
+				t.Fatalf("got unexpected manifest schema: %T", manifest)
+			}
+
+			if !reflect.DeepEqual(dm.Config, tc.expectedConfig) {
+				t.Errorf("got unexpected image config descriptor: %s", diff.ObjectGoPrintDiff(dm.Config, tc.expectedConfig))
+			}
+
+			refs := dm.References()
+			if !reflect.DeepEqual(refs, tc.expectedReferences) {
+				t.Errorf("got unexpected image references: %s", diff.ObjectGoPrintDiff(refs, tc.expectedReferences))
+			}
+		})
+	}
+}
+
+var manifestSchema2LayerDescriptors = []distribution.Descriptor{
+	{
+		MediaType: schema2.MediaTypeLayer,
+		Digest:    digest.Digest("sha256:afeb2bfd31c0760573e7262de6ae67a84da0e0a1c3e8157bbddd41a501b18a5c"),
+		Size:      22488057,
+	},
+	{
+		MediaType: schema2.MediaTypeLayer,
+		Digest:    digest.Digest("sha256:7ff5d10493db2cdfc1b7238434c503cc0664d48d0f7154ea9472e734b28a72dd"),
+		Size:      21869700,
+	},
+	{
+		MediaType: schema2.MediaTypeLayer,
+		Digest:    digest.Digest("sha256:d2562f1ae1d0593a26c54006ad0a6211c35fdc8b4067485d7208000d83477de2"),
+		Size:      201,
+	},
+}
+
+const manifestSchema2ConfigDigest = `sha256:da5939581ac835614e3cf6c765e7489e6d0fc602a44e98c07013f1c938f49675`
+
+var manifestSchema2ConfigDescriptor = distribution.Descriptor{
+	Digest:    digest.Digest(manifestSchema2ConfigDigest),
+	Size:      5838,
+	MediaType: schema2.MediaTypeConfig,
+}
+
+const manifestSchema2 = `{
+   "schemaVersion": 2,
+   "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
+   "config": {
+      "mediaType": "application/vnd.docker.container.image.v1+json",
+      "size": 5838,
+      "digest": "sha256:da5939581ac835614e3cf6c765e7489e6d0fc602a44e98c07013f1c938f49675"
+   },
+   "layers": [
+      {
+         "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
+         "size": 22488057,
+         "digest": "sha256:afeb2bfd31c0760573e7262de6ae67a84da0e0a1c3e8157bbddd41a501b18a5c"
+      },
+      {
+         "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
+         "size": 21869700,
+         "digest": "sha256:7ff5d10493db2cdfc1b7238434c503cc0664d48d0f7154ea9472e734b28a72dd"
+      },
+      {
+         "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
+         "size": 201,
+         "digest": "sha256:d2562f1ae1d0593a26c54006ad0a6211c35fdc8b4067485d7208000d83477de2"
+      }
+   ]
+}`
+
+const manifestSchema2Invalid = `{
+   "schemaVersion": 2,
+   "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
+   "config": {},
+   []
+}`