From 2bf8bffc5b9f7d33cecef263a213a632f3ec3ee8 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 Upstream-commit: 8fd0d71d7e4b03a815f2fb86b19ef2a71a1b7ce0 Component: engine --- .../engine/integration/container/copy_test.go | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/components/engine/integration/container/copy_test.go b/components/engine/integration/container/copy_test.go index 9c5c5ce297..58355b0921 100644 --- a/components/engine/integration/container/copy_test.go +++ b/components/engine/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 9f18b94c1bb258f77bc63f0b86cbbfc98f115c3b 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 Upstream-commit: c820334ff3e048d1207a63ba9d5b9d11e38f649d Component: engine --- .../engine/integration/container/copy_test.go | 95 +++++++++++-------- 1 file changed, 55 insertions(+), 40 deletions(-) diff --git a/components/engine/integration/container/copy_test.go b/components/engine/integration/container/copy_test.go index 58355b0921..218b575921 100644 --- a/components/engine/integration/container/copy_test.go +++ b/components/engine/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() + 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": ""}}, - tr := tar.NewReader(rdr) - expect := map[string]string{ - "/foo": "hello", - "/bar/baz": "world", + {"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 36f18a1f78c7012cd24954b9feafcf707eabd5aa 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 Upstream-commit: 70a837138aa1c5c0260a005b470ac0c5717984b4 Component: engine --- components/engine/daemon/archive.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/components/engine/daemon/archive.go b/components/engine/daemon/archive.go index 109376b4b5..21339cfb68 100644 --- a/components/engine/daemon/archive.go +++ b/components/engine/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 6532144af4c9d13e4e7a3909a831ef488dc5e2d1 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 Upstream-commit: 8677bbe3f31a8e4516e1ba413ce1063c803ea10c Component: engine --- components/engine/pkg/archive/archive_unix.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/components/engine/pkg/archive/archive_unix.go b/components/engine/pkg/archive/archive_unix.go index 1eec912b7b..d626336032 100644 --- a/components/engine/pkg/archive/archive_unix.go +++ b/components/engine/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 b0ff7b744e06879d10601947a1adf599f34275a2 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 Upstream-commit: 584c0857ab21895e62feac686448085113c6c977 Component: engine --- components/engine/integration/container/copy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/engine/integration/container/copy_test.go b/components/engine/integration/container/copy_test.go index 218b575921..9020b802f3 100644 --- a/components/engine/integration/container/copy_test.go +++ b/components/engine/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