kubectl icon indicating copy to clipboard operation
kubectl copied to clipboard

Feature Request: detach-keys option/setting for pod terminal

Open Nuru opened this issue 4 years ago • 7 comments

This is a revival of #693 which was sort of duplicated by #878 but both went stale before being addressed. #878 labeled this behavior a bug and put it on the backlog. I consider it a feature request, but am happy to let SIGCLI decide.

kubectl run -it, kubectl exec -it, and kubectl attach -it connect to a Docker container in a manner parallel to docker run -it and docker attach. All these commands by default listen for the key sequence ctrl-p,ctrl-q to cause the command to detach from the container. However, unlike kubectl, the docker commands have a [--detach-keys option] and configuration property to allow you to specify an alternate key sequence to use instead of the default. kubectl is missing this option. Please implement it.

The default is problematic with bash because by default, ctrl-p on the bash command line brings up the previous command from the history list. Users typing ctrl-p once expect to see the previous command, but get nothing until they type another character, because the ctrl-p (being the first character in a potential 2-character command) is held back until the next character is sent. If the next character is ctrl-q the connection is closed and the container never sees the ctrl-p. If the next character is something else, then the ctrl-p and the next character are sent as soon as the second character is typed. If that character is ctrl-p, then the user will get the to see the command before the previous command, as they have just typed ctrl-p,ctrl-p. So getting to the immediately previous command requires typing ctrl-p followed by some other character that will not alter the command line. This is annoying at best.

Whatever issues people may have with the default detach key sequence in docker are alleviated by the presence of the --detach-keys option and the detachKeys property of the docker configuration. However kubectl does not have this option.

Please add a --detach-keys option that operates the same way as docker --detach-keys does.

Nuru avatar Jan 24 '21 01:01 Nuru

Sorry this keeps getting closed. We will certainly accept a PR on this if anyone wants to pick it up.

/triage accepted /priority backlog /help

cc @her

eddiezane avatar Feb 03 '21 18:02 eddiezane

@eddiezane: This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-help command.

In response to this:

Sorry this keeps getting closed. We will certainly accept a PR on this if anyone wants to pick it up.

/triage accepted /priority backlog /help

cc @her

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 avatar Feb 03 '21 18:02 k8s-ci-robot

/lifecycle frozen

eddiezane avatar Feb 03 '21 18:02 eddiezane

Looks like this is a huge change, since kubernetes doesn't prepared for detach keys for now. This cannot achived by simply adding options to cli. I propose close this issue and reopen in kubernetes.

https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/cri/streaming/server.go#L65

*pls point out if i'm wrong, thanks!

book987 avatar Mar 05 '21 08:03 book987

It is a pity CRI doesn't seem to consider detach keys. Support would seem to be straightforward for dockershim: https://github.com/kubernetes/kubernetes/blob/336bbae82c4c0bd28f51cc5d7e34faeef8263bc4/pkg/kubelet/dockershim/docker_streaming.go#L129 -> https://github.com/kubernetes/kubernetes/blob/336bbae82c4c0bd28f51cc5d7e34faeef8263bc4/pkg/kubelet/dockershim/docker_streaming.go#L172 which fails to provide a hook to set DetachKeys, defined at https://github.com/kubernetes/kubernetes/blob/336bbae82c4c0bd28f51cc5d7e34faeef8263bc4/vendor/github.com/docker/docker/api/types/client.go#L37

mthome avatar Mar 05 '21 20:03 mthome

@book987 @mthome There is already an issue in Kubernetes for this feature, but it got closed as stale (as did #693 and #878 here). If you can get it re-opened and linked, that would be great.

We should not close the issue here, because even after support is added in kubelet we will still need to add the option in kubectl.

Nuru avatar Mar 05 '21 20:03 Nuru

As a workaround, create the pod using k run ... <-- bash> or k run -it <-- bash> , and debug it using exec -it.

bayeslearnerold avatar Mar 31 '22 21:03 bayeslearnerold

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

  • Confirm that this issue is still relevant with /triage accepted (org members only)
  • Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

k8s-triage-robot avatar Mar 31 '23 21:03 k8s-triage-robot

@eddiezane: Please see my updates to the Feature Request and add /triage accepted.

Nuru avatar Apr 05 '23 06:04 Nuru