From e020fb9ea5f8dec32bb32e9c89395125bdfadcb3 Mon Sep 17 00:00:00 2001 From: Brentley Jones Date: Wed, 3 Mar 2021 12:16:33 -0600 Subject: [PATCH 1/2] Allow signal bootstrap sends to commands on cancel to be customized Currently bootstrap sends a `SIGTERM` to commands when it, itself, is interrupted. This adds `cancel-signal` which accepts signals in the form of SIGTERM, SIGHUP, etc. This is the same config that `start` accepts, and is based on the PR that added that feature: https://github.com/buildkite/agent/pull/1041 Finally, `start` passes its `cancel-signal` to `bootstrap`, to prevent having to customize the `bootstrap-script`. --- bootstrap/bootstrap.go | 1 + bootstrap/config.go | 4 ++++ bootstrap/shell/shell.go | 27 ++++++++++++++++----------- clicommand/agent_start.go | 6 +++++- clicommand/bootstrap.go | 14 ++++++++++++++ 5 files changed, 40 insertions(+), 12 deletions(-) diff --git a/bootstrap/bootstrap.go b/bootstrap/bootstrap.go index f605058371..e09e5db0cb 100644 --- a/bootstrap/bootstrap.go +++ b/bootstrap/bootstrap.go @@ -85,6 +85,7 @@ func (b *Bootstrap) Run(ctx context.Context) (exitCode int) { b.shell.PTY = b.Config.RunInPty b.shell.Debug = b.Config.Debug + b.shell.InterruptSignal = b.Config.CancelSignal } // Listen for cancellation diff --git a/bootstrap/config.go b/bootstrap/config.go index f6cefef9b3..ffae206250 100644 --- a/bootstrap/config.go +++ b/bootstrap/config.go @@ -7,6 +7,7 @@ import ( "log" "github.com/buildkite/agent/v3/env" + "github.com/buildkite/agent/v3/process" ) // Config provides the configuration for the Bootstrap. Some of the keys are @@ -127,6 +128,9 @@ type Config struct { // Phases to execute, defaults to all phases Phases []string + // What signal to use for command cancellation + CancelSignal process.Signal + // List of environment variable globs to redact from job output RedactedVars []string diff --git a/bootstrap/shell/shell.go b/bootstrap/shell/shell.go index 2c28eeb1f1..cbdda3d675 100644 --- a/bootstrap/shell/shell.go +++ b/bootstrap/shell/shell.go @@ -64,6 +64,9 @@ type Shell struct { // Currently running command cmd *command cmdLock sync.Mutex + + // The signal to use to interrupt the command + InterruptSignal process.Signal } // New returns a new Shell @@ -102,12 +105,13 @@ func (s *Shell) WithStdin(r io.Reader) *Shell { defer s.cmdLock.Unlock() // Can't copy struct like `newsh := *s` because sync.Mutex can't be copied. return &Shell{ - Logger: s.Logger, - Env: s.Env, - stdin: r, // our new stdin - Writer: s.Writer, - wd: s.wd, - ctx: s.ctx, + Logger: s.Logger, + Env: s.Env, + stdin: r, // our new stdin + Writer: s.Writer, + wd: s.wd, + ctx: s.ctx, + InterruptSignal: s.InterruptSignal, } } @@ -375,11 +379,12 @@ func (s *Shell) buildCommand(ctx context.Context, name string, arg ...string) (* } cfg := process.Config{ - Path: absPath, - Args: arg, - Env: s.Env.ToSlice(), - Stdin: s.stdin, - Dir: s.wd, + Path: absPath, + Args: arg, + Env: s.Env.ToSlice(), + Stdin: s.stdin, + Dir: s.wd, + InterruptSignal: s.InterruptSignal, } // Create a sub-context so that shell.Cancel() can interrupt diff --git a/clicommand/agent_start.go b/clicommand/agent_start.go index 64b241e07a..43bfb0dbb7 100644 --- a/clicommand/agent_start.go +++ b/clicommand/agent_start.go @@ -536,7 +536,11 @@ var AgentStartCommand = cli.Command{ // Set a useful default for the bootstrap script if cfg.BootstrapScript == "" { - cfg.BootstrapScript = fmt.Sprintf("%s bootstrap", shellwords.Quote(os.Args[0])) + cfg.BootstrapScript = fmt.Sprintf( + "%s bootstrap --cancel-signal %s", + shellwords.Quote(os.Args[0]), + cfg.CancelSignal, + ) } // Show a warning if plugins are enabled by no-command-eval or no-local-hooks is set diff --git a/clicommand/bootstrap.go b/clicommand/bootstrap.go index cb7879e3b7..5bed3e80a0 100644 --- a/clicommand/bootstrap.go +++ b/clicommand/bootstrap.go @@ -12,6 +12,7 @@ import ( "github.com/buildkite/agent/v3/cliconfig" "github.com/buildkite/agent/v3/experiments" "github.com/buildkite/agent/v3/logger" + "github.com/buildkite/agent/v3/process" "github.com/urfave/cli" ) @@ -81,6 +82,7 @@ type BootstrapConfig struct { Experiments []string `cli:"experiment" normalize:"list"` Phases []string `cli:"phases" normalize:"list"` Profile string `cli:"profile"` + CancelSignal string `cli:"cancel-signal"` RedactedVars []string `cli:"redacted-vars" normalize:"list"` TracingBackend string `cli:"tracing-backend"` } @@ -297,6 +299,12 @@ var BootstrapCommand = cli.Command{ Usage: "The specific phases to execute. The order they're defined is irrelevant.", EnvVar: "BUILDKITE_BOOTSTRAP_PHASES", }, + cli.StringFlag{ + Name: "cancel-signal", + Usage: "The signal to use for cancellation", + EnvVar: "BUILDKITE_CANCEL_SIGNAL", + Value: "SIGTERM", + }, cli.StringSliceFlag{ Name: "redacted-vars", Usage: "Pattern of environment variable names containing sensitive values", @@ -353,6 +361,11 @@ var BootstrapCommand = cli.Command{ } } + cancelSig, err := process.ParseSignal(cfg.CancelSignal) + if err != nil { + l.Fatal("Failed to parse cancel-signal: %v", err) + } + // Configure the bootstraper bootstrap := bootstrap.New(bootstrap.Config{ Command: cfg.Command, @@ -392,6 +405,7 @@ var BootstrapCommand = cli.Command{ SSHKeyscan: cfg.SSHKeyscan, Shell: cfg.Shell, Phases: cfg.Phases, + CancelSignal: cancelSig, RedactedVars: cfg.RedactedVars, TracingBackend: cfg.TracingBackend, }) From 086eabec6dc82d761c616796e9c038e2ac90e4c3 Mon Sep 17 00:00:00 2001 From: Paul Annesley Date: Tue, 16 Mar 2021 21:45:42 +1100 Subject: [PATCH 2/2] JobRunner passes CancelSignal to bootstrap via env not argv --- agent/job_runner.go | 5 +++++ clicommand/agent_start.go | 6 +----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/agent/job_runner.go b/agent/job_runner.go index 81c438418c..d34eef2e37 100644 --- a/agent/job_runner.go +++ b/agent/job_runner.go @@ -509,6 +509,11 @@ func (r *JobRunner) createEnvironment() ([]string, error) { env["BUILDKITE_AGENT_EXPERIMENT"] = strings.Join(experiments.Enabled(), ",") env["BUILDKITE_REDACTED_VARS"] = strings.Join(r.conf.AgentConfiguration.RedactedVars, ",") + // propagate CancelSignal to bootstrap, unless it's the default SIGTERM + if r.conf.CancelSignal != process.SIGTERM { + env["BUILDKITE_CANCEL_SIGNAL"] = r.conf.CancelSignal.String() + } + // Whether to enable profiling in the bootstrap if r.conf.AgentConfiguration.Profile != "" { env["BUILDKITE_AGENT_PROFILE"] = r.conf.AgentConfiguration.Profile diff --git a/clicommand/agent_start.go b/clicommand/agent_start.go index 43bfb0dbb7..64b241e07a 100644 --- a/clicommand/agent_start.go +++ b/clicommand/agent_start.go @@ -536,11 +536,7 @@ var AgentStartCommand = cli.Command{ // Set a useful default for the bootstrap script if cfg.BootstrapScript == "" { - cfg.BootstrapScript = fmt.Sprintf( - "%s bootstrap --cancel-signal %s", - shellwords.Quote(os.Args[0]), - cfg.CancelSignal, - ) + cfg.BootstrapScript = fmt.Sprintf("%s bootstrap", shellwords.Quote(os.Args[0])) } // Show a warning if plugins are enabled by no-command-eval or no-local-hooks is set