From dbffdca92e00b91d8495f0a250da55a81898c350 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Mon, 22 Aug 2022 16:25:49 -0400 Subject: [PATCH] template: use pointer values for gid and uid (#14203) When a Nomad agent starts and loads jobs that already existed in the cluster, the default template uid and gid was being set to 0, since this is the zero value for int. This caused these jobs to fail in environments where it was not possible to use 0, such as in Windows clients. In order to differentiate between an explicit 0 and a template where these properties were not set we need to use a pointer. --- .changelog/14203.txt | 3 + api/jobs_test.go | 4 -- api/tasks.go | 6 -- .../taskrunner/template/template.go | 8 ++- .../taskrunner/template/template_test.go | 8 +-- command/agent/job_endpoint.go | 12 +--- command/agent/job_endpoint_test.go | 4 +- jobspec/helper.go | 5 -- jobspec/helper_test.go | 9 +++ jobspec/parse_task.go | 2 - jobspec/parse_test.go | 2 - jobspec2/helper_test.go | 5 ++ jobspec2/parse_job.go | 10 ---- nomad/structs/diff.go | 18 ++++++ nomad/structs/diff_test.go | 16 ++--- nomad/structs/structs.go | 4 +- nomad/structs/structs_test.go | 58 +++++++++++++++++++ .../docs/job-specification/template.mdx | 9 +-- .../content/docs/upgrade/upgrade-specific.mdx | 17 +++++- 19 files changed, 137 insertions(+), 63 deletions(-) create mode 100644 .changelog/14203.txt create mode 100644 jobspec/helper_test.go create mode 100644 jobspec2/helper_test.go diff --git a/.changelog/14203.txt b/.changelog/14203.txt new file mode 100644 index 000000000..f331d84c8 --- /dev/null +++ b/.changelog/14203.txt @@ -0,0 +1,3 @@ +```release-note:bug +template: Fixed a bug where job templates would use `uid` and `gid` 0 after upgrading to Nomad 1.3.3, causing tasks to fail with the error `failed looking up user: managing file ownership is not supported on Windows`. +``` diff --git a/api/jobs_test.go b/api/jobs_test.go index 78b042bc6..f6e3b3094 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -765,8 +765,6 @@ func TestJobs_Canonicalize(t *testing.T) { ChangeSignal: pointerOf(""), Splay: pointerOf(5 * time.Second), Perms: pointerOf("0644"), - Uid: pointerOf(-1), - Gid: pointerOf(-1), LeftDelim: pointerOf("{{"), RightDelim: pointerOf("}}"), Envvars: pointerOf(false), @@ -780,8 +778,6 @@ func TestJobs_Canonicalize(t *testing.T) { ChangeSignal: pointerOf(""), Splay: pointerOf(5 * time.Second), Perms: pointerOf("0644"), - Uid: pointerOf(-1), - Gid: pointerOf(-1), LeftDelim: pointerOf("{{"), RightDelim: pointerOf("}}"), Envvars: pointerOf(true), diff --git a/api/tasks.go b/api/tasks.go index 5c8db853e..b24c6b058 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -855,12 +855,6 @@ func (tmpl *Template) Canonicalize() { if tmpl.Perms == nil { tmpl.Perms = pointerOf("0644") } - if tmpl.Uid == nil { - tmpl.Uid = pointerOf(-1) - } - if tmpl.Gid == nil { - tmpl.Gid = pointerOf(-1) - } if tmpl.LeftDelim == nil { tmpl.LeftDelim = pointerOf("{{") } diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 9e307ff01..d3536018c 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -627,9 +627,11 @@ func parseTemplateConfigs(config *TaskTemplateManagerConfig) (map[*ctconf.Templa ct.Perms = &m } // Set ownership - if tmpl.Uid >= 0 && tmpl.Gid >= 0 { - ct.Uid = &tmpl.Uid - ct.Gid = &tmpl.Gid + if tmpl.Uid != nil && *tmpl.Uid >= 0 { + ct.Uid = tmpl.Uid + } + if tmpl.Gid != nil && *tmpl.Gid >= 0 { + ct.Gid = tmpl.Gid } ct.Finalize() diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index 467882234..b168e11dc 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -514,8 +514,8 @@ func TestTaskTemplateManager_Permissions(t *testing.T) { DestPath: file, ChangeMode: structs.TemplateChangeModeNoop, Perms: "777", - Uid: 503, - Gid: 20, + Uid: pointer.Of(503), + Gid: pointer.Of(20), } harness := newTestHarness(t, []*structs.Template{template}, false, false) @@ -541,8 +541,8 @@ func TestTaskTemplateManager_Permissions(t *testing.T) { } sys := fi.Sys() - uid := int(sys.(*syscall.Stat_t).Uid) - gid := int(sys.(*syscall.Stat_t).Gid) + uid := pointer.Of(int(sys.(*syscall.Stat_t).Uid)) + gid := pointer.Of(int(sys.(*syscall.Stat_t).Gid)) must.Eq(t, template.Uid, uid) must.Eq(t, template.Gid, gid) diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index c1df09cc9..20fbca217 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1209,14 +1209,6 @@ func ApiTaskToStructsTask(job *structs.Job, group *structs.TaskGroup, if len(apiTask.Templates) > 0 { structsTask.Templates = []*structs.Template{} for _, template := range apiTask.Templates { - uid := -1 - if template.Uid != nil { - uid = *template.Uid - } - gid := -1 - if template.Gid != nil { - gid = *template.Gid - } structsTask.Templates = append(structsTask.Templates, &structs.Template{ SourcePath: *template.SourcePath, @@ -1226,8 +1218,8 @@ func ApiTaskToStructsTask(job *structs.Job, group *structs.TaskGroup, ChangeSignal: *template.ChangeSignal, Splay: *template.Splay, Perms: *template.Perms, - Uid: uid, - Gid: gid, + Uid: template.Uid, + Gid: template.Gid, LeftDelim: *template.LeftDelim, RightDelim: *template.RightDelim, Envvars: *template.Envvars, diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index 08a63838e..0302d7cc7 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -3139,8 +3139,8 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { ChangeSignal: "SIGNAL", Splay: 1 * time.Minute, Perms: "666", - Uid: 1000, - Gid: 1000, + Uid: pointer.Of(1000), + Gid: pointer.Of(1000), LeftDelim: "abc", RightDelim: "def", Envvars: true, diff --git a/jobspec/helper.go b/jobspec/helper.go index 826cba065..99bf80f83 100644 --- a/jobspec/helper.go +++ b/jobspec/helper.go @@ -18,11 +18,6 @@ func stringToPtr(str string) *string { return &str } -// intToPtr returns the pointer to an int -func intToPtr(i int) *int { - return &i -} - // timeToPtr returns the pointer to a time.Duration. func timeToPtr(t time.Duration) *time.Duration { return &t diff --git a/jobspec/helper_test.go b/jobspec/helper_test.go new file mode 100644 index 000000000..4f52f4de5 --- /dev/null +++ b/jobspec/helper_test.go @@ -0,0 +1,9 @@ +package jobspec + +// These functions are copied from helper/funcs.go +// added here to avoid jobspec depending on any other package + +// intToPtr returns the pointer to an int +func intToPtr(i int) *int { + return &i +} diff --git a/jobspec/parse_task.go b/jobspec/parse_task.go index 19bb669cc..016de86ff 100644 --- a/jobspec/parse_task.go +++ b/jobspec/parse_task.go @@ -462,8 +462,6 @@ func parseTemplates(result *[]*api.Template, list *ast.ObjectList) error { ChangeMode: stringToPtr("restart"), Splay: timeToPtr(5 * time.Second), Perms: stringToPtr("0644"), - Uid: intToPtr(-1), - Gid: intToPtr(-1), } dec, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index 0d7467c16..75a8325ff 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -374,8 +374,6 @@ func TestParse(t *testing.T) { ChangeSignal: stringToPtr("foo"), Splay: timeToPtr(10 * time.Second), Perms: stringToPtr("0644"), - Uid: intToPtr(-1), - Gid: intToPtr(-1), Envvars: boolToPtr(true), VaultGrace: timeToPtr(33 * time.Second), }, diff --git a/jobspec2/helper_test.go b/jobspec2/helper_test.go new file mode 100644 index 000000000..57a6cd360 --- /dev/null +++ b/jobspec2/helper_test.go @@ -0,0 +1,5 @@ +package jobspec2 + +func intToPtr(v int) *int { + return &v +} diff --git a/jobspec2/parse_job.go b/jobspec2/parse_job.go index 25ec2381a..9b533874f 100644 --- a/jobspec2/parse_job.go +++ b/jobspec2/parse_job.go @@ -107,12 +107,6 @@ func normalizeTemplates(templates []*api.Template) { if t.Perms == nil { t.Perms = stringToPtr("0644") } - if t.Uid == nil { - t.Uid = intToPtr(-1) - } - if t.Gid == nil { - t.Gid = intToPtr(-1) - } if t.Splay == nil { t.Splay = durationToPtr(5 * time.Second) } @@ -127,10 +121,6 @@ func boolToPtr(v bool) *bool { return &v } -func intToPtr(v int) *int { - return &v -} - func stringToPtr(v string) *string { return &v } diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index 8c16123e1..bf827bf63 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -1671,6 +1671,24 @@ func templateDiff(old, new *Template, contextual bool) *ObjectDiff { newPrimitiveFlat = flatmap.Flatten(new, nil, true) } + // Add the pointer primitive fields. + if old != nil { + if old.Uid != nil { + oldPrimitiveFlat["Uid"] = fmt.Sprintf("%v", *old.Uid) + } + if old.Gid != nil { + oldPrimitiveFlat["Gid"] = fmt.Sprintf("%v", *old.Gid) + } + } + if new != nil { + if new.Uid != nil { + newPrimitiveFlat["Uid"] = fmt.Sprintf("%v", *new.Uid) + } + if new.Gid != nil { + newPrimitiveFlat["Gid"] = fmt.Sprintf("%v", *new.Gid) + } + } + // Diff the primitive fields. diff.Fields = fieldDiffs(oldPrimitiveFlat, newPrimitiveFlat, contextual) diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 3e96ca8b0..f141f06a5 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -7044,8 +7044,8 @@ func TestTaskDiff(t *testing.T) { ChangeSignal: "SIGHUP", Splay: 1, Perms: "0644", - Uid: 1001, - Gid: 21, + Uid: pointer.Of(1001), + Gid: pointer.Of(21), Wait: &WaitConfig{ Min: pointer.Of(5 * time.Second), Max: pointer.Of(5 * time.Second), @@ -7059,8 +7059,8 @@ func TestTaskDiff(t *testing.T) { ChangeSignal: "SIGHUP2", Splay: 2, Perms: "0666", - Uid: 1000, - Gid: 20, + Uid: pointer.Of(1000), + Gid: pointer.Of(20), Envvars: true, }, }, @@ -7075,8 +7075,8 @@ func TestTaskDiff(t *testing.T) { ChangeSignal: "SIGHUP", Splay: 1, Perms: "0644", - Uid: 1001, - Gid: 21, + Uid: pointer.Of(1001), + Gid: pointer.Of(21), Wait: &WaitConfig{ Min: pointer.Of(5 * time.Second), Max: pointer.Of(10 * time.Second), @@ -7090,8 +7090,8 @@ func TestTaskDiff(t *testing.T) { ChangeSignal: "SIGHUP3", Splay: 3, Perms: "0776", - Uid: 1002, - Gid: 22, + Uid: pointer.Of(1002), + Gid: pointer.Of(22), Wait: &WaitConfig{ Min: pointer.Of(5 * time.Second), Max: pointer.Of(10 * time.Second), diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 2f07ce897..0e14b4267 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -7647,8 +7647,8 @@ type Template struct { // Perms is the permission the file should be written out with. Perms string // User and group that should own the file. - Uid int - Gid int + Uid *int + Gid *int // LeftDelim and RightDelim are optional configurations to control what // delimiter is utilized when parsing the template. diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 1e26dd1bf..5684994f7 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -2579,6 +2579,64 @@ func TestTask_Validate_Template(t *testing.T) { } } +func TestTemplate_Copy(t *testing.T) { + ci.Parallel(t) + + t1 := &Template{ + 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, + Wait: &WaitConfig{ + Min: pointer.Of(time.Second), + Max: pointer.Of(time.Minute), + }, + } + t2 := t1.Copy() + + t1.SourcePath = "/local/file2.txt" + t1.DestPath = "/local/dest2.txt" + t1.EmbeddedTmpl = "tpl2" + t1.ChangeMode = TemplateChangeModeRestart + t1.ChangeSignal = "" + t1.Splay = 5 * time.Second + t1.Perms = "700" + t1.Uid = pointer.Of(5000) + t1.Gid = pointer.Of(6000) + t1.LeftDelim = "((" + t1.RightDelim = "))" + t1.Envvars = false + t1.VaultGrace = 2 * time.Minute + t1.Wait.Min = pointer.Of(2 * time.Second) + t1.Wait.Max = pointer.Of(2 * time.Minute) + + require.NotEqual(t, t1.SourcePath, t2.SourcePath) + 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.Splay, t2.Splay) + require.NotEqual(t, t1.Perms, t2.Perms) + require.NotEqual(t, t1.Uid, t2.Uid) + require.NotEqual(t, t1.Gid, t2.Gid) + require.NotEqual(t, t1.LeftDelim, t2.LeftDelim) + require.NotEqual(t, t1.RightDelim, t2.RightDelim) + require.NotEqual(t, t1.Envvars, t2.Envvars) + require.NotEqual(t, t1.VaultGrace, t2.VaultGrace) + require.NotEqual(t, t1.Wait.Min, t2.Wait.Min) + require.NotEqual(t, t1.Wait.Max, t2.Wait.Max) + +} + func TestTemplate_Validate(t *testing.T) { ci.Parallel(t) diff --git a/website/content/docs/job-specification/template.mdx b/website/content/docs/job-specification/template.mdx index 43c9c934f..80121f5b3 100644 --- a/website/content/docs/job-specification/template.mdx +++ b/website/content/docs/job-specification/template.mdx @@ -84,16 +84,17 @@ refer to the [Learn Go Template Syntax][gt_learn] Learn guide. - `perms` `(string: "644")` - Specifies the rendered template's permissions. File permissions are given as octal of the Unix file permissions `rwxrwxrwx`. -- `uid` `(int: -1)` - Specifies the rendered template owner's user ID. Negative - values will use the ID of the Nomad agent user. +- `uid` `(int: nil)` - Specifies the rendered template owner's user ID. If + negative or not specified (`nil`) the ID of the Nomad agent user wil be used. ~> **Caveat:** Works only on Unix-based systems. Be careful when using containerized drivers, such as `docker` or `podman`, as groups and users inside the container may have different IDs than on the host system. This feature will also **not** work with Docker Desktop. -- `gid` `(int: -1)` - Specifies the rendered template owner's group ID. - Negative values will use the ID of the Nomad agent group. +- `gid` `(int: nil)` - Specifies the rendered template owner's group ID. If + negative or not specified (`nil`) the ID of the Nomad agent group will be + used. ~> **Caveat:** Works only on Unix-based systems. Be careful when using containerized drivers, such as `docker` or `podman`, as groups and users diff --git a/website/content/docs/upgrade/upgrade-specific.mdx b/website/content/docs/upgrade/upgrade-specific.mdx index f3b712283..86b249b20 100644 --- a/website/content/docs/upgrade/upgrade-specific.mdx +++ b/website/content/docs/upgrade/upgrade-specific.mdx @@ -23,6 +23,19 @@ to version 3, and in Nomad 1.4.0 Nomad requires the use of raft protocol version 3. If [`raft_protocol`] version is explicitly set, it must now be set to `3`. For more information see the [Upgrading to Raft Protocol 3] guide. +## Nomad 1.3.3 + +Environments that don't support the use of [`uid`][template_uid] and +[`gid`][template_gid] in `template` blocks, such as Windows clients, may +experience task failures with the following message after upgrading to Nomad +1.3.3: + +``` +Template failed: error rendering "(dynamic)" => "...": failed looking up user: managing file ownership is not supported on Windows +``` + +It is recommended to avoid this version of Nomad in such environments. + ## Nomad 1.3.2, 1.2.9, 1.1.15 #### Client `max_kill_timeout` now enforced @@ -1428,6 +1441,8 @@ deleted and then Nomad 0.3.0 can be launched. [vault_grace]: /docs/job-specification/template [node drain]: https://www.nomadproject.io/docs/upgrade#5-upgrade-clients [`template.disable_file_sandbox`]: /docs/configuration/client#template-parameters +[template_gid]: /docs/job-specification/template#gid +[template_uid]: /docs/job-specification/template#uid [pki]: https://www.vaultproject.io/docs/secrets/pki [`volume create`]: /docs/commands/volume/create [`volume register`]: /docs/commands/volume/register @@ -1448,4 +1463,4 @@ deleted and then Nomad 0.3.0 can be launched. [anon_token]: https://www.consul.io/docs/security/acl/acl-tokens#special-purpose-tokens [consul_acl]: https://github.com/hashicorp/consul/issues/7414 [kill_timeout]: /docs/job-specification/task#kill_timeout -[max_kill_timeout]: /docs/configuration/client#max_kill_timeout \ No newline at end of file +[max_kill_timeout]: /docs/configuration/client#max_kill_timeout