Skips unique node ID check for old versions of Consul.

Fixes #3070.

agent/consul/merge.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/hashicorp/consul/agent/metadata"
 	"github.com/hashicorp/consul/types"
+	"github.com/hashicorp/go-version"
 	"github.com/hashicorp/serf/serf"
 )
 
@@ -18,6 +19,10 @@ type lanMergeDelegate struct {
 	segment  string
 }
 
+// uniqueIDMinVersion is the lowest version where we insist that nodes
+// have a unique ID.
+var uniqueIDMinVersion = version.Must(version.NewVersion("0.8.5"))
+
 func (md *lanMergeDelegate) NotifyMerge(members []*serf.Member) error {
 	nodeMap := make(map[types.NodeID]string)
 	for _, m := range members {
@@ -37,8 +42,17 @@ func (md *lanMergeDelegate) NotifyMerge(members []*serf.Member) error {
 				return fmt.Errorf("Member '%s' has conflicting node ID '%s' with member '%s'",
 					m.Name, nodeID, other)
 			}
+
+			// Only map nodes with a version that's newer than when
+			// we made host-based IDs opt-in, which helps prevent
+			// chaos when upgrading older clusters. See #3070 for
+			// more details.
+			if ver, err := metadata.Build(m); err == nil {
+				if ver.Compare(uniqueIDMinVersion) >= 0 {
 					nodeMap[nodeID] = m.Name
 				}
+			}
+		}
 
 		ok, dc := isConsulNode(*m)
 		if ok {

agent/consul/merge_test.go

@@ -8,7 +8,7 @@ import (
 	"github.com/hashicorp/serf/serf"
 )
 
-func makeNode(dc, name, id string, server bool) *serf.Member {
+func makeNode(dc, name, id string, server bool, build string) *serf.Member {
 	var role string
 	if server {
 		role = "consul"
@@ -23,7 +23,7 @@ func makeNode(dc, name, id string, server bool) *serf.Member {
 			"dc":      dc,
 			"id":      id,
 			"port":    "8300",
-			"build":   "0.7.5",
+			"build":   build,
 			"vsn":     "2",
 			"vsn_max": "3",
 			"vsn_min": "2",
@@ -43,7 +43,8 @@ func TestMerge_LAN(t *testing.T) {
 				makeNode("dc2",
 					"node1",
 					"96430788-246f-4379-94ce-257f7429e340",
-					false),
+					false,
+					"0.7.5"),
 			},
 			expect: "wrong datacenter",
 		},
@@ -53,7 +54,8 @@ func TestMerge_LAN(t *testing.T) {
 				makeNode("dc2",
 					"node1",
 					"96430788-246f-4379-94ce-257f7429e340",
-					true),
+					true,
+					"0.7.5"),
 			},
 			expect: "wrong datacenter",
 		},
@@ -63,7 +65,8 @@ func TestMerge_LAN(t *testing.T) {
 				makeNode("dc1",
 					"node1",
 					"ee954a2f-80de-4b34-8780-97b942a50a99",
-					true),
+					true,
+					"0.7.5"),
 			},
 			expect: "with this agent's ID",
 		},
@@ -73,11 +76,30 @@ func TestMerge_LAN(t *testing.T) {
 				makeNode("dc1",
 					"node1",
 					"6185913b-98d7-4441-bd8f-f7f7d854a4af",
-					true),
+					true,
+					"0.8.5"),
 				makeNode("dc1",
 					"node2",
 					"6185913b-98d7-4441-bd8f-f7f7d854a4af",
-					true),
+					true,
+					"0.9.0"),
+			},
+			expect: "with member",
+		},
+		// Cluster with existing conflicting node IDs, but version is
+		// old enough to skip the check.
+		{
+			members: []*serf.Member{
+				makeNode("dc1",
+					"node1",
+					"6185913b-98d7-4441-bd8f-f7f7d854a4af",
+					true,
+					"0.8.5"),
+				makeNode("dc1",
+					"node2",
+					"6185913b-98d7-4441-bd8f-f7f7d854a4af",
+					true,
+					"0.8.4"),
 			},
 			expect: "with member",
 		},
@@ -87,11 +109,13 @@ func TestMerge_LAN(t *testing.T) {
 				makeNode("dc1",
 					"node1",
 					"6185913b-98d7-4441-bd8f-f7f7d854a4af",
-					true),
+					true,
+					"0.8.5"),
 				makeNode("dc1",
 					"node2",
 					"cda916bc-a357-4a19-b886-59419fcee50c",
-					true),
+					true,
+					"0.8.5"),
 			},
 			expect: "",
 		},
@@ -128,7 +152,8 @@ func TestMerge_WAN(t *testing.T) {
 				makeNode("dc2",
 					"node1",
 					"96430788-246f-4379-94ce-257f7429e340",
-					false),
+					false,
+					"0.7.5"),
 			},
 			expect: "not a server",
 		},
@@ -138,11 +163,13 @@ func TestMerge_WAN(t *testing.T) {
 				makeNode("dc2",
 					"node1",
 					"6185913b-98d7-4441-bd8f-f7f7d854a4af",
-					true),
+					true,
+					"0.7.5"),
 				makeNode("dc3",
 					"node2",
 					"cda916bc-a357-4a19-b886-59419fcee50c",
-					true),
+					true,
+					"0.7.5"),
 			},
 			expect: "",
 		},

agent/metadata/build.go

@@ -0,0 +1,12 @@
+package metadata
+
+import (
+	"github.com/hashicorp/go-version"
+	"github.com/hashicorp/serf/serf"
+)
+
+// Build extracts the Consul version info for a member.
+func Build(m *serf.Member) (*version.Version, error) {
+	str := versionFormat.FindString(m.Tags["build"])
+	return version.NewVersion(str)
+}

agent/metadata/build_test.go

@@ -0,0 +1,75 @@
+package metadata
+
+import (
+	"testing"
+
+	"github.com/hashicorp/go-version"
+	"github.com/hashicorp/serf/serf"
+	"github.com/pascaldekloe/goe/verify"
+)
+
+func TestBuild(t *testing.T) {
+	tests := []struct {
+		desc string
+		m    *serf.Member
+		ver  *version.Version
+		err  bool
+	}{
+		{
+			"no version",
+			&serf.Member{},
+			nil,
+			true,
+		},
+		{
+			"bad version",
+			&serf.Member{
+				Tags: map[string]string{
+					"build": "nope",
+				},
+			},
+			nil,
+			true,
+		},
+		{
+			"good version",
+			&serf.Member{
+				Tags: map[string]string{
+					"build": "0.8.5",
+				},
+			},
+			version.Must(version.NewVersion("0.8.5")),
+			false,
+		},
+		{
+			"rc version",
+			&serf.Member{
+				Tags: map[string]string{
+					"build": "0.9.3rc1:d62743c",
+				},
+			},
+			version.Must(version.NewVersion("0.9.3")),
+			false,
+		},
+		{
+			"ent version",
+			&serf.Member{
+				Tags: map[string]string{
+					"build": "0.9.3+ent:d62743c",
+				},
+			},
+			version.Must(version.NewVersion("0.9.3")),
+			false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			ver, err := Build(tt.m)
+			gotErr := err != nil
+			if wantErr := tt.err; gotErr != wantErr {
+				t.Fatalf("got %v want %v", gotErr, wantErr)
+			}
+			verify.Values(t, "", ver, tt.ver)
+		})
+	}
+}

agent/metadata/server.go

@@ -1,8 +1,3 @@
-// Package agent provides a logical endpoint for Consul agents in the
-// network.  agent data originates from Serf gossip and is primarily used to
-// communicate Consul server information.  Gossiped information that ends up
-// in Server contains the necessary metadata required for servers.Manager to
-// select which server an RPC request should be routed to.
 package metadata
 
 import (
@@ -116,7 +111,7 @@ func IsConsulServer(m serf.Member) (bool, *Server) {
 		}
 	}
 
-	build_version, err := version.NewVersion(versionFormat.FindString(m.Tags["build"]))
+	build_version, err := Build(&m)
 	if err != nil {
 		return false, nil
 	}