Do not disable sig-proxy when using a TTY

[thaJeztah]

This partially reverts moby/moby@e0b59ab (moby/moby#2426), and does not automatically disable proxying signals in TTY-mode

fixes moby/moby#28872 (docker client doesn't pass signals when a terminal is attached)
fixes moby/moby#3793 docker client not passing signals to dockerd

relates to:

• Kill docker exec command will not terminate the spawned process moby/moby#9098 Kill docker exec command will not terminate the spawned process
  • This patch does not fix the docker exec case; it looks like there's no API to kill an exec'd process, so there's no signal-proxy for this yet

Before this change:

Start a container with a TTY in one shell:

docker run -it --init --name repro-28872 busybox sleep 30

then, in another shell, kill the docker cli:

kill `pgrep -f repro-28872`

Notice that the CLI was killed, but the signal not forwarded to the container;
the container continues running

docker container inspect --format '{{ .State.Status }}' repro-28872
running

docker container rm -f repro-28872

After this change:

Start a container with a TTY in one shell:

docker run -it --init --name repro-28872 busybox sleep 30

then, in another shell, kill the docker cli:

kill `pgrep -f repro-28872`

Verify that the signal was forwarded to the container, and the container exited

docker container inspect --format '{{ .State.Status }}' repro-28872
exited

docker container rm -f repro-28872

- Description for the changelog

- Fix docker client doesn't pass signals when a terminal is attached

- A picture of a cute animal (not mandatory but encouraged)

[thaJeztah]

@vdemeester I tried coming up with an e2e test for this, but I couldn't get it to work (creating a container with a TTY attached, then signal that docker cli); perhaps you have ideas?)

[codecov-io]

Codecov Report

Merging #1841 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1841      +/-   ##
==========================================
+ Coverage   56.73%   56.74%   +<.01%     
==========================================
  Files         310      310              
  Lines       21803    21801       -2     
==========================================
  Hits        12370    12370              
+ Misses       8518     8517       -1     
+ Partials      915      914       -1

[thaJeztah]

oooh, actually, I think I might've found it 🤗 - cleaning up a bit, and will add a test

[thaJeztah]

Dang; why doesn't it work here 😞

11:10:01 === FAIL: e2e/container TestSigProxyWithTTY (6.16s)
11:10:01     proxy_signal_test.go:38: terminating PID 3793
11:10:01     proxy_signal_test.go:42: timeout hit after 5s: expected status exited != running

[thaJeztah]

Hm... it works when running dind (using /var/run/docker.sock), but fails when using tcp:// 🤔

[thaJeztah]

Ok, so my test passes with /var/run/docker.sock, but many other tests fail; need to look into why that's happening 🤔

    revoke_test.go:59: assertion failed: 
        Command:  docker pull registry:5000/alpine:3.6
        ExitCode: 1
        Error:    exit status 1
        Stdout:   
        Stderr:   Error response from daemon: Get https://registry:5000/v2/: Service Unavailable

[thaJeztah]

If someone has any ideas on the test, help is appreciated 🤗

[thaJeztah]

I'll be damned.. LOL, tests are green now 🤷‍♂

let me remove the testing commits from here

[thaJeztah]

Well.. there you go; all green and shiny 🎉

ping @vdemeester @tianon @silvin-lubecki PTAL

[silvin-lubecki]

LGTM 👍

[vdemeester]

LGTM 🐯

[thaJeztah]

Speedy gonzales !