add --keep-dirlinks option to unpack #245

tych0 · 2018-06-26T23:36:33Z

Similar to rsync's --keep-dirlinks, let's have an option to allow symlinks
that point to directories to be used as directories themselves.

Signed-off-by: Tycho Andersen tycho@tycho.ws

tych0 · 2018-07-18T19:36:31Z

Ping, just occurred to me that I hadn't heard anything on this.

cyphar

I'm overall +1 on the idea, I just think that the conditional change is a bit hard to follow.

cyphar · 2018-07-19T17:49:01Z

cmd/umoci/unpack.go

-	Flags: []cli.Flag{},
+	Flags: []cli.Flag{
+		cli.BoolFlag{
+			Name:  "keep-dirlinks",


This flag should probably also be added to raw-unpack. And docs should be updated (it doesn't need much).

Cool, I pushed a commit for these.

cyphar · 2018-07-19T17:50:45Z

oci/layer/tar_extract.go

@@ -370,7 +396,13 @@ func (te *tarExtractor) unpackEntry(root string, hdr *tar.Header, r io.Reader) (
 	// "lower" file still exists (so the hard-link would point to the old
 	// inode). It's not clear if such an archive is actually valid though.
 	if !fi.IsDir() || hdr.Typeflag != tar.TypeDir {
-		if err := te.fsEval.RemoveAll(path); err != nil {
+		if fi.Mode()&os.ModeSymlink != 0 && hdr.Typeflag == tar.TypeDir &&
+			te.mapOptions.KeepDirlinks {


To me, this nested if looks a bit complicated (and is what stopped me from completing my review when you first sent the PR -- because I tried to come up with a more "clear" conditional and didn't get very far).

At the moment, I'm +1 on the design it just needs to be a bit less difficult to understand. I'll take another look and get back to you.

Sure, no worries. Perhaps a comment explaining that we want to write through symlinks instead of overwriting them is enough?

Ok, I added the comment (back from vacation now). If you merge this, I have the v2 api done on top of this and I can post that too.

tych0 · 2018-08-06T19:28:02Z

Ping, any thoughts on this?

tych0 · 2018-08-13T19:01:04Z

Ping, any thoughts on this?

hallyn · 2018-08-13T21:20:29Z

... could really use this...

cyphar · 2018-08-13T23:32:17Z

Needs a rebase, but I can do that. I don't really like the re-use of MapOptions for something unrelated to mapping -- but that just requires a struct type rename. I can fix that up later.

tych0 · 2018-08-15T21:11:01Z

Ok, I've rebased it. It turns out that renaming MapOptions isn't so easy, because the struct is emitted in UmociMeta, so we'd have to write some backwards compatible code for that. For now I just disabled it being emitted in the json, since it's not really relevant, and I'll let you decide how to handle it in the future as you say.

cyphar · 2018-08-16T02:17:02Z

oci/layer/tar_extract.go

+			te.mapOptions.KeepDirlinks {
+			isDirlink, err = te.isDirlink(root, path, hdr)
+			if err != nil {
+				return err


Needs an errors.Wrap(err, "check is dirlink").

cyphar · 2018-08-16T02:17:27Z

oci/layer/tar_extract.go

+		return false, errors.Wrap(err, "sanitize old target")
+	}
+
+	linkInfo, err := te.fsEval.Lstat(linkPath)


I just realised that this won't handle nested symlinks (a symlink to a symlink to a directory) properly -- we'd need to loop this logic and bail with an ELOOP. (I really wish we had openat(..., AT_THIS_ROOT) so we didn't have to write this logic in userspace.)

cyphar · 2018-08-16T02:18:03Z

test/unpack.bats

@@ -152,3 +152,35 @@ function teardown() {

 	image-verify "${IMAGE}"
 }
+
+@test "umoci unpack --keep-dirlinks" {


Once the nested symlink thing is fixed, I'd recommend adding a test for it.

tych0 · 2018-08-16T11:23:04Z

Ok, I fixed those comments too.

tych0 · 2018-08-17T16:32:48Z

Ok, my rebuild of this worked without http flakiness. I don't have perms to retrigger the travis build, but I think if you do it'll work and should be mergable.

cyphar

Aside from the small idiomatic comment, this looks fine. If I have push access to your branch, I'll send the fix-up.

cyphar · 2018-08-19T08:15:52Z

oci/layer/tar_extract.go

-		return false, errors.Wrap(err, "read existing link")
-	}
+func (te *tarExtractor) isDirlink(root string, path string) (bool, error) {
+	previous := []string{}


If you make this a map[string]struct{} it would probably be more "idiomatic" -- but I can push that on top of your branch.

cyphar · 2018-08-19T08:18:55Z

Yeah, Travis has been quite flaky recently. Not really sure what's going on there...

Similar to rsync's --keep-dirlinks, let's have an option to allow symlinks that point to directories to be used as directories themselves. Signed-off-by: Tycho Andersen <tycho@tycho.ws>

...also add some man page documentation for umoci unpack and umoci raw-unpack. Signed-off-by: Tycho Andersen <tycho@tycho.ws>

Signed-off-by: Tycho Andersen <tycho@tycho.ws>

* git-validation was not imported * the stock zypper image apparently doesn't have `which`, so the tests always failed. switch to `type` instead. Signed-off-by: Tycho Andersen <tycho@tycho.ws>

test coverage metrics are a waste of time. Signed-off-by: Tycho Andersen <tycho@tycho.ws>

cyphar · 2018-08-27T22:41:42Z

LGTM.

@cyphar

add some golang test code too! fix `make validate` handle nested symlinks + symlink loops wrap dirlink check error add clarifying comment about KeepDirlinks add --keep-dirlinks to umoci-raw-unpack add --keep-dirlinks option to unpack LGTMs: @cyphar Closes #245

cyphar · 2018-09-03T15:02:33Z

I was just re-reading the tests and it looks like they actually don't quite test --keep-dirlink. But don't worry, I'll work on this (it'll require having an umoci raw add-layer or something where we add a tar archive generated externally -- because umoci repack won't generate a dirlink and so testing it with just umoci repack won't quite work).

tych0 force-pushed the keep-dirlinks branch 4 times, most recently from 2974170 to 5d5d9b2 Compare June 27, 2018 15:59

cyphar reviewed Jul 19, 2018

View reviewed changes

tych0 force-pushed the keep-dirlinks branch 2 times, most recently from 5d3b7f3 to ef410ed Compare August 15, 2018 21:07

cyphar reviewed Aug 16, 2018

View reviewed changes

tych0 force-pushed the keep-dirlinks branch 2 times, most recently from 6b1a114 to 0bff8d0 Compare August 17, 2018 15:55

cyphar reviewed Aug 19, 2018

View reviewed changes

tych0 added 7 commits August 28, 2018 08:40

add --keep-dirlinks option to unpack

e08d95b

Similar to rsync's --keep-dirlinks, let's have an option to allow symlinks that point to directories to be used as directories themselves. Signed-off-by: Tycho Andersen <tycho@tycho.ws>

add --keep-dirlinks to umoci-raw-unpack

fc49ffe

...also add some man page documentation for umoci unpack and umoci raw-unpack. Signed-off-by: Tycho Andersen <tycho@tycho.ws>

add clarifying comment about KeepDirlinks

fdd5277

Signed-off-by: Tycho Andersen <tycho@tycho.ws>

wrap dirlink check error

a0b27d9

Signed-off-by: Tycho Andersen <tycho@tycho.ws>

handle nested symlinks + symlink loops

6d74142

Signed-off-by: Tycho Andersen <tycho@tycho.ws>

fix make validate

26f39a5

* git-validation was not imported * the stock zypper image apparently doesn't have `which`, so the tests always failed. switch to `type` instead. Signed-off-by: Tycho Andersen <tycho@tycho.ws>

add some golang test code too!

cc0b5e1

test coverage metrics are a waste of time. Signed-off-by: Tycho Andersen <tycho@tycho.ws>

cyphar force-pushed the keep-dirlinks branch from 0bff8d0 to cc0b5e1 Compare August 27, 2018 22:40

cyphar merged commit cc0b5e1 into opencontainers:master Aug 28, 2018

cyphar added this to the 0.4.2 milestone Sep 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add --keep-dirlinks option to unpack #245

add --keep-dirlinks option to unpack #245

tych0 commented Jun 26, 2018

tych0 commented Jul 18, 2018

cyphar left a comment

cyphar Jul 19, 2018

tych0 Jul 19, 2018

cyphar Jul 20, 2018

cyphar Jul 19, 2018

tych0 Jul 19, 2018

tych0 Jul 31, 2018

tych0 commented Aug 6, 2018

tych0 commented Aug 13, 2018

hallyn commented Aug 13, 2018

cyphar commented Aug 13, 2018

tych0 commented Aug 15, 2018

cyphar Aug 16, 2018

cyphar Aug 16, 2018

cyphar Aug 16, 2018

tych0 commented Aug 16, 2018

tych0 commented Aug 17, 2018

cyphar left a comment

cyphar Aug 19, 2018

cyphar commented Aug 19, 2018

cyphar commented Aug 27, 2018

cyphar commented Sep 3, 2018

add --keep-dirlinks option to unpack #245

add --keep-dirlinks option to unpack #245

Conversation

tych0 commented Jun 26, 2018

tych0 commented Jul 18, 2018

cyphar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tych0 commented Aug 6, 2018

tych0 commented Aug 13, 2018

hallyn commented Aug 13, 2018

cyphar commented Aug 13, 2018

tych0 commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tych0 commented Aug 16, 2018

tych0 commented Aug 17, 2018

cyphar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar commented Aug 19, 2018

cyphar commented Aug 27, 2018

cyphar commented Sep 3, 2018