middleware/file: don't reload zone when SOA isn't changed (#707)

* middleware/file: don't reload zone when SOA isn't changed

Give Parse an extra argument which is the SOA's serial, if > 0 we check
against the just parsed SOA and then just return.

Most notable use is in reload.go which is both used in the file and auto
middleware.

Fixes #415

* PR comments

middleware/auto/walk.go

@@ -45,9 +45,10 @@ func (a Auto) Walk() error {
 		}
 		defer reader.Close()
 
-		zo, err := file.Parse(reader, origin, path)
+		// Serial for loading a zone is 0, because it is a new zone.
+		zo, err := file.Parse(reader, origin, path, 0)
 		if err != nil {
-			// Parse barfs warning by itself...
+			log.Printf("[WARNING] Parse zone `%s': %v", origin, err)
 			return nil
 		}
 

middleware/dnssec/handler_test.go

@@ -81,7 +81,7 @@ var dnsTestCases = []test.Case{
 }
 
 func TestLookupZone(t *testing.T) {
-	zone, err := file.Parse(strings.NewReader(dbMiekNL), "miek.nl.", "stdin")
+	zone, err := file.Parse(strings.NewReader(dbMiekNL), "miek.nl.", "stdin", 0)
 	if err != nil {
 		return
 	}

middleware/file/closest_test.go

@@ -6,7 +6,7 @@ import (
 )
 
 func TestClosestEncloser(t *testing.T) {
-	z, err := Parse(strings.NewReader(dbMiekNL), testzone, "stdin")
+	z, err := Parse(strings.NewReader(dbMiekNL), testzone, "stdin", 0)
 	if err != nil {
 		t.Fatalf("expect no error when reading zone, got %q", err)
 	}

middleware/file/cname_test.go

@@ -15,7 +15,7 @@ import (
 
 func TestLookupCNAMEChain(t *testing.T) {
 	name := "example.org."
-	zone, err := Parse(strings.NewReader(dbExampleCNAME), name, "stdin")
+	zone, err := Parse(strings.NewReader(dbExampleCNAME), name, "stdin", 0)
 	if err != nil {
 		t.Fatalf("Expected no error when reading zone, got %q", err)
 	}
@@ -89,7 +89,7 @@ var cnameTestCases = []test.Case{
 
 func TestLookupCNAMEExternal(t *testing.T) {
 	name := "example.org."
-	zone, err := Parse(strings.NewReader(dbExampleCNAME), name, "stdin")
+	zone, err := Parse(strings.NewReader(dbExampleCNAME), name, "stdin", 0)
 	if err != nil {
 		t.Fatalf("Expected no error when reading zone, got %q", err)
 	}

middleware/file/delegation_test.go

@@ -152,7 +152,7 @@ func TestLookupSecureDelegation(t *testing.T) {
 }
 
 func testDelegation(t *testing.T, z, origin string, testcases []test.Case) {
-	zone, err := Parse(strings.NewReader(z), origin, "stdin")
+	zone, err := Parse(strings.NewReader(z), origin, "stdin", 0)
 	if err != nil {
 		t.Fatalf("Expect no error when reading zone, got %q", err)
 	}

middleware/file/dname_test.go

@@ -91,7 +91,7 @@ var dnameTestCases = []test.Case{
 }
 
 func TestLookupDNAME(t *testing.T) {
-	zone, err := Parse(strings.NewReader(dbMiekNLDNAME), testzone, "stdin")
+	zone, err := Parse(strings.NewReader(dbMiekNLDNAME), testzone, "stdin", 0)
 	if err != nil {
 		t.Fatalf("Expect no error when reading zone, got %q", err)
 	}
@@ -160,7 +160,7 @@ var dnameDnssecTestCases = []test.Case{
 }
 
 func TestLookupDNAMEDNSSEC(t *testing.T) {
-	zone, err := Parse(strings.NewReader(dbExampleDNAMESigned), testzone, "stdin")
+	zone, err := Parse(strings.NewReader(dbExampleDNAMESigned), testzone, "stdin", 0)
 	if err != nil {
 		t.Fatalf("Expect no error when reading zone, got %q", err)
 	}

middleware/file/dnssec_test.go

@@ -128,7 +128,7 @@ var auth = []dns.RR{
 }
 
 func TestLookupDNSSEC(t *testing.T) {
-	zone, err := Parse(strings.NewReader(dbMiekNLSigned), testzone, "stdin")
+	zone, err := Parse(strings.NewReader(dbMiekNLSigned), testzone, "stdin", 0)
 	if err != nil {
 		t.Fatalf("Expected no error when reading zone, got %q", err)
 	}
@@ -170,7 +170,7 @@ func TestLookupDNSSEC(t *testing.T) {
 }
 
 func BenchmarkLookupDNSSEC(b *testing.B) {
-	zone, err := Parse(strings.NewReader(dbMiekNLSigned), testzone, "stdin")
+	zone, err := Parse(strings.NewReader(dbMiekNLSigned), testzone, "stdin", 0)
 	if err != nil {
 		return
 	}

middleware/file/ds_test.go

@@ -52,7 +52,7 @@ var dsTestCases = []test.Case{
 }
 
 func TestLookupDS(t *testing.T) {
-	zone, err := Parse(strings.NewReader(dbMiekNLDelegation), testzone, "stdin")
+	zone, err := Parse(strings.NewReader(dbMiekNLDelegation), testzone, "stdin", 0)
 	if err != nil {
 		t.Fatalf("Expected no error when reading zone, got %q", err)
 	}

middleware/file/ent_test.go

@@ -32,7 +32,7 @@ var entTestCases = []test.Case{
 }
 
 func TestLookupEnt(t *testing.T) {
-	zone, err := Parse(strings.NewReader(dbMiekENTNL), testzone, "stdin")
+	zone, err := Parse(strings.NewReader(dbMiekENTNL), testzone, "stdin", 0)
 	if err != nil {
 		t.Fatalf("expect no error when reading zone, got %q", err)
 	}

middleware/file/file.go

@@ -3,6 +3,7 @@ package file
 
 import (
 	"errors"
+	"fmt"
 	"io"
 	"log"
 
@@ -109,14 +110,26 @@ func (f File) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (i
 func (f File) Name() string { return "file" }
 
 // Parse parses the zone in filename and returns a new Zone or an error.
-func Parse(f io.Reader, origin, fileName string) (*Zone, error) {
+// If serial >= 0 it will reload the zone, if the SOA hasn't changed
+// it returns an error indicating nothing was read.
+func Parse(f io.Reader, origin, fileName string, serial int64) (*Zone, error) {
 	tokens := dns.ParseZone(f, dns.Fqdn(origin), fileName)
 	z := NewZone(origin, fileName)
+	seenSOA := false
 	for x := range tokens {
 		if x.Error != nil {
-			log.Printf("[ERROR] Failed to parse `%s': %v", origin, x.Error)
 			return nil, x.Error
 		}
+
+		if !seenSOA && serial >= 0 {
+			if s, ok := x.RR.(*dns.SOA); ok {
+				if s.Serial == uint32(serial) { // same zone
+					return nil, fmt.Errorf("no change in serial: %d", serial)
+				}
+			}
+			seenSOA = true
+		}
+
 		if err := z.Insert(x.RR); err != nil {
 			return nil, err
 		}

middleware/file/file_test.go

@@ -7,6 +7,6 @@ import (
 
 func BenchmarkParseInsert(b *testing.B) {
 	for i := 0; i < b.N; i++ {
-		Parse(strings.NewReader(dbMiekENTNL), testzone, "stdin")
+		Parse(strings.NewReader(dbMiekENTNL), testzone, "stdin", 0)
 	}
 }

middleware/file/glue_test.go

@@ -34,7 +34,7 @@ var atoomTestCases = []test.Case{
 }
 
 func TestLookupGlue(t *testing.T) {
-	zone, err := Parse(strings.NewReader(dbAtoomNetSigned), atoom, "stdin")
+	zone, err := Parse(strings.NewReader(dbAtoomNetSigned), atoom, "stdin", 0)
 	if err != nil {
 		t.Fatalf("Expected no error when reading zone, got %q", err)
 	}

middleware/file/lookup_test.go

@@ -104,7 +104,7 @@ const (
 )
 
 func TestLookup(t *testing.T) {
-	zone, err := Parse(strings.NewReader(dbMiekNL), testzone, "stdin")
+	zone, err := Parse(strings.NewReader(dbMiekNL), testzone, "stdin", 0)
 	if err != nil {
 		t.Fatalf("expect no error when reading zone, got %q", err)
 	}
@@ -155,7 +155,7 @@ func TestLookupNil(t *testing.T) {
 }
 
 func BenchmarkLookup(b *testing.B) {
-	zone, err := Parse(strings.NewReader(dbMiekNL), testzone, "stdin")
+	zone, err := Parse(strings.NewReader(dbMiekNL), testzone, "stdin", 0)
 	if err != nil {
 		return
 	}

middleware/file/nsec3_test.go

@@ -6,14 +6,14 @@ import (
 )
 
 func TestParseNSEC3PARAM(t *testing.T) {
-	_, err := Parse(strings.NewReader(nsec3paramTest), "miek.nl", "stdin")
+	_, err := Parse(strings.NewReader(nsec3paramTest), "miek.nl", "stdin", 0)
 	if err == nil {
 		t.Fatalf("expected error when reading zone, got nothing")
 	}
 }
 
 func TestParseNSEC3(t *testing.T) {
-	_, err := Parse(strings.NewReader(nsec3Test), "miek.nl", "stdin")
+	_, err := Parse(strings.NewReader(nsec3Test), "miek.nl", "stdin", 0)
 	if err == nil {
 		t.Fatalf("expected error when reading zone, got nothing")
 	}

middleware/file/reload.go

@@ -0,0 +1,72 @@
+package file
+
+import (
+	"log"
+	"os"
+	"path"
+
+	"github.com/fsnotify/fsnotify"
+)
+
+// Reload reloads a zone when it is changed on disk. If z.NoRoload is true, no reloading will be done.
+func (z *Zone) Reload() error {
+	if z.NoReload {
+		return nil
+	}
+	watcher, err := fsnotify.NewWatcher()
+	if err != nil {
+		return err
+	}
+	err = watcher.Add(path.Dir(z.file))
+	if err != nil {
+		return err
+	}
+
+	go func() {
+		// TODO(miek): needs to be killed on reload.
+		for {
+			select {
+			case event := <-watcher.Events:
+				if path.Clean(event.Name) == z.file {
+
+					reader, err := os.Open(z.file)
+					if err != nil {
+						log.Printf("[ERROR] Failed to open `%s' for `%s': %v", z.file, z.origin, err)
+						continue
+					}
+
+					serial := z.SOASerialIfDefined()
+					zone, err := Parse(reader, z.origin, z.file, serial)
+					if err != nil {
+						log.Printf("[WARNING] Parsing zone `%s': %v", z.origin, err)
+						continue
+					}
+
+					// copy elements we need
+					z.reloadMu.Lock()
+					z.Apex = zone.Apex
+					z.Tree = zone.Tree
+					z.reloadMu.Unlock()
+
+					log.Printf("[INFO] Successfully reloaded zone `%s'", z.origin)
+					z.Notify()
+				}
+			case <-z.ReloadShutdown:
+				watcher.Close()
+				return
+			}
+		}
+	}()
+	return nil
+}
+
+// SOASerialIfDefind returns the SOA's serial if the zone has a SOA record in the Apex, or
+// -1 otherwise.
+func (z *Zone) SOASerialIfDefined() int64 {
+	z.reloadMu.Lock()
+	defer z.reloadMu.Unlock()
+	if z.Apex.SOA != nil {
+		return int64(z.Apex.SOA.Serial)
+	}
+	return -1
+}

middleware/file/reload_test.go

@@ -4,6 +4,7 @@ import (
 	"io/ioutil"
 	"log"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -25,7 +26,7 @@ func TestZoneReload(t *testing.T) {
 	if err != nil {
 		t.Fatalf("failed to open zone: %s", err)
 	}
-	z, err := Parse(reader, "miek.nl", fileName)
+	z, err := Parse(reader, "miek.nl", fileName, 0)
 	if err != nil {
 		t.Fatalf("failed to parse zone: %s", err)
 	}
@@ -60,6 +61,14 @@ func TestZoneReload(t *testing.T) {
 	}
 }
 
+func TestZoneReloadSOAChange(t *testing.T) {
+	_, err := Parse(strings.NewReader(reloadZoneTest), "miek.nl.", "stdin", 1460175181)
+	if err == nil {
+		t.Fatalf("zone should not have been re-parsed")
+	}
+
+}
+
 const reloadZoneTest = `miek.nl.		1627	IN	SOA	linode.atoom.net. miek.miek.nl. 1460175181 14400 3600 604800 14400
 miek.nl.		1627	IN	NS	ext.ns.whyscream.net.
 miek.nl.		1627	IN	NS	omval.tednet.nl.
@@ -67,7 +76,7 @@ miek.nl.		1627	IN	NS	linode.atoom.net.
 miek.nl.		1627	IN	NS	ns-ext.nlnetlabs.nl.
 `
 
-const reloadZone2Test = `miek.nl.		1627	IN	SOA	linode.atoom.net. miek.miek.nl. 1460175181 14400 3600 604800 14400
+const reloadZone2Test = `miek.nl.		1627	IN	SOA	linode.atoom.net. miek.miek.nl. 1460175182 14400 3600 604800 14400
 miek.nl.		1627	IN	NS	ext.ns.whyscream.net.
 miek.nl.		1627	IN	NS	omval.tednet.nl.
 `

middleware/file/setup.go

@@ -81,7 +81,7 @@ func fileParse(c *caddy.Controller) (Zones, error) {
 
 			for i := range origins {
 				origins[i] = middleware.Host(origins[i]).Normalize()
-				zone, err := Parse(reader, origins[i], fileName)
+				zone, err := Parse(reader, origins[i], fileName, 0)
 				if err == nil {
 					z[origins[i]] = zone
 				} else {

middleware/file/wildcard_test.go

@@ -79,7 +79,7 @@ var dnssexAuth = []dns.RR{
 }
 
 func TestLookupWildcard(t *testing.T) {
-	zone, err := Parse(strings.NewReader(dbDnssexNLSigned), testzone1, "stdin")
+	zone, err := Parse(strings.NewReader(dbDnssexNLSigned), testzone1, "stdin", 0)
 	if err != nil {
 		t.Fatalf("Expect no error when reading zone, got %q", err)
 	}
@@ -156,7 +156,7 @@ var exampleAuth = []dns.RR{
 }
 
 func TestLookupDoubleWildcard(t *testing.T) {
-	zone, err := Parse(strings.NewReader(exampleOrg), "example.org.", "stdin")
+	zone, err := Parse(strings.NewReader(exampleOrg), "example.org.", "stdin", 0)
 	if err != nil {
 		t.Fatalf("Expect no error when reading zone, got %q", err)
 	}

middleware/file/xfr_test.go

@@ -6,7 +6,7 @@ import (
 )
 
 func ExampleZone_All() {
-	zone, err := Parse(strings.NewReader(dbMiekNL), testzone, "stdin")
+	zone, err := Parse(strings.NewReader(dbMiekNL), testzone, "stdin", 0)
 	if err != nil {
 		return
 	}

middleware/file/zone.go

@@ -2,8 +2,6 @@ package file
 
 import (
 	"fmt"
-	"log"
-	"os"
 	"path"
 	"strings"
 	"sync"
@@ -12,7 +10,6 @@ import (
 	"github.com/coredns/coredns/middleware/proxy"
 	"github.com/coredns/coredns/request"
 
-	"github.com/fsnotify/fsnotify"
 	"github.com/miekg/dns"
 )
 
@@ -151,56 +148,6 @@ func (z *Zone) All() []dns.RR {
 	return append([]dns.RR{z.Apex.SOA}, records...)
 }
 
-// Reload reloads a zone when it is changed on disk. If z.NoRoload is true, no reloading will be done.
-func (z *Zone) Reload() error {
-	if z.NoReload {
-		return nil
-	}
-	watcher, err := fsnotify.NewWatcher()
-	if err != nil {
-		return err
-	}
-	err = watcher.Add(path.Dir(z.file))
-	if err != nil {
-		return err
-	}
-
-	go func() {
-		// TODO(miek): needs to be killed on reload.
-		for {
-			select {
-			case event := <-watcher.Events:
-				if path.Clean(event.Name) == z.file {
-
-					reader, err := os.Open(z.file)
-					if err != nil {
-						log.Printf("[ERROR] Failed to open `%s' for `%s': %v", z.file, z.origin, err)
-						continue
-					}
-					zone, err := Parse(reader, z.origin, z.file)
-					if err != nil {
-						log.Printf("[ERROR] Failed to parse `%s': %v", z.origin, err)
-						continue
-					}
-
-					// copy elements we need
-					z.reloadMu.Lock()
-					z.Apex = zone.Apex
-					z.Tree = zone.Tree
-					z.reloadMu.Unlock()
-
-					log.Printf("[INFO] Successfully reloaded zone `%s'", z.origin)
-					z.Notify()
-				}
-			case <-z.ReloadShutdown:
-				watcher.Close()
-				return
-			}
-		}
-	}()
-	return nil
-}
-
 // Print prints the zone's tree to stdout.
 func (z *Zone) Print() {
 	z.Tree.Print()