From 253339f330bc5ed5df0d0b2b302ac9fe936a121f Mon Sep 17 00:00:00 2001 From: hc-github-team-nomad-core <82989552+hc-github-team-nomad-core@users.noreply.github.com> Date: Mon, 22 Aug 2022 18:37:30 -0400 Subject: [PATCH] template: use pointer values for gid and uid (#14203) (#14220) 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. Co-authored-by: Luiz Aoqui --- .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 | 15 +++++ 19 files changed, 136 insertions(+), 62 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 000000000000..f331d84c8782 --- /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 970498e856b7..42dc5e4f2b22 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 c4db6224bdc8..a53efbe8ad3e 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -837,12 +837,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 b60088697580..f3bd26b0da54 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -624,9 +624,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 467882234c8e..b168e11dc2c4 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 1e24ed6829a9..3776a5405b45 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 aa07c9d759e2..990416deb07a 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -3134,8 +3134,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 826cba0652ba..99bf80f830f1 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 000000000000..4f52f4de565f --- /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 19bb669cc3b2..016de86ff309 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 bad9454cb886..01cb58cbfc7b 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 000000000000..57a6cd36028d --- /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 25ec2381ad80..9b533874f500 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 8c16123e1c82..bf827bf63625 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 ba8e9d40f563..bc049b6862bf 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 03b31532963f..d4e4f0eb0ccf 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -7689,8 +7689,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 2ddf2418b377..41dd0d14997a 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -2580,6 +2580,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 c324f3198553..650320d2f5c3 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, suck 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, suck 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 77b5f8d5866f..e5690a803501 100644 --- a/website/content/docs/upgrade/upgrade-specific.mdx +++ b/website/content/docs/upgrade/upgrade-specific.mdx @@ -13,6 +13,19 @@ upgrade. However, specific versions of Nomad may have more details provided for their upgrades as a result of new features or changed behavior. This page is used to document those details separately from the standard upgrade flow. +## 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 @@ -1418,6 +1431,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