Opt out of chowning and chmoding from kubectl cp. #69573

bjhaid · 2018-10-09T12:35:53Z

What this PR does / why we need it:

kubectl cp relies on tar to extract the copied file/directory in the
container, tar by default attempts to chown/chmod the extracted file
after extraction if the user is the "superuser"(root)

--same-owner
try extracting files with the same ownership as exists in the archive
(default for superuser)
-p, --preserve-permissions, --same-permissions
extract information about file permissions (default for superuser)

This fails in environment where the container runs as root but is not
granted the OWNER or CHOWN capability.

Before this patch below was the behavior of kubectl cp

kubectl cp README.md foo-67b6fcbd4c-qjlt6:/tmp
tar: README.md: Cannot change ownership to uid 1000, gid 1000: Operation
not permitted
tar: Exiting with failure status due to previous errors
command terminated with exit code 2
kubectl exec -it foo-67b6fcbd4c-qjlt6 -- ls -l /tmp/README.md
-rw------- 1 1000 1000 3179 Oct  7 22:00 /tmp/README.md

After this patch

kubectl cp -x a foo-67b6fcbd4c-qjlt6:/
kubectl exec -it foo-67b6fcbd4c-qjlt6 -- ls -l /tmp/README.md
-rw-r--r-- 1 root root 3179 Oct  7 22:00 /tmp/README.md

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #69572

Special notes for your reviewer:

I chose the short flag -x randomly, I don't know if there's an existing convention around selecting flags, also I don't have any tests for this, but if there's some e2e test that needs to be updated I'll appreciate pointers

Release note:

Opt out of chowning and chmoding from kubectl cp.

k8s-ci-robot · 2018-10-09T12:35:55Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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. I understand the commands that are listed here.

bjhaid · 2018-10-09T12:37:45Z

/sig cli

soltysh

Can you please add a unit test in https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cp/cp_test.go ?

soltysh · 2018-10-09T15:47:45Z

pkg/kubectl/cmd/cp/cp.go

@@ -98,6 +99,7 @@ func NewCmdCp(f cmdutil.Factory, ioStreams genericclioptions.IOStreams) *cobra.C
 		},
 	}
 	cmd.Flags().StringVarP(&o.Container, "container", "c", o.Container, "Container name. If omitted, the first container in the pod will be chosen")
+	cmd.Flags().BoolVarP(&o.NoPreserve, "no-preserve", "x", false, "The copied file/directory's ownership and permissions will not be preserved in the container")


Short flags are very sparse, don't use random values. Just don't specify, it's fine not to have it.

Addressed thanks!

neolit123 · 2018-10-10T05:24:53Z

/kind feature

bjhaid · 2018-10-10T14:08:53Z

Can you please add a unit test in https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cp/cp_test.go ?

I spent sometime looking at the tests especially around here:

https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cp/cp_test.go#L576-L644

And I can't seem to find a way to actually exercise the behavior introduced by this PR, I'll am open to ideas, if you have thoughts on how I can actually exercise this feature in the unit test.

Thanks!

soltysh · 2018-10-16T11:53:43Z

And I can't seem to find a way to actually exercise the behavior introduced by this PR, I'll am open to ideas, if you have thoughts on how I can actually exercise this feature in the unit test.

Verify input parameters of the ExecOptions and see if they match your expectations. That's very simple but should catch all modifications.

soltysh · 2018-10-16T11:53:49Z

/ok-to-test

bjhaid · 2018-10-17T13:39:39Z

@soltysh please re-review and let me know if the tests matches what you had in mind. Thanks!

bjhaid · 2018-10-17T14:22:18Z

/test pull-kubernetes-verify

soltysh · 2018-10-18T13:47:17Z

pkg/kubectl/cmd/cp/cp_test.go

+			File:         "foo",
+		}
+		opts.Complete(tf, cmd)
+		t.Run(name, func(t *testing.T) {


Generally this should be the entire test case, so move this to the top of the for loop.

I move the things above this out of the for loop since they don't need to be in the loop.

soltysh · 2018-10-18T13:47:35Z

pkg/kubectl/cmd/cp/cp_test.go

+			options := &kexec.ExecOptions{}
+			opts.NoPreserve = test.nopreserve
+			err = opts.copyToPod(src, dest, options)
+			for i, cmd := range test.expectedCmd {


Use reflect.DeepEqual instead it read better.

addressed thanks!

`kubectl cp` relies on tar to extract the copied file/directory in the container, tar by default attempts to chown/chmod the extracted file after extraction if the user is the "superuser"(root) ``` --same-owner try extracting files with the same ownership as exists in the archive (default for superuser) -p, --preserve-permissions, --same-permissions extract information about file permissions (default for superuser) ``` This fails in environment where the container runs as root but is not granted the OWNER or CHOWN capability. Before this patch below was the behavior of `kubectl cp` ``` kubectl cp README.md foo-67b6fcbd4c-qjlt6:/tmp tar: README.md: Cannot change ownership to uid 1000, gid 1000: Operation not permitted tar: Exiting with failure status due to previous errors command terminated with exit code 2 kubectl exec -it foo-67b6fcbd4c-qjlt6 -- ls -l /tmp/README.md -rw------- 1 1000 1000 3179 Oct 7 22:00 /tmp/README.md ``` After this patch ``` kubectl cp -x a foo-67b6fcbd4c-qjlt6:/ kubectl exec -it foo-67b6fcbd4c-qjlt6 -- ls -l /tmp/README.md -rw-r--r-- 1 root root 3179 Oct 7 22:00 /tmp/README.md ```

soltysh

/lgtm
/approve

soltysh · 2018-10-18T19:10:22Z

pkg/kubectl/cmd/cp/cp_test.go

+			options := &kexec.ExecOptions{}
+			opts.NoPreserve = test.nopreserve
+			err = opts.copyToPod(src, dest, options)
+			if !(reflect.DeepEqual(test.expectedCmd, options.Command)) {


Nit for future: the bracket are not necessary it's ok to write:

if !reflect.DeepEqual(test.expectedCmd, options.Command)

k8s-ci-robot · 2018-10-18T19:10:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bjhaid, 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:

~~pkg/kubectl/OWNERS~~ [soltysh]

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

jimmywku · 2020-03-14T11:25:04Z

Hi,
Whether this fix committed to any stable k8s version already? Since I also met this issue, while I`m using the stable version 1.17. If already merged to any version, which arg it is for this privilege. Thanks.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 9, 2018

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 9, 2018

k8s-ci-robot requested a review from ericchiang October 9, 2018 12:35

k8s-ci-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Oct 9, 2018

k8s-ci-robot requested a review from ghodss October 9, 2018 12:35

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. 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 Oct 9, 2018

bjhaid force-pushed the master branch from 0d7aeff to 545a34d Compare October 9, 2018 12:37

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 9, 2018

soltysh requested changes Oct 9, 2018

View reviewed changes

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 10, 2018

bjhaid force-pushed the master branch from 545a34d to f657dfa Compare October 10, 2018 14:04

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 16, 2018

bjhaid force-pushed the master branch from f657dfa to 9e2e4bc Compare October 17, 2018 13:38

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 17, 2018

bjhaid force-pushed the master branch from 9e2e4bc to 7f27a9d Compare October 17, 2018 14:26

soltysh requested changes Oct 18, 2018

View reviewed changes

bjhaid force-pushed the master branch from 7f27a9d to 44bed94 Compare October 18, 2018 14:16

soltysh approved these changes Oct 18, 2018

View reviewed changes

k8s-ci-robot assigned soltysh Oct 18, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2018

k8s-ci-robot merged commit 00dd32b into kubernetes:master Oct 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Opt out of chowning and chmoding from kubectl cp. #69573

Opt out of chowning and chmoding from kubectl cp. #69573

bjhaid commented Oct 9, 2018 •

edited

k8s-ci-robot commented Oct 9, 2018

bjhaid commented Oct 9, 2018

soltysh left a comment

soltysh Oct 9, 2018

bjhaid Oct 10, 2018

neolit123 commented Oct 10, 2018

bjhaid commented Oct 10, 2018

soltysh commented Oct 16, 2018

soltysh commented Oct 16, 2018

bjhaid commented Oct 17, 2018

bjhaid commented Oct 17, 2018

soltysh Oct 18, 2018

bjhaid Oct 18, 2018

soltysh Oct 18, 2018

bjhaid Oct 18, 2018

soltysh left a comment

soltysh Oct 18, 2018

k8s-ci-robot commented Oct 18, 2018

jimmywku commented Mar 14, 2020 •

edited

Opt out of chowning and chmoding from kubectl cp. #69573

Opt out of chowning and chmoding from kubectl cp. #69573

Conversation

bjhaid commented Oct 9, 2018 • edited

k8s-ci-robot commented Oct 9, 2018

bjhaid commented Oct 9, 2018

soltysh left a comment

Choose a reason for hiding this comment

soltysh Oct 9, 2018

Choose a reason for hiding this comment

bjhaid Oct 10, 2018

Choose a reason for hiding this comment

neolit123 commented Oct 10, 2018

bjhaid commented Oct 10, 2018

soltysh commented Oct 16, 2018

soltysh commented Oct 16, 2018

bjhaid commented Oct 17, 2018

bjhaid commented Oct 17, 2018

soltysh Oct 18, 2018

Choose a reason for hiding this comment

bjhaid Oct 18, 2018

Choose a reason for hiding this comment

soltysh Oct 18, 2018

Choose a reason for hiding this comment

bjhaid Oct 18, 2018

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

soltysh Oct 18, 2018

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 18, 2018

jimmywku commented Mar 14, 2020 • edited

bjhaid commented Oct 9, 2018 •

edited

jimmywku commented Mar 14, 2020 •

edited