From 9fc036500ab443dbc3510ed21fb1ae2d761f06b9 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Mon, 29 Aug 2022 15:39:34 +0200 Subject: [PATCH 1/2] bugfix: fixed template validation panic in case of incorrect ChangeScript configuration --- nomad/structs/structs.go | 68 +++++++++++++++++++++++++---------- nomad/structs/structs_test.go | 58 +++++++++++++++++++++++------- 2 files changed, 95 insertions(+), 31 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index aa089d1ab9eb..4f14eea842de 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -7644,20 +7644,6 @@ var ( TemplateChangeModeInvalidError = errors.New("Invalid change mode. Must be one of the following: noop, signal, script, restart") ) -// ChangeScript holds the configuration for the script that is executed if -// change mode is set to script -type ChangeScript struct { - // Command is the full path to the script - Command string - // Args is a slice of arguments passed to the script - Args []string - // Timeout is the amount of seconds we wait for the script to finish - Timeout time.Duration - // FailOnError indicates whether a task should fail in case script execution - // fails or log script failure and don't interrupt the task - FailOnError bool -} - // Template represents a template configuration to be rendered for a given task type Template struct { // SourcePath is the path to the template to be rendered @@ -7739,6 +7725,10 @@ func (t *Template) Copy() *Template { nt.Wait = t.Wait.Copy() } + if t.ChangeScript != nil { + nt.ChangeScript = t.ChangeScript.Copy() + } + return nt } @@ -7780,8 +7770,12 @@ func (t *Template) Validate() error { _ = multierror.Append(&mErr, fmt.Errorf("cannot use signals with env var templates")) } case TemplateChangeModeScript: - if t.ChangeScript.Command == "" { - _ = multierror.Append(&mErr, fmt.Errorf("must specify script path value when change mode is script")) + if t.ChangeScript == nil { + _ = multierror.Append(&mErr, fmt.Errorf("must specify change script configuration value when change mode is script")) + } + + if err = t.ChangeScript.Validate(); err != nil { + _ = multierror.Append(&mErr, err) } default: _ = multierror.Append(&mErr, TemplateChangeModeInvalidError) @@ -7822,6 +7816,44 @@ func (t *Template) DiffID() string { return t.DestPath } +// ChangeScript holds the configuration for the script that is executed if +// change mode is set to script +type ChangeScript struct { + // Command is the full path to the script + Command string + // Args is a slice of arguments passed to the script + Args []string + // Timeout is the amount of seconds we wait for the script to finish + Timeout time.Duration + // FailOnError indicates whether a task should fail in case script execution + // fails or log script failure and don't interrupt the task + FailOnError bool +} + +func (cs *ChangeScript) Copy() *ChangeScript { + if cs == nil { + return nil + } + + ncs := new(ChangeScript) + *ncs = *cs + + return ncs +} + +// Validate makes sure all the required fields of ChangeScript are present +func (cs *ChangeScript) Validate() error { + if cs == nil { + return nil + } + + if cs.Command == "" { + return fmt.Errorf("must specify script path value when change mode is script") + } + + return nil +} + // WaitConfig is the Min/Max duration used by the Consul Template Watcher. Consul // Template relies on pointer based business logic. This struct uses pointers so // that we tell the different between zero values and unset values. @@ -7839,11 +7871,11 @@ func (wc *WaitConfig) Copy() *WaitConfig { nwc := new(WaitConfig) if wc.Min != nil { - nwc.Min = &*wc.Min + nwc.Min = wc.Min } if wc.Max != nil { - nwc.Max = &*wc.Max + nwc.Max = wc.Max } return nwc diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 5684994f783a..f69690c0fa53 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -2586,16 +2586,19 @@ func TestTemplate_Copy(t *testing.T) { SourcePath: "/local/file.txt", DestPath: "/local/dest.txt", EmbeddedTmpl: "tpl", - ChangeMode: TemplateChangeModeSignal, - ChangeSignal: "SIGHUP", - Splay: 10 * time.Second, - Perms: "777", - Uid: pointer.Of(1000), - Gid: pointer.Of(2000), - LeftDelim: "[[", - RightDelim: "]]", - Envvars: true, - VaultGrace: time.Minute, + ChangeMode: TemplateChangeModeScript, + ChangeScript: &ChangeScript{ + Command: "/bin/foo", + Args: []string{"--force", "--debug"}, + }, + Splay: 10 * time.Second, + Perms: "777", + Uid: pointer.Of(1000), + Gid: pointer.Of(2000), + LeftDelim: "[[", + RightDelim: "]]", + Envvars: true, + VaultGrace: time.Minute, Wait: &WaitConfig{ Min: pointer.Of(time.Second), Max: pointer.Of(time.Minute), @@ -2606,8 +2609,9 @@ func TestTemplate_Copy(t *testing.T) { t1.SourcePath = "/local/file2.txt" t1.DestPath = "/local/dest2.txt" t1.EmbeddedTmpl = "tpl2" - t1.ChangeMode = TemplateChangeModeRestart - t1.ChangeSignal = "" + t1.ChangeMode = TemplateChangeModeSignal + t1.ChangeScript.Command = "/bin/foobar" + t1.ChangeScript.Args = []string{"--forces", "--debugs"} t1.Splay = 5 * time.Second t1.Perms = "700" t1.Uid = pointer.Of(5000) @@ -2623,7 +2627,8 @@ func TestTemplate_Copy(t *testing.T) { require.NotEqual(t, t1.DestPath, t2.DestPath) require.NotEqual(t, t1.EmbeddedTmpl, t2.EmbeddedTmpl) require.NotEqual(t, t1.ChangeMode, t2.ChangeMode) - require.NotEqual(t, t1.ChangeSignal, t2.ChangeSignal) + require.NotEqual(t, t1.ChangeScript.Command, t2.ChangeScript.Command) + require.NotEqual(t, t1.ChangeScript.Args, t2.ChangeScript.Args) require.NotEqual(t, t1.Splay, t2.Splay) require.NotEqual(t, t1.Perms, t2.Perms) require.NotEqual(t, t1.Uid, t2.Uid) @@ -2760,6 +2765,33 @@ func TestTemplate_Validate(t *testing.T) { }, Fail: false, }, + { + Tmpl: &Template{ + SourcePath: "foo", + DestPath: "local/foo", + ChangeMode: "script", + ChangeScript: nil, + }, + Fail: true, + }, + { + Tmpl: &Template{ + SourcePath: "foo", + DestPath: "local/foo", + ChangeMode: "script", + ChangeScript: &ChangeScript{Command: ""}, + }, + Fail: true, + }, + { + Tmpl: &Template{ + SourcePath: "foo", + DestPath: "local/foo", + ChangeMode: "script", + ChangeScript: &ChangeScript{Command: "/bin/foo"}, + }, + Fail: false, + }, } for i, c := range cases { From c7ad53c3c1d6973f655ce14a6b52b5a8a249be7b Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Mon, 29 Aug 2022 16:16:22 +0200 Subject: [PATCH 2/2] Tim's comments --- nomad/structs/structs.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 4f14eea842de..6ac3e5c652e4 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -45,6 +45,7 @@ import ( "github.com/miekg/dns" "github.com/mitchellh/copystructure" "golang.org/x/crypto/blake2b" + "golang.org/x/exp/slices" ) var ( @@ -7721,13 +7722,8 @@ func (t *Template) Copy() *Template { nt := new(Template) *nt = *t - if t.Wait != nil { - nt.Wait = t.Wait.Copy() - } - - if t.ChangeScript != nil { - nt.ChangeScript = t.ChangeScript.Copy() - } + nt.ChangeScript = t.ChangeScript.Copy() + nt.Wait = t.Wait.Copy() return nt } @@ -7838,6 +7834,9 @@ func (cs *ChangeScript) Copy() *ChangeScript { ncs := new(ChangeScript) *ncs = *cs + // args is a slice! + ncs.Args = slices.Clone(cs.Args) + return ncs }