Merge pull request #4792 from laurazard/alternative-fix-no-socket-notif

thaJeztah · web-flow · commit 350d6bcc2c32 · 2024-01-16T18:28:38.000+01:00
cli/plugins: use same pgid + skip signal forwarding when attached to a TTY
diff --git a/cli-plugins/manager/manager.go b/cli-plugins/manager/manager.go
@@ -233,7 +233,6 @@ func PluginRunCommand(dockerCli command.Cli, name string, rootcmd *cobra.Command
 		cmd.Stdin = os.Stdin
 		cmd.Stdout = os.Stdout
 		cmd.Stderr = os.Stderr
-		configureOSSpecificCommand(cmd)
 
 		cmd.Env = os.Environ()
 		cmd.Env = append(cmd.Env, ReexecEnvvar+"="+os.Args[0])
diff --git a/cli-plugins/manager/manager_unix.go b/cli-plugins/manager/manager_unix.go
@@ -2,21 +2,7 @@
 
 package manager
 
-import (
-	"os/exec"
-	"syscall"
-)
-
 var defaultSystemPluginDirs = []string{
 	"/usr/local/lib/docker/cli-plugins", "/usr/local/libexec/docker/cli-plugins",
 	"/usr/lib/docker/cli-plugins", "/usr/libexec/docker/cli-plugins",
 }
-
-func configureOSSpecificCommand(cmd *exec.Cmd) {
-	// Spawn the plugin process in a new process group, so that signals are not forwarded by the OS.
-	// The foreground process group is e.g. sent a SIGINT when Ctrl-C is input to the TTY, but we
-	// implement our own job control for the plugin.
-	cmd.SysProcAttr = &syscall.SysProcAttr{
-		Setpgid: true,
-	}
-}
diff --git a/cli-plugins/manager/manager_windows.go b/cli-plugins/manager/manager_windows.go
@@ -2,15 +2,10 @@ package manager
 
 import (
 	"os"
-	"os/exec"
 	"path/filepath"
 )
 
 var defaultSystemPluginDirs = []string{
 	filepath.Join(os.Getenv("ProgramData"), "Docker", "cli-plugins"),
 	filepath.Join(os.Getenv("ProgramFiles"), "Docker", "cli-plugins"),
 }
-
-func configureOSSpecificCommand(cmd *exec.Cmd) {
-	// no-op
-}
diff --git a/cli-plugins/socket/socket.go b/cli-plugins/socket/socket.go
@@ -14,21 +14,20 @@ import (
 const EnvKey = "DOCKER_CLI_PLUGIN_SOCKET"
 
 // SetupConn sets up a Unix socket listener, establishes a goroutine to handle connections
-// and update the conn pointer, and returns the environment variable to pass to the plugin.
-func SetupConn(conn **net.UnixConn) (string, error) {
+// and update the conn pointer, and returns the listener for the socket (which the caller
+// is responsible for closing when it's no longer needed).
+func SetupConn(conn **net.UnixConn) (*net.UnixListener, error) {
 	listener, err := listen("docker_cli_" + uuid.Generate().String())
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 
 	accept(listener, conn)
 
-	return EnvKey + "=" + listener.Addr().String(), nil
+	return listener, nil
 }
 
 func accept(listener *net.UnixListener, conn **net.UnixConn) {
-	defer listener.Close()
-
 	go func() {
 		for {
 			// ignore error here, if we failed to accept a connection,
diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go
@@ -223,9 +223,10 @@ func tryPluginRun(dockerCli command.Cli, cmd *cobra.Command, subcommand string,
 
 	// Establish the plugin socket, adding it to the environment under a well-known key if successful.
 	var conn *net.UnixConn
-	socketenv, err := socket.SetupConn(&conn)
+	listener, err := socket.SetupConn(&conn)
 	if err == nil {
-		envs = append(envs, socketenv)
+		envs = append(envs, socket.EnvKey+"="+listener.Addr().String())
+		defer listener.Close()
 	}
 
 	plugincmd.Env = append(envs, plugincmd.Env...)
@@ -240,16 +241,17 @@ func tryPluginRun(dockerCli command.Cli, cmd *cobra.Command, subcommand string,
 	// we send a SIGKILL to the plugin process and exit
 	go func() {
 		retries := 0
-		for s := range signals {
+		for range signals {
+			if dockerCli.Out().IsTerminal() {
+				// running attached to a terminal, so the plugin will already
+				// receive signals due to sharing a pgid with the parent CLI
+				continue
+			}
 			if conn != nil {
 				if err := conn.Close(); err != nil {
 					_, _ = fmt.Fprintf(dockerCli.Err(), "failed to signal plugin to close: %v\n", err)
 				}
 				conn = nil
-			} else {
-				// When the plugin is communicating via socket with the host CLI, we perform job control via the socket.
-				// However, if the plugin is an old version that is not socket-aware, we need to explicitly forward termination signals.
-				plugincmd.Process.Signal(s)
 			}
 			retries++
 			if retries >= exitLimit {

Original file line number	Diff line number	Diff line change
`@@ -2,15 +2,10 @@ package manager`
`2`	`2`
`3`	`3`	`import (`
`4`	`4`	`"os"`
`5`		`- "os/exec"`
`6`	`5`	`"path/filepath"`
`7`	`6`	`)`
`8`	`7`
`9`	`8`	`var defaultSystemPluginDirs = []string{`
`10`	`9`	`filepath.Join(os.Getenv("ProgramData"), "Docker", "cli-plugins"),`
`11`	`10`	`filepath.Join(os.Getenv("ProgramFiles"), "Docker", "cli-plugins"),`
`12`	`11`	`}`
`13`		`-`
`14`		`-func configureOSSpecificCommand(cmd *exec.Cmd) {`
`15`		`- // no-op`
`16`		`-}`