From 6db9f1c3d6e9ad634554cacaf197a435efcf8833 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Mon, 10 Jun 2019 19:03:58 -0700 Subject: [PATCH 1/4] 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 --- 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 1955f770ba..184a968c1f 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 02f1eb89a42b4a9e91a8c80c213f7dc3193c75b9 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Thu, 13 Jun 2019 05:36:21 +0000 Subject: [PATCH 2/4] add more tests Signed-off-by: Tibor Vass --- integration/container/copy_test.go | 95 +++++++++++++++++------------- 1 file changed, 55 insertions(+), 40 deletions(-) diff --git a/integration/container/copy_test.go b/integration/container/copy_test.go index 184a968c1f..0b6123a80b 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() + 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 171538c190ee3a1a8211946ab8fa78cdde54b47a Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Tue, 11 Jun 2019 01:55:38 +0000 Subject: [PATCH 3/4] 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 --- daemon/archive.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/daemon/archive.go b/daemon/archive.go index 109376b4b5..21339cfb68 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 7410f1a859063d4ed3d8fca44f27bdde4c2cb5a3 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Wed, 12 Jun 2019 23:01:04 +0000 Subject: [PATCH 4/4] pkg/archive: keep walkRoot clean if source is / Previously, getWalkRoot("/", "foo") would return "//foo" Now it returns "/foo" Signed-off-by: Tibor Vass --- 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 1eec912b7b..d626336032 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