Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli/plugins: use same pgid + skip signal forwarding when attached to a TTY #4792

Merged
merged 3 commits into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion cli-plugins/manager/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Expand Down
14 changes: 0 additions & 14 deletions cli-plugins/manager/manager_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}
5 changes: 0 additions & 5 deletions cli-plugins/manager/manager_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
11 changes: 5 additions & 6 deletions cli-plugins/socket/socket.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
16 changes: 9 additions & 7 deletions cmd/docker/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -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...)
Expand All @@ -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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if dockerCli.Out() is ideal here, eg. for a command like build -o type=tar . > t.tar . Although afaics, the actual behavior change from this line is minimal.

// 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 {
Expand Down
Loading