From 26560ff93c42cfe8237bb0ba40cdf349fcf7d812 Mon Sep 17 00:00:00 2001 From: Laura Brehm Date: Mon, 15 Jan 2024 13:14:06 +0000 Subject: [PATCH 1/3] Revert "plugins: run plugin with new process group ID" This reverts commit ef5e5fa03f0603f48678bfd7039c5b8121d98df1. Running new plugins under a new pgid isn't a viable solution due to it causing issues with plugin processes attempting to read from the TTY (see: https://github.com/moby/moby/issues/47073). Signed-off-by: Laura Brehm --- cli-plugins/manager/manager.go | 1 - cli-plugins/manager/manager_unix.go | 14 -------------- cli-plugins/manager/manager_windows.go | 5 ----- cmd/docker/docker.go | 6 +----- 4 files changed, 1 insertion(+), 25 deletions(-) diff --git a/cli-plugins/manager/manager.go b/cli-plugins/manager/manager.go index 02ad0293f0..d23934940d 100644 --- 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 index 67107ef97c..000c058df0 100644 --- 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 index e04ebd4c5e..2ce5a75973 100644 --- a/cli-plugins/manager/manager_windows.go +++ b/cli-plugins/manager/manager_windows.go @@ -2,7 +2,6 @@ package manager import ( "os" - "os/exec" "path/filepath" ) @@ -10,7 +9,3 @@ 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/cmd/docker/docker.go b/cmd/docker/docker.go index b320a6eefb..2b1b951a50 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -240,16 +240,12 @@ 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 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 { From 5f6c55a72449177e8f370f70c7009ed53ebd0ed7 Mon Sep 17 00:00:00 2001 From: Laura Brehm Date: Mon, 15 Jan 2024 13:16:53 +0000 Subject: [PATCH 2/3] plugins: don't handle signal/notify if TTY In order to solve the "double notification" issue (see: https://github.com/docker/cli/commit/ef5e5fa03f0603f48678bfd7039c5b8121d98df1) without running the plugin process under a new pgid (see: https://github.com/moby/moby/issues/47073) we instead check if we're attached to a TTY, and if so skip signalling the plugin process since it will already be signalled. Signed-off-by: Laura Brehm --- cmd/docker/docker.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 2b1b951a50..f3bc230080 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -241,6 +241,11 @@ func tryPluginRun(dockerCli command.Cli, cmd *cobra.Command, subcommand string, go func() { retries := 0 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) From 508346ef61ed7495a6eadaa1ca68b310085e126e Mon Sep 17 00:00:00 2001 From: Laura Brehm Date: Mon, 15 Jan 2024 14:29:48 +0000 Subject: [PATCH 3/3] plugins: fix plugin socket being closed before use Signed-off-by: Laura Brehm --- cli-plugins/socket/socket.go | 11 +++++------ cmd/docker/docker.go | 5 +++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/cli-plugins/socket/socket.go b/cli-plugins/socket/socket.go index a7ad00746b..beb46ae3a3 100644 --- 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 index f3bc230080..cfc53a6fa1 100644 --- 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...)