Implement configurable escape key for attach/exec

[vdemeester]

Implement configurable escape keys (for attach, exec, run and start) using the client-side configuration and/or flags --detach-keys. This takes some pieces from #6460 from @vieux — except that this time it uses the client configuration (~/.docker/config.json for example). 🐧

This PR allows you to change the set of keys you have to type to detach in TTY mode, it can be less or more that two.
The basic ASCII table is supported, for example you can do: docker -d --detach-keys="ctrl-@" and detach in one keystroke or docker -d --detach-keys="DEL,+" if you don't like to have a ctrl key

The configuration looks like this :

{
    "detachKeys": "ctrl-a,a"
}

The default keybinding stays the same : ctrl-p ctrl-q but it makes possible to configure something else.

There is room for improvements on the code that should be address during the design review process I think :

• For attach command I use queryString (detachKeys=ctrl-a%2Ca) but for exec command I used runConfig.ExecConfig. That felt weird and it's not coherent, it should be the same thing for both. It could maybe be passed as HTTP Header, I don't know 😅 — that's why I'm putting this in review.
• Right now, I don't validate the configuration client side, so if there is something wrong with the detachKeys passed, It's just logged and the defaults one are used.
• Some refactoring is there is a little too much duplicated code for my taste (the keys, err = term.ToBytes(detachKeys) part is repeated few times..).
• Probably add a json:omitempty for DetachKeys on runconfig.ExecConfig.

What is still to do :

• Rebase it 😅
• Implement flags by commands (run, start, attach, exec)
• Write some integration tests.
• Update some documentation.
• Update the commit message to make it more clear 😅.

🐸

Fixes #3519.

Signed-off-by: Vincent Demeester vincent@sbr.pm

[crquan]

👍

[tiborvass]

A question to answer is if there are already configs in the client config.json that have no flag equivalents, and if not, then is that something we want ? IOW, if we add a config in config.json, should we also add a flag?

[vdemeester]

@tiborvass good question 🐱 I'm wondering what should be the flag then 😅.

/cc @duglin @thaJeztah @runcom 😝

[runcom]

if we add a config in config.json, should we also add a flag

if I'm understanding correctly your question, another question is, what's the use case for having this defined as a flag? do really ppl run their containers assigning different escape keys? why would they?
docker run -ti --escape-keys "ctrl-a,a" my_cont0 bash
docker exec -ti --escape-keys "ctrl-b,b" my_cont1 bash
I don't like it, but maybe I don't see the value in this. So, I think even if config.json doesn't have configurations w/o an equivalent flag I still see this option better suited in the json instead of a command flag

(notificed the original issue is for run command as well)

[vdemeester]

@runcom talking with @tiborvass a bit on IRC, there might be usecase for a one-off different escape keys : when doing dind and need to detach from the inner container without detaching from the outer one — editing the config file inside the outer container is not that convenient. A flag would be handy there 😅

[runcom]

Ok that's the use case I haven't thought about :)

[tiborvass]

As an addendum, I don't think this should be on exec, only run and possibly attach. I wonder about start too (when doing start -ai)

[vdemeester]

@tiborvass you're talking only about the flag right ? (for not having it on exec)

[tiborvass]

@vdemeester i just realized we still allow detaching from exec, that's a mistake. I don't know what to do then.

[vdemeester]

@tiborvass hum you think we should disable detaching from exec no matter this PR ? I kinda agree on this but pretty sure this should be another PR (probably to be merged before that one). I can take a look 😉

[tiborvass]

@vdemeester I agree that issue is a separate one :)

[rhatdan]

I would want to change this to be system default.
Is there an easy way to cause the default config.json to have different defaults?

[vdemeester]

@rhatdan what do you mean by that ?

[rhatdan]

I would want to change all containers to run with a different detach command. Something like what tmux does. I want to set it in a config or on the daemon and be done with it. I don't want to think about if for every docker run command.

http://superuser.com/questions/74492/whats-the-best-prefix-escape-sequence-for-screen-or-tmux

[vdemeester]

@rhatdan well this is the goal of this PR : to add a config options (on client side, on ~/.docker/config.json) so that by default it uses something else, and an option (yet to be written/updated) to be able to do it per containers. So this would answer your use case then (the config part of it)

[thaJeztah]

damn you github, why are you removing/adding the wrong labels 😢

[thaJeztah]

It's green 🎉 \o/

[vdemeester]

🎉 🎅

[pulserdd]

Hi, I have a question on using this detachKeys config. We are using ecs instance on which these docker tasks are run. Following the documentation, I added the "detachKeys": "ctrl-e" to /root/.docker/config.json but it does not seem to take effect. I still have to use ctrl-c to exit on attach and that kills the container. Could someone share some pointers on resolving this? I have tried restarting the docker daemon but to no avail. Would highly appreciate some help. Thanks.

[dryfish]

@pulserdd I've just tested with 1.11.1 and "DetachKeys": "ctrl-x" is working for me. I think docker JSON files always use title casing.

[thaJeztah]

hm, in that case either the documentation is incorrect, or something changed in the parsing of that file https://docs.docker.com/engine/reference/commandline/cli/#configuration-files

@vdemeester ?

[dryfish]

Ah. I've been looking at docker-machine configs recently and seeing title case.

I've just tried docker run with detachKeys and that also works for me.

(ctrl-e,e and ctrl-x,x working for me when put into ~/.docker/config.json for the user running docker run. Testing with an image running /bin/bash; type a command; ctrl-p (once) to get it back from history; ctrl-x x to detach.)

[vdemeester]

@pulserdd tried it out and work as expected. In the config file it takes both detachKeys or DetachKeys. As @dryfish it has to be set in $HOME/.docker/config.json of the user that do the docker run, or docker attach, or … It's client-side so no need to restart the daemon or anything.

[pulserdd]

Thanks for your replies. I tried to follow your comments but still the same issue. Here's what I have in config
[root@ip-1-2-3-4 ec2-user]# cat ~/.docker/config.json { "auths": { "quay.io": { "auth": "xxx", "email": "" } }, "DetachKeys": "ctrl-e" }

Result:
[root@ip-1-2-3-4 ec2-user]# docker attach 7fe7a4ed6213 E E Ee C2016-05-09 18:06:05,463 INFO [cs.iosmdm.iosmdm] wsgi exiting

my docker version
Docker version 1.9.1, build a34a1d5/1.9.1

[vdemeester]

@pulserdd oh it's normal then, this feature is only available starting docker 1.10…

[pulserdd]

oh ok sorry I missed that while following the docs. Thanks once again for a great community support.

[MhdSyrwan]

@vdemeester Can you please correct the example in the first post since people sometimes refer to this pull request as a documentation page (like me 😄 ).

The wrong example:

{
    "detachKey": "ctrl-a,a"
}

The correct one:

{
    "detachKeys": "ctrl-a,a"
}

[thaJeztah]

@MhdSyrwan thanks for letting us know! I updated the description