From 8fd0d71d7e4b03a815f2fb86b19ef2a71a1b7ce0 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Mon, 10 Jun 2019 19:03:58 -0700 Subject: [PATCH 1/5] Add test for copying entire container rootfs CID=$(docker create alpine) docker cp $CID:/ out Signed-off-by: Brian Goff Signed-off-by: Tibor Vass (cherry picked from commit 6db9f1c3d6e9ad634554cacaf197a435efcf8833) Signed-off-by: Sebastiaan van Stijn --- integration/container/copy_test.go | 82 ++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/integration/container/copy_test.go b/integration/container/copy_test.go index 9c5c5ce297317..58355b0921cad 100644 --- a/integration/container/copy_test.go +++ b/integration/container/copy_test.go @@ -1,13 +1,20 @@ package container // import "github.com/docker/docker/integration/container" import ( + "archive/tar" "context" + "encoding/json" "fmt" + "io" + "io/ioutil" + "os" "testing" "github.com/docker/docker/api/types" "github.com/docker/docker/client" "github.com/docker/docker/integration/internal/container" + "github.com/docker/docker/internal/test/fakecontext" + "github.com/docker/docker/pkg/jsonmessage" "gotest.tools/assert" is "gotest.tools/assert/cmp" "gotest.tools/skip" @@ -64,3 +71,78 @@ func TestCopyToContainerPathIsNotDir(t *testing.T) { err := apiclient.CopyToContainer(ctx, cid, "/etc/passwd/", nil, types.CopyToContainerOptions{}) assert.Assert(t, is.ErrorContains(err, "not a directory")) } + +func TestCopyFromContainerRoot(t *testing.T) { + skip.If(t, testEnv.DaemonInfo.OSType == "windows") + defer setupTest(t)() + + ctx := context.Background() + apiClient := testEnv.APIClient() + + dir, err := ioutil.TempDir("", t.Name()) + assert.NilError(t, err) + defer os.RemoveAll(dir) + + buildCtx := fakecontext.New(t, dir, fakecontext.WithFile("foo", "hello"), fakecontext.WithFile("baz", "world"), fakecontext.WithDockerfile(` + FROM scratch + COPY foo /foo + COPY baz /bar/baz + CMD /fake + `)) + defer buildCtx.Close() + + resp, err := apiClient.ImageBuild(ctx, buildCtx.AsTarReader(t), types.ImageBuildOptions{}) + assert.NilError(t, err) + defer resp.Body.Close() + + var imageID string + err = jsonmessage.DisplayJSONMessagesStream(resp.Body, ioutil.Discard, 0, false, func(msg jsonmessage.JSONMessage) { + var r types.BuildResult + assert.NilError(t, json.Unmarshal(*msg.Aux, &r)) + imageID = r.ID + }) + assert.NilError(t, err) + assert.Assert(t, imageID != "") + + cid := container.Create(ctx, t, apiClient, container.WithImage(imageID)) + + rdr, _, err := apiClient.CopyFromContainer(ctx, cid, "/") + assert.NilError(t, err) + defer rdr.Close() + + tr := tar.NewReader(rdr) + expect := map[string]string{ + "/foo": "hello", + "/bar/baz": "world", + } + found := make(map[string]bool, 2) + var numFound int + for { + h, err := tr.Next() + if err == io.EOF { + break + } + assert.NilError(t, err) + + expected, exists := expect[h.Name] + if !exists { + // this archive will have extra stuff in it since we are copying from root + // and docker adds a bunch of stuff + continue + } + + numFound++ + found[h.Name] = true + + buf, err := ioutil.ReadAll(tr) + assert.NilError(t, err) + assert.Check(t, is.Equal(string(buf), expected)) + + if numFound == len(expect) { + break + } + } + + assert.Check(t, found["/foo"], "/foo file not found in archive") + assert.Check(t, found["/bar/baz"], "/bar/baz file not found in archive") +} From c820334ff3e048d1207a63ba9d5b9d11e38f649d Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Thu, 13 Jun 2019 05:36:21 +0000 Subject: [PATCH 2/5] add more tests Signed-off-by: Tibor Vass (cherry picked from commit 02f1eb89a42b4a9e91a8c80c213f7dc3193c75b9) Signed-off-by: Sebastiaan van Stijn --- integration/container/copy_test.go | 97 +++++++++++++++++------------- 1 file changed, 56 insertions(+), 41 deletions(-) diff --git a/integration/container/copy_test.go b/integration/container/copy_test.go index 58355b0921cad..218b5759215cc 100644 --- a/integration/container/copy_test.go +++ b/integration/container/copy_test.go @@ -72,7 +72,7 @@ func TestCopyToContainerPathIsNotDir(t *testing.T) { assert.Assert(t, is.ErrorContains(err, "not a directory")) } -func TestCopyFromContainerRoot(t *testing.T) { +func TestCopyFromContainer(t *testing.T) { skip.If(t, testEnv.DaemonInfo.OSType == "windows") defer setupTest(t)() @@ -84,9 +84,10 @@ func TestCopyFromContainerRoot(t *testing.T) { defer os.RemoveAll(dir) buildCtx := fakecontext.New(t, dir, fakecontext.WithFile("foo", "hello"), fakecontext.WithFile("baz", "world"), fakecontext.WithDockerfile(` - FROM scratch + FROM busybox COPY foo /foo - COPY baz /bar/baz + COPY baz /bar/quux/baz + RUN ln -s notexist /bar/notarget && ln -s quux/baz /bar/filesymlink && ln -s quux /bar/dirsymlink && ln -s / /bar/root CMD /fake `)) defer buildCtx.Close() @@ -106,43 +107,57 @@ func TestCopyFromContainerRoot(t *testing.T) { cid := container.Create(ctx, t, apiClient, container.WithImage(imageID)) - rdr, _, err := apiClient.CopyFromContainer(ctx, cid, "/") - assert.NilError(t, err) - defer rdr.Close() - - tr := tar.NewReader(rdr) - expect := map[string]string{ - "/foo": "hello", - "/bar/baz": "world", + for _, x := range []struct { + src string + expect map[string]string + }{ + {"/", map[string]string{"/": "", "/foo": "hello", "/bar/quux/baz": "world", "/bar/filesymlink": "", "/bar/dirsymlink": "", "/bar/notarget": ""}}, + {"/bar/root", map[string]string{"root": ""}}, + {"/bar/root/", map[string]string{"root/": "", "root/foo": "hello", "root/bar/quux/baz": "world", "root/bar/filesymlink": "", "root/bar/dirsymlink": "", "root/bar/notarget": ""}}, + + {"bar/quux", map[string]string{"quux/": "", "quux/baz": "world"}}, + {"bar/quux/", map[string]string{"quux/": "", "quux/baz": "world"}}, + {"bar/quux/baz", map[string]string{"baz": "world"}}, + + {"bar/filesymlink", map[string]string{"filesymlink": ""}}, + {"bar/dirsymlink", map[string]string{"dirsymlink": ""}}, + {"bar/dirsymlink/", map[string]string{"dirsymlink/": "", "dirsymlink/baz": "world"}}, + {"bar/notarget", map[string]string{"notarget": ""}}, + } { + t.Run(x.src, func(t *testing.T) { + rdr, _, err := apiClient.CopyFromContainer(ctx, cid, x.src) + assert.NilError(t, err) + defer rdr.Close() + + found := make(map[string]bool, len(x.expect)) + var numFound int + tr := tar.NewReader(rdr) + for numFound < len(x.expect) { + h, err := tr.Next() + if err == io.EOF { + break + } + assert.NilError(t, err) + + expected, exists := x.expect[h.Name] + if !exists { + // this archive will have extra stuff in it since we are copying from root + // and docker adds a bunch of stuff + continue + } + + numFound++ + found[h.Name] = true + + buf, err := ioutil.ReadAll(tr) + if err == nil { + assert.Check(t, is.Equal(string(buf), expected)) + } + } + + for f := range x.expect { + assert.Check(t, found[f], f+" not found in archive") + } + }) } - found := make(map[string]bool, 2) - var numFound int - for { - h, err := tr.Next() - if err == io.EOF { - break - } - assert.NilError(t, err) - - expected, exists := expect[h.Name] - if !exists { - // this archive will have extra stuff in it since we are copying from root - // and docker adds a bunch of stuff - continue - } - - numFound++ - found[h.Name] = true - - buf, err := ioutil.ReadAll(tr) - assert.NilError(t, err) - assert.Check(t, is.Equal(string(buf), expected)) - - if numFound == len(expect) { - break - } - } - - assert.Check(t, found["/foo"], "/foo file not found in archive") - assert.Check(t, found["/bar/baz"], "/bar/baz file not found in archive") } From 70a837138aa1c5c0260a005b470ac0c5717984b4 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Tue, 11 Jun 2019 01:55:38 +0000 Subject: [PATCH 3/5] daemon: fix docker cp when container source is / Before 7a7357da, archive.TarResourceRebase was being used to copy files and folders from the container. That function splits the source path into a dirname + basename pair to support copying a file: if you wanted to tar `dir/file` it would tar from `dir` the file `file` (as part of the IncludedFiles option). However, that path splitting logic was kept for folders as well, which resulted in weird inputs to archive.TarWithOptions: if you wanted to tar `dir1/dir2` it would tar from `dir1` the directory `dir2` (as part of IncludedFiles option). Although it was weird, it worked fine until we started chrooting into the container rootfs when doing a `docker cp` with container source set to `/` (cf 3029e765). The fix is to only do the path splitting logic if the source is a file. Unfortunately, 7a7357da added support for LCOW by duplicating some of this subtle logic. Ideally we would need to do more refactoring of the archive codebase to properly encapsulate these behaviors behind well- documented APIs. This fix does not do that. Instead, it fixes the issue inline. Signed-off-by: Tibor Vass (cherry picked from commit 171538c190ee3a1a8211946ab8fa78cdde54b47a) Signed-off-by: Sebastiaan van Stijn --- daemon/archive.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/daemon/archive.go b/daemon/archive.go index 109376b4b566e..21339cfb68c35 100644 --- a/daemon/archive.go +++ b/daemon/archive.go @@ -236,7 +236,13 @@ func (daemon *Daemon) containerArchivePath(container *container.Container, path if driver.Base(resolvedPath) == "." { resolvedPath += string(driver.Separator()) + "." } - sourceDir, sourceBase := driver.Dir(resolvedPath), driver.Base(resolvedPath) + + sourceDir := resolvedPath + sourceBase := "." + + if stat.Mode&os.ModeDir == 0 { // not dir + sourceDir, sourceBase = driver.Split(resolvedPath) + } opts := archive.TarResourceRebaseOpts(sourceBase, driver.Base(absPath)) data, err := archivePath(driver, sourceDir, opts, container.BaseFS.Path()) @@ -426,9 +432,6 @@ func (daemon *Daemon) containerCopy(container *container.Container, resource str d, f := driver.Split(basePath) basePath = d filter = []string{f} - } else { - filter = []string{driver.Base(basePath)} - basePath = driver.Dir(basePath) } archive, err := archivePath(driver, basePath, &archive.TarOptions{ Compression: archive.Uncompressed, From 8677bbe3f31a8e4516e1ba413ce1063c803ea10c Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Wed, 12 Jun 2019 23:01:04 +0000 Subject: [PATCH 4/5] pkg/archive: keep walkRoot clean if source is / Previously, getWalkRoot("/", "foo") would return "//foo" Now it returns "/foo" Signed-off-by: Tibor Vass (cherry picked from commit 7410f1a859063d4ed3d8fca44f27bdde4c2cb5a3) Signed-off-by: Sebastiaan van Stijn --- pkg/archive/archive_unix.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/archive/archive_unix.go b/pkg/archive/archive_unix.go index 1eec912b7b0b6..d626336032ba8 100644 --- a/pkg/archive/archive_unix.go +++ b/pkg/archive/archive_unix.go @@ -7,6 +7,7 @@ import ( "errors" "os" "path/filepath" + "strings" "syscall" "github.com/docker/docker/pkg/idtools" @@ -26,7 +27,7 @@ func fixVolumePathPrefix(srcPath string) string { // can't use filepath.Join(srcPath,include) because this will clean away // a trailing "." or "/" which may be important. func getWalkRoot(srcPath string, include string) string { - return srcPath + string(filepath.Separator) + include + return strings.TrimSuffix(srcPath, string(filepath.Separator)) + string(filepath.Separator) + include } // CanonicalTarNameForPath returns platform-specific filepath From 584c0857ab21895e62feac686448085113c6c977 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Fri, 14 Jun 2019 18:23:21 +0000 Subject: [PATCH 5/5] integration: have container.Create call compile For reference on why this is needed: https://github.com/docker/engine/pull/280#issuecomment-502056661 Signed-off-by: Tibor Vass (cherry picked from commit 8f4b96f19e64b96df9d8c43208cefb113715ccbf) Signed-off-by: Sebastiaan van Stijn --- integration/container/copy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/container/copy_test.go b/integration/container/copy_test.go index 218b5759215cc..9020b802f3884 100644 --- a/integration/container/copy_test.go +++ b/integration/container/copy_test.go @@ -105,7 +105,7 @@ func TestCopyFromContainer(t *testing.T) { assert.NilError(t, err) assert.Assert(t, imageID != "") - cid := container.Create(ctx, t, apiClient, container.WithImage(imageID)) + cid := container.Create(t, ctx, apiClient, container.WithImage(imageID)) for _, x := range []struct { src string