Merge pull request #414 from alvaroaleman/do-not-require-clusterwide-…

…unique-serviceid Identify services using both the ID and the Node
fabiolb · Dec 21, 2017 · 8b9a885 · 8b9a885
2 parents 46e38c5 + b155591
commit 8b9a885
Show file tree

Hide file tree

Showing 3 changed files with 12 additions and 4 deletions.
diff --git a/registry/consul/passing.go b/registry/consul/passing.go
@@ -26,11 +26,14 @@ func passingServices(checks []*api.HealthCheck, status []string) []*api.HealthCh
 		// then check whether the agent is still alive and both the
 		// node and the service are not in maintenance mode.
 		for _, c := range checks {
-			if c.CheckID == "serfHealth" && c.Node == svc.Node && c.Status == "critical" {
+			if c.Node != svc.Node {
+				continue
+			}
+			if c.CheckID == "serfHealth" && c.Status == "critical" {
 				log.Printf("[DEBUG] consul: Skipping service %q since agent on node %q is down: %s", svc.ServiceID, svc.Node, c.Output)
 				goto skip
 			}
-			if c.CheckID == "_node_maintenance" && c.Node == svc.Node {
+			if c.CheckID == "_node_maintenance" {
 				log.Printf("[DEBUG] consul: Skipping service %q since node %q is in maintenance mode: %s", svc.ServiceID, svc.Node, c.Output)
 				goto skip
 			}

diff --git a/registry/consul/passing_test.go b/registry/consul/passing_test.go
@@ -12,6 +12,7 @@ func TestPassingServices(t *testing.T) {
 		serfPass     = &api.HealthCheck{Node: "node", CheckID: "serfHealth", Status: "passing"}
 		serfFail     = &api.HealthCheck{Node: "node", CheckID: "serfHealth", Status: "critical"}
 		svc1Pass     = &api.HealthCheck{Node: "node", CheckID: "service:abc", Status: "passing", ServiceName: "abc", ServiceID: "abc-1"}
+		svc1N2Pass   = &api.HealthCheck{Node: "node2", CheckID: "service:abc", Status: "passing", ServiceName: "abc", ServiceID: "abc-1"}
 		svc1Warn     = &api.HealthCheck{Node: "node", CheckID: "service:abc", Status: "warning", ServiceName: "abc", ServiceID: "abc-2"}
 		svc1Crit     = &api.HealthCheck{Node: "node", CheckID: "service:abc", Status: "critical", ServiceName: "abc", ServiceID: "abc-3"}
 		svc2Pass     = &api.HealthCheck{Node: "node", CheckID: "my-check-id", Status: "passing", ServiceName: "def", ServiceID: "def-1"}
@@ -36,6 +37,7 @@ func TestPassingServices(t *testing.T) {
 		{[]string{"passing"}, []*api.HealthCheck{serfFail, nodeMaint, svc1Maint, svc1Pass}, nil},
 		{[]string{"passing"}, []*api.HealthCheck{svc1ID2Maint, svc1Pass}, []*api.HealthCheck{svc1Pass}},
 		{[]string{"passing"}, []*api.HealthCheck{svc1Maint, svc1Pass, svc2Pass}, []*api.HealthCheck{svc2Pass}},
+		{[]string{"passing"}, []*api.HealthCheck{svc1Maint, svc1N2Pass}, []*api.HealthCheck{svc1N2Pass}},
 		{[]string{"passing", "warning"}, []*api.HealthCheck{serfPass, svc1Pass, svc1Crit}, []*api.HealthCheck{svc1Pass}},
 		{[]string{"passing", "warning"}, []*api.HealthCheck{serfPass, svc1Warn, svc1Crit}, []*api.HealthCheck{svc1Warn}},
 		{[]string{"passing", "warning"}, []*api.HealthCheck{serfPass, svc1Pass, svc1Warn}, []*api.HealthCheck{svc1Pass, svc1Warn}},

diff --git a/registry/consul/service.go b/registry/consul/service.go
@@ -39,7 +39,10 @@ func servicesConfig(client *api.Client, checks []*api.HealthCheck, tagPrefix str
 	// map service name to list of service passing for which the health check is ok
 	m := map[string]map[string]bool{}
 	for _, check := range checks {
-		name, id := check.ServiceName, check.ServiceID
+		// Make the node part of the id, because according to the Consul docs
+		// the ServiceID is unique per agent but not cluster wide
+		// https://www.consul.io/api/agent/service.html#id
+		name, id := check.ServiceName, check.ServiceID+check.Node
 
 		if _, ok := m[name]; !ok {
 			m[name] = map[string]bool{}
@@ -85,7 +88,7 @@ func serviceConfig(client *api.Client, name string, passing map[string]bool, tag
 	for _, svc := range svcs {
 		// check if the instance is in the list of instances
 		// which passed the health check
-		if _, ok := passing[svc.ServiceID]; !ok {
+		if _, ok := passing[svc.ServiceID+svc.Node]; !ok {
 			continue
 		}