Query node_meta info using DNS TXT query API #3343

sodre · 2017-08-01T07:15:45Z

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 · 2017-08-05T06:31:27Z

agent/dns.go

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


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

sodre · 2017-08-05T06:33:18Z

website/source/docs/agent/dns.html.md

@@ -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 


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

@slackpad,

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

magiconair · 2017-08-08T08:24:59Z

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

sodre · 2017-08-09T00:54:25Z

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

sodre · 2017-08-09T06:53:07Z

@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

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

sodre · 2017-08-09T07:25:19Z

agent/dns.go

@@ -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)


Undo formatNodeRecord change from commit 76319f7

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

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

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

sodre · 2017-08-09T07:25:29Z

agent/dns.go

@@ -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)


Undo formatNodeRecord change from commit 76319f7

sodre · 2017-08-09T07:25:59Z

agent/dns.go

+}
+
+// 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) {


Undo formatNodeRecord change from commit 76319f7

sodre · 2017-08-09T07:27:43Z

agent/dns.go

@@ -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) {


Add test in case we are called from line 323.

len(node.Meta) > 0 is redundant

sodre · 2017-08-09T07:28:24Z

agent/dns.go

@@ -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)


Undo formatNodeRecord change from commit 76319f7

sodre · 2017-08-09T07:28:31Z

agent/dns.go

@@ -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)


Undo formatNodeRecord change from commit 76319f7

sodre · 2017-08-09T07:34:44Z

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

magiconair · 2017-08-09T10:10:12Z

agent/dns.go

 	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-


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 Feature request: DNS TXT resource records #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 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?

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.

magiconair · 2017-08-09T10:11:51Z

agent/dns.go

@@ -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) {


len(node.Meta) > 0 is redundant

magiconair · 2017-08-09T10:20:53Z

agent/dns_test.go

+		t.Fatalf("err: %v", err)
+	}
+
+	// Should have the 1 TXT record reply


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

magiconair · 2017-08-09T10:21:12Z

agent/dns_test.go

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


Pls verify.Values, if possible.

magiconair · 2017-08-09T10:33:58Z

agent/dns_test.go

@@ -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


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

magiconair · 2017-08-09T10:39:56Z

agent/dns.go

@@ -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 function signature is starting to get a bit out of hand but that's a story for another day.

magiconair · 2017-08-09T10:40:15Z

agent/dns.go

@@ -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


Oxford comma?

- Add explanation to the difference between RFC1035 and RFC1464 queries.

- 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

- Move the logic of rfc1035 out of the encoding function - Left basic version of encodingKV as 'k=v'

sodre · 2017-08-11T02:54:11Z

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 · 2017-08-16T00:50:10Z

@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 · 2017-08-16T15:17:53Z

@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 · 2017-08-29T22:56:54Z

@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 · 2017-08-30T10:32:04Z

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 · 2017-09-26T12:48:10Z

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 · 2017-09-26T12:49:54Z

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 · 2017-09-28T10:42:49Z

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 · 2017-09-28T10:54:49Z

rebased and merged to master.

Update CHANGELOG, as GH-3343 references RFC1464 not 1434.

sodre commented Aug 5, 2017

View reviewed changes

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

magiconair self-requested a review August 8, 2017 08:25

magiconair self-assigned this Aug 8, 2017

magiconair removed their request for review August 8, 2017 08:25

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 commented Aug 9, 2017

View reviewed changes

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

magiconair reviewed Aug 9, 2017

View reviewed changes

sodre and others added 11 commits August 10, 2017 22:29

Update docs to include support for TXT records

38bc9c1

- Add explanation to the difference between RFC1035 and RFC1464 queries.

Add test for querying Node.Meta with DNS TXT

cde5f0d

- 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

Add test for NoteLookup ANY request

31901c3

Return Node.Meta info using the DNS interface

22de633

Remove redundant check of Node.Meta size

b3689da

Fix editorial suggestions

67d07fd

Refactor formatTxtRecords as encodeKVasRFC1464

257eadb

- Move the logic of rfc1035 out of the encoding function - Left basic version of encodingKV as 'k=v'

Use verify for NodeLookup CNAME, and TXT tests

aa79189

Turn encodeKVasRFC1464 into a plain function

94b6443

Add RFC1464 tests

bf96e1e

Implement encodeKVasRFC1464 function

39e37d0

Update docs on RFC1464 vs RFC1035 options

faf31cb

magiconair added this to the 1.0 milestone Sep 22, 2017

Merge branch 'master' into f-node-dns-txt-record

629aa20

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

Merge pull request #3343 from zeroae/f-node-dns-txt-record

622bc84

magiconair closed this Sep 28, 2017

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

Update CHANGELOG, as hashicorpGH-3343 references RFC1464 not 1434.

31cdb66

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

Merge pull request #3643 from jhmartin/changelog_rfc1434

ec09009

Update CHANGELOG, as GH-3343 references RFC1464 not 1434.

slackpad mentioned this pull request Jan 5, 2018

Feature request: DNS TXT resource records #2709

Closed

Query node_meta info using DNS TXT query API #3343

Query node_meta info using DNS TXT query API #3343

Conversation

sodre commented Aug 1, 2017 • edited by slackpad Loading

sodre Aug 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sodre left a comment

Choose a reason for hiding this comment

magiconair commented Aug 8, 2017

sodre commented Aug 9, 2017 • edited Loading

sodre commented Aug 9, 2017

sodre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sodre commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sodre commented Aug 11, 2017

sodre commented Aug 16, 2017

magiconair commented Aug 16, 2017

sodre commented Aug 29, 2017

magiconair commented Aug 30, 2017

magiconair commented Sep 26, 2017

magiconair commented Sep 26, 2017

magiconair commented Sep 28, 2017

magiconair commented Sep 28, 2017

sodre commented Aug 1, 2017 •

edited by slackpad

Loading

sodre Aug 5, 2017 •

edited

Loading

sodre commented Aug 9, 2017 •

edited

Loading