Fixes races in test and klog (#3079)

Various fixes to make things less flaky:

* kubernetes: put klog.SetOutput in the setup function, not in the init
  function to see if that helps
* file: make z.Expired a boolean instead of a pointer to a boolean
* test: fix TestSecondaryZoneTransfer test, which wasn't actually
  testing in the right way. It's more right now, but may still be racy
  (race introduced because a file's lazy loading of zones)

Signed-off-by: Miek Gieben <miek@miek.nl>

6 files changed, 11 insertions, 16 deletions

diff --git a/plugin/file/file.go b/plugin/file/file.go
index 8e10499fa..1aebf6cda 100644
--- a/plugin/file/file.go
+++ b/plugin/file/file.go
@@ -72,7 +72,7 @@ func (f File) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (i
 	z.RLock()
 	exp := z.Expired
 	z.RUnlock()
-	if exp != nil && *exp {
+	if exp {
 		log.Errorf("Zone %s is expired", zone)
 		return dns.RcodeServerFailure, nil
 	}
diff --git a/plugin/file/secondary.go b/plugin/file/secondary.go
index 408ee887a..6cb571fe5 100644
--- a/plugin/file/secondary.go
+++ b/plugin/file/secondary.go
@@ -54,7 +54,7 @@ Transfer:
 	z.Lock()
 	z.Tree = z1.Tree
 	z.Apex = z1.Apex
-	*z.Expired = false
+	z.Expired = false
 	z.Unlock()
 	log.Infof("Transferred: %s from %s", z.origin, tr)
 	return nil
@@ -129,7 +129,7 @@ Restart:
 			if !retryActive {
 				break
 			}
-			*z.Expired = true
+			z.Expired = true
 
 		case <-retryTicker.C:
 			if !retryActive {
diff --git a/plugin/file/secondary_test.go b/plugin/file/secondary_test.go
index db98b4f97..5c393e440 100644
--- a/plugin/file/secondary_test.go
+++ b/plugin/file/secondary_test.go
@@ -125,7 +125,6 @@ func TestTransferIn(t *testing.T) {
 	defer s.Shutdown()
 
 	z := new(Zone)
-	z.Expired = new(bool)
 	z.origin = testZone
 	z.TransferFrom = []string{addrstr}
 
@@ -140,7 +139,6 @@ func TestTransferIn(t *testing.T) {
 
 func TestIsNotify(t *testing.T) {
 	z := new(Zone)
-	z.Expired = new(bool)
 	z.origin = testZone
 	state := newRequest(testZone, dns.TypeSOA)
 	// need to set opcode
diff --git a/plugin/file/zone.go b/plugin/file/zone.go
index d3adca29e..9a12ca69f 100644
--- a/plugin/file/zone.go
+++ b/plugin/file/zone.go
@@ -22,7 +22,7 @@ type Zone struct {
 	file    string
 	*tree.Tree
 	Apex
-	Expired *bool
+	Expired bool
 
 	sync.RWMutex
 
@@ -46,16 +46,13 @@ type Apex struct {
 
 // NewZone returns a new zone.
 func NewZone(name, file string) *Zone {
-	z := &Zone{
+	return &Zone{
 		origin:         dns.Fqdn(name),
 		origLen:        dns.CountLabel(dns.Fqdn(name)),
 		file:           filepath.Clean(file),
 		Tree:           &tree.Tree{},
-		Expired:        new(bool),
 		reloadShutdown: make(chan bool),
 	}
-	*z.Expired = false
-	return z
 }
 
 // Copy copies a zone.
diff --git a/plugin/kubernetes/setup.go b/plugin/kubernetes/setup.go
index 1b0f20769..00c698181 100644
--- a/plugin/kubernetes/setup.go
+++ b/plugin/kubernetes/setup.go
@@ -19,7 +19,7 @@ import (
 	"github.com/miekg/dns"
 	meta "k8s.io/apimachinery/pkg/apis/meta/v1"
 
-	// Pull this in for logtostderr flag parsing
+	// Pull this in setting klog's output to stdout
 	"k8s.io/klog"
 
 	// Excluding azure because it is failing to compile
@@ -35,8 +35,6 @@ import (
 var log = clog.NewWithPlugin("kubernetes")
 
 func init() {
-	klog.SetOutput(os.Stdout)
-
 	caddy.RegisterPlugin("kubernetes", caddy.Plugin{
 		ServerType: "dns",
 		Action:     setup,
@@ -44,6 +42,8 @@ func init() {
 }
 
 func setup(c *caddy.Controller) error {
+	klog.SetOutput(os.Stdout)
+
 	k, err := kubernetesParse(c)
 	if err != nil {
 		return plugin.Error("kubernetes", err)
diff --git a/test/secondary_test.go b/test/secondary_test.go
index bfa1da05b..c0df1632b 100644
--- a/test/secondary_test.go
+++ b/test/secondary_test.go
@@ -73,13 +73,13 @@ func TestSecondaryZoneTransfer(t *testing.T) {
 	var r *dns.Msg
 	// This is now async; we we need to wait for it to be transferred.
 	for i := 0; i < 10; i++ {
-		r, _ = dns.Exchange(m, udp)
-		if len(r.Answer) == 0 {
+		r, err = dns.Exchange(m, udp)
+		if len(r.Answer) != 0 {
 			break
 		}
 		time.Sleep(100 * time.Microsecond)
 	}
-	if len(r.Answer) != 0 {
+	if len(r.Answer) == 0 {
 		t.Fatalf("Expected answer section")
 	}
 }