Merge pull request #4131 from pierresouchay/enable_full_dns_compression

Enable full dns compression

agent/dns.go

@@ -773,23 +773,21 @@ func dnsBinaryTruncate(resp *dns.Msg, maxSize int, index map[string]dns.RR, hasE
 func (d *DNSServer) trimTCPResponse(req, resp *dns.Msg) (trimmed bool) {
 	hasExtra := len(resp.Extra) > 0
 	// There is some overhead, 65535 does not work
-	maxSize := 65533 // 64k - 2 bytes
-	// In order to compute properly, we have to avoid compress first
-	compressed := resp.Compress
-	resp.Compress = false
+	maxSize := 65523 // 64k - 12 bytes DNS raw overhead
 
 	// We avoid some function calls and allocations by only handling the
 	// extra data when necessary.
 	var index map[string]dns.RR
 	originalSize := resp.Len()
 	originalNumRecords := len(resp.Answer)
 
-	// Beyond 2500 records, performance gets bad
-	// Limit the number of records at once, anyway, it won't fit in 64k
-	// For SRV Records, the max is around 500 records, for A, less than 2k
-	truncateAt := 2048
+	// It is not possible to return more than 4k records even with compression
+        // Since we are performing binary search it is not a big deal, but it
+        // improves a bit performance, even with binary search
+	truncateAt := 4096
 	if req.Question[0].Qtype == dns.TypeSRV {
-		truncateAt = 640
+		// More than 1024 SRV records do not fit in 64k
+		truncateAt = 1024
 	}
 	if len(resp.Answer) > truncateAt {
 		resp.Answer = resp.Answer[:truncateAt]
@@ -801,7 +799,7 @@ func (d *DNSServer) trimTCPResponse(req, resp *dns.Msg) (trimmed bool) {
 	truncated := false
 
 	// This enforces the given limit on 64k, the max limit for DNS messages
-	for len(resp.Answer) > 0 && resp.Len() > maxSize {
+	for len(resp.Answer) > 1 && resp.Len() > maxSize {
 		truncated = true
 		// More than 100 bytes, find with a binary search
 		if resp.Len()-maxSize > 100 {
@@ -819,8 +817,6 @@ func (d *DNSServer) trimTCPResponse(req, resp *dns.Msg) (trimmed bool) {
 			req.Question,
 			len(resp.Answer), originalNumRecords, resp.Len(), originalSize)
 	}
-	// Restore compression if any
-	resp.Compress = compressed
 	return truncated
 }
 
@@ -850,7 +846,10 @@ func trimUDPResponse(req, resp *dns.Msg, udpAnswerLimit int) (trimmed bool) {
 
 	// This cuts UDP responses to a useful but limited number of responses.
 	maxAnswers := lib.MinInt(maxUDPAnswerLimit, udpAnswerLimit)
+	compress := resp.Compress
 	if maxSize == defaultMaxUDPSize && numAnswers > maxAnswers {
+		// We disable computation of Len ONLY for non-eDNS request (512 bytes)
+		resp.Compress = false
 		resp.Answer = resp.Answer[:maxAnswers]
 		if hasExtra {
 			syncExtra(index, resp)
@@ -863,9 +862,9 @@ func trimUDPResponse(req, resp *dns.Msg, udpAnswerLimit int) (trimmed bool) {
 	// that will not exceed 512 bytes uncompressed, which is more conservative and
 	// will allow our responses to be compliant even if some downstream server
 	// uncompresses them.
-	compress := resp.Compress
-	resp.Compress = false
-	for len(resp.Answer) > 0 && resp.Len() > maxSize {
+	// Even when size is too big for one single record, try to send it anyway
+	// (usefull for 512 bytes messages)
+	for len(resp.Answer) > 1 && resp.Len() > maxSize {
 		// More than 100 bytes, find with a binary search
 		if resp.Len()-maxSize > 100 {
 			bestIndex := dnsBinaryTruncate(resp, maxSize, index, hasExtra)
@@ -877,6 +876,8 @@ func trimUDPResponse(req, resp *dns.Msg, udpAnswerLimit int) (trimmed bool) {
 			syncExtra(index, resp)
 		}
 	}
+	// For 512 non-eDNS responses, while we compute size non-compressed,
+	// we send result compressed
 	resp.Compress = compress
 
 	return len(resp.Answer) < numAnswers

agent/dns_test.go

@@ -3229,7 +3229,7 @@ func TestDNS_TCP_and_UDP_Truncate(t *testing.T) {
 			args := &structs.RegisterRequest{
 				Datacenter: "dc1",
 				Node:       fmt.Sprintf("%s-%d.acme.com", service, i),
-				Address:    fmt.Sprintf("127.%d.%d.%d", index, (i / 255), i%255),
+				Address:    fmt.Sprintf("127.%d.%d.%d", 0, (i / 255), i%255),
 				Service: &structs.NodeService{
 					Service: service,
 					Port:    8000,
@@ -3270,32 +3270,39 @@ func TestDNS_TCP_and_UDP_Truncate(t *testing.T) {
 			"tcp",
 			"udp",
 		}
-		for _, qType := range []uint16{dns.TypeANY, dns.TypeA, dns.TypeSRV} {
-			for _, question := range questions {
-				for _, protocol := range protocols {
-					for _, compress := range []bool{true, false} {
-						t.Run(fmt.Sprintf("lookup %s %s (qType:=%d) compressed=%v", question, protocol, qType, compress), func(t *testing.T) {
-							m := new(dns.Msg)
-							m.SetQuestion(question, dns.TypeANY)
-							if protocol == "udp" {
-								m.SetEdns0(8192, true)
-							}
-							c := new(dns.Client)
-							c.Net = protocol
-							m.Compress = compress
-							in, out, err := c.Exchange(m, a.DNSAddr())
-							if err != nil && err != dns.ErrTruncated {
-								t.Fatalf("err: %v", err)
-							}
-							// Check for the truncate bit
-							shouldBeTruncated := numServices > 5000
-
-							if shouldBeTruncated != in.Truncated || len(in.Answer) > 2000 || len(in.Answer) < 1 || in.Len() > 65535 {
+		for _, maxSize := range []uint16{8192, 65535} {
+			for _, qType := range []uint16{dns.TypeANY, dns.TypeA, dns.TypeSRV} {
+				for _, question := range questions {
+					for _, protocol := range protocols {
+						for _, compress := range []bool{true, false} {
+							t.Run(fmt.Sprintf("lookup %s %s (qType:=%d) compressed=%v", question, protocol, qType, compress), func(t *testing.T) {
+								m := new(dns.Msg)
+								m.SetQuestion(question, dns.TypeANY)
+								maxSz := maxSize
+								if protocol == "udp" {
+									maxSz = 8192
+								}
+								m.SetEdns0(uint16(maxSz), true)
+								c := new(dns.Client)
+								c.Net = protocol
+								m.Compress = compress
+								in, _, err := c.Exchange(m, a.DNSAddr())
+								if err != nil && err != dns.ErrTruncated {
+									t.Fatalf("err: %v", err)
+								}
+
+								// Check for the truncate bit
+								buf, err := m.Pack()
 								info := fmt.Sprintf("service %s question:=%s (%s) (%d total records) sz:= %d in %v",
-									service, question, protocol, numServices, len(in.Answer), out)
-								t.Fatalf("Should have truncated:=%v for %s", shouldBeTruncated, info)
-							}
-						})
+									service, question, protocol, numServices, len(in.Answer), in)
+								if err != nil {
+									t.Fatalf("Error while packing: %v ; info:=%s", err, info)
+								}
+								if len(buf) > int(maxSz) {
+									t.Fatalf("len(buf) := %d > maxSz=%d for %v", len(buf), maxSz, info)
+								}
+							})
+						}
 					}
 				}
 			}