Adds node ID integrity checking to the catalog and the LAN and WAN clusters.

consul/catalog_endpoint.go

@@ -90,11 +90,13 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error
 		}
 	}
 
-	_, err = c.srv.raftApply(structs.RegisterRequestType, args)
+	resp, err := c.srv.raftApply(structs.RegisterRequestType, args)
 	if err != nil {
 		return err
 	}
-
+	if respErr, ok := resp.(error); ok {
+		return respErr
+	}
 	return nil
 }
 

consul/client.go

@@ -156,7 +156,11 @@ func (c *Client) setupSerf(conf *serf.Config, ch chan serf.Event, path string) (
 	conf.SnapshotPath = filepath.Join(c.config.DataDir, path)
 	conf.ProtocolVersion = protocolVersionMap[c.config.ProtocolVersion]
 	conf.RejoinAfterLeave = c.config.RejoinAfterLeave
-	conf.Merge = &lanMergeDelegate{dc: c.config.Datacenter}
+	conf.Merge = &lanMergeDelegate{
+		dc:       c.config.Datacenter,
+		nodeID:   c.config.NodeID,
+		nodeName: c.config.NodeName,
+	}
 	if err := lib.EnsurePath(conf.SnapshotPath, false); err != nil {
 		return nil, err
 	}

consul/merge.go

@@ -4,18 +4,41 @@ import (
 	"fmt"
 
 	"github.com/hashicorp/consul/consul/agent"
+	"github.com/hashicorp/consul/types"
 	"github.com/hashicorp/serf/serf"
 )
 
 // lanMergeDelegate is used to handle a cluster merge on the LAN gossip
 // ring. We check that the peers are in the same datacenter and abort the
 // merge if there is a mis-match.
 type lanMergeDelegate struct {
-	dc string
+	dc       string
+	nodeID   types.NodeID
+	nodeName string
 }
 
 func (md *lanMergeDelegate) NotifyMerge(members []*serf.Member) error {
+	nodeMap := make(map[types.NodeID]string)
 	for _, m := range members {
+		if rawID, ok := m.Tags["id"]; ok && rawID != "" {
+			nodeID := types.NodeID(rawID)
+
+			// See if there's another node that conflicts with us.
+			if (nodeID == md.nodeID) && (m.Name != md.nodeName) {
+				return fmt.Errorf("Member '%s' has conflicting node ID '%s' with this agent's ID",
+					m.Name, nodeID)
+			}
+
+			// See if there are any two nodes that conflict with each
+			// other. This lets us only do joins into a hygienic
+			// cluster now that node IDs are critical for operation.
+			if other, ok := nodeMap[nodeID]; ok {
+				return fmt.Errorf("Member '%s' has conflicting node ID '%s' with member '%s'",
+					m.Name, nodeID, other)
+			}
+			nodeMap[nodeID] = m.Name
+		}
+
 		ok, dc := isConsulNode(*m)
 		if ok {
 			if dc != md.dc {

consul/merge_test.go

@@ -0,0 +1,160 @@
+package consul
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/hashicorp/consul/types"
+	"github.com/hashicorp/serf/serf"
+)
+
+func makeNode(dc, name, id string, server bool) *serf.Member {
+	var role string
+	if server {
+		role = "consul"
+	} else {
+		role = "node"
+	}
+
+	return &serf.Member{
+		Name: name,
+		Tags: map[string]string{
+			"role":    role,
+			"dc":      dc,
+			"id":      id,
+			"port":    "8300",
+			"build":   "0.7.5",
+			"vsn":     "2",
+			"vsn_max": "3",
+			"vsn_min": "2",
+		},
+	}
+}
+
+func TestMerge_LAN(t *testing.T) {
+	cases := []struct {
+		members []*serf.Member
+		expect  string
+	}{
+		// Client in the wrong datacenter.
+		{
+			members: []*serf.Member{
+				makeNode("dc2",
+					"node1",
+					"96430788-246f-4379-94ce-257f7429e340",
+					false),
+			},
+			expect: "wrong datacenter",
+		},
+		// Server in the wrong datacenter.
+		{
+			members: []*serf.Member{
+				makeNode("dc2",
+					"node1",
+					"96430788-246f-4379-94ce-257f7429e340",
+					true),
+			},
+			expect: "wrong datacenter",
+		},
+		// Node ID conflict with delegate's ID.
+		{
+			members: []*serf.Member{
+				makeNode("dc1",
+					"node1",
+					"ee954a2f-80de-4b34-8780-97b942a50a99",
+					true),
+			},
+			expect: "with this agent's ID",
+		},
+		// Cluster with existing conflicting node IDs.
+		{
+			members: []*serf.Member{
+				makeNode("dc1",
+					"node1",
+					"6185913b-98d7-4441-bd8f-f7f7d854a4af",
+					true),
+				makeNode("dc1",
+					"node2",
+					"6185913b-98d7-4441-bd8f-f7f7d854a4af",
+					true),
+			},
+			expect: "with member",
+		},
+		// Good cluster.
+		{
+			members: []*serf.Member{
+				makeNode("dc1",
+					"node1",
+					"6185913b-98d7-4441-bd8f-f7f7d854a4af",
+					true),
+				makeNode("dc1",
+					"node2",
+					"cda916bc-a357-4a19-b886-59419fcee50c",
+					true),
+			},
+			expect: "",
+		},
+	}
+
+	delegate := &lanMergeDelegate{
+		dc:       "dc1",
+		nodeID:   types.NodeID("ee954a2f-80de-4b34-8780-97b942a50a99"),
+		nodeName: "node0",
+	}
+	for i, c := range cases {
+		if err := delegate.NotifyMerge(c.members); c.expect == "" {
+			if err != nil {
+				t.Fatalf("case %d: err: %v", i+1, err)
+			}
+		} else {
+			if err == nil || !strings.Contains(err.Error(), c.expect) {
+				t.Fatalf("case %d: err: %v", i+1, err)
+			}
+		}
+	}
+}
+
+func TestMerge_WAN(t *testing.T) {
+	cases := []struct {
+		members []*serf.Member
+		expect  string
+	}{
+		// Not a server
+		{
+			members: []*serf.Member{
+				makeNode("dc2",
+					"node1",
+					"96430788-246f-4379-94ce-257f7429e340",
+					false),
+			},
+			expect: "not a server",
+		},
+		// Good cluster.
+		{
+			members: []*serf.Member{
+				makeNode("dc2",
+					"node1",
+					"6185913b-98d7-4441-bd8f-f7f7d854a4af",
+					true),
+				makeNode("dc3",
+					"node2",
+					"cda916bc-a357-4a19-b886-59419fcee50c",
+					true),
+			},
+			expect: "",
+		},
+	}
+
+	delegate := &wanMergeDelegate{}
+	for i, c := range cases {
+		if err := delegate.NotifyMerge(c.members); c.expect == "" {
+			if err != nil {
+				t.Fatalf("case %d: err: %v", i+1, err)
+			}
+		} else {
+			if err == nil || !strings.Contains(err.Error(), c.expect) {
+				t.Fatalf("case %d: err: %v", i+1, err)
+			}
+		}
+	}
+}

consul/server.go

@@ -396,7 +396,11 @@ func (s *Server) setupSerf(conf *serf.Config, ch chan serf.Event, path string, w
 	if wan {
 		conf.Merge = &wanMergeDelegate{}
 	} else {
-		conf.Merge = &lanMergeDelegate{dc: s.config.Datacenter}
+		conf.Merge = &lanMergeDelegate{
+			dc:       s.config.Datacenter,
+			nodeID:   s.config.NodeID,
+			nodeName: s.config.NodeName,
+		}
 	}
 
 	// Until Consul supports this fully, we disable automatic resolution.

consul/state/catalog.go

@@ -165,22 +165,45 @@ func (s *StateStore) EnsureNode(idx uint64, node *structs.Node) error {
 // registration or modify an existing one in the state store. It allows
 // passing in a memdb transaction so it may be part of a larger txn.
 func (s *StateStore) ensureNodeTxn(tx *memdb.Txn, idx uint64, node *structs.Node) error {
-	// Check for an existing node
-	existing, err := tx.First("nodes", "id", node.Node)
-	if err != nil {
-		return fmt.Errorf("node name lookup failed: %s", err)
+	// See if there's an existing node with this UUID, and make sure the
+	// name is the same.
+	var n *structs.Node
+	if node.ID != "" {
+		existing, err := tx.First("nodes", "uuid", string(node.ID))
+		if err != nil {
+			return fmt.Errorf("node lookup failed: %s", err)
+		}
+		if existing != nil {
+			n = existing.(*structs.Node)
+			fmt.Printf("XXX %#v\n", *n)
+			if n.Node != node.Node {
+				return fmt.Errorf("node ID %q for node %q aliases existing node %q",
+					node.ID, node.Node, n.Node)
+			}
+		}
 	}
 
-	// Get the indexes
-	if existing != nil {
-		node.CreateIndex = existing.(*structs.Node).CreateIndex
+	// Check for an existing node by name to support nodes with no IDs.
+	if n == nil {
+		existing, err := tx.First("nodes", "id", node.Node)
+		if err != nil {
+			return fmt.Errorf("node name lookup failed: %s", err)
+		}
+		if existing != nil {
+			n = existing.(*structs.Node)
+		}
+	}
+
+	// Get the indexes.
+	if n != nil {
+		node.CreateIndex = n.CreateIndex
 		node.ModifyIndex = idx
 	} else {
 		node.CreateIndex = idx
 		node.ModifyIndex = idx
 	}
 
-	// Insert the node and update the index
+	// Insert the node and update the index.
 	if err := tx.Insert("nodes", node); err != nil {
 		return fmt.Errorf("failed inserting node: %s", err)
 	}

consul/state/catalog_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"reflect"
 	"sort"
+	"strings"
 	"testing"
 
 	"github.com/hashicorp/consul/consul/structs"
@@ -419,6 +420,23 @@ func TestStateStore_EnsureNode(t *testing.T) {
 	if idx != 3 {
 		t.Fatalf("bad index: %d", idx)
 	}
+
+	// Add an ID to the node
+	in.ID = types.NodeID("cda916bc-a357-4a19-b886-59419fcee50c")
+	if err := s.EnsureNode(4, in); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// Now try to add another node with the same ID
+	in = &structs.Node{
+		Node:    "nope",
+		ID:      types.NodeID("cda916bc-a357-4a19-b886-59419fcee50c"),
+		Address: "1.2.3.4",
+	}
+	err = s.EnsureNode(5, in)
+	if err == nil || !strings.Contains(err.Error(), "aliases existing node") {
+		t.Fatalf("err: %v", err)
+	}
 }
 
 func TestStateStore_GetNodes(t *testing.T) {