Correct missing sizes for manifest schema 1 images

Make sure to encode the updated sizes into image.DockerImageMetadata.Raw
otherwise the changes will be lost.

Signed-off-by: Michal Minář <miminar@redhat.com>

pkg/dockerregistry/server/manifesthandler.go

@@ -9,16 +9,24 @@ import (
 	"github.com/docker/distribution/manifest/schema1"
 	"github.com/docker/distribution/manifest/schema2"
 
+	imageapi "github.com/openshift/origin/pkg/image/apis/image"
 	imageapiv1 "github.com/openshift/origin/pkg/image/apis/image/v1"
 )
 
 // 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)
+
+	// Layers returns a list of image layers.
+	Layers(ctx context.Context) ([]imageapiv1.ImageLayer /* TODO: map[string]string, */, error)
+
+	// Metadata returns image configuration in internal representation.
+	Metadata(ctx context.Context /*, layers []imageapiv1.ImageLayer */) (*imageapi.DockerImage, error)
 
 	// Manifest returns a deserialized manifest object.
 	Manifest() distribution.Manifest
@@ -27,11 +35,11 @@ type ManifestHandler interface {
 	// signatures or an error if the information could not be fetched.
 	Payload() (mediaType string, payload []byte, canonical []byte, err error)
 
+	// Signatures returns signature blobs embedded in the manifest. This applies only to manifest schema 1.
+	Signatures(ctx context.Context) ([][]byte, error)
+
 	// 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.

pkg/dockerregistry/server/manifestschema1handler.go

@@ -2,6 +2,7 @@ package server
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"path"
 
@@ -18,6 +19,8 @@ import (
 	imageapiv1 "github.com/openshift/origin/pkg/image/apis/image/v1"
 )
 
+var ErrNoManifestMetadata = errors.New("no manifest metadata found")
+
 func unmarshalManifestSchema1(content []byte, signatures [][]byte) (distribution.Manifest, error) {
 	// prefer signatures from the manifest
 	if _, err := libtrust.ParsePrettySignature(content, "signatures"); err == nil {
@@ -46,69 +49,84 @@ func unmarshalManifestSchema1(content []byte, signatures [][]byte) (distribution
 }
 
 type manifestSchema1Handler struct {
-	repo     *repository
-	manifest *schema1.SignedManifest
+	repo         *repository
+	manifest     *schema1.SignedManifest
+	cachedLayers []imageapiv1.ImageLayer
 }
 
 var _ ManifestHandler = &manifestSchema1Handler{}
 
-func (h *manifestSchema1Handler) FillImageMetadata(ctx context.Context, image *imageapiv1.Image) error {
-	signatures, err := h.manifest.Signatures()
-	if err != nil {
-		return err
+func (h *manifestSchema1Handler) Layers(ctx context.Context) ([]imageapiv1.ImageLayer, error) {
+	if h.cachedLayers == nil {
+		var sizeContainer = imageapi.DockerV1CompatibilityImageSize{}
+
+		layers := make([]imageapiv1.ImageLayer, len(h.manifest.FSLayers))
+		for hi, li := 0, len(h.manifest.FSLayers)-1; hi < len(h.manifest.FSLayers) && li >= 0; hi, li = hi+1, li-1 {
+			layer := &layers[li]
+			sizeContainer.Size = 0
+			if hi < len(h.manifest.History) {
+				if err := json.Unmarshal([]byte(h.manifest.History[hi].V1Compatibility), &sizeContainer); err != nil {
+					sizeContainer.Size = 0
+				}
+			}
+			if err := h.updateLayerMetadata(ctx, layer, &h.manifest.FSLayers[hi], sizeContainer.Size); err != nil {
+				return nil, err
+			}
+		}
+
+		h.cachedLayers = layers
 	}
 
-	for _, signDigest := range signatures {
-		image.DockerImageSignatures = append(image.DockerImageSignatures, signDigest)
+	layers := make([]imageapiv1.ImageLayer, len(h.cachedLayers))
+	for i, l := range h.cachedLayers {
+		layers[i] = l
 	}
 
-	refs := h.manifest.References()
+	return layers, nil
+}
 
-	if err := imageMetadataFromManifest(image); err != nil {
-		return fmt.Errorf("unable to fill image %s metadata: %v", image.Name, err)
+func (h *manifestSchema1Handler) Metadata(ctx context.Context) (*imageapi.DockerImage, error) {
+	if len(h.manifest.History) == 0 {
+		// should never have an empty history, but just in case...
+		return nil, ErrNoManifestMetadata
 	}
 
-	blobSet := sets.NewString()
-	meta, ok := image.DockerImageMetadata.Object.(*imageapi.DockerImage)
-	if !ok {
-		return fmt.Errorf("image %q does not have metadata", image.Name)
+	v1Metadata := imageapi.DockerV1CompatibilityImage{}
+	if err := json.Unmarshal([]byte(h.manifest.History[0].V1Compatibility), &v1Metadata); err != nil {
+		return nil, err
 	}
-	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)
+	var (
+		dockerImageSize int64
+		layerSet        = sets.NewString()
+	)
+
+	layers, err := h.Layers(ctx)
+	if err != nil {
+		return nil, err
+	}
+	for _, layer := range layers {
+		if !layerSet.Has(layer.Name) {
+			dockerImageSize += layer.LayerSize
+			layerSet.Insert(layer.Name)
 		}
 	}
-	image.DockerImageMetadata.Object = meta
 
-	return nil
+	return v1ToDockerImageMetadata(&v1Metadata, "", dockerImageSize), nil
+}
+
+func (h *manifestSchema1Handler) Signatures(ctx context.Context) ([][]byte, error) {
+	return h.manifest.Signatures()
 }
 
 func (h *manifestSchema1Handler) Manifest() distribution.Manifest {
 	return h.manifest
 }
 
+func (h *manifestSchema1Handler) Config(ctx context.Context) ([]byte, error) {
+	return nil, nil
+}
+
 func (h *manifestSchema1Handler) Payload() (mediaType string, payload []byte, canonical []byte, err error) {
 	mt, payload, err := h.manifest.Payload()
 	return mt, payload, h.manifest.Canonical, err
@@ -187,3 +205,25 @@ func (h *manifestSchema1Handler) Verify(ctx context.Context, skipDependencyVerif
 func (h *manifestSchema1Handler) Digest() (digest.Digest, error) {
 	return digest.FromBytes(h.manifest.Canonical), nil
 }
+
+func (h *manifestSchema1Handler) updateLayerMetadata(
+	ctx context.Context,
+	layerMetadata *imageapiv1.ImageLayer,
+	manifestLayer *schema1.FSLayer,
+	size int64,
+) error {
+	layerMetadata.Name = manifestLayer.BlobSum.String()
+	layerMetadata.MediaType = schema1.MediaTypeManifestLayer
+	if size > 0 {
+		layerMetadata.LayerSize = size
+		return nil
+	}
+
+	desc, err := h.repo.Blobs(ctx).Stat(ctx, digest.Digest(layerMetadata.Name))
+	if err != nil {
+		context.GetLogger(ctx).Errorf("failed to stat blob %s", layerMetadata.Name)
+		return err
+	}
+	layerMetadata.LayerSize = desc.Size
+	return nil
+}