From bb45a7a776a45841317919946d0492e31e49b7d1 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Mon, 28 Oct 2019 10:41:51 -0400 Subject: [PATCH 1/3] add tests for consul connect validation --- nomad/job_endpoint_test.go | 92 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/nomad/job_endpoint_test.go b/nomad/job_endpoint_test.go index 7ba97d940..6ceeb35c8 100644 --- a/nomad/job_endpoint_test.go +++ b/nomad/job_endpoint_test.go @@ -1,6 +1,7 @@ package nomad import ( + "errors" "fmt" "reflect" "strings" @@ -4321,6 +4322,97 @@ func TestJobEndpoint_ImplicitConstraints_Vault(t *testing.T) { } } +func TestJobEndpoint_ValidateJob_ConsulConnect(t *testing.T) { + t.Parallel() + + s1 := TestServer(t, nil) + defer s1.Shutdown() + codec := rpcClient(t, s1) + testutil.WaitForLeader(t, s1.RPC) + + validateJob := func(j *structs.Job) error { + req := &structs.JobRegisterRequest{ + Job: j, + WriteRequest: structs.WriteRequest{ + Region: "global", + Namespace: j.Namespace, + }, + } + var resp structs.JobValidateResponse + if err := msgpackrpc.CallWithCodec(codec, "Job.Validate", req, &resp); err != nil { + return err + } + + if resp.Error != "" { + return errors.New(resp.Error) + } + + if len(resp.ValidationErrors) != 0 { + return errors.New(strings.Join(resp.ValidationErrors, ",")) + } + + if resp.Warnings != "" { + return errors.New(resp.Warnings) + } + + return nil + } + + tgServices := []*structs.Service{ + { + Name: "count-api", + PortLabel: "9001", + Connect: &structs.ConsulConnect{ + SidecarService: &structs.ConsulSidecarService{}, + }, + }, + } + + t.Run("plain job", func(t *testing.T) { + j := mock.Job() + require.NoError(t, validateJob(j)) + }) + t.Run("valid consul connect", func(t *testing.T) { + j := mock.Job() + + tg := j.TaskGroups[0] + tg.Services = tgServices + tg.Networks = structs.Networks{ + {Mode: "bridge"}, + } + + err := validateJob(j) + require.NoError(t, err) + }) + + t.Run("consul connect but missing network", func(t *testing.T) { + j := mock.Job() + + tg := j.TaskGroups[0] + tg.Services = tgServices + + err := validateJob(j) + require.Error(t, err) + require.Contains(t, err.Error(), `Consul Connect sidecars require exactly 1 network`) + }) + + t.Run("consul connect but non bridge network", func(t *testing.T) { + j := mock.Job() + + tg := j.TaskGroups[0] + tg.Services = tgServices + + tg.Networks = structs.Networks{ + {Mode: "host"}, + } + + err := validateJob(j) + require.Error(t, err) + require.Contains(t, err.Error(), `Consul Connect sidecar requires bridge network, found "host" in group "web"`) + }) + +} + func TestJobEndpoint_ImplicitConstraints_Signals(t *testing.T) { t.Parallel() s1 := TestServer(t, func(c *Config) { From abb930249abf7d621ec72bba1fcc09725b3b186c Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Mon, 28 Oct 2019 10:46:51 -0400 Subject: [PATCH 2/3] consul connect: do basic validation before mutating job `groupConnectHook` assumes that Networks is a non-empty slice, but TG hasn't been validated yet and validation may depend on mutation results. As such, we do basic check here before dereferencing network slice elements. --- nomad/job_endpoint_hook_connect.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/nomad/job_endpoint_hook_connect.go b/nomad/job_endpoint_hook_connect.go index f4d619dc1..5b7388f83 100644 --- a/nomad/job_endpoint_hook_connect.go +++ b/nomad/job_endpoint_hook_connect.go @@ -50,6 +50,15 @@ func (jobConnectHook) Name() string { func (jobConnectHook) Mutate(job *structs.Job) (_ *structs.Job, warnings []error, err error) { for _, g := range job.TaskGroups { + // TG isn't validated yet, but validation + // may depend on mutation results. + // Do basic validation here and skip mutation, + // so Validate can return a meaningful error + // messages + if len(g.Networks) == 0 { + continue + } + if err := groupConnectHook(g); err != nil { return nil, nil, err } From c5d8d667879eb51ee122d00b85bc7732fbcada5e Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Mon, 28 Oct 2019 10:49:08 -0400 Subject: [PATCH 3/3] Fix admissionValidators `admissionValidators` doesn't aggregate errors correctly, as it aggregates errors in `errs` reference yet it always returns the nil `err`. Here, we avoid shadowing `err`, and move variable declarations to where they are used. --- nomad/job_endpoint_hooks.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/nomad/job_endpoint_hooks.go b/nomad/job_endpoint_hooks.go index 5153e1b27..2168c977f 100644 --- a/nomad/job_endpoint_hooks.go +++ b/nomad/job_endpoint_hooks.go @@ -53,21 +53,23 @@ func (j *Job) admissionMutators(job *structs.Job) (_ *structs.Job, warnings []er // admissionValidators returns a slice of validation warnings and a multierror // of validation failures. -func (j *Job) admissionValidators(origJob *structs.Job) (warnings []error, err error) { +func (j *Job) admissionValidators(origJob *structs.Job) ([]error, error) { // ensure job is not mutated job := origJob.Copy() - var w []error - errs := new(multierror.Error) + var warnings []error + var errs error + for _, validator := range j.validators { - w, err = validator.Validate(job) + w, err := validator.Validate(job) j.logger.Trace("job validate results", "validator", validator.Name(), "warnings", w, "error", err) if err != nil { - multierror.Append(errs, err) + errs = multierror.Append(errs, err) } warnings = append(warnings, w...) } - return warnings, err + + return warnings, errs }