From a83d6f996e043f687ff33cfcdc439b2a96d10891 Mon Sep 17 00:00:00 2001
From: Michael Schurter <schmichael@hashicorp.com>
Date: Tue, 18 Jul 2017 13:23:01 -0700
Subject: [PATCH] Never remove unknown agent services

Fixes #2827

This is a tradeoff. The pro is that you can run separate client and
server agents on the same node and advertise both. The con is that if a
Nomad agent crashes and isn't restarted on that node in the same mode
its entry will not be cleaned up.

That con scenario seems far less likely to occur than the scenario on
the pro side, and even if we do leak an agent entry the checks will be
failing so nothing should attempt to use it.
---
 command/agent/consul/client.go    |  6 ++++--
 command/agent/consul/unit_test.go | 24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/command/agent/consul/client.go b/command/agent/consul/client.go
index 733f899a3..a36109a5e 100644
--- a/command/agent/consul/client.go
+++ b/command/agent/consul/client.go
@@ -779,7 +779,9 @@ func createCheckReg(serviceID, checkID string, check *structs.ServiceCheck, host
 }
 
 // isNomadService returns true if the ID matches the pattern of a Nomad managed
-// service.
+// service. Agent services return false as independent client and server agents
+// may be running on the same machine. #2827
 func isNomadService(id string) bool {
-	return strings.HasPrefix(id, nomadServicePrefix)
+	const prefix = nomadServicePrefix + "-executor"
+	return strings.HasPrefix(id, prefix)
 }
diff --git a/command/agent/consul/unit_test.go b/command/agent/consul/unit_test.go
index ceec772b5..15a6a5f2d 100644
--- a/command/agent/consul/unit_test.go
+++ b/command/agent/consul/unit_test.go
@@ -1231,3 +1231,27 @@ func TestConsul_DriverNetwork_Change(t *testing.T) {
 
 	syncAndAssertPort(net.PortMap["x"])
 }
+
+// TestIsNomadService asserts the isNomadService helper returns true for Nomad
+// task IDs and false for unknown IDs and Nomad agent IDs (see #2827).
+func TestIsNomadService(t *testing.T) {
+	tests := []struct {
+		id     string
+		result bool
+	}{
+		{"_nomad-client-nomad-client-http", false},
+		{"_nomad-server-nomad-serf", false},
+		{"_nomad-executor-abc", true},
+		{"_nomad-executor", true},
+		{"not-nomad", false},
+	}
+
+	for _, test := range tests {
+		t.Run(test.id, func(t *testing.T) {
+			actual := isNomadService(test.id)
+			if actual != test.result {
+				t.Error("%q should be %t but found %t", test.id, test.result, actual)
+			}
+		})
+	}
+}