Skip to content

Commit

Permalink
Merge pull request #16325 from miminar/encode-dockerimagemetadata-for…
Browse files Browse the repository at this point in the history
…-schema1

Automatic merge from submit-queue.

Correct missing sizes for manifest schema 1 images

Instead of filling metadata in the image object in the registry, send the manifest and config blobs to the master and let it do the job.

Resolves #16306
Resolves: [bz#1491589](https://bugzilla.redhat.com/show_bug.cgi?id=1491589)
  • Loading branch information
openshift-merge-robot committed Oct 3, 2017
2 parents afd90cd + 5ac2762 commit 5471922
Show file tree
Hide file tree
Showing 13 changed files with 820 additions and 306 deletions.
14 changes: 6 additions & 8 deletions pkg/dockerregistry/server/manifesthandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
64 changes: 5 additions & 59 deletions pkg/dockerregistry/server/manifestschema1handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
375 changes: 375 additions & 0 deletions pkg/dockerregistry/server/manifestschema1handler_test.go

Large diffs are not rendered by default.

50 changes: 28 additions & 22 deletions pkg/dockerregistry/server/manifestschema2handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -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 {
Expand Down Expand Up @@ -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
}
138 changes: 138 additions & 0 deletions pkg/dockerregistry/server/manifestschema2handler_test.go
Original file line number Diff line number Diff line change
@@ -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": {},
[]
}`
Loading

0 comments on commit 5471922

Please sign in to comment.