From 13982879b6f76ded20e270d2d8e2b20bbdc1494c Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 7 Jul 2017 16:17:05 -0700 Subject: [PATCH 1/3] Prevent port conflicts Validate that no two tasks in the same task group can reserve the same static port. --- nomad/structs/structs.go | 20 +++++++++++++++++++- nomad/structs/structs_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 2a92c4b44586..42e3bf0d160d 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -2527,8 +2527,10 @@ func (tg *TaskGroup) Validate(j *Job) error { } } - // Check for duplicate tasks and that there is only leader task if any + // Check for duplicate tasks, that there is only leader task if any, + // and no duplicated static ports tasks := make(map[string]int) + staticPorts := make(map[int]string) leaderTasks := 0 for idx, task := range tg.Tasks { if task.Name == "" { @@ -2542,6 +2544,22 @@ func (tg *TaskGroup) Validate(j *Job) error { if task.Leader { leaderTasks++ } + + if task.Resources == nil { + continue + } + + for _, net := range task.Resources.Networks { + for _, port := range net.ReservedPorts { + if other, ok := staticPorts[port.Value]; ok { + err := fmt.Errorf("Task %q and %q both reserve static port %d", + other, task.Name, port.Value) + mErr.Errors = append(mErr.Errors, err) + } else { + staticPorts[port.Value] = task.Name + } + } + } } if leaderTasks > 1 { diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 00516b9d5f7c..d2f11c16d032 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -806,6 +806,36 @@ func TestTaskGroup_Validate(t *testing.T) { t.Fatalf("err: %s", err) } + tg = &TaskGroup{ + Tasks: []*Task{ + &Task{ + Name: "task-a", + Resources: &Resources{ + Networks: []*NetworkResource{ + &NetworkResource{ + ReservedPorts: []Port{{Label: "foo", Value: 123}}, + }, + }, + }, + }, + &Task{ + Name: "task-b", + Resources: &Resources{ + Networks: []*NetworkResource{ + &NetworkResource{ + ReservedPorts: []Port{{Label: "foo", Value: 123}}, + }, + }, + }, + }, + }, + } + err = tg.Validate(&Job{}) + expected := `Task "task-a" and "task-b" both reserve static port 123` + if !strings.Contains(err.Error(), expected) { + t.Errorf("expected %s but found: %v", expected, err) + } + tg = &TaskGroup{ Name: "web", Count: 1, From d2472033286907fbc2a6de93cf6e3c85e44261b2 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Fri, 7 Jul 2017 16:58:20 -0700 Subject: [PATCH 2/3] Handle same task reserving ports twice --- nomad/structs/structs.go | 5 ++--- nomad/structs/structs_test.go | 25 ++++++++++++++++++++++++- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 42e3bf0d160d..50eae105a93a 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -2552,11 +2552,10 @@ func (tg *TaskGroup) Validate(j *Job) error { for _, net := range task.Resources.Networks { for _, port := range net.ReservedPorts { if other, ok := staticPorts[port.Value]; ok { - err := fmt.Errorf("Task %q and %q both reserve static port %d", - other, task.Name, port.Value) + err := fmt.Errorf("Static port %d already reserved by %s", port.Value, other) mErr.Errors = append(mErr.Errors, err) } else { - staticPorts[port.Value] = task.Name + staticPorts[port.Value] = fmt.Sprintf("%s:%s", task.Name, port.Label) } } } diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index d2f11c16d032..aae87936e8c6 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -831,7 +831,30 @@ func TestTaskGroup_Validate(t *testing.T) { }, } err = tg.Validate(&Job{}) - expected := `Task "task-a" and "task-b" both reserve static port 123` + expected := `Static port 123 already reserved by task-a:foo` + if !strings.Contains(err.Error(), expected) { + t.Errorf("expected %s but found: %v", expected, err) + } + + tg = &TaskGroup{ + Tasks: []*Task{ + &Task{ + Name: "task-a", + Resources: &Resources{ + Networks: []*NetworkResource{ + &NetworkResource{ + ReservedPorts: []Port{ + {Label: "foo", Value: 123}, + {Label: "bar", Value: 123}, + }, + }, + }, + }, + }, + }, + } + err = tg.Validate(&Job{}) + expected = `Static port 123 already reserved by task-a:foo` if !strings.Contains(err.Error(), expected) { t.Errorf("expected %s but found: %v", expected, err) } From c26e813c3120b989a439dec2a1a06991d57d1969 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Fri, 7 Jul 2017 16:59:07 -0700 Subject: [PATCH 3/3] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c5f8130c4cc..2ce2c9530fa5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ BUG FIXES: * driver/exec: Properly set file/dir ownership in chroots [GH-2552] * driver/docker: Fix panic in Docker driver on Windows [GH-2614] * driver/rkt: Fix env var interpolation [GH-2777] + * jobspec/validation: Prevent static port conflicts [GH-2807] * server: Reject non-TLS clients when TLS enabled [GH-2525] * server: Fix a panic in plan evaluation with partial failures and all_at_once set [GH-2544]