From bbedeb670d5d127db766b54d7b53dc71d78285bf Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Wed, 15 Jan 2020 09:29:47 -0600 Subject: [PATCH] nomad,client: apply more comment/style PR tweaks --- .../allocrunner/taskrunner/envoybootstrap_hook.go | 5 +++++ .../taskrunner/envoybootstrap_hook_test.go | 7 +++++++ client/allocrunner/taskrunner/sids_hook_test.go | 3 ++- client/allocrunner/taskrunner/task_runner_hooks.go | 13 ++++++------- client/allocrunner/taskrunner/task_runner_test.go | 5 +++-- nomad/job_endpoint.go | 11 ++++------- nomad/leader.go | 2 +- 7 files changed, 28 insertions(+), 18 deletions(-) diff --git a/client/allocrunner/taskrunner/envoybootstrap_hook.go b/client/allocrunner/taskrunner/envoybootstrap_hook.go index 7382c24b1..d945a2912 100644 --- a/client/allocrunner/taskrunner/envoybootstrap_hook.go +++ b/client/allocrunner/taskrunner/envoybootstrap_hook.go @@ -230,6 +230,9 @@ func (h *envoyBootstrapHook) execute(cmd *exec.Cmd) (string, error) { return stdout.String(), nil } +// envoyBootstrapArgs is used to accumulate CLI arguments that will be passed +// along to the exec invocation of consul which will then generate the bootstrap +// configuration file for envoy. type envoyBootstrapArgs struct { sidecarFor string grpcAddr string @@ -238,6 +241,8 @@ type envoyBootstrapArgs struct { siToken string } +// args returns the CLI arguments consul needs in the correct order, with the +// -token argument present or not present depending on whether it is set. func (e envoyBootstrapArgs) args() []string { arguments := []string{ "connect", diff --git a/client/allocrunner/taskrunner/envoybootstrap_hook_test.go b/client/allocrunner/taskrunner/envoybootstrap_hook_test.go index f4be8d80a..276a9bd1b 100644 --- a/client/allocrunner/taskrunner/envoybootstrap_hook_test.go +++ b/client/allocrunner/taskrunner/envoybootstrap_hook_test.go @@ -1,3 +1,7 @@ +// +build !windows +// todo(shoenig): Once Connect is supported on Windows, we'll need to make this +// set of tests work there too. + package taskrunner import ( @@ -40,6 +44,9 @@ func writeTmp(t *testing.T, s string, fm os.FileMode) string { func TestEnvoyBootstrapHook_maybeLoadSIToken(t *testing.T) { t.Parallel() + // This test fails when running as root because the test case for checking + // the error condition when the file is unreadable fails (root can read the + // file even though the permissions are set to 0200). if unix.Geteuid() == 0 { t.Skip("test only works as non-root") } diff --git a/client/allocrunner/taskrunner/sids_hook_test.go b/client/allocrunner/taskrunner/sids_hook_test.go index 7a2035d30..415d1f980 100644 --- a/client/allocrunner/taskrunner/sids_hook_test.go +++ b/client/allocrunner/taskrunner/sids_hook_test.go @@ -11,6 +11,7 @@ import ( "github.com/hashicorp/nomad/client/consul" "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/testlog" + "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/nomad/structs" "github.com/stretchr/testify/require" ) @@ -51,7 +52,7 @@ func TestSIDSHook_recoverToken(t *testing.T) { logger: testlog.HCLogger(t), }) - expected := "12345678-1234-1234-1234-1234567890" + expected := uuid.Generate() err := h.writeToken(secrets, expected) r.NoError(err) diff --git a/client/allocrunner/taskrunner/task_runner_hooks.go b/client/allocrunner/taskrunner/task_runner_hooks.go index a817eb0bd..2da0c9fe5 100644 --- a/client/allocrunner/taskrunner/task_runner_hooks.go +++ b/client/allocrunner/taskrunner/task_runner_hooks.go @@ -116,15 +116,14 @@ func (tr *TaskRunner) initHooks() { lifecycle: tr, logger: hookLogger, })) + // envoy bootstrap must execute after sidsHook maybe sets SI token + tr.runnerHooks = append(tr.runnerHooks, newEnvoyBootstrapHook(&envoyBootstrapHookConfig{ + alloc: alloc, + consulHTTPAddr: tr.clientConfig.ConsulConfig.Addr, + logger: hookLogger, + })) } - // envoy bootstrap must execute after sidsHook maybe sets SI token - tr.runnerHooks = append(tr.runnerHooks, newEnvoyBootstrapHook(&envoyBootstrapHookConfig{ - alloc: alloc, - consulHTTPAddr: tr.clientConfig.ConsulConfig.Addr, - logger: hookLogger, - })) - // If there are any script checks, add the hook scriptCheckHook := newScriptCheckHook(scriptCheckHookConfig{ alloc: tr.Alloc(), diff --git a/client/allocrunner/taskrunner/task_runner_test.go b/client/allocrunner/taskrunner/task_runner_test.go index b36172302..7b59d626a 100644 --- a/client/allocrunner/taskrunner/task_runner_test.go +++ b/client/allocrunner/taskrunner/task_runner_test.go @@ -28,6 +28,7 @@ import ( mockdriver "github.com/hashicorp/nomad/drivers/mock" "github.com/hashicorp/nomad/drivers/rawexec" "github.com/hashicorp/nomad/helper/testlog" + "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/plugins/device" @@ -1127,7 +1128,7 @@ func TestTaskRunner_BlockForSIDSToken(t *testing.T) { defer cleanup() // control when we get a Consul SI token - token := "12345678-1234-1234-1234-1234567890" + token := uuid.Generate() waitCh := make(chan struct{}) deriveFn := func(*structs.Allocation, []string) (map[string]string, error) { <-waitCh @@ -1191,7 +1192,7 @@ func TestTaskRunner_DeriveSIToken_Retry(t *testing.T) { defer cleanup() // control when we get a Consul SI token - token := "12345678-1234-1234-1234-1234567890" + token := uuid.Generate() deriveCount := 0 deriveFn := func(*structs.Allocation, []string) (map[string]string, error) { if deriveCount > 0 { diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index e7f74b27b..d4c77030d 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -234,13 +234,10 @@ func (j *Job) Register(args *structs.JobRegisterRequest, reply *structs.JobRegis } // Enforce that the operator has necessary Consul ACL permissions - connectTasks := args.Job.ConnectTasks() - if len(connectTasks) > 0 { - for _, tg := range connectTasks { - for _, task := range tg { - if err := checkOperatorToken(task); err != nil { - return err - } + for _, tg := range args.Job.ConnectTasks() { + for _, task := range tg { + if err := checkOperatorToken(task); err != nil { + return err } } } diff --git a/nomad/leader.go b/nomad/leader.go index 1898c03bc..a202bb33e 100644 --- a/nomad/leader.go +++ b/nomad/leader.go @@ -441,7 +441,7 @@ func (s *Server) revokeSITokenAccessorsOnRestore() error { if len(toRevoke) > 0 { ctx := context.Background() - _ = s.consulACLs.RevokeTokens(ctx, toRevoke, true) + s.consulACLs.RevokeTokens(ctx, toRevoke, true) } return nil