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.

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

Remove symlink support from kubectl cp #82143

Merged
merged 1 commit into from Sep 3, 2019

Conversation

@soltysh
Copy link
Contributor

commented Aug 29, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Based on the recent discussion in SIG-CLI we've decided to drop support when copying data from pods.

Special notes for your reviewer:
/assign @liggitt @tallclair
Only the second commit matters, the first is coming from #82087

Does this PR introduce a user-facing change?:

`kubectl cp` no longer supports copying symbolic links from containers; to support this use case, see `kubectl exec --help` for examples using `tar` directly
@soltysh

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

/sig cli
/priority important-longterm

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Some packages in hack/.staticcheck_failures are passing staticcheck. Please remove them.

  pkg/kubectl/cmd/cp
Executing tests from //pkg/kubectl/cmd/cp:go_default_test
-----------------------------------------------------------------------------
--- FAIL: TestTarUntar (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
@@ -143,6 +148,8 @@ func extractFileSpec(arg string) (fileSpec, error) {

// Complete completes all the required options
func (o *CopyOptions) Complete(f cmdutil.Factory, cmd *cobra.Command) error {
o.cmd = cmd

This comment has been minimized.

Copy link
@liggitt

liggitt Aug 29, 2019

Member

rather than holding a pointer to the command, consider storing the name of the parent command here (e.g. o.ExecParentCmdName = cmd.Parent().Name()

then, make the hint about using exec condition on ExecParentCmdName being non-empty. that way, CopyOptions stays reusable

if err := os.Symlink(linkname, destFileName); err != nil {
return err
}
fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is a symlink, skipping (consider using %s exec for symlink support)\n", destFileName, o.cmd.Parent().Name())

This comment has been minimized.

Copy link
@liggitt

liggitt Aug 29, 2019

Member

this should refer users to help describing how to use exec for symlink support... it is not immediately obvious how you would do that (even just see cp command help for details)

if err := os.Symlink(linkname, destFileName); err != nil {
return err
}
fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is a symlink, skipping (consider using %s exec for symlink support)\n", destFileName, o.cmd.Parent().Name())

This comment has been minimized.

Copy link
@tallclair

tallclair Aug 29, 2019

Member

nit: this is verbose for an error that might be printed a bunch of times. Maybe simplify it to something like:

Suggested change
fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is a symlink, skipping (consider using %s exec for symlink support)\n", destFileName, o.cmd.Parent().Name())
fmt.Fprintf(o.IOStreams.ErrOut, "skipping symlink: %q -> %q\n", destFileName, linkTarget)

This comment has been minimized.

Copy link
@liggitt

liggitt Aug 29, 2019

Member

I'd tend to agree. the pointer to exec can be printed just once

return err
}
fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is a symlink, skipping (consider using %s exec for symlink support)\n", destFileName, o.cmd.Parent().Name())
continue

This comment has been minimized.

Copy link
@odinuge

odinuge Aug 30, 2019

Member

This continue makes no difference, so I think it should be omitted.

Another alternative (my personal favorite) would be to remove the else and keep the continue to make the code path even cleaner and easier to read.

This comment has been minimized.

Copy link
@soltysh

soltysh Sep 2, 2019

Author Contributor

Fair point, I prefer the no else approach too.

if err := os.Symlink(linkname, destFileName); err != nil {
return err
}
fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is a symlink, skipping (consider using %s exec for symlink support)\n", destFileName, o.cmd.Parent().Name())

This comment has been minimized.

Copy link
@odinuge

odinuge Aug 30, 2019

Member

I think o.cmd can be nil in some of the tests, as the o.Complete func that sets cmd isn't executed in all of them. The applies to at least TestTarUntar.

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

/milestone v1.16
/area security

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 31, 2019
@soltysh soltysh force-pushed the soltysh:cp_no_links branch from 73b8ea8 to 2a615ef Sep 2, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Sep 2, 2019
@soltysh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

@liggitt @tallclair @odinuge updated, sample output:

$ kubectl cp hello-1-b5fwp:/tmp/test ./
tar: Removing leading `/' from member names
warning: file "link" is a symlink, skipping (consider using "kubectl exec -n "" "hello-1-b5fwp" -- tar cf - "/tmp/test" | tar xf -")
warning: skipping symlink: "link2" -> "file2"
@soltysh soltysh force-pushed the soltysh:cp_no_links branch from 2a615ef to 258de3e Sep 2, 2019
@soltysh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

/retest

1 similar comment
@soltysh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

/retest

tar cf - /tmp/foo | kubectl exec -i -n <some-namespace> <some-pod> -- tar xf -
# Copy /tmp/foo from a remote pod to /tmp/bar locally
kubectl exec -n <some-namespace> <some-pod> -- tar cf - /tmp/foo | tar xf -

This comment has been minimized.

Copy link
@liggitt

liggitt Sep 3, 2019

Member

nit: does this copy to /tmp/bar like the description says?

# file mode preservation consider using 'kubectl exec'.
# Copy /tmp/foo local file to /tmp/bar in a remote pod in namespace <some-namespace>
tar cf - /tmp/foo | kubectl exec -i -n <some-namespace> <some-pod> -- tar xf -

This comment has been minimized.

Copy link
@liggitt

liggitt Sep 3, 2019

Member

nit: does this copy to /tmp/bar like the description says?

// from within tar archive.
evaledPath, err := filepath.EvalSymlinks(baseName)
if mode&os.ModeSymlink != 0 {
if !symlinkWarningPrinted {

This comment has been minimized.

Copy link
@liggitt

liggitt Sep 3, 2019

Member

&& len(o.ExecParentCmdName) > 0

pkg/kubectl/cmd/cp/cp.go Show resolved Hide resolved
@soltysh soltysh force-pushed the soltysh:cp_no_links branch from 258de3e to 786acdb Sep 3, 2019
@soltysh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

@liggitt nits addressed, comments answered, ptal

@liggitt

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

/lgtm
/approve
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 3, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, soltysh

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@odinuge
odinuge approved these changes Sep 3, 2019
Copy link
Member

left a comment

/lgtm

@odinuge

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

/retest

evaledPath, err := filepath.EvalSymlinks(baseName)
if mode&os.ModeSymlink != 0 {
if !symlinkWarningPrinted && len(o.ExecParentCmdName) > 0 {
fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is a symlink, skipping (consider using \"%s exec -n %q %q -- tar cf - %q | tar xf -\")\n", destFileName, o.ExecParentCmdName, src.PodNamespace, src.PodName, src.File)

This comment has been minimized.

Copy link
@tallclair

tallclair Sep 3, 2019

Member

nit: I would make this error message more consistent with the other one, so they can be visually scanned together

Suggested change
fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is a symlink, skipping (consider using \"%s exec -n %q %q -- tar cf - %q | tar xf -\")\n", destFileName, o.ExecParentCmdName, src.PodNamespace, src.PodName, src.File)
fmt.Fprintf(o.IOStreams.ErrOut, "warning: skipping symlink: %q -> %q (consider using \"%s exec -n %q %q -- tar cf - %q | tar xf -\")\n", destFileName, o.ExecParentCmdName, src.PodNamespace, src.PodName, src.File)
@fejta-bot

This comment has been minimized.

Copy link

commented Sep 3, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 35e0f17 into kubernetes:master Sep 3, 2019
24 checks passed
24 checks passed
cla/linuxfoundation soltysh authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@soltysh soltysh deleted the soltysh:cp_no_links branch Sep 4, 2019
@soltysh soltysh referenced this pull request Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.