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

layout: implement unpacking and validation #82

Merged
merged 1 commit into from
May 31, 2016
Merged

layout: implement unpacking and validation #82

merged 1 commit into from
May 31, 2016

Conversation

s-urbaniak
Copy link
Collaborator

@s-urbaniak s-urbaniak commented May 22, 2016 •
edited
Loading

Fixes #75

Signed-off-by: Sergiusz Urbaniak sur@coreos.com

@s-urbaniak s-urbaniak force-pushed the layout branch 7 times, most recently from bd4afcd to 4aa9e1f Compare May 22, 2016 17:10
@s-urbaniak
Copy link
Collaborator Author

s-urbaniak commented May 22, 2016 •
edited
Loading

Blocked by #81: I need the config validator merged to thoroughly validate the image/image layout.

@philips
Copy link
Contributor

philips commented May 24, 2016

@s-urbaniak #81 is merged, what is the next step?

@s-urbaniak
Copy link
Collaborator Author

@philips implementing image layout validation now, should be ready by EOD

@philips philips added this to the v0.2.0 milestone May 24, 2016
@s-urbaniak s-urbaniak changed the title [WIP] layout: unpacking initial commit layout: implement unpacking and validation May 25, 2016
@s-urbaniak
Copy link
Collaborator Author

@philips @vbatts @stevvooe PTAL; I implemented this using self-made image available here https://github.com/s-urbaniak/oci-image-test.

@philips
Copy link
Contributor

philips commented May 25, 2016

@s-urbaniak Are you generating the OCI Runtime config.json? https://github.com/opencontainers/runtime-spec/blob/master/config.md We need to generate that from the serialization.config thingie.

@s-urbaniak
Copy link
Collaborator Author

@philips nope, as discussed OOB, this is just "pure" unpacking. The runtime config.json would have to be solved in a separate PR.

@s-urbaniak s-urbaniak mentioned this pull request May 25, 2016
Closed
@stevvooe
Copy link
Contributor

@s-urbaniak I'm looking at this today. Sorry for the delay.

philips
philips reviewed May 25, 2016
View reviewed changes
image/unpack.go
info := hdr.FileInfo()

if strings.HasPrefix(info.Name(), `.wh.`) {
path = strings.Replace(path, `.wh.`, "", 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to use backtick instead of regular quotes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no reason, just too fast typing ;-) fixed

@stevvooe
Copy link
Contributor

@s-urbaniak This is looking very solid. Is there a corresponding specification for this format?

The only large change I'd suggest is that rather than manifests, this should be refs. These files should simply contain a descriptor pointing to the manifest blob. This will allow us to avoid the half-deserialized manifests.

stevvooe
stevvooe reviewed May 26, 2016
View reviewed changes
image/unpack.go
return nil // ignore
}

if err := extractLayer(dest, r); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we unpack directly to disk, these layers really need to be verified against the manifest's digest.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@philips
Copy link
Contributor

philips commented May 26, 2016

@stevvooe what do you mean by "avoid the half-deserialized manifests"?

@philips
Copy link
Contributor

philips commented May 26, 2016

@stevvooe are you thinking something like this for the refs schema:

$ cat refs/v1.0
{
  "media-types": {
    "application/vnd.oci.image.manifest.v1+json": {"size": "4096", "digest": "sha256:afff3928"
  }
}

@s-urbaniak
Copy link
Collaborator Author

@stevvooe @philips thanks a lot for the review! just fyi: I'm sitting between flights though so won't make to implement changes before tomorrow.

stevvooe
stevvooe reviewed May 26, 2016
View reviewed changes
cmd/oci-image-tool/autodetect.go

// supported autodetection types
const (
typeImageLayout = "imageLayout"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use the mediatypes directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this gives me finer grained control, i.e. typeImageLayout, and typeImage don't have media types at all and must be treated differently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course!

@stevvooe
Copy link
Contributor

@philips I really referring to https://github.com/opencontainers/image-spec/pull/82/files#diff-1d4f0c730d731e154e77caf8c8ce36a4R80.

But, I was thinking about something like this:

$ cat refs/v1.0
{"size": 4096, "digest": "sha256:afff3928", "mediatype": "application/vnd.oci.image.manifest.v1+json"}

@philips
Copy link
Contributor

philips commented May 26, 2016

@stevvooe I think we need an array of media types though as we need a way of emulating Accept with a filesystem. What if my ref is serving a v1 and v1.1 manifest? Let me file a new issue on this.

@wking
Copy link
Contributor

wking commented May 26, 2016

On Thu, May 26, 2016 at 02:14:36PM -0700, Brandon Philips wrote:

What if my ref is serving a v1 and v1.1 manifest?

Make the root reference point to a manifest.list 1?

@philips
Copy link
Contributor

philips commented May 26, 2016

@wking I think that is the approach we are going to take. Let me work up a proposal.

@philips
Copy link
Contributor

philips commented May 27, 2016

@stevvooe here is a concrete format specification: #94

philips
philips reviewed May 27, 2016
View reviewed changes
image/unpack.go
}

func diffIDs(w walker, manifest string) ([]string, error) {
manifest = filepath.Join("manifests", manifest)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you change this code to follow the spec here: #94

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@s-urbaniak
Copy link
Collaborator Author

@stevvooe @philips refactored this towards the proposed layout spec in #94.

@s-urbaniak
layout: implement unpacking and validation
d1874b6 
Fixes opencontainers#75

Signed-off-by: Sergiusz Urbaniak <sur@coreos.com>
@philips
Copy link
Contributor

philips commented May 27, 2016

Overall LGTM for a starting point.

Anyone else give this an LGTM @opencontainers/image-spec-maintainers

@jonboulle
Copy link
Contributor

LGTM

@jonboulle jonboulle merged commit 3468157 into opencontainers:master May 31, 2016
@cyphar cyphar mentioned this pull request Jun 8, 2016
Closed
@wking wking mentioned this pull request Jun 14, 2016
Merged
@wking wking mentioned this pull request Jul 22, 2016
Merged
@philips philips mentioned this pull request Sep 21, 2016
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
component/oci-image-tool priority/P0
Projects
None yet
Milestone
v0.2.0
Development

Successfully merging this pull request may close these issues.
5 participants
@s-urbaniak @philips @stevvooe @wking @jonboulle