Fixed function GetNodeID that was not working due to wrong type when searching node from its ID

Thus, all tests about renaming were not working properly.

Added the full test cas that allowed me to detect it.

Author: pierresouchay

agent/consul/state/catalog.go

@@ -1,6 +1,7 @@
 package state
 
 import (
+	"encoding/hex"
 	"fmt"
 	"strings"
 
@@ -375,12 +376,12 @@ func (s *Store) ensureNodeTxn(tx *memdb.Txn, idx uint64, node *structs.Node) err
 	// name is the same.
 	var n *structs.Node
 	if node.ID != "" {
-		existing, err := tx.First("nodes", "uuid", string(node.ID))
+		existing, err := getNodeIDTxn(tx, node.ID)
 		if err != nil {
 			return fmt.Errorf("node lookup failed: %s", err)
 		}
 		if existing != nil {
-			n = existing.(*structs.Node)
+			n = existing
 			if n.Node != node.Node {
 				// Lets first get all nodes and check whether name do match, we do not allow clash on nodes without ID
 				dupNameError := s.ensureNoNodeWithSimilarNameTxn(tx, node, false)
@@ -461,6 +462,25 @@ func (s *Store) GetNode(id string) (uint64, *structs.Node, error) {
 	return idx, nil, nil
 }
 
+func getNodeIDTxn(tx *memdb.Txn, id types.NodeID) (*structs.Node, error) {
+	sanitized := strings.Replace(string(id), "-", "", -1)
+	sanitizedLength := len(sanitized)
+	if sanitizedLength%2 != 0 {
+		return nil, fmt.Errorf("Input (without hyphens) must be even length")
+	}
+
+	dec, err := hex.DecodeString(sanitized)
+
+	node, err := tx.First("nodes", "uuid", dec)
+	if err != nil {
+		return nil, fmt.Errorf("node lookup failed: %s", err)
+	}
+	if node != nil {
+		return node.(*structs.Node), nil
+	}
+	return nil, nil
+}
+
 // GetNodeID is used to retrieve a node registration by node ID.
 func (s *Store) GetNodeID(id types.NodeID) (uint64, *structs.Node, error) {
 	tx := s.db.Txn(false)
@@ -470,14 +490,8 @@ func (s *Store) GetNodeID(id types.NodeID) (uint64, *structs.Node, error) {
 	idx := maxIndexTxn(tx, "nodes")
 
 	// Retrieve the node from the state store
-	node, err := tx.First("nodes", "uuid", string(id))
-	if err != nil {
-		return 0, nil, fmt.Errorf("node lookup failed: %s", err)
-	}
-	if node != nil {
-		return idx, node.(*structs.Node), nil
-	}
-	return idx, nil, nil
+	node, err := getNodeIDTxn(tx, id)
+	return idx, node, err
 }
 
 // Nodes is used to return all of the known nodes.

agent/consul/state/catalog_test.go

@@ -477,6 +477,113 @@ func TestStateStore_EnsureNodeDeprecated(t *testing.T) {
 	}
 }
 
+func TestNodeRenamingNodes(t *testing.T) {
+	s := testStateStore(t)
+
+	nodeID1 := types.NodeID("b789bf0a-d96b-4f70-a4a6-ac5dfaece53d")
+	nodeID2 := types.NodeID("27bee224-a4d7-45d0-9b8e-65b3c94a61ba")
+
+	// Node1 with ID
+	in1 := &structs.Node{
+		ID:      nodeID1,
+		Node:    "node1",
+		Address: "1.1.1.1",
+	}
+
+	if err := s.EnsureNode(1, in1); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	// Node2 with ID
+	in2 := &structs.Node{
+		ID:      nodeID2,
+		Node:    "node2",
+		Address: "1.1.1.2",
+	}
+
+	if err := s.EnsureNode(2, in2); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	// Node3 without ID
+	in3 := &structs.Node{
+		Node:    "node3",
+		Address: "1.1.1.3",
+	}
+
+	if err := s.EnsureNode(3, in3); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	if _, node, err := s.GetNodeID(nodeID1); err != nil || node == nil || node.ID != nodeID1 {
+		t.Fatalf("err: %s, node:= %q", err, node)
+	}
+
+	if _, node, err := s.GetNodeID(nodeID2); err != nil && node == nil || node.ID != nodeID2 {
+		t.Fatalf("err: %s", err)
+	}
+
+	// Renaming node2 into node1 should fail
+	in2Modify := &structs.Node{
+		ID:      nodeID2,
+		Node:    "node1",
+		Address: "1.1.1.2",
+	}
+	if err := s.EnsureNode(4, in2Modify); err == nil {
+		t.Fatalf("Renaming node2 into node1 should fail")
+	}
+
+	// Conflict with case insensitive matching as well
+	in2Modify = &structs.Node{
+		ID:      nodeID2,
+		Node:    "NoDe1",
+		Address: "1.1.1.2",
+	}
+	if err := s.EnsureNode(5, in2Modify); err == nil {
+		t.Fatalf("Renaming node2 into node1 should fail")
+	}
+
+	// Conflict with case insensitive on node without ID
+	in2Modify = &structs.Node{
+		ID:      nodeID2,
+		Node:    "NoDe3",
+		Address: "1.1.1.2",
+	}
+	if err := s.EnsureNode(6, in2Modify); err == nil {
+		t.Fatalf("Renaming node2 into node1 should fail")
+	}
+
+	// No conflict, should work
+	in2Modify = &structs.Node{
+		ID:      nodeID2,
+		Node:    "node2bis",
+		Address: "1.1.1.2",
+	}
+	if err := s.EnsureNode(6, in2Modify); err != nil {
+		t.Fatalf("Renaming node2 into node1 should fail")
+	}
+
+	// Retrieve the node again
+	idx, out, err := s.GetNode("node2bis")
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	// Retrieve the node again
+	idx2, out2, err := s.GetNodeID(nodeID2)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	if idx != idx2 {
+		t.Fatalf("node should be the same")
+	}
+
+	if out.ID != out2.ID || out.Node != out2.Node {
+		t.Fatalf("all should match")
+	}
+}
+
 func TestStateStore_EnsureNode(t *testing.T) {
 	s := testStateStore(t)