From dbbb4a957a6d1906ce03cc2a986d45f84da1318b Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Mon, 23 Jul 2018 11:06:49 -0500 Subject: [PATCH] Fail validation if system job has affinities --- nomad/structs/structs.go | 48 +++++++++++++++++++++++------------ nomad/structs/structs_test.go | 45 +++++++++++++++++++++++--------- 2 files changed, 65 insertions(+), 28 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 8a59c8a9d..77f6e1b4d 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -2172,11 +2172,16 @@ func (j *Job) Validate() error { mErr.Errors = append(mErr.Errors, outer) } } - - for idx, affinity := range j.Affinities { - if err := affinity.Validate(); err != nil { - outer := fmt.Errorf("Affinity %d validation failed: %s", idx+1, err) - mErr.Errors = append(mErr.Errors, outer) + if j.Type == JobTypeSystem { + if j.Affinities != nil { + mErr.Errors = append(mErr.Errors, fmt.Errorf("System jobs should not have an affinity stanza")) + } + } else { + for idx, affinity := range j.Affinities { + if err := affinity.Validate(); err != nil { + outer := fmt.Errorf("Affinity %d validation failed: %s", idx+1, err) + mErr.Errors = append(mErr.Errors, outer) + } } } @@ -3424,11 +3429,16 @@ func (tg *TaskGroup) Validate(j *Job) error { mErr.Errors = append(mErr.Errors, outer) } } - - for idx, affinity := range tg.Affinities { - if err := affinity.Validate(); err != nil { - outer := fmt.Errorf("Affinity %d validation failed: %s", idx+1, err) - mErr.Errors = append(mErr.Errors, outer) + if j.Type == JobTypeSystem { + if tg.Affinities != nil { + mErr.Errors = append(mErr.Errors, fmt.Errorf("System jobs should not have an affinity stanza")) + } + } else { + for idx, affinity := range tg.Affinities { + if err := affinity.Validate(); err != nil { + outer := fmt.Errorf("Affinity %d validation failed: %s", idx+1, err) + mErr.Errors = append(mErr.Errors, outer) + } } } @@ -3528,7 +3538,7 @@ func (tg *TaskGroup) Validate(j *Job) error { // Validate the tasks for _, task := range tg.Tasks { - if err := task.Validate(tg.EphemeralDisk); err != nil { + if err := task.Validate(tg.EphemeralDisk, j.Type); err != nil { outer := fmt.Errorf("Task %s validation failed: %v", task.Name, err) mErr.Errors = append(mErr.Errors, outer) } @@ -4164,7 +4174,7 @@ func (t *Task) GoString() string { } // Validate is used to sanity check a task -func (t *Task) Validate(ephemeralDisk *EphemeralDisk) error { +func (t *Task) Validate(ephemeralDisk *EphemeralDisk, jobType string) error { var mErr multierror.Error if t.Name == "" { mErr.Errors = append(mErr.Errors, errors.New("Missing task name")) @@ -4218,10 +4228,16 @@ func (t *Task) Validate(ephemeralDisk *EphemeralDisk) error { } } - for idx, affinity := range t.Affinities { - if err := affinity.Validate(); err != nil { - outer := fmt.Errorf("Affinity %d validation failed: %s", idx+1, err) - mErr.Errors = append(mErr.Errors, outer) + if jobType == JobTypeSystem { + if t.Affinities != nil { + mErr.Errors = append(mErr.Errors, fmt.Errorf("System jobs should not have an affinity stanza")) + } + } else { + for idx, affinity := range t.Affinities { + if err := affinity.Validate(); err != nil { + outer := fmt.Errorf("Affinity %d validation failed: %s", idx+1, err) + mErr.Errors = append(mErr.Errors, outer) + } } } diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 79d6b28ea..60ef60349 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -384,6 +384,27 @@ func TestJob_SystemJob_Validate(t *testing.T) { if err := j.Validate(); err != nil { t.Fatalf("unexpected err: %v", err) } + + // Add affinities at job, task group and task level, that should fail validation + + j.Affinities = []*Affinity{{ + Operand: "=", + LTarget: "${node.datacenter}", + RTarget: "dc1", + }} + j.TaskGroups[0].Affinities = []*Affinity{{ + Operand: "=", + LTarget: "${meta.rack}", + RTarget: "r1", + }} + j.TaskGroups[0].Tasks[0].Affinities = []*Affinity{{ + Operand: "=", + LTarget: "${meta.rack}", + RTarget: "r1", + }} + err = j.Validate() + require.NotNil(t, err) + require.Contains(t, err.Error(), "System jobs should not have an affinity stanza") } func TestJob_VaultPolicies(t *testing.T) { @@ -739,7 +760,7 @@ func TestTaskGroup_Validate(t *testing.T) { func TestTask_Validate(t *testing.T) { task := &Task{} ephemeralDisk := DefaultEphemeralDisk() - err := task.Validate(ephemeralDisk) + err := task.Validate(ephemeralDisk, JobTypeBatch) mErr := err.(*multierror.Error) if !strings.Contains(mErr.Errors[0].Error(), "task name") { t.Fatalf("err: %s", err) @@ -752,7 +773,7 @@ func TestTask_Validate(t *testing.T) { } task = &Task{Name: "web/foo"} - err = task.Validate(ephemeralDisk) + err = task.Validate(ephemeralDisk, JobTypeBatch) mErr = err.(*multierror.Error) if !strings.Contains(mErr.Errors[0].Error(), "slashes") { t.Fatalf("err: %s", err) @@ -769,7 +790,7 @@ func TestTask_Validate(t *testing.T) { LogConfig: DefaultLogConfig(), } ephemeralDisk.SizeMB = 200 - err = task.Validate(ephemeralDisk) + err = task.Validate(ephemeralDisk, JobTypeBatch) if err != nil { t.Fatalf("err: %s", err) } @@ -783,7 +804,7 @@ func TestTask_Validate(t *testing.T) { LTarget: "${meta.rack}", }) - err = task.Validate(ephemeralDisk) + err = task.Validate(ephemeralDisk, JobTypeBatch) mErr = err.(*multierror.Error) if !strings.Contains(mErr.Errors[0].Error(), "task level: distinct_hosts") { t.Fatalf("err: %s", err) @@ -866,7 +887,7 @@ func TestTask_Validate_Services(t *testing.T) { }, } - err := task.Validate(ephemeralDisk) + err := task.Validate(ephemeralDisk, JobTypeService) if err == nil { t.Fatal("expected an error") } @@ -887,7 +908,7 @@ func TestTask_Validate_Services(t *testing.T) { t.Fatalf("err: %v", err) } - if err = task1.Validate(ephemeralDisk); err != nil { + if err = task1.Validate(ephemeralDisk, JobTypeService); err != nil { t.Fatalf("err : %v", err) } } @@ -946,7 +967,7 @@ func TestTask_Validate_Service_AddressMode_Ok(t *testing.T) { for _, service := range cases { task := getTask(service) t.Run(service.Name, func(t *testing.T) { - if err := task.Validate(ephemeralDisk); err != nil { + if err := task.Validate(ephemeralDisk, JobTypeService); err != nil { t.Fatalf("unexpected err: %v", err) } }) @@ -999,7 +1020,7 @@ func TestTask_Validate_Service_AddressMode_Bad(t *testing.T) { for _, service := range cases { task := getTask(service) t.Run(service.Name, func(t *testing.T) { - err := task.Validate(ephemeralDisk) + err := task.Validate(ephemeralDisk, JobTypeService) if err == nil { t.Fatalf("expected an error") } @@ -1320,7 +1341,7 @@ func TestTask_Validate_LogConfig(t *testing.T) { SizeMB: 1, } - err := task.Validate(ephemeralDisk) + err := task.Validate(ephemeralDisk, JobTypeService) mErr := err.(*multierror.Error) if !strings.Contains(mErr.Errors[3].Error(), "log storage") { t.Fatalf("err: %s", err) @@ -1337,7 +1358,7 @@ func TestTask_Validate_Template(t *testing.T) { SizeMB: 1, } - err := task.Validate(ephemeralDisk) + err := task.Validate(ephemeralDisk, JobTypeService) if !strings.Contains(err.Error(), "Template 1 validation failed") { t.Fatalf("err: %s", err) } @@ -1350,7 +1371,7 @@ func TestTask_Validate_Template(t *testing.T) { } task.Templates = []*Template{good, good} - err = task.Validate(ephemeralDisk) + err = task.Validate(ephemeralDisk, JobTypeService) if !strings.Contains(err.Error(), "same destination as") { t.Fatalf("err: %s", err) } @@ -1363,7 +1384,7 @@ func TestTask_Validate_Template(t *testing.T) { }, } - err = task.Validate(ephemeralDisk) + err = task.Validate(ephemeralDisk, JobTypeService) if err == nil { t.Fatalf("expected error from Template.Validate") }