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

@@ -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
 }

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,

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,

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