From f77b4baebbc39798e3b3b3d9d9890a9025483887 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Wed, 29 Nov 2023 16:46:27 -0500 Subject: [PATCH] service_hook: ensure task-level `consul.namespace` is respected (#19224) The task-level service hook is using the group-level method to get the provider namespace, but this was not designed with task-level `consul` blocks in mind. This leads to task-level services using the group-level `consul.namespace`. Fix by creating a method to get the correct namespace and move this into the service hook itself rather than in the outer `initHooks` method. --- client/allocrunner/taskrunner/service_hook.go | 9 ++---- .../taskrunner/service_hook_test.go | 3 +- .../taskrunner/task_runner_hooks.go | 5 --- nomad/structs/alloc.go | 32 +++++++++++++++++-- 4 files changed, 34 insertions(+), 15 deletions(-) diff --git a/client/allocrunner/taskrunner/service_hook.go b/client/allocrunner/taskrunner/service_hook.go index a337c493a..b76c575da 100644 --- a/client/allocrunner/taskrunner/service_hook.go +++ b/client/allocrunner/taskrunner/service_hook.go @@ -33,10 +33,6 @@ type serviceHookConfig struct { alloc *structs.Allocation task *structs.Task - // namespace is the Nomad or Consul namespace in which service - // registrations will be made. - providerNamespace string - // serviceRegWrapper is the handler wrapper that is used to perform service // and check registration and deregistration. serviceRegWrapper *wrapper.HandlerWrapper @@ -93,6 +89,7 @@ type serviceHook struct { func newServiceHook(c serviceHookConfig) *serviceHook { tg := c.alloc.Job.LookupTaskGroup(c.alloc.TaskGroup) + providerNamespace := c.alloc.ServiceProviderNamespaceForTask(c.task.Name) h := &serviceHook{ allocID: c.alloc.ID, @@ -101,7 +98,7 @@ func newServiceHook(c serviceHookConfig) *serviceHook { taskName: c.task.Name, tg: tg, namespace: c.alloc.Namespace, - providerNamespace: c.providerNamespace, + providerNamespace: providerNamespace, serviceRegWrapper: c.serviceRegWrapper, services: c.task.Services, restarter: c.restarter, @@ -191,7 +188,7 @@ func (h *serviceHook) updateHookFields(req *interfaces.TaskUpdateRequest) error // An update may change the service provider, therefore we need to account // for how namespaces work across providers also. - h.providerNamespace = req.Alloc.ServiceProviderNamespace() + h.providerNamespace = req.Alloc.ServiceProviderNamespaceForTask(h.taskName) return nil } diff --git a/client/allocrunner/taskrunner/service_hook_test.go b/client/allocrunner/taskrunner/service_hook_test.go index 607a00922..b58df2717 100644 --- a/client/allocrunner/taskrunner/service_hook_test.go +++ b/client/allocrunner/taskrunner/service_hook_test.go @@ -26,7 +26,7 @@ var _ interfaces.TaskExitedHook = (*serviceHook)(nil) var _ interfaces.TaskPreKillHook = (*serviceHook)(nil) var _ interfaces.TaskUpdateHook = (*serviceHook)(nil) -func TestUpdate_beforePoststart(t *testing.T) { +func Test_serviceHook_Update_beforePoststart(t *testing.T) { alloc := mock.Alloc() alloc.Job.Canonicalize() logger := testlog.HCLogger(t) @@ -183,7 +183,6 @@ func Test_serviceHook_Nomad(t *testing.T) { h := newServiceHook(serviceHookConfig{ alloc: alloc, task: alloc.LookupTask("web"), - providerNamespace: "default", serviceRegWrapper: regWrapper, restarter: agentconsul.NoopRestarter(), logger: logger, diff --git a/client/allocrunner/taskrunner/task_runner_hooks.go b/client/allocrunner/taskrunner/task_runner_hooks.go index 193757127..9cf6009aa 100644 --- a/client/allocrunner/taskrunner/task_runner_hooks.go +++ b/client/allocrunner/taskrunner/task_runner_hooks.go @@ -107,10 +107,6 @@ func (tr *TaskRunner) initHooks() { // Get the consul namespace for the TG of the allocation. consulNamespace := tr.alloc.ConsulNamespaceForTask(tr.taskName) - // Identify the service registration provider, which can differ from the - // Consul namespace depending on which provider is used. - serviceProviderNamespace := tr.alloc.ServiceProviderNamespace() - // If there are templates is enabled, add the hook if len(task.Templates) != 0 { tr.runnerHooks = append(tr.runnerHooks, newTemplateHook(&templateHookConfig{ @@ -133,7 +129,6 @@ func (tr *TaskRunner) initHooks() { tr.runnerHooks = append(tr.runnerHooks, newServiceHook(serviceHookConfig{ alloc: tr.Alloc(), task: tr.Task(), - providerNamespace: serviceProviderNamespace, serviceRegWrapper: tr.serviceRegWrapper, restarter: tr, hookResources: tr.allocHookResources, diff --git a/nomad/structs/alloc.go b/nomad/structs/alloc.go index a75948bf1..6cb57846b 100644 --- a/nomad/structs/alloc.go +++ b/nomad/structs/alloc.go @@ -30,10 +30,11 @@ type AllocServiceRegistrationsResponse struct { // services should be registered. This takes into account the different // providers that can provide service registrations. In the event no services // are found, the function will return the Consul namespace which allows hooks -// to work as they did before this feature. +// to work as they did before Nomad native services. // // It currently assumes that all services within an allocation use the same -// provider and therefore the same namespace. +// provider and therefore the same namespace, which is enforced at job submit +// time. func (a *Allocation) ServiceProviderNamespace() string { tg := a.Job.LookupTaskGroup(a.TaskGroup) @@ -60,6 +61,33 @@ func (a *Allocation) ServiceProviderNamespace() string { return tg.Consul.GetNamespace() } +// ServiceProviderNamespaceForTask returns the namespace within which a given +// tasks's services should be registered. This takes into account the different +// providers that can provide service registrations. In the event no services +// are found, the function will return the Consul namespace which allows hooks +// to work as they did before Nomad native services. +// +// It currently assumes that all services within a task use the same provider +// and therefore the same namespace, which is enforced at job submit time. +func (a *Allocation) ServiceProviderNamespaceForTask(taskName string) string { + tg := a.Job.LookupTaskGroup(a.TaskGroup) + + for _, task := range tg.Tasks { + if task.Name == taskName { + for _, service := range task.Services { + switch service.Provider { + case ServiceProviderNomad: + return a.Job.Namespace + default: + return a.ConsulNamespaceForTask(taskName) + } + } + } + } + + return a.ConsulNamespaceForTask(taskName) +} + type AllocInfo struct { AllocID string