From 3da28ad928efcc8a243aa097575e5f457229dedb Mon Sep 17 00:00:00 2001 From: Laura Brehm Date: Thu, 12 Oct 2023 20:20:16 +0100 Subject: [PATCH] cli-plugins: terminate plugin when CLI exits Previously, long lived CLI plugin processes weren't properly handled (see: https://github.com/docker/cli/issues/4402) resulting in plugin processes being left behind running, after the CLI process exits. This commit changes the plugin handling code to open an abstract unix socket before running the plugin and passing it to the plugin process, and changes the signal handling on the CLI side to close this socket which tells the plugin that it should exit. This implementation makes use of sockets instead of simply setting PDEATHSIG on the plugin process so that it will work on both BSDs, assorted UNIXes and Windows. Signed-off-by: Laura Brehm --- cli-plugins/plugin/plugin.go | 41 ++++++++++++++ cmd/docker/docker.go | 55 +++++++++++++++++-- cmd/docker/internal/appcontext/appcontext.go | 44 --------------- .../internal/appcontext/appcontext_unix.go | 12 ---- .../internal/appcontext/appcontext_windows.go | 7 --- cmd/docker/internal/signals/signals_unix.go | 14 +++++ .../internal/signals/signals_windows.go | 7 +++ 7 files changed, 112 insertions(+), 68 deletions(-) delete mode 100644 cmd/docker/internal/appcontext/appcontext.go delete mode 100644 cmd/docker/internal/appcontext/appcontext_unix.go delete mode 100644 cmd/docker/internal/appcontext/appcontext_windows.go create mode 100644 cmd/docker/internal/signals/signals_unix.go create mode 100644 cmd/docker/internal/signals/signals_windows.go diff --git a/cli-plugins/plugin/plugin.go b/cli-plugins/plugin/plugin.go index 73059578f983..542c4ca066ef 100644 --- a/cli-plugins/plugin/plugin.go +++ b/cli-plugins/plugin/plugin.go @@ -2,7 +2,10 @@ package plugin import ( "encoding/json" + "errors" "fmt" + "io" + "net" "os" "sync" @@ -14,6 +17,11 @@ import ( "github.com/spf13/cobra" ) +// CliPluginCloseSocketEnvKey is used to pass the plugin being +// executed the abstract socket name it should listen on to know +// when the CLI has exited. +const CliPluginCloseSocketEnvKey = "CLI_PLUGIN_EXIT_SOCKET" + // PersistentPreRunE must be called by any plugin command (or // subcommand) which uses the cobra `PersistentPreRun*` hook. Plugins // which do not make use of `PersistentPreRun*` do not need to call @@ -24,12 +32,45 @@ import ( // called. var PersistentPreRunE func(*cobra.Command, []string) error +// closeOnParentSocketClose connects to the socket specified +// by the CLI_PLUGIN_EXIT_SOCKET env var, if present, and attempts +// to read from it until it receives an EOF, which signals that +// the CLI is going to exit and the plugin should also exit. +func closeOnParentSocketClose() { + socketAddr, ok := os.LookupEnv(CliPluginCloseSocketEnvKey) + if !ok { + // if a plugin compiled against a more recent version of docker/cli + // is executed by an older CLI binary, ignore missing environment + // variable and behave as usual + return + } + addr, err := net.ResolveUnixAddr("unix", socketAddr) + if err != nil { + return + } + cliCloseConn, err := net.DialUnix("unix", nil, addr) + if err != nil { + return + } + + go func() { + for { + b := make([]byte, 1) + _, err := cliCloseConn.Read(b) + if errors.Is(err, io.EOF) { + os.Exit(0) + } + } + }() +} + // RunPlugin executes the specified plugin command func RunPlugin(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager.Metadata) error { tcmd := newPluginCommand(dockerCli, plugin, meta) var persistentPreRunOnce sync.Once PersistentPreRunE = func(_ *cobra.Command, _ []string) error { + closeOnParentSocketClose() var err error persistentPreRunOnce.Do(func() { var opts []command.InitializeOpt diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 6a99b58b1555..89e774ee5bd5 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -2,18 +2,22 @@ package main import ( "fmt" + "net" "os" "os/exec" + "os/signal" "strings" "syscall" "github.com/docker/cli/cli" pluginmanager "github.com/docker/cli/cli-plugins/manager" + "github.com/docker/cli/cli-plugins/plugin" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/commands" cliflags "github.com/docker/cli/cli/flags" "github.com/docker/cli/cli/version" - "github.com/docker/cli/cmd/docker/internal/appcontext" + platformsignals "github.com/docker/cli/cmd/docker/internal/signals" + "github.com/docker/distribution/uuid" "github.com/docker/docker/api/types/versions" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -187,6 +191,12 @@ func setValidateArgs(dockerCli command.Cli, cmd *cobra.Command) { }) } +func setupPluginSocket() (*net.UnixListener, error) { + uuid := uuid.Generate().String() + pluginCloseSocketAddr := net.UnixAddr{Name: "@docker_cli_" + uuid, Net: "unix"} + return net.ListenUnix("unix", &pluginCloseSocketAddr) +} + func tryPluginRun(dockerCli command.Cli, cmd *cobra.Command, subcommand string, envs []string) error { plugincmd, err := pluginmanager.PluginRunCommand(dockerCli, subcommand, cmd) if err != nil { @@ -194,10 +204,45 @@ func tryPluginRun(dockerCli command.Cli, cmd *cobra.Command, subcommand string, } plugincmd.Env = append(envs, plugincmd.Env...) - go func() { - // override SIGTERM handler so we let the plugin shut down first - <-appcontext.Context().Done() - }() + listener, err := setupPluginSocket() + // if we somehow failed to set up the socket to alert the plugin + // when the CLI process wants to exit, fallback to old behaviour + if err == nil { + defer listener.Close() + plugincmd.Env = append(plugincmd.Env, plugin.CliPluginCloseSocketEnvKey+"="+listener.Addr().String()) + + go func() { + signals := make(chan os.Signal, 2048) + signal.Notify(signals, platformsignals.TerminationSignals...) + + const exitLimit = 3 + retries := 0 + + var conn *net.UnixConn + go func() { + conn, err = listener.AcceptUnix() + if err != nil { + // TODO(laurazard): change me, halt execution or fallback to + // previous behaviour if we couldn't set up socket connection + println("got a weird error!", err.Error()) + } + }() + + go func() { + for { + <-signals + if err := conn.Close(); err != nil { + logrus.Errorf("failed to signal plugin to close: %v", err) + } + retries++ + if retries >= exitLimit { + logrus.Errorf("got %d SIGTERM/SIGINTs, exiting without cleanup", retries) + os.Exit(1) + } + } + }() + }() + } if err := plugincmd.Run(); err != nil { statusCode := 1 diff --git a/cmd/docker/internal/appcontext/appcontext.go b/cmd/docker/internal/appcontext/appcontext.go deleted file mode 100644 index f41f4b6d7508..000000000000 --- a/cmd/docker/internal/appcontext/appcontext.go +++ /dev/null @@ -1,44 +0,0 @@ -package appcontext - -import ( - "context" - "os" - "os/signal" - "sync" - - "github.com/sirupsen/logrus" -) - -var ( - appContextCache context.Context - appContextOnce sync.Once -) - -// Context returns a static context that reacts to termination signals of the -// running process. Useful in CLI tools. -func Context() context.Context { - appContextOnce.Do(func() { - signals := make(chan os.Signal, 2048) - signal.Notify(signals, terminationSignals...) - - const exitLimit = 3 - retries := 0 - - ctx := context.Background() - ctx, cancel := context.WithCancel(ctx) - appContextCache = ctx - - go func() { - for { - <-signals - cancel() - retries++ - if retries >= exitLimit { - logrus.Errorf("got %d SIGTERM/SIGINTs, forcing shutdown", retries) - os.Exit(1) - } - } - }() - }) - return appContextCache -} diff --git a/cmd/docker/internal/appcontext/appcontext_unix.go b/cmd/docker/internal/appcontext/appcontext_unix.go deleted file mode 100644 index 366edc68b399..000000000000 --- a/cmd/docker/internal/appcontext/appcontext_unix.go +++ /dev/null @@ -1,12 +0,0 @@ -//go:build !windows -// +build !windows - -package appcontext - -import ( - "os" - - "golang.org/x/sys/unix" -) - -var terminationSignals = []os.Signal{unix.SIGTERM, unix.SIGINT} diff --git a/cmd/docker/internal/appcontext/appcontext_windows.go b/cmd/docker/internal/appcontext/appcontext_windows.go deleted file mode 100644 index 0a8bcbe7df2a..000000000000 --- a/cmd/docker/internal/appcontext/appcontext_windows.go +++ /dev/null @@ -1,7 +0,0 @@ -package appcontext - -import ( - "os" -) - -var terminationSignals = []os.Signal{os.Interrupt} diff --git a/cmd/docker/internal/signals/signals_unix.go b/cmd/docker/internal/signals/signals_unix.go new file mode 100644 index 000000000000..41ffb46952db --- /dev/null +++ b/cmd/docker/internal/signals/signals_unix.go @@ -0,0 +1,14 @@ +//go:build !windows +// +build !windows + +package signals + +import ( + "os" + + "golang.org/x/sys/unix" +) + +// TerminationSignals represents the list of signals we +// want to special-case handle, on this platform. +var TerminationSignals = []os.Signal{unix.SIGTERM, unix.SIGINT} diff --git a/cmd/docker/internal/signals/signals_windows.go b/cmd/docker/internal/signals/signals_windows.go new file mode 100644 index 000000000000..d6c5773f3668 --- /dev/null +++ b/cmd/docker/internal/signals/signals_windows.go @@ -0,0 +1,7 @@ +package signals + +import "os" + +// TerminationSignals represents the list of signals we +// want to special-case handle, on this platform. +var TerminationSignals = []os.Signal{os.Interrupt}