Remove dnsutil.Dedup (#1867)

Remove the code and remove the call in etcd and kubernetes handlers.
This does mean we should not add dups in the first place, which means
adding maps in backend_lookup to prevent dups from begin added.

This should cut down on the allocations because dnsutil.Dedup is very
expensive by converting everything to strings, we avoid doing that now.

plugin/backend_lookup.go

@@ -19,6 +19,8 @@ func A(b ServiceBackend, zone string, state request.Request, previousRecords []d
 		return nil, err
 	}
 
+	dup := make(map[string]bool)
+
 	for _, serv := range services {
 
 		what, ip := serv.HostType()
@@ -67,10 +69,13 @@ func A(b ServiceBackend, zone string, state request.Request, previousRecords []d
 			continue
 
 		case dns.TypeA:
-			records = append(records, serv.NewA(state.QName(), ip))
+			if _, ok := dup[serv.Host]; !ok {
+				dup[serv.Host] = true
+				records = append(records, serv.NewA(state.QName(), ip))
+			}
 
 		case dns.TypeAAAA:
-			// nodata?
+			// nada
 		}
 	}
 	return records, nil
@@ -83,6 +88,8 @@ func AAAA(b ServiceBackend, zone string, state request.Request, previousRecords
 		return nil, err
 	}
 
+	dup := make(map[string]bool)
+
 	for _, serv := range services {
 
 		what, ip := serv.HostType()
@@ -132,10 +139,13 @@ func AAAA(b ServiceBackend, zone string, state request.Request, previousRecords
 			// both here again
 
 		case dns.TypeA:
-			// nada?
+			// nada
 
 		case dns.TypeAAAA:
-			records = append(records, serv.NewAAAA(state.QName(), ip))
+			if _, ok := dup[serv.Host]; !ok {
+				dup[serv.Host] = true
+				records = append(records, serv.NewAAAA(state.QName(), ip))
+			}
 		}
 	}
 	return records, nil
@@ -149,7 +159,18 @@ func SRV(b ServiceBackend, zone string, state request.Request, opt Options) (rec
 		return nil, nil, err
 	}
 
-	// Looping twice to get the right weight vs priority
+	type s struct {
+		n string
+		p uint16
+	}
+	dup := make(map[s]bool)
+	type a struct {
+		n string
+		a string
+	}
+	dupAddr := make(map[a]bool)
+
+	// Looping twice to get the right weight vs priority. This might break because we may drop duplicate SRV records latter on.
 	w := make(map[int]int)
 	for _, serv := range services {
 		weight := 100
@@ -212,11 +233,19 @@ func SRV(b ServiceBackend, zone string, state request.Request, opt Options) (rec
 			// IPv6 lookups here as well? AAAA(zone, state1, nil).
 
 		case dns.TypeA, dns.TypeAAAA:
+			addr := serv.Host
 			serv.Host = msg.Domain(serv.Key)
 			srv := serv.NewSRV(state.QName(), weight)
 
-			records = append(records, srv)
-			extra = append(extra, newAddress(serv, srv.Target, ip, what))
+			if _, ok := dup[s{srv.Target, srv.Port}]; !ok {
+				dup[s{srv.Target, srv.Port}] = true
+				records = append(records, srv)
+			}
+
+			if _, ok := dupAddr[a{srv.Target, addr}]; !ok {
+				dupAddr[a{srv.Target, addr}] = true
+				extra = append(extra, newAddress(serv, srv.Target, ip, what))
+			}
 		}
 	}
 	return records, extra, nil
@@ -229,6 +258,17 @@ func MX(b ServiceBackend, zone string, state request.Request, opt Options) (reco
 		return nil, nil, err
 	}
 
+	type s struct {
+		n string
+		p uint16
+	}
+	dup := make(map[s]bool)
+	type a struct {
+		n string
+		a string
+	}
+	dupAddr := make(map[a]bool)
+
 	lookup := make(map[string]bool)
 	for _, serv := range services {
 		if !serv.Mail {
@@ -271,9 +311,19 @@ func MX(b ServiceBackend, zone string, state request.Request, opt Options) (reco
 			// e.AAAA as well
 
 		case dns.TypeA, dns.TypeAAAA:
+			addr := serv.Host
 			serv.Host = msg.Domain(serv.Key)
-			records = append(records, serv.NewMX(state.QName()))
-			extra = append(extra, newAddress(serv, serv.Host, ip, what))
+			mx := serv.NewMX(state.QName())
+
+			if _, ok := dup[s{mx.Mx, mx.Preference}]; !ok {
+				dup[s{mx.Mx, mx.Preference}] = true
+				records = append(records, mx)
+			}
+			// Fake port to be 0 for address...
+			if _, ok := dupAddr[a{serv.Host, addr}]; !ok {
+				dupAddr[a{serv.Host, addr}] = true
+				extra = append(extra, newAddress(serv, serv.Host, ip, what))
+			}
 		}
 	}
 	return records, extra, nil
@@ -318,9 +368,14 @@ func PTR(b ServiceBackend, zone string, state request.Request, opt Options) (rec
 		return nil, err
 	}
 
+	dup := make(map[string]bool)
+
 	for _, serv := range services {
 		if ip := net.ParseIP(serv.Host); ip == nil {
-			records = append(records, serv.NewPTR(state.QName(), serv.Host))
+			if _, ok := dup[serv.Host]; !ok {
+				dup[serv.Host] = true
+				records = append(records, serv.NewPTR(state.QName(), serv.Host))
+			}
 		}
 	}
 	return records, nil

plugin/etcd/handler.go

@@ -4,7 +4,6 @@ import (
 	"context"
 
 	"github.com/coredns/coredns/plugin"
-	"github.com/coredns/coredns/plugin/pkg/dnsutil"
 	"github.com/coredns/coredns/request"
 
 	"github.com/miekg/dns"
@@ -88,8 +87,6 @@ func (e *Etcd) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (
 	m.Answer = append(m.Answer, records...)
 	m.Extra = append(m.Extra, extra...)
 
-	// TODO(miek): get rid of this by not adding dups in the first place, dnsutil.Append()?
-	m = dnsutil.Dedup(m)
 	state.SizeAndDo(m)
 	m, _ = state.Scrub(m)
 	w.WriteMsg(m)

plugin/etcd/msg/service.go

@@ -2,7 +2,6 @@
 package msg
 
 import (
-	"fmt"
 	"net"
 	"strings"
 
@@ -37,36 +36,6 @@ type Service struct {
 	Key string `json:"-"`
 }
 
-// RR returns an RR representation of s. It is in a condensed form to minimize space
-// when this is returned in a DNS message.
-// The RR will look like:
-//	1.rails.production.east.skydns.local. 300 CH TXT "service1.example.com:8080(10,0,,false)[0,]"
-//                      etcd Key              Ttl               Host:Port          <   see below   >
-// between parens: (Priority, Weight, Text (only first 200 bytes!), Mail)
-// between blockquotes: [TargetStrip,Group]
-// If the record is synthesised by CoreDNS (i.e. no lookup in etcd happened):
-//
-//	TODO(miek): what to put here?
-//
-func (s *Service) RR() *dns.TXT {
-	l := len(s.Text)
-	if l > 200 {
-		l = 200
-	}
-	t := new(dns.TXT)
-	t.Hdr.Class = dns.ClassCHAOS
-	t.Hdr.Ttl = s.TTL
-	t.Hdr.Rrtype = dns.TypeTXT
-	t.Hdr.Name = Domain(s.Key)
-
-	t.Txt = make([]string, 1)
-	t.Txt[0] = fmt.Sprintf("%s:%d(%d,%d,%s,%t)[%d,%s]",
-		s.Host, s.Port,
-		s.Priority, s.Weight, s.Text[:l], s.Mail,
-		s.TargetStrip, s.Group)
-	return t
-}
-
 // NewSRV returns a new SRV record based on the Service.
 func (s *Service) NewSRV(name string, weight uint16) *dns.SRV {
 	host := targetStrip(dns.Fqdn(s.Host), s.TargetStrip)

plugin/kubernetes/handler.go

@@ -4,7 +4,6 @@ import (
 	"context"
 
 	"github.com/coredns/coredns/plugin"
-	"github.com/coredns/coredns/plugin/pkg/dnsutil"
 	"github.com/coredns/coredns/request"
 
 	"github.com/miekg/dns"
@@ -79,9 +78,6 @@ func (k Kubernetes) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.M
 	m.Answer = append(m.Answer, records...)
 	m.Extra = append(m.Extra, extra...)
 
-	// TODO(miek): get rid of this by not adding dups in the first place, dnsutil.Append()?
-	m = dnsutil.Dedup(m)
-
 	state.SizeAndDo(m)
 	m, _ = state.Scrub(m)
 	w.WriteMsg(m)

plugin/kubernetes/kubernetes.go

@@ -107,7 +107,7 @@ func (k *Kubernetes) Services(state request.Request, exact bool, opt plugin.Opti
 		return []msg.Service{svc}, nil
 
 	case dns.TypeNS:
-		// We can only get here if the qname equal the zone, see ServeDNS in handler.go.
+		// We can only get here if the qname equals the zone, see ServeDNS in handler.go.
 		ns := k.nsAddr()
 		svc := msg.Service{Host: ns.A.String(), Key: msg.Path(state.QName(), "coredns")}
 		return []msg.Service{svc}, nil

plugin/pkg/dnsutil/dedup.go

@@ -1,12 +0,0 @@
-package dnsutil
-
-import "github.com/miekg/dns"
-
-// Dedup de-duplicates a message.
-func Dedup(m *dns.Msg) *dns.Msg {
-	// TODO(miek): expensive!
-	m.Answer = dns.Dedup(m.Answer, nil)
-	m.Ns = dns.Dedup(m.Ns, nil)
-	m.Extra = dns.Dedup(m.Extra, nil)
-	return m
-}