From b7b62ca26aaf50a97b5fe9dab00bd58fbcc9aff0 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 30 Oct 2017 17:52:08 -0400 Subject: [PATCH] Adding unit tests for cp Signed-off-by: Daniel Nephin --- cli/command/container/client_test.go | 28 ++++- cli/command/container/cp.go | 85 +++++++------- cli/command/container/cp_test.go | 160 +++++++++++++++++++++++++++ 3 files changed, 224 insertions(+), 49 deletions(-) create mode 100644 cli/command/container/cp_test.go diff --git a/cli/command/container/client_test.go b/cli/command/container/client_test.go index 875fee7049..9a17875c2d 100644 --- a/cli/command/container/client_test.go +++ b/cli/command/container/client_test.go @@ -12,12 +12,14 @@ import ( type fakeClient struct { client.Client - inspectFunc func(string) (types.ContainerJSON, error) - execInspectFunc func(execID string) (types.ContainerExecInspect, error) - execCreateFunc func(container string, config types.ExecConfig) (types.IDResponse, error) - createContainerFunc func(config *container.Config, hostConfig *container.HostConfig, networkingConfig *network.NetworkingConfig, containerName string) (container.ContainerCreateCreatedBody, error) - imageCreateFunc func(parentReference string, options types.ImageCreateOptions) (io.ReadCloser, error) - infoFunc func() (types.Info, error) + inspectFunc func(string) (types.ContainerJSON, error) + execInspectFunc func(execID string) (types.ContainerExecInspect, error) + execCreateFunc func(container string, config types.ExecConfig) (types.IDResponse, error) + createContainerFunc func(config *container.Config, hostConfig *container.HostConfig, networkingConfig *network.NetworkingConfig, containerName string) (container.ContainerCreateCreatedBody, error) + imageCreateFunc func(parentReference string, options types.ImageCreateOptions) (io.ReadCloser, error) + infoFunc func() (types.Info, error) + containerStatPathFunc func(container, path string) (types.ContainerPathStat, error) + containerCopyFromFunc func(container, srcPath string) (io.ReadCloser, types.ContainerPathStat, error) } func (f *fakeClient) ContainerInspect(_ context.Context, containerID string) (types.ContainerJSON, error) { @@ -71,3 +73,17 @@ func (f *fakeClient) Info(_ context.Context) (types.Info, error) { } return types.Info{}, nil } + +func (f *fakeClient) ContainerStatPath(_ context.Context, container, path string) (types.ContainerPathStat, error) { + if f.containerStatPathFunc != nil { + return f.containerStatPathFunc(container, path) + } + return types.ContainerPathStat{}, nil +} + +func (f *fakeClient) CopyFromContainer(_ context.Context, container, srcPath string) (io.ReadCloser, types.ContainerPathStat, error) { + if f.containerCopyFromFunc != nil { + return f.containerCopyFromFunc(container, srcPath) + } + return nil, types.ContainerPathStat{}, nil +} diff --git a/cli/command/container/cp.go b/cli/command/container/cp.go index 32427cc329..398917aae0 100644 --- a/cli/command/container/cp.go +++ b/cli/command/container/cp.go @@ -26,13 +26,17 @@ type copyOptions struct { type copyDirection int const ( - fromContainer copyDirection = (1 << iota) + fromContainer copyDirection = 1 << iota toContainer acrossContainers = fromContainer | toContainer ) type cpConfig struct { followLink bool + copyUIDGID bool + sourcePath string + destPath string + container string } // NewCopyCommand creates a new `docker cp` command @@ -65,58 +69,57 @@ func NewCopyCommand(dockerCli command.Cli) *cobra.Command { } flags := cmd.Flags() - flags.BoolVarP(&opts.followLink, "follow-link", "L", false, "Always follow symbol link in SRC_PATH") flags.BoolVarP(&opts.copyUIDGID, "archive", "a", false, "Archive mode (copy all uid/gid information)") - return cmd } func runCopy(dockerCli command.Cli, opts copyOptions) error { srcContainer, srcPath := splitCpArg(opts.source) - dstContainer, dstPath := splitCpArg(opts.destination) + destContainer, destPath := splitCpArg(opts.destination) + + copyConfig := cpConfig{ + followLink: opts.followLink, + copyUIDGID: opts.copyUIDGID, + sourcePath: srcPath, + destPath: destPath, + } var direction copyDirection if srcContainer != "" { direction |= fromContainer + copyConfig.container = srcContainer } - if dstContainer != "" { + if destContainer != "" { direction |= toContainer - } - - cpParam := &cpConfig{ - followLink: opts.followLink, + copyConfig.container = destContainer } ctx := context.Background() switch direction { case fromContainer: - return copyFromContainer(ctx, dockerCli, srcContainer, srcPath, dstPath, cpParam) + return copyFromContainer(ctx, dockerCli, copyConfig) case toContainer: - return copyToContainer(ctx, dockerCli, srcPath, dstContainer, dstPath, cpParam, opts.copyUIDGID) + return copyToContainer(ctx, dockerCli, copyConfig) case acrossContainers: - // Copying between containers isn't supported. return errors.New("copying between containers is not supported") default: - // User didn't specify any container. return errors.New("must specify at least one container source") } } -func statContainerPath(ctx context.Context, dockerCli command.Cli, containerName, path string) (types.ContainerPathStat, error) { - return dockerCli.Client().ContainerStatPath(ctx, containerName, path) -} - func resolveLocalPath(localPath string) (absPath string, err error) { if absPath, err = filepath.Abs(localPath); err != nil { return } - return archive.PreserveTrailingDotOrSeparator(absPath, localPath, filepath.Separator), nil } -func copyFromContainer(ctx context.Context, dockerCli command.Cli, srcContainer, srcPath, dstPath string, cpParam *cpConfig) (err error) { +func copyFromContainer(ctx context.Context, dockerCli command.Cli, copyConfig cpConfig) (err error) { + dstPath := copyConfig.destPath + srcPath := copyConfig.sourcePath + if dstPath != "-" { // Get an absolute destination path. dstPath, err = resolveLocalPath(dstPath) @@ -125,10 +128,11 @@ func copyFromContainer(ctx context.Context, dockerCli command.Cli, srcContainer, } } + client := dockerCli.Client() // if client requests to follow symbol link, then must decide target file to be copied var rebaseName string - if cpParam.followLink { - srcStat, err := statContainerPath(ctx, dockerCli, srcContainer, srcPath) + if copyConfig.followLink { + srcStat, err := client.ContainerStatPath(ctx, copyConfig.container, srcPath) // If the destination is a symbolic link, we should follow it. if err == nil && srcStat.Mode&os.ModeSymlink != 0 { @@ -145,20 +149,17 @@ func copyFromContainer(ctx context.Context, dockerCli command.Cli, srcContainer, } - content, stat, err := dockerCli.Client().CopyFromContainer(ctx, srcContainer, srcPath) + content, stat, err := client.CopyFromContainer(ctx, copyConfig.container, srcPath) if err != nil { return err } defer content.Close() if dstPath == "-" { - // Send the response to STDOUT. - _, err = io.Copy(os.Stdout, content) - + _, err = io.Copy(dockerCli.Out(), content) return err } - // Prepare source copy info. srcInfo := archive.CopyInfo{ Path: srcPath, Exists: true, @@ -171,13 +172,17 @@ func copyFromContainer(ctx context.Context, dockerCli command.Cli, srcContainer, _, srcBase := archive.SplitPathDirEntry(srcInfo.Path) preArchive = archive.RebaseArchiveEntries(content, srcBase, srcInfo.RebaseName) } - // See comments in the implementation of `archive.CopyTo` for exactly what - // goes into deciding how and whether the source archive needs to be - // altered for the correct copy behavior. return archive.CopyTo(preArchive, srcInfo, dstPath) } -func copyToContainer(ctx context.Context, dockerCli command.Cli, srcPath, dstContainer, dstPath string, cpParam *cpConfig, copyUIDGID bool) (err error) { +// In order to get the copy behavior right, we need to know information +// about both the source and destination. The API is a simple tar +// archive/extract API but we can use the stat info header about the +// destination to be more informed about exactly what the destination is. +func copyToContainer(ctx context.Context, dockerCli command.Cli, copyConfig cpConfig) (err error) { + srcPath := copyConfig.sourcePath + dstPath := copyConfig.destPath + if srcPath != "-" { // Get an absolute source path. srcPath, err = resolveLocalPath(srcPath) @@ -186,14 +191,10 @@ func copyToContainer(ctx context.Context, dockerCli command.Cli, srcPath, dstCon } } - // In order to get the copy behavior right, we need to know information - // about both the source and destination. The API is a simple tar - // archive/extract API but we can use the stat info header about the - // destination to be more informed about exactly what the destination is. - + client := dockerCli.Client() // Prepare destination copy info by stat-ing the container path. dstInfo := archive.CopyInfo{Path: dstPath} - dstStat, err := statContainerPath(ctx, dockerCli, dstContainer, dstPath) + dstStat, err := client.ContainerStatPath(ctx, copyConfig.container, dstPath) // If the destination is a symbolic link, we should evaluate it. if err == nil && dstStat.Mode&os.ModeSymlink != 0 { @@ -205,7 +206,7 @@ func copyToContainer(ctx context.Context, dockerCli command.Cli, srcPath, dstCon } dstInfo.Path = linkTarget - dstStat, err = statContainerPath(ctx, dockerCli, dstContainer, linkTarget) + dstStat, err = client.ContainerStatPath(ctx, copyConfig.container, linkTarget) } // Ignore any error and assume that the parent directory of the destination @@ -224,15 +225,14 @@ func copyToContainer(ctx context.Context, dockerCli command.Cli, srcPath, dstCon ) if srcPath == "-" { - // Use STDIN. content = os.Stdin resolvedDstPath = dstInfo.Path if !dstInfo.IsDir { - return errors.Errorf("destination \"%s:%s\" must be a directory", dstContainer, dstPath) + return errors.Errorf("destination \"%s:%s\" must be a directory", copyConfig.container, dstPath) } } else { // Prepare source copy info. - srcInfo, err := archive.CopyInfoSourcePath(srcPath, cpParam.followLink) + srcInfo, err := archive.CopyInfoSourcePath(srcPath, copyConfig.followLink) if err != nil { return err } @@ -267,10 +267,9 @@ func copyToContainer(ctx context.Context, dockerCli command.Cli, srcPath, dstCon options := types.CopyToContainerOptions{ AllowOverwriteDirWithFile: false, - CopyUIDGID: copyUIDGID, + CopyUIDGID: copyConfig.copyUIDGID, } - - return dockerCli.Client().CopyToContainer(ctx, dstContainer, resolvedDstPath, content, options) + return client.CopyToContainer(ctx, copyConfig.container, resolvedDstPath, content, options) } // We use `:` as a delimiter between CONTAINER and PATH, but `:` could also be diff --git a/cli/command/container/cp_test.go b/cli/command/container/cp_test.go new file mode 100644 index 0000000000..db7760c4b2 --- /dev/null +++ b/cli/command/container/cp_test.go @@ -0,0 +1,160 @@ +package container + +import ( + "io" + "io/ioutil" + "runtime" + "strings" + "testing" + + "github.com/docker/cli/internal/test" + "github.com/docker/cli/internal/test/testutil" + "github.com/docker/docker/api/types" + "github.com/docker/docker/pkg/archive" + "github.com/gotestyourself/gotestyourself/fs" + "github.com/gotestyourself/gotestyourself/skip" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRunCopyWithInvalidArguments(t *testing.T) { + var testcases = []struct { + doc string + options copyOptions + expectedErr string + }{ + { + doc: "copy between container", + options: copyOptions{ + source: "first:/path", + destination: "second:/path", + }, + expectedErr: "copying between containers is not supported", + }, + { + doc: "copy without a container", + options: copyOptions{ + source: "./source", + destination: "./dest", + }, + expectedErr: "must specify at least one container source", + }, + } + for _, testcase := range testcases { + t.Run(testcase.doc, func(t *testing.T) { + err := runCopy(test.NewFakeCli(nil), testcase.options) + assert.EqualError(t, err, testcase.expectedErr) + }) + } +} + +func TestRunCopyFromContainerToStdout(t *testing.T) { + tarContent := "the tar content" + + fakeClient := &fakeClient{ + containerCopyFromFunc: func(container, srcPath string) (io.ReadCloser, types.ContainerPathStat, error) { + assert.Equal(t, "container", container) + return ioutil.NopCloser(strings.NewReader(tarContent)), types.ContainerPathStat{}, nil + }, + } + options := copyOptions{source: "container:/path", destination: "-"} + cli := test.NewFakeCli(fakeClient) + err := runCopy(cli, options) + require.NoError(t, err) + assert.Equal(t, tarContent, cli.OutBuffer().String()) + assert.Equal(t, "", cli.ErrBuffer().String()) +} + +func TestRunCopyFromContainerToFilesystem(t *testing.T) { + destDir := fs.NewDir(t, "cp-test", + fs.WithFile("file1", "content\n")) + defer destDir.Remove() + + fakeClient := &fakeClient{ + containerCopyFromFunc: func(container, srcPath string) (io.ReadCloser, types.ContainerPathStat, error) { + assert.Equal(t, "container", container) + readCloser, err := archive.TarWithOptions(destDir.Path(), &archive.TarOptions{}) + return readCloser, types.ContainerPathStat{}, err + }, + } + options := copyOptions{source: "container:/path", destination: destDir.Path()} + cli := test.NewFakeCli(fakeClient) + err := runCopy(cli, options) + require.NoError(t, err) + assert.Equal(t, "", cli.OutBuffer().String()) + assert.Equal(t, "", cli.ErrBuffer().String()) + + content, err := ioutil.ReadFile(destDir.Join("file1")) + require.NoError(t, err) + assert.Equal(t, "content\n", string(content)) +} + +func TestRunCopyFromContainerToFilesystemMissingDestinationDirectory(t *testing.T) { + destDir := fs.NewDir(t, "cp-test", + fs.WithFile("file1", "content\n")) + defer destDir.Remove() + + fakeClient := &fakeClient{ + containerCopyFromFunc: func(container, srcPath string) (io.ReadCloser, types.ContainerPathStat, error) { + assert.Equal(t, "container", container) + readCloser, err := archive.TarWithOptions(destDir.Path(), &archive.TarOptions{}) + return readCloser, types.ContainerPathStat{}, err + }, + } + + options := copyOptions{ + source: "container:/path", + destination: destDir.Join("missing", "foo"), + } + cli := test.NewFakeCli(fakeClient) + err := runCopy(cli, options) + testutil.ErrorContains(t, err, destDir.Join("missing")) +} + +func TestSplitCpArg(t *testing.T) { + var testcases = []struct { + doc string + path string + os string + expectedContainer string + expectedPath string + }{ + { + doc: "absolute path with colon", + os: "linux", + path: "/abs/path:withcolon", + expectedPath: "/abs/path:withcolon", + }, + { + doc: "relative path with colon", + path: "./relative:path", + expectedPath: "./relative:path", + }, + { + doc: "absolute path with drive", + os: "windows", + path: `d:\abs\path`, + expectedPath: `d:\abs\path`, + }, + { + doc: "no separator", + path: "relative/path", + expectedPath: "relative/path", + }, + { + doc: "with separator", + path: "container:/opt/foo", + expectedPath: "/opt/foo", + expectedContainer: "container", + }, + } + for _, testcase := range testcases { + t.Run(testcase.doc, func(t *testing.T) { + skip.IfCondition(t, testcase.os != "" && testcase.os != runtime.GOOS) + + container, path := splitCpArg(testcase.path) + assert.Equal(t, testcase.expectedContainer, container) + assert.Equal(t, testcase.expectedPath, path) + }) + } +}