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.

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

Fix broken oci-image-tool #177

Merged
merged 5 commits into from
Jul 29, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .tool/lint
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ for d in $(find . -type d -not -iwholename '*.git*' -a -not -iname '.tool' -a -n
--exclude='schema/fs.go' \
--disable=aligncheck \
--disable=gotype \
--cyclo-over=20 \
--cyclo-over=35 \
--tests \
--deadline=10s "${d}"
done
5 changes: 5 additions & 0 deletions cmd/oci-image-tool/create_runtime_bundle.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@ func (v *bundleCmd) Run(cmd *cobra.Command, args []string) {
os.Exit(1)
}

if _, err := os.Stat(args[1]); os.IsNotExist(err) {
v.stderr.Printf("destination path %s does not exist", args[1])
os.Exit(1)
}

if v.typ == "" {
typ, err := autodetect(args[0])
if err != nil {
Expand Down
19 changes: 14 additions & 5 deletions image/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ type config struct {

func findConfig(w walker, d *descriptor) (*config, error) {
var c config
cpath := filepath.Join("blobs", d.Digest)
cpath := filepath.Join("blobs", d.normalizeDigest())

f := func(path string, info os.FileInfo, r io.Reader) error {
if info.IsDir() {
Expand Down Expand Up @@ -97,12 +97,21 @@ func (c *config) runtimeSpec(rootfs string) (*specs.Spec, error) {

var s specs.Spec
s.Version = "0.5.0"
// we should at least apply the default spec, otherwise this is totally useless
s.Process.Terminal = true
s.Root.Path = rootfs
s.Process.Cwd = c.Config.WorkingDir
s.Process.Env = append([]string(nil), c.Config.Env...)
s.Process.Args = append([]string(nil), c.Config.Entrypoint...)
s.Process.Cwd = "/"
if c.Config.WorkingDir != "" {
s.Process.Cwd = c.Config.WorkingDir
}
s.Process.Env = append(s.Process.Env, c.Config.Env...)
s.Process.Args = append(s.Process.Args, c.Config.Entrypoint...)
s.Process.Args = append(s.Process.Args, c.Config.Cmd...)

if len(s.Process.Args) == 0 {
s.Process.Args = append(s.Process.Args, "sh")
Copy link
Collaborator

Choose a reason for hiding this comment

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

is sh spec'ed as a default?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Copy link
Member Author

Choose a reason for hiding this comment

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

ocitools is also merging a library which will ease all of this and painlessly merge the image's config and the default one.

}

if uid, err := strconv.Atoi(c.Config.User); err == nil {
s.Process.User.UID = uint32(uid)
} else if ug := strings.Split(c.Config.User, ":"); len(ug) == 2 {
Expand All @@ -118,7 +127,7 @@ func (c *config) runtimeSpec(rootfs string) (*specs.Spec, error) {

s.Process.User.UID = uint32(uid)
s.Process.User.GID = uint32(gid)
} else {
} else if c.Config.User != "" {
return nil, errors.New("config.User: unsupported format")
}

Expand Down
11 changes: 8 additions & 3 deletions image/descriptor.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"io"
"os"
"path/filepath"
"strings"

"github.com/pkg/errors"
)
Expand All @@ -32,6 +33,10 @@ type descriptor struct {
Size int64 `json:"size"`
}

func (d *descriptor) normalizeDigest() string {
return strings.Replace(d.Digest, ":", "-", -1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rethink the method name, maybe normalizeDigest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this actually doesn't need to be attached as a receiver method on *descriptor, but just accept a string.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this way since it's something on the descriptor itself...

}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a dup of #165, and I'd still rather fix it by getting #159 landed ;).

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, but oci-image-tool isn't working at all w/o this fix and your #159 it's a rather bigger PR with a broader scope, I'd love to have this merged so people can use this tool instead of waiting on #159

Copy link
Contributor

Choose a reason for hiding this comment

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

On Fri, Jul 22, 2016 at 07:18:32AM -0700, Antonio Murdaca wrote:

+func (d *descriptor) normalizeDigest() string {

  • return strings.Replace(d.Digest, ":", "-", -1)
    +}

I see, but oci-image-tool isn't working at all w/o this fix and
your #159 it's a rather bigger PR with a broader scope, I'd love to
have this merged so people can use this tool instead of waiting on
#159

I'm not sure what's holding #159 up, it's been out there for a while
;). So I'm not sure what timeframe to expect before people can use it
(although folks are certainly free to use it now, without waiting on
the branch to land in this repo ;).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what's holding #159 up, it's been out there for a while
;). So I'm not sure what timeframe to expect before people can use it
(although folks are certainly free to use it now, without waiting on
the branch to land in this repo ;).

I don't know either - just that you PR is 1000+ loc changed - while this one is just pulling in 10 additions (0e8f74b) to fix a fatal bug.

I believe this should go in asap. Potentially #159 can take a lot more time to be reviewed (not maintainers reviewed it yet)

Copy link
Contributor

Choose a reason for hiding this comment

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

On Fri, Jul 22, 2016 at 07:52:32AM -0700, Antonio Murdaca wrote:

I believe this should go in asap. Potentially #159 can take a lot
more time to be reviewed (not maintainers reviewed it yet)

I think it would be more efficient to put better abstractions into
place as soon as possible, instead of iterating on an approach we
expect to replace. But that's a throughput vs. latency issue for the
maintainers to figure out ;).

Copy link
Member Author

Choose a reason for hiding this comment

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

the point now is that, unfortunately this tool is broken and it's broken since a lot now probably (#165 is 28 days old why was that closed)
Anyhow I'm ok either way...

Copy link
Contributor

Choose a reason for hiding this comment

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

On Fri, Jul 22, 2016 at 08:09:47AM -0700, Antonio Murdaca wrote:

the point now is that, unfortunately this tool is broken and it's
broken since a lot now…

It's been broken (on this semicolon point) since it was born, because
the implementation was merged in #82 well before the spec landed in
#94. That means that nobody has ever used it for spec-compliant
unpacking, which takes the pressure off (in my opinion) for fixing it
ASAP.


func findDescriptor(w walker, name string) (*descriptor, error) {
var d descriptor
dpath := filepath.Join("refs", name)
Expand Down Expand Up @@ -71,7 +76,7 @@ func (d *descriptor) validate(w walker) error {
}

digest, err := filepath.Rel("blobs", filepath.Clean(path))
if err != nil || d.Digest != digest {
if err != nil || d.normalizeDigest() != digest {
return nil // ignore
}

Expand All @@ -84,11 +89,11 @@ func (d *descriptor) validate(w walker) error {

switch err := w.walk(f); err {
case nil:
return fmt.Errorf("%s: not found", d.Digest)
return fmt.Errorf("%s: not found", d.normalizeDigest())
case errEOW:
// found, continue below
default:
return errors.Wrapf(err, "%s: validation failed", d.Digest)
return errors.Wrapf(err, "%s: validation failed", d.normalizeDigest())
}

return nil
Expand Down
50 changes: 41 additions & 9 deletions image/manifest.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ type manifest struct {

func findManifest(w walker, d *descriptor) (*manifest, error) {
var m manifest
mpath := filepath.Join("blobs", d.Digest)
mpath := filepath.Join("blobs", d.normalizeDigest())

f := func(path string, info os.FileInfo, r io.Reader) error {
if info.IsDir() {
Expand Down Expand Up @@ -107,7 +107,7 @@ func (m *manifest) unpack(w walker, dest string) error {
}

dd, err := filepath.Rel("blobs", filepath.Clean(path))
if err != nil || d.Digest != dd {
if err != nil || d.normalizeDigest() != dd {
return nil // ignore
}

Expand All @@ -134,6 +134,7 @@ func unpackLayer(dest string, r io.Reader) error {
}
defer gz.Close()

var dirs []*tar.Header
tr := tar.NewReader(gz)

loop:
Expand All @@ -148,8 +149,26 @@ loop:
return errors.Wrapf(err, "error advancing tar stream")
}

path := filepath.Join(dest, filepath.Clean(hdr.Name))
hdr.Name = filepath.Clean(hdr.Name)
if !strings.HasSuffix(hdr.Name, string(os.PathSeparator)) {
// Not the root directory, ensure that the parent directory exists
parent := filepath.Dir(hdr.Name)
parentPath := filepath.Join(dest, parent)
if _, err2 := os.Lstat(parentPath); err2 != nil && os.IsNotExist(err2) {
if err3 := os.MkdirAll(parentPath, 0777); err3 != nil {
return err3
}
}
}
path := filepath.Join(dest, hdr.Name)
rel, err := filepath.Rel(dest, path)
if err != nil {
return err
}
info := hdr.FileInfo()
if strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
return fmt.Errorf("%q is outside of %q", hdr.Name, dest)
}

if strings.HasPrefix(info.Name(), ".wh.") {
path = strings.Replace(path, ".wh.", "", 1)
Expand All @@ -163,12 +182,14 @@ loop:

switch hdr.Typeflag {
case tar.TypeDir:
if err := os.MkdirAll(path, info.Mode()); err != nil {
return errors.Wrap(err, "error creating directory")
if fi, err := os.Lstat(path); !(err == nil && fi.IsDir()) {
if err2 := os.MkdirAll(path, info.Mode()); err2 != nil {
return errors.Wrap(err2, "error creating directory")
}
}

case tar.TypeReg, tar.TypeRegA:
f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, info.Mode())
f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, info.Mode())
if err != nil {
return errors.Wrap(err, "unable to open file")
}
Expand Down Expand Up @@ -200,13 +221,24 @@ loop:
if err := os.Symlink(hdr.Linkname, path); err != nil {
return err
}

case tar.TypeXGlobalHeader:
return nil
}
// Directory mtimes must be handled at the end to avoid further
// file creation in them to modify the directory mtime
if hdr.Typeflag == tar.TypeDir {
dirs = append(dirs, hdr)
}
}
for _, hdr := range dirs {
path := filepath.Join(dest, hdr.Name)

if err := os.Chtimes(path, time.Now().UTC(), info.ModTime()); err != nil {
finfo := hdr.FileInfo()
// I believe the old version was using time.Now().UTC() to overcome an
// invalid error from chtimes.....but here we lose hdr.AccessTime like this...
if err := os.Chtimes(path, time.Now().UTC(), finfo.ModTime()); err != nil {
return errors.Wrap(err, "error changing time")
}
}

return nil
}