From ec2aa05cdfa5b2318554841818f7b18d5be4582a Mon Sep 17 00:00:00 2001 From: Mike Brown Date: Thu, 20 Jul 2017 20:44:02 -0500 Subject: [PATCH] addressing comments from stevvooe Signed-off-by: Mike Brown --- manifest/ocischema/builder_test.go | 150 +++++++------------- manifest/ocischema/manifest.go | 2 +- manifest/ocischema/manifest_test.go | 30 ++-- registry/handlers/manifests.go | 10 +- registry/storage/ocimanifesthandler.go | 18 --- registry/storage/ocimanifesthandler_test.go | 20 +-- 6 files changed, 86 insertions(+), 144 deletions(-) diff --git a/manifest/ocischema/builder_test.go b/manifest/ocischema/builder_test.go index ca785495..27562cfd 100644 --- a/manifest/ocischema/builder_test.go +++ b/manifest/ocischema/builder_test.go @@ -10,7 +10,6 @@ import ( "github.com/opencontainers/image-spec/specs-go/v1" ) -// TODO (not assigned): consider making mockBlobService common for ocischema, schema2, and schema1 type mockBlobService struct { descriptors map[digest.Digest]distribution.Descriptor } @@ -50,110 +49,65 @@ func (bs *mockBlobService) Resume(ctx context.Context, id string) (distribution. func TestBuilder(t *testing.T) { imgJSON := []byte(`{ + "created": "2015-10-31T22:22:56.015925234Z", + "author": "Alyssa P. Hacker ", "architecture": "amd64", - "config": { - "AttachStderr": false, - "AttachStdin": false, - "AttachStdout": false, - "Cmd": [ - "/bin/sh", - "-c", - "echo hi" - ], - "Domainname": "", - "Entrypoint": null, - "Env": [ - "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", - "derived=true", - "asdf=true" - ], - "Hostname": "23304fc829f9", - "Image": "sha256:4ab15c48b859c2920dd5224f92aabcd39a52794c5b3cf088fb3bbb438756c246", - "Labels": {}, - "OnBuild": [], - "OpenStdin": false, - "StdinOnce": false, - "Tty": false, - "User": "", - "Volumes": null, - "WorkingDir": "" - }, - "container": "e91032eb0403a61bfe085ff5a5a48e3659e5a6deae9f4d678daa2ae399d5a001", - "container_config": { - "AttachStderr": false, - "AttachStdin": false, - "AttachStdout": false, - "Cmd": [ - "/bin/sh", - "-c", - "#(nop) CMD [\"/bin/sh\" \"-c\" \"echo hi\"]" - ], - "Domainname": "", - "Entrypoint": null, - "Env": [ - "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", - "derived=true", - "asdf=true" - ], - "Hostname": "23304fc829f9", - "Image": "sha256:4ab15c48b859c2920dd5224f92aabcd39a52794c5b3cf088fb3bbb438756c246", - "Labels": {}, - "OnBuild": [], - "OpenStdin": false, - "StdinOnce": false, - "Tty": false, - "User": "", - "Volumes": null, - "WorkingDir": "" - }, - "created": "2015-11-04T23:06:32.365666163Z", - "docker_version": "1.9.0-dev", - "history": [ - { - "created": "2015-10-31T22:22:54.690851953Z", - "created_by": "/bin/sh -c #(nop) ADD file:a3bc1e842b69636f9df5256c49c5374fb4eef1e281fe3f282c65fb853ee171c5 in /" - }, - { - "created": "2015-10-31T22:22:55.613815829Z", - "created_by": "/bin/sh -c #(nop) CMD [\"sh\"]" - }, - { - "created": "2015-11-04T23:06:30.934316144Z", - "created_by": "/bin/sh -c #(nop) ENV derived=true", - "empty_layer": true - }, - { - "created": "2015-11-04T23:06:31.192097572Z", - "created_by": "/bin/sh -c #(nop) ENV asdf=true", - "empty_layer": true - }, - { - "created": "2015-11-04T23:06:32.083868454Z", - "created_by": "/bin/sh -c dd if=/dev/zero of=/file bs=1024 count=1024" - }, - { - "created": "2015-11-04T23:06:32.365666163Z", - "created_by": "/bin/sh -c #(nop) CMD [\"/bin/sh\" \"-c\" \"echo hi\"]", - "empty_layer": true - } - ], "os": "linux", - "rootfs": { - "diff_ids": [ - "sha256:c6f988f4874bb0add23a778f753c65efe992244e148a1d2ec2a8b664fb66bbd1", - "sha256:5f70bf18a086007016e948b04aed3b82103a36bea41755b6cddfaf10ace3c6ef", - "sha256:13f53e08df5a220ab6d13c58b2bf83a59cbdc2e04d0a3f041ddf4b0ba4112d49" + "config": { + "User": "alice", + "ExposedPorts": { + "8080/tcp": {} + }, + "Env": [ + "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", + "FOO=oci_is_a", + "BAR=well_written_spec" ], - "type": "layers" - } + "Entrypoint": [ + "/bin/my-app-binary" + ], + "Cmd": [ + "--foreground", + "--config", + "/etc/my-app.d/default.cfg" + ], + "Volumes": { + "/var/job-result-data": {}, + "/var/log/my-app-logs": {} + }, + "WorkingDir": "/home/alice", + "Labels": { + "com.example.project.git.url": "https://example.com/project.git", + "com.example.project.git.commit": "45a939b2999782a3f005621a8d0f29aa387e1d6b" + } + }, + "rootfs": { + "diff_ids": [ + "sha256:c6f988f4874bb0add23a778f753c65efe992244e148a1d2ec2a8b664fb66bbd1", + "sha256:5f70bf18a086007016e948b04aed3b82103a36bea41755b6cddfaf10ace3c6ef" + ], + "type": "layers" + }, + "history": [ + { + "created": "2015-10-31T22:22:54.690851953Z", + "created_by": "/bin/sh -c #(nop) ADD file:a3bc1e842b69636f9df5256c49c5374fb4eef1e281fe3f282c65fb853ee171c5 in /" + }, + { + "created": "2015-10-31T22:22:55.613815829Z", + "created_by": "/bin/sh -c #(nop) CMD [\"sh\"]", + "empty_layer": true + } + ] }`) configDigest := digest.FromBytes(imgJSON) descriptors := []distribution.Descriptor{ { - Digest: digest.Digest("sha256:a3ed95caeb02ffe68cdd9fd84406680ae93d633cb16422d00e8a7c22955b46d4"), - Size: 5312, - MediaType: v1.MediaTypeImageLayerGzip, + Digest: digest.Digest("sha256:a3ed95caeb02ffe68cdd9fd84406680ae93d633cb16422d00e8a7c22955b46d4"), + Size: 5312, + MediaType: v1.MediaTypeImageLayerGzip, + Annotations: map[string]string{"apple": "orange", "lettuce": "wrap"}, }, { Digest: digest.Digest("sha256:86e0e091d0da6bde2456dbb48306f3956bbeb2eae1b5b9a43045843f69fe4aaa"), @@ -200,7 +154,7 @@ func TestBuilder(t *testing.T) { if target.MediaType != v1.MediaTypeImageConfig { t.Fatalf("unexpected media type in target: %s", target.MediaType) } - if target.Size != 3153 { + if target.Size != 1582 { t.Fatalf("unexpected size in target: %d", target.Size) } diff --git a/manifest/ocischema/manifest.go b/manifest/ocischema/manifest.go index c3d9046a..afab12bd 100644 --- a/manifest/ocischema/manifest.go +++ b/manifest/ocischema/manifest.go @@ -15,7 +15,7 @@ var ( // SchemaVersion provides a pre-initialized version structure for this // packages version of the manifest. SchemaVersion = manifest.Versioned{ - SchemaVersion: 2, // TODO: (mikebrow/stevvooe) this could be confusing cause oci version 1 is closer to docker 2 than 1 + SchemaVersion: 2, // historical value here.. does not pertain to OCI or docker version MediaType: v1.MediaTypeImageManifest, } ) diff --git a/manifest/ocischema/manifest_test.go b/manifest/ocischema/manifest_test.go index 714c50a5..c37fe8fb 100644 --- a/manifest/ocischema/manifest_test.go +++ b/manifest/ocischema/manifest_test.go @@ -17,13 +17,19 @@ var expectedManifestSerialization = []byte(`{ "config": { "mediaType": "application/vnd.oci.image.config.v1+json", "size": 985, - "digest": "sha256:1a9ec845ee94c202b2d5da74a24f0ed2058318bfa9879fa541efaecba272e86b" + "digest": "sha256:1a9ec845ee94c202b2d5da74a24f0ed2058318bfa9879fa541efaecba272e86b", + "annotations": { + "apple": "orange" + } }, "layers": [ { "mediaType": "application/vnd.oci.image.layer.v1.tar+gzip", "size": 153263, - "digest": "sha256:62d8908bee94c202b2d35224a221aaa2058318bfa9879fa541efaecba272331b" + "digest": "sha256:62d8908bee94c202b2d35224a221aaa2058318bfa9879fa541efaecba272331b", + "annotations": { + "lettuce": "wrap" + } } ] }`) @@ -32,15 +38,17 @@ func TestManifest(t *testing.T) { manifest := Manifest{ Versioned: SchemaVersion, Config: distribution.Descriptor{ - Digest: "sha256:1a9ec845ee94c202b2d5da74a24f0ed2058318bfa9879fa541efaecba272e86b", - Size: 985, - MediaType: v1.MediaTypeImageConfig, + Digest: "sha256:1a9ec845ee94c202b2d5da74a24f0ed2058318bfa9879fa541efaecba272e86b", + Size: 985, + MediaType: v1.MediaTypeImageConfig, + Annotations: map[string]string{"apple": "orange"}, }, Layers: []distribution.Descriptor{ { - Digest: "sha256:62d8908bee94c202b2d35224a221aaa2058318bfa9879fa541efaecba272331b", - Size: 153263, - MediaType: v1.MediaTypeImageLayerGzip, + Digest: "sha256:62d8908bee94c202b2d35224a221aaa2058318bfa9879fa541efaecba272331b", + Size: 153263, + MediaType: v1.MediaTypeImageLayerGzip, + Annotations: map[string]string{"lettuce": "wrap"}, }, }, } @@ -90,6 +98,9 @@ func TestManifest(t *testing.T) { if target.Size != 985 { t.Fatalf("unexpected size in target: %d", target.Size) } + if target.Annotations["apple"] != "orange" { + t.Fatalf("unexpected annotation in target: %s", target.Annotations["apple"]) + } references := deserialized.References() if len(references) != 2 { @@ -110,4 +121,7 @@ func TestManifest(t *testing.T) { if references[1].Size != 153263 { t.Fatalf("unexpected size in reference: %d", references[0].Size) } + if references[1].Annotations["lettuce"] != "wrap" { + t.Fatalf("unexpected annotation in reference: %s", references[1].Annotations["lettuce"]) + } } diff --git a/registry/handlers/manifests.go b/registry/handlers/manifests.go index ad39ccb5..53c7c807 100644 --- a/registry/handlers/manifests.go +++ b/registry/handlers/manifests.go @@ -122,8 +122,7 @@ func (imh *manifestHandler) GetManifest(w http.ResponseWriter, r *http.Request) if imh.Tag != "" { tags := imh.Repository.Tags(imh) - var desc distribution.Descriptor - desc, err = tags.Get(imh, imh.Tag) + desc, err := tags.Get(imh, imh.Tag) if err != nil { if _, ok := err.(distribution.ErrTagUnknown); ok { imh.Errors = append(imh.Errors, v2.ErrorCodeManifestUnknown.WithDetail(err)) @@ -144,8 +143,7 @@ func (imh *manifestHandler) GetManifest(w http.ResponseWriter, r *http.Request) if imh.Tag != "" { options = append(options, distribution.WithTag(imh.Tag)) } - var manifest distribution.Manifest - manifest, err = manifests.Get(imh, imh.Digest, options...) + manifest, err := manifests.Get(imh, imh.Digest, options...) if err != nil { if _, ok := err.(distribution.ErrManifestUnknownRevision); ok { imh.Errors = append(imh.Errors, v2.ErrorCodeManifestUnknown.WithDetail(err)) @@ -266,7 +264,7 @@ func (imh *manifestHandler) convertSchema2Manifest(schema2Manifest *schema2.Dese builder := schema1.NewConfigManifestBuilder(imh.Repository.Blobs(imh), imh.Context.App.trustKey, ref, configJSON) for _, d := range schema2Manifest.Layers { - if err = builder.AppendReference(d); err != nil { + if err := builder.AppendReference(d); err != nil { imh.Errors = append(imh.Errors, v2.ErrorCodeManifestInvalid.WithDetail(err)) return nil, err } @@ -339,7 +337,7 @@ func (imh *manifestHandler) PutManifest(w http.ResponseWriter, r *http.Request) options = append(options, distribution.WithTag(imh.Tag)) } - if err = imh.applyResourcePolicy(manifest); err != nil { + if err := imh.applyResourcePolicy(manifest); err != nil { imh.Errors = append(imh.Errors, err) return } diff --git a/registry/storage/ocimanifesthandler.go b/registry/storage/ocimanifesthandler.go index 0efef56b..c61beb4b 100644 --- a/registry/storage/ocimanifesthandler.go +++ b/registry/storage/ocimanifesthandler.go @@ -3,7 +3,6 @@ package storage import ( "encoding/json" "fmt" - "net/url" "github.com/docker/distribution" "github.com/docker/distribution/context" @@ -80,23 +79,6 @@ func (ms *ocischemaManifestHandler) verifyManifest(ctx context.Context, mnfst oc var err error switch descriptor.MediaType { - // TODO: mikebrow/steveoe verify we should treat oci nondistributable like foreign layers? - case v1.MediaTypeImageLayerNonDistributable, v1.MediaTypeImageLayerNonDistributableGzip: - // Clients download this layer from an external URL, so do not check for - // its presence. - 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 v1.MediaTypeImageManifest: var exists bool exists, err = manifestService.Exists(ctx, descriptor.Digest) diff --git a/registry/storage/ocimanifesthandler_test.go b/registry/storage/ocimanifesthandler_test.go index d04ce531..047f1c3a 100644 --- a/registry/storage/ocimanifesthandler_test.go +++ b/registry/storage/ocimanifesthandler_test.go @@ -53,12 +53,6 @@ func TestVerifyOCIManifestNonDistributableLayer(t *testing.T) { cases := []testcase{ { - nonDistributableLayer, - nil, - errMissingURL, - }, - { - // regular layers may have foreign urls (non-Distributable Layers) layer, []string{"http://foo/bar"}, nil, @@ -66,37 +60,37 @@ func TestVerifyOCIManifestNonDistributableLayer(t *testing.T) { { nonDistributableLayer, []string{"file:///local/file"}, - errInvalidURL, + nil, }, { nonDistributableLayer, []string{"http://foo/bar#baz"}, - errInvalidURL, + nil, }, { nonDistributableLayer, []string{""}, - errInvalidURL, + nil, }, { nonDistributableLayer, []string{"https://foo/bar", ""}, - errInvalidURL, + nil, }, { nonDistributableLayer, []string{"", "https://foo/bar"}, - errInvalidURL, + nil, }, { nonDistributableLayer, []string{"http://nope/bar"}, - errInvalidURL, + nil, }, { nonDistributableLayer, []string{"http://foo/nope"}, - errInvalidURL, + nil, }, { nonDistributableLayer,