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

Fix command exec -- COMMAND can not contain spaces #43663

Merged
merged 1 commit into from May 23, 2017

Conversation

Projects
None yet
9 participants
@shiywang
Member

shiywang commented Mar 25, 2017

Fixes #7688
the problem is when you execute command:
cluster/kubectl.sh exec -p client-blue-8yw37 -c client -i -t -- 'ls -t /usr'
the args is
[client-blue-8yw37 , ls -t /usr]
instead of
[client-blue-8yw37, ls, -t, /usr]
@kubernetes/sig-cli-pr-reviews, so I add a warning, wdyt ?
cc @ymqytw @adohe @fabianofranz

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 25, 2017

Hi @shiywang. 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 @k8s-bot 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.

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.

@k8s-reviewable

This comment has been minimized.

k8s-reviewable commented Mar 25, 2017

This change is Reviewable

@ncdc

This comment has been minimized.

Member

ncdc commented Mar 27, 2017

@shiywang does this break kubectl exec ... -- /bin/bash -c 'echo "hello world"'? I think it will, and we can't do that.

cc @smarterclayton @fabianofranz

@shiywang

This comment has been minimized.

Member

shiywang commented Mar 27, 2017

@ncdc sorry, you are right, we can't do that. I haven't figure out how to deal that in an elegant way. for now the best way I think of is add an example in --help to tell user be be cautious of quotas ?

@adohe

This comment has been minimized.

Member

adohe commented Mar 27, 2017

@shiywang I would think we just need to update the help doc, and don't break any current usage.

@shiywang shiywang force-pushed the shiywang:quato branch from f0021d9 to 49f8c11 May 9, 2017

@shiywang shiywang force-pushed the shiywang:quato branch from 49f8c11 to bc49ea2 May 9, 2017

@shiywang

This comment has been minimized.

Member

shiywang commented May 9, 2017

@adohe @ncdc I update some help information, wdyt ?

kubectl exec 123456-7890 -c ruby-container -i -t -- bash -il`))
kubectl exec 123456-7890 -c ruby-container -i -t -- bash -il
# List directory contents /usr from pod 123456-7890 and sort by modification time

This comment has been minimized.

@ncdc

ncdc May 15, 2017

Member

I would do something more along these lines:

# List contents of /usr from the first container of pod 123456-7890 and sort by modification time.
# If the command you want to execute in the pod has any flags in common (e.g. -i),
# you must use two dashes (--) to separate your command's flags/arguments.
# Also note, do not surround your command and its flags/arguments with quotes
# unless that is how you would execute it normally (i.e., do ls -t /usr, not "ls -t /usr").
kubectl exec 123456-7890 -i -t -- ls -t /usr

This comment has been minimized.

@shiywang

shiywang May 15, 2017

Member

thanks, updated, ptal

@shiywang shiywang force-pushed the shiywang:quato branch from bc49ea2 to 6d44c84 May 15, 2017

@k8s-merge-robot k8s-merge-robot added size/S and removed size/XS labels May 15, 2017

@shiywang

This comment has been minimized.

Member

shiywang commented May 18, 2017

@shiywang

This comment has been minimized.

Member

shiywang commented May 18, 2017

/release-note-none

@ncdc

This comment has been minimized.

Member

ncdc commented May 18, 2017

LGTM, you ok with it @fabianofranz ?

@shiywang

This comment has been minimized.

Member

shiywang commented May 22, 2017

ping @fabianofranz for approval

@fabianofranz

This comment has been minimized.

Contributor

fabianofranz commented May 22, 2017

/lgtm
/approve
Thanks @shiywang!

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented May 22, 2017

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabianofranz, shiywang

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented May 23, 2017

Automatic merge from submit-queue (batch tested with PRs 38990, 45781, 46225, 44899, 43663)

@k8s-merge-robot k8s-merge-robot merged commit 199465c into kubernetes:master May 23, 2017

17 checks passed

Jenkins Bazel Build Job succeeded.
Details
Jenkins GCE Node e2e Context retired. Status moved to "pull-kubernetes-node-e2e".
Jenkins GCE etcd3 e2e Context retired. Status moved to "pull-kubernetes-e2e-gce-etcd3".
Jenkins Kubemark GCE e2e Context retired. Status moved to "pull-kubernetes-kubemark-e2e-gce".
Jenkins kops AWS e2e Context retired. Status moved to "pull-kubernetes-e2e-kops-aws".
Jenkins unit/integration Context retired. Status moved to "pull-kubernetes-unit".
Jenkins verification Context retired. Status moved to "pull-kubernetes-verify".
Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation shiywang authorized
Details
pull-kubernetes-bazel Job succeeded.
Details
pull-kubernetes-e2e-gce-etcd3 Jenkins job succeeded.
Details
pull-kubernetes-e2e-kops-aws Jenkins job succeeded.
Details
pull-kubernetes-federation-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-node-e2e Jenkins job succeeded.
Details
pull-kubernetes-unit Jenkins job succeeded.
Details
pull-kubernetes-verify Jenkins job succeeded.
Details

@shiywang shiywang deleted the shiywang:quato branch May 23, 2017

@aranair

This comment has been minimized.

aranair commented Nov 2, 2018

Hmm, seems like this will be unable to handle nested commands? (and that echo hello world example above wouldn't work too).

kubectl exec ... -- bash -c "node -e 'console.log(123)'"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment