'podman cp' copy between host and container #2151
Conversation
cmd/podman/cp.go
Outdated
| } | ||
|
|
||
| if os.Geteuid() != 0 { | ||
| rootless.SetSkipStorageSetup(true) |
There was a problem hiding this comment.
On the rootless code here - can we possibly make this work with rootless by leaking the file descriptor for whatever we want to copy into the userns? Wouldn't work for folders, but might be enough to let us copy stuff into rootless containers without using vfs. @giuseppe
There was a problem hiding this comment.
I think we'd need to join the user namespace, once we have joined the user+mount namespace we can also access the storage.
@QiWang19 would you like to take care of it in this PR or would you prefer me to work on it as a followup patch?
Basically we need to use rootless.JoinDirectUserAndMountNS()
There was a problem hiding this comment.
@giuseppe sure 👌 , I prefer you work on that
There was a problem hiding this comment.
could we just add an error for now? cp should fail when running in rootless mode
cmd/podman/cp.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| idmappingopts, err := ctr.IDMappings() |
There was a problem hiding this comment.
I don't think ctr.IDMappings ever returns an err, so we should probably change the interface.
There was a problem hiding this comment.
I think it might return nil sometimes, though - do we need to check if it's non-nil?
There was a problem hiding this comment.
Camelcase the idmappingsopts identifier - idMappingOpts
|
I think we need to make sure the cases listed in https://docs.docker.com/engine/reference/commandline/cp/ right after the line beginning with |
cmd/podman/cp.go
Outdated
| } | ||
|
|
||
| for _, gsrc := range glob { | ||
| esrc, err := filepath.EvalSymlinks(gsrc) |
There was a problem hiding this comment.
Need to make sure this is scoped into the container if source is container
|
Also, I'd love more comments, the code is very hard to follow in places right now |
QiWang19
force-pushed
the
cp
branch
2 times, most recently
from
4719679 to
b385c3d
Compare
|
|
|
☔ The latest upstream changes (presumably #2184) made this pull request unmergeable. Please resolve the merge conflicts. |
openshift-ci-robot
added
the
needs-rebase
openshift-ci-robot
removed
the
needs-rebase
|
It helps when I press the right button and don't accidentally close PRs |
|
@mheon thanks for the explanations :) So yeah I running containers as a non-root user |
|
We should through an error right now when they run rootless until @giuseppe Fixes the issues. |
|
☔ The latest upstream changes (presumably #2274) made this pull request unmergeable. Please resolve the merge conflicts. |
openshift-ci-robot
added
the
needs-rebase
openshift-ci-robot
added
size/L
and removed
needs-rebase
cmd/podman/cp.go
Outdated
| srcPath = filepath.Join(mountPoint, ctr.WorkingDir(), srcPath) | ||
| } | ||
| } | ||
| glob, _ = filepath.Glob(srcPath) |
| } | ||
| } | ||
|
|
||
| copyFileWithTar := chrootarchive.CopyFileWithTarAndChown(chownOpts, digest.Canonical.Digester().Hash(), idMappingOpts.UIDMap, idMappingOpts.GIDMap) |
There was a problem hiding this comment.
Maybe a comment here explaining that these return functions for copying items?
cmd/podman/cp.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| if srcfi.IsDir() || dest[len(dest)-1] == os.PathSeparator { |
There was a problem hiding this comment.
Use strings.HasSuffix() instead of the len(dest)-1 here
|
@QiWang19 Lets finish this up tomorrow, I am back in office. |
Signed-off-by: Qi Wang <[email protected]>
|
LGTM |
|
/lgtm |
|
needs approve label to test tide? |
|
Oh, nobody approved |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon, QiWang19 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
fix #613 Copy files/folders between a container and the local filesystem
Signed-off-by: Qi Wang [email protected]