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

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

Merged
merged 1 commit into from Oct 18, 2018

Conversation

@bjhaid
Copy link
Contributor

bjhaid commented Oct 9, 2018

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

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 9, 2018

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.


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

This comment has been minimized.

Copy link
Contributor Author

bjhaid commented Oct 9, 2018

/sig cli

@soltysh
Copy link
Contributor

soltysh left a comment

@@ -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")

This comment has been minimized.

Copy link
@soltysh

soltysh Oct 9, 2018

Contributor

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

This comment has been minimized.

Copy link
@bjhaid

bjhaid Oct 10, 2018

Author Contributor

Addressed thanks!

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Oct 10, 2018

/kind feature

@bjhaid bjhaid force-pushed the bjhaid:master branch from 545a34d to f657dfa Oct 10, 2018

@bjhaid

This comment has been minimized.

Copy link
Contributor Author

bjhaid commented Oct 10, 2018

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

This comment has been minimized.

Copy link
Contributor

soltysh commented Oct 16, 2018

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

This comment has been minimized.

Copy link
Contributor

soltysh commented Oct 16, 2018

/ok-to-test

@bjhaid bjhaid force-pushed the bjhaid:master branch from f657dfa to 9e2e4bc Oct 17, 2018

@k8s-ci-robot k8s-ci-robot added size/L and removed size/S labels Oct 17, 2018

@bjhaid

This comment has been minimized.

Copy link
Contributor Author

bjhaid commented Oct 17, 2018

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

@bjhaid

This comment has been minimized.

Copy link
Contributor Author

bjhaid commented Oct 17, 2018

/test pull-kubernetes-verify

@bjhaid bjhaid force-pushed the bjhaid:master branch from 9e2e4bc to 7f27a9d Oct 17, 2018

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

This comment has been minimized.

Copy link
@soltysh

soltysh Oct 18, 2018

Contributor

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

This comment has been minimized.

Copy link
@bjhaid

bjhaid Oct 18, 2018

Author Contributor

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

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

This comment has been minimized.

Copy link
@soltysh

soltysh Oct 18, 2018

Contributor

Use reflect.DeepEqual instead it read better.

This comment has been minimized.

Copy link
@bjhaid

bjhaid Oct 18, 2018

Author Contributor

addressed thanks!

Opt out of chowning and chmoding from kubectl cp.
`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
```

@bjhaid bjhaid force-pushed the bjhaid:master branch from 7f27a9d to 44bed94 Oct 18, 2018

@soltysh
Copy link
Contributor

soltysh left a comment

/lgtm
/approve

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

This comment has been minimized.

Copy link
@soltysh

soltysh Oct 18, 2018

Contributor

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

if !reflect.DeepEqual(test.expectedCmd, options.Command)
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 18, 2018

[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:

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

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

14 of 18 checks passed

pull-kubernetes-e2e-gce Job triggered.
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-e2e-kops-aws Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation bjhaid authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
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.