From 34d6c2d5e1f4d7bc8cf700b4ab0792e97ec49b2e Mon Sep 17 00:00:00 2001 From: James Phillips Date: Thu, 11 Aug 2016 21:46:14 -0700 Subject: [PATCH 1/5] Fixes the DNS SRV trim bug. --- command/agent/dns.go | 66 ++++++- command/agent/dns_test.go | 358 +++++++++++++++++++++++++++++++++++++- 2 files changed, 409 insertions(+), 15 deletions(-) diff --git a/command/agent/dns.go b/command/agent/dns.go index 62fdd87a9c..2a9aa2a7ed 100644 --- a/command/agent/dns.go +++ b/command/agent/dns.go @@ -494,16 +494,64 @@ func (d *DNSServer) formatNodeRecord(node *structs.Node, addr, qName string, qTy return records } -// trimUDPAnswers makes sure a UDP response is not longer than allowed by RFC +// indexRRs creates a map which indexes a given list of RRs by name. +func indexRRs(rrs []dns.RR) map[string]dns.RR { + index := make(map[string]dns.RR, len(rrs)) + for _, rr := range rrs { + name := rr.Header().Name + if _, ok := index[name]; !ok { + index[name] = rr + } + } + return index +} + +// syncExtra takes a DNS response message and sets the extra data to the most +// minimal set needed to cover the answer data. A pre-made index of RRs is given +// so that can be re-used between calls. This assumes that the extra data is +// only used to provide info for SRV records. If that's not the case, then this +// will wipe out any additional data. +func syncExtra(index map[string]dns.RR, resp *dns.Msg) { + seen := make(map[string]struct{}, len(resp.Answer)) + extra := make([]dns.RR, 0, len(resp.Answer)) + for _, ansRR := range resp.Answer { + srv, ok := ansRR.(*dns.SRV) + if !ok { + continue + } + target := srv.Target + + RESOLVE: + if _, ok := seen[target]; ok { + continue + } + seen[target] = struct{}{} + + extraRR, ok := index[target] + if ok { + extra = append(extra, extraRR) + if cname, ok := extraRR.(*dns.CNAME); ok { + target = cname.Target + goto RESOLVE + } + } + } + resp.Extra = extra +} + +// trimUDPResponse makes sure a UDP response is not longer than allowed by RFC // 1035. Enforce an arbitrary limit that can be further ratcheted down by -// config, and then make sure the response doesn't exceed 512 bytes. -func trimUDPAnswers(config *DNSConfig, resp *dns.Msg) (trimmed bool) { +// config, and then make sure the response doesn't exceed 512 bytes. Any extra +// records will be trimmed along with answers. +func trimUDPResponse(config *DNSConfig, resp *dns.Msg) (trimmed bool) { numAnswers := len(resp.Answer) + index := indexRRs(resp.Extra) // This cuts UDP responses to a useful but limited number of responses. maxAnswers := lib.MinInt(maxUDPAnswerLimit, config.UDPAnswerLimit) if numAnswers > maxAnswers { resp.Answer = resp.Answer[:maxAnswers] + syncExtra(index, resp) } // This enforces the hard limit of 512 bytes per the RFC. Note that we @@ -515,6 +563,7 @@ func trimUDPAnswers(config *DNSConfig, resp *dns.Msg) (trimmed bool) { resp.Compress = false for len(resp.Answer) > 0 && resp.Len() > 512 { resp.Answer = resp.Answer[:len(resp.Answer)-1] + syncExtra(index, resp) } resp.Compress = compress @@ -574,15 +623,15 @@ RPC: // Add various responses depending on the request qType := req.Question[0].Qtype - d.serviceNodeRecords(datacenter, out.Nodes, req, resp, ttl) - if qType == dns.TypeSRV { d.serviceSRVRecords(datacenter, out.Nodes, req, resp, ttl) + } else { + d.serviceNodeRecords(datacenter, out.Nodes, req, resp, ttl) } // If the network is not TCP, restrict the number of responses if network != "tcp" { - wasTrimmed := trimUDPAnswers(d.config, resp) + wasTrimmed := trimUDPResponse(d.config, resp) // Flag that there are more records to return in the UDP response if wasTrimmed && d.config.EnableTruncate { @@ -679,14 +728,15 @@ RPC: // Add various responses depending on the request. qType := req.Question[0].Qtype - d.serviceNodeRecords(datacenter, out.Nodes, req, resp, ttl) if qType == dns.TypeSRV { d.serviceSRVRecords(datacenter, out.Nodes, req, resp, ttl) + } else { + d.serviceNodeRecords(datacenter, out.Nodes, req, resp, ttl) } // If the network is not TCP, restrict the number of responses. if network != "tcp" { - wasTrimmed := trimUDPAnswers(d.config, resp) + wasTrimmed := trimUDPResponse(d.config, resp) // Flag that there are more records to return in the UDP response if wasTrimmed && d.config.EnableTruncate { diff --git a/command/agent/dns_test.go b/command/agent/dns_test.go index 2ef693ac5d..78f966aaba 100644 --- a/command/agent/dns_test.go +++ b/command/agent/dns_test.go @@ -5,6 +5,7 @@ import ( "math/rand" "net" "os" + "reflect" "strings" "testing" "time" @@ -1986,8 +1987,8 @@ func TestDNS_ServiceLookup_LargeResponses(t *testing.T) { longServiceName := "this-is-a-very-very-very-very-very-long-name-for-a-service" - // Register 3 nodes - for i := 0; i < 3; i++ { + // Register a lot of nodes. + for i := 0; i < 4; i++ { args := &structs.RegisterRequest{ Datacenter: "dc1", Node: fmt.Sprintf("foo%d", i), @@ -2042,12 +2043,32 @@ func TestDNS_ServiceLookup_LargeResponses(t *testing.T) { // Make sure the response size is RFC 1035-compliant for UDP messages if in.Len() > 512 { - t.Fatalf("Bad: %#v", in.Len()) + t.Fatalf("Bad: %d", in.Len()) } // We should only have two answers now if len(in.Answer) != 2 { - t.Fatalf("Bad: %#v", len(in.Answer)) + t.Fatalf("Bad: %d", len(in.Answer)) + } + + // Make sure the ADDITIONAL section matches the ANSWER section. + if len(in.Answer) != len(in.Extra) { + t.Fatalf("Bad: %d vs. %d", len(in.Answer), len(in.Extra)) + } + for i := 0; i < len(in.Answer); i++ { + srv, ok := in.Answer[i].(*dns.SRV) + if !ok { + t.Fatalf("Bad: %#v", in.Answer[i]) + } + + a, ok := in.Extra[i].(*dns.A) + if !ok { + t.Fatalf("Bad: %#v", in.Extra[i]) + } + + if srv.Target != a.Hdr.Name { + t.Fatalf("Bad: %#v %#v", srv, a) + } } // Check for the truncate bit @@ -3201,11 +3222,334 @@ func TestDNS_PreparedQuery_AgentSource(t *testing.T) { } } -func TestDNS_Compression_trimUDPAnswers(t *testing.T) { +func TestDNS_trimUDPResponse_NoTrim(t *testing.T) { + resp := &dns.Msg{ + Answer: []dns.RR{ + &dns.SRV{ + Hdr: dns.RR_Header{ + Name: "redis-cache-redis.service.consul.", + Rrtype: dns.TypeSRV, + Class: dns.ClassINET, + }, + Target: "ip-10-0-1-185.node.dc1.consul.", + }, + }, + Extra: []dns.RR{ + &dns.A{ + Hdr: dns.RR_Header{ + Name: "ip-10-0-1-185.node.dc1.consul.", + Rrtype: dns.TypeA, + Class: dns.ClassINET, + }, + A: net.ParseIP("10.0.1.185"), + }, + }, + } + + config := &DefaultConfig().DNSConfig + if trimmed := trimUDPResponse(config, resp); trimmed { + t.Fatalf("Bad %#v", *resp) + } + + expected := &dns.Msg{ + Answer: []dns.RR{ + &dns.SRV{ + Hdr: dns.RR_Header{ + Name: "redis-cache-redis.service.consul.", + Rrtype: dns.TypeSRV, + Class: dns.ClassINET, + }, + Target: "ip-10-0-1-185.node.dc1.consul.", + }, + }, + Extra: []dns.RR{ + &dns.A{ + Hdr: dns.RR_Header{ + Name: "ip-10-0-1-185.node.dc1.consul.", + Rrtype: dns.TypeA, + Class: dns.ClassINET, + }, + A: net.ParseIP("10.0.1.185"), + }, + }, + } + if !reflect.DeepEqual(resp, expected) { + t.Fatalf("Bad %#v vs. %#v", *resp, *expected) + } +} + +func TestDNS_trimUDPResponse_TrimLimit(t *testing.T) { + config := &DefaultConfig().DNSConfig + + resp, expected := &dns.Msg{}, &dns.Msg{} + for i := 0; i < config.UDPAnswerLimit+1; i++ { + target := fmt.Sprintf("ip-10-0-1-%d.node.dc1.consul.", 185+i) + srv := &dns.SRV{ + Hdr: dns.RR_Header{ + Name: "redis-cache-redis.service.consul.", + Rrtype: dns.TypeSRV, + Class: dns.ClassINET, + }, + Target: target, + } + a := &dns.A{ + Hdr: dns.RR_Header{ + Name: target, + Rrtype: dns.TypeA, + Class: dns.ClassINET, + }, + A: net.ParseIP(fmt.Sprintf("10.0.1.%d", 185+i)), + } + + resp.Answer = append(resp.Answer, srv) + resp.Extra = append(resp.Extra, a) + if i < config.UDPAnswerLimit { + expected.Answer = append(expected.Answer, srv) + expected.Extra = append(expected.Extra, a) + } + } + + if trimmed := trimUDPResponse(config, resp); !trimmed { + t.Fatalf("Bad %#v", *resp) + } + if !reflect.DeepEqual(resp, expected) { + t.Fatalf("Bad %#v vs. %#v", *resp, *expected) + } +} + +func TestDNS_trimUDPResponse_TrimSize(t *testing.T) { + config := &DefaultConfig().DNSConfig + + resp := &dns.Msg{} + for i := 0; i < 100; i++ { + target := fmt.Sprintf("ip-10-0-1-%d.node.dc1.consul.", 185+i) + srv := &dns.SRV{ + Hdr: dns.RR_Header{ + Name: "redis-cache-redis.service.consul.", + Rrtype: dns.TypeSRV, + Class: dns.ClassINET, + }, + Target: target, + } + a := &dns.A{ + Hdr: dns.RR_Header{ + Name: target, + Rrtype: dns.TypeA, + Class: dns.ClassINET, + }, + A: net.ParseIP(fmt.Sprintf("10.0.1.%d", 185+i)), + } + + resp.Answer = append(resp.Answer, srv) + resp.Extra = append(resp.Extra, a) + } + + // We don't know the exact trim, but we know the resulting answer + // data should match its extra data. + if trimmed := trimUDPResponse(config, resp); !trimmed { + t.Fatalf("Bad %#v", *resp) + } + if len(resp.Answer) == 0 || len(resp.Answer) != len(resp.Extra) { + t.Fatalf("Bad %#v", *resp) + } + for i, _ := range resp.Answer { + srv, ok := resp.Answer[i].(*dns.SRV) + if !ok { + t.Fatalf("should be SRV") + } + + a, ok := resp.Extra[i].(*dns.A) + if !ok { + t.Fatalf("should be A") + } + + if srv.Target != a.Header().Name { + t.Fatalf("Bad %#v vs. %#v", *srv, *a) + } + } +} + +func TestDNS_syncExtra(t *testing.T) { + resp := &dns.Msg{ + Answer: []dns.RR{ + // These two are on the same host so the redundant extra + // records should get deduplicated. + &dns.SRV{ + Hdr: dns.RR_Header{ + Name: "redis-cache-redis.service.consul.", + Rrtype: dns.TypeSRV, + Class: dns.ClassINET, + }, + Port: 1001, + Target: "ip-10-0-1-185.node.dc1.consul.", + }, + &dns.SRV{ + Hdr: dns.RR_Header{ + Name: "redis-cache-redis.service.consul.", + Rrtype: dns.TypeSRV, + Class: dns.ClassINET, + }, + Port: 1002, + Target: "ip-10-0-1-185.node.dc1.consul.", + }, + // This one isn't in the Consul domain so it will get a + // CNAME and then an A record from the recursor. + &dns.SRV{ + Hdr: dns.RR_Header{ + Name: "redis-cache-redis.service.consul.", + Rrtype: dns.TypeSRV, + Class: dns.ClassINET, + }, + Port: 1003, + Target: "demo.consul.io.", + }, + // This is also a CNAME, but it'll be set up to loop to + // make sure we don't crash. + &dns.SRV{ + Hdr: dns.RR_Header{ + Name: "redis-cache-redis.service.consul.", + Rrtype: dns.TypeSRV, + Class: dns.ClassINET, + }, + Port: 1001, + Target: "deadly.consul.io.", + }, + // This is also a CNAME, but it won't have another record. + &dns.SRV{ + Hdr: dns.RR_Header{ + Name: "redis-cache-redis.service.consul.", + Rrtype: dns.TypeSRV, + Class: dns.ClassINET, + }, + Port: 1001, + Target: "nope.consul.io.", + }, + }, + Extra: []dns.RR{ + // These should get deduplicated. + &dns.A{ + Hdr: dns.RR_Header{ + Name: "ip-10-0-1-185.node.dc1.consul.", + Rrtype: dns.TypeA, + Class: dns.ClassINET, + }, + A: net.ParseIP("10.0.1.185"), + }, + &dns.A{ + Hdr: dns.RR_Header{ + Name: "ip-10-0-1-185.node.dc1.consul.", + Rrtype: dns.TypeA, + Class: dns.ClassINET, + }, + A: net.ParseIP("10.0.1.185"), + }, + // This is a normal CNAME followed by an A record but we + // have flipped the order. The algorithm should emit them + // in the opposite order. + &dns.A{ + Hdr: dns.RR_Header{ + Name: "fakeserver.consul.io.", + Rrtype: dns.TypeA, + Class: dns.ClassINET, + }, + A: net.ParseIP("127.0.0.1"), + }, + &dns.CNAME{ + Hdr: dns.RR_Header{ + Name: "demo.consul.io.", + Rrtype: dns.TypeCNAME, + Class: dns.ClassINET, + }, + Target: "fakeserver.consul.io.", + }, + // This doesn't appear in the answer, so should get + // dropped. + &dns.A{ + Hdr: dns.RR_Header{ + Name: "ip-10-0-1-186.node.dc1.consul.", + Rrtype: dns.TypeA, + Class: dns.ClassINET, + }, + A: net.ParseIP("10.0.1.186"), + }, + // These two test edge cases with CNAME handling. + &dns.CNAME{ + Hdr: dns.RR_Header{ + Name: "deadly.consul.io.", + Rrtype: dns.TypeCNAME, + Class: dns.ClassINET, + }, + Target: "deadly.consul.io.", + }, + &dns.CNAME{ + Hdr: dns.RR_Header{ + Name: "nope.consul.io.", + Rrtype: dns.TypeCNAME, + Class: dns.ClassINET, + }, + Target: "notthere.consul.io.", + }, + }, + } + + index := indexRRs(resp.Extra) + syncExtra(index, resp) + + expected := &dns.Msg{ + Answer: resp.Answer, + Extra: []dns.RR{ + &dns.A{ + Hdr: dns.RR_Header{ + Name: "ip-10-0-1-185.node.dc1.consul.", + Rrtype: dns.TypeA, + Class: dns.ClassINET, + }, + A: net.ParseIP("10.0.1.185"), + }, + &dns.CNAME{ + Hdr: dns.RR_Header{ + Name: "demo.consul.io.", + Rrtype: dns.TypeCNAME, + Class: dns.ClassINET, + }, + Target: "fakeserver.consul.io.", + }, + &dns.A{ + Hdr: dns.RR_Header{ + Name: "fakeserver.consul.io.", + Rrtype: dns.TypeA, + Class: dns.ClassINET, + }, + A: net.ParseIP("127.0.0.1"), + }, + &dns.CNAME{ + Hdr: dns.RR_Header{ + Name: "deadly.consul.io.", + Rrtype: dns.TypeCNAME, + Class: dns.ClassINET, + }, + Target: "deadly.consul.io.", + }, + &dns.CNAME{ + Hdr: dns.RR_Header{ + Name: "nope.consul.io.", + Rrtype: dns.TypeCNAME, + Class: dns.ClassINET, + }, + Target: "notthere.consul.io.", + }, + }, + } + if !reflect.DeepEqual(resp, expected) { + t.Fatalf("Bad %#v vs. %#v", *resp, *expected) + } +} + +func TestDNS_Compression_trimUDPResponse(t *testing.T) { config := &DefaultConfig().DNSConfig m := dns.Msg{} - trimUDPAnswers(config, &m) + trimUDPResponse(config, &m) if m.Compress { t.Fatalf("compression should be off") } @@ -3213,7 +3557,7 @@ func TestDNS_Compression_trimUDPAnswers(t *testing.T) { // The trim function temporarily turns off compression, so we need to // make sure the setting gets restored properly. m.Compress = true - trimUDPAnswers(config, &m) + trimUDPResponse(config, &m) if !m.Compress { t.Fatalf("compression should be on") } From e30b99cef54590eab786ad248d41a89df3349c20 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Thu, 11 Aug 2016 22:01:23 -0700 Subject: [PATCH 2/5] Renames "seen" to "resolved". --- command/agent/dns.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/command/agent/dns.go b/command/agent/dns.go index 2a9aa2a7ed..589ea73af1 100644 --- a/command/agent/dns.go +++ b/command/agent/dns.go @@ -512,8 +512,8 @@ func indexRRs(rrs []dns.RR) map[string]dns.RR { // only used to provide info for SRV records. If that's not the case, then this // will wipe out any additional data. func syncExtra(index map[string]dns.RR, resp *dns.Msg) { - seen := make(map[string]struct{}, len(resp.Answer)) extra := make([]dns.RR, 0, len(resp.Answer)) + resolved := make(map[string]struct{}, len(resp.Answer)) for _, ansRR := range resp.Answer { srv, ok := ansRR.(*dns.SRV) if !ok { @@ -522,10 +522,10 @@ func syncExtra(index map[string]dns.RR, resp *dns.Msg) { target := srv.Target RESOLVE: - if _, ok := seen[target]; ok { + if _, ok := resolved[target]; ok { continue } - seen[target] = struct{}{} + resolved[target] = struct{}{} extraRR, ok := index[target] if ok { From 6332e2b3676fe66f784b03a36b220b742eb3310b Mon Sep 17 00:00:00 2001 From: James Phillips Date: Fri, 12 Aug 2016 10:29:57 -0700 Subject: [PATCH 3/5] Avoids allocations and function calls if no extra data is present. --- command/agent/dns.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/command/agent/dns.go b/command/agent/dns.go index 589ea73af1..b2c72f5309 100644 --- a/command/agent/dns.go +++ b/command/agent/dns.go @@ -545,13 +545,22 @@ func syncExtra(index map[string]dns.RR, resp *dns.Msg) { // records will be trimmed along with answers. func trimUDPResponse(config *DNSConfig, resp *dns.Msg) (trimmed bool) { numAnswers := len(resp.Answer) - index := indexRRs(resp.Extra) + hasExtra := len(resp.Extra) > 0 + + // We avoid some function calls and allocations by only handling the + // extra data when necessary. + var index map[string]dns.RR + if hasExtra { + index = indexRRs(resp.Extra) + } // This cuts UDP responses to a useful but limited number of responses. maxAnswers := lib.MinInt(maxUDPAnswerLimit, config.UDPAnswerLimit) if numAnswers > maxAnswers { resp.Answer = resp.Answer[:maxAnswers] - syncExtra(index, resp) + if hasExtra { + syncExtra(index, resp) + } } // This enforces the hard limit of 512 bytes per the RFC. Note that we @@ -563,7 +572,9 @@ func trimUDPResponse(config *DNSConfig, resp *dns.Msg) (trimmed bool) { resp.Compress = false for len(resp.Answer) > 0 && resp.Len() > 512 { resp.Answer = resp.Answer[:len(resp.Answer)-1] - syncExtra(index, resp) + if hasExtra { + syncExtra(index, resp) + } } resp.Compress = compress From f7fcb030043c9845b9679279c78705e0e4e7a046 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Fri, 12 Aug 2016 12:16:21 -0700 Subject: [PATCH 4/5] Makes name compares case-insensitive. --- command/agent/dns.go | 14 ++++++++---- command/agent/dns_test.go | 46 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/command/agent/dns.go b/command/agent/dns.go index b2c72f5309..f5f8521679 100644 --- a/command/agent/dns.go +++ b/command/agent/dns.go @@ -494,11 +494,13 @@ func (d *DNSServer) formatNodeRecord(node *structs.Node, addr, qName string, qTy return records } -// indexRRs creates a map which indexes a given list of RRs by name. +// indexRRs creates a map which indexes a given list of RRs by name. NOTE that +// the names are all squashed to lower case so we can perform case-insensitive +// lookups; the RRs are not modified. func indexRRs(rrs []dns.RR) map[string]dns.RR { index := make(map[string]dns.RR, len(rrs)) for _, rr := range rrs { - name := rr.Header().Name + name := strings.ToLower(rr.Header().Name) if _, ok := index[name]; !ok { index[name] = rr } @@ -519,7 +521,11 @@ func syncExtra(index map[string]dns.RR, resp *dns.Msg) { if !ok { continue } - target := srv.Target + + // Note that we always use lower case when using the index so + // that compares are not case-sensitive. We don't alter the actual + // RRs we add into the extra section, however. + target := strings.ToLower(srv.Target) RESOLVE: if _, ok := resolved[target]; ok { @@ -531,7 +537,7 @@ func syncExtra(index map[string]dns.RR, resp *dns.Msg) { if ok { extra = append(extra, extraRR) if cname, ok := extraRR.(*dns.CNAME); ok { - target = cname.Target + target = strings.ToLower(cname.Target) goto RESOLVE } } diff --git a/command/agent/dns_test.go b/command/agent/dns_test.go index 78f966aaba..073ba23a10 100644 --- a/command/agent/dns_test.go +++ b/command/agent/dns_test.go @@ -3403,6 +3403,19 @@ func TestDNS_syncExtra(t *testing.T) { Port: 1003, Target: "demo.consul.io.", }, + // This one isn't in the Consul domain and it will get + // a CNAME and A record from a recursor that alters the + // case of the name. This proves we look up in the index + // in a case-insensitive way. + &dns.SRV{ + Hdr: dns.RR_Header{ + Name: "redis-cache-redis.service.consul.", + Rrtype: dns.TypeSRV, + Class: dns.ClassINET, + }, + Port: 1001, + Target: "insensitive.consul.io.", + }, // This is also a CNAME, but it'll be set up to loop to // make sure we don't crash. &dns.SRV{ @@ -3462,6 +3475,23 @@ func TestDNS_syncExtra(t *testing.T) { }, Target: "fakeserver.consul.io.", }, + // These differ in case to test case insensitivity. + &dns.CNAME{ + Hdr: dns.RR_Header{ + Name: "INSENSITIVE.CONSUL.IO.", + Rrtype: dns.TypeCNAME, + Class: dns.ClassINET, + }, + Target: "Another.Server.Com.", + }, + &dns.A{ + Hdr: dns.RR_Header{ + Name: "another.server.com.", + Rrtype: dns.TypeA, + Class: dns.ClassINET, + }, + A: net.ParseIP("127.0.0.1"), + }, // This doesn't appear in the answer, so should get // dropped. &dns.A{ @@ -3522,6 +3552,22 @@ func TestDNS_syncExtra(t *testing.T) { }, A: net.ParseIP("127.0.0.1"), }, + &dns.CNAME{ + Hdr: dns.RR_Header{ + Name: "INSENSITIVE.CONSUL.IO.", + Rrtype: dns.TypeCNAME, + Class: dns.ClassINET, + }, + Target: "Another.Server.Com.", + }, + &dns.A{ + Hdr: dns.RR_Header{ + Name: "another.server.com.", + Rrtype: dns.TypeA, + Class: dns.ClassINET, + }, + A: net.ParseIP("127.0.0.1"), + }, &dns.CNAME{ Hdr: dns.RR_Header{ Name: "deadly.consul.io.", From 17c10d78bcb7533c7317f2cae48ac0216ac664ce Mon Sep 17 00:00:00 2001 From: James Phillips Date: Fri, 12 Aug 2016 14:51:50 -0700 Subject: [PATCH 5/5] Passes the index by reference so we can control the allocation. --- command/agent/dns.go | 9 ++++----- command/agent/dns_test.go | 3 ++- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/command/agent/dns.go b/command/agent/dns.go index f5f8521679..59d0601c18 100644 --- a/command/agent/dns.go +++ b/command/agent/dns.go @@ -494,18 +494,16 @@ func (d *DNSServer) formatNodeRecord(node *structs.Node, addr, qName string, qTy return records } -// indexRRs creates a map which indexes a given list of RRs by name. NOTE that +// indexRRs populates a map which indexes a given list of RRs by name. NOTE that // the names are all squashed to lower case so we can perform case-insensitive // lookups; the RRs are not modified. -func indexRRs(rrs []dns.RR) map[string]dns.RR { - index := make(map[string]dns.RR, len(rrs)) +func indexRRs(rrs []dns.RR, index map[string]dns.RR) { for _, rr := range rrs { name := strings.ToLower(rr.Header().Name) if _, ok := index[name]; !ok { index[name] = rr } } - return index } // syncExtra takes a DNS response message and sets the extra data to the most @@ -557,7 +555,8 @@ func trimUDPResponse(config *DNSConfig, resp *dns.Msg) (trimmed bool) { // extra data when necessary. var index map[string]dns.RR if hasExtra { - index = indexRRs(resp.Extra) + index = make(map[string]dns.RR, len(resp.Extra)) + indexRRs(resp.Extra, index) } // This cuts UDP responses to a useful but limited number of responses. diff --git a/command/agent/dns_test.go b/command/agent/dns_test.go index 073ba23a10..8d0165d4c1 100644 --- a/command/agent/dns_test.go +++ b/command/agent/dns_test.go @@ -3522,7 +3522,8 @@ func TestDNS_syncExtra(t *testing.T) { }, } - index := indexRRs(resp.Extra) + index := make(map[string]dns.RR) + indexRRs(resp.Extra, index) syncExtra(index, resp) expected := &dns.Msg{