DNS lookup by Consul node ID #2702
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If the length isn't `36`, return `false` immediately before firing up the regexp engine.
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
```
slackpad
suggested changes
Feb 1, 2017
consul/state/schema.go
Outdated
| @@ -78,6 +78,14 @@ func nodesTableSchema() *memdb.TableSchema { | |||
| Lowercase: true, | |||
| }, | |||
| }, | |||
| "uuid": &memdb.IndexSchema{ | |||
| Name: "uuid", | |||
| AllowMissing: false, | |||
There was a problem hiding this comment.
This needs to be true since old nodes won't have this yet.
| } | ||
| return types.NodeID(id) | ||
| } | ||
|
|
||
| func TestStateStore_EnsureRegistration(t *testing.T) { |
There was a problem hiding this comment.
I think it's worth it to add another test, or a clause here that calls NodeServices() with a UUID.
consul/state/catalog.go
Outdated
| 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) |
There was a problem hiding this comment.
I think you wan't to use the iterator form here to make sure the lookup isn't ambiguous:
iter, err := tx.Get("nodes", "uuid_prefix", nodeName)
n = iter.Next()
if n != nil {
if iter.Next() == nil {
// got it
} else {
// it was ambiguous, return nil
}
}
consul/state/catalog.go
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
I'd rename nodeName to nodeNameOrID in here.
slackpad
reviewed
Feb 1, 2017
| } | ||
|
|
||
| idWatchCh := iter.WatchCh() | ||
| if iter.Next() != nil { |
There was a problem hiding this comment.
This is probably worth a comment about trying to make sure there's not another node that matches the prefix. It's pretty obscure logic :-)
There was a problem hiding this comment.
Also wrt the comment below - it's not that we didn't find anything - there were too many things.
slackpad
reviewed
Feb 1, 2017
consul/state/catalog.go
Outdated
| return 0, nil, nil | ||
| } | ||
|
|
||
| // Attempt to lookup the node by it's node ID |
slackpad
reviewed
Feb 1, 2017
consul/state/catalog.go
Outdated
| // 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) |
There was a problem hiding this comment.
This case is probably a soft error (maybe debug log it) where we should just add the watch channel and return 0, nil, nil. If they type in a bad name they could hit this and get a confusing error.
…ookup returned nil. Add a TODO to note where a future point of logging should occur once a logger is present.
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.
slackpad
approved these changes
Feb 2, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Assuming the following output from a consul agent:
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:
chosen as an arbitrary number)
returned.