Skip to content

Commit

Permalink
cli-plugins: terminate plugin when CLI exits
Browse files Browse the repository at this point in the history
Previously, long lived CLI plugin processes weren't
properly handled
(see: #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 <[email protected]>
  • Loading branch information
laurazard committed Oct 12, 2023
1 parent a46f850 commit 3da28ad
Show file tree
Hide file tree
Showing 7 changed files with 112 additions and 68 deletions.
41 changes: 41 additions & 0 deletions cli-plugins/plugin/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ package plugin

import (
"encoding/json"
"errors"
"fmt"
"io"
"net"
"os"
"sync"

Expand All @@ -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
Expand All @@ -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
Expand Down
55 changes: 50 additions & 5 deletions cmd/docker/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -187,17 +191,58 @@ 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 {
return err
}
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
Expand Down
44 changes: 0 additions & 44 deletions cmd/docker/internal/appcontext/appcontext.go

This file was deleted.

12 changes: 0 additions & 12 deletions cmd/docker/internal/appcontext/appcontext_unix.go

This file was deleted.

7 changes: 0 additions & 7 deletions cmd/docker/internal/appcontext/appcontext_windows.go

This file was deleted.

14 changes: 14 additions & 0 deletions cmd/docker/internal/signals/signals_unix.go
Original file line number Diff line number Diff line change
@@ -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}
7 changes: 7 additions & 0 deletions cmd/docker/internal/signals/signals_windows.go
Original file line number Diff line number Diff line change
@@ -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}

0 comments on commit 3da28ad

Please sign in to comment.