New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docker cp to and from containers #13171

Merged
merged 6 commits into from Jul 21, 2015

Conversation

Projects
None yet
@jlhawn
Contributor

jlhawn commented May 13, 2015

Copy files/folders between containers and the local filesystem.

Usage:  docker cp [options] CONTAINER:PATH LOCALPATH|-
        docker cp [options] LOCALPATH|- CONTAINER:PATH

--help  Print usage statement

In the first synopsis form, the docker cp utility copies the contents of
PATH from the filesystem of CONTAINER to the LOCALPATH (or stream as
a Tar Archive to STDOUT if - is specified).

In the second synopsis form, the contents of LOCALPATH (or a Tar Archive
streamed from STDIN if - is specified) are copied from the local machine to
PATH in the filesystem of CONTAINER.

You can copy to or from either a running or stopped container. The PATH can
be a file or directory. The docker cp command assumes all CONTAINER:PATH
values are relative to the / (root) directory of the container. This means
supplying the initial forward slash is optional; The command sees
compassionate_darwin:/tmp/foo/myfile.txt and
compassionate_darwin:tmp/foo/myfile.txt as identical. If a LOCALPATH value
is not absolute, is it considered relative to the current working directory.

Behavior is similar to the common Unix utility cp -a in that directories are
copied recursively and file mode, permission, and ownership are preserved if
possible.

Assuming a path separator of /, a first argument of SRC_PATH and second
argument of DST_PATH, the behavior is as follows:

  • SRC_PATH specifies a file
    • DST_PATH does not exist
      • the file is saved to a file created at DST_PATH
    • DST_PATH does not exist and ends with /
      • Error condition: the destination directory must exist.
    • DST_PATH exists and is a file
      • the destination is overwritten with the contents of the source file
    • DST_PATH exists and is a directory
      • the file is copied into this directory using the basename from
        SRC_PATH
  • SRC_PATH specifies a directory
    • DST_PATH does not exist
      • DST_PATH is created as a directory and the contents of the source
        directory are copied into this directory
    • DST_PATH exists and is a file
      • Error condition: cannot copy a directory to a file
    • DST_PATH exists and is a directory
      • SRC_PATH does not end with /.
        • the source directory is copied into this directory
      • SRC_PAPTH does end with /.
        • the content of the source directory is copied into this
          directory

The command requires SRC_PATH and DST_PATH to exist according to the above
rules. If SRC_PATH is a symbolic link, the symbolic link, not the target, is
copied. If a path separator immediately follows the symbolic link, it will be
resolved to its target and the target resource will be copied.

A colon (:) is used as a delimiter between CONTAINER and PATH, but :
could also be in a valid LOCALPATH, like file:name.txt. This ambiguity is
resolved by requiring a LOCALPATH with a : to be made explicit with a
relative or absolute path, for example:

`/path/to/file:name.txt` or `./file:name.txt`

It is not possible to copy certain system files such as resources under
/proc, /sys, /dev, and mounts created by the user in the container.

Using - as the first argument in place of a LOCALPATH will stream the
contents of STDIN as a Tar Archive which will be extracted to the PATH in
the filesystem of the destination container. In this case, PATH must specify
a directory.

Using - as the second argument in place of a LOCALPATH will stream the
contents of the resource from the source container as a Tar Archive to
STDOUT.

replaces #10198

fixes #5846
fixes #4272
fixes #10040

@jlhawn jlhawn self-assigned this May 13, 2015

@jlhawn jlhawn added this to the 1.7.0 milestone May 13, 2015

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts May 13, 2015

Contributor

Oh my goodness. I wanted this some much last year, but we couldn't agree on the syntax of addresssing src and dest.

Contributor

vbatts commented May 13, 2015

Oh my goodness. I wanted this some much last year, but we couldn't agree on the syntax of addresssing src and dest.

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin May 14, 2015

Contributor

Not sure if it impacts your help comment, but FYI: #11858

Contributor

duglin commented May 14, 2015

Not sure if it impacts your help comment, but FYI: #11858

@jlhawn

This comment has been minimized.

Show comment
Hide comment
@jlhawn

jlhawn May 14, 2015

Contributor

@duglin I would definitely have to rebase if that gets merged before this ;-)

Contributor

jlhawn commented May 14, 2015

@duglin I would definitely have to rebase if that gets merged before this ;-)

@jlhawn

This comment has been minimized.

Show comment
Hide comment
@jlhawn

jlhawn May 15, 2015

Contributor

Wow, just mention the word "Windows" and @GordonTheTurtle gets excited.

Contributor

jlhawn commented May 15, 2015

Wow, just mention the word "Windows" and @GordonTheTurtle gets excited.

@jlhawn

This comment has been minimized.

Show comment
Hide comment
@jlhawn

jlhawn May 15, 2015

Contributor

At last! Windows+Janky tests passing! 🎆

Contributor

jlhawn commented May 15, 2015

At last! Windows+Janky tests passing! 🎆

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 15, 2015

Member

Wow, just mention the word "W1ndows" and @GordonTheTurtle gets excited.

Yup, it's a bit too eager on adding that. Starting to wonder if the Redmond guys bribed him or put something extra in his food 😜

Member

thaJeztah commented May 15, 2015

Wow, just mention the word "W1ndows" and @GordonTheTurtle gets excited.

Yup, it's a bit too eager on adding that. Starting to wonder if the Redmond guys bribed him or put something extra in his food 😜

@jlhawn jlhawn changed the title from [WIP] docker cp to and from containers to docker cp to and from containers May 15, 2015

@jlhawn

This comment has been minimized.

Show comment
Hide comment
@jlhawn

jlhawn May 15, 2015

Contributor

@docker/maintainers IT'S READY

Contributor

jlhawn commented May 15, 2015

@docker/maintainers IT'S READY

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass May 15, 2015

Collaborator

Design +1

Collaborator

tiborvass commented May 15, 2015

Design +1

const (
fromContainer copyDirection = (1 << iota)
toContainer
acrossContainers = fromContainer | toContainer

This comment has been minimized.

@duglin

duglin May 18, 2015

Contributor

tease! :-)

@duglin

duglin May 18, 2015

Contributor

tease! :-)

Show outdated Hide outdated api/client/cp.go
Show outdated Hide outdated api/client/cp.go
@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin May 18, 2015

Contributor

In the description of this PR you talk about preserving "ownership" - is that true? Do you do something explicit for that or does the archive stuff automagically do it?

Contributor

duglin commented May 18, 2015

In the description of this PR you talk about preserving "ownership" - is that true? Do you do something explicit for that or does the archive stuff automagically do it?

Show outdated Hide outdated daemon/copy.go
Show outdated Hide outdated daemon/volumes.go
Show outdated Hide outdated docs/man/docker-cp.1.md
Show outdated Hide outdated docs/man/docker-cp.1.md
Show outdated Hide outdated docs/man/docker-cp.1.md
Show outdated Hide outdated docs/man/docker-cp.1.md
Show outdated Hide outdated docs/man/docker-cp.1.md
Show outdated Hide outdated api/types/types.go
If not an absolute path, it is relative to the container's root directory.
The **path** resource must exist.
- **noOverwriteDirNonDir** - If "1", "true", or "True" then it will be an error

This comment has been minimized.

@thaJeztah

thaJeztah Jul 21, 2015

Member

Sorry (as usual), I'm placing these comments after design was approved, but shouldn't this be the default? I.e. produce an error if I try to overwrite a file with a directory (and vice-versa), but allow to override / suppress this by passing a flag?

@thaJeztah

thaJeztah Jul 21, 2015

Member

Sorry (as usual), I'm placing these comments after design was approved, but shouldn't this be the default? I.e. produce an error if I try to overwrite a file with a directory (and vice-versa), but allow to override / suppress this by passing a flag?

This comment has been minimized.

@jlhawn

jlhawn Jul 21, 2015

Contributor

It's the default for docker cp because copy behavior is expected to have this be an error. But if you extract an archive using tar it will overwrite existing files/dirs with the same name.

@jlhawn

jlhawn Jul 21, 2015

Contributor

It's the default for docker cp because copy behavior is expected to have this be an error. But if you extract an archive using tar it will overwrite existing files/dirs with the same name.

This comment has been minimized.

@thaJeztah

thaJeztah Jul 21, 2015

Member

Just wondering what people would expect; would they expect an archive to be extracted and replace folders with files, or expect an error? (I'm leaning towards getting an error)

@thaJeztah

thaJeztah Jul 21, 2015

Member

Just wondering what people would expect; would they expect an archive to be extracted and replace folders with files, or expect an error? (I'm leaning towards getting an error)

This comment has been minimized.

@jlhawn

jlhawn Jul 21, 2015

Contributor

I think people would expect the same thing that using the tar utility would do. I also would expect them to read the docs ;-)

@jlhawn

jlhawn Jul 21, 2015

Contributor

I think people would expect the same thing that using the tar utility would do. I also would expect them to read the docs ;-)

"mtime": "2014-02-27T20:51:23Z"
}
A `HEAD` request can also be made to this endpoint if only this information is

This comment has been minimized.

@thaJeztah

thaJeztah Jul 21, 2015

Member

Given that we use separate sections / examples for GET and PUT, I think HEAD should also be in its own section / example.

@thaJeztah

thaJeztah Jul 21, 2015

Member

Given that we use separate sections / examples for GET and PUT, I think HEAD should also be in its own section / example.

This comment has been minimized.

@thaJeztah

thaJeztah Jul 21, 2015

Member

^^ e.g. "Retrieving information about files and folders in a container"
But you could refer to this section for the details (so that we don't duplicate that information )

@thaJeztah

thaJeztah Jul 21, 2015

Member

^^ e.g. "Retrieving information about files and folders in a container"
But you could refer to this section for the details (so that we don't duplicate that information )

This comment has been minimized.

@jlhawn

jlhawn Jul 21, 2015

Contributor

Okay, I've added a very short section just before the GET section.

@jlhawn

jlhawn Jul 21, 2015

Contributor

Okay, I've added a very short section just before the GET section.

jlhawn added some commits May 13, 2015

api/server: StatPath, ArchivePath, ExtractToDir
Adds http handlers for new API endpoints:

GET ContainersArchivePath
  Return a Tar Archive of the contents at the specified location in a
  container. Deprecates POST ContainersCopy. Use a HEAD request to stat
  the resource.

PUT ContainersExtractToDir
  Extract the Tar Archive from the request body to the directory at the
  specified location inside a container.

Docker-DCO-1.1-Signed-off-by: Josh Hawn <josh.hawn@docker.com> (github: jlhawn)
api/client: New and Improved `docker cp` behavior
Supports copying things INTO a container from a local file or from a tar
archive read from stdin.

Docker-DCO-1.1-Signed-off-by: Josh Hawn <josh.hawn@docker.com> (github: jlhawn)
integration-cli: New `docker cp` integration tests
Adds several integration tests for `docker cp` behavior with over a dozen
tests for each of:

  container -> local
  local -> container

Docker-DCO-1.1-Signed-off-by: Josh Hawn <josh.hawn@docker.com> (github: jlhawn)
docs: Updated for docker cp and its API changes
Documented changes to API to enable new `docker cp` behavior.

Added documentation on `docker cp` usage and behavior.

Docker-DCO-1.1-Signed-off-by: Josh Hawn <josh.hawn@docker.com> (github: jlhawn)
@icecrime

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime Jul 21, 2015

Contributor

We have "get out of jail" card from @moxiegirl on this one: "why don’t we merge it with the changes he has and I’ll take his initial docs and do a follow on PR with any changes I might have".

Contributor

icecrime commented Jul 21, 2015

We have "get out of jail" card from @moxiegirl on this one: "why don’t we merge it with the changes he has and I’ll take his initial docs and do a follow on PR with any changes I might have".

@icecrime

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime Jul 21, 2015

Contributor

Thanks for the work and for the wait @jlhawn, and thanks to all reviewers who participated in this journey.

Contributor

icecrime commented Jul 21, 2015

Thanks for the work and for the wait @jlhawn, and thanks to all reviewers who participated in this journey.

icecrime pushed a commit that referenced this pull request Jul 21, 2015

Arnaud Porterie
Merge pull request #13171 from jlhawn/archive_copy
docker cp to and from containers

@icecrime icecrime merged commit c986f85 into moby:master Jul 21, 2015

4 checks passed

docker/dco-signed All commits signed
Details
experimental Jenkins build Docker-PRs-experimental 3324 has succeeded
Details
janky Jenkins build Docker-PRs 11884 has succeeded
Details
windows Jenkins build Windows-PRs 8514 has succeeded
Details
@jlhawn

This comment has been minimized.

Show comment
Hide comment
@jlhawn

jlhawn Jul 22, 2015

Contributor

🎉 Thanks everyone!

Contributor

jlhawn commented Jul 22, 2015

🎉 Thanks everyone!

@tobegit3hub

This comment has been minimized.

Show comment
Hide comment
@tobegit3hub

tobegit3hub commented Jul 22, 2015

Great job @jlhawn 👍

@sa2ajj

This comment has been minimized.

Show comment
Hide comment
@sa2ajj

sa2ajj Jul 22, 2015

Contributor

😄

Contributor

sa2ajj commented Jul 22, 2015

😄

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 22, 2015

Member

Wow, it's there!! looks like you need to update your story #13171 (comment) @jlhawn :-)

Member

thaJeztah commented Jul 22, 2015

Wow, it's there!! looks like you need to update your story #13171 (comment) @jlhawn :-)

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Jul 22, 2015

Member

Some follow-up...

Symlink sources don't seem to rebase properly:

docker run --name t0 busybox sh -c "mkdir /foo && touch /foo/bar && ln -s /foo /baz"
docker cp t0:/baz bax
ls -l # should have bax/ but has foo/

It's because client expects base directory baz in tar when it does the replacement but daemon returns foo.


If I run docker cp id:/foo bar I would expect that the client protects me against anything being put outside local file/dir bar. Currently client trusts API to provide valid data in this case. If there is anything weird inside the tar then it can leak out at least one level.


How can we protect this from running containers updating a filesystem/volumes when the request is taking place? I mean we check for breakouts in the beginning of GET/PUT but if a container is using the filesystem it could just flip a symlink in the right time and then we would have full read/write to host.

Member

tonistiigi commented Jul 22, 2015

Some follow-up...

Symlink sources don't seem to rebase properly:

docker run --name t0 busybox sh -c "mkdir /foo && touch /foo/bar && ln -s /foo /baz"
docker cp t0:/baz bax
ls -l # should have bax/ but has foo/

It's because client expects base directory baz in tar when it does the replacement but daemon returns foo.


If I run docker cp id:/foo bar I would expect that the client protects me against anything being put outside local file/dir bar. Currently client trusts API to provide valid data in this case. If there is anything weird inside the tar then it can leak out at least one level.


How can we protect this from running containers updating a filesystem/volumes when the request is taking place? I mean we check for breakouts in the beginning of GET/PUT but if a container is using the filesystem it could just flip a symlink in the right time and then we would have full read/write to host.

// than the name of the directory. This would cause extraction of the
// archive to *not* make another directory, but instead use the current
// directory.
resolvedPath = archive.PreserveTrailingDotOrSeparator(resolvedPath, absPath)

This comment has been minimized.

@tonistiigi

tonistiigi Jul 22, 2015

Member

Is this correct? GetResourcePath() should never return a symlink so I don't think this has much effect. Similar logic in ExtractToDir and comment in ArchivePath.

When I request stat for a symlink (with or without slash) I always get a directory as a response. AFAIK there isn't actually any harm of runnning Lstat on a path that is only joined and symlinks aren't evaluated. Reading/writing is different of course.

@tonistiigi

tonistiigi Jul 22, 2015

Member

Is this correct? GetResourcePath() should never return a symlink so I don't think this has much effect. Similar logic in ExtractToDir and comment in ArchivePath.

When I request stat for a symlink (with or without slash) I always get a directory as a response. AFAIK there isn't actually any harm of runnning Lstat on a path that is only joined and symlinks aren't evaluated. Reading/writing is different of course.

This comment has been minimized.

@jlhawn

jlhawn Jul 22, 2015

Contributor

You're right, if FollowSymlinkInScope resolves all symlinks then that part doesn't matter. But, (It doesn't mention it in the comment), a trailing separator is also important because it asserts that the resource is a directory. The Lstat a couple of lines below this should capture that error condition (not a directory).

When I request stat for a symlink (with or without slash) I always get a directory as a response.

Is that when you stat a symlink on your local system or using this API?

Stat-ing a symlink with a trailing separator has different behavior depending on the system you're running on. Apparently on darwin, if a symlink foo points to a file bar and you call stat foo/ it will return stat info for bar even though bar is not a directory. On linux though, it will say stat: cannot stat 'foo/': Not a directory which is the error I expect it to pick up here.

@jlhawn

jlhawn Jul 22, 2015

Contributor

You're right, if FollowSymlinkInScope resolves all symlinks then that part doesn't matter. But, (It doesn't mention it in the comment), a trailing separator is also important because it asserts that the resource is a directory. The Lstat a couple of lines below this should capture that error condition (not a directory).

When I request stat for a symlink (with or without slash) I always get a directory as a response.

Is that when you stat a symlink on your local system or using this API?

Stat-ing a symlink with a trailing separator has different behavior depending on the system you're running on. Apparently on darwin, if a symlink foo points to a file bar and you call stat foo/ it will return stat info for bar even though bar is not a directory. On linux though, it will say stat: cannot stat 'foo/': Not a directory which is the error I expect it to pick up here.

@onesuper

This comment has been minimized.

Show comment
Hide comment
@onesuper

onesuper Nov 22, 2015

Great job @jlhawn

I have marked this serious and constructive discussion : )

onesuper commented Nov 22, 2015

Great job @jlhawn

I have marked this serious and constructive discussion : )

@calebebrim

This comment has been minimized.

Show comment
Hide comment
@calebebrim

calebebrim Dec 7, 2015

I have tried something like this:

$ docker cp 0converted sleepy_rosalind:/home/test/data/aero_spectrum

cannot copy directory

What is wrong?
I'm running version 1.9.

where 0converted is an directory and aero_spectrum is another directory inside my container.

calebebrim commented Dec 7, 2015

I have tried something like this:

$ docker cp 0converted sleepy_rosalind:/home/test/data/aero_spectrum

cannot copy directory

What is wrong?
I'm running version 1.9.

where 0converted is an directory and aero_spectrum is another directory inside my container.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Dec 8, 2015

Contributor

@calebebrim Please avoid commenting on closed issues. There are many other avenues to get support on using docker cp, such as the mailing list or IRC. If you still can't find a solution to your problem, you may then want to open an issue at that time.

Contributor

stevvooe commented Dec 8, 2015

@calebebrim Please avoid commenting on closed issues. There are many other avenues to get support on using docker cp, such as the mailing list or IRC. If you still can't find a solution to your problem, you may then want to open an issue at that time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment