-
Notifications
You must be signed in to change notification settings - Fork 18.9k
docker cp to and from containers #13171
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
Conversation
658bce0 to
450e672
Compare
|
Oh my goodness. I wanted this some much last year, but we couldn't agree on the syntax of addresssing src and dest. |
3d410b7 to
677dfc4
Compare
|
Not sure if it impacts your help comment, but FYI: #11858 |
|
@duglin I would definitely have to rebase if that gets merged before this ;-) |
16edb82 to
693fbdd
Compare
e34da4b to
505e798
Compare
|
🎉 Thanks everyone! |
|
Great job @jlhawn 👍 |
|
😄 |
|
Wow, it's there!! looks like you need to update your story #13171 (comment) @jlhawn :-) |
|
Some follow-up... Symlink sources don't seem to rebase properly: It's because client expects base directory If I run 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
Great job @jlhawn I have marked this serious and constructive discussion : ) |
|
I have tried something like this: $ docker cp 0converted sleepy_rosalind:/home/test/data/aero_spectrum
What is wrong? where 0converted is an directory and aero_spectrum is another directory inside my container. |
|
@calebebrim Please avoid commenting on closed issues. There are many other avenues to get support on using |
This change changes the default for noOverwriteDirNonDir to be true internally, with the intent to change the default at the API to follow accordingly. The `AllowOverwriteDirWithFile` option in the Client was added when reimplementing the CLI using the API Client lib in [moby/moby@1b2b91b]. Before that refactor, the `noOverwriteDirNonDir` query argument [would be set unconditionally][1] by the CLI, with no options to control the behavior. The `noOverwriteDirNonDir` query parameter was added in [moby/moby@db9cc91] to set the `NoOverwriteDirNonDir` option that was implemented in pkg/archive in [moby/moby@a74799b]. It was added in [PR13171-comment2], following a discussion on the risk of replacing a directory with a file and vice-versa in [PR13171-comment]. > In my latest changes from yesterday: > > - Removed the `GET stat-path` endpoint and added a `HEAD` handler to > the `archive-path` endpoint. Updated the api docs to reflect this. > Also moved api docs changes from `v1.19` to `v1.20`. > - Added a `NoOverwriteDirNonDir` flag to `archive.TarOptions` to indicate > that we do not want to overwrite a directory with a non-directory (and > vice versa) when unpacking an archive. > - Added a corresponding but optional `noOverwriteDirNonDir` parameter > to the `PUT extract-to-dir` endpoint to specify desired behavior. > > These changes combine to keep the behavior we want It's unclear why these were added as an *option* and why it was implemented as opt-in (not opt-out), as overwriting a file with a directory (or vice-versa) would generally be unexpected behavior. [1]: https://github.com/moby/moby/blob/8c9ad7b818c0a7b1e39f8df1fabba243a0961c2d/api/client/cp.go#L345-L346 [moby/moby@1b2b91b]: moby@1b2b91b [moby/moby@a74799b]: moby@a74799b [moby/moby@db9cc91]: moby@db9cc91 [PR13171-comment]: moby#13171 (comment) [PR13171-comment2]: moby#13171 (comment) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This change changes the default for noOverwriteDirNonDir to be true internally, with the intent to change the default at the API to follow accordingly. The `AllowOverwriteDirWithFile` option in the Client was added when reimplementing the CLI using the API Client lib in [moby/moby@1b2b91b]. Before that refactor, the `noOverwriteDirNonDir` query argument [would be set unconditionally][1] by the CLI, with no options to control the behavior. The `noOverwriteDirNonDir` query parameter was added in [moby/moby@db9cc91] to set the `NoOverwriteDirNonDir` option that was implemented in pkg/archive in [moby/moby@a74799b]. It was added in [PR13171-comment2], following a discussion on the risk of replacing a directory with a file and vice-versa in [PR13171-comment]. > In my latest changes from yesterday: > > - Removed the `GET stat-path` endpoint and added a `HEAD` handler to > the `archive-path` endpoint. Updated the api docs to reflect this. > Also moved api docs changes from `v1.19` to `v1.20`. > - Added a `NoOverwriteDirNonDir` flag to `archive.TarOptions` to indicate > that we do not want to overwrite a directory with a non-directory (and > vice versa) when unpacking an archive. > - Added a corresponding but optional `noOverwriteDirNonDir` parameter > to the `PUT extract-to-dir` endpoint to specify desired behavior. > > These changes combine to keep the behavior we want It's unclear why these were added as an *option* and why it was implemented as opt-in (not opt-out), as overwriting a file with a directory (or vice-versa) would generally be unexpected behavior. [1]: https://github.com/moby/moby/blob/8c9ad7b818c0a7b1e39f8df1fabba243a0961c2d/api/client/cp.go#L345-L346 [moby/moby@1b2b91b]: moby@1b2b91b [moby/moby@a74799b]: moby@a74799b [moby/moby@db9cc91]: moby@db9cc91 [PR13171-comment]: moby#13171 (comment) [PR13171-comment2]: moby#13171 (comment) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This change changes the default for noOverwriteDirNonDir to be true internally, with the intent to change the default at the API to follow accordingly. The `AllowOverwriteDirWithFile` option in the Client was added when reimplementing the CLI using the API Client lib in [moby/moby@1b2b91b]. Before that refactor, the `noOverwriteDirNonDir` query argument [would be set unconditionally][1] by the CLI, with no options to control the behavior. The `noOverwriteDirNonDir` query parameter was added in [moby/moby@db9cc91] to set the `NoOverwriteDirNonDir` option that was implemented in pkg/archive in [moby/moby@a74799b]. It was added in [PR13171-comment2], following a discussion on the risk of replacing a directory with a file and vice-versa in [PR13171-comment]. > In my latest changes from yesterday: > > - Removed the `GET stat-path` endpoint and added a `HEAD` handler to > the `archive-path` endpoint. Updated the api docs to reflect this. > Also moved api docs changes from `v1.19` to `v1.20`. > - Added a `NoOverwriteDirNonDir` flag to `archive.TarOptions` to indicate > that we do not want to overwrite a directory with a non-directory (and > vice versa) when unpacking an archive. > - Added a corresponding but optional `noOverwriteDirNonDir` parameter > to the `PUT extract-to-dir` endpoint to specify desired behavior. > > These changes combine to keep the behavior we want It's unclear why these were added as an *option* and why it was implemented as opt-in (not opt-out), as overwriting a file with a directory (or vice-versa) would generally be unexpected behavior. [1]: https://github.com/moby/moby/blob/8c9ad7b818c0a7b1e39f8df1fabba243a0961c2d/api/client/cp.go#L345-L346 [moby/moby@1b2b91b]: moby@1b2b91b [moby/moby@a74799b]: moby@a74799b [moby/moby@db9cc91]: moby@db9cc91 [PR13171-comment]: moby#13171 (comment) [PR13171-comment2]: moby#13171 (comment) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Copy files/folders between containers and the local filesystem.
In the first synopsis form, the
docker cputility copies the contents ofPATHfrom the filesystem ofCONTAINERto theLOCALPATH(or stream asa Tar Archive to
STDOUTif-is specified).In the second synopsis form, the contents of
LOCALPATH(or a Tar Archivestreamed from
STDINif-is specified) are copied from the local machine toPATHin the filesystem ofCONTAINER.You can copy to or from either a running or stopped container. The
PATHcanbe a file or directory. The
docker cpcommand assumes allCONTAINER:PATHvalues are relative to the
/(root) directory of the container. This meanssupplying the initial forward slash is optional; The command sees
compassionate_darwin:/tmp/foo/myfile.txtandcompassionate_darwin:tmp/foo/myfile.txtas identical. If aLOCALPATHvalueis not absolute, is it considered relative to the current working directory.
Behavior is similar to the common Unix utility
cp -ain that directories arecopied recursively and file mode, permission, and ownership are preserved if
possible.
Assuming a path separator of
/, a first argument ofSRC_PATHand secondargument of
DST_PATH, the behavior is as follows:SRC_PATHspecifies a fileDST_PATHdoes not existDST_PATHDST_PATHdoes not exist and ends with/DST_PATHexists and is a fileDST_PATHexists and is a directorySRC_PATHSRC_PATHspecifies a directoryDST_PATHdoes not existDST_PATHis created as a directory and the contents of the sourcedirectory are copied into this directory
DST_PATHexists and is a fileDST_PATHexists and is a directorySRC_PATHdoes not end with/.SRC_PAPTHdoes end with/.directory
The command requires
SRC_PATHandDST_PATHto exist according to the aboverules. If
SRC_PATHis a symbolic link, the symbolic link, not the target, iscopied. 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 betweenCONTAINERandPATH, but:could also be in a valid
LOCALPATH, likefile:name.txt. This ambiguity isresolved by requiring a
LOCALPATHwith a:to be made explicit with arelative or absolute path, for example:
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 aLOCALPATHwill stream thecontents of
STDINas a Tar Archive which will be extracted to thePATHinthe filesystem of the destination container. In this case,
PATHmust specifya directory.
Using
-as the second argument in place of aLOCALPATHwill stream thecontents of the resource from the source container as a Tar Archive to
STDOUT.docker cpdoes not cause a conflict when the archived directory structure replaces a directory with a file or vice versa #10040