addressing comments from stevvooe

Signed-off-by: Mike Brown <brownwm@us.ibm.com>
2017-07-20 20:44:02 -05:00 · 2017-07-20 20:44:02 -05:00 · ec2aa05cdf
commit ec2aa05cdf
parent c1532332ad
6 changed files with 86 additions and 144 deletions
--- 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,64 +49,45 @@ 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 <alyspdev@example.com>",
    "architecture": "amd64",
+    "os": "linux",
    "config": {
-        "AttachStderr": false,
-        "AttachStdin": false,
-        "AttachStdout": false,
-        "Cmd": [
-            "/bin/sh",
-            "-c",
-            "echo hi"
-        ],
-        "Domainname": "",
-        "Entrypoint": null,
+        "User": "alice",
+        "ExposedPorts": {
+            "8080/tcp": {}
+        },
        "Env": [
            "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
-            "derived=true",
-            "asdf=true"
+            "FOO=oci_is_a",
+            "BAR=well_written_spec"
+        ],
+        "Entrypoint": [
+            "/bin/my-app-binary"
        ],
-        "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\"]"
+            "--foreground",
+            "--config",
+            "/etc/my-app.d/default.cfg"
        ],
-        "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": ""
+        "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"
    },
-    "created": "2015-11-04T23:06:32.365666163Z",
-    "docker_version": "1.9.0-dev",
    "history": [
      {
        "created": "2015-10-31T22:22:54.690851953Z",
@ -115,37 +95,10 @@ func TestBuilder(t *testing.T) {
      },
      {
        "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\"]",
+        "created_by": "/bin/sh -c #(nop) CMD [\"sh\"]",
        "empty_layer": true
      }
-    ],
-    "os": "linux",
-    "rootfs": {
-        "diff_ids": [
-            "sha256:c6f988f4874bb0add23a778f753c65efe992244e148a1d2ec2a8b664fb66bbd1",
-            "sha256:5f70bf18a086007016e948b04aed3b82103a36bea41755b6cddfaf10ace3c6ef",
-            "sha256:13f53e08df5a220ab6d13c58b2bf83a59cbdc2e04d0a3f041ddf4b0ba4112d49"
-        ],
-        "type": "layers"
-    }
+    ]
 }`)
 	configDigest := digest.FromBytes(imgJSON)

@ -154,6 +107,7 @@ func TestBuilder(t *testing.T) {
 			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)
 	}

--- 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,
 	}
 )
--- 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"
+         }
      }
   ]
 }`)
@ -35,12 +41,14 @@ func TestManifest(t *testing.T) {
 			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,
+				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"])
+	}
 }
--- 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
 	}
--- 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)
--- 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,