Add option `-L` to allow `docker cp` follow symbol link #16613

WeiZhang555 · 2015-09-27T13:05:47Z

When user try to cp a symbol link to/from a container, what the user
really want is to copy the link target but not the symbol link, as the
symbol link is only valid and useful in host/container's scope.
All in all, who will need a symbol link copied to container which points
to some file on host? That's meaningless.

And this is also the default behavior of a unix cp command.

Signed-off-by: Zhang Wei [email protected]

WeiZhang555 · 2015-09-27T13:43:30Z

docker exec test failed, I have no idea about this:

----------------------------------------------------------------------
FAIL: docker_cli_exec_test.go:615: DockerSuite.TestExecStartFails

docker_cli_exec_test.go:622:
    c.Fatal("exec error message not received. The daemon might had crashed")
... Error: exec error message not received. The daemon might had crashed


----------------------------------------------------------------------

calavera · 2015-09-28T18:56:44Z

Removing a whole test without adding a new one makes me quite uncomfortable.

I'd like to know what @jlhawn thinks about this change before giving it my 👍

WeiZhang555 · 2015-09-29T01:22:23Z

@calavera Understand, that test case is wholly removed because it's totally inappropriate.
I'll add one more testcases after we make a deal about the cp behaviour. :)

WeiZhang555 · 2015-09-29T06:49:47Z

@calavera
Is there any way to run some specific integration test file instead of all? It's quite slow and inconvenient to run all the integration scripts. :)

tonistiigi · 2015-09-29T17:46:25Z

Like pointed out in #16555 docker cp currently uses cp -a syntax. This isn't accidental, but documented, together with symlink following rules. We can consider supporting both, but probably not switching the behavior as we also have opposite issues from the time before refactoring took place(#13157).

More importantly, there is an API for pulling and pushing files to/from containers and there is a cli tool. IMHO if we allow following of the source we should do it in the client side and not change API. This is also how we do it currently when you copy local files into a symlink inside a container. We resolve the actual target directory in the client side and start upload to that.

WeiZhang555 · 2015-09-30T02:30:05Z

@tonistiigi
All right, how about that we reserve the default docker cp behaviour, but add more options such as -L(following symbol link) and -r (copy directory recursely), this will also need a little change to the API, e.g. GET "/containers/{name:.*}/archive?recurse=true&linkFollow=false".
Of course we can make default behaviour unchanged, but it's hard to change cp behaviour without modifying REST API. For example, the TARed file will contain file name, if we don't pass linkFollow param through to daemon, daemon can't choose a correct rebaseName.

What do you think of extra option of -L and -r ? Is that acceptable?

tonistiigi · 2015-09-30T03:31:15Z

@WeiZhang555 I'm not sure if its better to add -L or make the current behavior docker cp -a but I'd really like if this would be client side change only. Take a look how we handle remote symlink targets. Also I think current rebasing all happens on client side anyway.

WeiZhang555 · 2015-09-30T03:44:44Z

@tonistiigi
We can make origal REST API GET "/containers/{name:.*}/archive" behave like GET "/containers/{name:.*}/archive?recurse=true&linkFollow=false", so it won't impact exsiting tools. And IMO it's better.

But it's also OK to change client only if you really feel uncomfortable with API change, I can take a look at what I can do for that.

tonistiigi · 2015-09-30T03:56:05Z

@WeiZhang555 I think the current API has the correct behavior. /archive does not assume any special handling for symlinks. We can reconsider if client-only change turns out to be impossible but I think its doable. Also make sure that copy-from-container and copy-to-container always work the same way.

WeiZhang555 · 2015-09-30T05:06:47Z

@tonistiigi All right, I can try to find a way to make client-only change, maybe need a couple of days. :)

WeiZhang555 · 2015-10-01T08:04:36Z

Just refreshed my PR, add one '-L' option to allow following symbol links.
e.g.
docker cp xxx:xxx xxx means copy symbol link file itself;
doker cp -L xxx:xxx xxx means copy target file.

Let's see how it works.
I'm still working on test cases and docs thing.

WeiZhang555 · 2015-10-02T04:17:55Z

@tonistiigi @calavera @jlhawn
Just add some unit tests, I will keep working on integration test.
Please tell me what do you think of the -L option, is this OK for you?

One more thing, the reason why I use the -L instead of -a is that -a is an option which is combined of -d and -R(reference unix cp command), but -L is kind of a standalone option, And also -L can keep original behavior of docker cp so it won't influence existing tools which makes use of docker cp.

Let me know whether this is acceptable for you in case I write too many codes that will never be merged. :)

tonistiigi · 2015-10-02T07:11:15Z

@WeiZhang555 I didn't look at the code too carefully atm but the approach looks good to me. Tests look good as well but yes, inctegration-cli and docs are also needed. I wonder if -R would be a better flag because -L usually means resolve all symlinks but this is for resolving the source only.

tonistiigi · 2015-10-02T07:13:44Z

pkg/archive/copy_test.go

-func TestCopyCaseC(t *testing.T) {
+// C.1 SRC specifies a file and DST exists as a file. This should overwrite
+//     the file at DST with the contents of the source file.
+func TestCopyCaseC1(t *testing.T) {


I'd use TestCopyCaseCFollowSymlink or TestCopyCaseCFSym instead the 1/2 suffix.

OK, I can fix this.

WeiZhang555 · 2015-10-02T08:04:07Z

@tonistiigi We can find a nice option together, but -R seems easy to be mixed with -r(recursively) and may not be a good choice. :)

I'll go on adding the integration testa and docs, also more tests to see if this change works correctly. :)

WeiZhang555 · 2015-10-05T02:28:17Z

@tonistiigi updated, please help review :)

vdemeester · 2015-10-05T06:52:53Z

@WeiZhang555 sorry, needs a rebase 😓

thaJeztah · 2015-11-20T11:06:46Z

docs/reference/commandline/cp.md

-a tar archive to `STDOUT` if `-` is specified).
+The `docker cp` utility copies the contents of `SRC_PATH` to the `DEST_PATH`.
+You can copy from the container's file system to the local machine or the
+reverse, from the local filesystem to the container. if If `-` is specified for


double "if" here

Ah, you are right! 👍

thaJeztah · 2015-11-20T11:26:01Z

Thanks @WeiZhang555, I added my comments inline

WeiZhang555 · 2015-11-20T11:41:31Z

@thaJeztah Very detailed suggestion, I'll do it now 👍

WeiZhang555 · 2015-11-20T12:14:34Z

@thaJeztah @moxiegirl updated.

If - is specified for
either the SRC_PATH or DEST_PATH, you can also stream a tar archive to
STDOUT.

I think it's not very accurate. How about

If `-` is specified for
either the `SRC_PATH` or `DEST_PATH`, you can also stream a tar archive from `STDIN` or to
`STDOUT`.

Maybe this is better? WDYT?

thaJeztah · 2015-11-20T12:48:01Z

Hm, yes, I think that makes sense

Fixes moby#16555 Original docker `cp` always copy symbol link itself instead of target, now we provide '-L' option to allow docker to follow symbol link to real target. Signed-off-by: Zhang Wei <[email protected]>

WeiZhang555 · 2015-11-20T16:41:21Z

@thaJeztah updated. 😄

moxiegirl · 2015-11-20T20:18:25Z

LGTM! Thank you @WeiZhang555 :-D

thaJeztah · 2015-11-20T23:54:59Z

Thanks @WeiZhang555 for your contribution!

LGTM

thaJeztah · 2015-11-20T23:56:53Z

restarting Windows build

vdemeester · 2015-11-21T11:02:56Z

Windows builds are definitely in holidays.. 😅

Add option `-L` to allow `docker cp` follow symbol link

WeiZhang555 · 2015-11-21T18:31:34Z

Yeah, Windows build is a naughty boy 😄

Thank you all for helping! 😆

GordonTheTurtle added the status/0-triage label Sep 27, 2015

WeiZhang555 force-pushed the docker-cp-symlink branch from 7344988 to 8f84705 Compare September 28, 2015 06:57

calavera added status/2-code-review and removed status/0-triage labels Sep 28, 2015

WeiZhang555 force-pushed the docker-cp-symlink branch from 8f84705 to 1747fc5 Compare October 1, 2015 08:00

WeiZhang555 force-pushed the docker-cp-symlink branch 2 times, most recently from 4815599 to 3871588 Compare October 1, 2015 08:30

WeiZhang555 changed the title ~~Fix bug that docker cp can't follow symbol link~~ Add option -L to allow docker cp follow symbol link Oct 2, 2015

WeiZhang555 force-pushed the docker-cp-symlink branch from 3871588 to 3aca000 Compare October 2, 2015 04:07

tonistiigi reviewed Oct 2, 2015
View reviewed changes

WeiZhang555 force-pushed the docker-cp-symlink branch from 3aca000 to 352c4c4 Compare October 2, 2015 07:59

WeiZhang555 force-pushed the docker-cp-symlink branch 2 times, most recently from ed655e7 to 2b152cb Compare October 3, 2015 02:23

WeiZhang555 force-pushed the docker-cp-symlink branch from 2b152cb to 58e77ac Compare October 5, 2015 14:28

thaJeztah reviewed Nov 20, 2015
View reviewed changes

WeiZhang555 force-pushed the docker-cp-symlink branch from 9919b66 to 6cb2b2b Compare November 20, 2015 12:09

Add '-L' option for cp

92600bd

Fixes moby#16555 Original docker `cp` always copy symbol link itself instead of target, now we provide '-L' option to allow docker to follow symbol link to real target. Signed-off-by: Zhang Wei <[email protected]>

WeiZhang555 force-pushed the docker-cp-symlink branch from 6cb2b2b to 92600bd Compare November 20, 2015 16:38

thaJeztah added status/4-merge impact/changelog impact/cli and removed status/3-docs-review labels Nov 20, 2015

runcom added a commit that referenced this pull request Nov 21, 2015

Merge pull request #16613 from WeiZhang555/docker-cp-symlink

3ff9bb5

Add option `-L` to allow `docker cp` follow symbol link

runcom merged commit 3ff9bb5 into moby:master Nov 21, 2015

WeiZhang555 deleted the docker-cp-symlink branch November 21, 2015 18:31

thaJeztah added this to the 1.10.0 milestone Jan 15, 2016

albers mentioned this pull request Jan 24, 2016

bash completion for docker cp --follow-link #19632

Merged

sdurrheimer mentioned this pull request Jan 25, 2016

Add zsh completion for 'docker cp -L --follow-link' #19655

Merged

WeiZhang555 mentioned this pull request Mar 12, 2016

docker cp -L --follow-link fails to dereference file link #21146

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option `-L` to allow `docker cp` follow symbol link #16613

Add option `-L` to allow `docker cp` follow symbol link #16613

WeiZhang555 commented Sep 27, 2015

WeiZhang555 commented Sep 27, 2015

calavera commented Sep 28, 2015

WeiZhang555 commented Sep 29, 2015

WeiZhang555 commented Sep 29, 2015

tonistiigi commented Sep 29, 2015

WeiZhang555 commented Sep 30, 2015

tonistiigi commented Sep 30, 2015

WeiZhang555 commented Sep 30, 2015

tonistiigi commented Sep 30, 2015

WeiZhang555 commented Sep 30, 2015

WeiZhang555 commented Oct 1, 2015

WeiZhang555 commented Oct 2, 2015

tonistiigi commented Oct 2, 2015

tonistiigi Oct 2, 2015

WeiZhang555 Oct 2, 2015

WeiZhang555 commented Oct 2, 2015

WeiZhang555 commented Oct 5, 2015

vdemeester commented Oct 5, 2015

thaJeztah Nov 20, 2015

WeiZhang555 Nov 20, 2015

thaJeztah commented Nov 20, 2015

WeiZhang555 commented Nov 20, 2015

WeiZhang555 commented Nov 20, 2015

thaJeztah commented Nov 20, 2015

WeiZhang555 commented Nov 20, 2015

moxiegirl commented Nov 20, 2015

thaJeztah commented Nov 20, 2015

thaJeztah commented Nov 20, 2015

vdemeester commented Nov 21, 2015

WeiZhang555 commented Nov 21, 2015

Add option -L to allow docker cp follow symbol link #16613

Add option -L to allow docker cp follow symbol link #16613

Conversation

WeiZhang555 commented Sep 27, 2015

WeiZhang555 commented Sep 27, 2015

calavera commented Sep 28, 2015

WeiZhang555 commented Sep 29, 2015

WeiZhang555 commented Sep 29, 2015

tonistiigi commented Sep 29, 2015

WeiZhang555 commented Sep 30, 2015

tonistiigi commented Sep 30, 2015

WeiZhang555 commented Sep 30, 2015

tonistiigi commented Sep 30, 2015

WeiZhang555 commented Sep 30, 2015

WeiZhang555 commented Oct 1, 2015

WeiZhang555 commented Oct 2, 2015

tonistiigi commented Oct 2, 2015

tonistiigi Oct 2, 2015

Choose a reason for hiding this comment

WeiZhang555 Oct 2, 2015

Choose a reason for hiding this comment

WeiZhang555 commented Oct 2, 2015

WeiZhang555 commented Oct 5, 2015

vdemeester commented Oct 5, 2015

thaJeztah Nov 20, 2015

Choose a reason for hiding this comment

WeiZhang555 Nov 20, 2015

Choose a reason for hiding this comment

thaJeztah commented Nov 20, 2015

WeiZhang555 commented Nov 20, 2015

WeiZhang555 commented Nov 20, 2015

thaJeztah commented Nov 20, 2015

WeiZhang555 commented Nov 20, 2015

moxiegirl commented Nov 20, 2015

thaJeztah commented Nov 20, 2015

thaJeztah commented Nov 20, 2015

vdemeester commented Nov 21, 2015

WeiZhang555 commented Nov 21, 2015

Add option `-L` to allow `docker cp` follow symbol link #16613

Add option `-L` to allow `docker cp` follow symbol link #16613