From ef5e5fa03f0603f48678bfd7039c5b8121d98df1 Mon Sep 17 00:00:00 2001 From: Laura Brehm Date: Mon, 8 Jan 2024 11:59:30 +0000 Subject: [PATCH] plugins: run plugin with new process group ID Changes were made in 1554ac3b5f38147301c518c60da16f3f80c1717b to provide a mechanism for the CLI to notify running plugin processes that they should exit, in order to improve the general CLI/plugin UX. The current implementation boils down to: 1. The CLI creates a socket 2. The CLI executes the plugin 3. The plugin connects to the socket 4. (When) the CLI receives a termination signal, it uses the socket to notify the plugin that it should exit 5. The plugin's gets notified via the socket, and cancels it's `cmd.Context`, which then gets handled appropriately This change works in most cases and fixes the issue it sets out to solve (see: https://github.com/docker/compose/pull/11292) however, in the case where the user has a TTY attached and the plugin is not already handling received signals, steps 4+ changes: 4. (When) the CLI receives a termination signal, before it can use the socket to notify the plugin that it should exit, the plugin process also receives a signal due to sharing the pgid with the CLI Since we now have a proper "job control" mechanism, we can simplify the scenarios by executing the plugins with their own process group id, thereby removing the "double notification" issue and making it so that plugins can handle the same whether attached to a TTY or not. In order to make this change "plugin-binary" backwards-compatible, in the case that a plugin does not connect to the socket, the CLI passes the signal to the plugin process. Co-authored-by: Bjorn Neergaard Signed-off-by: Laura Brehm Signed-off-by: Bjorn Neergaard --- 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, 25 insertions(+), 1 deletion(-) diff --git a/cli-plugins/manager/manager.go b/cli-plugins/manager/manager.go index bcf150542f..e46e817c50 100644 --- a/cli-plugins/manager/manager.go +++ b/cli-plugins/manager/manager.go @@ -232,6 +232,7 @@ 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 000c058df0..67107ef97c 100644 --- a/cli-plugins/manager/manager_unix.go +++ b/cli-plugins/manager/manager_unix.go @@ -2,7 +2,21 @@ 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 2ce5a75973..e04ebd4c5e 100644 --- a/cli-plugins/manager/manager_windows.go +++ b/cli-plugins/manager/manager_windows.go @@ -2,6 +2,7 @@ package manager import ( "os" + "os/exec" "path/filepath" ) @@ -9,3 +10,7 @@ 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 e2d8d07f08..50cb0e473a 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -230,12 +230,16 @@ 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 range signals { + for s := 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 {