Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Query node_meta info using DNS TXT query API #3343

Closed
wants to merge 13 commits into from

Conversation

Projects
None yet
3 participants
@sodre
Copy link
Contributor

commented Aug 1, 2017

This is the node-part of feature request #2709

  • Update documentation
  • Return results for TXT query
  • Return results for ANY query
  • Return resuts for recursive DNS queries
  • Request review by hashicorp (@slackpad)
  • Merge & test with 0.9.1
  • Write RFC1464 encoder and test cases
  • Rebase and reorder commits

@sodre sodre force-pushed the zeroae:f-node-dns-txt-record branch 4 times, most recently from 14b50cc to d120880 Aug 2, 2017

switch {
case ipv4 != nil && (qType == dns.TypeANY || qType == dns.TypeA):
return []dns.RR{&dns.A{
records = append(records, &dns.A{

This comment has been minimized.

Copy link
@sodre

sodre Aug 5, 2017

Author Contributor

We have to append in order to support ANY queries when the node also has metadata key-values

@@ -57,8 +57,9 @@ we can instead use `foo.node.consul.` This convention allows for terse
syntax where appropriate while supporting queries of nodes in remote
datacenters as necessary.

For a node lookup, the only records returned are A records containing
the IP address of the node.
For a node lookup, the only records returned are A and AAAA records

This comment has been minimized.

Copy link
@sodre

sodre Aug 5, 2017

Author Contributor

I added language for 'AAAA' records as well since it is in available in the code. Feel free to remove that in case it is not officially supported yet.

@sodre
Copy link
Contributor Author

left a comment

@slackpad,

I would you like your feedback on this PR when you get a chance.

@sodre sodre changed the title [WIP] Query node_meta info using DNS TXT query API Query node_meta info using DNS TXT query API Aug 5, 2017

@sodre sodre force-pushed the zeroae:f-node-dns-txt-record branch 6 times, most recently from 6378987 to eecaf27 Aug 5, 2017

@magiconair

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2017

I can have a look at this since I'm in that codepath right now. Please note that we will merge #3353 first.

@magiconair magiconair self-requested a review Aug 8, 2017

@magiconair magiconair self-assigned this Aug 8, 2017

@magiconair magiconair removed their request for review Aug 8, 2017

@sodre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2017

The changes were in more places than I thought, it will take some time to fix.

@sodre sodre force-pushed the zeroae:f-node-dns-txt-record branch from 980c1ae to eecaf27 Aug 9, 2017

@sodre sodre changed the title Query node_meta info using DNS TXT query API [WIP] Query node_meta info using DNS TXT query API Aug 9, 2017

@sodre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2017

@magiconair,
There was as an internal API change/refactoring which removed the Node struct out of the formatNodeRecord function.

In order to solve this conflict, I will likely need to change every call to formatNodeRecord to include node.Meta.

Please advise if you are okay with that change.

@sodre sodre force-pushed the zeroae:f-node-dns-txt-record branch from eecaf27 to f498c53 Aug 9, 2017

@sodre
Copy link
Contributor Author

left a comment

Include justification for changing the formatNodeRecord signature back to 0.9.0 branch.

@@ -320,7 +320,7 @@ func (d *DNSServer) nameservers(edns bool) (ns []dns.RR, extra []dns.RR) {
ns = append(ns, nsrr)

// A or AAAA glue record
glue := d.formatNodeRecord(ip.String(), name, dns.TypeANY, d.config.NodeTTL, edns)
glue := d.formatNodeRecord(nil, ip.String(), name, dns.TypeANY, d.config.NodeTTL, edns)

This comment has been minimized.

Copy link
@sodre

sodre Aug 9, 2017

Author Contributor

Undo formatNodeRecord change from commit 76319f7

This comment has been minimized.

Copy link
@magiconair

magiconair Aug 9, 2017

Contributor

This function signature is starting to get a bit out of hand but that's a story for another day.

This comment has been minimized.

Copy link
@preetapan

preetapan Aug 9, 2017

Member

I removed node from there in that commit because it was an unused param. why are we adding it back?

This comment has been minimized.

Copy link
@magiconair

magiconair Aug 9, 2017

Contributor

Because he needs it to get to the meta data of the node. We might also just pass the map in.

@@ -509,45 +509,58 @@ RPC:
n := out.NodeServices.Node
edns := req.IsEdns0() != nil
addr := d.agent.TranslateAddress(datacenter, n.Address, n.TaggedAddresses)
records := d.formatNodeRecord(addr, req.Question[0].Name, qType, d.config.NodeTTL, edns)
records := d.formatNodeRecord(out.NodeServices.Node, addr, req.Question[0].Name, qType, d.config.NodeTTL, edns)

This comment has been minimized.

Copy link
@sodre

sodre Aug 9, 2017

Author Contributor

Undo formatNodeRecord change from commit 76319f7

}

// formatNodeRecord takes a Node and returns an A, AAAA, TXT or CNAME record
func (d *DNSServer) formatNodeRecord(node *structs.Node, addr, qName string, qType uint16, ttl time.Duration, edns bool) (records []dns.RR) {

This comment has been minimized.

Copy link
@sodre

sodre Aug 9, 2017

Author Contributor

Undo formatNodeRecord change from commit 76319f7

@@ -575,6 +588,21 @@ func (d *DNSServer) formatNodeRecord(addr, qName string, qType uint16, ttl time.
}
}
}

if node != nil && len(node.Meta) > 0 && (qType == dns.TypeANY || qType == dns.TypeTXT) {

This comment has been minimized.

Copy link
@sodre

sodre Aug 9, 2017

Author Contributor

Add test in case we are called from line 323.

This comment has been minimized.

Copy link
@magiconair

magiconair Aug 9, 2017

Contributor

len(node.Meta) > 0 is redundant

@@ -896,7 +924,7 @@ func (d *DNSServer) serviceNodeRecords(dc string, nodes structs.CheckServiceNode
handled[addr] = struct{}{}

// Add the node record
records := d.formatNodeRecord(addr, qName, qType, ttl, edns)
records := d.formatNodeRecord(node.Node, addr, qName, qType, ttl, edns)

This comment has been minimized.

Copy link
@sodre

sodre Aug 9, 2017

Author Contributor

Undo formatNodeRecord change from commit 76319f7

@@ -940,7 +968,7 @@ func (d *DNSServer) serviceSRVRecords(dc string, nodes structs.CheckServiceNodes
}

// Add the extra record
records := d.formatNodeRecord(addr, srvRec.Target, dns.TypeANY, ttl, edns)
records := d.formatNodeRecord(node.Node, addr, srvRec.Target, dns.TypeANY, ttl, edns)

This comment has been minimized.

Copy link
@sodre

sodre Aug 9, 2017

Author Contributor

Undo formatNodeRecord change from commit 76319f7

@sodre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2017

This is ready for review again.
The travis-ci build that failed looked unrelated to my changes.

@sodre sodre changed the title [WIP] Query node_meta info using DNS TXT query API Query node_meta info using DNS TXT query API Aug 9, 2017

if records != nil {
resp.Answer = append(resp.Answer, records...)
}
}

// formatNodeRecord takes a Node and returns an A, AAAA, or CNAME record
func (d *DNSServer) formatNodeRecord(addr, qName string, qType uint16, ttl time.Duration, edns bool) (records []dns.RR) {
// formatTxtRecords takes a kv-map and returns it as "[k=]v" for non-empty k not starting with rfc1035-

This comment has been minimized.

Copy link
@magiconair

magiconair Aug 9, 2017

Contributor
  • I'd suggest that you describe what the function does and not how it does that, e.g.

formatTxtRecords encodes the map values according to RFC 1464.

  • formatTxtRecords isn't a good name since it doesn't return records ([]*dns.RR) but only the values of TXT records. Maybe name it encodeKVasRFC1464 or something similar. I didn't think this all the way through but you should get the drift.

  • The comment does not match the code since k can be empty and you'd end up with =v.

  • RFC 1464 says that key names with special characters must be encoded:

https://www.ietf.org/rfc/rfc1464.txt

Attribute Names

Any printable ASCII character is permitted for the attribute name.
If an equals sign is embedded in the attribute name, it must be
quoted with a preceding grave accent (or backquote: "`"). A
backquote must also be quoted with an additional "`".

also further down:

Leading and trailing whitespace (spaces and tabs) in the attribute
name are ignored unless they are quoted (with a ""). For example, "abc" matches " abc<tab>" but does not match ``" abc"``.

Note that most DNS server implementations require a backslash () or
double quote (") in a text string to be quoted with a preceding
backslash. Accent grave ("`") was chosen as a quoting character in
this syntax to avoid confusion with "" (and remove the need for
confusing strings that include sequences like "\\").

and

Attribute Values

All printable ASCII characters are permitted in the attribute value.
No characters need to be quoted with a "`". In other words, the
first unquoted equals sign in the TXT record is the name/value
delimiter. All subsequent characters are part of the value.

Once again, note that in most implementations the backslash character
is an active quoting character (and must, itself, be quoted).

  • I'd prefer a switch statement over if/else

  • Please add a table driven test for this function especially the edge cases

  • When looking at #2709 I've noticed the use of datacenter in the example. Would that be the consul datacenter value and if yes how does this get in here? We may want to think about some expansion language like ${DC} or similar to fill in values from the other consul node data but that may be overkill.

This comment has been minimized.

Copy link
@sodre

sodre Aug 10, 2017

Author Contributor

This PR only closes the TXT lookup side for Nodes. I am open to suggestions on how to solve it at the datacenter level.

My initial thought was to use the KV system, but that looked like too much of a hack.

Are there any plans to make data-center level metadata values available?

This comment has been minimized.

Copy link
@magiconair

magiconair Aug 10, 2017

Contributor

I was referring to a value containing the configured datacenter of consul. Lets consider this when it is requested and get this one done first.

@@ -575,6 +588,21 @@ func (d *DNSServer) formatNodeRecord(addr, qName string, qType uint16, ttl time.
}
}
}

if node != nil && len(node.Meta) > 0 && (qType == dns.TypeANY || qType == dns.TypeTXT) {

This comment has been minimized.

Copy link
@magiconair

magiconair Aug 9, 2017

Contributor

len(node.Meta) > 0 is redundant

t.Fatalf("err: %v", err)
}

// Should have the 1 TXT record reply

This comment has been minimized.

Copy link
@magiconair

magiconair Aug 9, 2017

Contributor

Pls use verify.Values to compare full responses, if possible.

}

// Should have 1 A and 1 TXT record
if len(in.Answer) != 2 {

This comment has been minimized.

Copy link
@magiconair

magiconair Aug 9, 2017

Contributor

Pls verify.Values, if possible.

@@ -334,8 +347,8 @@ func TestDNS_NodeLookup_CNAME(t *testing.T) {
t.Fatalf("err: %v", err)
}

// Should have the service record, CNAME record + A record
if len(in.Answer) != 3 {
// Should have the service record, CNAME record + A + TXT record

This comment has been minimized.

Copy link
@magiconair

magiconair Aug 9, 2017

Contributor

Should have the service, CNAME, A, and TXT records

@@ -320,7 +320,7 @@ func (d *DNSServer) nameservers(edns bool) (ns []dns.RR, extra []dns.RR) {
ns = append(ns, nsrr)

// A or AAAA glue record
glue := d.formatNodeRecord(ip.String(), name, dns.TypeANY, d.config.NodeTTL, edns)
glue := d.formatNodeRecord(nil, ip.String(), name, dns.TypeANY, d.config.NodeTTL, edns)

This comment has been minimized.

Copy link
@magiconair

magiconair Aug 9, 2017

Contributor

This function signature is starting to get a bit out of hand but that's a story for another day.

@@ -464,9 +464,9 @@ INVALID:

// nodeLookup is used to handle a node query
func (d *DNSServer) nodeLookup(network, datacenter, node string, req, resp *dns.Msg) {
// Only handle ANY, A and AAAA type requests
// Only handle ANY, A, AAAA and TXT type requests

This comment has been minimized.

Copy link
@magiconair

magiconair Aug 9, 2017

Contributor

Oxford comma?

@sodre sodre force-pushed the zeroae:f-node-dns-txt-record branch 3 times, most recently from 5abd2fd to 3e3269e Aug 11, 2017

sodre added some commits Aug 1, 2017

Add test for querying Node.Meta with DNS TXT
  - Lookup TXT records using recursive lookups
  - Expect TXT record equal to value if key starts with rfc1035-
  - Expect TXT record in rfc1464 otherwise, i.e. (k=v)

ref #2709
Update docs to include support for TXT records
  - Add explanation to the difference between RFC1035
    and RFC1464 queries.
Refactor formatTxtRecords as encodeKVasRFC1464
  - Move the logic of rfc1035 out of the encoding function
  - Left basic version of encodingKV as 'k=v'

@sodre sodre force-pushed the zeroae:f-node-dns-txt-record branch from b544089 to 39e37d0 Aug 11, 2017

@sodre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2017

I added the tests from RFC1464.
Because of the restrictions on node-meta Some of the tests cases from RFC1464 would never get executed in the live system, e.g. '=', or empty keys.

@sodre sodre force-pushed the zeroae:f-node-dns-txt-record branch from 7c2e6ce to faf31cb Aug 11, 2017

@sodre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2017

@magiconair
Do you need any additional changes? If not I would like to reorder some commits and squash a few things before the final merge.

Thanks!

@magiconair

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2017

@sodre I didn't get a chance to review your latest changes yet. I'll ping you when I'm done. If you want and there are no changes then I can do the re-order squash for you if you let me know what you want to do.

@sodre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2017

@magiconair
Can we get a milestone for you to review this piece? I want to avoid fixing the conflict now, and another one popping up because the merge is taking too long.

@magiconair

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2017

Hi @sodre, sorry for the delay. I was on vacation for a couple of weeks and have two large things I'm working on right now. That's why I'm avoiding to context switch ATM. I'll try to review it this week.

@magiconair magiconair added this to the 1.0 milestone Sep 22, 2017

@magiconair

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2017

I've rebased the code and fixed the tests but I'm not getting the result I thought:

$ consul agent -dev -node-meta "baz:bang" -node-meta 'blurb' -node foo

$ dig @localhost -p 8600 foo.node.consul. any

; <<>> DiG 9.8.3-P1 <<>> @localhost -p 8600 foo.node.consul. any
; (2 servers found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 32778
;; flags: qr aa rd; QUERY: 1, ANSWER: 4, AUTHORITY: 0, ADDITIONAL: 0
;; WARNING: recursion requested but not available

;; QUESTION SECTION:
;foo.node.consul.		IN	ANY

;; ANSWER SECTION:
foo.node.consul.	0	IN	A	127.0.0.1
foo.node.consul.	0	IN	TXT	"consul-network-segment="
foo.node.consul.	0	IN	TXT	"blurb="
foo.node.consul.	0	IN	TXT	"baz=bang"

I thought meta keys without a value shouldn't have a trailing = sign.

@magiconair

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2017

I've pushed my changes to the https://github.com/hashicorp/consul/tree/zeroae-f-node-dns-txt-record-magiconair branch. I'll see whether I can fix this later today.

@magiconair

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2017

Just re-read https://tools.ietf.org/html/rfc1464 so I think we're good.

   Examples

   <sp> indicates a space character.

   Attribute    Attribute       Internal Form           External Form
   Name         Value           (server to resolver)    (TXT record)

   color        blue            color=blue              "color=blue"
   equation     a=4             equation=a=4            "equation=a=4"
   a=a          true            a`=a=true               "a`=a=true"
   a\=a false           a\`=a=false             "a\\`=a=false"
   =            \=              `==\=                   "`==\\="

   string       "Cat"           string="Cat"            "string=\"Cat\""
   string2      `abc`           string2=``abc``         "string2=``abc``"


   novalue                      novalue=                "novalue="   <-- this


   a b          c d             a b=c d                 "a b=c d"
   abc<sp>      123<sp>         abc` =123<sp>           "abc` =123 "

magiconair added a commit that referenced this pull request Sep 28, 2017

@magiconair

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2017

rebased and merged to master.

@magiconair magiconair closed this Sep 28, 2017

jhmartin added a commit to jhmartin/consul that referenced this pull request Nov 3, 2017

slackpad added a commit that referenced this pull request Nov 3, 2017

Merge pull request #3643 from jhmartin/changelog_rfc1434
Update CHANGELOG, as GH-3343 references RFC1464 not 1434.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.