From d324a9864eddd3262b39679aa7a44cfe0ee40870 Mon Sep 17 00:00:00 2001 From: Preetha Appan <460133+preetapan@users.noreply.github.com> Date: Thu, 8 Aug 2019 14:45:24 -0500 Subject: [PATCH 1/5] Add validation for kind field if it is a consul connect proxy --- nomad/structs/structs.go | 32 ++++++++++++++++++++++++++++++-- nomad/structs/structs_test.go | 24 ++++++++++++------------ 2 files changed, 42 insertions(+), 14 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index ef2aa5a78..8755cbedf 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -4923,7 +4923,7 @@ func (tg *TaskGroup) Validate(j *Job) error { } } - if err := task.Validate(tg.EphemeralDisk, j.Type); err != nil { + if err := task.Validate(tg.EphemeralDisk, j.Type, tg.Services); err != nil { outer := fmt.Errorf("Task %s validation failed: %v", task.Name, err) mErr.Errors = append(mErr.Errors, outer) } @@ -5301,7 +5301,7 @@ func (t *Task) GoString() string { } // Validate is used to sanity check a task -func (t *Task) Validate(ephemeralDisk *EphemeralDisk, jobType string) error { +func (t *Task) Validate(ephemeralDisk *EphemeralDisk, jobType string, tgServices []*Service) error { var mErr multierror.Error if t.Name == "" { mErr.Errors = append(mErr.Errors, errors.New("Missing task name")) @@ -5410,6 +5410,34 @@ func (t *Task) Validate(ephemeralDisk *EphemeralDisk, jobType string) error { } } + // Validation for Kind field which is used for Consul Connect integration + // TODO better wording for all error messages + if strings.HasPrefix(t.Kind, "connect:") { + // This task is a Connect proxy so it should not have service stanzas + if len(t.Services) > 0 { + mErr.Errors = append(mErr.Errors, fmt.Errorf("Connect proxy task should not have service stanza in it")) + } + if t.Leader { + mErr.Errors = append(mErr.Errors, fmt.Errorf("Connect proxy task must not have leader set")) + } + + parts := strings.Split(t.Kind, ":") + serviceName := strings.Join(parts[1:], "") + if len(parts) > 2 { + mErr.Errors = append(mErr.Errors, fmt.Errorf("Proxy service name %q should not contain `:`", serviceName)) + } + // TODO Check if service exists at group level + found := false + for _, svc := range tgServices { + if svc.Name == serviceName { + found = true + break + } + } + if !found { + mErr.Errors = append(mErr.Errors, fmt.Errorf("Connect proxy service name not found in services from task group")) + } + } return mErr.ErrorOrNil() } diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 378844ed2..8c3b27c4f 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -942,7 +942,7 @@ func TestTaskGroup_Validate(t *testing.T) { func TestTask_Validate(t *testing.T) { task := &Task{} ephemeralDisk := DefaultEphemeralDisk() - err := task.Validate(ephemeralDisk, JobTypeBatch) + err := task.Validate(ephemeralDisk, JobTypeBatch, nil) mErr := err.(*multierror.Error) if !strings.Contains(mErr.Errors[0].Error(), "task name") { t.Fatalf("err: %s", err) @@ -955,7 +955,7 @@ func TestTask_Validate(t *testing.T) { } task = &Task{Name: "web/foo"} - err = task.Validate(ephemeralDisk, JobTypeBatch) + err = task.Validate(ephemeralDisk, JobTypeBatch, nil) mErr = err.(*multierror.Error) if !strings.Contains(mErr.Errors[0].Error(), "slashes") { t.Fatalf("err: %s", err) @@ -971,7 +971,7 @@ func TestTask_Validate(t *testing.T) { LogConfig: DefaultLogConfig(), } ephemeralDisk.SizeMB = 200 - err = task.Validate(ephemeralDisk, JobTypeBatch) + err = task.Validate(ephemeralDisk, JobTypeBatch, nil) if err != nil { t.Fatalf("err: %s", err) } @@ -985,7 +985,7 @@ func TestTask_Validate(t *testing.T) { LTarget: "${meta.rack}", }) - err = task.Validate(ephemeralDisk, JobTypeBatch) + err = task.Validate(ephemeralDisk, JobTypeBatch, nil) mErr = err.(*multierror.Error) if !strings.Contains(mErr.Errors[0].Error(), "task level: distinct_hosts") { t.Fatalf("err: %s", err) @@ -1067,7 +1067,7 @@ func TestTask_Validate_Services(t *testing.T) { }, } - err := task.Validate(ephemeralDisk, JobTypeService) + err := task.Validate(ephemeralDisk, JobTypeService, nil) if err == nil { t.Fatal("expected an error") } @@ -1088,7 +1088,7 @@ func TestTask_Validate_Services(t *testing.T) { t.Fatalf("err: %v", err) } - if err = task1.Validate(ephemeralDisk, JobTypeService); err != nil { + if err = task1.Validate(ephemeralDisk, JobTypeService, nil); err != nil { t.Fatalf("err : %v", err) } } @@ -1147,7 +1147,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, JobTypeService); err != nil { + if err := task.Validate(ephemeralDisk, JobTypeService, nil); err != nil { t.Fatalf("unexpected err: %v", err) } }) @@ -1200,7 +1200,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, JobTypeService) + err := task.Validate(ephemeralDisk, JobTypeService, nil) if err == nil { t.Fatalf("expected an error") } @@ -1521,7 +1521,7 @@ func TestTask_Validate_LogConfig(t *testing.T) { SizeMB: 1, } - err := task.Validate(ephemeralDisk, JobTypeService) + err := task.Validate(ephemeralDisk, JobTypeService, nil) mErr := err.(*multierror.Error) if !strings.Contains(mErr.Errors[3].Error(), "log storage") { t.Fatalf("err: %s", err) @@ -1538,7 +1538,7 @@ func TestTask_Validate_Template(t *testing.T) { SizeMB: 1, } - err := task.Validate(ephemeralDisk, JobTypeService) + err := task.Validate(ephemeralDisk, JobTypeService, nil) if !strings.Contains(err.Error(), "Template 1 validation failed") { t.Fatalf("err: %s", err) } @@ -1551,7 +1551,7 @@ func TestTask_Validate_Template(t *testing.T) { } task.Templates = []*Template{good, good} - err = task.Validate(ephemeralDisk, JobTypeService) + err = task.Validate(ephemeralDisk, JobTypeService, nil) if !strings.Contains(err.Error(), "same destination as") { t.Fatalf("err: %s", err) } @@ -1564,7 +1564,7 @@ func TestTask_Validate_Template(t *testing.T) { }, } - err = task.Validate(ephemeralDisk, JobTypeService) + err = task.Validate(ephemeralDisk, JobTypeService, nil) if err == nil { t.Fatalf("expected error from Template.Validate") } From 35506c516d9ff207203bcf99c307f2f9983ca51b Mon Sep 17 00:00:00 2001 From: Preetha Appan <460133+preetapan@users.noreply.github.com> Date: Fri, 9 Aug 2019 16:40:51 -0500 Subject: [PATCH 2/5] Improve validation logic and add table driven tests --- command/agent/job_endpoint.go | 2 +- nomad/structs/structs.go | 53 ++++++++++------ nomad/structs/structs_test.go | 111 ++++++++++++++++++++++++++++++++++ 3 files changed, 148 insertions(+), 18 deletions(-) diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 1c2ab1348..f1204074b 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -805,7 +805,7 @@ func ApiTaskToStructsTask(apiTask *api.Task, structsTask *structs.Task) { structsTask.KillTimeout = *apiTask.KillTimeout structsTask.ShutdownDelay = apiTask.ShutdownDelay structsTask.KillSignal = apiTask.KillSignal - structsTask.Kind = apiTask.Kind + structsTask.Kind = structs.Kind(apiTask.Kind) structsTask.Constraints = ApiConstraintsToStructs(apiTask.Constraints) structsTask.Affinities = ApiAffinitiesToStructs(apiTask.Affinities) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 8755cbedf..4fc0116ed 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -5412,7 +5412,8 @@ func (t *Task) Validate(ephemeralDisk *EphemeralDisk, jobType string, tgServices // Validation for Kind field which is used for Consul Connect integration // TODO better wording for all error messages - if strings.HasPrefix(t.Kind, "connect:") { + taskKind := t.Kind + if taskKind.IsConnect() { // This task is a Connect proxy so it should not have service stanzas if len(t.Services) > 0 { mErr.Errors = append(mErr.Errors, fmt.Errorf("Connect proxy task should not have service stanza in it")) @@ -5420,22 +5421,9 @@ func (t *Task) Validate(ephemeralDisk *EphemeralDisk, jobType string, tgServices if t.Leader { mErr.Errors = append(mErr.Errors, fmt.Errorf("Connect proxy task must not have leader set")) } - - parts := strings.Split(t.Kind, ":") - serviceName := strings.Join(parts[1:], "") - if len(parts) > 2 { - mErr.Errors = append(mErr.Errors, fmt.Errorf("Proxy service name %q should not contain `:`", serviceName)) - } - // TODO Check if service exists at group level - found := false - for _, svc := range tgServices { - if svc.Name == serviceName { - found = true - break - } - } - if !found { - mErr.Errors = append(mErr.Errors, fmt.Errorf("Connect proxy service name not found in services from task group")) + serviceErr := taskKind.ValidateService(tgServices) + if serviceErr != nil { + mErr.Errors = append(mErr.Errors, serviceErr) } } return mErr.ErrorOrNil() @@ -5581,6 +5569,37 @@ func (t *Task) Warnings() error { return mErr.ErrorOrNil() } +type Kind string + +const connect_prefix = "connect:" + +func (k Kind) IsConnect() bool { + return strings.HasPrefix(string(k), connect_prefix) && len(k) > len(connect_prefix) +} + +func (k Kind) ValidateService(tgServices []*Service) error { + var mErr multierror.Error + parts := strings.Split(string(k), ":") + serviceName := strings.Join(parts[1:], "") + if len(parts) > 2 { + mErr.Errors = append(mErr.Errors, fmt.Errorf("Connect proxy service kind %q should not contain `:`", k)) + } + + found := false + for _, svc := range tgServices { + if svc.Name == serviceName && svc.Connect != nil && svc.Connect.SidecarService != nil { + found = true + break + } + } + + if !found { + mErr.Errors = append(mErr.Errors, fmt.Errorf("Connect proxy service name not found in services from task group")) + } + + return mErr.ErrorOrNil() +} + const ( // TemplateChangeModeNoop marks that no action should be taken if the // template is re-rendered diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 8c3b27c4f..0d528aa64 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -1513,6 +1513,117 @@ func TestTask_Validate_Service_Check_CheckRestart(t *testing.T) { assert.Nil(t, validCheckRestart.Validate()) } +func TestTask_Validate_ConnectKind(t *testing.T) { + ephemeralDisk := DefaultEphemeralDisk() + getTask := func(kind Kind, leader bool) *Task { + task := &Task{ + Name: "web", + Driver: "docker", + Resources: DefaultResources(), + LogConfig: DefaultLogConfig(), + Kind: kind, + Leader: leader, + } + task.Resources.Networks = []*NetworkResource{ + { + MBits: 10, + DynamicPorts: []Port{ + { + Label: "http", + Value: 80, + }, + }, + }, + } + return task + } + + cases := []struct { + Desc string + Kind Kind + Leader bool + Service *Service + TgService []*Service + ErrContains string + }{ + { + Desc: "Not connect", + Kind: "test", + }, + { + Desc: "Invalid because of service in task definition", + Kind: "connect:redis", + Service: &Service{ + Name: "redis", + }, + ErrContains: "Connect proxy task should not have service stanza in it", + }, + { + Desc: "Leader should not be set", + Kind: "connect:redis", + Leader: true, + Service: &Service{ + Name: "redis", + }, + ErrContains: "Connect proxy task must not have leader set", + }, + { + Desc: "Service name invalid", + Kind: "connect:redis:test", + Service: &Service{ + Name: "redis", + }, + ErrContains: "Connect proxy service kind \"connect:redis:test\" should not contain `:`", + }, + { + Desc: "Service name not found in group", + Kind: "connect:redis", + ErrContains: "Connect proxy service name not found in services from task group", + }, + { + Desc: "Connect stanza not configured in group", + Kind: "connect:redis", + TgService: []*Service{{ + Name: "redis", + }}, + ErrContains: "Connect proxy service name not found in services from task group", + }, + { + Desc: "Valid connect proxy kind", + Kind: "connect:redis", + TgService: []*Service{{ + Name: "redis", + Connect: &ConsulConnect{ + SidecarService: &ConsulSidecarService{ + Port: "db", + }, + }, + }}, + }, + } + + for _, tc := range cases { + tc := tc + task := getTask(tc.Kind, tc.Leader) + if tc.Service != nil { + task.Services = []*Service{tc.Service} + } + t.Run(tc.Desc, func(t *testing.T) { + err := task.Validate(ephemeralDisk, "service", tc.TgService) + if err == nil && tc.ErrContains == "" { + // Ok! + return + } + if err == nil { + t.Fatalf("no error returned. expected: %s", tc.ErrContains) + } + if !strings.Contains(err.Error(), tc.ErrContains) { + t.Fatalf("expected %q but found: %v", tc.ErrContains, err) + } + }) + } + +} func TestTask_Validate_LogConfig(t *testing.T) { task := &Task{ LogConfig: DefaultLogConfig(), From 219dc0554159fa33a8542c99ea3dfef3d9bee471 Mon Sep 17 00:00:00 2001 From: Preetha Appan <460133+preetapan@users.noreply.github.com> Date: Mon, 12 Aug 2019 11:41:14 -0500 Subject: [PATCH 3/5] Fix type for kind --- nomad/structs/structs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 4fc0116ed..f84567965 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -5204,7 +5204,7 @@ type Task struct { // Used internally to manage tasks according to their Kind. Initial use case // is for Consul Connect - Kind string + Kind Kind } func (t *Task) Copy() *Task { From 76c8a11b311e686d9858fcbfd482a0d152e41185 Mon Sep 17 00:00:00 2001 From: Preetha <460133+preetapan@users.noreply.github.com> Date: Mon, 12 Aug 2019 17:03:30 -0500 Subject: [PATCH 4/5] Apply suggestions from code review Co-Authored-By: Michael Schurter --- nomad/structs/structs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index f84567965..9b9ed2be0 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -5416,7 +5416,7 @@ func (t *Task) Validate(ephemeralDisk *EphemeralDisk, jobType string, tgServices if taskKind.IsConnect() { // This task is a Connect proxy so it should not have service stanzas if len(t.Services) > 0 { - mErr.Errors = append(mErr.Errors, fmt.Errorf("Connect proxy task should not have service stanza in it")) + mErr.Errors = append(mErr.Errors, fmt.Errorf("Connect proxy task must not have a service stanza")) } if t.Leader { mErr.Errors = append(mErr.Errors, fmt.Errorf("Connect proxy task must not have leader set")) From 72e45dd01eb666b49a1b8a4114881e3838983fff Mon Sep 17 00:00:00 2001 From: Preetha Appan <460133+preetapan@users.noreply.github.com> Date: Mon, 12 Aug 2019 17:41:40 -0500 Subject: [PATCH 5/5] More code review feedback --- command/agent/job_endpoint.go | 2 +- nomad/structs/structs.go | 24 +++++++++++++----------- nomad/structs/structs_test.go | 20 ++++++++++---------- 3 files changed, 24 insertions(+), 22 deletions(-) diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index f1204074b..12f85237a 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -805,7 +805,7 @@ func ApiTaskToStructsTask(apiTask *api.Task, structsTask *structs.Task) { structsTask.KillTimeout = *apiTask.KillTimeout structsTask.ShutdownDelay = apiTask.ShutdownDelay structsTask.KillSignal = apiTask.KillSignal - structsTask.Kind = structs.Kind(apiTask.Kind) + structsTask.Kind = structs.TaskKind(apiTask.Kind) structsTask.Constraints = ApiConstraintsToStructs(apiTask.Constraints) structsTask.Affinities = ApiAffinitiesToStructs(apiTask.Affinities) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 9b9ed2be0..003122c5f 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -5202,9 +5202,9 @@ type Task struct { // specification and defaults to SIGINT KillSignal string - // Used internally to manage tasks according to their Kind. Initial use case + // Used internally to manage tasks according to their TaskKind. Initial use case // is for Consul Connect - Kind Kind + Kind TaskKind } func (t *Task) Copy() *Task { @@ -5410,8 +5410,7 @@ func (t *Task) Validate(ephemeralDisk *EphemeralDisk, jobType string, tgServices } } - // Validation for Kind field which is used for Consul Connect integration - // TODO better wording for all error messages + // Validation for TaskKind field which is used for Consul Connect integration taskKind := t.Kind if taskKind.IsConnect() { // This task is a Connect proxy so it should not have service stanzas @@ -5421,7 +5420,7 @@ func (t *Task) Validate(ephemeralDisk *EphemeralDisk, jobType string, tgServices if t.Leader { mErr.Errors = append(mErr.Errors, fmt.Errorf("Connect proxy task must not have leader set")) } - serviceErr := taskKind.ValidateService(tgServices) + serviceErr := taskKind.validateProxyService(tgServices) if serviceErr != nil { mErr.Errors = append(mErr.Errors, serviceErr) } @@ -5569,20 +5568,23 @@ func (t *Task) Warnings() error { return mErr.ErrorOrNil() } -type Kind string +type TaskKind string -const connect_prefix = "connect:" +const connectPrefix = "connect-proxy:" -func (k Kind) IsConnect() bool { - return strings.HasPrefix(string(k), connect_prefix) && len(k) > len(connect_prefix) +func (k TaskKind) IsConnect() bool { + return strings.HasPrefix(string(k), connectPrefix) && len(k) > len(connectPrefix) } -func (k Kind) ValidateService(tgServices []*Service) error { +// validateProxyService checks that the service that is being +// proxied by this task exists in the task group and contains +// valid Connect config. +func (k TaskKind) validateProxyService(tgServices []*Service) error { var mErr multierror.Error parts := strings.Split(string(k), ":") serviceName := strings.Join(parts[1:], "") if len(parts) > 2 { - mErr.Errors = append(mErr.Errors, fmt.Errorf("Connect proxy service kind %q should not contain `:`", k)) + mErr.Errors = append(mErr.Errors, fmt.Errorf("Connect proxy service kind %q must not contain `:`", k)) } found := false diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 0d528aa64..e9679099f 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -1515,7 +1515,7 @@ func TestTask_Validate_Service_Check_CheckRestart(t *testing.T) { func TestTask_Validate_ConnectKind(t *testing.T) { ephemeralDisk := DefaultEphemeralDisk() - getTask := func(kind Kind, leader bool) *Task { + getTask := func(kind TaskKind, leader bool) *Task { task := &Task{ Name: "web", Driver: "docker", @@ -1540,7 +1540,7 @@ func TestTask_Validate_ConnectKind(t *testing.T) { cases := []struct { Desc string - Kind Kind + Kind TaskKind Leader bool Service *Service TgService []*Service @@ -1552,15 +1552,15 @@ func TestTask_Validate_ConnectKind(t *testing.T) { }, { Desc: "Invalid because of service in task definition", - Kind: "connect:redis", + Kind: "connect-proxy:redis", Service: &Service{ Name: "redis", }, - ErrContains: "Connect proxy task should not have service stanza in it", + ErrContains: "Connect proxy task must not have a service stanza", }, { Desc: "Leader should not be set", - Kind: "connect:redis", + Kind: "connect-proxy:redis", Leader: true, Service: &Service{ Name: "redis", @@ -1569,20 +1569,20 @@ func TestTask_Validate_ConnectKind(t *testing.T) { }, { Desc: "Service name invalid", - Kind: "connect:redis:test", + Kind: "connect-proxy:redis:test", Service: &Service{ Name: "redis", }, - ErrContains: "Connect proxy service kind \"connect:redis:test\" should not contain `:`", + ErrContains: "Connect proxy service kind \"connect-proxy:redis:test\" must not contain `:`", }, { Desc: "Service name not found in group", - Kind: "connect:redis", + Kind: "connect-proxy:redis", ErrContains: "Connect proxy service name not found in services from task group", }, { Desc: "Connect stanza not configured in group", - Kind: "connect:redis", + Kind: "connect-proxy:redis", TgService: []*Service{{ Name: "redis", }}, @@ -1590,7 +1590,7 @@ func TestTask_Validate_ConnectKind(t *testing.T) { }, { Desc: "Valid connect proxy kind", - Kind: "connect:redis", + Kind: "connect-proxy:redis", TgService: []*Service{{ Name: "redis", Connect: &ConsulConnect{