middleware/httpproxy: add debug queries (#446)

* middleware/httproxy: implement debug queries

Not too useful at the moment, but o-o.debug queries are supported
and return the Comment from dns.google.com.

Note that this is not always set.

* improve documentation

* Testing cleanups

12 files changed, 108 insertions, 55 deletions

diff --git a/middleware/etcd/README.md b/middleware/etcd/README.md
index 80eff35cf..030977fef 100644
--- a/middleware/etcd/README.md
+++ b/middleware/etcd/README.md
@@ -37,7 +37,7 @@ etcd [ZONES...] {
 * **ENDPOINT** the etcd endpoints. Defaults to "http://localhost:2397".
 * `upstream` upstream resolvers to be used resolve external names found in etcd (think CNAMEs)
   pointing to external names. If you want CoreDNS to act as a proxy for clients, you'll need to add
-  the proxy middleware. **ADDRESS* can be an IP address, and IP:port or a string pointing to a file
+  the proxy middleware. **ADDRESS** can be an IP address, and IP:port or a string pointing to a file
   that is structured as /etc/resolv.conf.
 * `tls` followed the cert, key and the CA's cert filenames.
 * `debug` allows for debug queries. Prefix the name with `o-o.debug.` to retrieve extra information in the
@@ -127,7 +127,7 @@ Or with *debug* queries enabled:
 When debug queries are enabled CoreDNS will return errors and etcd records encountered during the resolution
 process in the response. The general form looks like this:
 
-    skydns.test.skydns.dom.a.	300	CH	TXT	"127.0.0.1:0(10,0,,false)[0,]"
+    skydns.test.skydns.dom.a.	0	CH	TXT	"127.0.0.1:0(10,0,,false)[0,]"
 
 This shows the complete key as the owername, the rdata of the TXT record has:
 `host:port(priority,weight,txt content,mail)[targetstrip,group]`.
diff --git a/middleware/etcd/debug.go b/middleware/etcd/debug.go
deleted file mode 100644
index d2dd66830..000000000
--- a/middleware/etcd/debug.go
+++ /dev/null
@@ -1,20 +0,0 @@
-package etcd
-
-import "strings"
-
-const debugName = "o-o.debug."
-
-// isDebug checks if name is a debugging name, i.e. starts with o-o.debug.
-// it return the empty string if it is not a debug message, otherwise it will return the
-// name with o-o.debug. stripped off. Must be called with name lowercased.
-func isDebug(name string) string {
-	if len(name) == len(debugName) {
-		return ""
-	}
-	name = strings.ToLower(name)
-	debug := strings.HasPrefix(name, debugName)
-	if !debug {
-		return ""
-	}
-	return name[len(debugName):]
-}
diff --git a/middleware/etcd/debug_test.go b/middleware/etcd/debug_test.go
index b1dbb0264..aa26dd846 100644
--- a/middleware/etcd/debug_test.go
+++ b/middleware/etcd/debug_test.go
@@ -4,7 +4,6 @@ package etcd
 
 import (
 	"sort"
-	"strings"
 	"testing"
 
 	"github.com/miekg/coredns/middleware/etcd/msg"
@@ -14,21 +13,6 @@ import (
 	"github.com/miekg/dns"
 )
 
-func TestIsDebug(t *testing.T) {
-	if ok := isDebug("o-o.debug.miek.nl."); ok != "miek.nl." {
-		t.Errorf("expected o-o.debug.miek.nl. to be debug")
-	}
-	if ok := isDebug(strings.ToLower("o-o.Debug.miek.nl.")); ok != "miek.nl." {
-		t.Errorf("expected o-o.Debug.miek.nl. to be debug")
-	}
-	if ok := isDebug("i-o.debug.miek.nl."); ok != "" {
-		t.Errorf("expected i-o.Debug.miek.nl. to be non-debug")
-	}
-	if ok := isDebug(strings.ToLower("i-o.Debug.")); ok != "" {
-		t.Errorf("expected o-o.Debug. to be non-debug")
-	}
-}
-
 func TestDebugLookup(t *testing.T) {
 	etc := newEtcdMiddleware()
 	etc.Debugging = true
diff --git a/middleware/etcd/handler.go b/middleware/etcd/handler.go
index c4fa3c46e..b462d7c5b 100644
--- a/middleware/etcd/handler.go
+++ b/middleware/etcd/handler.go
@@ -5,6 +5,7 @@ import (
 
 	"github.com/miekg/coredns/middleware"
 	"github.com/miekg/coredns/middleware/etcd/msg"
+	"github.com/miekg/coredns/middleware/pkg/debug"
 	"github.com/miekg/coredns/middleware/pkg/dnsutil"
 	"github.com/miekg/coredns/request"
 
@@ -21,10 +22,10 @@ func (e *Etcd) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (
 	}
 	name := state.Name()
 	if e.Debugging {
-		if debug := isDebug(name); debug != "" {
+		if bug := debug.IsDebug(name); bug != "" {
 			opt.Debug = r.Question[0].Name
 			state.Clear()
-			state.Req.Question[0].Name = debug
+			state.Req.Question[0].Name = bug
 		}
 	}
 
diff --git a/middleware/httpproxy/README.md b/middleware/httpproxy/README.md
index 026fbdc71..f0bf58903 100644
--- a/middleware/httpproxy/README.md
+++ b/middleware/httpproxy/README.md
@@ -48,3 +48,24 @@ proxy . dns.google.com {
     upstream /etc/resolv.conf
 }
 ~~~
+
+## Debug queries
+
+Debug queries are enabled by default and currently there is no way to turn them off. When CoreDNS
+receives a debug queries (i.e. the name is prefixed with `o-o.debug.` a TXT record with Comment from
+`dns.google.com` is added. Note this is not always set, but sometimes you'll see:
+
+`dig @localhost -p 1053 mx o-o.debug.example.org`:
+
+~~~ txt
+;; OPT PSEUDOSECTION:
+; EDNS: version: 0, flags:; udp: 4096
+;; QUESTION SECTION:
+;o-o.debug.example.org.		IN	MX
+
+;; AUTHORITY SECTION:
+example.org.		1799	IN	SOA	sns.dns.icann.org. noc.dns.icann.org. 2016110711 7200 3600 1209600 3600
+
+;; ADDITIONAL SECTION:
+.			0	CH	TXT	"Response from 199.43.133.53"
+~~~
diff --git a/middleware/httpproxy/google.go b/middleware/httpproxy/google.go
index 78b5ea864..dafd0d6a6 100644
--- a/middleware/httpproxy/google.go
+++ b/middleware/httpproxy/google.go
@@ -12,7 +12,9 @@ import (
 	"sync/atomic"
 	"time"
 
+	"github.com/miekg/coredns/middleware/pkg/debug"
 	"github.com/miekg/coredns/middleware/proxy"
+	"github.com/miekg/coredns/request"
 
 	"github.com/miekg/dns"
 )
@@ -30,11 +32,17 @@ type google struct {
 
 func newGoogle() *google { return &google{client: newClient(ghost), quit: make(chan bool)} }
 
-func (g *google) Exchange(req *dns.Msg) (*dns.Msg, error) {
+func (g *google) Exchange(state request.Request) (*dns.Msg, error) {
 	v := url.Values{}
 
-	v.Set("name", req.Question[0].Name)
-	v.Set("type", fmt.Sprintf("%d", req.Question[0].Qtype))
+	v.Set("name", state.Name())
+	v.Set("type", fmt.Sprintf("%d", state.QType()))
+
+	optDebug := false
+	if bug := debug.IsDebug(state.Name()); bug != "" {
+		optDebug = true
+		v.Set("name", bug)
+	}
 
 	start := time.Now()
 
@@ -60,12 +68,20 @@ func (g *google) Exchange(req *dns.Msg) (*dns.Msg, error) {
 				return nil, err
 			}
 
-			m, err := toMsg(gm)
+			m, debug, err := toMsg(gm)
 			if err != nil {
 				return nil, err
 			}
 
-			m.Id = req.Id
+			if optDebug {
+				// reset question
+				m.Question[0].Name = state.QName()
+				// prepend debug RR to the additional section
+				m.Extra = append([]dns.RR{debug}, m.Extra...)
+
+			}
+
+			m.Id = state.Req.Id
 			return m, nil
 		}
 
@@ -223,8 +239,11 @@ func (g *google) do(addr, json string) ([]byte, error) {
 	return buf, nil
 }
 
-func toMsg(g *googleMsg) (*dns.Msg, error) {
+// toMsg converts a googleMsg into the dns message. The returned RR is the comment disquised as a TXT
+// record.
+func toMsg(g *googleMsg) (*dns.Msg, dns.RR, error) {
 	m := new(dns.Msg)
+	m.Response = true
 	m.Rcode = g.Status
 	m.Truncated = g.TC
 	m.RecursionDesired = g.RD
@@ -243,23 +262,24 @@ func toMsg(g *googleMsg) (*dns.Msg, error) {
 	for i := 0; i < len(m.Answer); i++ {
 		m.Answer[i], err = toRR(g.Answer[i])
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 	}
 	for i := 0; i < len(m.Ns); i++ {
 		m.Ns[i], err = toRR(g.Authority[i])
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 	}
 	for i := 0; i < len(m.Extra); i++ {
 		m.Extra[i], err = toRR(g.Additional[i])
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 	}
 
-	return m, nil
+	txt, _ := dns.NewRR(". 0 CH TXT " + g.Comment)
+	return m, txt, nil
 }
 
 func toRR(g googleRR) (dns.RR, error) {
diff --git a/middleware/httpproxy/proxy.go b/middleware/httpproxy/proxy.go
index 6b1243dff..3ef638a8f 100644
--- a/middleware/httpproxy/proxy.go
+++ b/middleware/httpproxy/proxy.go
@@ -27,9 +27,9 @@ func (p *Proxy) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg)
 	start := time.Now()
 	state := request.Request{W: w, Req: r}
 
-	reply, backendErr := p.e.Exchange(r)
+	reply, backendErr := p.e.Exchange(state)
 
-	if backendErr == nil {
+	if backendErr == nil && reply != nil {
 		state.SizeAndDo(reply)
 
 		w.WriteMsg(reply)
diff --git a/middleware/httpproxy/setup_test.go b/middleware/httpproxy/setup_test.go
index 71d631220..82db40aff 100644
--- a/middleware/httpproxy/setup_test.go
+++ b/middleware/httpproxy/setup_test.go
@@ -2,6 +2,7 @@ package httpproxy
 
 import (
 	"io/ioutil"
+	"log"
 	"os"
 	"strings"
 	"testing"
@@ -9,7 +10,9 @@ import (
 	"github.com/mholt/caddy"
 )
 
-func TestSetupChaos(t *testing.T) {
+func TestSetupHttpproxy(t *testing.T) {
+	log.SetOutput(ioutil.Discard)
+
 	tests := []struct {
 		input              string
 		shouldErr          bool
@@ -55,7 +58,6 @@ func TestSetupChaos(t *testing.T) {
 		}
 
 		if err != nil {
-			t.Logf("%q", err)
 			if !test.shouldErr {
 				t.Errorf("Test %d: Expected no error but found one for input %s. Error was: %v", i, test.input, err)
 			}
diff --git a/middleware/httpproxy/tls.go b/middleware/httpproxy/tls.go
index 9651ac1c6..2c05a0331 100644
--- a/middleware/httpproxy/tls.go
+++ b/middleware/httpproxy/tls.go
@@ -5,13 +5,14 @@ import (
 	"net/http"
 	"time"
 
+	"github.com/miekg/coredns/request"
 	"github.com/miekg/dns"
 )
 
 // Exchanger is an interface that specifies a type implementing a DNS resolver that
 // uses a HTTPS server.
 type Exchanger interface {
-	Exchange(*dns.Msg) (*dns.Msg, error)
+	Exchange(request.Request) (*dns.Msg, error)
 
 	SetUpstream(*simpleUpstream) error
 	OnStartup() error
diff --git a/middleware/pkg/debug/debug.go b/middleware/pkg/debug/debug.go
new file mode 100644
index 000000000..b3c33b344
--- /dev/null
+++ b/middleware/pkg/debug/debug.go
@@ -0,0 +1,20 @@
+package debug
+
+import "strings"
+
+const Name = "o-o.debug."
+
+// IsDebug checks if name is a debugging name, i.e. starts with o-o.debug.
+// it returns the empty string if it is not a debug message, otherwise it will return the
+// name with o-o.debug. stripped off. Must be called with name lowercased.
+func IsDebug(name string) string {
+	if len(name) == len(Name) {
+		return ""
+	}
+	name = strings.ToLower(name)
+	debug := strings.HasPrefix(name, Name)
+	if !debug {
+		return ""
+	}
+	return name[len(Name):]
+}
diff --git a/middleware/pkg/debug/debug_test.go b/middleware/pkg/debug/debug_test.go
new file mode 100644
index 000000000..9b0d9ea4f
--- /dev/null
+++ b/middleware/pkg/debug/debug_test.go
@@ -0,0 +1,21 @@
+package debug
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestIsDebug(t *testing.T) {
+	if ok := IsDebug("o-o.debug.miek.nl."); ok != "miek.nl." {
+		t.Errorf("expected o-o.debug.miek.nl. to be debug")
+	}
+	if ok := IsDebug(strings.ToLower("o-o.Debug.miek.nl.")); ok != "miek.nl." {
+		t.Errorf("expected o-o.Debug.miek.nl. to be debug")
+	}
+	if ok := IsDebug("i-o.debug.miek.nl."); ok != "" {
+		t.Errorf("expected i-o.Debug.miek.nl. to be non-debug")
+	}
+	if ok := IsDebug(strings.ToLower("i-o.Debug.")); ok != "" {
+		t.Errorf("expected o-o.Debug. to be non-debug")
+	}
+}
diff --git a/middleware/root/root_test.go b/middleware/root/root_test.go
index 06c81a05e..434c82dfd 100644
--- a/middleware/root/root_test.go
+++ b/middleware/root/root_test.go
@@ -3,6 +3,7 @@ package root
 import (
 	"fmt"
 	"io/ioutil"
+	"log"
 	"os"
 	"path/filepath"
 	"strings"
@@ -14,6 +15,8 @@ import (
 )
 
 func TestRoot(t *testing.T) {
+	log.SetOutput(ioutil.Discard)
+
 	// Predefined error substrings
 	parseErrContent := "Parse error:"
 	unableToAccessErrContent := "Unable to access root path"