Change uid/gid on 'docker cp' and preserve it on 'docker cp -a'#34099
Change uid/gid on 'docker cp' and preserve it on 'docker cp -a'#34099chipironcin wants to merge 1 commit intomoby:masterfrom
Conversation
|
Actually, I think this would need proper tests. |
Signed-off-by: Jorge Marin <[email protected]>
chipironcin
force-pushed
the
fix_wrong_cp_behavior
branch
from
1b7bc00 to
f1858a2
Compare
|
Thanks @chipironcin - this change looks correct indeed; while looking for a way to test this, I discovered other issues as well in this functionality; see #34142 I was already looking into the |
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
I'm OK with merging this as-is, as the other issues need to be looked into as well, and complicate testing this functionality
open to suggestions though
LGTM
|
pushed #34143 (work in progress) to see if the uid/gid lookup can be addressed |
|
This doesn't look quite correct yet because of this condition moby/daemon/archive_tarcopyoptions_unix.go Lines 12 to 14 in 458f671 docker cp would rewrite files to root if --user=root is set. If no specific user is defined(still equivalent to root) it would preserve the original owners. That also seems to be the reason why the testcase in https://github.com/moby/moby/pull/28923/files#diff-d16a9bdd478399b633d0f1b343f4b9d1R17 doesn't actually test anything.
|
|
any update on this? |
|
ping @erikh could you have a look? |
|
@tonistiigi is correct that this is not the right fix. See #34096 (comment) Since this has not seen any activity in 3 months I'm going to close this PR. If you're still interested in submitting a fix please update and comment here. We can re-open it. |
|
@dnephin we'll still need a fix though 😅 |
|
Pls reopen, I haven't had time to work on this and I have no previous experience with golang :( |
|
I'll try to check it out soon |
|
Reopening, let us know if you need help |
|
Are there any updates on the status of this ticket? I ran into this today and the docs seem to indicate that this behaviour is expected. |
|
I still ran into this problem with |
|
What is the status of this bug? Was it fixed but just stalled due to a merge conflict? The bug bit me today and it is a little worrying that docker cp has been broken for so long. |
|
I am afraid that at this point fixing this would actually broke existing code that relies on current truly odd behaviour. |
|
Folks at |
13 participants
Signed-off-by: Jorge Marin [email protected]
Fixes #34096
- What I did
Modified behavior of
docker cpcommand to revert it back to the original behavior after #28923- How I did it
Negate a variable and add an explanatory comment.
- How to verify it
Read #34096
- Description for the changelog
Revert
docker cpbehavior back to old way of setting uid/gid when copying files from host to a container while preservingdocker cp -aoption for copying all uid/gid information.- A picture of a cute animal (not mandatory but encouraged)