middleware/reverse: random updates (#516)

* middleware/reverse: random updates

Make the documentation somewhat shorter (and hopefully clearer in the
process). Also to be on-par with the *auto* middleware, start counting
the referenced zones from 1 (instead of 0).
Some variable cleanups and use the NextOrFailure in the ServeDNS
function.

* More TODOs

5 files changed, 122 insertions, 153 deletions

diff --git a/middleware/reverse/README.md b/middleware/reverse/README.md
index 7437a3079..e9189e5fe 100644
--- a/middleware/reverse/README.md
+++ b/middleware/reverse/README.md
@@ -1,42 +1,43 @@
 # reverse
 
-The *reverse* middleware allows CoreDNS to respond dynamic to an PTR request and the related A/AAAA request.
+The *reverse* middleware allows CoreDNS to respond dynamicly to an PTR request and the related A/AAAA request.
 
 ## Syntax
 
 ~~~
-reverse NETWORK.. {
+reverse NETWORK... {
     hostname TEMPLATE
     [ttl TTL]
     [fallthrough]
 ~~~
 
 * **NETWORK** one or more CIDR formatted networks to respond on.
-* `hostname` inject the ip and zone to an template for the hostname. Defaults to "ip-{ip}.{zone[0]}". See below for template.
+* `hostname` inject the IP and zone to an template for the hostname. Defaults to "ip-{IP}.{zone[1]}". See below for template.
 * `ttl` defaults to 60
 * `fallthrough` If zone matches and no record can be generated, pass request to the next middleware.
 
 ### Template Syntax
-The template for the hostname is used for generating the PTR for an reverse lookup and matching the forward lookup back to an ip.
+
+The template for the hostname is used for generating the PTR for an reverse lookup and matching the
+forward lookup back to an IP.
 
 #### `{ip}`
-This symbol is **required** to work.
-V4 network replaces the "." with an "-". 10.1.1.1 results in "10-1-1-1"
-V6 network removes the ":" and fills the zeros. "ffff::ffff" results in "ffff000000000000000000000000ffff"
+
+The `{ip}` symbol is **required** to make reverse work.
+For IPv4 lookups the "." is replaced with an "-", i.e.: 10.1.1.1 results in "10-1-1-1"
+With IPv6 lookups the ":" is removed, and any zero ranged are expanded, i.e.:
+"ffff::ffff" results in "ffff000000000000000000000000ffff"
 
 #### `{zone[i]}`
-This symbol is **optional** to use and can be replaced by a fix zone string.
-The zone will be matched by the configured listener on the server block key.
-`i` needs to be replaced to the index of the configured listener zones, starting with 0.
 
-`arpa.:53 domain.com.:8053` will resolve `zone{0}` to `arpa.` and `zone{1}` to `domain.com.`
+The `{zone[i]}` symbol is **optional** and can be replaced by a fixed (zone) string.
+The zone will be matched by the zones listed in *this* configuration stanza.
+`i` needs to be replaced to the index of the configured listener zones, starting with 1.
 
 ## Examples
 
-~~~
-# Serve on port 53
-# match arpa. and compute.internal. to resolv reverse and forward lookup 
-.arpa.:53 compute.internal.:53 {
+~~~ txt
+arpa compute.internal {
     # proxy unmatched requests
     proxy . 8.8.8.8
 
@@ -48,32 +49,25 @@ The zone will be matched by the configured listener on the server block key.
     # AAAA ip-fd010000000000000000000000000001.compute.internal. 3600 fd01::1
     reverse 10.32.0.0/16 fd01::/16 {
         # template of the ip injection to hostname, zone resolved to compute.internal.
-        hostname ip-{ip}.{zone[1]}
+        hostname ip-{ip}.{zone[2]}
 
-        # set time-to-live of the RR
         ttl 3600
 
-        # forward unanswered or unmatched requests to proxy
-        # without this flag, requesting A/AAAA records on compute.internal. will end here
+        # Forward unanswered or unmatched requests to proxy # without this flag, requesting A/AAAA
+        records on compute.internal. will end here.
         fallthrough
     }
-
-    # cache with ttl timeout
-    cache
 }
 ~~~
 
 
-~~~
-# Serve on port 53
-# listen only on the specific network
-32.10.in-addr.arpa.arpa.:53 arpa.company.org.:53 {
+~~~ txt
+32.10.in-addr.arpa.arpa arpa.company.org {
 
     reverse 10.32.0.0/16 {
         # template of the ip injection to hostname, zone resolved to arpa.company.org.
-        hostname "ip-{ip}.v4.{zone[1]}"
+        hostname "ip-{ip}.v4.{zone[2]}"
 
-        # set time-to-live of the RR
         ttl 3600
 
         # fallthrough is not required, v4.arpa.company.org. will be only answered here
@@ -84,14 +78,7 @@ The zone will be matched by the configured listener on the server block key.
         # its also possible to set fix domain suffix
         hostname ip-{ip}.fix.arpa.company.org.
 
-        # set time-to-live of the RR
         ttl 3600
     }
-
-    # cache with ttl timeout
-    cache
 }
 ~~~
-
-
-
diff --git a/middleware/reverse/network.go b/middleware/reverse/network.go
index 7f408326b..f5101e46d 100644
--- a/middleware/reverse/network.go
+++ b/middleware/reverse/network.go
@@ -1,47 +1,46 @@
 package reverse
 
 import (
+	"bytes"
 	"net"
 	"regexp"
-	"bytes"
 	"strings"
 )
 
 type network struct {
 	IPnet        *net.IPNet
-	Zone 	     string // forward lookup zone
+	Zone         string // forward lookup zone
 	Template     string
 	TTL          uint32
 	RegexMatchIP *regexp.Regexp
 	Fallthrough  bool
 }
 
+// TODO: we might want to get rid of these regexes.
 const hexDigit = "0123456789abcdef"
 const templateNameIP = "{ip}"
 const regexMatchV4 = "((?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\-){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))"
 const regexMatchV6 = "([0-9a-fA-F]{32})"
 
-// For forward lookup
-// converts the hostname back to an ip, based on the template
-// returns nil if there is no ip found
+// hostnameToIP converts the hostname back to an ip, based on the template
+// returns nil if there is no IP found.
 func (network *network) hostnameToIP(rname string) net.IP {
 	var matchedIP net.IP
 
-	// use precompiled regex by setup
 	match := network.RegexMatchIP.FindStringSubmatch(rname)
-	// regex did not matched
-	if (len(match) != 2) {
+	if len(match) != 2 {
 		return nil
 	}
 
 	if network.IPnet.IP.To4() != nil {
 		matchedIP = net.ParseIP(strings.Replace(match[1], "-", ".", 4))
 	} else {
+		// TODO: can probably just allocate a []byte and use that.
 		var buf bytes.Buffer
 		// convert back to an valid ipv6 string with colons
-		for i := 0; i < 8 * 4; i += 4 {
-			buf.WriteString(match[1][i:i + 4])
-			if (i < 28) {
+		for i := 0; i < 8*4; i += 4 {
+			buf.WriteString(match[1][i : i+4])
+			if i < 28 {
 				buf.WriteString(":")
 			}
 		}
@@ -56,13 +55,9 @@ func (network *network) hostnameToIP(rname string) net.IP {
 	return matchedIP
 }
 
-// For reverse lookup
-// Converts an Ip to an dns compatible hostname and injects it into the template.domain
-func (network *network) ipToHostname(ip net.IP) string {
-	var name string
-
-	ipv4 := ip.To4()
-	if ipv4 != nil {
+// ipToHostname converts an IP to an DNS compatible hostname and injects it into the template.domain.
+func (network *network) ipToHostname(ip net.IP) (name string) {
+	if ipv4 := ip.To4(); ipv4 != nil {
 		// replace . to -
 		name = uitoa(ipv4[0]) + "-" +
 			uitoa(ipv4[1]) + "-" +
@@ -71,11 +66,11 @@ func (network *network) ipToHostname(ip net.IP) string {
 	} else {
 		// assume v6
 		// ensure zeros are present in string
-		buf := make([]byte, 0, len(ip) * 4)
+		buf := make([]byte, 0, len(ip)*4)
 		for i := 0; i < len(ip); i++ {
 			v := ip[i]
-			buf = append(buf, hexDigit[v >> 4])
-			buf = append(buf, hexDigit[v & 0xF])
+			buf = append(buf, hexDigit[v>>4])
+			buf = append(buf, hexDigit[v&0xF])
 		}
 		name = string(buf)
 	}
@@ -93,7 +88,7 @@ func uitoa(val uint8) string {
 	i := len(buf) - 1
 	for val >= 10 {
 		q := val / 10
-		buf[i] = byte('0' + val - q * 10)
+		buf[i] = byte('0' + val - q*10)
 		i--
 		val = q
 	}
@@ -104,20 +99,12 @@ func uitoa(val uint8) string {
 
 type networks []network
 
-// implements the sort interface
-func (slice networks) Len() int {
-	return len(slice)
-}
+func (n networks) Len() int      { return len(n) }
+func (n networks) Swap(i, j int) { n[i], n[j] = n[j], n[i] }
 
-// implements the sort interface
 // cidr closer to the ip wins (by netmask)
-func (slice networks) Less(i, j int) bool {
-	isize, _ := slice[i].IPnet.Mask.Size()
-	jsize, _ := slice[j].IPnet.Mask.Size()
+func (n networks) Less(i, j int) bool {
+	isize, _ := n[i].IPnet.Mask.Size()
+	jsize, _ := n[j].IPnet.Mask.Size()
 	return isize > jsize
 }
-
-// implements the sort interface
-func (slice networks) Swap(i, j int) {
-	slice[i], slice[j] = slice[j], slice[i]
-}
\ No newline at end of file
diff --git a/middleware/reverse/reverse.go b/middleware/reverse/reverse.go
index 9cf7d751a..3e019affd 100644
--- a/middleware/reverse/reverse.go
+++ b/middleware/reverse/reverse.go
@@ -3,31 +3,33 @@ package reverse
 import (
 	"net"
 
-	"github.com/miekg/coredns/request"
 	"github.com/miekg/coredns/middleware"
 	"github.com/miekg/coredns/middleware/pkg/dnsutil"
+	"github.com/miekg/coredns/request"
 
 	"github.com/miekg/dns"
 	"golang.org/x/net/context"
 )
 
-// Reverse provides dynamic reverse dns and the related forward rr
+// Reverse provides dynamic reverse DNS and the related forward RR.
 type Reverse struct {
 	Next     middleware.Handler
 	Networks networks
 }
 
 // ServeDNS implements the middleware.Handler interface.
-func (reverse Reverse) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
-	var rr dns.RR
-	nextHandler := true
+func (re Reverse) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
+	var (
+		rr          dns.RR
+		fallThrough bool
+	)
 
 	state := request.Request{W: w, Req: r}
 	m := new(dns.Msg)
 	m.SetReply(r)
 	m.Authoritative, m.RecursionAvailable, m.Compress = true, true, true
 
-	switch state.QType(){
+	switch state.QType() {
 	case dns.TypePTR:
 		address := dnsutil.ExtractAddressFromReverse(state.Name())
 
@@ -38,11 +40,11 @@ func (reverse Reverse) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dn
 
 		ip := net.ParseIP(address)
 		// loop through the configured networks
-		for _, n := range reverse.Networks {
-			if (n.IPnet.Contains(ip)) {
-				nextHandler = n.Fallthrough
+		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},
+					Hdr: dns.RR_Header{Name: state.QName(), Rrtype: dns.TypePTR, Class: dns.ClassINET, Ttl: n.TTL},
 					Ptr: n.ipToHostname(ip),
 				}
 				break
@@ -50,9 +52,9 @@ func (reverse Reverse) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dn
 		}
 
 	case dns.TypeA:
-		for _, n := range reverse.Networks {
+		for _, n := range re.Networks {
 			if dns.IsSubDomain(n.Zone, state.Name()) {
-				nextHandler = n.Fallthrough
+				fallThrough = n.Fallthrough
 
 				// skip if requesting an v4 address and network is not v4
 				if n.IPnet.IP.To4() == nil {
@@ -62,8 +64,8 @@ func (reverse Reverse) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dn
 				result := n.hostnameToIP(state.Name())
 				if result != nil {
 					rr = &dns.A{
-						Hdr:  dns.RR_Header{Name: state.QName(), Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: n.TTL},
-						A: result,
+						Hdr: dns.RR_Header{Name: state.QName(), Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: n.TTL},
+						A:   result,
 					}
 					break
 				}
@@ -71,9 +73,9 @@ func (reverse Reverse) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dn
 		}
 
 	case dns.TypeAAAA:
-		for _, n := range reverse.Networks {
+		for _, n := range re.Networks {
 			if dns.IsSubDomain(n.Zone, state.Name()) {
-				nextHandler = n.Fallthrough
+				fallThrough = n.Fallthrough
 
 				// Do not use To16 which tries to make v4 in v6
 				if n.IPnet.IP.To4() != nil {
@@ -93,13 +95,8 @@ func (reverse Reverse) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dn
 
 	}
 
-	if rr == nil {
-		if reverse.Next == nil || !nextHandler {
-			// could not resolv
-			w.WriteMsg(m)
-			return dns.RcodeNameError, nil
-		}
-		return reverse.Next.ServeDNS(ctx, w, r)
+	if rr == nil && !fallThrough {
+		return middleware.NextOrFailure(re.Name(), re.Next, ctx, w, r)
 	}
 
 	m.Answer = append(m.Answer, rr)
@@ -109,6 +106,4 @@ func (reverse Reverse) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dn
 }
 
 // Name implements the Handler interface.
-func (reverse Reverse) Name() string {
-	return "reverse"
-}
+func (re Reverse) Name() string { return "reverse" }
diff --git a/middleware/reverse/setup.go b/middleware/reverse/setup.go
index 024f7a39e..0dc4534c4 100644
--- a/middleware/reverse/setup.go
+++ b/middleware/reverse/setup.go
@@ -2,10 +2,10 @@ package reverse
 
 import (
 	"net"
+	"regexp"
 	"sort"
-	"strings"
 	"strconv"
-	"regexp"
+	"strings"
 
 	"github.com/miekg/coredns/core/dnsserver"
 	"github.com/miekg/coredns/middleware"
@@ -27,7 +27,7 @@ func setupReverse(c *caddy.Controller) error {
 	}
 
 	dnsserver.GetConfig(c).AddMiddleware(func(next middleware.Handler) middleware.Handler {
-		return Reverse{Next: next, Networks:networks}
+		return Reverse{Next: next, Networks: networks}
 	})
 
 	return nil
@@ -50,13 +50,13 @@ func reverseParse(c *caddy.Controller) (networks, error) {
 			var cidrs []*net.IPNet
 
 			// parse all networks
-			for _, cidr :=  range c.RemainingArgs() {
+			for _, cidr := range c.RemainingArgs() {
 				if cidr == "{" {
 					break
 				}
 				_, ipnet, err := net.ParseCIDR(cidr)
 				if err != nil {
-					return nil, c.Errf("%v needs to be an CIDR formatted Network\n", cidr)
+					return nil, c.Errf("network needs to be CIDR formatted: %q\n", cidr)
 				}
 				cidrs = append(cidrs, ipnet)
 			}
@@ -66,9 +66,9 @@ func reverseParse(c *caddy.Controller) (networks, error) {
 
 			// set defaults
 			var (
-				template = "ip-" + templateNameIP + ".{zone[0]}"
-				ttl = 60
-				fall = false
+				template = "ip-" + templateNameIP + ".{zone[1]}"
+				ttl      = 60
+				fall     = false
 			)
 			for c.NextBlock() {
 				switch c.Val() {
@@ -98,7 +98,9 @@ func reverseParse(c *caddy.Controller) (networks, error) {
 			// prepare template
 			// replace {zone[index]} by the listen zone/domain of this config block
 			for i, zone := range zones {
-				template = strings.Replace(template, "{zone[" + string(i + 48) + "]}", zone, 1)
+				// TODO: we should be smarter about actually replacing this. This works, but silently allows "zone[-1]"
+				// for instance.
+				template = strings.Replace(template, "{zone["+strconv.Itoa(i+1)+"]}", zone, 1)
 			}
 			if !strings.HasSuffix(template, ".") {
 				template += "."
@@ -107,7 +109,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, c.Errf("cannot find domain in template '%v'", template)
 			}
 
 			// Create for each configured network in this stanza
@@ -122,19 +124,18 @@ func reverseParse(c *caddy.Controller) (networks, error) {
 						regexp.QuoteMeta(template), // escape dots
 						regexp.QuoteMeta(templateNameIP),
 						regexIP,
-						1, ) + "$")
+						1) + "$")
 				if err != nil {
-					// invalid regex
 					return nil, err
 				}
 
 				networks = append(networks, network{
-					IPnet: ipnet,
-					Zone: templateZone[1],
-					Template: template,
+					IPnet:        ipnet,
+					Zone:         templateZone[1],
+					Template:     template,
 					RegexMatchIP: regex,
-					TTL: uint32(ttl),
-					Fallthrough: fall,
+					TTL:          uint32(ttl),
+					Fallthrough:  fall,
 				})
 			}
 		}
diff --git a/middleware/reverse/setup_test.go b/middleware/reverse/setup_test.go
index 4ec9e8a55..0a67d5cca 100644
--- a/middleware/reverse/setup_test.go
+++ b/middleware/reverse/setup_test.go
@@ -1,10 +1,10 @@
 package reverse
 
 import (
-	"testing"
 	"net"
 	"reflect"
 	"regexp"
+	"testing"
 
 	"github.com/mholt/caddy"
 )
@@ -19,7 +19,7 @@ func TestSetupParse(t *testing.T) {
 	regexIpv6dynamic, _ := regexp.Compile("^dynamic-" + regexMatchV6 + "-intern\\.dynamic\\.domain\\.com\\.$")
 	regexIpv4vpndynamic, _ := regexp.Compile("^dynamic-" + regexMatchV4 + "-vpn\\.dynamic\\.domain\\.com\\.$")
 
-	serverBlockKeys := []string{"domain.com.:8053", "dynamic.domain.com.:8053" }
+	serverBlockKeys := []string{"domain.com.:8053", "dynamic.domain.com.:8053"}
 
 	tests := []struct {
 		inputFileRules string
@@ -31,12 +31,12 @@ func TestSetupParse(t *testing.T) {
 			`reverse fd01::/64`,
 			false,
 			networks{network{
-				IPnet: net6,
-				Template: "ip-{ip}.domain.com.",
-				Zone: "domain.com.",
-				TTL: 60,
+				IPnet:        net6,
+				Template:     "ip-{ip}.domain.com.",
+				Zone:         "domain.com.",
+				TTL:          60,
 				RegexMatchIP: regexIP6,
-				Fallthrough: false,
+				Fallthrough:  false,
 			}},
 		},
 		{
@@ -98,52 +98,52 @@ func TestSetupParse(t *testing.T) {
 		},
 		{
 			`reverse fd01::/64 {
-				hostname dynamic-{ip}-intern.{zone[1]}
+				hostname dynamic-{ip}-intern.{zone[2]}
 				ttl 50
 			}
 			reverse 10.1.1.0/24 {
-				hostname dynamic-{ip}-vpn.{zone[1]}
+				hostname dynamic-{ip}-vpn.{zone[2]}
 				fallthrough
 			}`,
 			false,
 			networks{network{
-				IPnet: net6,
-				Template: "dynamic-{ip}-intern.dynamic.domain.com.",
-				Zone: "dynamic.domain.com.",
-				TTL: 50,
-				RegexMatchIP:regexIpv6dynamic,
-				Fallthrough: false,
+				IPnet:        net6,
+				Template:     "dynamic-{ip}-intern.dynamic.domain.com.",
+				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,
+				IPnet:        net4,
+				Template:     "dynamic-{ip}-vpn.dynamic.domain.com.",
+				Zone:         "dynamic.domain.com.",
+				TTL:          60,
 				RegexMatchIP: regexIpv4vpndynamic,
-				Fallthrough:true,
+				Fallthrough:  true,
 			}},
 		},
 		{
 			// multiple networks in one stanza
 			`reverse fd01::/64 10.1.1.0/24 {
-				hostname dynamic-{ip}-intern.{zone[1]}
+				hostname dynamic-{ip}-intern.{zone[2]}
 				ttl 50
 				fallthrough
 			}`,
 			false,
 			networks{network{
-				IPnet: net6,
-				Template: "dynamic-{ip}-intern.dynamic.domain.com.",
-				Zone: "dynamic.domain.com.",
-				TTL: 50,
-				RegexMatchIP:regexIpv6dynamic,
-				Fallthrough: true,
+				IPnet:        net6,
+				Template:     "dynamic-{ip}-intern.dynamic.domain.com.",
+				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,
+				IPnet:        net4,
+				Template:     "dynamic-{ip}-intern.dynamic.domain.com.",
+				Zone:         "dynamic.domain.com.",
+				TTL:          50,
 				RegexMatchIP: regexIpv4dynamic,
-				Fallthrough: true,
+				Fallthrough:  true,
 			}},
 		},
 		{
@@ -155,15 +155,14 @@ func TestSetupParse(t *testing.T) {
 			}`,
 			false,
 			networks{network{
-				IPnet: net6,
-				Template: "dynamic-{ip}-intern.dynamic.domain.com.",
-				Zone: "dynamic.domain.com.",
-				TTL: 300,
-				RegexMatchIP:regexIpv6dynamic,
-				Fallthrough: true,
+				IPnet:        net6,
+				Template:     "dynamic-{ip}-intern.dynamic.domain.com.",
+				Zone:         "dynamic.domain.com.",
+				TTL:          300,
+				RegexMatchIP: regexIpv6dynamic,
+				Fallthrough:  true,
 			}},
 		},
-
 	}
 	for i, test := range tests {
 		c := caddy.NewTestController("dns", test.inputFileRules)
@@ -183,4 +182,4 @@ func TestSetupParse(t *testing.T) {
 			}
 		}
 	}
-}
\ No newline at end of file
+}