Document fallthrough and fix rewrite (#537)

* Document fallthrough and fix *reverse*

While documenting the fallthrough behavior and testing it I noticed
the did not properly work. This PR does a tiny bit too much as it

- Documents fallthrough
- Fixes fallthrough in reverse
- Makes directives_generate complain on duplicate priorities
- Moved reverse *before* file in middleware.cfg
- Add a test that tests the reverse fallthrough behavior with a file
  backend

Fixes #515

* ....and fix the tests

9 files changed, 215 insertions, 57 deletions

diff --git a/core/dnsserver/zdirectives.go b/core/dnsserver/zdirectives.go
index d6c7e4657..9de0df252 100644
--- a/core/dnsserver/zdirectives.go
+++ b/core/dnsserver/zdirectives.go
@@ -23,12 +23,12 @@ var directives = []string{
 	"rewrite",
 	"loadbalance",
 	"dnssec",
+	"reverse",
 	"file",
 	"auto",
 	"secondary",
 	"etcd",
 	"kubernetes",
-	"reverse",
 	"proxy",
 	"whoami",
 	"erratic",
diff --git a/directives_generate.go b/directives_generate.go
index 62e69e165..98d368c86 100644
--- a/directives_generate.go
+++ b/directives_generate.go
@@ -37,6 +37,9 @@ func main() {
 		priority, err := strconv.Atoi(items[0])
 		fatalIfErr(err)
 
+		if v, ok := md[priority]; ok {
+			log.Fatalf("Duplicate priority '%d', slot already taken by %q", priority, v)
+		}
 		md[priority] = items[1]
 		mi[items[1]] = middlewarePath + items[2] // Default, unless overriden by 3rd arg
 
diff --git a/middleware.cfg b/middleware.cfg
index dcec2c99a..b43a71dcf 100644
--- a/middleware.cfg
+++ b/middleware.cfg
@@ -32,12 +32,12 @@
 110:rewrite:rewrite
 120:loadbalance:loadbalance
 130:dnssec:dnssec
-140:file:file
-150:auto:auto
-160:secondary:secondary
-170:etcd:etcd
-180:kubernetes:kubernetes
-185:reverse:reverse
-190:proxy:proxy
+140:reverse:reverse
+150:file:file
+160:auto:auto
+170:secondary:secondary
+180:etcd:etcd
+190:kubernetes:kubernetes
+200:proxy:proxy
 210:whoami:whoami
 220:erratic:erratic
diff --git a/middleware.md b/middleware.md
index 98a6adf89..46e6e0673 100644
--- a/middleware.md
+++ b/middleware.md
@@ -29,9 +29,12 @@ So CoreDNS treats:
 as special and will then assume nothing has written to the client. In all other cases it is assumes
 something has been written to the client (by the middleware).
 
-## Hooking it up
+## Hooking It Up
 
-TODO(miek): text here on how to hook up middleware.
+See a couple of blog posts on how to write and add middleware to CoreDNS:
+
+* <https://blog.coredns.io/#> TO BE PUBLISHED.
+* <https://blog.coredns.io/2016/12/19/writing-middleware-for-coredns/>, slightly older, but useful.
 
 ## Metrics
 
@@ -60,3 +63,72 @@ We use the Unix manual page style:
 * Optional text: in block quotes: `[optional]`.
 * Use three dots to indicate multiple options are allowed: `arg...`.
 * Item used literal: `literal`.
+
+### Example Domain Names
+
+Please be sure to use `example.org` or `example.net` in any examples you provide. These are the
+standard domain names created for this purpose.
+
+## Fallthrough
+
+In a perfect world the following would be true for middleware: "Either you are responsible for
+a zone or not". If the answer is "not", the middleware should call the next middleware in the chain.
+If "yes" it should handle *all* names that fall in this zone and the names below - i.e. it should
+handle the entire domain.
+
+~~~ txt
+. {
+    file example.org db.example
+}
+~~~
+In this example the *file* middleware is handling all names below (and including) `example.org`. If
+a query comes in that is not a subdomain (or equal to) `example.org` the next middleware is called.
+
+Now, the world isn't perfect, and there are good reasons to "fallthrough" to the next middlware,
+meaning a middleware is only responsible for a subset of names within the zone. The first of these
+to appear was the *reverse* middleware that synthesis PTR and A/AAAA responses (useful with IPv6).
+
+The nature of the *reverse* middleware is such that it only deals with A,AAAA and PTR and then only
+for a subset of the names. Ideally you would want to layer *reverse* **in front off** another
+middleware such as *file* or *auto* (or even *proxy*). This means *reverse* handles some special
+reverse cases and **all other** request are handled by the backing middleware. This is exactly what
+"fallthrough" does. To keep things explicit we've opted that middlewares implement such behavior
+should implement a `fallthrough` keyword.
+
+### Example Fallthrough Usage
+
+The following Corefile example, sets up the *reverse* middleware, but disables fallthrough. It
+also defines a zonefile for use with the *file* middleware for other names in the `compute.internal`.
+
+~~~ txt
+arpa compute.internal {
+    reverse 10.32.0.0/16 {
+        hostname ip-{ip}.{zone[2]}
+        #fallthrough
+    }
+    file db.compute.internal compute.internal
+}
+~~~
+
+This works for returning a response to a PTR request:
+
+~~~ sh
+% dig +nocmd @localhost +noall +ans -x 10.32.0.1
+1.0.32.10.in-addr.arpa.	3600	IN	PTR	ip-10-32-0-1.compute.internal.
+~~~
+
+And for the forward:
+
+~~~ sh
+% dig +nocmd @localhost +noall +ans A ip-10-32-0-1.compute.internal
+ip-10-32-0-1.compute.internal. 3600 IN	A	10.32.0.1
+~~~
+
+But a query for `mx compute.internal` will return SERVFAIL. Now when we remove the '#' from
+fallthrough and reload (on Unix: `kill -SIGUSR1 $(pidof coredns)`) CoreDNS, we *should* get an
+answer for the MX query:
+
+~~~ sh
+% dig +nocmd @localhost +noall +ans MX compute.internal
+compute.internal.	3600	IN	MX	10 mx.compute.internal.
+~~~
diff --git a/middleware/reverse/network.go b/middleware/reverse/network.go
index f5101e46d..83ce21309 100644
--- a/middleware/reverse/network.go
+++ b/middleware/reverse/network.go
@@ -13,7 +13,6 @@ type network struct {
 	Template     string
 	TTL          uint32
 	RegexMatchIP *regexp.Regexp
-	Fallthrough  bool
 }
 
 // TODO: we might want to get rid of these regexes.
diff --git a/middleware/reverse/reverse.go b/middleware/reverse/reverse.go
index 3e019affd..44d28ddc3 100644
--- a/middleware/reverse/reverse.go
+++ b/middleware/reverse/reverse.go
@@ -13,16 +13,14 @@ import (
 
 // Reverse provides dynamic reverse DNS and the related forward RR.
 type Reverse struct {
-	Next     middleware.Handler
-	Networks networks
+	Next        middleware.Handler
+	Networks    networks
+	Fallthrough bool
 }
 
 // ServeDNS implements the middleware.Handler interface.
 func (re Reverse) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
-	var (
-		rr          dns.RR
-		fallThrough bool
-	)
+	var rr dns.RR
 
 	state := request.Request{W: w, Req: r}
 	m := new(dns.Msg)
@@ -42,7 +40,6 @@ func (re Reverse) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg
 		// loop through the configured networks
 		for _, n := range re.Networks {
 			if n.IPnet.Contains(ip) {
-				fallThrough = n.Fallthrough
 				rr = &dns.PTR{
 					Hdr: dns.RR_Header{Name: state.QName(), Rrtype: dns.TypePTR, Class: dns.ClassINET, Ttl: n.TTL},
 					Ptr: n.ipToHostname(ip),
@@ -54,7 +51,6 @@ func (re Reverse) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg
 	case dns.TypeA:
 		for _, n := range re.Networks {
 			if dns.IsSubDomain(n.Zone, state.Name()) {
-				fallThrough = n.Fallthrough
 
 				// skip if requesting an v4 address and network is not v4
 				if n.IPnet.IP.To4() == nil {
@@ -75,7 +71,6 @@ func (re Reverse) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg
 	case dns.TypeAAAA:
 		for _, n := range re.Networks {
 			if dns.IsSubDomain(n.Zone, state.Name()) {
-				fallThrough = n.Fallthrough
 
 				// Do not use To16 which tries to make v4 in v6
 				if n.IPnet.IP.To4() != nil {
@@ -95,14 +90,17 @@ func (re Reverse) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg
 
 	}
 
-	if rr == nil && !fallThrough {
-		return middleware.NextOrFailure(re.Name(), re.Next, ctx, w, r)
+	if rr != nil {
+		m.Answer = append(m.Answer, rr)
+		state.SizeAndDo(m)
+		w.WriteMsg(m)
+		return dns.RcodeSuccess, nil
 	}
 
-	m.Answer = append(m.Answer, rr)
-	state.SizeAndDo(m)
-	w.WriteMsg(m)
-	return dns.RcodeSuccess, nil
+	if re.Fallthrough {
+		return middleware.NextOrFailure(re.Name(), re.Next, ctx, w, r)
+	}
+	return dns.RcodeServerFailure, nil
 }
 
 // Name implements the Handler interface.
diff --git a/middleware/reverse/setup.go b/middleware/reverse/setup.go
index 0dc4534c4..e9f6c78b4 100644
--- a/middleware/reverse/setup.go
+++ b/middleware/reverse/setup.go
@@ -21,29 +21,29 @@ func init() {
 }
 
 func setupReverse(c *caddy.Controller) error {
-	networks, err := reverseParse(c)
+	networks, fallThrough, err := reverseParse(c)
 	if err != nil {
 		return middleware.Error("reverse", err)
 	}
 
 	dnsserver.GetConfig(c).AddMiddleware(func(next middleware.Handler) middleware.Handler {
-		return Reverse{Next: next, Networks: networks}
+		return Reverse{Next: next, Networks: networks, Fallthrough: fallThrough}
 	})
 
 	return nil
 }
 
-func reverseParse(c *caddy.Controller) (networks, error) {
-	var err error
+func reverseParse(c *caddy.Controller) (nets networks, fall bool, err error) {
 
 	// normalize zones, validation is almost done by dnsserver
+	// TODO(miek): need sane helpers for these.
 	zones := make([]string, len(c.ServerBlockKeys))
+
 	for i, str := range c.ServerBlockKeys {
 		host, _, _ := net.SplitHostPort(str)
 		zones[i] = strings.ToLower(host)
 	}
 
-	networks := networks{}
 	for c.Next() {
 		if c.Val() == "reverse" {
 
@@ -56,42 +56,41 @@ func reverseParse(c *caddy.Controller) (networks, error) {
 				}
 				_, ipnet, err := net.ParseCIDR(cidr)
 				if err != nil {
-					return nil, c.Errf("network needs to be CIDR formatted: %q\n", cidr)
+					return nil, false, c.Errf("network needs to be CIDR formatted: %q\n", cidr)
 				}
 				cidrs = append(cidrs, ipnet)
 			}
 			if len(cidrs) == 0 {
-				return nil, c.ArgErr()
+				return nil, false, c.ArgErr()
 			}
 
 			// set defaults
 			var (
 				template = "ip-" + templateNameIP + ".{zone[1]}"
 				ttl      = 60
-				fall     = false
 			)
 			for c.NextBlock() {
 				switch c.Val() {
 				case "hostname":
 					if !c.NextArg() {
-						return nil, c.ArgErr()
+						return nil, false, c.ArgErr()
 					}
 					template = c.Val()
 
 				case "ttl":
 					if !c.NextArg() {
-						return nil, c.ArgErr()
+						return nil, false, c.ArgErr()
 					}
 					ttl, err = strconv.Atoi(c.Val())
 					if err != nil {
-						return nil, err
+						return nil, false, err
 					}
 
 				case "fallthrough":
 					fall = true
 
 				default:
-					return nil, c.ArgErr()
+					return nil, false, c.ArgErr()
 				}
 			}
 
@@ -109,7 +108,7 @@ func reverseParse(c *caddy.Controller) (networks, error) {
 			// extract zone from template
 			templateZone := strings.SplitAfterN(template, ".", 2)
 			if len(templateZone) != 2 || templateZone[1] == "" {
-				return nil, c.Errf("cannot find domain in template '%v'", template)
+				return nil, false, c.Errf("cannot find domain in template '%v'", template)
 			}
 
 			// Create for each configured network in this stanza
@@ -126,22 +125,21 @@ func reverseParse(c *caddy.Controller) (networks, error) {
 						regexIP,
 						1) + "$")
 				if err != nil {
-					return nil, err
+					return nil, false, err
 				}
 
-				networks = append(networks, network{
+				nets = append(nets, network{
 					IPnet:        ipnet,
 					Zone:         templateZone[1],
 					Template:     template,
 					RegexMatchIP: regex,
 					TTL:          uint32(ttl),
-					Fallthrough:  fall,
 				})
 			}
 		}
 	}
 
 	// sort by cidr
-	sort.Sort(networks)
-	return networks, nil
+	sort.Sort(nets)
+	return nets, fall, nil
 }
diff --git a/middleware/reverse/setup_test.go b/middleware/reverse/setup_test.go
index 0a67d5cca..88dbccf1c 100644
--- a/middleware/reverse/setup_test.go
+++ b/middleware/reverse/setup_test.go
@@ -36,7 +36,6 @@ func TestSetupParse(t *testing.T) {
 				Zone:         "domain.com.",
 				TTL:          60,
 				RegexMatchIP: regexIP6,
-				Fallthrough:  false,
 			}},
 		},
 		{
@@ -112,14 +111,12 @@ func TestSetupParse(t *testing.T) {
 				Zone:         "dynamic.domain.com.",
 				TTL:          50,
 				RegexMatchIP: regexIpv6dynamic,
-				Fallthrough:  false,
 			}, network{
 				IPnet:        net4,
 				Template:     "dynamic-{ip}-vpn.dynamic.domain.com.",
 				Zone:         "dynamic.domain.com.",
 				TTL:          60,
 				RegexMatchIP: regexIpv4vpndynamic,
-				Fallthrough:  true,
 			}},
 		},
 		{
@@ -136,14 +133,12 @@ func TestSetupParse(t *testing.T) {
 				Zone:         "dynamic.domain.com.",
 				TTL:          50,
 				RegexMatchIP: regexIpv6dynamic,
-				Fallthrough:  true,
 			}, network{
 				IPnet:        net4,
 				Template:     "dynamic-{ip}-intern.dynamic.domain.com.",
 				Zone:         "dynamic.domain.com.",
 				TTL:          50,
 				RegexMatchIP: regexIpv4dynamic,
-				Fallthrough:  true,
 			}},
 		},
 		{
@@ -160,25 +155,23 @@ func TestSetupParse(t *testing.T) {
 				Zone:         "dynamic.domain.com.",
 				TTL:          300,
 				RegexMatchIP: regexIpv6dynamic,
-				Fallthrough:  true,
 			}},
 		},
 	}
 	for i, test := range tests {
 		c := caddy.NewTestController("dns", test.inputFileRules)
 		c.ServerBlockKeys = serverBlockKeys
-		networks, err := reverseParse(c)
+		networks, _, err := reverseParse(c)
 
 		if err == nil && test.shouldErr {
 			t.Fatalf("Test %d expected errors, but got no error", i)
 		} else if err != nil && !test.shouldErr {
 			t.Fatalf("Test %d expected no errors, but got '%v'", i, err)
-		} else {
-			for j, n := range networks {
-				reflect.DeepEqual(test.networks[j], n)
-				if !reflect.DeepEqual(test.networks[j], n) {
-					t.Fatalf("Test %d/%d expected %v, got %v", i, j, test.networks[j], n)
-				}
+		}
+		for j, n := range networks {
+			reflect.DeepEqual(test.networks[j], n)
+			if !reflect.DeepEqual(test.networks[j], n) {
+				t.Fatalf("Test %d/%d expected %v, got %v", i, j, test.networks[j], n)
 			}
 		}
 	}
diff --git a/test/reverse_test.go b/test/reverse_test.go
new file mode 100644
index 000000000..f1acb0a65
--- /dev/null
+++ b/test/reverse_test.go
@@ -0,0 +1,95 @@
+package test
+
+import (
+	"io/ioutil"
+	"log"
+	"testing"
+
+	"github.com/miekg/coredns/middleware/proxy"
+	"github.com/miekg/coredns/middleware/test"
+	"github.com/miekg/coredns/request"
+
+	"github.com/miekg/dns"
+)
+
+func TestReverseFallthrough(t *testing.T) {
+	t.Parallel()
+	name, rm, err := test.TempFile(".", exampleOrg)
+	if err != nil {
+		t.Fatalf("failed to created zone: %s", err)
+	}
+	defer rm()
+
+	corefile := `arpa:0 example.org:0 {
+    reverse 10.32.0.0/16 {
+        hostname ip-{ip}.{zone[2]}
+        #fallthrough
+    }
+    file ` + name + ` example.org
+}
+`
+
+	i, err := CoreDNSServer(corefile)
+	if err != nil {
+		t.Fatalf("Could not get CoreDNS serving instance: %s", err)
+	}
+
+	udp, _ := CoreDNSServerPorts(i, 0)
+	if udp == "" {
+		t.Fatalf("Could not get UDP listening port")
+	}
+
+	log.SetOutput(ioutil.Discard)
+
+	p := proxy.NewLookup([]string{udp})
+	state := request.Request{W: &test.ResponseWriter{}, Req: new(dns.Msg)}
+	resp, err := p.Lookup(state, "example.org.", dns.TypeA)
+	if err != nil {
+		t.Fatal("Expected to receive reply, but didn't")
+	}
+	// Reply should be SERVFAIL because of no fallthrough
+	if resp.Rcode != dns.RcodeServerFailure {
+		t.Fatalf("Expected SERVFAIL, but got: %d", resp.Rcode)
+	}
+
+	// Stop the server.
+	i.Stop()
+
+	// And redo with fallthrough enabled
+
+	corefile = `arpa:0 example.org:0 {
+    reverse 10.32.0.0/16 {
+        hostname ip-{ip}.{zone[2]}
+        fallthrough
+    }
+    file ` + name + ` example.org
+}
+`
+
+	i, err = CoreDNSServer(corefile)
+	if err != nil {
+		t.Fatalf("Could not get CoreDNS serving instance: %s", err)
+	}
+
+	udp, _ = CoreDNSServerPorts(i, 0)
+	if udp == "" {
+		t.Fatalf("Could not get UDP listening port")
+	}
+	defer i.Stop()
+
+	p = proxy.NewLookup([]string{udp})
+	resp, err = p.Lookup(state, "example.org.", dns.TypeA)
+	if err != nil {
+		t.Fatal("Expected to receive reply, but didn't")
+	}
+
+	if len(resp.Answer) == 0 {
+		t.Error("Expected to at least one RR in the answer section, got none")
+	}
+	if resp.Answer[0].Header().Rrtype != dns.TypeA {
+		t.Errorf("Expected RR to A, got: %d", resp.Answer[0].Header().Rrtype)
+	}
+	if resp.Answer[0].(*dns.A).A.String() != "127.0.0.1" {
+		t.Errorf("Expected 127.0.0.1, got: %s", resp.Answer[0].(*dns.A).A.String())
+	}
+}