From e39dd09bfa14c38f4f96cc850d982d46047d7dc4 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 1 Feb 2017 11:00:06 -0800 Subject: [PATCH 1/8] Small premature optimization in `isUUID()`. If the length isn't `36`, return `false` immediately before firing up the regexp engine. --- consul/state/prepared_query.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/consul/state/prepared_query.go b/consul/state/prepared_query.go index c9c4f7e13..e49ebceeb 100644 --- a/consul/state/prepared_query.go +++ b/consul/state/prepared_query.go @@ -14,6 +14,11 @@ var validUUID = regexp.MustCompile(`(?i)^[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f // isUUID returns true if the given string is a valid UUID. func isUUID(str string) bool { + const uuidLen = 36 + if len(str) != uuidLen { + return false + } + return validUUID.MatchString(str) } From c9eea45b1c39000e506ac6a55b2c3aaa45351ea2 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 1 Feb 2017 14:20:25 -0800 Subject: [PATCH 2/8] Enable looking up consul nodes by their node ID. Assuming the following output from a consul agent: ``` ==> Consul agent running! Version: 'v0.7.3-43-gc5e140c-dev (c5e140c+CHANGES)' Node ID: '40e4a748-2192-161a-0510-9bf59fe950b5' Node name: 'myhost' ``` it is now possible to lookup nodes by their Node Name or Node ID, or a prefix match of the Node ID, with the following caveats re: the prefix match: 1) first eight digits of the Node ID are a required minimum (eight was chosen as an arbitrary number) 2) the length of the Node ID must be an even number or no result will be returned. ``` % dig @127.0.0.1 -p 8600 myhost.node.dc1.consul. myhost.node.dc1.consul. 0 IN A 127.0.0.1 % dig @127.0.0.1 -p 8600 40e4a748-2192-161a-0510-9bf59fe950b5.node.dc1.consul. 40e4a748-2192-161a-0510-9bf59fe950b5.node.dc1.consul. 0 IN A 127.0.0.1 % dig @127.0.0.1 -p 8600 40e4a748.node.dc1.consul. 40e4a748.node.dc1.consul. 0 IN A 127.0.0.1 % dig @127.0.0.1 -p 8600 40e4a74821.node.dc1.consul. 40e4a74821.node.dc1.consul. 0 IN A 127.0.0.1 % dig @127.0.0.1 -p 8600 40e4a748-21.node.dc1.consul. 40e4a748-21.node.dc1.consul. 0 IN A 127.0.0.1 ``` --- consul/state/catalog.go | 55 ++++++++++++++++++++++++++++++++---- consul/state/catalog_test.go | 22 +++++++++++++-- consul/state/schema.go | 8 ++++++ 3 files changed, 78 insertions(+), 7 deletions(-) diff --git a/consul/state/catalog.go b/consul/state/catalog.go index f10a35189..6c3f93a22 100644 --- a/consul/state/catalog.go +++ b/consul/state/catalog.go @@ -9,6 +9,13 @@ import ( "github.com/hashicorp/go-memdb" ) +const ( + // minUUIDLookupLen is used as a minimum length of a node name required before + // we test to see if the name is actually a UUID and perform an ID-based node + // lookup. + minUUIDLookupLen = 8 +) + // Nodes is used to pull the full list of nodes for use during snapshots. func (s *StateSnapshot) Nodes() (memdb.ResultIterator, error) { iter, err := s.tx.Get("nodes", "id") @@ -151,7 +158,7 @@ func (s *StateStore) ensureNodeTxn(tx *memdb.Txn, idx uint64, node *structs.Node // Check for an existing node existing, err := tx.First("nodes", "id", node.Node) if err != nil { - return fmt.Errorf("node lookup failed: %s", err) + return fmt.Errorf("node name lookup failed: %s", err) } // Get the indexes @@ -174,7 +181,7 @@ func (s *StateStore) ensureNodeTxn(tx *memdb.Txn, idx uint64, node *structs.Node return nil } -// GetNode is used to retrieve a node registration by node ID. +// GetNode is used to retrieve a node registration by node name ID. func (s *StateStore) GetNode(id string) (uint64, *structs.Node, error) { tx := s.db.Txn(false) defer tx.Abort() @@ -193,6 +200,25 @@ func (s *StateStore) GetNode(id string) (uint64, *structs.Node, error) { return idx, nil, nil } +// GetNodeID is used to retrieve a node registration by node ID. +func (s *StateStore) GetNodeID(id types.NodeID) (uint64, *structs.Node, error) { + tx := s.db.Txn(false) + defer tx.Abort() + + // Get the table index. + 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 +} + // Nodes is used to return all of the known nodes. func (s *StateStore) Nodes(ws memdb.WatchSet) (uint64, structs.Nodes, error) { tx := s.db.Txn(false) @@ -662,15 +688,34 @@ func (s *StateStore) NodeServices(ws memdb.WatchSet, nodeName string) (uint64, * // Get the table index. idx := maxIndexTxn(tx, "nodes", "services") - // Query the node + // Query the node by node name watchCh, n, err := tx.FirstWatch("nodes", "id", nodeName) if err != nil { return 0, nil, fmt.Errorf("node lookup failed: %s", err) } - ws.Add(watchCh) + if n == nil { - return 0, nil, nil + if len(nodeName) >= minUUIDLookupLen { + // Attempt to lookup the node by it's node ID + var idWatchCh <-chan struct{} + idWatchCh, n, err = tx.FirstWatch("nodes", "uuid_prefix", nodeName) + if err != nil { + return 0, nil, fmt.Errorf("node ID lookup failed: %s", err) + } + if n == nil { + ws.Add(watchCh) + return 0, nil, nil + } else { + ws.Add(idWatchCh) + } + } else { + ws.Add(watchCh) + return 0, nil, nil + } + } else { + ws.Add(watchCh) } + node := n.(*structs.Node) // Read all of the services diff --git a/consul/state/catalog_test.go b/consul/state/catalog_test.go index eaecfcef7..e142b0938 100644 --- a/consul/state/catalog_test.go +++ b/consul/state/catalog_test.go @@ -10,14 +10,23 @@ import ( "github.com/hashicorp/consul/lib" "github.com/hashicorp/consul/types" "github.com/hashicorp/go-memdb" + uuid "github.com/hashicorp/go-uuid" ) +func makeRandomNodeID(t *testing.T) types.NodeID { + id, err := uuid.GenerateUUID() + if err != nil { + t.Fatalf("err: %v", err) + } + return types.NodeID(id) +} + func TestStateStore_EnsureRegistration(t *testing.T) { s := testStateStore(t) // Start with just a node. req := &structs.RegisterRequest{ - ID: types.NodeID("40e4a748-2192-161a-0510-9bf59fe950b5"), + ID: makeRandomNodeID(t), Node: "node1", Address: "1.2.3.4", TaggedAddresses: map[string]string{ @@ -27,6 +36,7 @@ func TestStateStore_EnsureRegistration(t *testing.T) { "somekey": "somevalue", }, } + nodeID := req.ID if err := s.EnsureRegistration(1, req); err != nil { t.Fatalf("err: %s", err) } @@ -37,7 +47,7 @@ func TestStateStore_EnsureRegistration(t *testing.T) { if err != nil { t.Fatalf("err: %s", err) } - if out.ID != types.NodeID("40e4a748-2192-161a-0510-9bf59fe950b5") || + if out.ID != nodeID || out.Node != "node1" || out.Address != "1.2.3.4" || len(out.TaggedAddresses) != 1 || out.TaggedAddresses["hello"] != "world" || @@ -45,6 +55,13 @@ func TestStateStore_EnsureRegistration(t *testing.T) { out.CreateIndex != 1 || out.ModifyIndex != 1 { t.Fatalf("bad node returned: %#v", out) } + _, out2, err := s.GetNodeID(nodeID) + if err != nil { + t.Fatalf("err: %s", err) + } + if !reflect.DeepEqual(out, out2) { + t.Fatalf("bad node returned: %#v -- %#v", out, out2) + } } verifyNode() @@ -183,6 +200,7 @@ func TestStateStore_EnsureRegistration_Restore(t *testing.T) { // Start with just a node. req := &structs.RegisterRequest{ + ID: makeRandomNodeID(t), Node: "node1", Address: "1.2.3.4", } diff --git a/consul/state/schema.go b/consul/state/schema.go index cf701d64e..5c39bb3b7 100644 --- a/consul/state/schema.go +++ b/consul/state/schema.go @@ -78,6 +78,14 @@ func nodesTableSchema() *memdb.TableSchema { Lowercase: true, }, }, + "uuid": &memdb.IndexSchema{ + Name: "uuid", + AllowMissing: false, + Unique: true, + Indexer: &memdb.UUIDFieldIndex{ + Field: "ID", + }, + }, "meta": &memdb.IndexSchema{ Name: "meta", AllowMissing: true, From 19c2cd106ab4589d0a52b8cb7f88cccc8823924a Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 1 Feb 2017 14:58:34 -0800 Subject: [PATCH 3/8] Toggle `AllowMissing` to false to accommodate old clients without Node IDs. --- consul/state/schema.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consul/state/schema.go b/consul/state/schema.go index 5c39bb3b7..9b93e2b5f 100644 --- a/consul/state/schema.go +++ b/consul/state/schema.go @@ -80,7 +80,7 @@ func nodesTableSchema() *memdb.TableSchema { }, "uuid": &memdb.IndexSchema{ Name: "uuid", - AllowMissing: false, + AllowMissing: true, Unique: true, Indexer: &memdb.UUIDFieldIndex{ Field: "ID", From 324215c842b2de4c58d6e1f20cb1b980e756871a Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 1 Feb 2017 14:59:24 -0800 Subject: [PATCH 4/8] Rename `nodeName` to `nodeNameOrID`. --- consul/state/catalog.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/consul/state/catalog.go b/consul/state/catalog.go index 6c3f93a22..aa1c98db7 100644 --- a/consul/state/catalog.go +++ b/consul/state/catalog.go @@ -681,7 +681,7 @@ func (s *StateStore) NodeService(nodeName string, serviceID string) (uint64, *st } // NodeServices is used to query service registrations by node ID. -func (s *StateStore) NodeServices(ws memdb.WatchSet, nodeName string) (uint64, *structs.NodeServices, error) { +func (s *StateStore) NodeServices(ws memdb.WatchSet, nodeNameOrID string) (uint64, *structs.NodeServices, error) { tx := s.db.Txn(false) defer tx.Abort() @@ -689,16 +689,16 @@ func (s *StateStore) NodeServices(ws memdb.WatchSet, nodeName string) (uint64, * idx := maxIndexTxn(tx, "nodes", "services") // Query the node by node name - watchCh, n, err := tx.FirstWatch("nodes", "id", nodeName) + watchCh, n, err := tx.FirstWatch("nodes", "id", nodeNameOrID) if err != nil { return 0, nil, fmt.Errorf("node lookup failed: %s", err) } if n == nil { - if len(nodeName) >= minUUIDLookupLen { + if len(nodeNameOrID) >= minUUIDLookupLen { // Attempt to lookup the node by it's node ID var idWatchCh <-chan struct{} - idWatchCh, n, err = tx.FirstWatch("nodes", "uuid_prefix", nodeName) + idWatchCh, n, err = tx.FirstWatch("nodes", "uuid_prefix", nodeNameOrID) if err != nil { return 0, nil, fmt.Errorf("node ID lookup failed: %s", err) } @@ -719,9 +719,9 @@ func (s *StateStore) NodeServices(ws memdb.WatchSet, nodeName string) (uint64, * node := n.(*structs.Node) // Read all of the services - services, err := tx.Get("services", "node", nodeName) + services, err := tx.Get("services", "node", nodeNameOrID) if err != nil { - return 0, nil, fmt.Errorf("failed querying services for node %q: %s", nodeName, err) + return 0, nil, fmt.Errorf("failed querying services for node %q: %s", nodeNameOrID, err) } ws.Add(services.WatchCh()) From b241cfc7fdae471bfff04801617a90b02c88f7c9 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 1 Feb 2017 15:18:00 -0800 Subject: [PATCH 5/8] Whoops. Return an empty set in the event that there are multiple matches. --- consul/state/catalog.go | 42 ++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/consul/state/catalog.go b/consul/state/catalog.go index aa1c98db7..c0212c05a 100644 --- a/consul/state/catalog.go +++ b/consul/state/catalog.go @@ -694,26 +694,34 @@ func (s *StateStore) NodeServices(ws memdb.WatchSet, nodeNameOrID string) (uint6 return 0, nil, fmt.Errorf("node lookup failed: %s", err) } - if n == nil { - if len(nodeNameOrID) >= minUUIDLookupLen { - // Attempt to lookup the node by it's node ID - var idWatchCh <-chan struct{} - idWatchCh, n, err = tx.FirstWatch("nodes", "uuid_prefix", nodeNameOrID) - if err != nil { - return 0, nil, fmt.Errorf("node ID lookup failed: %s", err) - } - if n == nil { - ws.Add(watchCh) - return 0, nil, nil - } else { - ws.Add(idWatchCh) - } - } else { + if n != nil { + ws.Add(watchCh) + } else { + if len(nodeNameOrID) < minUUIDLookupLen { ws.Add(watchCh) return 0, nil, nil } - } else { - ws.Add(watchCh) + + // Attempt to lookup the node by it's node ID + iter, err := tx.Get("nodes", "uuid_prefix", nodeNameOrID) + if err != nil { + return 0, nil, fmt.Errorf("node ID lookup failed: %s", err) + } + n = iter.Next() + if n == nil { + ws.Add(watchCh) + return 0, nil, nil + } + + idWatchCh := iter.WatchCh() + if iter.Next() != nil { + // Watch on the channel that did a node name lookup if we don't find + // anything when searching by Node ID. + ws.Add(watchCh) + return 0, nil, nil + } + + ws.Add(idWatchCh) } node := n.(*structs.Node) From 1d9c5a3efba988afa326f878775e864b0d8fb4ca Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 1 Feb 2017 15:41:10 -0800 Subject: [PATCH 6/8] Run a test of `NodeServices()` with a NodeID as an argument. --- consul/state/catalog.go | 5 ++-- consul/state/catalog_test.go | 49 +++++++++++++++++++++++------------- 2 files changed, 35 insertions(+), 19 deletions(-) diff --git a/consul/state/catalog.go b/consul/state/catalog.go index c0212c05a..e9280e979 100644 --- a/consul/state/catalog.go +++ b/consul/state/catalog.go @@ -725,11 +725,12 @@ func (s *StateStore) NodeServices(ws memdb.WatchSet, nodeNameOrID string) (uint6 } node := n.(*structs.Node) + nodeName := node.Node // Read all of the services - services, err := tx.Get("services", "node", nodeNameOrID) + services, err := tx.Get("services", "node", nodeName) if err != nil { - return 0, nil, fmt.Errorf("failed querying services for node %q: %s", nodeNameOrID, err) + return 0, nil, fmt.Errorf("failed querying services for node %q: %s", nodeName, err) } ws.Add(services.WatchCh()) diff --git a/consul/state/catalog_test.go b/consul/state/catalog_test.go index e142b0938..b8979c4ee 100644 --- a/consul/state/catalog_test.go +++ b/consul/state/catalog_test.go @@ -204,6 +204,8 @@ func TestStateStore_EnsureRegistration_Restore(t *testing.T) { Node: "node1", Address: "1.2.3.4", } + nodeID := string(req.ID) + nodeName := string(req.Node) restore := s.Restore() if err := restore.Registration(1, req); err != nil { t.Fatalf("err: %s", err) @@ -211,17 +213,26 @@ func TestStateStore_EnsureRegistration_Restore(t *testing.T) { restore.Commit() // Retrieve the node and verify its contents. - verifyNode := func() { - _, out, err := s.GetNode("node1") + verifyNode := func(nodeLookup string) { + _, out, err := s.GetNode(nodeLookup) if err != nil { t.Fatalf("err: %s", err) } - if out.Node != "node1" || out.Address != "1.2.3.4" || + if out == nil { + _, out, err = s.GetNodeID(types.NodeID(nodeLookup)) + if err != nil { + t.Fatalf("err: %s", err) + } + } + + if out == nil || out.Address != "1.2.3.4" || + !(out.Node == nodeLookup || string(out.ID) == nodeLookup) || out.CreateIndex != 1 || out.ModifyIndex != 1 { t.Fatalf("bad node returned: %#v", out) } } - verifyNode() + verifyNode(nodeID) + verifyNode(nodeName) // Add in a service definition. req.Service = &structs.NodeService{ @@ -237,8 +248,8 @@ func TestStateStore_EnsureRegistration_Restore(t *testing.T) { restore.Commit() // Verify that the service got registered. - verifyService := func() { - idx, out, err := s.NodeServices(nil, "node1") + verifyService := func(nodeLookup string) { + idx, out, err := s.NodeServices(nil, nodeLookup) if err != nil { t.Fatalf("err: %s", err) } @@ -258,7 +269,7 @@ func TestStateStore_EnsureRegistration_Restore(t *testing.T) { // Add in a top-level check. req.Check = &structs.HealthCheck{ - Node: "node1", + Node: nodeName, CheckID: "check1", Name: "check", } @@ -270,7 +281,7 @@ func TestStateStore_EnsureRegistration_Restore(t *testing.T) { // Verify that the check got registered. verifyCheck := func() { - idx, out, err := s.NodeChecks(nil, "node1") + idx, out, err := s.NodeChecks(nil, nodeName) if err != nil { t.Fatalf("err: %s", err) } @@ -281,19 +292,21 @@ func TestStateStore_EnsureRegistration_Restore(t *testing.T) { t.Fatalf("bad: %#v", out) } c := out[0] - if c.Node != "node1" || c.CheckID != "check1" || c.Name != "check" || + if c.Node != nodeName || c.CheckID != "check1" || c.Name != "check" || c.CreateIndex != 3 || c.ModifyIndex != 3 { t.Fatalf("bad check returned: %#v", c) } } - verifyNode() - verifyService() + verifyNode(nodeID) + verifyNode(nodeName) + verifyService(nodeID) + verifyService(nodeName) verifyCheck() // Add in another check via the slice. req.Checks = structs.HealthChecks{ &structs.HealthCheck{ - Node: "node1", + Node: nodeName, CheckID: "check2", Name: "check", }, @@ -305,10 +318,12 @@ func TestStateStore_EnsureRegistration_Restore(t *testing.T) { restore.Commit() // Verify that the additional check got registered. - verifyNode() - verifyService() + verifyNode(nodeID) + verifyNode(nodeName) + verifyService(nodeID) + verifyService(nodeName) func() { - idx, out, err := s.NodeChecks(nil, "node1") + idx, out, err := s.NodeChecks(nil, nodeName) if err != nil { t.Fatalf("err: %s", err) } @@ -319,13 +334,13 @@ func TestStateStore_EnsureRegistration_Restore(t *testing.T) { t.Fatalf("bad: %#v", out) } c1 := out[0] - if c1.Node != "node1" || c1.CheckID != "check1" || c1.Name != "check" || + if c1.Node != nodeName || c1.CheckID != "check1" || c1.Name != "check" || c1.CreateIndex != 3 || c1.ModifyIndex != 4 { t.Fatalf("bad check returned: %#v", c1) } c2 := out[1] - if c2.Node != "node1" || c2.CheckID != "check2" || c2.Name != "check" || + if c2.Node != nodeName || c2.CheckID != "check2" || c2.Name != "check" || c2.CreateIndex != 4 || c2.ModifyIndex != 4 { t.Fatalf("bad check returned: %#v", c2) } From 6844ebb43b88782d6d4f6ab39a1273f17ee57b35 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 1 Feb 2017 15:51:25 -0800 Subject: [PATCH 7/8] Treat a uuid prefix lookup error as a soft error, as if a node name lookup returned nil. Add a TODO to note where a future point of logging should occur once a logger is present. --- consul/state/catalog.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/consul/state/catalog.go b/consul/state/catalog.go index e9280e979..9e5a1768f 100644 --- a/consul/state/catalog.go +++ b/consul/state/catalog.go @@ -702,10 +702,13 @@ func (s *StateStore) NodeServices(ws memdb.WatchSet, nodeNameOrID string) (uint6 return 0, nil, nil } - // Attempt to lookup the node by it's node ID + // Attempt to lookup the node by its node ID iter, err := tx.Get("nodes", "uuid_prefix", nodeNameOrID) if err != nil { - return 0, nil, fmt.Errorf("node ID lookup failed: %s", err) + ws.Add(watchCh) + // TODO(sean@): We could/should log an error re: the uuid_prefix lookup + // failing once a logger has been introduced to the catalog. + return 0, nil, nil } n = iter.Next() if n == nil { From f8b64ec5f8eeb4874550c96217f10265ed55f55b Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 1 Feb 2017 15:51:25 -0800 Subject: [PATCH 8/8] Treat a uuid prefix lookup error as a soft error, as if a node name lookup returned nil. Add a TODO to note where a future point of logging should occur once a logger is present and a few additional comments to explain the program flow. --- consul/state/catalog.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/consul/state/catalog.go b/consul/state/catalog.go index 9e5a1768f..cb06283f5 100644 --- a/consul/state/catalog.go +++ b/consul/state/catalog.go @@ -710,16 +710,18 @@ func (s *StateStore) NodeServices(ws memdb.WatchSet, nodeNameOrID string) (uint6 // failing once a logger has been introduced to the catalog. return 0, nil, nil } + n = iter.Next() if n == nil { + // No nodes matched, even with the Node ID: add a watch on the node name. ws.Add(watchCh) return 0, nil, nil } idWatchCh := iter.WatchCh() if iter.Next() != nil { - // Watch on the channel that did a node name lookup if we don't find - // anything when searching by Node ID. + // More than one match present: Watch on the node name channel and return + // an empty result (node lookups can not be ambiguous). ws.Add(watchCh) return 0, nil, nil }