mirror of https://github.com/status-im/consul.git
Fixes the DNS SRV trim bug.
This commit is contained in:
James Phillips
parent
c93bf8da92
commit
34d6c2d5e1
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue