Fix TestStubLookup and TestLookup (#213)

Changes large parts of proxy lookup mechanism.

The duplicate zone checking erroneous added a nameserver for each
zone we are auth. for, creating to many backend hosts. So even when a
host was determined do be Down() we still got an (identical) new one
from the list.

The Down() and failure checking for upstream hosts had data race in the
uh.Fails check - we now use atomic.LoadInt32 for that.

Use and debug the test/server.go test servers implementation in the
TestStubLookup test to prevent going out to the internet.

Also delete the stub cycle test. That test was wrong and did not test
what it needed to be testing.  Deleted for now.

middleware/etcd/multi_test.go

@@ -14,19 +14,20 @@ import (
 )
 
 func TestMultiLookup(t *testing.T) {
-	etcMulti := *etc
+	etc.Zones = []string{"skydns.test.", "miek.nl."}
-	etcMulti.Zones = []string{"skydns.test.", "miek.nl."}
+	defer func() { etc.Zones = []string{"skydns.test.", "skydns_extra.test.", "in-addr.arpa."} }()
-	etcMulti.Next = test.ErrorHandler()
+	etc.Next = test.ErrorHandler()
+	defer func() { etc.Next = nil }()
 
 	for _, serv := range servicesMulti {
-		set(t, &etcMulti, serv.Key, 0, serv)
+		set(t, etc, serv.Key, 0, serv)
-		defer delete(t, &etcMulti, serv.Key)
+		defer delete(t, etc, serv.Key)
 	}
 	for _, tc := range dnsTestCasesMulti {
 		m := tc.Msg()
 
 		rec := middleware.NewResponseRecorder(&test.ResponseWriter{})
-		_, err := etcMulti.ServeDNS(ctxt, rec, m)
+		_, err := etc.ServeDNS(ctxt, rec, m)
 		if err != nil {
 			t.Errorf("expected no error, got %v\n", err)
 			return

middleware/etcd/proxy_lookup_test.go

@@ -21,7 +21,7 @@ func TestProxyLookupFailDebug(t *testing.T) {
 	}
 
 	prxy := etc.Proxy
-	etc.Proxy = proxy.New([]string{"127.0.0.0:154"})
+	etc.Proxy = proxy.New([]string{"127.0.0.1:154"})
 	defer func() { etc.Proxy = prxy }()
 
 	etc.Debug = true

middleware/etcd/stub.go

@@ -37,6 +37,7 @@ func (e *Etcd) updateStubZones() {
 	// track the nameservers on a per domain basis, but allow a list on the domain.
 	nameservers := map[string][]string{}
 
+Services:
 	for _, serv := range services {
 		if serv.Port == 0 {
 			serv.Port = 53
@@ -58,11 +59,12 @@ func (e *Etcd) updateStubZones() {
 			domain = dns.Fqdn(strings.Join(labels[1:len(labels)-dns.CountLabel(z)-2], "."))
 			if domain == z {
 				log.Printf("[WARNING] Skipping nameserver for domain we are authoritative for: %s", domain)
-				continue
+				continue Services
 			}
-			nameservers[domain] = append(nameservers[domain], net.JoinHostPort(serv.Host, strconv.Itoa(serv.Port)))
 		}
+		nameservers[domain] = append(nameservers[domain], net.JoinHostPort(serv.Host, strconv.Itoa(serv.Port)))
 	}
+
 	for domain, nss := range nameservers {
 		stubmap[domain] = proxy.New(nss)
 	}

middleware/etcd/stub_cycle_test.go

@@ -1,40 +0,0 @@
-// +build etcd
-
-package etcd
-
-import (
-	"testing"
-
-	"github.com/miekg/coredns/middleware"
-	"github.com/miekg/coredns/middleware/test"
-	"github.com/miekg/dns"
-)
-
-func TestStubCycle(t *testing.T) {
-	// reuse servics from stub_test.go
-	for _, serv := range servicesStub {
-		set(t, etc, serv.Key, 0, serv)
-		defer delete(t, etc, serv.Key)
-	}
-	etc.updateStubZones()
-	defer func() { etc.Stubmap = nil }()
-
-	for _, tc := range dnsTestCasesCycleStub {
-		m := tc.Msg()
-		if tc.Do {
-			// add our wacky edns fluff
-			m.Extra[0] = ednsStub
-		}
-
-		rec := middleware.NewResponseRecorder(&test.ResponseWriter{})
-		_, err := etc.ServeDNS(ctxt, rec, m)
-		if err == nil {
-			t.Errorf("expected error, got none")
-			continue
-		}
-		// err should have been, set msg is nil, CoreDNS middlware handling takes
-		// care of proper error to client.
-	}
-}
-
-var dnsTestCasesCycleStub = []test.Case{{Qname: "example.org.", Qtype: dns.TypeA, Rcode: dns.RcodeRefused, Do: true}}

middleware/etcd/stub_test.go

@@ -3,7 +3,9 @@
 package etcd
 
 import (
+	"net"
 	"sort"
+	"strconv"
 	"testing"
 
 	"github.com/miekg/coredns/middleware"
@@ -13,11 +15,37 @@ import (
 	"github.com/miekg/dns"
 )
 
+func fakeStubServerExampleNet(t *testing.T) (*dns.Server, string) {
+	server, addr, err := test.UDPServer(t, "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("failed to create a UDP server: %s", err)
+	}
+	// add handler for example.net
+	dns.HandleFunc("example.net.", func(w dns.ResponseWriter, r *dns.Msg) {
+		t.Logf("writing response for example.net.")
+		m := new(dns.Msg)
+		m.SetReply(r)
+		m.Answer = []dns.RR{test.A("example.net.	86400	IN	A	93.184.216.34")}
+		w.WriteMsg(m)
+	})
+
+	return server, addr
+}
+
 func TestStubLookup(t *testing.T) {
+	server, addr := fakeStubServerExampleNet(t)
+	defer server.Shutdown()
+
+	host, p, _ := net.SplitHostPort(addr)
+	port, _ := strconv.Atoi(p)
+	exampleNetStub := &msg.Service{Host: host, Port: port, Key: "a.example.net.stub.dns.skydns.test."}
+	servicesStub = append(servicesStub, exampleNetStub)
+
 	for _, serv := range servicesStub {
 		set(t, etc, serv.Key, 0, serv)
 		defer delete(t, etc, serv.Key)
 	}
+
 	etc.updateStubZones()
 	defer func() { etc.Stubmap = nil }()
 
@@ -26,13 +54,13 @@ func TestStubLookup(t *testing.T) {
 
 		rec := middleware.NewResponseRecorder(&test.ResponseWriter{})
 		_, err := etc.ServeDNS(ctxt, rec, m)
-		if err != nil {
+		if err != nil && m.Question[0].Name == "example.org." {
-			if tc.Rcode != dns.RcodeServerFailure {
-				t.Errorf("expected no error, got %v\n", err)
-			}
 			// This is OK, we expect this backend to *not* work.
 			continue
 		}
+		if err != nil {
+			t.Errorf("expected no error, got %v for %s\n", err, m.Question[0].Name)
+		}
 		resp := rec.Msg()
 		if resp == nil {
 			// etcd not running?
@@ -63,8 +91,6 @@ var servicesStub = []*msg.Service{
 	// Two tests, ask a question that should return servfail because remote it no accessible
 	// and one with edns0 option added, that should return refused.
 	{Host: "127.0.0.1", Port: 666, Key: "b.example.org.stub.dns.skydns.test."},
-	// Actual test that goes out to the internet.
-	{Host: "199.43.132.53", Key: "a.example.net.stub.dns.skydns.test."},
 }
 
 var dnsTestCasesStub = []test.Case{
@@ -74,23 +100,6 @@ var dnsTestCasesStub = []test.Case{
 	{
 		Qname: "example.net.", Qtype: dns.TypeA,
 		Answer: []dns.RR{test.A("example.net.	86400	IN	A	93.184.216.34")},
-		Ns: []dns.RR{
-			test.NS("example.net.	86400	IN	NS	a.iana-servers.net."),
-			test.NS("example.net.	86400	IN	NS	b.iana-servers.net."),
-		},
 		Extra: []dns.RR{test.OPT(4096, false)}, // This will have an EDNS0 section, because *we* added our local stub forward to detect loops.
 	},
-	{
-		Qname: "example.net.", Qtype: dns.TypeA, Do: true,
-		Answer: []dns.RR{
-			test.A("example.net.	86400	IN	A	93.184.216.34"),
-			test.RRSIG("example.net.	86400	IN	RRSIG	A 8 2 86400 20160428060557 20160406182909 40948 example.net. Vm+rH5KN"),
-		},
-		Ns: []dns.RR{
-			test.NS("example.net.	86400	IN	NS	a.iana-servers.net."),
-			test.NS("example.net.	86400	IN	NS	b.iana-servers.net."),
-			test.RRSIG("example.net.	86400	IN	RRSIG	NS 8 2 86400 20160428110538 20160407002909 40948 example.net. z74YR2"),
-		},
-		Extra: []dns.RR{test.OPT(4096, true)},
-	},
 }

middleware/file/secondary_test.go

@@ -74,7 +74,7 @@ func TestShouldTransfer(t *testing.T) {
 	dns.HandleFunc(testZone, soa.Handler)
 	defer dns.HandleRemove(testZone)
 
-	s, addrstr, err := test.TCPServer("127.0.0.1:0")
+	s, addrstr, err := test.TCPServer(t, "127.0.0.1:0")
 	if err != nil {
 		t.Fatalf("unable to run test server: %v", err)
 	}
@@ -110,7 +110,7 @@ func TestTransferIn(t *testing.T) {
 	dns.HandleFunc(testZone, soa.Handler)
 	defer dns.HandleRemove(testZone)
 
-	s, addrstr, err := test.TCPServer("127.0.0.1:0")
+	s, addrstr, err := test.TCPServer(t, "127.0.0.1:0")
 	if err != nil {
 		t.Fatalf("unable to run test server: %v", err)
 	}

middleware/proxy/lookup.go

@@ -19,6 +19,7 @@ func New(hosts []string) Proxy {
 		from:        "",
 		Hosts:       make([]*UpstreamHost, len(hosts)),
 		Policy:      &Random{},
+		Spray:       nil,
 		FailTimeout: 10 * time.Second,
 		MaxFails:    1,
 	}
@@ -35,8 +36,8 @@ func New(hosts []string) Proxy {
 					if uh.Unhealthy {
 						return true
 					}
-					if uh.Fails >= upstream.MaxFails &&
+					fails := atomic.LoadInt32(&uh.Fails)
-						upstream.MaxFails != 0 {
+					if fails >= upstream.MaxFails && upstream.MaxFails != 0 {
 						return true
 					}
 					return false
@@ -77,13 +78,11 @@ func (p Proxy) lookup(state middleware.State, r *dns.Msg) (*dns.Msg, error) {
 		// hosts until timeout (or until we get a nil host).
 		for time.Now().Sub(start) < tryDuration {
 			host := upstream.Select()
-
 			if host == nil {
 				return nil, errUnreachable
 			}
 
 			atomic.AddInt64(&host.Conns, 1)
-			// tls+tcp ?
 			if state.Proto() == "tcp" {
 				reply, err = middleware.Exchange(p.Client.TCP, r, host.Name)
 			} else {

middleware/proxy/proxy.go

@@ -59,7 +59,8 @@ type UpstreamHost struct {
 func (uh *UpstreamHost) Down() bool {
 	if uh.CheckDown == nil {
 		// Default settings
-		return uh.Unhealthy || uh.Fails > 0
+		fails := atomic.LoadInt32(&uh.Fails)
+		return uh.Unhealthy || fails > 0
 	}
 	return uh.CheckDown(uh)
 }

middleware/proxy/upstream.go

@@ -8,6 +8,7 @@ import (
 	"net/http"
 	"strconv"
 	"strings"
+	"sync/atomic"
 	"time"
 
 	"github.com/miekg/coredns/core/parse"
@@ -90,8 +91,9 @@ func NewStaticUpstreams(c parse.Dispenser) ([]Upstream, error) {
 						if uh.Unhealthy {
 							return true
 						}
-						if uh.Fails >= upstream.MaxFails &&
+
-							upstream.MaxFails != 0 {
+						fails := atomic.LoadInt32(&uh.Fails)
+						if fails >= upstream.MaxFails && upstream.MaxFails != 0 {
 							return true
 						}
 						return false

middleware/test/server.go

@@ -3,12 +3,13 @@ package test
 import (
 	"net"
 	"sync"
+	"testing"
 	"time"
 
 	"github.com/miekg/dns"
 )
 
-func TCPServer(laddr string) (*dns.Server, string, error) {
+func TCPServer(t *testing.T, laddr string) (*dns.Server, string, error) {
 	l, err := net.Listen("tcp", laddr)
 	if err != nil {
 		return nil, "", err
@@ -18,7 +19,7 @@ func TCPServer(laddr string) (*dns.Server, string, error) {
 
 	waitLock := sync.Mutex{}
 	waitLock.Lock()
-	server.NotifyStartedFunc = waitLock.Unlock
+	server.NotifyStartedFunc = func() { t.Logf("started TCP server on %s", l.Addr()); waitLock.Unlock() }
 
 	go func() {
 		server.ActivateAndServe()
@@ -29,25 +30,22 @@ func TCPServer(laddr string) (*dns.Server, string, error) {
 	return server, l.Addr().String(), nil
 }
 
-func UDPServer(laddr string) (*dns.Server, string, chan bool, error) {
+func UDPServer(t *testing.T, laddr string) (*dns.Server, string, error) {
 	pc, err := net.ListenPacket("udp", laddr)
 	if err != nil {
-		return nil, "", nil, err
+		return nil, "", err
 	}
 	server := &dns.Server{PacketConn: pc, ReadTimeout: time.Hour, WriteTimeout: time.Hour}
 
 	waitLock := sync.Mutex{}
 	waitLock.Lock()
-	server.NotifyStartedFunc = waitLock.Unlock
+	server.NotifyStartedFunc = func() { t.Logf("started UDP server on %s", pc.LocalAddr()); waitLock.Unlock() }
-
-	stop := make(chan bool)
 
 	go func() {
 		server.ActivateAndServe()
-		close(stop)
 		pc.Close()
 	}()
 
 	waitLock.Lock()
-	return server, pc.LocalAddr().String(), stop, nil
+	return server, pc.LocalAddr().String(), nil
 }