Kubectl cp error using kubectl version 1.14.2 #659 && kubectl cp fails on symlinks #78211 #78785

pswica · 2019-06-07T05:16:55Z

What type of PR is this?

/kind bug

What this PR does / why we need it:
Fixes kubernetes/kubectl#659
Which issue(s) this PR fixes:
Fixes #78211
Fixes kubernetes/kubectl#659
Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fixes a bug where kubectl cp could not copy to a folder directly on the host system. Also fixes a bug when copying to symlinked absolute paths.

k8s-ci-robot · 2019-06-07T05:17:03Z

Hi @pswica. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

pswica · 2019-06-07T05:26:57Z

/assign @janetkuo

seans3 · 2019-06-07T17:44:01Z

/assign soltysh

pswica · 2019-06-08T05:41:48Z

/cc @draveness
would you be able to give this a review and start up a test?

draveness · 2019-06-08T06:52:49Z

pkg/kubectl/cmd/cp/cp.go

@@ -280,6 +280,15 @@ func (o *CopyOptions) copyFromPod(src, dest fileSpec) error {
 		return errFileCannotBeEmpty
 	}

+	stat, err := os.Stat(dest.File)


Thanks!

/ok-to-test
/priority backlog

Thanks a lot for taking the time to review!

dims · 2019-06-14T01:44:55Z

/assign @adohe

k8s-ci-robot · 2019-06-15T18:06:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pswica
To complete the pull request process, please assign adohe
You can assign the PR to them by writing /assign @adohe in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/kubectl/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pswica · 2019-06-16T16:10:12Z

/retest

pswica · 2019-06-16T19:05:31Z

/retest

…tl cp fails on symlinks kubernetes#78211

pswica · 2019-06-16T21:08:42Z

/retest

dixudx · 2019-06-17T02:53:47Z

pkg/kubectl/cmd/cp/cp.go

+		return "", err
+	}
+
+	if !os.IsNotExist(err) && stat.IsDir() {


Tar will append the right filename.
No need to do like this.

I needed to do this to address the kubectl cp bug that occurs now when you use to on a directory. That is why I need to do this.

@pswica Actually this is a bug of wrong prefix when untar. We do have a file prefix checking there.

In https://github.com/kubernetes/kubernetes/pull/78928/files#diff-5103d5139d8d0e3143b6d7b6d1934f50R339, I've included a fix for this issue.

Interesting. I guess it depends where you think the logic should go in implementing the path that gets put into tar. I prefer my approach as it cleans everything before sending it to tar. This, to me, separates the logic between the two functions and makes them more loosely coupled .

Perhaps @liggitt or @siosphere can weigh in here.

@dixudx another thing I'll mention, looking at your PR, is that it currently has a lot of failing tests:

I0616 14:51:04.155] should copy a file from a running Pod [It] I0616 14:51:24.751] should copy a file from a running Pod [It] I0616 14:53:57.520] runs ReplicaSets to verify preemption running path [It] ...

I don't think it would be fair to state that, at this moment, your solution fixes any bugs that mine fixes.

dixudx · 2019-06-17T02:54:30Z

pkg/kubectl/cmd/cp/cp.go

+// If destination is a directory, filename will be the same as the source.
+// Also, symlink destinations are converted to true paths
+func convertHostDestFilename(srcFile string, destFile string) (string, error) {
+	stat, err := os.Stat(destFile)


We only need to EvalSymlinks for destFile here. No more steps are needed.

The symlink steps were to address the other bug that my PR addresses, which is kubectl cp failing to symlinks on full paths. Keeping those bugs I address in mind should make more clear why the code was created.

soltysh · 2019-08-22T11:07:23Z

pkg/kubectl/cmd/cp/cp_test.go

+		},
+	}
+	for _, test := range tests {
+		err := os.Mkdir(test.destFile, 755)


It's better to create a temporary directory ioutil.TempDir at the beginning of the test and then just clean it. See the first lines of TestTarUntarWrongPrefix

kubernetes/staging/src/k8s.io/kubectl/pkg/cmd/cp/cp_test.go

Lines 397 to 406 in 8dea331

dir, err := ioutil.TempDir("", "input")

checkErr(t, err)

dir2, err := ioutil.TempDir("", "output")

checkErr(t, err)

dir = dir + "/"

defer func() {

os.RemoveAll(dir)

os.RemoveAll(dir2)

}()

soltysh · 2019-08-22T11:35:44Z

pkg/kubectl/cmd/cp/cp.go

 func (o *CopyOptions) copyFromPod(src, dest fileSpec) error {
 	if len(src.File) == 0 || len(dest.File) == 0 {
 		return errFileCannotBeEmpty
 	}

+	d, err := convertHostDestFilename(src.File, dest.File)


I've created an alternative here: #81782

booooh · 2019-09-01T20:00:14Z

Running into the same problem +1 for the fix

fejta-bot · 2019-11-30T20:04:09Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2019-12-30T20:48:52Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

k8s-ci-robot · 2019-12-30T20:48:59Z

@pswica: PR needs rebase.

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.

fejta-bot · 2020-01-29T21:32:19Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2020-01-29T21:32:28Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 7, 2019

k8s-ci-robot requested review from rootfs and seans3 June 7, 2019 05:18

k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 7, 2019

k8s-ci-robot assigned janetkuo Jun 7, 2019

pswica mentioned this pull request Jun 7, 2019

Kubectl cp error using kubectl version 1.14.2 kubernetes/kubectl#659

Closed

k8s-ci-robot assigned soltysh Jun 7, 2019

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 8, 2019

draveness reviewed Jun 8, 2019

View reviewed changes

k8s-ci-robot assigned adohe-zz Jun 14, 2019

pswica force-pushed the master branch from 2f1c9b1 to bae3778 Compare June 15, 2019 18:05

k8s-ci-robot removed the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 15, 2019

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 15, 2019

pswica force-pushed the master branch from bae3778 to 558c329 Compare June 15, 2019 18:08

pswica changed the title ~~Kubectl cp error using kubectl version 1.14.2 #659~~ Kubectl cp error using kubectl version 1.14.2 #659 && kubectl cp fails on symlinks #78211 Jun 15, 2019

pswica force-pushed the master branch from 558c329 to aa0ac6c Compare June 16, 2019 18:11

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 16, 2019

Kubectl cp error using kubectl version 1.14.2 kubernetes#659 && kubec…

872b533

…tl cp fails on symlinks kubernetes#78211

pswica force-pushed the master branch from aa0ac6c to 872b533 Compare June 16, 2019 19:54

dixudx suggested changes Jun 17, 2019

View reviewed changes

soltysh requested changes Aug 22, 2019

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 30, 2019

k8s-ci-robot closed this Jan 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kubectl cp error using kubectl version 1.14.2 #659 && kubectl cp fails on symlinks #78211 #78785

Kubectl cp error using kubectl version 1.14.2 #659 && kubectl cp fails on symlinks #78211 #78785

pswica commented Jun 7, 2019 •

edited

k8s-ci-robot commented Jun 7, 2019

pswica commented Jun 7, 2019

seans3 commented Jun 7, 2019

pswica commented Jun 8, 2019 •

edited

draveness Jun 8, 2019

pswica Jun 8, 2019

dims commented Jun 14, 2019

k8s-ci-robot commented Jun 15, 2019

pswica commented Jun 16, 2019

pswica commented Jun 16, 2019

pswica commented Jun 16, 2019

dixudx Jun 17, 2019

pswica Jun 17, 2019

dixudx Jun 17, 2019

pswica Jun 17, 2019

pswica Jun 17, 2019

dixudx Jun 17, 2019

pswica Jun 17, 2019

soltysh Aug 22, 2019

soltysh Aug 22, 2019

booooh commented Sep 1, 2019

fejta-bot commented Nov 30, 2019

fejta-bot commented Dec 30, 2019

k8s-ci-robot commented Dec 30, 2019

fejta-bot commented Jan 29, 2020

k8s-ci-robot commented Jan 29, 2020

	dir, err := ioutil.TempDir("", "input")
	checkErr(t, err)
	dir2, err := ioutil.TempDir("", "output")
	checkErr(t, err)

	dir = dir + "/"
	defer func() {
	os.RemoveAll(dir)
	os.RemoveAll(dir2)
	}()

Kubectl cp error using kubectl version 1.14.2 #659 && kubectl cp fails on symlinks #78211 #78785

Kubectl cp error using kubectl version 1.14.2 #659 && kubectl cp fails on symlinks #78211 #78785

Conversation

pswica commented Jun 7, 2019 • edited

k8s-ci-robot commented Jun 7, 2019

pswica commented Jun 7, 2019

seans3 commented Jun 7, 2019

pswica commented Jun 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dims commented Jun 14, 2019

k8s-ci-robot commented Jun 15, 2019

pswica commented Jun 16, 2019

pswica commented Jun 16, 2019

pswica commented Jun 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

booooh commented Sep 1, 2019

fejta-bot commented Nov 30, 2019

fejta-bot commented Dec 30, 2019

k8s-ci-robot commented Dec 30, 2019

fejta-bot commented Jan 29, 2020

k8s-ci-robot commented Jan 29, 2020

pswica commented Jun 7, 2019 •

edited

pswica commented Jun 8, 2019 •

edited