From cbf8f14451a8ffbda1ac1e75d1f3925796894901 Mon Sep 17 00:00:00 2001 From: Matt Keeler Date: Mon, 9 Jul 2018 11:41:58 -0400 Subject: [PATCH] Ensure TXT RRs always end up in the Additional section except for ANY or TXT queries This also changes where the enforcement of the enable_additional_node_meta_txt configuration gets applied. formatNodeRecord returns the main RRs and the meta/TXT RRs in separate slices. Its then up to the caller to add to the appropriate sections or not. --- agent/dns.go | 60 +++++++++++++-------- agent/dns_test.go | 130 +++++++++++++++++++++++++++++----------------- 2 files changed, 121 insertions(+), 69 deletions(-) diff --git a/agent/dns.go b/agent/dns.go index 2372d16c86..bd5fe820d8 100644 --- a/agent/dns.go +++ b/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 -func (d *DNSServer) formatNodeRecord(node *structs.Node, addr, qName string, qType uint16, ttl time.Duration, edns, answer bool) (records []dns.RR) { +// formatNodeRecord takes a Node and returns the RRs associated with that node +// +// 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 { - 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 { + if node != nil { 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...) + } } } } diff --git a/agent/dns_test.go b/agent/dns_test.go index bc6fdaadb0..48143d0203 100644 --- a/agent/dns_test.go +++ b/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 { - t.Fatalf("err: %v", err) - } - - if len(in.Answer) != 1 { - t.Fatalf("Bad: %#v", in) - } + require.NoError(t, err) + require.Len(t, in.Answer, 2) + require.Len(t, in.Extra, 0) aRec, ok := in.Answer[0].(*dns.A) - if !ok { - t.Fatalf("Bad: %#v", in.Answer[0]) - } - if aRec.A.String() != "127.0.0.1" { - t.Fatalf("Bad: %#v", in.Answer[0]) - } - if aRec.Hdr.Ttl != 0 { - t.Fatalf("Bad: %#v", in.Answer[0]) - } + require.True(t, ok, "First 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.Answer[1].(*dns.TXT) + require.True(t, ok, "Second answer is not a TXT record") + require.Len(t, txt.Txt, 1) + 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 { - t.Fatalf("err: %v", err) - } - - if len(in.Answer) != 1 { - t.Fatalf("Bad: %#v", in) - } + require.NoError(t, err) + require.Len(t, in.Answer, 2) + require.Len(t, in.Extra, 0) aRec, ok = in.Answer[0].(*dns.A) - if !ok { - t.Fatalf("Bad: %#v", in.Answer[0]) - } - if aRec.A.String() != "127.0.0.1" { - t.Fatalf("Bad: %#v", in.Answer[0]) - } - if aRec.Hdr.Ttl != 0 { - t.Fatalf("Bad: %#v", in.Answer[0]) - } + require.True(t, ok, "First 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.Answer[1].(*dns.TXT) + require.True(t, ok, "Second answer is not a TXT record") // 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 { - t.Fatalf("err: %v", err) - } - - if len(in.Ns) != 1 { - t.Fatalf("Bad: %#v %#v", in, len(in.Answer)) - } - + require.NoError(t, err) + require.Len(t, in.Ns, 1) soaRec, ok := in.Ns[0].(*dns.SOA) - if !ok { - t.Fatalf("Bad: %#v", in.Ns[0]) - } - if soaRec.Hdr.Ttl != 0 { - t.Fatalf("Bad: %#v", in.Ns[0]) - } - + require.True(t, ok, "NS RR is not a SOA record") + require.Equal(t, uint32(0), soaRec.Hdr.Ttl) } 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(), "")