Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

update test vendor to image-spec rc5 #241

Merged
merged 2 commits into from
May 10, 2017
Merged

update test vendor to image-spec rc5 #241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df66618
oci: layout: update to image-spec RC5
runcom Feb 25, 2017
088dc7d
oci: layout: write oci layout file in Commit
runcom May 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions manifest/fixtures/ociv1.image.index.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{
"schemaVersion": 2,
"manifests": [
{
"mediaType": "application/vnd.oci.image.manifest.v1+json",
"size": 7143,
"digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f",
"platform": {
"architecture": "ppc64le",
"os": "linux"
}
},
{
"mediaType": "application/vnd.oci.image.manifest.v1+json",
"size": 7682,
"digest": "sha256:5b0bcabd1ed22e9fb1310cf6c2dec7cdef19f0ad69efa1f392e94a4333501270",
"platform": {
"architecture": "amd64",
"os": "linux",
"os.features": [
"sse4"
]
}
}
],
"annotations": {
"com.example.key1": "value1",
"com.example.key2": "value2"
}
}
51 changes: 27 additions & 24 deletions manifest/fixtures/ociv1.manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,29 @@
{
"schemaVersion": 2,
"mediaType": "application/vnd.oci.image.manifest.v1+json",
"config": {
"mediaType": "application/vnd.oci.image.serialization.config.v1+json",
"size": 7023,
"digest": "sha256:b5b2b2c507a0944348e0303114d8d93aaaa081732b86451d9bce1f432a537bc7"
"schemaVersion": 2,
"config": {
"mediaType": "application/vnd.oci.image.config.v1+json",
"size": 7023,
"digest": "sha256:b5b2b2c507a0944348e0303114d8d93aaaa081732b86451d9bce1f432a537bc7"
},
"layers": [
{
"mediaType": "application/vnd.oci.image.layer.v1.tar+gzip",
"size": 32654,
"digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f"
},
"layers": [
{
"mediaType": "application/vnd.oci.image.serialization.rootfs.tar.gzip",
"size": 32654,
"digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f"
},
{
"mediaType": "application/vnd.oci.image.serialization.rootfs.tar.gzip",
"size": 16724,
"digest": "sha256:3c3a4604a545cdc127456d94e421cd355bca5b528f4a9c1905b15da2eb4a4c6b"
},
{
"mediaType": "application/vnd.oci.image.serialization.rootfs.tar.gzip",
"size": 73109,
"digest": "sha256:ec4b8955958665577945c89419d1af06b5f7636b4ac3da7f12184802ad867736"
}
]
}
{
"mediaType": "application/vnd.oci.image.layer.v1.tar+gzip",
"size": 16724,
"digest": "sha256:3c3a4604a545cdc127456d94e421cd355bca5b528f4a9c1905b15da2eb4a4c6b"
},
{
"mediaType": "application/vnd.oci.image.layer.v1.tar+gzip",
"size": 73109,
"digest": "sha256:ec4b8955958665577945c89419d1af06b5f7636b4ac3da7f12184802ad867736"
}
],
"annotations": {
"com.example.key1": "value1",
"com.example.key2": "value2"
}
}
56 changes: 0 additions & 56 deletions manifest/fixtures/ociv1list.manifest.json
View file
Open in desktop

This file was deleted.

28 changes: 26 additions & 2 deletions manifest/manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func GuessMIMEType(manifest []byte) string {
}

switch meta.MediaType {
case DockerV2Schema2MediaType, DockerV2ListMediaType, imgspecv1.MediaTypeImageManifest, imgspecv1.MediaTypeImageManifestList: // A recognized type.
case DockerV2Schema2MediaType, DockerV2ListMediaType: // A recognized type.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vaguely remember there was an earlier conversation about this, but I can't remember the details: why is guessing that a manifest is OCI no longer applicable? Because a manifest can never be a stand-alone object with OCi without an index now? Or just because OCI decided not to include the type in the JSON?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No mime type in Json anymore

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To expand on the above, it's because types are now implemented using descriptor types (so objects are no longer self-descriptive).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way, this is awkward.

  • dirImageSource and storageImageSource depend on GuessMIMEType to work; otherwise c/i/image assumes schema1. (Not trivially fixable.)
  • dockerImageDestination depends on GuessMIMEType for setting Content-Type on upload. (This one is probably fixable by adding an explicit parameter, and using the c/i/image handler to get the type we are using.)

Is there another heuristic we can use? e.g. schemaVersion == 2 && mediaType missing? Or perhaps together with config.mediaType?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed I think

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Could the if meta.SchemaVersion == 2 code be unified with case 2 below?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return meta.MediaType
}
// this is the only way the function can return DockerV2Schema1MediaType, and recognizing that is essential for stripping the JWS signatures = computing the correct manifest digest.
Expand All @@ -64,7 +64,31 @@ func GuessMIMEType(manifest []byte) string {
return DockerV2Schema1SignedMediaType
}
return DockerV2Schema1MediaType
case 2: // Really should not happen, meta.MediaType should have been set. But given the data, this is our best guess.
case 2:
// best effort to understand if this is an OCI image since mediaType
// isn't in the manifest for OCI anymore
// for docker v2s2 meta.MediaType should have been set. But given the data, this is our best guess.
ociMan := struct {
Config struct {
MediaType string `json:"mediaType"`
} `json:"config"`
Layers []imgspecv1.Descriptor `json:"layers"`
}{}
if err := json.Unmarshal(manifest, &ociMan); err != nil {
return ""
}
if ociMan.Config.MediaType == imgspecv1.MediaTypeImageConfig && len(ociMan.Layers) != 0 {
return imgspecv1.MediaTypeImageManifest
}
ociIndex := struct {
Manifests []imgspecv1.Descriptor `json:"manifests"`
}{}
if err := json.Unmarshal(manifest, &ociIndex); err != nil {
return ""
}
if len(ociIndex.Manifests) != 0 && ociIndex.Manifests[0].MediaType == imgspecv1.MediaTypeImageManifest {
return imgspecv1.MediaTypeImageIndex
}
return DockerV2Schema2MediaType
}
return ""
Expand Down
4 changes: 2 additions & 2 deletions manifest/manifest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@ func TestGuessMIMEType(t *testing.T) {
path string
mimeType string
}{
{"ociv1.manifest.json", imgspecv1.MediaTypeImageManifest},
{"ociv1list.manifest.json", imgspecv1.MediaTypeImageManifestList},
{"v2s2.manifest.json", DockerV2Schema2MediaType},
{"v2list.manifest.json", DockerV2ListMediaType},
{"v2s1.manifest.json", DockerV2Schema1SignedMediaType},
Expand All @@ -31,6 +29,8 @@ func TestGuessMIMEType(t *testing.T) {
{"v2s2nomime.manifest.json", DockerV2Schema2MediaType}, // It is unclear whether this one is legal, but we should guess v2s2 if anything at all.
{"unknown-version.manifest.json", ""},
{"non-json.manifest.json", ""}, // Not a manifest (nor JSON) at all
{"ociv1.manifest.json", imgspecv1.MediaTypeImageManifest},
{"ociv1.image.index.json", imgspecv1.MediaTypeImageIndex},
}

for _, c := range cases {
Expand Down
47 changes: 31 additions & 16 deletions oci/layout/oci_dest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,30 @@ import (
"io/ioutil"
"os"
"path/filepath"
"runtime"

"github.com/pkg/errors"

"github.com/containers/image/manifest"
"github.com/containers/image/types"
"github.com/opencontainers/go-digest"
imgspec "github.com/opencontainers/image-spec/specs-go"
imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
)

type ociImageDestination struct {
ref ociReference
ref ociReference
index imgspecv1.ImageIndex
}

// newImageDestination returns an ImageDestination for writing to an existing directory.
func newImageDestination(ref ociReference) types.ImageDestination {
return &ociImageDestination{ref: ref}
index := imgspecv1.ImageIndex{
Versioned: imgspec.Versioned{
SchemaVersion: 2,
},
}
return &ociImageDestination{ref: ref, index: index}
}

// Reference returns the reference used to set up this destination. Note that this should directly correspond to user's intent,
Expand Down Expand Up @@ -152,10 +160,6 @@ func (d *ociImageDestination) PutManifest(m []byte) error {
// TODO(runcom): beaware and add support for OCI manifest list
desc.MediaType = imgspecv1.MediaTypeImageManifest
desc.Size = int64(len(m))
data, err := json.Marshal(desc)
if err != nil {
return err
}

blobPath, err := d.ref.blobPath(digest)
if err != nil {
Expand All @@ -167,15 +171,19 @@ func (d *ociImageDestination) PutManifest(m []byte) error {
if err := ioutil.WriteFile(blobPath, m, 0644); err != nil {
return err
}
// TODO(runcom): ugly here?
if err := ioutil.WriteFile(d.ref.ociLayoutPath(), []byte(`{"imageLayoutVersion": "1.0.0"}`), 0644); err != nil {
return err
}
descriptorPath := d.ref.descriptorPath(d.ref.tag)
if err := ensureParentDirectoryExists(descriptorPath); err != nil {
return err
}
return ioutil.WriteFile(descriptorPath, data, 0644)

annotations := make(map[string]string)
annotations["org.opencontainers.ref.name"] = d.ref.tag
desc.Annotations = annotations
d.index.Manifests = append(d.index.Manifests, imgspecv1.ManifestDescriptor{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gives a bit of an impression that the code adds more manifests to an existing index, but we actually never read the original file and blindly overwrite it if it exists.

I guess the intent is to perhaps add the “update existing index” over time? If so, this code does make enough sense; if not, and the intent is to always do a simple overwrite, the code structure could probably be a bit simpler (do nothing in Commit, just build a single ImageIndex local variable in PutManifest).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, the intent is to "update existing index"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, the intent is to "update existing index"

As in, with this PR, or at some later time? Because it is not updating an existing index now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you call PutManifest many times and at the end Commit you'll have a multi-manifest index with this code, am I stupidly missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, true. I wasn’t considering that because we have no tools to do that (and the semantics of doing that + PutSignatures is unclear — what manifest are the signatures attached to?).

Anyway, that can be useful for someone who explicitly expects that oci: behavior, and it doesn’t break anything, so fair enough.

Descriptor: desc,
Platform: imgspecv1.Platform{
Architecture: runtime.GOARCH,
OS: runtime.GOOS,
},
})

return nil
}

func ensureDirectoryExists(path string) error {
Expand Down Expand Up @@ -204,5 +212,12 @@ func (d *ociImageDestination) PutSignatures(signatures [][]byte) error {
// - Uploaded data MAY be visible to others before Commit() is called
// - Uploaded data MAY be removed or MAY remain around if Close() is called without Commit() (i.e. rollback is allowed but not guaranteed)
func (d *ociImageDestination) Commit() error {
return nil
if err := ioutil.WriteFile(d.ref.ociLayoutPath(), []byte(`{"imageLayoutVersion": "1.0.0"}`), 0644); err != nil {
return err
}
indexJSON, err := json.Marshal(d.index)
if err != nil {
return err
}
return ioutil.WriteFile(d.ref.indexPath(), indexJSON, 0644)
}
28 changes: 10 additions & 18 deletions oci/layout/oci_src.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package layout

import (
"encoding/json"
"io"
"io/ioutil"
"os"
Expand All @@ -12,12 +11,17 @@ import (
)

type ociImageSource struct {
ref ociReference
ref ociReference
descriptor imgspecv1.ManifestDescriptor
}

// newImageSource returns an ImageSource for reading from an existing directory.
func newImageSource(ref ociReference) types.ImageSource {
return &ociImageSource{ref: ref}
func newImageSource(ref ociReference) (types.ImageSource, error) {
descriptor, err := ref.getManifestDescriptor()
if err != nil {
return nil, err
}
return &ociImageSource{ref: ref, descriptor: descriptor}, nil
}

// Reference returns the reference used to set up this source.
Expand All @@ -33,19 +37,7 @@ func (s *ociImageSource) Close() error {
// GetManifest returns the image's manifest along with its MIME type (which may be empty when it can't be determined but the manifest is available).
// It may use a remote (= slow) service.
func (s *ociImageSource) GetManifest() ([]byte, string, error) {
descriptorPath := s.ref.descriptorPath(s.ref.tag)
data, err := ioutil.ReadFile(descriptorPath)
if err != nil {
return nil, "", err
}

desc := imgspecv1.Descriptor{}
err = json.Unmarshal(data, &desc)
if err != nil {
return nil, "", err
}

manifestPath, err := s.ref.blobPath(digest.Digest(desc.Digest))
manifestPath, err := s.ref.blobPath(digest.Digest(s.descriptor.Digest))
if err != nil {
return nil, "", err
}
Expand All @@ -54,7 +46,7 @@ func (s *ociImageSource) GetManifest() ([]byte, string, error) {
return nil, "", err
}

return m, desc.MediaType, nil
return m, s.descriptor.MediaType, nil
}

func (s *ociImageSource) GetTargetManifest(digest digest.Digest) ([]byte, string, error) {
Expand Down
Loading