From f0acf72ae7b97f346397b6d34cbf04ebc28e39b7 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Fri, 10 Nov 2023 13:42:30 -0500 Subject: [PATCH] client: fix Consul token retrievel for templates (#19058) The template hook must use the Consul token for the cluster defined in the task-level `consul` block or, if `nil, in the group-level `consul` block. The Consul tokens are generated by the allocrunner consul hook, but during the transition period we must fallback to the Nomad agent token if workload identities are not being used. So an empty token returned from `GetConsulTokens()` is not enough to determine if we should use the legacy flow (either this is an old task or the cluster is not configured for Consul WI), or if there is a misconfiguration (task or group is `consul` block is using a cluster that doesn't have an `identity` set). In order to distinguish between the two scenarios we must iterate over the task identities looking for one suitable for the Consul cluster being used. --- .../allocrunner/taskrunner/template_hook.go | 37 +++-- .../taskrunner/template_hook_test.go | 129 ++++++++++-------- 2 files changed, 97 insertions(+), 69 deletions(-) diff --git a/client/allocrunner/taskrunner/template_hook.go b/client/allocrunner/taskrunner/template_hook.go index 2bcaed9a3..3824dd02a 100644 --- a/client/allocrunner/taskrunner/template_hook.go +++ b/client/allocrunner/taskrunner/template_hook.go @@ -8,7 +8,6 @@ import ( "fmt" "sync" - consulapi "github.com/hashicorp/consul/api" log "github.com/hashicorp/go-hclog" "github.com/hashicorp/nomad/client/allocrunner/interfaces" ti "github.com/hashicorp/nomad/client/allocrunner/taskrunner/interfaces" @@ -130,29 +129,43 @@ func (h *templateHook) Prestart(ctx context.Context, req *interfaces.TaskPrestar h.vaultToken = req.VaultToken h.nomadToken = req.NomadToken - // Set the consul token if the task uses WI + // Set the consul token if the task uses WI. + tg := h.config.alloc.Job.LookupTaskGroup(h.config.alloc.TaskGroup) + consulBlock := tg.Consul if req.Task.Consul != nil { + consulBlock = req.Task.Consul + } + consulWIDName := consulBlock.IdentityName() + + // Check if task has an identity for Consul and assume WI flow if it does. + // COMPAT simplify this logic and assume WI flow in 1.9+ + hasConsulIdentity := false + for _, wid := range req.Task.Identities { + if wid.Name == consulWIDName { + hasConsulIdentity = true + break + } + } + if hasConsulIdentity { + consulCluster := req.Task.GetConsulClusterName(tg) consulTokens := h.config.hookResources.GetConsulTokens() + clusterTokens := consulTokens[consulCluster] - var found bool - tg := h.config.alloc.Job.LookupTaskGroup(h.config.alloc.TaskGroup) - cluster := req.Task.GetConsulClusterName(tg) - - if _, found = consulTokens[cluster]; !found { + if clusterTokens == nil { return fmt.Errorf( "consul tokens for cluster %s requested by task %s not found", - cluster, req.Task.Name, + consulCluster, req.Task.Name, ) } - var consulToken *consulapi.ACLToken - consulToken, found = consulTokens[cluster][req.Task.Consul.IdentityName()] - if !found { + consulToken := clusterTokens[consulWIDName] + if consulToken == nil { return fmt.Errorf( "consul tokens for cluster %s and identity %s requested by task %s not found", - cluster, req.Task.Consul.IdentityName(), req.Task.Name, + consulCluster, consulWIDName, req.Task.Name, ) } + h.consulToken = consulToken.SecretID } diff --git a/client/allocrunner/taskrunner/template_hook_test.go b/client/allocrunner/taskrunner/template_hook_test.go index 73bedafc7..2b09513eb 100644 --- a/client/allocrunner/taskrunner/template_hook_test.go +++ b/client/allocrunner/taskrunner/template_hook_test.go @@ -28,95 +28,110 @@ func Test_templateHook_Prestart_ConsulWI(t *testing.T) { ci.Parallel(t) logger := testlog.HCLogger(t) - // mock some consul tokens - hr := cstructs.NewAllocHookResources() - hr.SetConsulTokens( + // Create some alloc hook resources, one with tokens and an empty one. + defaultToken := uuid.Generate() + hrTokens := cstructs.NewAllocHookResources() + hrTokens.SetConsulTokens( map[string]map[string]*consulapi.ACLToken{ structs.ConsulDefaultCluster: { fmt.Sprintf("consul_%s", structs.ConsulDefaultCluster): &consulapi.ACLToken{ - SecretID: uuid.Generate()}, - }, - "test": { - "consul_test": &consulapi.ACLToken{SecretID: uuid.Generate()}, + SecretID: defaultToken, + }, }, }, ) - - a := mock.Alloc() - clientConfig := &config.Config{Region: "global"} - envBuilder := taskenv.NewBuilder(mock.Node(), a, a.Job.TaskGroups[0].Tasks[0], clientConfig.Region) - taskHooks := trtesting.NewMockTaskHooks() - - conf := &templateHookConfig{ - alloc: a, - logger: logger, - lifecycle: taskHooks, - events: &trtesting.MockEmitter{}, - clientConfig: clientConfig, - envBuilder: envBuilder, - hookResources: hr, - } + hrEmpty := cstructs.NewAllocHookResources() tests := []struct { - name string - req *interfaces.TaskPrestartRequest - wantErrMsg string - consulToken *consulapi.ACLToken + name string + taskConsul *structs.Consul + groupConsul *structs.Consul + hr *cstructs.AllocHookResources + wantErrMsg string + wantConsulToken string + legacyFlow bool }{ { - name: "task with no Consul WI", - req: &interfaces.TaskPrestartRequest{ - Task: &structs.Task{}, - TaskDir: &allocdir.TaskDir{Dir: "foo"}, - }, + // COMPAT remove in 1.9+ + name: "legecy flow", + hr: hrEmpty, + legacyFlow: true, + wantConsulToken: "", }, { - name: "task with Consul WI but no corresponding identity", - req: &interfaces.TaskPrestartRequest{ - Task: &structs.Task{ - Name: "foo", - Consul: &structs.Consul{Cluster: "bar"}, - }, - TaskDir: &allocdir.TaskDir{Dir: "foo"}, - }, - // note: the exact message will vary between CE and ENT because they - // have different helpers for Consul cluster name lookup + name: "task missing Consul token", + hr: hrEmpty, wantErrMsg: "not found", }, { - name: "task with Consul WI", - req: &interfaces.TaskPrestartRequest{ - Task: &structs.Task{ - Name: "foo", - Consul: &structs.Consul{Cluster: "default"}, - }, - TaskDir: &allocdir.TaskDir{Dir: "foo"}, + name: "task without consul blocks uses default cluster", + hr: hrTokens, + wantConsulToken: defaultToken, + }, + { + name: "task with consul block at task level", + hr: hrTokens, + taskConsul: &structs.Consul{ + Cluster: structs.ConsulDefaultCluster, }, - consulToken: hr.GetConsulTokens()[structs.ConsulDefaultCluster]["consul_default"], + wantConsulToken: defaultToken, + }, + { + name: "task with consul block at group level", + hr: hrTokens, + groupConsul: &structs.Consul{ + Cluster: structs.ConsulDefaultCluster, + }, + wantConsulToken: defaultToken, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + a := mock.Alloc() + + task := a.Job.TaskGroups[0].Tasks[0] + if !tt.legacyFlow { + task.Identities = []*structs.WorkloadIdentity{ + {Name: fmt.Sprintf("%s_%s", + structs.ConsulTaskIdentityNamePrefix, + structs.ConsulDefaultCluster, + )}, + } + } + + clientConfig := &config.Config{Region: "global"} + envBuilder := taskenv.NewBuilder(mock.Node(), a, task, clientConfig.Region) + taskHooks := trtesting.NewMockTaskHooks() + + conf := &templateHookConfig{ + alloc: a, + logger: logger, + lifecycle: taskHooks, + events: &trtesting.MockEmitter{}, + clientConfig: clientConfig, + envBuilder: envBuilder, + hookResources: tt.hr, + } h := &templateHook{ config: conf, logger: logger, managerLock: sync.Mutex{}, driverHandle: nil, } + req := &interfaces.TaskPrestartRequest{ + Task: a.Job.TaskGroups[0].Tasks[0], + TaskDir: &allocdir.TaskDir{Dir: "foo"}, + } - err := h.Prestart(context.Background(), tt.req, nil) + err := h.Prestart(context.Background(), req, nil) if tt.wantErrMsg != "" { - must.NotNil(t, err) + must.Error(t, err) must.ErrorContains(t, err, tt.wantErrMsg) } else { - must.Nil(t, err) + must.NoError(t, err) } - if tt.consulToken != nil { - must.Eq(t, tt.consulToken.SecretID, h.consulToken) - } else { - must.Eq(t, "", h.consulToken) - } + must.Eq(t, tt.wantConsulToken, h.consulToken) }) } }