From ed4758e02b9483abc100912e268ba324eab2c1cc Mon Sep 17 00:00:00 2001 From: John Eikenberry Date: Wed, 9 Feb 2022 14:41:22 -0800 Subject: [PATCH] support command lists This updates the exec{} block's `command` setting to support command lists. That is a command formatted like `["echo", "hello", "world"]`. It is backwards compatible with the old format, turning single commands w/o spaces into lists. Single commands with spaces still use the `sh -c` setup. This will allow for multi-word commands on Windows and in setups (like bare docker containers) without `sh` installed. --- README.md | 47 ++++++++++++--------- cli.go | 2 +- cli_test.go | 2 +- config/config_test.go | 12 +++--- config/exec.go | 15 +++++-- config/exec_test.go | 74 ++++++++++++++++++++++++++------- config/template.go | 19 +++++---- config/template_test.go | 54 ++++++++++++------------ config/testdata/foo | 1 + docs/configuration.md | 35 +++++++++------- docs/modes.md | 2 +- manager/command-prep.go | 44 +++++++++++--------- manager/command-prep_test.go | 36 ++++++++++++++++ manager/command-prep_windows.go | 21 ++++++---- manager/runner.go | 23 +++++----- manager/runner_test.go | 66 ++++++++++++++++------------- 16 files changed, 290 insertions(+), 163 deletions(-) create mode 100644 config/testdata/foo create mode 100644 manager/command-prep_test.go diff --git a/README.md b/README.md index 49dab2b16..0198b2060 100644 --- a/README.md +++ b/README.md @@ -239,25 +239,34 @@ users the ability to further customize their command script. The command configured for running on template rendering must take one of two forms. -The first is as a single command without spaces in its name and no arguments. -This form of command will be called directly by consul-template and is good for -any situation. The command can be a shell script or an executable, anything -called via a single word, and must be either on the runtime search PATH or the -absolute path to the executable. The single word limination is necessary to -eliminate any need for parsing the command line. For example.. - -`command = "/opt/foo"` or, if on PATH, `command = "foo"` - -The second form is as a multi-word command, a command with arguments or a more -complex shell command. This form **requires** a shell named `sh` be on the -executable search path (eg. PATH on *nix). This is the standard on all *nix -systems and should work out of the box on those systems. This won't work on, -for example, Docker images with only the executable and not a minimal system -like Alpine. Using this form you can join multiple commands with logical -operators, `&&` and `||`, use pipelines with `|`, conditionals, etc. Note that -the shell `sh` is normally `/bin/sh` on *nix systems and is either a POSIX -shell or a shell run in POSIX compatible mode, so it is best to stick to POSIX -shell syntax in this command. For example.. +The first is as a list of the command and arguments split at spaces. The +command can use an absolute path or be found on the execution environment's +PATH and must be the first item in the list. This form allows for single or +multi-word commands that can be executed directly with a system call. For +example... + +`command = ["echo", "hello"]` +`command = ["/opt/foo-package/bin/run-foo"]` +`command = ["foo"]` + +Note that if you give a single command without the list denoting square +brackets (`[]`) it is converted into a list with a single argument. + +This: +`command = "foo"` +is equivalent to: +`command = ["foo"]` + +The second form is as a single quoted command using system shell features. This +form **requires** a shell named `sh` be on the executable search path (eg. PATH +on *nix). This is the standard on all *nix systems and should work out of the +box on those systems. This won't work on, for example, Docker images with only +the executable and without a minimal system like Alpine. Using this form you +can join multiple commands with logical operators, `&&` and `||`, use pipelines +with `|`, conditionals, etc. Note that the shell `sh` is normally `/bin/sh` on +\*nix systems and is either a POSIX shell or a shell run in POSIX compatible +mode, so it is best to stick to POSIX shell syntax in this command. For +example.. `command = "/opt/foo && /opt/bar"` diff --git a/cli.go b/cli.go index e24f3e9c6..cc803c8a7 100644 --- a/cli.go +++ b/cli.go @@ -346,7 +346,7 @@ func (cli *CLI) ParseFlags(args []string) ( flags.Var((funcVar)(func(s string) error { c.Exec.Enabled = config.Bool(true) - c.Exec.Command = config.String(s) + c.Exec.Command = []string{s} return nil }), "exec", "") diff --git a/cli_test.go b/cli_test.go index cc3503b90..6ecd0f86a 100644 --- a/cli_test.go +++ b/cli_test.go @@ -304,7 +304,7 @@ func TestCLI_ParseFlags(t *testing.T) { &config.Config{ Exec: &config.ExecConfig{ Enabled: config.Bool(true), - Command: config.String("command"), + Command: []string{"command"}, }, }, false, diff --git a/config/config_test.go b/config/config_test.go index 26524ac8a..70780db52 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -369,7 +369,7 @@ func TestParse(t *testing.T) { }`, &Config{ Exec: &ExecConfig{ - Command: String("command"), + Command: []string{"command"}, }, }, false, @@ -726,7 +726,7 @@ func TestParse(t *testing.T) { &Config{ Templates: &TemplateConfigs{ &TemplateConfig{ - Command: String("command"), + Command: []string{"command"}, }, }, }, @@ -813,7 +813,7 @@ func TestParse(t *testing.T) { Templates: &TemplateConfigs{ &TemplateConfig{ Exec: &ExecConfig{ - Command: String("command"), + Command: []string{"command"}, }, }, }, @@ -1855,17 +1855,17 @@ func TestConfig_Merge(t *testing.T) { "exec", &Config{ Exec: &ExecConfig{ - Command: String("command"), + Command: []string{"command"}, }, }, &Config{ Exec: &ExecConfig{ - Command: String("command-diff"), + Command: []string{"command-diff"}, }, }, &Config{ Exec: &ExecConfig{ - Command: String("command-diff"), + Command: []string{"command-diff"}, }, }, }, diff --git a/config/exec.go b/config/exec.go index c3977d48b..ecc87ce25 100644 --- a/config/exec.go +++ b/config/exec.go @@ -32,7 +32,7 @@ var ( // exec/supervise mode. type ExecConfig struct { // Command is the command to execute and watch as a child process. - Command *string `mapstructure:"command"` + Command commandList `mapstructure:"command"` // Enabled controls if this exec is enabled. Enabled *bool `mapstructure:"enabled"` @@ -61,6 +61,13 @@ type ExecConfig struct { Timeout *time.Duration `mapstructure:"timeout"` } +// commandList is a []string with a common method for testing for content +type commandList []string + +func (c commandList) Empty() bool { + return len(c) == 0 || c[0] == "" +} + // DefaultExecConfig returns a configuration that is populated with the // default values. func DefaultExecConfig() *ExecConfig { @@ -154,11 +161,11 @@ func (c *ExecConfig) Merge(o *ExecConfig) *ExecConfig { // Finalize ensures there no nil pointers. func (c *ExecConfig) Finalize() { if c.Enabled == nil { - c.Enabled = Bool(StringPresent(c.Command)) + c.Enabled = Bool(!c.Command.Empty()) } if c.Command == nil { - c.Command = String("") + c.Command = []string{} } if c.Env == nil { @@ -203,7 +210,7 @@ func (c *ExecConfig) GoString() string { "Splay:%s, "+ "Timeout:%s"+ "}", - StringGoString(c.Command), + c.Command, BoolGoString(c.Enabled), c.Env, SignalGoString(c.KillSignal), diff --git a/config/exec_test.go b/config/exec_test.go index 2249af461..1761a8be9 100644 --- a/config/exec_test.go +++ b/config/exec_test.go @@ -25,7 +25,7 @@ func TestExecConfig_Copy(t *testing.T) { { "copy", &ExecConfig{ - Command: String("command"), + Command: []string{"command"}, Enabled: Bool(true), Env: &EnvConfig{Pristine: Bool(true)}, KillSignal: Signal(syscall.SIGINT), @@ -81,27 +81,27 @@ func TestExecConfig_Merge(t *testing.T) { }, { "command_overrides", - &ExecConfig{Command: String("command")}, - &ExecConfig{Command: String("")}, - &ExecConfig{Command: String("")}, + &ExecConfig{Command: []string{"command"}}, + &ExecConfig{Command: []string{}}, + &ExecConfig{Command: []string{}}, }, { "command_empty_one", - &ExecConfig{Command: String("command")}, + &ExecConfig{Command: []string{"command"}}, &ExecConfig{}, - &ExecConfig{Command: String("command")}, + &ExecConfig{Command: []string{"command"}}, }, { "command_empty_two", &ExecConfig{}, - &ExecConfig{Command: String("command")}, - &ExecConfig{Command: String("command")}, + &ExecConfig{Command: []string{"command"}}, + &ExecConfig{Command: []string{"command"}}, }, { "command_same", - &ExecConfig{Command: String("command")}, - &ExecConfig{Command: String("command")}, - &ExecConfig{Command: String("command")}, + &ExecConfig{Command: []string{"command"}}, + &ExecConfig{Command: []string{"command"}}, + &ExecConfig{Command: []string{"command"}}, }, { "enabled_overrides", @@ -294,7 +294,7 @@ func TestExecConfig_Finalize(t *testing.T) { "empty", &ExecConfig{}, &ExecConfig{ - Command: String(""), + Command: []string{}, Enabled: Bool(false), Env: &EnvConfig{ Allowlist: []string{}, @@ -314,10 +314,56 @@ func TestExecConfig_Finalize(t *testing.T) { { "with_command", &ExecConfig{ - Command: String("command"), + Command: []string{"command"}, }, &ExecConfig{ - Command: String("command"), + Command: []string{"command"}, + Enabled: Bool(true), + Env: &EnvConfig{ + Denylist: []string{}, + DenylistDeprecated: []string{}, + Custom: []string{}, + Pristine: Bool(false), + Allowlist: []string{}, + AllowlistDeprecated: []string{}, + }, + KillSignal: Signal(DefaultExecKillSignal), + KillTimeout: TimeDuration(DefaultExecKillTimeout), + ReloadSignal: Signal(DefaultExecReloadSignal), + Splay: TimeDuration(0 * time.Second), + Timeout: TimeDuration(DefaultExecTimeout), + }, + }, + { + "with_command_list", + &ExecConfig{ + Command: []string{"command", "argument1", "argument2"}, + }, + &ExecConfig{ + Command: []string{"command", "argument1", "argument2"}, + Enabled: Bool(true), + Env: &EnvConfig{ + Denylist: []string{}, + DenylistDeprecated: []string{}, + Custom: []string{}, + Pristine: Bool(false), + Allowlist: []string{}, + AllowlistDeprecated: []string{}, + }, + KillSignal: Signal(DefaultExecKillSignal), + KillTimeout: TimeDuration(DefaultExecKillTimeout), + ReloadSignal: Signal(DefaultExecReloadSignal), + Splay: TimeDuration(0 * time.Second), + Timeout: TimeDuration(DefaultExecTimeout), + }, + }, + { + "with_shell_command", + &ExecConfig{ + Command: []string{"command | pipe && command"}, + }, + &ExecConfig{ + Command: []string{"command | pipe && command"}, Enabled: Bool(true), Env: &EnvConfig{ Denylist: []string{}, diff --git a/config/template.go b/config/template.go index 1633bf75d..727f6d823 100644 --- a/config/template.go +++ b/config/template.go @@ -33,7 +33,7 @@ type TemplateConfig struct { // Command is the arbitrary command to execute after a template has // successfully rendered. This is DEPRECATED. Use Exec instead. - Command *string `mapstructure:"command"` + Command commandList `mapstructure:"command"` // CommandTimeout is the amount of time to wait for the command to finish // before force-killing it. This is DEPRECATED. Use Exec instead. @@ -268,7 +268,7 @@ func (c *TemplateConfig) Finalize() { } if c.Command == nil { - c.Command = String("") + c.Command = []string{} } if c.CommandTimeout == nil { @@ -303,8 +303,12 @@ func (c *TemplateConfig) Finalize() { if c.Exec.Command == nil && c.Command != nil { c.Exec.Command = c.Command } - if c.Exec.Timeout == nil && c.CommandTimeout != nil { + // backwards compat with command_timeout and default support for exec.timeout + switch { + case c.Exec.Timeout == nil && c.CommandTimeout != nil: c.Exec.Timeout = c.CommandTimeout + case c.Exec.Timeout == nil: + c.Exec.Timeout = TimeDuration(DefaultTemplateCommandTimeout) } c.Exec.Finalize() @@ -366,7 +370,7 @@ func (c *TemplateConfig) GoString() string { "SandboxPath:%s"+ "}", BoolGoString(c.Backup), - StringGoString(c.Command), + c.Command, TimeDurationGoString(c.CommandTimeout), StringGoString(c.Contents), BoolGoString(c.CreateDestDirs), @@ -492,7 +496,8 @@ func ParseTemplateConfig(s string) (*TemplateConfig, error) { command = strings.Join(parts[2:], ":") } - var sourcePtr, destinationPtr, commandPtr *string + var sourcePtr, destinationPtr *string + var commandL commandList if source != "" { sourcePtr = String(source) } @@ -500,12 +505,12 @@ func ParseTemplateConfig(s string) (*TemplateConfig, error) { destinationPtr = String(destination) } if command != "" { - commandPtr = String(command) + commandL = []string{command} } return &TemplateConfig{ Source: sourcePtr, Destination: destinationPtr, - Command: commandPtr, + Command: commandL, }, nil } diff --git a/config/template_test.go b/config/template_test.go index 89880abde..0de6a393c 100644 --- a/config/template_test.go +++ b/config/template_test.go @@ -25,12 +25,12 @@ func TestTemplateConfig_Copy(t *testing.T) { "same_enabled", &TemplateConfig{ Backup: Bool(true), - Command: String("command"), + Command: []string{"command"}, CommandTimeout: TimeDuration(10 * time.Second), Contents: String("contents"), CreateDestDirs: Bool(true), Destination: String("destination"), - Exec: &ExecConfig{Command: String("command")}, + Exec: &ExecConfig{Command: []string{"command"}}, Perms: FileMode(0600), Source: String("source"), Wait: &WaitConfig{Min: TimeDuration(10)}, @@ -108,27 +108,27 @@ func TestTemplateConfig_Merge(t *testing.T) { }, { "command_overrides", - &TemplateConfig{Command: String("command")}, - &TemplateConfig{Command: String("")}, - &TemplateConfig{Command: String("")}, + &TemplateConfig{Command: []string{"command"}}, + &TemplateConfig{Command: []string{}}, + &TemplateConfig{Command: []string{}}, }, { "command_empty_one", - &TemplateConfig{Command: String("command")}, + &TemplateConfig{Command: []string{"command"}}, &TemplateConfig{}, - &TemplateConfig{Command: String("command")}, + &TemplateConfig{Command: []string{"command"}}, }, { "command_empty_two", &TemplateConfig{}, - &TemplateConfig{Command: String("command")}, - &TemplateConfig{Command: String("command")}, + &TemplateConfig{Command: []string{"command"}}, + &TemplateConfig{Command: []string{"command"}}, }, { "command_same", - &TemplateConfig{Command: String("command")}, - &TemplateConfig{Command: String("command")}, - &TemplateConfig{Command: String("command")}, + &TemplateConfig{Command: []string{"command"}}, + &TemplateConfig{Command: []string{"command"}}, + &TemplateConfig{Command: []string{"command"}}, }, { "command_timeout_overrides", @@ -252,27 +252,27 @@ func TestTemplateConfig_Merge(t *testing.T) { }, { "exec_overrides", - &TemplateConfig{Exec: &ExecConfig{Command: String("command")}}, - &TemplateConfig{Exec: &ExecConfig{Command: String("")}}, - &TemplateConfig{Exec: &ExecConfig{Command: String("")}}, + &TemplateConfig{Exec: &ExecConfig{Command: []string{"command"}}}, + &TemplateConfig{Exec: &ExecConfig{Command: []string{}}}, + &TemplateConfig{Exec: &ExecConfig{Command: []string{}}}, }, { "exec_empty_one", - &TemplateConfig{Exec: &ExecConfig{Command: String("command")}}, + &TemplateConfig{Exec: &ExecConfig{Command: []string{"command"}}}, &TemplateConfig{Exec: &ExecConfig{}}, - &TemplateConfig{Exec: &ExecConfig{Command: String("command")}}, + &TemplateConfig{Exec: &ExecConfig{Command: []string{"command"}}}, }, { "exec_empty_two", &TemplateConfig{Exec: &ExecConfig{}}, - &TemplateConfig{Exec: &ExecConfig{Command: String("command")}}, - &TemplateConfig{Exec: &ExecConfig{Command: String("command")}}, + &TemplateConfig{Exec: &ExecConfig{Command: []string{"command"}}}, + &TemplateConfig{Exec: &ExecConfig{Command: []string{"command"}}}, }, { "exec_same", - &TemplateConfig{Exec: &ExecConfig{Command: String("command")}}, - &TemplateConfig{Exec: &ExecConfig{Command: String("command")}}, - &TemplateConfig{Exec: &ExecConfig{Command: String("command")}}, + &TemplateConfig{Exec: &ExecConfig{Command: []string{"command"}}}, + &TemplateConfig{Exec: &ExecConfig{Command: []string{"command"}}}, + &TemplateConfig{Exec: &ExecConfig{Command: []string{"command"}}}, }, { "perms_overrides", @@ -418,7 +418,7 @@ func TestTemplateConfig_Finalize(t *testing.T) { &TemplateConfig{}, &TemplateConfig{ Backup: Bool(false), - Command: String(""), + Command: []string{}, CommandTimeout: TimeDuration(DefaultTemplateCommandTimeout), Contents: String(""), CreateDestDirs: Bool(true), @@ -426,7 +426,7 @@ func TestTemplateConfig_Finalize(t *testing.T) { ErrMissingKey: Bool(false), ErrFatal: Bool(true), Exec: &ExecConfig{ - Command: String(""), + Command: []string{}, Enabled: Bool(false), Env: &EnvConfig{ Denylist: []string{}, @@ -540,7 +540,7 @@ func TestParseTemplateConfig(t *testing.T) { &TemplateConfig{ Source: String("/tmp/a.txt"), Destination: String("/tmp/b.txt"), - Command: String("command"), + Command: []string{"command"}, }, false, }, @@ -566,7 +566,7 @@ func TestParseTemplateConfig(t *testing.T) { &TemplateConfig{ Source: String(`C:\abc\123`), Destination: String(`D:\xyz\789`), - Command: String(`command`), + Command: []string{`command`}, }, false, }, @@ -576,7 +576,7 @@ func TestParseTemplateConfig(t *testing.T) { &TemplateConfig{ Source: String(`C:\abc\123`), Destination: String(`D:\xyz\789`), - Command: String(`sub:command`), + Command: []string{`sub:command`}, }, false, }, diff --git a/config/testdata/foo b/config/testdata/foo new file mode 100644 index 000000000..5716ca598 --- /dev/null +++ b/config/testdata/foo @@ -0,0 +1 @@ +bar diff --git a/docs/configuration.md b/docs/configuration.md index 899f54d64..406a6785b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -446,19 +446,6 @@ template { # `source` option. contents = "{{ keyOrDefault \"service/redis/maxconns@east-aws\" \"5\" }}" - # This is the optional command to run when the template is rendered. The - # command will only run if the resulting template changes. The command must - # return within 30s (configurable), and it must have a successful exit code. - # Consul Template is not a replacement for a process monitor or init system. - # Please see the Commands section in the README for more. - command = "restart service foo" - - # This is the maximum amount of time to wait for the optional command to - # return. If you set the timeout to 0s the command is run in the background - # without monitoring it for errors. If also using Once, consul-template can - # exit before the command is finished. Default is 30s. - command_timeout = "60s" - # Exit with an error when accessing a struct or map field/key that does not # exist. The default behavior will print "" when accessing a field # that does not exist. It is highly recommended you set this to "true" when @@ -506,6 +493,21 @@ template { min = "2s" max = "10s" } + + # This is the optional exec block to give a command to be run when the template + # is rendered. The command will only run if the resulting template changes. + # The command must return within 30s (configurable), and it must have a + # successful exit code. + # See the Exec section below and the Commands section in the README for more. + exec { + command = ["restart", "service", "foo"] + timout = "30s" + } + + # For backwards compatibility the template block also supports a bare + # `command` and `command_timeout` setting. + command = ["restart", "service", "foo"] + command_timeout = "60s" } ``` @@ -552,7 +554,12 @@ how exec mode operates and the caveats of this mode. exec { # This is the command to exec as a child process. There can be only one # command per Consul Template process. - command = "/usr/bin/app" + # Please see the Commands section in the README for more. + command = ["/usr/bin/app"] + + # Timeout is the maximum amount of time to wait for a command to complete. + # By default, this is 0, which means "wait forever". + timeout = "0" # This is a random splay to wait before killing the command. The default # value is 0 (no wait), but large clusters should consider setting a splay diff --git a/docs/modes.md b/docs/modes.md index afe59d2c5..acae1334f 100644 --- a/docs/modes.md +++ b/docs/modes.md @@ -109,7 +109,7 @@ The same rules that apply to the [commands](../README.md#commands) apply here, that is if you want to use a complex, shell-like command you need to be running on a system with `sh` on your PATH. These commands are run using `sh -c` with the shell handling all shell parsing. Otherwise you want the command to be a -single word (no spaces) found on the search PATH or using an absolute path. +a single command or a, list formatted, command with arguments. Note that on supporing systems (*nix, with `sh`) the [`setpgid`](https://man7.org/linux/man-pages/man2/setpgid.2.html) flag is set diff --git a/manager/command-prep.go b/manager/command-prep.go index 1403ef2b6..6895ef731 100644 --- a/manager/command-prep.go +++ b/manager/command-prep.go @@ -1,3 +1,4 @@ +//go:build !windows // +build !windows package manager @@ -7,28 +8,31 @@ import ( "strings" ) -func prepCommand(command string) ([]string, error) { - switch len(strings.Fields(command)) { - case 0: - return []string{}, nil - case 1: - return []string{command}, nil - } - - // default to 'sh' on path, else try a couple common absolute paths - shell := "sh" - if _, err := exec.LookPath(shell); err != nil { - for _, sh := range []string{"/bin/sh", "/usr/bin/sh"} { - if sh, err := exec.LookPath(sh); err == nil { - shell = sh - break +func prepCommand(command []string) ([]string, error) { + switch { + case len(command) == 1 && len(strings.Fields(command[0])) > 1: + // command is []string{"command using arguments or shell features"} + shell := "sh" + // default to 'sh' on path, else try a couple common absolute paths + if _, err := exec.LookPath(shell); err != nil { + shell = "" + for _, sh := range []string{"/bin/sh", "/usr/bin/sh"} { + if sh, err := exec.LookPath(sh); err == nil { + shell = sh + break + } } } - } - if shell == "" { + if shell == "" { + return []string{}, exec.ErrNotFound + } + cmd := []string{shell, "-c", command[0]} + return cmd, nil + case len(command) >= 1 && len(strings.TrimSpace(command[0])) > 0: + // command is already good ([]string{"foo"}, []string{"foo", "bar"}, ..) + return command, nil + default: + // command is []string{} or []string{""} return []string{}, exec.ErrNotFound } - - cmd := []string{shell, "-c", command} - return cmd, nil } diff --git a/manager/command-prep_test.go b/manager/command-prep_test.go new file mode 100644 index 000000000..125a1c24d --- /dev/null +++ b/manager/command-prep_test.go @@ -0,0 +1,36 @@ +package manager + +import ( + "os/exec" + "reflect" + "testing" +) + +func Test_prepCommand(t *testing.T) { + type cmd []string + cases := []struct { + n string + in cmd + out cmd + err error + }{ + {n: "0", in: cmd{}, out: cmd{}, err: exec.ErrNotFound}, + {n: "''", in: cmd{""}, out: cmd{}, err: exec.ErrNotFound}, + {n: "' '", in: cmd{" "}, out: cmd{}, err: exec.ErrNotFound}, + {n: "'f'", in: cmd{"foo"}, out: cmd{"foo"}, err: nil}, + {n: "'f b'", in: cmd{"foo bar"}, out: cmd{"sh", "-c", "foo bar"}, err: nil}, + {n: "'f','b'", in: cmd{"foo", "bar"}, out: cmd{"foo", "bar"}, err: nil}, + {n: "'f','b','z'", in: cmd{"foo", "bar", "zed"}, out: cmd{"foo", "bar", "zed"}, err: nil}, + } + for _, tc := range cases { + t.Run(tc.n, func(t *testing.T) { + out, err := prepCommand(tc.in) + if !reflect.DeepEqual(cmd(out), tc.out) { + t.Errorf("bad prepCommand output. wanted: %#v, got %#v", tc.out, out) + } + if err != tc.err { + t.Errorf("bad prepCommand error. wanted: %v, got %v", tc.err, err) + } + }) + } +} diff --git a/manager/command-prep_windows.go b/manager/command-prep_windows.go index 31097bd9c..4ebeb4e05 100644 --- a/manager/command-prep_windows.go +++ b/manager/command-prep_windows.go @@ -1,18 +1,23 @@ +//go:build windows // +build windows package manager import ( - "fmt" + "os/exec" "strings" ) -func prepCommand(command string) ([]string, error) { - switch len(strings.Fields(command)) { - case 0: - return []string{}, nil - case 1: - return []string{command}, nil +func prepCommand(command []string) ([]string, error) { + switch { + case len(command) == 1 && len(strings.Fields(command[0])) == 1: + // command is []string{"foo"} + return []string{command[0]}, nil + case len(command) > 1: + // command is []string{"foo", "bar"} + return command, nil + default: + // command is []string{}, []string{""}, []string{"foo bar"} + return []string{}, exec.ErrNotFound } - return []string{}, fmt.Errorf("only single commands supported on windows") } diff --git a/manager/runner.go b/manager/runner.go index d9f27fb45..c3f40ce4f 100644 --- a/manager/runner.go +++ b/manager/runner.go @@ -6,6 +6,7 @@ import ( "io" "log" "os" + "reflect" "strconv" "sync" "time" @@ -265,7 +266,7 @@ func (r *Runner) Start() { // If an exec command was given and a command is not currently running, // spawn the child process for supervision. - if config.StringPresent(r.config.Exec.Command) { + if !r.config.Exec.Command.Empty() { // Lock the child because we are about to check if it exists. r.childLock.Lock() @@ -278,7 +279,7 @@ func (r *Runner) Start() { Stdin: r.inStream, Stdout: r.outStream, Stderr: r.errStream, - Command: config.StringVal(r.config.Exec.Command), + Command: r.config.Exec.Command, Env: env.Env(), ReloadSignal: config.SignalVal(r.config.Exec.ReloadSignal), KillSignal: config.SignalVal(r.config.Exec.KillSignal), @@ -568,15 +569,15 @@ func (r *Runner) Run() error { // ensures all commands execute at least once. var errs []error for _, t := range runCtx.commands { - command := config.StringVal(t.Exec.Command) - log.Printf("[INFO] (runner) executing command %q from %s", command, t.Display()) + log.Printf("[INFO] (runner) executing command %q from %s", + fmt.Sprintf("%q", t.Exec.Command), t.Display()) env := t.Exec.Env.Copy() env.Custom = append(r.childEnv(), env.Custom...) if _, err := spawnChild(&spawnChildInput{ Stdin: r.inStream, Stdout: r.outStream, Stderr: r.errStream, - Command: command, + Command: t.Exec.Command, Env: env.Env(), Timeout: config.TimeDurationVal(t.Exec.Timeout), ReloadSignal: config.SignalVal(t.Exec.ReloadSignal), @@ -584,7 +585,8 @@ func (r *Runner) Run() error { KillTimeout: config.TimeDurationVal(t.Exec.KillTimeout), Splay: config.TimeDurationVal(t.Exec.Splay), }); err != nil { - s := fmt.Sprintf("failed to execute command %q from %s", command, t.Display()) + s := fmt.Sprintf("failed to execute command %q from %s", + fmt.Sprintf("%q", t.Exec.Command), t.Display()) errs = append(errs, errors.Wrap(err, s)) } } @@ -847,8 +849,7 @@ func (r *Runner) runTemplate(tmpl *template.Template, runCtx *templateRunCtx) (* // in the order in which they are provided in the TemplateConfig // definitions. If we inserted commands into a map, we would lose that // relative ordering and people would be unhappy. - // if config.StringPresent(ctemplate.Command) - if c := config.StringVal(templateConfig.Exec.Command); c != "" { + if c := templateConfig.Exec.Command; !c.Empty() { existing := findCommand(templateConfig, runCtx.commands) if existing != nil { log.Printf("[DEBUG] (runner) skipping command %q from %s (already appended from %s)", @@ -1176,7 +1177,7 @@ type spawnChildInput struct { Stdin io.Reader Stdout io.Writer Stderr io.Writer - Command string + Command []string Timeout time.Duration Env []string ReloadSignal os.Signal @@ -1270,9 +1271,9 @@ func (q *quiescence) tick() { // findCommand searches the list of template configs for the given command and // returns it if it exists. func findCommand(c *config.TemplateConfig, templates []*config.TemplateConfig) *config.TemplateConfig { - needle := config.StringVal(c.Exec.Command) + needle := c.Exec.Command for _, t := range templates { - if needle == config.StringVal(t.Exec.Command) { + if reflect.DeepEqual(needle, t.Exec.Command) { return t } } diff --git a/manager/runner_test.go b/manager/runner_test.go index 65f8402ea..f5b61e280 100644 --- a/manager/runner_test.go +++ b/manager/runner_test.go @@ -324,12 +324,12 @@ func TestRunner_Run(t *testing.T) { Templates: &config.TemplateConfigs{ &config.TemplateConfig{ Contents: config.String("hello"), - Command: config.String("echo 123"), + Command: []string{"echo 123"}, Destination: config.String("/tmp/ct-runs_commands_a"), }, &config.TemplateConfig{ Contents: config.String("world"), - Command: config.String("echo 456"), + Command: []string{"echo 456"}, Destination: config.String("/tmp/ct-runs_commands_b"), }, }, @@ -368,7 +368,7 @@ func TestRunner_Run(t *testing.T) { Templates: &config.TemplateConfigs{ &config.TemplateConfig{ Contents: config.String("hello"), - Command: config.String("echo 123"), + Command: []string{"echo 123"}, Destination: config.String("/tmp/ct-no_command_if_same_template"), }, }, @@ -391,12 +391,12 @@ func TestRunner_Run(t *testing.T) { Templates: &config.TemplateConfigs{ &config.TemplateConfig{ Contents: config.String("hello"), - Command: config.String("echo 123"), + Command: []string{"echo 123"}, Destination: config.String("/tmp/ct-no_duplicate_commands_a"), }, &config.TemplateConfig{ Contents: config.String("world"), - Command: config.String("echo 123"), + Command: []string{"echo 123"}, Destination: config.String("/tmp/ct-no_duplicate_commands_b"), }, }, @@ -423,7 +423,7 @@ func TestRunner_Run(t *testing.T) { Templates: &config.TemplateConfigs{ &config.TemplateConfig{ Contents: config.String("hello"), - Command: config.String("env"), + Command: []string{"env"}, Destination: config.String("/tmp/ct-env_a"), }, }, @@ -672,7 +672,7 @@ func TestRunner_Start(t *testing.T) { c := config.DefaultConfig().Merge(&config.Config{ Exec: &config.ExecConfig{ - Command: config.String(`sleep 30`), + Command: []string{`sleep 30`}, KillTimeout: config.TimeDuration(time.Duration(10 * time.Second)), }, Templates: &config.TemplateConfigs{ @@ -728,7 +728,7 @@ func TestRunner_Start(t *testing.T) { c := config.DefaultConfig().Merge(&config.Config{ Exec: &config.ExecConfig{ - Command: config.String(`sleep 30`), + Command: []string{`sleep 30`}, }, Templates: &config.TemplateConfigs{ &config.TemplateConfig{ @@ -796,7 +796,7 @@ func TestRunner_Start(t *testing.T) { }, Exec: &config.ExecConfig{ // `cat filename` would fail if template hadn't rendered - Command: config.String(`cat ` + firstOut.Name()), + Command: []string{`cat ` + firstOut.Name()}, }, Templates: &config.TemplateConfigs{ &config.TemplateConfig{ @@ -869,7 +869,7 @@ func TestRunner_Start(t *testing.T) { Max: config.TimeDuration(10 * time.Millisecond), }, Exec: &config.ExecConfig{ - Command: config.String(`sleep 30`), + Command: []string{`sleep 30`}, }, Templates: &config.TemplateConfigs{ &config.TemplateConfig{ @@ -1065,8 +1065,8 @@ func TestRunner_quiescence(t *testing.T) { func TestRunner_command(t *testing.T) { type testCase struct { - name, input, out string - parsed []string + name, out string + input, parsed []string } os.Setenv("FOO", "bar") @@ -1074,7 +1074,7 @@ func TestRunner_command(t *testing.T) { out, err := prepCommand(tc.input) mismatchErr := "bad command parse\n got: '%#v'\nwanted: '%#v'" switch { - case err != nil: + case err != nil && len(tc.input) > 0 && tc.input[0] != "": t.Error("unexpected error:", err) case len(out) != len(tc.parsed): t.Errorf(mismatchErr, out, tc.parsed) @@ -1085,7 +1085,13 @@ func TestRunner_command(t *testing.T) { runTest := func(tc testCase) { stdout := new(bytes.Buffer) stderr := new(bytes.Buffer) - args, _ := prepCommand(tc.input) + args, err := prepCommand(tc.input) + switch { + case err == exec.ErrNotFound && len(args) == 0: + return // expected + case err != nil: + t.Fatal("unexpected error", err) + } child, err := child.New(&child.NewInput{ Stdin: os.Stdin, Stdout: stdout, @@ -1113,91 +1119,91 @@ func TestRunner_command(t *testing.T) { for i, tc := range []testCase{ { name: "null", - input: "", + input: []string{""}, parsed: []string{}, out: "", }, { name: "single", - input: "echo", + input: []string{"echo"}, parsed: []string{"echo"}, out: "\n", }, { name: "simple", - input: "printf hi", + input: []string{"printf hi"}, parsed: []string{"sh", "-c", "printf hi"}, out: "hi", }, { name: "subshell-bash", // GH-1456 & GH-1463 - input: "bash -c 'printf hi'", + input: []string{"bash -c 'printf hi'"}, parsed: []string{"sh", "-c", "bash -c 'printf hi'"}, out: "hi", }, { name: "subshell-single-quoting", // GH-1456 & GH-1463 - input: "sh -c 'printf hi'", + input: []string{"sh -c 'printf hi'"}, parsed: []string{"sh", "-c", "sh -c 'printf hi'"}, out: "hi", }, { name: "subshell-double-quoting", - input: `sh -c "echo -n hi"`, + input: []string{`sh -c "echo -n hi"`}, parsed: []string{"sh", "-c", `sh -c "echo -n hi"`}, out: "hi", }, { name: "subshell-call", - input: `echo -n $(echo -n foo)`, + input: []string{`echo -n $(echo -n foo)`}, parsed: []string{"sh", "-c", "echo -n $(echo -n foo)"}, out: "foo", }, { name: "pipe", - input: `seq 1 5 | grep 3`, + input: []string{`seq 1 5 | grep 3`}, parsed: []string{"sh", "-c", "seq 1 5 | grep 3"}, out: "3\n", }, { name: "conditional", - input: `sh -c 'if true; then printf foo; fi'`, + input: []string{`sh -c 'if true; then printf foo; fi'`}, parsed: []string{"sh", "-c", "sh -c 'if true; then printf foo; fi'"}, out: "foo", }, { name: "command-substition", - input: `sh -c 'echo -n $(which /bin/sh)'`, + input: []string{`sh -c 'echo -n $(which /bin/sh)'`}, parsed: []string{"sh", "-c", "sh -c 'echo -n $(which /bin/sh)'"}, out: "/bin/sh", }, { name: "curly-brackets", - input: "sh -c '{ if [ -f /bin/sh ]; then echo -n foo; else echo -n bar; fi }'", + input: []string{"sh -c '{ if [ -f /bin/sh ]; then echo -n foo; else echo -n bar; fi }'"}, parsed: []string{"sh", "-c", "sh -c '{ if [ -f /bin/sh ]; then echo -n foo; else echo -n bar; fi }'"}, out: "foo", }, { name: "and", - input: "true && true && echo -n and", + input: []string{"true && true && echo -n and"}, parsed: []string{"sh", "-c", "true && true && echo -n and"}, out: "and", }, { name: "or", - input: "false || false || echo -n or", + input: []string{"false || false || echo -n or"}, parsed: []string{"sh", "-c", "false || false || echo -n or"}, out: "or", }, { name: "backgrounded", - input: "(sleep .1; echo -n hi) &", + input: []string{"(sleep .1; echo -n hi) &"}, parsed: []string{"sh", "-c", "(sleep .1; echo -n hi) &"}, out: "hi", }, { name: "env", - input: "echo -n ${FOO}", + input: []string{"echo -n ${FOO}"}, parsed: []string{"sh", "-c", "echo -n ${FOO}"}, out: "bar", }, @@ -1216,7 +1222,7 @@ func TestRunner_commandPath(t *testing.T) { PATH := os.Getenv("PATH") defer os.Setenv("PATH", PATH) os.Setenv("PATH", "") - cmd, err := prepCommand("echo hi") + cmd, err := prepCommand([]string{"echo hi"}) if err != nil && err != exec.ErrNotFound { t.Fatal(err) }