plugin/file: fix local CNAME lookup (#1866)

* plugin/file: fix local CNAME lookup

Issue #1864 explains it will, when we serve the child zone as well we
should just recursive into ourself (upstream self). Thus relax the
IsSubDomain check in file/lookup.go and just query (even if the query
will hit a remote server).

I've looped over all other plugins that do something similar (CNAME
resolving) and they didn't do the IsSubDomain check; therefor I've
removed it from *file* as well.

Added test in file_upstream_test that shows this failed before but now
results in a reply.

Fixes #1864

* self does not need to be exported

* Fix test

We don't know if we had a valid reply. Check this.

5 files changed, 83 insertions, 28 deletions

diff --git a/plugin/backend_lookup.go b/plugin/backend_lookup.go
index 33d1846a8..7ae6f8cce 100644
--- a/plugin/backend_lookup.go
+++ b/plugin/backend_lookup.go
@@ -122,12 +122,9 @@ func AAAA(b ServiceBackend, zone string, state request.Request, previousRecords
 				}
 				continue
 			}
+
 			// This means we can not complete the CNAME, try to look else where.
 			target := newRecord.Target
-			if dns.IsSubDomain(zone, target) {
-				// We should already have found it
-				continue
-			}
 			m1, e1 := b.Lookup(state, target, state.QType())
 			if e1 != nil {
 				continue
diff --git a/plugin/file/lookup.go b/plugin/file/lookup.go
index 943b1d52f..31248f314 100644
--- a/plugin/file/lookup.go
+++ b/plugin/file/lookup.go
@@ -106,7 +106,7 @@ func (z *Zone) Lookup(state request.Request, qname string) ([]dns.RR, []dns.RR,
 			// Only one DNAME is allowed per name. We just pick the first one to synthesize from.
 			dname := dnamerrs[0]
 			if cname := synthesizeCNAME(state.Name(), dname.(*dns.DNAME)); cname != nil {
-				answer, ns, extra, rcode := z.searchCNAME(state, elem, []dns.RR{cname})
+				answer, ns, extra, rcode := z.additionalProcessing(state, elem, []dns.RR{cname})
 
 				if do {
 					sigs := elem.Types(dns.TypeRRSIG)
@@ -157,7 +157,7 @@ func (z *Zone) Lookup(state request.Request, qname string) ([]dns.RR, []dns.RR,
 	if found && shot {
 
 		if rrs := elem.Types(dns.TypeCNAME); len(rrs) > 0 && qtype != dns.TypeCNAME {
-			return z.searchCNAME(state, elem, rrs)
+			return z.additionalProcessing(state, elem, rrs)
 		}
 
 		rrs := elem.Types(qtype, qname)
@@ -193,7 +193,7 @@ func (z *Zone) Lookup(state request.Request, qname string) ([]dns.RR, []dns.RR,
 		auth := z.ns(do)
 
 		if rrs := wildElem.Types(dns.TypeCNAME, qname); len(rrs) > 0 {
-			return z.searchCNAME(state, wildElem, rrs)
+			return z.additionalProcessing(state, wildElem, rrs)
 		}
 
 		rrs := wildElem.Types(qtype, qname)
@@ -295,8 +295,8 @@ func (z *Zone) ns(do bool) []dns.RR {
 	return z.Apex.NS
 }
 
-// TODO(miek): should be better named, like aditionalProcessing?
-func (z *Zone) searchCNAME(state request.Request, elem *tree.Elem, rrs []dns.RR) ([]dns.RR, []dns.RR, []dns.RR, Result) {
+// aditionalProcessing adds signatures and tries to resolve CNAMEs that point to external names.
+func (z *Zone) additionalProcessing(state request.Request, elem *tree.Elem, rrs []dns.RR) ([]dns.RR, []dns.RR, []dns.RR, Result) {
 
 	qtype := state.QType()
 	do := state.Do()
@@ -312,9 +312,7 @@ func (z *Zone) searchCNAME(state request.Request, elem *tree.Elem, rrs []dns.RR)
 	targetName := rrs[0].(*dns.CNAME).Target
 	elem, _ = z.Tree.Search(targetName)
 	if elem == nil {
-		if !dns.IsSubDomain(z.origin, targetName) {
-			rrs = append(rrs, z.externalLookup(state, targetName, qtype)...)
-		}
+		rrs = append(rrs, z.externalLookup(state, targetName, qtype)...)
 		return rrs, z.ns(do), nil, Success
 	}
 
@@ -335,11 +333,7 @@ Redo:
 		targetName := cname[0].(*dns.CNAME).Target
 		elem, _ = z.Tree.Search(targetName)
 		if elem == nil {
-			if !dns.IsSubDomain(z.origin, targetName) {
-				if !dns.IsSubDomain(z.origin, targetName) {
-					rrs = append(rrs, z.externalLookup(state, targetName, qtype)...)
-				}
-			}
+			rrs = append(rrs, z.externalLookup(state, targetName, qtype)...)
 			return rrs, z.ns(do), nil, Success
 		}
 
@@ -380,7 +374,10 @@ func cnameForType(targets []dns.RR, origQtype uint16) []dns.RR {
 func (z *Zone) externalLookup(state request.Request, target string, qtype uint16) []dns.RR {
 	m, e := z.Upstream.Lookup(state, target, qtype)
 	if e != nil {
-		// TODO(miek): debugMsg for this as well? Log?
+		// TODO(miek): Log, or return error here?
+		return nil
+	}
+	if m == nil {
 		return nil
 	}
 	return m.Answer
diff --git a/plugin/pkg/upstream/upstream.go b/plugin/pkg/upstream/upstream.go
index fc85d3f5c..6ef131755 100644
--- a/plugin/pkg/upstream/upstream.go
+++ b/plugin/pkg/upstream/upstream.go
@@ -18,7 +18,8 @@ type Upstream struct {
 	Forward *proxy.Proxy
 }
 
-// NewUpstream creates a new Upstream for given destination(s)
+// NewUpstream creates a new Upstream for given destination(s). If dests is empty
+// it default to upstreaming to Self.
 func NewUpstream(dests []string) (Upstream, error) {
 	u := Upstream{}
 	if len(dests) == 0 {
@@ -35,21 +36,23 @@ func NewUpstream(dests []string) (Upstream, error) {
 	return u, nil
 }
 
-// Lookup routes lookups to Self or Forward
+// Lookup routes lookups to our selves or forward to a remote.
 func (u Upstream) Lookup(state request.Request, name string, typ uint16) (*dns.Msg, error) {
 	if u.self {
-		// lookup via self
 		req := new(dns.Msg)
 		req.SetQuestion(name, typ)
-		state.SizeAndDo(req)
+
 		nw := nonwriter.New(state.W)
-		state2 := request.Request{W: nw, Req: req}
 		server := state.Context.Value(dnsserver.Key{}).(*dnsserver.Server)
-		server.ServeDNS(state.Context, state2.W, req)
+
+		server.ServeDNS(state.Context, nw, req)
+
 		return nw.Msg, nil
 	}
+
 	if u.Forward != nil {
 		return u.Forward.Lookup(state, name, typ)
 	}
-	return &dns.Msg{}, nil
+
+	return nil, nil
 }
diff --git a/plugin/secondary/setup.go b/plugin/secondary/setup.go
index 1ccc09648..90c3a3dc5 100644
--- a/plugin/secondary/setup.go
+++ b/plugin/secondary/setup.go
@@ -5,8 +5,8 @@ import (
 	"github.com/coredns/coredns/plugin"
 	"github.com/coredns/coredns/plugin/file"
 	"github.com/coredns/coredns/plugin/pkg/parse"
-
 	"github.com/coredns/coredns/plugin/pkg/upstream"
+
 	"github.com/mholt/caddy"
 )
 
diff --git a/test/file_upstream_test.go b/test/file_upstream_test.go
index dc4cbc45f..36f2bbc56 100644
--- a/test/file_upstream_test.go
+++ b/test/file_upstream_test.go
@@ -6,8 +6,6 @@ import (
 	"github.com/miekg/dns"
 )
 
-// TODO(miek): this test needs to be fleshed out.
-
 func TestFileUpstream(t *testing.T) {
 	name, rm, err := TempFile(".", `$ORIGIN example.org.
 @	3600 IN	SOA sns.dns.icann.org. noc.dns.icann.org. (
@@ -59,3 +57,63 @@ www 3600 IN CNAME   www.example.net.
 		t.Errorf("Failed to get address for CNAME, expected 10.0.0.1 got %s", x)
 	}
 }
+
+// TestFileUpstreamAdditional runs two CoreDNS servers that serve example.org and foo.example.org.
+// example.org contains a cname to foo.example.org; this should be resolved via upstream.Self.
+func TestFileUpstreamAdditional(t *testing.T) {
+	name, rm, err := TempFile(".", `$ORIGIN example.org.
+@	3600 IN	SOA sns.dns.icann.org. noc.dns.icann.org. 2017042745 7200 3600 1209600 3600
+
+	3600 IN NS b.iana-servers.net.
+
+www 3600 IN CNAME   www.foo
+`)
+	if err != nil {
+		t.Fatalf("Failed to create zone: %s", err)
+	}
+	defer rm()
+
+	name2, rm2, err2 := TempFile(".", `$ORIGIN foo.example.org.
+@	3600 IN	SOA sns.dns.icann.org. noc.dns.icann.org. 2017042745 7200 3600 1209600 3600
+
+	3600 IN NS b.iana-servers.net.
+
+www 3600 IN A   127.0.0.53
+`)
+	if err2 != nil {
+		t.Fatalf("Failed to create zone: %s", err2)
+	}
+	defer rm2()
+
+	corefile := `.:0 {
+	file ` + name + ` example.org {
+	       upstream
+	}
+	file ` + name2 + ` foo.example.org {
+	       upstream
+	}
+}
+`
+	i, udp, _, err := CoreDNSServerAndPorts(corefile)
+	if err != nil {
+		t.Fatalf("Could not get CoreDNS serving instance: %s", err)
+	}
+	defer i.Stop()
+
+	m := new(dns.Msg)
+	m.SetQuestion("www.example.org.", dns.TypeA)
+
+	r, err := dns.Exchange(m, udp)
+	if err != nil {
+		t.Fatalf("Could not exchange msg: %s", err)
+	}
+	if r.Rcode == dns.RcodeServerFailure {
+		t.Fatalf("Rcode should not be dns.RcodeServerFailure")
+	}
+	if x := len(r.Answer); x != 2 {
+		t.Errorf("Expected 2 RR in reply, got %d", x)
+	}
+	if x := r.Answer[1].(*dns.A).A.String(); x != "127.0.0.53" {
+		t.Errorf("Failed to get address for CNAME, expected 127.0.0.53, got %s", x)
+	}
+}