use internal copy rather than tar for copy fix #6003 #6279
Conversation
This patch uses internal copy to copy containers rather than tar. The problem with tar is that it does not copy symlinks pointing outside of the volume.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: joequant The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @joequant. Thanks for your PR. I'm waiting for a containers member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
the
needs-ok-to-test
|
/ok-to-test |
openshift-ci-robot
added
ok-to-test
and removed
needs-ok-to-test
|
@nalind PTAL |
|
|
||
| for _, entry := range entries { | ||
| srcPath := filepath.Join(src, entry.Name()) | ||
| dstPath := filepath.Join(dst, entry.Name()) |
There was a problem hiding this comment.
I think you want to user securejoin for these
| func (c *Container) copyWithOwnerAndPerms(src, dst string) error { | ||
| if samefile(src, dst) { | ||
| return &CopyError{src, dst} | ||
| } | ||
|
|
||
| srcStat, err := os.Lstat(src) | ||
| if err != nil { | ||
| return err | ||
| } else if specialfile(srcStat) { | ||
| return &CopyError{src, dst} | ||
| } | ||
|
|
||
| if dstInfo, err := os.Stat(dst); err == nil && dstInfo.Mode().IsDir() { | ||
| dst = filepath.Join(dst, filepath.Base(src)) | ||
| } else if err != nil && !os.IsNotExist(err) { | ||
| return err | ||
| } | ||
|
|
||
| if IsSymlink(srcStat) { | ||
| if srcLink, err := os.Readlink(src); err != nil { | ||
| return err | ||
| } else if err := os.Symlink(srcLink, dst); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| // Do the actual copy | ||
| fsrc, err := os.Open(src) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer fsrc.Close() | ||
|
|
||
| fdst, err := os.Create(dst) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer fdst.Close() | ||
|
|
||
| if size, err := io.Copy(fdst, fsrc); err != nil { | ||
| return err | ||
| } else if size != srcStat.Size() { | ||
| return fmt.Errorf("%s: %d/%d copied", src, size, srcStat.Size()) | ||
| } | ||
|
|
||
| if err = c.copyOwnerAndPerms(src, dst); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
could we use CopyFile from github.com/mrunalp/fileutils here?
There was a problem hiding this comment.
The tar-based logic would have attempted to preserve timestamps and xattrs on items it copied, and made an effort to preserve hard links among multiple items being copied. If we don't want to lose those features (whether podman wants to keep those things could well be different than the previous implementation), they'll need to be added back here.
There was a problem hiding this comment.
Xattrs are likely important. I don't know if timestamps are; need to check.
There was a problem hiding this comment.
Hardlinks also, to save on size.
| return (fi.Mode() & os.ModeNamedPipe) == os.ModeNamedPipe | ||
| } | ||
|
|
||
| func IsSymlink(fi os.FileInfo) bool { |
There was a problem hiding this comment.
This function looks like it doesn't need to be exported.
|
@joequant Still working on this, needs a rebase and some comments addressed. |
|
Friendly ping. |
|
@mheon If you are going to work on rewriting copy, you might want to take this over. |
|
A friendly reminder that this PR had no activity for 30 days. |
|
The Copy rewrite is continuing on Buildah and is progressing, I think we should close this and work on the new Copy once it merges into Buildah. |
|
Thanks. Also any ETA for when the changes will get merged in? I'm using docker and would like to migrate over to podman. |
|
Can you link the issue that tracks this on buildah? Thanks. |
github-actions
bot
added
the
locked - please file new issue/PR
This patch uses internal copy to copy containers rather than
tar. The problem with tar is that it does not copy symlinks pointing
outside of the volume.
This is a very tricky patch to get right because you have to get the
ownerships and permissions right as well as handle symlinks
correctly. Off the shelf copy trees don't work because they dont
have the right combination of options to do the right thing.