From c26ec1d5b3bd84435936fc6410a464f49c7ffcb4 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Fri, 19 Aug 2022 09:48:35 -0400 Subject: [PATCH 1/4] template: use pointer values for gid and uid 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. --- 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 +++++++++++++++++++ 16 files changed, 113 insertions(+), 58 deletions(-) create mode 100644 jobspec/helper_test.go create mode 100644 jobspec2/helper_test.go diff --git a/api/jobs_test.go b/api/jobs_test.go index 78b042bc63ae..f6e3b3094c8e 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 9e307ff013e9..d3536018c2e8 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 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 c1df09cc943d..20fbca217668 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 08a63838e045..0302d7cc7889 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 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 0d7467c1692f..75a8325ff23d 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 3e96ca8b0665..f141f06a5296 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 2f07ce897666..0e14b4267b7c 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 1e26dd1bfc10..5684994f783a 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) From 7fc130604f0ec397362ebee7ca0ef74ab69d9221 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Fri, 19 Aug 2022 17:36:02 -0400 Subject: [PATCH 2/4] docs: update template uid and gid and add upgrade notice --- .../content/docs/job-specification/template.mdx | 4 ++-- .../content/docs/upgrade/upgrade-specific.mdx | 17 ++++++++++++++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/website/content/docs/job-specification/template.mdx b/website/content/docs/job-specification/template.mdx index 43c9c934faa3..23d5849d035b 100644 --- a/website/content/docs/job-specification/template.mdx +++ b/website/content/docs/job-specification/template.mdx @@ -84,7 +84,7 @@ 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 +- `uid` `(int: nil)` - Specifies the rendered template owner's user ID. Negative values will use the ID of the Nomad agent user. ~> **Caveat:** Works only on Unix-based systems. Be careful when using @@ -92,7 +92,7 @@ refer to the [Learn Go Template Syntax][gt_learn] Learn guide. 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. +- `gid` `(int: nil)` - Specifies the rendered template owner's group ID. Negative values will use the ID of the Nomad agent group. ~> **Caveat:** Works only on Unix-based systems. Be careful when using diff --git a/website/content/docs/upgrade/upgrade-specific.mdx b/website/content/docs/upgrade/upgrade-specific.mdx index f3b712283ea8..86b249b205d9 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 From 2934bc4ed96f6616021dc8af40bcb348c9e87822 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Fri, 19 Aug 2022 17:36:30 -0400 Subject: [PATCH 3/4] changelog: add entry for #14203 --- .changelog/14203.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/14203.txt diff --git a/.changelog/14203.txt b/.changelog/14203.txt new file mode 100644 index 000000000000..6dc13f1c15a4 --- /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 +``` From da34115ac520632fa383691f12af4a622e5cf497 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Mon, 22 Aug 2022 15:32:16 -0400 Subject: [PATCH 4/4] apply code review --- .changelog/14203.txt | 2 +- website/content/docs/job-specification/template.mdx | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.changelog/14203.txt b/.changelog/14203.txt index 6dc13f1c15a4..f331d84c8782 100644 --- a/.changelog/14203.txt +++ b/.changelog/14203.txt @@ -1,3 +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 +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/website/content/docs/job-specification/template.mdx b/website/content/docs/job-specification/template.mdx index 23d5849d035b..80121f5b398e 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: nil)` - 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: nil)` - 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