Merge pull request #9020 from hashicorp/b-8796-i-do-not-want-no-null-…

…chars

job, task group, and task IDs should not allow null characters

CHANGELOG.md

@@ -11,6 +11,7 @@ IMPROVEMENTS:
  * jobspec: Lowered minimum CPU allowed from 10 to 1. [[GH-8996](https://github.com/hashicorp/nomad/issues/8996)]
 
 __BACKWARDS INCOMPATIBILITIES:__
+ * 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:

api/jobs_test.go

@@ -1291,7 +1291,7 @@ func TestJobs_Info(t *testing.T) {
 
 	// Trying to retrieve a job by ID before it exists
 	// returns an error
-	id := "job-id/with\\troublesome:characters\n?&字\000"
+	id := "job-id/with\\troublesome:characters\n?&字"
 	_, _, err := jobs.Info(id, nil)
 	if err == nil || !strings.Contains(err.Error(), "not found") {
 		t.Fatalf("expected not found error, got: %#v", err)
@@ -1993,7 +1993,7 @@ func TestJobs_ScaleAction(t *testing.T) {
 	defer s.Stop()
 	jobs := c.Jobs()
 
-	id := "job-id/with\\troublesome:characters\n?&字\000"
+	id := "job-id/with\\troublesome:characters\n?&字"
 	job := testJobWithScalingPolicy()
 	job.ID = &id
 	groupName := *job.TaskGroups[0].Name
@@ -2054,7 +2054,7 @@ func TestJobs_ScaleAction_Error(t *testing.T) {
 	defer s.Stop()
 	jobs := c.Jobs()
 
-	id := "job-id/with\\troublesome:characters\n?&字\000"
+	id := "job-id/with\\troublesome:characters\n?&字"
 	job := testJobWithScalingPolicy()
 	job.ID = &id
 	groupName := *job.TaskGroups[0].Name
@@ -2106,7 +2106,7 @@ func TestJobs_ScaleAction_Noop(t *testing.T) {
 	defer s.Stop()
 	jobs := c.Jobs()
 
-	id := "job-id/with\\troublesome:characters\n?&字\000"
+	id := "job-id/with\\troublesome:characters\n?&字"
 	job := testJobWithScalingPolicy()
 	job.ID = &id
 	groupName := *job.TaskGroups[0].Name
@@ -2161,7 +2161,7 @@ func TestJobs_ScaleStatus(t *testing.T) {
 	jobs := c.Jobs()
 
 	// Trying to retrieve a status before it exists returns an error
-	id := "job-id/with\\troublesome:characters\n?&字\000"
+	id := "job-id/with\\troublesome:characters\n?&字"
 	_, _, err := jobs.ScaleStatus(id, nil)
 	require.Error(err)
 	require.Contains(err.Error(), "not found")

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.

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 := cli.NewMockUi()
+		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 := cli.NewMockUi()
+		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)
+		}
+	}
+}

nomad/structs/structs.go

@@ -3943,9 +3943,13 @@ func (j *Job) Validate() error {
 		mErr.Errors = append(mErr.Errors, errors.New("Missing job ID"))
 	} else if strings.Contains(j.ID, " ") {
 		mErr.Errors = append(mErr.Errors, errors.New("Job ID contains a space"))
+	} else if strings.Contains(j.ID, "\000") {
+		mErr.Errors = append(mErr.Errors, errors.New("Job ID contains a null chararacter"))
 	}
 	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"))
@@ -5740,6 +5744,8 @@ func (tg *TaskGroup) Validate(j *Job) error {
 	var mErr multierror.Error
 	if tg.Name == "" {
 		mErr.Errors = append(mErr.Errors, errors.New("Missing task group name"))
+	} else if strings.Contains(tg.Name, "\000") {
+		mErr.Errors = append(mErr.Errors, errors.New("Task group name contains null character"))
 	}
 	if tg.Count < 0 {
 		mErr.Errors = append(mErr.Errors, errors.New("Task group count can't be negative"))
@@ -6462,6 +6468,8 @@ func (t *Task) Validate(ephemeralDisk *EphemeralDisk, jobType string, tgServices
 		// We enforce this so that when creating the directory on disk it will
 		// not have any slashes.
 		mErr.Errors = append(mErr.Errors, errors.New("Task name cannot include slashes"))
+	} else if strings.Contains(t.Name, "\000") {
+		mErr.Errors = append(mErr.Errors, errors.New("Task name cannot include null characters"))
 	}
 	if t.Driver == "" {
 		mErr.Errors = append(mErr.Errors, errors.New("Missing task driver"))

nomad/structs/structs_test.go

@@ -170,6 +170,30 @@ func TestJob_ValidateScaling(t *testing.T) {
 	require.Contains(mErr.Errors[0].Error(), "task group count must not be less than minimum count in scaling policy")
 }
 
+func TestJob_ValidateNullChar(t *testing.T) {
+	assert := assert.New(t)
+
+	// job id should not allow null characters
+	job := testJob()
+	job.ID = "id_with\000null_character"
+	assert.Error(job.Validate(), "null character in job ID should not validate")
+
+	// 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")
+
+	// task name should not allow null characters
+	job.TaskGroups[0].Name = "so_much_better"
+	job.TaskGroups[0].Tasks[0].Name = "ive_had_it_with_these_\000_chars_in_these_names"
+	assert.Error(job.Validate(), "null character in task name should not validate")
+}
+
 func TestJob_Warnings(t *testing.T) {
 	cases := []struct {
 		Name     string

website/pages/docs/upgrade/upgrade-specific.mdx

@@ -24,6 +24,14 @@ 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 region, datacenter, job name/ID, task group name, and task names
+
+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
 
 ### `mbits` and Task Network Resource deprecation