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

fixesmoby/moby#28872 (docker client doesn't pass signals when a terminal is attached)
fixesmoby/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 !