consul: always include task services hook

CHANGELOG.md

@@ -5,6 +5,7 @@ IMPROVEMENTS:
  * consul/connect: interpolate the connect, service meta, and service canary meta blocks with the task environment [[GH-9586](https://github.com/hashicorp/nomad/pull/9586)]
 
 BUG FIXES:
+ * consul: Fixed a bug where updating a task to include services would not work [[GH-9707](https://github.com/hashicorp/nomad/issues/9707)]
  * template: Fixed multiple issues in template src/dest and artifact dest interpolation [[GH-9671](https://github.com/hashicorp/nomad/issues/9671)]
  * template: Fixed a bug where dynamic secrets did not trigger the template `change_mode` after a client restart. [[GH-9636](https://github.com/hashicorp/nomad/issues/9636)]
  * server: Fixed a bug where new servers may bootstrap prematurely when configured with `bootstrap_expect = 0`.

client/allocrunner/taskrunner/task_runner_hooks.go

@@ -101,16 +101,15 @@ func (tr *TaskRunner) initHooks() {
 		}))
 	}
 
-	// If there are any services, add the service hook
-	if len(task.Services) != 0 {
-		tr.runnerHooks = append(tr.runnerHooks, newServiceHook(serviceHookConfig{
-			alloc:     tr.Alloc(),
-			task:      tr.Task(),
-			consul:    tr.consulServiceClient,
-			restarter: tr,
-			logger:    hookLogger,
-		}))
-	}
+	// Always add the service hook. A task with no services on initial registration
+	// may be updated to include services, which must be handled with this hook.
+	tr.runnerHooks = append(tr.runnerHooks, newServiceHook(serviceHookConfig{
+		alloc:     tr.Alloc(),
+		task:      tr.Task(),
+		consul:    tr.consulServiceClient,
+		restarter: tr,
+		logger:    hookLogger,
+	}))
 
 	// If this is a Connect sidecar proxy (or a Connect Native) service,
 	// add the sidsHook for requesting a Service Identity token (if ACLs).

e2e/consul/consul.go

@@ -17,6 +17,9 @@ import (
 const (
 	consulJobBasic      = "consul/input/consul_example.nomad"
 	consulJobCanaryTags = "consul/input/canary_tags.nomad"
+
+	consulJobRegisterOnUpdatePart1 = "consul/input/services_empty.nomad"
+	consulJobRegisterOnUpdatePart2 = "consul/input/services_present.nomad"
 )
 
 type ConsulE2ETest struct {
@@ -47,24 +50,25 @@ func (tc *ConsulE2ETest) AfterEach(f *framework.F) {
 	}
 
 	for _, id := range tc.jobIds {
-		tc.Nomad().Jobs().Deregister(id, true, nil)
+		_, _, err := tc.Nomad().Jobs().Deregister(id, true, nil)
+		require.NoError(f.T(), err)
 	}
 	tc.jobIds = []string{}
-	tc.Nomad().System().GarbageCollect()
+	require.NoError(f.T(), tc.Nomad().System().GarbageCollect())
 }
 
 // TestConsulRegistration asserts that a job registers services with tags in Consul.
 func (tc *ConsulE2ETest) TestConsulRegistration(f *framework.F) {
 	t := f.T()
+	r := require.New(t)
 
 	nomadClient := tc.Nomad()
-	catalog := tc.Consul().Catalog()
-	jobId := "consul" + uuid.Generate()[0:8]
+	jobId := "consul" + uuid.Short()
 	tc.jobIds = append(tc.jobIds, jobId)
 
-	allocs := e2eutil.RegisterAndWaitForAllocs(f.T(), nomadClient, consulJobBasic, jobId, "")
-	require.Equal(t, 3, len(allocs))
-	allocIDs := e2eutil.AllocIDsFromAllocationListStubs(allocs)
+	allocations := e2eutil.RegisterAndWaitForAllocs(f.T(), nomadClient, consulJobBasic, jobId, "")
+	require.Equal(t, 3, len(allocations))
+	allocIDs := e2eutil.AllocIDsFromAllocationListStubs(allocations)
 	e2eutil.WaitForAllocsRunning(t, tc.Nomad(), allocIDs)
 
 	expectedTags := []string{
@@ -73,42 +77,58 @@ func (tc *ConsulE2ETest) TestConsulRegistration(f *framework.F) {
 	}
 
 	// Assert services get registered
-	testutil.WaitForResult(func() (bool, error) {
-		services, _, err := catalog.Service("consul-example", "", nil)
-		if err != nil {
-			return false, fmt.Errorf("error contacting Consul: %v", err)
-		}
-		if expected := 3; len(services) != expected {
-			return false, fmt.Errorf("expected %d services but found %d", expected, len(services))
-		}
-		for _, s := range services {
-			// If we've made it this far the tags should *always* match
-			require.True(t, helper.CompareSliceSetString(expectedTags, s.ServiceTags))
-		}
-		return true, nil
-	}, func(err error) {
-		t.Fatalf("error waiting for services to be registered: %v", err)
-	})
+	e2eutil.RequireConsulRegistered(r, tc.Consul(), "consul-example", 3)
+	services, _, err := tc.Consul().Catalog().Service("consul-example", "", nil)
+	require.NoError(t, err)
+	for _, s := range services {
+		// If we've made it this far the tags should *always* match
+		require.ElementsMatch(t, expectedTags, s.ServiceTags)
+	}
 
 	// Stop the job
 	e2eutil.WaitForJobStopped(t, nomadClient, jobId)
 
-	// Verify that services were deregistered in Consul
-	testutil.WaitForResult(func() (bool, error) {
-		s, _, err := catalog.Service("consul-example", "", nil)
-		if err != nil {
-			return false, err
-		}
+	// Verify that services were de-registered in Consul
+	e2eutil.RequireConsulDeregistered(r, tc.Consul(), "consul-example")
+}
 
-		return len(s) == 0, fmt.Errorf("expected 0 services but found: %v", s)
-	}, func(err error) {
-		t.Fatalf("error waiting for services to be deregistered: %v", err)
-	})
+func (tc *ConsulE2ETest) TestConsulRegisterOnUpdate(f *framework.F) {
+	t := f.T()
+	r := require.New(t)
+
+	nomadClient := tc.Nomad()
+	catalog := tc.Consul().Catalog()
+	jobID := "consul" + uuid.Short()
+	tc.jobIds = append(tc.jobIds, jobID)
+
+	// Initial job has no services for task.
+	allocations := e2eutil.RegisterAndWaitForAllocs(t, nomadClient, consulJobRegisterOnUpdatePart1, jobID, "")
+	require.Equal(t, 1, len(allocations))
+	allocIDs := e2eutil.AllocIDsFromAllocationListStubs(allocations)
+	e2eutil.WaitForAllocsRunning(t, tc.Nomad(), allocIDs)
+
+	// Assert service not yet registered.
+	results, _, err := catalog.Service("nc-service", "", nil)
+	require.NoError(t, err)
+	require.Empty(t, results)
+
+	// On update, add services for task.
+	allocations = e2eutil.RegisterAndWaitForAllocs(t, nomadClient, consulJobRegisterOnUpdatePart2, jobID, "")
+	require.Equal(t, 1, len(allocations))
+	allocIDs = e2eutil.AllocIDsFromAllocationListStubs(allocations)
+	e2eutil.WaitForAllocsRunning(t, tc.Nomad(), allocIDs)
+
+	// Assert service is now registered.
+	e2eutil.RequireConsulRegistered(r, tc.Consul(), "nc-service", 1)
 }
 
 // TestCanaryInplaceUpgrades verifies setting and unsetting canary tags
 func (tc *ConsulE2ETest) TestCanaryInplaceUpgrades(f *framework.F) {
 	t := f.T()
+
+	// TODO(shoenig) https://github.com/hashicorp/nomad/issues/9627
+	t.Skip("THIS TEST IS BROKEN (#9627)")
+
 	nomadClient := tc.Nomad()
 	consulClient := tc.Consul()
 	jobId := "consul" + uuid.Generate()[0:8]

e2e/consul/input/consul_example.nomad

@@ -23,7 +23,7 @@ job "consul-example" {
     healthy_deadline = "5m"
   }
 
-  group "cache" {
+  group "group" {
     count = 3
 
     network {
@@ -41,7 +41,7 @@ job "consul-example" {
       size = 300
     }
 
-    task "redis" {
+    task "example" {
       driver = "docker"
 
       config {

e2e/consul/input/services_empty.nomad

@@ -0,0 +1,29 @@
+job "consul-register-on-update" {
+  datacenters = ["dc1"]
+  type        = "service"
+
+  constraint {
+    attribute = "${attr.kernel.name}"
+    value     = "linux"
+  }
+
+  group "echo" {
+
+    task "busybox-nc" {
+      driver = "docker"
+
+      config {
+        image   = "busybox:1"
+        command = "nc"
+        args = [
+          "-ll",
+          "-p",
+          "1234",
+          "-e",
+        "/bin/cat"]
+      }
+
+      # no initial service definition
+    }
+  }
+}

e2e/consul/input/services_present.nomad

@@ -0,0 +1,31 @@
+job "consul-register-on-update" {
+  datacenters = ["dc1"]
+  type        = "service"
+
+  constraint {
+    attribute = "${attr.kernel.name}"
+    value     = "linux"
+  }
+
+  group "echo" {
+
+    task "busybox-nc" {
+      driver = "docker"
+
+      config {
+        image   = "busybox:1"
+        command = "nc"
+        args = [
+          "-ll",
+          "-p",
+          "1234",
+          "-e",
+        "/bin/cat"]
+      }
+
+      service {
+        name = "nc-service"
+      }
+    }
+  }
+}