From 965fc9cf62413c6640f3b835fb73869467ef419d Mon Sep 17 00:00:00 2001 From: Matt Keeler Date: Thu, 12 Jul 2018 11:19:21 -0400 Subject: [PATCH] Revert "Allow changing Node names since Node now have IDs" --- agent/consul/state/catalog.go | 34 +------- agent/consul/state/catalog_test.go | 128 ++--------------------------- 2 files changed, 8 insertions(+), 154 deletions(-) diff --git a/agent/consul/state/catalog.go b/agent/consul/state/catalog.go index 4d6c079d5..e7d5ba3c0 100644 --- a/agent/consul/state/catalog.go +++ b/agent/consul/state/catalog.go @@ -350,21 +350,6 @@ func (s *Store) EnsureNode(idx uint64, node *structs.Node) error { return nil } -func (s *Store) ensureNoNodeWithSimilarNameTxn(tx *memdb.Txn, node *structs.Node) error { - // Retrieve all of the nodes - enodes, err := tx.Get("nodes", "id") - if err != nil { - return fmt.Errorf("Cannot lookup all nodes: %s", err) - } - for nodeIt := enodes.Next(); nodeIt != nil; nodeIt = enodes.Next() { - enode := nodeIt.(*structs.Node) - if strings.EqualFold(node.Node, enode.Node) && node.ID != enode.ID { - return fmt.Errorf("Node name %s is reserved by node %s with name %s", node.Node, enode.ID, enode.Node) - } - } - return nil -} - // ensureNodeTxn is the inner function called to actually create a node // 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. @@ -380,23 +365,8 @@ func (s *Store) ensureNodeTxn(tx *memdb.Txn, idx uint64, node *structs.Node) err if existing != nil { n = existing.(*structs.Node) if n.Node != node.Node { - // Lets first get all nodes and check whether name do match - dupNameError := s.ensureNoNodeWithSimilarNameTxn(tx, node) - if dupNameError != nil { - return fmt.Errorf("Error while renaming Node ID: %q: %s", node.ID, dupNameError) - } - // We are actually renaming a node, remove its reference first - err := s.deleteNodeTxn(tx, idx, n.Node) - if err != nil { - return fmt.Errorf("Error while renaming Node ID: %q from %s to %s", - node.ID, n.Node, node.Node) - } - } - - } else { - dupNameError := s.ensureNoNodeWithSimilarNameTxn(tx, node) - if dupNameError != nil { - return fmt.Errorf("Error while adding Node ID: %q: %s", node.ID, dupNameError) + return fmt.Errorf("node ID %q for node %q aliases existing node %q", + node.ID, node.Node, n.Node) } } } diff --git a/agent/consul/state/catalog_test.go b/agent/consul/state/catalog_test.go index 0bff61be7..17a411aab 100644 --- a/agent/consul/state/catalog_test.go +++ b/agent/consul/state/catalog_test.go @@ -480,131 +480,15 @@ func TestStateStore_EnsureNode(t *testing.T) { t.Fatalf("err: %v", err) } - // Renaming a node should work if ID is the same + // Now try to add another node with the same ID in = &structs.Node{ - Node: "node1-renamed", + Node: "nope", ID: types.NodeID("cda916bc-a357-4a19-b886-59419fcee50c"), - Address: "1.1.1.2", + Address: "1.2.3.4", } - if err := s.EnsureNode(6, in); err != nil { - t.Fatalf("err: %s", err) - } - - // Retrieve the node - idx, out, err = s.GetNode("node1") - if out != nil { - t.Fatalf("Node should not exist anymore: %q", out) - } - - idx, out, err = s.GetNode("node1-renamed") - if err != nil { - t.Fatalf("err: %s", err) - } - - // Node and indexes were updated - if out.CreateIndex != 1 || out.ModifyIndex != 6 || out.Address != "1.1.1.2" || out.Node != "node1-renamed" { - t.Fatalf("bad: %#v", out) - } - if idx != 6 { - t.Fatalf("bad index: %d", idx) - } - - newNodeID := types.NodeID("d0347693-65cc-4d9f-a6e0-5025b2e6513f") - - // Adding another node with same name should fail - in = &structs.Node{ - Node: "node1-renamed", - ID: newNodeID, - Address: "1.1.1.7", - } - if err := s.EnsureNode(8, in); err == nil { - t.Fatalf("err: %s", err) - } - - // Adding another node with same name but different case should fail - in = &structs.Node{ - Node: "Node1-RENAMED", - ID: newNodeID, - Address: "1.1.1.7", - } - if err := s.EnsureNode(8, in); err == nil { - t.Fatalf("err: %s", err) - } - - // Lets add another valid node now - in = &structs.Node{ - Node: "Node1bis", - ID: newNodeID, - Address: "1.1.1.7", - } - if err := s.EnsureNode(9, in); err != nil { - t.Fatalf("err: %s", err) - } - - // Retrieve the node - idx, out, err = s.GetNode("Node1bis") - if out == nil { - t.Fatalf("Node should exist, but was null") - } - - // Renaming should fail - in = &structs.Node{ - Node: "Node1bis", - ID: newNodeID, - Address: "1.1.1.7", - } - if err := s.EnsureNode(9, in); err != nil { - t.Fatalf("err: %s", err) - } - - idx, out, err = s.GetNode("Node1bis") - if err != nil { - t.Fatalf("err: %s", err) - } - - // Node and indexes were updated - if out.ID != newNodeID || out.CreateIndex != 9 || out.ModifyIndex != 9 || out.Address != "1.1.1.7" || out.Node != "Node1bis" { - t.Fatalf("bad: %#v", out) - } - if idx != 9 { - t.Fatalf("bad index: %d", idx) - } - - // Renaming to same value as first node should fail as well - // Adding another node with same name but different case should fail - in = &structs.Node{ - Node: "node1-renamed", - ID: newNodeID, - Address: "1.1.1.7", - } - if err := s.EnsureNode(10, in); err == nil { - t.Fatalf("err: %s", err) - } - - // It should fail also with different case - in = &structs.Node{ - Node: "Node1-Renamed", - ID: newNodeID, - Address: "1.1.1.7", - } - if err := s.EnsureNode(10, in); err == nil { - t.Fatalf("err: %s", err) - } - - // But should work if names are different - in = &structs.Node{ - Node: "Node1-Renamed2", - ID: newNodeID, - Address: "1.1.1.7", - } - if err := s.EnsureNode(11, in); err != nil { - t.Fatalf("err: %s", err) - } - - // Node renamed should be Ok - idx, out, err = s.GetNode("Node1-Renamed2") - if err != nil { - t.Fatalf("err: %s", err) + err = s.EnsureNode(5, in) + if err == nil || !strings.Contains(err.Error(), "aliases existing node") { + t.Fatalf("err: %v", err) } }