From a0485008fcec516188744e393afdff1f76faf7e1 Mon Sep 17 00:00:00 2001 From: Chris Baker <1675087+cgbaker@users.noreply.github.com> Date: Mon, 5 Oct 2020 14:52:07 +0000 Subject: [PATCH] updated docs and validation to further prohibit null chars in region, datacenter, and job name --- CHANGELOG.md | 3 +- command/agent/command.go | 12 +++ command/agent/command_test.go | 101 +++++++++++++++++- nomad/structs/structs.go | 2 + nomad/structs/structs_test.go | 7 +- .../pages/docs/upgrade/upgrade-specific.mdx | 10 +- 6 files changed, 128 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5f0157f7e85..0c05535d7c2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,8 @@ IMPROVEMENTS: * jobspec: Lowered minimum CPU allowed from 10 to 1. [[GH-8996](https://github.com/hashicorp/nomad/issues/8996)] __BACKWARDS INCOMPATIBILITIES:__ - * core: job IDs, task group names, and task names are not allowed to contain null characters [[GH-9020](https://github.com/hashicorp/nomad/issues/9020)] + * core: null characters are prohibited in region, datacenter, job name/ID, task group name, and task name [[GH-9020 + ](https://github.com/hashicorp/nomad/issues/9020)] * driver/docker: Tasks are now issued SIGTERM instead of SIGINT when stopping [[GH-8932](https://github.com/hashicorp/nomad/issues/8932)] BUG FIXES: diff --git a/command/agent/command.go b/command/agent/command.go index b394ca99faf6..d94671e4e061 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -302,6 +302,18 @@ func (c *Command) isValidConfig(config, cmdConfig *Config) bool { return false } + // Check that the region does not contain invalid characters + if strings.ContainsAny(config.Region, "\000") { + c.Ui.Error("Region contains invalid characters") + return false + } + + // Check that the datacenter name does not contain invalid characters + if strings.ContainsAny(config.Datacenter, "\000") { + c.Ui.Error("Datacenter contains invalid characters") + return false + } + // Set up the TLS configuration properly if we have one. // XXX chelseakomlo: set up a TLSConfig New method which would wrap // constructor-type actions like this. diff --git a/command/agent/command_test.go b/command/agent/command_test.go index eed6bba82ed2..ad7090bfec68 100644 --- a/command/agent/command_test.go +++ b/command/agent/command_test.go @@ -7,8 +7,9 @@ import ( "strings" "testing" - "github.com/hashicorp/nomad/version" "github.com/mitchellh/cli" + + "github.com/hashicorp/nomad/version" ) func TestCommand_Implements(t *testing.T) { @@ -142,3 +143,101 @@ func TestCommand_MetaConfigValidation(t *testing.T) { } } } + +func TestCommand_NullCharInDatacenter(t *testing.T) { + tmpDir, err := ioutil.TempDir("", "nomad") + if err != nil { + t.Fatalf("err: %s", err) + } + defer os.RemoveAll(tmpDir) + + tcases := []string{ + "char-\\000-in-the-middle", + "ends-with-\\000", + "\\000-at-the-beginning", + } + for _, tc := range tcases { + configFile := filepath.Join(tmpDir, "conf1.hcl") + err = ioutil.WriteFile(configFile, []byte(` + datacenter = "`+tc+`" + client{ + enabled = true + }`), 0600) + if err != nil { + t.Fatalf("err: %s", err) + } + + // Make a new command. We preemptively close the shutdownCh + // so that the command exits immediately instead of blocking. + ui := new(cli.MockUi) + shutdownCh := make(chan struct{}) + close(shutdownCh) + cmd := &Command{ + Version: version.GetVersion(), + Ui: ui, + ShutdownCh: shutdownCh, + } + + // To prevent test failures on hosts whose hostname resolves to + // a loopback address, we must append a bind address + args := []string{"-client", "-data-dir=" + tmpDir, "-config=" + configFile, "-bind=169.254.0.1"} + if code := cmd.Run(args); code != 1 { + t.Fatalf("args: %v\nexit: %d\n", args, code) + } + + out := ui.ErrorWriter.String() + exp := "Datacenter contains invalid characters" + if !strings.Contains(out, exp) { + t.Fatalf("expect to find %q\n\n%s", exp, out) + } + } +} + +func TestCommand_NullCharInRegion(t *testing.T) { + tmpDir, err := ioutil.TempDir("", "nomad") + if err != nil { + t.Fatalf("err: %s", err) + } + defer os.RemoveAll(tmpDir) + + tcases := []string{ + "char-\\000-in-the-middle", + "ends-with-\\000", + "\\000-at-the-beginning", + } + for _, tc := range tcases { + configFile := filepath.Join(tmpDir, "conf1.hcl") + err = ioutil.WriteFile(configFile, []byte(` + region = "`+tc+`" + client{ + enabled = true + }`), 0600) + if err != nil { + t.Fatalf("err: %s", err) + } + + // Make a new command. We preemptively close the shutdownCh + // so that the command exits immediately instead of blocking. + ui := new(cli.MockUi) + shutdownCh := make(chan struct{}) + close(shutdownCh) + cmd := &Command{ + Version: version.GetVersion(), + Ui: ui, + ShutdownCh: shutdownCh, + } + + // To prevent test failures on hosts whose hostname resolves to + // a loopback address, we must append a bind address + args := []string{"-client", "-data-dir=" + tmpDir, "-config=" + configFile, "-bind=169.254.0.1"} + if code := cmd.Run(args); code != 1 { + t.Fatalf("args: %v\nexit: %d\n", args, code) + } + + out := ui.ErrorWriter.String() + exp := "Region contains invalid characters" + if !strings.Contains(out, exp) { + t.Fatalf("expect to find %q\n\n%s", exp, out) + } + } +} diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 41f8adc4ae87..66150b2dd566 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -3948,6 +3948,8 @@ func (j *Job) Validate() error { } if j.Name == "" { mErr.Errors = append(mErr.Errors, errors.New("Missing job name")) + } else if strings.Contains(j.Name, "\000") { + mErr.Errors = append(mErr.Errors, errors.New("Job Name contains a null chararacter")) } if j.Namespace == "" { mErr.Errors = append(mErr.Errors, errors.New("Job must be in a namespace")) diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 9de7bece9986..0f11942633cb 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -178,8 +178,13 @@ func TestJob_ValidateNullChar(t *testing.T) { job.ID = "id_with\000null_character" assert.Error(job.Validate(), "null character in job ID should not validate") - // task group name should not allow null characters + // job name should not allow null characters job.ID = "happy_little_job_id" + job.Name = "my job name with \000 characters" + assert.Error(job.Validate(), "null character in job name should not validate") + + // task group name should not allow null characters + job.Name = "my job" job.TaskGroups[0].Name = "oh_no_another_\000_char" assert.Error(job.Validate(), "null character in task group name should not validate") diff --git a/website/pages/docs/upgrade/upgrade-specific.mdx b/website/pages/docs/upgrade/upgrade-specific.mdx index ff2a9c0f1c02..d04949403969 100644 --- a/website/pages/docs/upgrade/upgrade-specific.mdx +++ b/website/pages/docs/upgrade/upgrade-specific.mdx @@ -24,11 +24,13 @@ When stopping tasks running with the Docker task driver, Nomad documents that a versions of Nomad would issue `SIGINT` instead. Starting again with Nomad v0.13.0 `SIGTERM` will be sent by default when stopping Docker tasks. -### Null characters in job, task group, and task IDs +### Null characters in region, datacenter, job name/ID, task group name, and task names -Starting with Nomad v0.13.0, job will fail validation if any of the following -contain null character: the job ID, the task group name, or the task name. Any -jobs meeting this requirement should be modified before an update to v0.13.0. +Starting with Nomad v0.13.0, jobs will fail validation if any of the following +contain null character: the job ID or name, the task group name, or the task name. Any +jobs meeting this requirement should be modified before an update to v0.13.0. Similarly, +client and server config validation will prohibit either the region or the datacenter +from containing null characters. ## Nomad 0.12.0