plugins: run plugin with new process group ID

laurazard · laurazard · commit c719dfb65472 · 2024-01-10T14:07:59.000Z
Changes were made in #4599 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: docker/compose#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. Signed-off-by: Laura Brehm <laurabrehm@hey.com>
diff --git 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
@@ -2,7 +2,18 @@
 
 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) {
+	cmd.SysProcAttr = &syscall.SysProcAttr{
+		Setpgid: true,
+	}
+}
diff --git a/cli-plugins/manager/manager_windows.go b/cli-plugins/manager/manager_windows.go
@@ -2,10 +2,15 @@ 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/cmd/docker/docker.go b/cmd/docker/docker.go
@@ -230,12 +230,14 @@ 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 {
+				plugincmd.Process.Signal(s)
 			}
 			retries++
 			if retries >= exitLimit {

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