Merge pull request #4362 from hashicorp/bugfix/gh-4354

Ensure TXT RRs always end up in the Additional section except for ANY or TXT queries

agent/dns.go

@@ -376,8 +376,11 @@ func (d *DNSServer) nameservers(edns bool) (ns []dns.RR, extra []dns.RR) {
 		}
 		ns = append(ns, nsrr)
 
-		glue := d.formatNodeRecord(nil, addr, fqdn, dns.TypeANY, d.config.NodeTTL, edns, false)
+		glue, meta := d.formatNodeRecord(nil, addr, fqdn, dns.TypeANY, d.config.NodeTTL, edns)
 		extra = append(extra, glue...)
+		if meta != nil && d.config.NodeMetaTXT {
+			extra = append(extra, meta...)
+		}
 
 		// don't provide more than 3 servers
 		if len(ns) >= 3 {
@@ -592,10 +595,15 @@ RPC:
 	n := out.NodeServices.Node
 	edns := req.IsEdns0() != nil
 	addr := d.agent.TranslateAddress(datacenter, n.Address, n.TaggedAddresses)
-	records := d.formatNodeRecord(out.NodeServices.Node, addr, req.Question[0].Name, qType, d.config.NodeTTL, edns, true)
+	records, meta := d.formatNodeRecord(out.NodeServices.Node, addr, req.Question[0].Name, qType, d.config.NodeTTL, edns)
 	if records != nil {
 		resp.Answer = append(resp.Answer, records...)
 	}
+	if meta != nil && (qType == dns.TypeANY || qType == dns.TypeTXT) {
+		resp.Answer = append(resp.Answer, meta...)
+	} else if meta != nil && d.config.NodeMetaTXT {
+		resp.Extra = append(resp.Extra, meta...)
+	}
 }
 
 // encodeKVasRFC1464 encodes a key-value pair according to RFC1464
@@ -619,8 +627,12 @@ func encodeKVasRFC1464(key, value string) (txt string) {
 	return key + "=" + value
 }
 
-// formatNodeRecord takes a Node and returns an A, AAAA, TXT or CNAME record
+// formatNodeRecord takes a Node and returns the RRs associated with that node
-func (d *DNSServer) formatNodeRecord(node *structs.Node, addr, qName string, qType uint16, ttl time.Duration, edns, answer bool) (records []dns.RR) {
+//
+// The return value is two slices. The first slice is the main answer slice (containing the A, AAAA, CNAME) RRs for the node
+// and the second slice contains any TXT RRs created from the node metadata. It is up to the caller to determine where the
+// generated RRs should go and if they should be used at all.
+func (d *DNSServer) formatNodeRecord(node *structs.Node, addr, qName string, qType uint16, ttl time.Duration, edns bool) (records, meta []dns.RR) {
 	// Parse the IP
 	ip := net.ParseIP(addr)
 	var ipv4 net.IP
@@ -681,26 +693,14 @@ func (d *DNSServer) formatNodeRecord(node *structs.Node, addr, qName string, qTy
 		}
 	}
 
-	node_meta_txt := false
+	if node != nil {
-
-	if node == nil {
-		node_meta_txt = false
-	} else if answer {
-		node_meta_txt = true
-	} else {
-		// Use configuration when the TXT RR would
-		// end up in the Additional section of the
-		// DNS response
-		node_meta_txt = d.config.NodeMetaTXT
-	}
-
-	if node_meta_txt {
 		for key, value := range node.Meta {
 			txt := value
 			if !strings.HasPrefix(strings.ToLower(key), "rfc1035-") {
 				txt = encodeKVasRFC1464(key, value)
 			}
-			records = append(records, &dns.TXT{
+
+			meta = append(meta, &dns.TXT{
 				Hdr: dns.RR_Header{
 					Name:   qName,
 					Rrtype: dns.TypeTXT,
@@ -712,7 +712,7 @@ func (d *DNSServer) formatNodeRecord(node *structs.Node, addr, qName string, qTy
 		}
 	}
 
-	return records
+	return records, meta
 }
 
 // indexRRs populates a map which indexes a given list of RRs by name. NOTE that
@@ -1167,9 +1167,21 @@ func (d *DNSServer) serviceNodeRecords(dc string, nodes structs.CheckServiceNode
 		handled[addr] = struct{}{}
 
 		// Add the node record
-		records := d.formatNodeRecord(node.Node, addr, qName, qType, ttl, edns, true)
+		had_answer := false
+		records, meta := d.formatNodeRecord(node.Node, addr, qName, qType, ttl, edns)
 		if records != nil {
 			resp.Answer = append(resp.Answer, records...)
+			had_answer = true
+		}
+
+		if meta != nil && (qType == dns.TypeANY || qType == dns.TypeTXT) {
+			resp.Answer = append(resp.Answer, meta...)
+			had_answer = true
+		} else if meta != nil && d.config.NodeMetaTXT {
+			resp.Extra = append(resp.Extra, meta...)
+		}
+
+		if had_answer {
 			count++
 			if count == d.config.ARecordLimit {
 				// We stop only if greater than 0 or we reached the limit
@@ -1216,7 +1228,7 @@ func (d *DNSServer) serviceSRVRecords(dc string, nodes structs.CheckServiceNodes
 		}
 
 		// Add the extra record
-		records := d.formatNodeRecord(node.Node, addr, srvRec.Target, dns.TypeANY, ttl, edns, false)
+		records, meta := d.formatNodeRecord(node.Node, addr, srvRec.Target, dns.TypeANY, ttl, edns)
 		if len(records) > 0 {
 			// Use the node address if it doesn't differ from the service address
 			if addr == node.Node.Address {
@@ -1246,6 +1258,10 @@ func (d *DNSServer) serviceSRVRecords(dc string, nodes structs.CheckServiceNodes
 					resp.Extra = append(resp.Extra, records...)
 				}
 			}
+
+			if meta != nil && d.config.NodeMetaTXT {
+				resp.Extra = append(resp.Extra, meta...)
+			}
 		}
 	}
 }

agent/dns_test.go

@@ -178,6 +178,9 @@ func TestDNS_NodeLookup(t *testing.T) {
 		TaggedAddresses: map[string]string{
 			"wan": "127.0.0.2",
 		},
+		NodeMeta: map[string]string{
+			"key": "value",
+		},
 	}
 
 	var out struct{}
@@ -190,24 +193,40 @@ func TestDNS_NodeLookup(t *testing.T) {
 
 	c := new(dns.Client)
 	in, _, err := c.Exchange(m, a.DNSAddr())
-	if err != nil {
+	require.NoError(t, err)
-		t.Fatalf("err: %v", err)
+	require.Len(t, in.Answer, 2)
-	}
+	require.Len(t, in.Extra, 0)
-
-	if len(in.Answer) != 1 {
-		t.Fatalf("Bad: %#v", in)
-	}
 
 	aRec, ok := in.Answer[0].(*dns.A)
-	if !ok {
+	require.True(t, ok, "First answer is not an A record")
-		t.Fatalf("Bad: %#v", in.Answer[0])
+	require.Equal(t, "127.0.0.1", aRec.A.String())
-	}
+	require.Equal(t, uint32(0), aRec.Hdr.Ttl)
-	if aRec.A.String() != "127.0.0.1" {
+
-		t.Fatalf("Bad: %#v", in.Answer[0])
+	txt, ok := in.Answer[1].(*dns.TXT)
-	}
+	require.True(t, ok, "Second answer is not a TXT record")
-	if aRec.Hdr.Ttl != 0 {
+	require.Len(t, txt.Txt, 1)
-		t.Fatalf("Bad: %#v", in.Answer[0])
+	require.Equal(t, "key=value", txt.Txt[0])
-	}
+
+	// Re-do the query, but only for an A RR
+
+	m = new(dns.Msg)
+	m.SetQuestion("foo.node.consul.", dns.TypeA)
+
+	c = new(dns.Client)
+	in, _, err = c.Exchange(m, a.DNSAddr())
+	require.NoError(t, err)
+	require.Len(t, in.Answer, 1)
+	require.Len(t, in.Extra, 1)
+
+	aRec, ok = in.Answer[0].(*dns.A)
+	require.True(t, ok, "Answer is not an A record")
+	require.Equal(t, "127.0.0.1", aRec.A.String())
+	require.Equal(t, uint32(0), aRec.Hdr.Ttl)
+
+	txt, ok = in.Extra[0].(*dns.TXT)
+	require.True(t, ok, "Extra record is not a TXT record")
+	require.Len(t, txt.Txt, 1)
+	require.Equal(t, "key=value", txt.Txt[0])
 
 	// Re-do the query, but specify the DC
 	m = new(dns.Msg)
@@ -215,24 +234,17 @@ func TestDNS_NodeLookup(t *testing.T) {
 
 	c = new(dns.Client)
 	in, _, err = c.Exchange(m, a.DNSAddr())
-	if err != nil {
+	require.NoError(t, err)
-		t.Fatalf("err: %v", err)
+	require.Len(t, in.Answer, 2)
-	}
+	require.Len(t, in.Extra, 0)
-
-	if len(in.Answer) != 1 {
-		t.Fatalf("Bad: %#v", in)
-	}
 
 	aRec, ok = in.Answer[0].(*dns.A)
-	if !ok {
+	require.True(t, ok, "First answer is not an A record")
-		t.Fatalf("Bad: %#v", in.Answer[0])
+	require.Equal(t, "127.0.0.1", aRec.A.String())
-	}
+	require.Equal(t, uint32(0), aRec.Hdr.Ttl)
-	if aRec.A.String() != "127.0.0.1" {
+
-		t.Fatalf("Bad: %#v", in.Answer[0])
+	txt, ok = in.Answer[1].(*dns.TXT)
-	}
+	require.True(t, ok, "Second answer is not a TXT record")
-	if aRec.Hdr.Ttl != 0 {
-		t.Fatalf("Bad: %#v", in.Answer[0])
-	}
 
 	// lookup a non-existing node, we should receive a SOA
 	m = new(dns.Msg)
@@ -240,22 +252,11 @@ func TestDNS_NodeLookup(t *testing.T) {
 
 	c = new(dns.Client)
 	in, _, err = c.Exchange(m, a.DNSAddr())
-	if err != nil {
+	require.NoError(t, err)
-		t.Fatalf("err: %v", err)
+	require.Len(t, in.Ns, 1)
-	}
-
-	if len(in.Ns) != 1 {
-		t.Fatalf("Bad: %#v %#v", in, len(in.Answer))
-	}
-
 	soaRec, ok := in.Ns[0].(*dns.SOA)
-	if !ok {
+	require.True(t, ok, "NS RR is not a SOA record")
-		t.Fatalf("Bad: %#v", in.Ns[0])
+	require.Equal(t, uint32(0), soaRec.Hdr.Ttl)
-	}
-	if soaRec.Hdr.Ttl != 0 {
-		t.Fatalf("Bad: %#v", in.Ns[0])
-	}
-
 }
 
 func TestDNS_CaseInsensitiveNodeLookup(t *testing.T) {
@@ -598,6 +599,41 @@ func TestDNS_NodeLookup_ANY_DontSuppressTXT(t *testing.T) {
 	verify.Values(t, "answer", in.Answer, wantAnswer)
 }
 
+func TestDNS_NodeLookup_A_SuppressTXT(t *testing.T) {
+	a := NewTestAgent(t.Name(), `dns_config = { enable_additional_node_meta_txt = false }`)
+	defer a.Shutdown()
+
+	args := &structs.RegisterRequest{
+		Datacenter: "dc1",
+		Node:       "bar",
+		Address:    "127.0.0.1",
+		NodeMeta: map[string]string{
+			"key": "value",
+		},
+	}
+
+	var out struct{}
+	require.NoError(t, a.RPC("Catalog.Register", args, &out))
+
+	m := new(dns.Msg)
+	m.SetQuestion("bar.node.consul.", dns.TypeA)
+
+	c := new(dns.Client)
+	in, _, err := c.Exchange(m, a.DNSAddr())
+	require.NoError(t, err)
+
+	wantAnswer := []dns.RR{
+		&dns.A{
+			Hdr: dns.RR_Header{Name: "bar.node.consul.", Rrtype: dns.TypeA, Class: dns.ClassINET, Rdlength: 0x4},
+			A:   []byte{0x7f, 0x0, 0x0, 0x1}, // 127.0.0.1
+		},
+	}
+	verify.Values(t, "answer", in.Answer, wantAnswer)
+
+	// ensure TXT RR suppression
+	require.Len(t, in.Extra, 0)
+}
+
 func TestDNS_EDNS0(t *testing.T) {
 	t.Parallel()
 	a := NewTestAgent(t.Name(), "")