From 1333fa57a1e4871e47b103b32235ebd2553620d7 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Tue, 5 Sep 2017 22:57:29 -0700 Subject: [PATCH 1/2] Skips unique node ID check for old versions of Consul. Fixes #3070. --- agent/consul/merge.go | 16 +++++++- agent/consul/merge_test.go | 51 ++++++++++++++++++------ agent/metadata/build.go | 12 ++++++ agent/metadata/build_test.go | 75 ++++++++++++++++++++++++++++++++++++ agent/metadata/server.go | 7 +--- 5 files changed, 142 insertions(+), 19 deletions(-) create mode 100644 agent/metadata/build.go create mode 100644 agent/metadata/build_test.go diff --git a/agent/consul/merge.go b/agent/consul/merge.go index 9092725426..dfe26b3d6a 100644 --- a/agent/consul/merge.go +++ b/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,7 +42,16 @@ 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) } - nodeMap[nodeID] = m.Name + + // 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) diff --git a/agent/consul/merge_test.go b/agent/consul/merge_test.go index 04d39223b3..91e86a1243 100644 --- a/agent/consul/merge_test.go +++ b/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: "", }, diff --git a/agent/metadata/build.go b/agent/metadata/build.go new file mode 100644 index 0000000000..721a2a0af0 --- /dev/null +++ b/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) +} diff --git a/agent/metadata/build_test.go b/agent/metadata/build_test.go new file mode 100644 index 0000000000..f6a051447f --- /dev/null +++ b/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) + }) + } +} diff --git a/agent/metadata/server.go b/agent/metadata/server.go index cdef6b3485..88884b2a50 100644 --- a/agent/metadata/server.go +++ b/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 } From 520060e138404541cd15a4ca86c0fda5fe8a5e97 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Wed, 6 Sep 2017 13:23:19 -0700 Subject: [PATCH 2/2] Fixes incorrect comment. --- agent/consul/merge.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/consul/merge.go b/agent/consul/merge.go index dfe26b3d6a..b74634c8d9 100644 --- a/agent/consul/merge.go +++ b/agent/consul/merge.go @@ -43,7 +43,7 @@ func (md *lanMergeDelegate) NotifyMerge(members []*serf.Member) error { m.Name, nodeID, other) } - // Only map nodes with a version that's newer than when + // Only map nodes with a version that's >= than when // we made host-based IDs opt-in, which helps prevent // chaos when upgrading older clusters. See #3070 for // more details.