correct EDNS responses (#96)

Tests updated as well and all the middleware. And Prometheus renamed to
metrics (directive is still prometheus).

core/setup/prometheus.go → core/setup/metrics.go

@@ -4,7 +4,7 @@ import (
 	"sync"
 
 	"github.com/miekg/coredns/middleware"
-	prom "github.com/miekg/coredns/middleware/prometheus"
+	"github.com/miekg/coredns/middleware/metrics"
 )
 
 const (
@@ -15,60 +15,60 @@ const (
 var once sync.Once
 
 func Prometheus(c *Controller) (middleware.Middleware, error) {
-	metrics, err := parsePrometheus(c)
+	met, err := parsePrometheus(c)
 	if err != nil {
 		return nil, err
 	}
 
 	once.Do(func() {
-		c.Startup = append(c.Startup, metrics.Start)
+		c.Startup = append(c.Startup, met.Start)
 	})
 
 	return func(next middleware.Handler) middleware.Handler {
-		metrics.Next = next
-		return metrics
+		met.Next = next
+		return met
 	}, nil
 }
 
-func parsePrometheus(c *Controller) (prom.Metrics, error) {
+func parsePrometheus(c *Controller) (metrics.Metrics, error) {
 	var (
-		metrics prom.Metrics
+		met metrics.Metrics
 		err error
 	)
 
 	for c.Next() {
-		if len(metrics.ZoneNames) > 0 {
-			return prom.Metrics{}, c.Err("prometheus: can only have one metrics module per server")
+		if len(met.ZoneNames) > 0 {
+			return metrics.Metrics{}, c.Err("metrics: can only have one metrics module per server")
 		}
-		metrics = prom.Metrics{ZoneNames: c.ServerBlockHosts}
-		for i, _ := range metrics.ZoneNames {
-			metrics.ZoneNames[i] = middleware.Host(metrics.ZoneNames[i]).Normalize()
+		met = metrics.Metrics{ZoneNames: c.ServerBlockHosts}
+		for i, _ := range met.ZoneNames {
+			met.ZoneNames[i] = middleware.Host(met.ZoneNames[i]).Normalize()
 		}
 		args := c.RemainingArgs()
 
 		switch len(args) {
 		case 0:
 		case 1:
-			metrics.Addr = args[0]
+			met.Addr = args[0]
 		default:
-			return prom.Metrics{}, c.ArgErr()
+			return metrics.Metrics{}, c.ArgErr()
 		}
 		for c.NextBlock() {
 			switch c.Val() {
 			case "address":
 				args = c.RemainingArgs()
 				if len(args) != 1 {
-					return prom.Metrics{}, c.ArgErr()
+					return metrics.Metrics{}, c.ArgErr()
 				}
-				metrics.Addr = args[0]
+				met.Addr = args[0]
 			default:
-				return prom.Metrics{}, c.Errf("prometheus: unknown item: %s", c.Val())
+				return metrics.Metrics{}, c.Errf("metrics: unknown item: %s", c.Val())
 			}
 
 		}
 	}
-	if metrics.Addr == "" {
-		metrics.Addr = addr
+	if met.Addr == "" {
+		met.Addr = addr
 	}
-	return metrics, err
+	return met, err
 }

middleware/chaos/chaos.go

@@ -43,6 +43,7 @@ func (c Chaos) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (
 		}
 		m.Answer = []dns.RR{&dns.TXT{Hdr: hdr, Txt: []string{trim(hostname)}}}
 	}
+	state.SizeAndDo(m)
 	w.WriteMsg(m)
 	return 0, nil
 }

middleware/edns.go

@@ -32,3 +32,14 @@ func Edns0Version(req *dns.Msg) (*dns.Msg, error) {
 
 	return m, errors.New("EDNS0 BADVERS")
 }
+
+// edns0Size returns a normalized size based on proto.
+func edns0Size(proto string, size int) int {
+	if proto == "tcp" {
+		return dns.MaxMsgSize
+	}
+	if size < dns.MinMsgSize {
+		return dns.MinMsgSize
+	}
+	return size
+}

middleware/errors/errors.go

@@ -37,6 +37,7 @@ func (h ErrorHandler) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns
 			answer := debugMsg(rcode, r)
 			txt, _ := dns.NewRR(". IN 0 TXT " + errMsg)
 			answer.Answer = append(answer.Answer, txt)
+			state.SizeAndDo(answer)
 			w.WriteMsg(answer)
 			return 0, err
 		}
@@ -52,6 +53,7 @@ func (h ErrorHandler) recovery(ctx context.Context, w dns.ResponseWriter, r *dns
 		return
 	}
 
+	state := middleware.State{W: w, Req: r}
 	// Obtain source of panic
 	// From: https://gist.github.com/swdunlop/9629168
 	var name, file string // function name, file name
@@ -86,6 +88,7 @@ func (h ErrorHandler) recovery(ctx context.Context, w dns.ResponseWriter, r *dns
 		// add stack buf in TXT, limited to 255 chars for now.
 		txt, _ := dns.NewRR(". IN 0 TXT " + string(stack[:255]))
 		answer.Answer = append(answer.Answer, txt)
+		state.SizeAndDo(answer)
 		w.WriteMsg(answer)
 	} else {
 		// Currently we don't use the function name, since file:line is more conventional

middleware/etcd/handler.go

@@ -66,46 +66,40 @@ func (e Etcd) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (i
 		return 0, nil
 	}
 	if isEtcdNameError(err) {
-		m := new(dns.Msg)
-		m.SetRcode(state.Req, dns.RcodeNameError)
-		m.Ns = []dns.RR{e.SOA(zone, state)}
-		state.W.WriteMsg(m)
-		return dns.RcodeNameError, nil
+		return e.Err(zone, dns.RcodeNameError, state)
 	}
 	if err != nil {
 		return dns.RcodeServerFailure, err
 	}
 
 	if len(records) == 0 {
-		// NODATE function, see below
-		m := new(dns.Msg)
-		m.SetReply(state.Req)
-		m.Ns = []dns.RR{e.SOA(zone, state)}
-		state.W.WriteMsg(m)
-		return dns.RcodeSuccess, nil
+		return e.Err(zone, dns.RcodeSuccess, state)
 	}
-	if len(records) > 0 {
+
 	m.Answer = append(m.Answer, records...)
-	}
-	if len(extra) > 0 {
 	m.Extra = append(m.Extra, extra...)
-	}
 
 	m = dedup(m)
+	state.SizeAndDo(m)
 	m, _ = state.Scrub(m)
-	state.W.WriteMsg(m)
-	return 0, nil
+	w.WriteMsg(m)
+	return dns.RcodeSuccess, nil
 }
 
 // NoData write a nodata response to the client.
-func (e Etcd) NoData(zone string, state middleware.State) {
-	// TODO(miek): write it
+func (e Etcd) Err(zone string, rcode int, state middleware.State) (int, error) {
+	m := new(dns.Msg)
+	m.SetRcode(state.Req, rcode)
+	m.Ns = []dns.RR{e.SOA(zone, state)}
+	state.SizeAndDo(m)
+	state.W.WriteMsg(m)
+	return rcode, nil
 }
 
 func dedup(m *dns.Msg) *dns.Msg {
-	ma := make(map[string]dns.RR)
-	m.Answer = dns.Dedup(m.Answer, ma)
-	m.Ns = dns.Dedup(m.Ns, ma)
-	m.Extra = dns.Dedup(m.Extra, ma)
+	// 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
 }

middleware/etcd/stub_handler.go

@@ -22,14 +22,15 @@ func (s Stub) ServeDNS(ctx context.Context, w dns.ResponseWriter, req *dns.Msg)
 	if !ok { // somebody made a mistake..
 		return dns.RcodeServerFailure, nil
 	}
-	state := middleware.State{W: w, Req: req}
 
-	m1, e1 := proxy.Forward(state)
-	if e1 != nil {
-		return dns.RcodeServerFailure, e1
+	state := middleware.State{W: w, Req: req}
+	m, e := proxy.Forward(state)
+	if e != nil {
+		return dns.RcodeServerFailure, e
 	}
-	m1.RecursionAvailable, m1.Compress = true, true
-	state.W.WriteMsg(m1)
+	m.RecursionAvailable, m.Compress = true, true
+	state.SizeAndDo(m)
+	w.WriteMsg(m)
 	return dns.RcodeSuccess, nil
 }
 

middleware/file/dnssec_test.go

@@ -20,6 +20,7 @@ var dnssecTestCases = []coretest.Case{
 			coretest.RRSIG("miek.nl.	1800	IN	RRSIG	SOA 8 2 1800 20160426031301 20160327031301 12051 miek.nl. FIrzy07acBbtyQczy1dc="),
 			coretest.SOA("miek.nl.	1800	IN	SOA	linode.atoom.net. miek.miek.nl. 1282630057 14400 3600 604800 14400"),
 		},
+		Extra: []dns.RR{coretest.OPT(4096, true)},
 	},
 	{
 		Qname: "miek.nl.", Qtype: dns.TypeAAAA, Do: true,
@@ -27,6 +28,7 @@ var dnssecTestCases = []coretest.Case{
 			coretest.AAAA("miek.nl.	1800	IN	AAAA	2a01:7e00::f03c:91ff:fef1:6735"),
 			coretest.RRSIG("miek.nl.	1800	IN	RRSIG	AAAA 8 2 1800 20160426031301 20160327031301 12051 miek.nl. SsRT="),
 		},
+		Extra: []dns.RR{coretest.OPT(4096, true)},
 	},
 	{
 		Qname: "miek.nl.", Qtype: dns.TypeMX, Do: true,
@@ -38,6 +40,7 @@ var dnssecTestCases = []coretest.Case{
 			coretest.MX("miek.nl.	1800	IN	MX	5 alt2.aspmx.l.google.com."),
 			coretest.RRSIG("miek.nl.	1800	IN	RRSIG	MX 8 2 1800 20160426031301 20160327031301 12051 miek.nl. kLqG+iOr="),
 		},
+		Extra: []dns.RR{coretest.OPT(4096, true)},
 	},
 	{
 		Qname: "www.miek.nl.", Qtype: dns.TypeA, Do: true,
@@ -46,6 +49,7 @@ var dnssecTestCases = []coretest.Case{
 		},
 
 		Extra: []dns.RR{
+			coretest.OPT(4096, true),
 			coretest.A("a.miek.nl.	1800	IN	A	139.162.196.78"),
 			coretest.RRSIG("a.miek.nl.	1800	IN	RRSIG	A 8 3 1800 20160426031301 20160327031301 12051 miek.nl. lxLotCjWZ3kihTxk="),
 		},
@@ -59,6 +63,7 @@ var dnssecTestCases = []coretest.Case{
 			coretest.RRSIG("miek.nl.	1800	IN	RRSIG	SOA 8 2 1800 20160426031301 20160327031301 12051 miek.nl. FIrzy07acBbtyQczy1dc="),
 			coretest.SOA("miek.nl.	1800	IN	SOA	linode.atoom.net. miek.miek.nl. 1282630057 14400 3600 604800 14400"),
 		},
+		Extra: []dns.RR{coretest.OPT(4096, true)},
 	},
 	{
 		Qname: "b.miek.nl.", Qtype: dns.TypeA, Do: true,
@@ -71,6 +76,7 @@ var dnssecTestCases = []coretest.Case{
 			coretest.RRSIG("miek.nl.	1800	IN	RRSIG	SOA 8 2 1800 20160426031301 20160327031301 12051 miek.nl. FIrzy07acBbtyQczy1dc="),
 			coretest.SOA("miek.nl.	1800	IN	SOA	linode.atoom.net. miek.miek.nl. 1282630057 14400 3600 604800 14400"),
 		},
+		Extra: []dns.RR{coretest.OPT(4096, true)},
 	},
 	{
 		Qname: "b.blaat.miek.nl.", Qtype: dns.TypeA, Do: true,
@@ -83,6 +89,7 @@ var dnssecTestCases = []coretest.Case{
 			coretest.RRSIG("miek.nl.	1800	IN	RRSIG	SOA 8 2 1800 20160426031301 20160327031301 12051 miek.nl. FIrzy07acBbtyQczy1dc="),
 			coretest.SOA("miek.nl.	1800	IN	SOA	linode.atoom.net. miek.miek.nl. 1282630057 14400 3600 604800 14400"),
 		},
+		Extra: []dns.RR{coretest.OPT(4096, true)},
 	},
 	{
 		Qname: "b.a.miek.nl.", Qtype: dns.TypeA, Do: true,
@@ -94,6 +101,7 @@ var dnssecTestCases = []coretest.Case{
 			coretest.RRSIG("miek.nl.	1800	IN	RRSIG	SOA 8 2 1800 20160426031301 20160327031301 12051 miek.nl. FIrzy07acBbtyQczy1dc="),
 			coretest.SOA("miek.nl.	1800	IN	SOA	linode.atoom.net. miek.miek.nl. 1282630057 14400 3600 604800 14400"),
 		},
+		Extra: []dns.RR{coretest.OPT(4096, true)},
 	},
 }
 

middleware/file/ent_test.go

@@ -27,6 +27,7 @@ var entTestCases = []coretest.Case{
 			coretest.RRSIG("miek.nl.	1800	IN	RRSIG	SOA 8 2 1800 20160502144311 20160402144311 12051 miek.nl. KegoBxA3Tbrhlc4cEdkRiteIkOfsq"),
 			coretest.SOA("miek.nl.	1800	IN	SOA	linode.atoom.net. miek.miek.nl. 1282630057 14400 3600 604800 14400"),
 		},
+		Extra: []dns.RR{coretest.OPT(4096, true)},
 	},
 }
 

middleware/file/file.go

@@ -25,6 +25,7 @@ type (
 
 func (f File) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
 	state := middleware.State{W: w, Req: r}
+
 	if state.QClass() != dns.ClassINET {
 		return dns.RcodeServerFailure, fmt.Errorf("can only deal with ClassINET")
 	}
@@ -45,6 +46,7 @@ func (f File) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (i
 			m := new(dns.Msg)
 			m.SetReply(r)
 			m.Authoritative, m.RecursionAvailable, m.Compress = true, true, true
+			state.SizeAndDo(m)
 			w.WriteMsg(m)
 
 			log.Printf("[INFO] Notify from %s for %s: checking transfer", state.IP(), zone)
@@ -93,6 +95,8 @@ func (f File) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (i
 	case ServerFailure:
 		return dns.RcodeServerFailure, nil
 	}
+
+	state.SizeAndDo(m)
 	m, _ = state.Scrub(m)
 	w.WriteMsg(m)
 	return dns.RcodeSuccess, nil

middleware/file/wildcard_test.go

@@ -25,6 +25,7 @@ var wildcardTestCases = []coretest.Case{
 			coretest.RRSIG("wild.dnssex.nl.	1800	IN	RRSIG	TXT 8 2 1800 20160428190224 20160329190224 14460 dnssex.nl. FUZSTyvZfeuuOpCm"),
 			coretest.TXT(`wild.dnssex.nl.	1800	IN	TXT	"Doing It Safe Is Better"`),
 		},
+		Extra: []dns.RR{coretest.OPT(4096, true)},
 	},
 	// nodata reponse
 	/*

middleware/log/log.go

@@ -3,11 +3,13 @@ package log
 
 import (
 	"log"
-
-	"golang.org/x/net/context"
+	"time"
 
 	"github.com/miekg/coredns/middleware"
+	"github.com/miekg/coredns/middleware/metrics"
+
 	"github.com/miekg/dns"
+	"golang.org/x/net/context"
 )
 
 // Logger is a basic request logging middleware.
@@ -30,9 +32,13 @@ func (l Logger) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg)
 				if l.ErrorFunc != nil {
 					l.ErrorFunc(responseRecorder, r, rcode)
 				} else {
-					// Default failover error handler
+					rc := middleware.RcodeToString(rcode)
+
 					answer := new(dns.Msg)
 					answer.SetRcode(r, rcode)
+					state.SizeAndDo(answer)
+
+					metrics.Report(metrics.Dropped, state.Type(), rc, answer.Len(), time.Now())
 					w.WriteMsg(answer)
 				}
 				rcode = 0

middleware/prometheus/metrics.go → middleware/metrics/metrics.go

@@ -81,3 +81,6 @@ func define(subsystem string) {
 		Help:      "Counter of response status codes.",
 	}, []string{"zone", "rcode", "qtype"})
 }
+
+// Dropped indicates we dropped the query before any handling. It has no closing dot, so it can not be a valid zone.
+const Dropped = "dropped"

middleware/proxy/lookup.go

@@ -52,16 +52,13 @@ func New(hosts []string) Proxy {
 	return p
 }
 
-// Lookup will use name and tpe to forge a new message and will send that upstream. It will
+// Lookup will use name and type to forge a new message and will send that upstream. It will
 // set any EDNS0 options correctly so that downstream will be able to process the reply.
-// Lookup is not suitable for forwarding request. So Forward for that.
+// Lookup is not suitable for forwarding request. Ssee for that.
 func (p Proxy) Lookup(state middleware.State, name string, tpe uint16) (*dns.Msg, error) {
 	req := new(dns.Msg)
 	req.SetQuestion(name, tpe)
-
-	opt := state.SizeAndDo()
-	req.Extra = []dns.RR{opt}
-
+	state.SizeAndDo(req)
 	return p.lookup(state, req)
 }
 

middleware/state.go

@@ -110,43 +110,41 @@ func (s *State) Size() int {
 		return s.size
 	}
 
-	if s.Proto() == "tcp" {
-		s.size = dns.MaxMsgSize
-		return dns.MaxMsgSize
-	}
+	size := 0
 	if o := s.Req.IsEdns0(); o != nil {
 		if o.Do() == true {
 			s.do = doTrue
 		} else {
 			s.do = doFalse
 		}
-
-		size := o.UDPSize()
-		if size < dns.MinMsgSize {
-			size = dns.MinMsgSize
-		}
-		s.size = int(size)
-		return int(size)
+		size = int(o.UDPSize())
 	}
-	s.size = dns.MinMsgSize
-	return dns.MinMsgSize
+	size = edns0Size(s.Proto(), size)
+	s.size = size
+	return size
 }
 
-// SizeAndDo returns a ready made OPT record that the reflects the intent from
-// state. This can be added to upstream requests that will then hopefully
-// return a message that is fits the buffer in the client.
-func (s *State) SizeAndDo() *dns.OPT {
+// SizeAndDo adds an OPT record that the reflects the intent from state.
+// The returned bool indicated if an record was added.
+func (s *State) SizeAndDo(m *dns.Msg) bool {
+	o := s.Req.IsEdns0() // TODO(miek): speed this up
+	if o == nil {
+		return false
+	}
+
 	size := s.Size()
 	Do := s.Do()
 
-	o := new(dns.OPT)
 	o.Hdr.Name = "."
 	o.Hdr.Rrtype = dns.TypeOPT
+	o.SetVersion(0)
 	o.SetUDPSize(uint16(size))
 	if Do {
 		o.SetDo()
 	}
-	return o
+	// TODO(miek): test how this works with stub forwarding in etcd middleware.
+	m.Extra = append(m.Extra, o)
+	return true
 }
 
 // Result is the result of Scrub.
@@ -170,9 +168,11 @@ func (s *State) Scrub(reply *dns.Msg) (*dns.Msg, Result) {
 	if size >= l {
 		return reply, ScrubIgnored
 	}
-	// If not delegation, drop additional section.
 	// TODO(miek): check for delegation
+
+	// If not delegation, drop additional section.
 	reply.Extra = nil
+	s.SizeAndDo(reply)
 	l = reply.Len()
 	if size >= l {
 		return reply, ScrubDone

middleware/testing/helpers.go

@@ -57,6 +57,18 @@ func MX(rr string) *dns.MX       { r, _ := dns.NewRR(rr); return r.(*dns.MX) }
 func RRSIG(rr string) *dns.RRSIG { r, _ := dns.NewRR(rr); return r.(*dns.RRSIG) }
 func NSEC(rr string) *dns.NSEC   { r, _ := dns.NewRR(rr); return r.(*dns.NSEC) }
 
+func OPT(bufsize int, do bool) *dns.OPT {
+	o := new(dns.OPT)
+	o.Hdr.Name = "."
+	o.Hdr.Rrtype = dns.TypeOPT
+	o.SetVersion(0)
+	o.SetUDPSize(uint16(bufsize))
+	if do {
+		o.SetDo()
+	}
+	return o
+}
+
 func Header(t *testing.T, tc Case, resp *dns.Msg) bool {
 	if resp.Rcode != tc.Rcode {
 		t.Errorf("rcode is %q, expected %q", dns.RcodeToString[resp.Rcode], dns.RcodeToString[tc.Rcode])
@@ -192,6 +204,16 @@ func Section(t *testing.T, tc Case, sect Sect, rr []dns.RR) bool {
 				t.Errorf("NS nameserver should be %q, but is %q", x.Ns, tt.Ns)
 				return false
 			}
+		case *dns.OPT:
+			tt := section[i].(*dns.OPT)
+			if x.Do() != tt.Do() {
+				t.Errorf("OPT DO should be %q, but is %q", x.Do(), tt.Do())
+				return false
+			}
+			if x.UDPSize() != tt.UDPSize() {
+				t.Errorf("OPT UDPSize should be %q, but is %q", x.UDPSize(), tt.UDPSize())
+				return false
+			}
 		}
 	}
 	return true

server/server.go

@@ -17,7 +17,7 @@ import (
 
 	"github.com/miekg/coredns/middleware"
 	"github.com/miekg/coredns/middleware/chaos"
-	"github.com/miekg/coredns/middleware/prometheus"
+	"github.com/miekg/coredns/middleware/metrics"
 
 	"github.com/miekg/dns"
 	"golang.org/x/net/context"
@@ -282,7 +282,7 @@ func (s *Server) ServeDNS(w dns.ResponseWriter, r *dns.Msg) {
 	if m, err := middleware.Edns0Version(r); err != nil { // Wrong EDNS version, return at once.
 		qtype := dns.Type(r.Question[0].Qtype).String()
 		rc := middleware.RcodeToString(dns.RcodeBadVers)
-		metrics.Report(dropped, qtype, rc, m.Len(), time.Now())
+		metrics.Report(metrics.Dropped, qtype, rc, m.Len(), time.Now())
 		w.WriteMsg(m)
 		return
 	}
@@ -336,17 +336,16 @@ func (s *Server) ServeDNS(w dns.ResponseWriter, r *dns.Msg) {
 	log.Printf("[INFO] %s - No such zone at %s (Remote: %s)", q, s.Addr, remoteHost)
 }
 
-// DefaultErrorFunc responds to an HTTP request with a simple description
-// of the specified HTTP status code.
+// DefaultErrorFunc responds to an DNS request with an error.
 func DefaultErrorFunc(w dns.ResponseWriter, r *dns.Msg, rcode int) {
-	qtype := dns.Type(r.Question[0].Qtype).String()
+	state := middleware.State{W: w, Req: r}
 	rc := middleware.RcodeToString(rcode)
 
 	answer := new(dns.Msg)
 	answer.SetRcode(r, rcode)
-	// Default zone to dropped (without closing dot, so no zone) here to not blow up this metric.
-	metrics.Report(dropped, qtype, rc, answer.Len(), time.Now())
+	state.SizeAndDo(answer)
 
+	metrics.Report(metrics.Dropped, state.Type(), rc, answer.Len(), time.Now())
 	w.WriteMsg(answer)
 }
 
@@ -459,5 +458,3 @@ func RcodeNoClientWrite(rcode int) bool {
 	}
 	return false
 }
-
-const dropped = "dropped"