kubernetes / kubectl

Issue tracker and mirror of kubectl code
Apache License 2.0
2.83k stars 913 forks source link

kubectl cp prints tar warnings to stdout #1576

Closed hhallman closed 6 months ago

hhallman commented 6 months ago

What happened?

When using kubectl cp, it seems that tar's stderr output is sent to kubectl's stdout. In my case, this behavior interfers with a script.

Notice how tar prints to stderr:

$ touch /tmp/afile
$ tar cvf /tmp/anarchive /tmp/afile > stdout 2> stderr
$ cat stdout
$ cat stderr
tar: Removing leading '/' from member names
a tmp/afile

However, when using kubectl cp, the output is sent to stdout:

$ kubectl cp mypod:/tmp/afile mylocalfile > stdout 2> stderr
$ cat stdout
tar: removing leading '/' from member names
cat stderr

What did you expect to happen?

I would expect the tar output to be sent to stderr

How can we reproduce it (as minimally and precisely as possible)?

Check that stdout does NOT have the leading / warning:

$ touch afile
$ kubectl cp afile mypod:/tmp/afile
$ kubectl cp mypod:/tmp/afile mylocalfile | grep 'removing leading' && echo 'FAIL, the tar warning removing leading / should be on stderr'

Check that stderr DOES have the leading / warning:

$ touch afile
$ kubectl cp afile mypod:/tmp/afile
$ kubectl cp mypod:/tmp/afile mylocalfile > stdout 2>stderr
$ cat stderr | grep 'removing leading' && echo 'PASS, the tar warning removing leading / is on stderr'

Anything else we need to know?

My reason for requesting this change is that the following script would be easier to write and more intuitive

function getReport {
    kubectl cp mypod:/tmp/report /tmp/localreport 1>&2 || return 1
    echo /tmp/localreport
    return 0
}

After the requested change:

function getReport {
    kubectl cp mypod:/tmp/report /tmp/localreport || return 1
    echo /tmp/localreport
    return 0
}

The getReport function is further used as so:

function publishReport {
    reportFile="$(getReport)" || return 1
    process "$reportFile"
}

As you can see, publishReport depends on the output from getReport to be clean, which it will not be if kubectl prints to stdout.

Kubernetes version

```console $ kubectl version Client Version: v1.29.0 Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3 Server Version: v1.26.6 WARNING: version difference between client (1.29) and server (1.26) exceeds the supported minor version skew of +/-1 ```

Cloud provider

n/a

OS version

MacOS Sonoma 14.4

Install tools

Container runtime (CRI) and version (if applicable)

Related plugins (CNI, CSI, ...) and versions (if applicable)

k8s-ci-robot commented 6 months ago

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
hhallman commented 6 months ago

/sig cli

hhallman commented 6 months ago

Hi,

Thank you for considering my change request. This is my first contact with the k8s project, and I would love to get the chance to make this change myself is feasible. I have found the relevant place in the kubectl codebase. I have read the contribution guidelines and followed them as best as I can. Sorry beforehand if I did something incorrectly.

Thank you!

Ritikaa96 commented 6 months ago

i think it is better to be transferred to kubectl /transfer kubectl

marcio-pessoa commented 6 months ago

I struggled with this same problem, so I created a small program called kubectl-cp that has no dependencies and allows me to freely copy files between containers running on Kubernetes:

https://github.com/marcio-pessoa/kubectl-cp

I hope this can help.

eddiezane commented 6 months ago

Thanks for opening this detailed issue. Your request is valid but we think of tar here as an (unfortunate) implemntation detail for copy.

In general, for all of the "remote" commands we combine both streams into stdout. The design choice here is to reserve stderr for actual command feedback of the kubectl command you are running.

We're actually drafting up a new proposal to move copy logic down into the container runtime and do away with the requirement for tar with kubectl cp. Stay tuned for that.

/close

k8s-ci-robot commented 6 months ago

@eddiezane: Closing this issue.

In response to [this](https://github.com/kubernetes/kubectl/issues/1576#issuecomment-2024194996): >Thanks for opening this detailed issue. Your request is valid but we think of tar here as an (unfortunate) implemntation detail for copy. > >In general, for all of the "remote" commands we combine both streams into stdout. The design choice here is to reserve stderr for actual command feedback of the kubectl command you are running. > >We're actually drafting up a new proposal to move copy logic down into the container runtime and do away with the requirement for tar with kubectl cp. Stay tuned for that. > >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
hhallman commented 6 months ago

Thank you @eddiezane for your consideration and feedback. As you may see in my details, the workaround for this issue is very simple, so no problem.

eddiezane commented 4 months ago

https://github.com/kubernetes/enhancements/issues/4582