Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Carry 643] cp cmd #995

Merged
merged 1 commit into from
Apr 21, 2022
Merged

[Carry 643] cp cmd #995

merged 1 commit into from
Apr 21, 2022

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Apr 18, 2022 •
edited

Based on Fahed Dorgaa's PR #643 but preserves the file owner information as in docker cp.
This implementation also avoids mixing up the archive/tar pkg and tar command.

nerdctl cp -a is not implemented, as the actual behavior of docker cp -a does not seem clearly defined.

Tests are added to to cover the conditions listed in https://docs.docker.com/engine/reference/commandline/cp/

TODO (low priority, probably in another PR): Support stdio tar balls such as nerdctl cp - DST and nerdctl cp SRC -

Co-authored-by: fahed dorgaa @fahedouch
Signed-off-by: Akihiro Suda @AkihiroSuda

Closes #643
Closes #212

@AkihiroSuda AkihiroSuda changed the title Carry 443] cp cmd [Carry 643] cp cmd Apr 18, 2022
@AkihiroSuda AkihiroSuda mentioned this pull request Apr 18, 2022
Closed
@AkihiroSuda AkihiroSuda added this to the v0.19.0 (tentative) milestone Apr 18, 2022
@AkihiroSuda AkihiroSuda added the enhancement New feature or request label Apr 18, 2022
@fahedouch fahedouch self-requested a review April 18, 2022 16:25
@AkihiroSuda AkihiroSuda force-pushed the carry-643-cp branch 2 times, most recently from f0bd83d to e87ea8f Compare April 19, 2022 09:34
@AkihiroSuda AkihiroSuda marked this pull request as ready for review April 19, 2022 09:34
@AkihiroSuda AkihiroSuda requested a review from ktock April 19, 2022 09:34
AkihiroSuda
AkihiroSuda commented Apr 19, 2022
View reviewed changes
README.md Outdated
- `nerdctl cp [OPTIONS] CONTAINER:SRC_PATH DEST_PATH|-`
- `nerdctl cp [OPTIONS] SRC_PATH|- CONTAINER:DEST_PATH`

:warning: This command is not designed to be used with untrusted containers. Unexpected behavior of `nerdctl cp` are not treated as a vulnerability. Users must be conscious of that.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @dmcgowan @samuelkarp for the security policy

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been hoping to find time to review this PR (and its predecessor) but have been unable to do so.

With that said, maybe we can reword the warning a bit?

⚠️ nerdctl cp is designed only for use with trusted, cooperating containers. Using nerdctl cp with untrusted or malicious containers is unsupported and may not provide protection against unexpected behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, reworded

@AkihiroSuda AkihiroSuda force-pushed the carry-643-cp branch 3 times, most recently from e0933c6 to d7de290 Compare April 19, 2022 10:26
@AkihiroSuda
Copy link
Member Author

I'm planning to release v0.19 after merging this

cc @fahedouch

@AkihiroSuda
Copy link
Member Author

CI is failing for containerd@main due to:

ktock
ktock reviewed Apr 20, 2022
View reviewed changes
cmd/nerdctl/cp_linux.go Outdated
SilenceErrors: true,
}

cpCommand.Flags().BoolP("follow-link", "L", false, "Always follow symbol link in SRC_PATH.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/symbol link/symbolic link?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

cmd/nerdctl/cp_linux.go Outdated
Comment on lines 217 to 221
tarC := []string{tarBinary, "-c", "-f"}
if followSymlink {
tarC = append(tarC, "-h")
}
tarC = append(tarC, "-", tarCArg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates tar -c -f -h - which is not the right syntax. This should be tar -h -c -f - or something.

# nerdctl --debug cp -L /tmp/hello-sym ubuntu-b1768:/hello-sym2-follow
nerdctl --debug cp -L /tmp/hello-sym ubuntu-b1768:/hello-sym2-follow
DEBU[0000] executing [/bin/tar -c -f -h - hello-sym] in "/tmp" 
DEBU[0000] executing [/bin/tar -x -f -] in "/proc/245919/root/hello-sym2-follow" 
/bin/tar: -: Cannot stat: No such file or directory
/bin/tar: Exiting with failure status due to previous errors
/bin/tar: This does not look like a tar archive
/bin/tar: Exiting with failure status due to previous errors
FATA[0000] failed to wait [/bin/tar -c -f -h - hello-sym]: exit status 2

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

ktock
ktock approved these changes Apr 20, 2022
View reviewed changes
Copy link
Member

@ktock ktock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on green

fahedouch
fahedouch reviewed Apr 20, 2022
View reviewed changes
cmd/nerdctl/cp_linux.go
if destSpec.Path == "-" {
return fmt.Errorf("support for writing a tar archive to stdout is not implemented yet")
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to throw error if if len(srcSpec.Path) == 0 || len(destSpec.Path) == 0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

pkg/tarutil/tarutil.go
if exe, err := exec.LookPath("tar"); err == nil {
return exe, isGNU(exe), nil
}
return "", false, fmt.Errorf("failed to find `tar` binary")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like you check multiple paths, can we also add a flag --tar-path to point to host tar ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covered by an env var $TAR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AkihiroSuda We need to add this information to the help section so that it is visible to users

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@fahedouch @AkihiroSuda
[Carry 643] cp cmd
85ca787 
Based on Fahed Dorgaa's PR 643 but preserves the file owner information as in `docker cp`.
This implementation also avoids mixing up the `archive/tar` pkg and `tar` command.

`nerdctl cp -a` is not implemented, as the actual behavior of `docker cp -a` does not seem clearly defined.

Tests are added to to cover the conditions listed in https://docs.docker.com/engine/reference/commandline/cp/

TODO (low priority): Support stdio tar balls such as `nerdctl cp - DST` and `nerdctl cp SRC -`

Co-authored-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
@AkihiroSuda AkihiroSuda merged commit 3ed9238 into containerd:master Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuelkarp samuelkarp samuelkarp left review comments

@fahedouch fahedouch fahedouch approved these changes

@ktock ktock ktock approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
v0.19.0
Development

Successfully merging this pull request may close these issues.

nerdctl cp design
4 participants
@AkihiroSuda @samuelkarp @fahedouch @ktock