From efdfef868f8d4ff5012e342195358a98446a87cc Mon Sep 17 00:00:00 2001
From: Danielle Lancashire <dani@hashicorp.com>
Date: Thu, 13 Jun 2019 14:57:27 +0200
Subject: [PATCH] consul: Include port-label in service registration

It is possible to provide multiple identically named services with
different port assignments in a Nomad configuration.

We introduced a regression when migrating to stable service identifiers where
multiple services with the same name would conflict, and the last definition
would take precedence.

This commit includes the port label in the stable service identifier to
allow the previous behaviour where this was supported, for example
providing:

```hcl
service {
  name = "redis-cache"
  tags = ["global", "cache"]
  port = "db"
  check {
    name     = "alive"
    type     = "tcp"
    interval = "10s"
    timeout  = "2s"
  }
}

service {
  name = "redis-cache"
  tags = ["global", "foo"]
  port = "foo"

  check {
    name     = "alive"
    type     = "tcp"
    port     = "db"
    interval = "10s"
    timeout  = "2s"
  }
}

service {
  name = "redis-cache"
  tags = ["global", "bar"]
  port = "bar"

  check {
    name     = "alive"
    type     = "tcp"
    port     = "db"
    interval = "10s"
    timeout  = "2s"
  }
}
```

in a nomad task definition is now completely valid. Each service
definition with the same name must still have a unique port label however.
---
 command/agent/consul/client.go    |  7 ++-
 command/agent/consul/unit_test.go | 71 ++++++++++++++++++++++++++++---
 2 files changed, 69 insertions(+), 9 deletions(-)

diff --git a/command/agent/consul/client.go b/command/agent/consul/client.go
index ae095c863174..ad605690c8be 100644
--- a/command/agent/consul/client.go
+++ b/command/agent/consul/client.go
@@ -1107,12 +1107,11 @@ func makeAgentServiceID(role string, service *structs.Service) string {
 }
 
 // makeTaskServiceID creates a unique ID for identifying a task service in
-// Consul. All structs.Service fields are included in the ID's hash except
-// Checks. This allows updates to merely compare IDs.
+// Consul.
 //
-//	Example Service ID: _nomad-task-b4e61df9-b095-d64e-f241-23860da1375f-redis-http
+//	Example Service ID: _nomad-task-b4e61df9-b095-d64e-f241-23860da1375f-redis-http-http
 func makeTaskServiceID(allocID, taskName string, service *structs.Service, canary bool) string {
-	return fmt.Sprintf("%s%s-%s-%s", nomadTaskPrefix, allocID, taskName, service.Name)
+	return fmt.Sprintf("%s%s-%s-%s-%s", nomadTaskPrefix, allocID, taskName, service.Name, service.PortLabel)
 }
 
 // makeCheckID creates a unique ID for a check.
diff --git a/command/agent/consul/unit_test.go b/command/agent/consul/unit_test.go
index fc2d77053a28..cb9606e80b91 100644
--- a/command/agent/consul/unit_test.go
+++ b/command/agent/consul/unit_test.go
@@ -205,7 +205,7 @@ func TestConsul_ChangePorts(t *testing.T) {
 		require.Equal(xPort, v.Port)
 	}
 
-	require.Equal(3, len(ctx.FakeConsul.checks))
+	require.Len(ctx.FakeConsul.checks, 3)
 
 	origTCPKey := ""
 	origScriptKey := ""
@@ -279,12 +279,12 @@ func TestConsul_ChangePorts(t *testing.T) {
 	for k, v := range ctx.FakeConsul.checks {
 		switch v.Name {
 		case "c1":
-			// C1 is not changed
-			require.Equal(origTCPKey, k)
+			// C1 is changed because the service was re-registered
+			require.NotEqual(origTCPKey, k)
 			require.Equal(fmt.Sprintf(":%d", xPort), v.TCP)
 		case "c2":
-			// C2 is not changed and should not have been re-registered
-			require.Equal(origScriptKey, k)
+			// C2 is changed because the service was re-registered
+			require.NotEqual(origScriptKey, k)
 		case "c3":
 			require.NotEqual(origHTTPKey, k)
 			require.Equal(fmt.Sprintf("http://:%d/", yPort), v.HTTP)
@@ -1615,3 +1615,64 @@ func TestGetAddress(t *testing.T) {
 		})
 	}
 }
+
+func TestConsul_ServiceName_Duplicates(t *testing.T) {
+	t.Parallel()
+	ctx := setupFake(t)
+	require := require.New(t)
+
+	ctx.Task.Services = []*structs.Service{
+		{
+			Name:      "best-service",
+			PortLabel: "x",
+			Tags: []string{
+				"foo",
+			},
+			Checks: []*structs.ServiceCheck{
+				{
+					Name:     "check-a",
+					Type:     "tcp",
+					Interval: time.Second,
+					Timeout:  time.Second,
+				},
+			},
+		},
+		{
+			Name:      "best-service",
+			PortLabel: "y",
+			Tags: []string{
+				"bar",
+			},
+			Checks: []*structs.ServiceCheck{
+				{
+					Name:     "checky-mccheckface",
+					Type:     "tcp",
+					Interval: time.Second,
+					Timeout:  time.Second,
+				},
+			},
+		},
+		{
+			Name:      "worst-service",
+			PortLabel: "y",
+		},
+	}
+
+	require.NoError(ctx.ServiceClient.RegisterTask(ctx.Task))
+
+	require.NoError(ctx.syncOnce())
+
+	require.Len(ctx.FakeConsul.services, 3)
+
+	for _, v := range ctx.FakeConsul.services {
+		if v.Name == ctx.Task.Services[0].Name && v.Port == xPort {
+			require.ElementsMatch(v.Tags, ctx.Task.Services[0].Tags)
+			require.Len(v.Checks, 1)
+		} else if v.Name == ctx.Task.Services[1].Name && v.Port == yPort {
+			require.ElementsMatch(v.Tags, ctx.Task.Services[1].Tags)
+			require.Len(v.Checks, 1)
+		} else if v.Name == ctx.Task.Services[2].Name {
+			require.Len(v.Checks, 0)
+		}
+	}
+}