middleware/proxy: config syntax cleanups (#435)

* middleware/proxy: config syntax cleanups

Allow port numbers to be used in the transfer statements and clean
up the proxy stanza parsing. Also allow, when specifying an upstream,
/etc/resolv.conf (or any other file) to be used for getting the upstream
nameserver.

Add tests and fix the documentation to make clear what is allowed.

* Fix the other upstream parse as well

middleware/etcd/README.md

@@ -37,7 +37,8 @@ etcd [ZONES...] {
 * **ENDPOINT** the etcd endpoints. Defaults to "http://localhost:2397".
 * `upstream` upstream resolvers to be used resolve external names found in etcd (think CNAMEs)
   pointing to external names. If you want CoreDNS to act as a proxy for clients, you'll need to add
-  the proxy middleware.
+  the proxy middleware. **ADDRESS* can be an IP address, and IP:port or a string pointing to a file
+  that is structured as /etc/resolv.conf.
 * `tls` followed the cert, key and the CA's cert filenames.
 * `debug` allows for debug queries. Prefix the name with `o-o.debug.` to retrieve extra information in the
   additional section of the reply in the form of TXT records.
@@ -61,6 +62,21 @@ This is the default SkyDNS setup, with everying specified in full:
 }
 ~~~
 
+Or a setup where we use `/etc/resolv.conf` as the basis for the proxy and the upstream
+when resolving external pointing CNAMEs.
+
+~~~
+.:53 {
+    etcd skydns.local {
+        path /skydns
+        upstream /etc/resolv.conf
+    }
+    cache 160 skydns.local
+    proxy . /etc/resolv.conf
+}
+~~~
+
+
 ### Reverse zones
 
 Reverse zones are supported. You need to make CoreDNS aware of the fact that you are also

middleware/etcd/setup.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/miekg/coredns/core/dnsserver"
 	"github.com/miekg/coredns/middleware"
+	"github.com/miekg/coredns/middleware/pkg/dnsutil"
 	"github.com/miekg/coredns/middleware/pkg/singleflight"
 	"github.com/miekg/coredns/middleware/proxy"
 
@@ -93,13 +94,11 @@ func etcdParse(c *caddy.Controller) (*Etcd, bool, error) {
 					if len(args) == 0 {
 						return &Etcd{}, false, c.ArgErr()
 					}
-					for i := 0; i < len(args); i++ {
+					ups, err := dnsutil.ParseHostPortOrFile(args...)
-						h, p, e := net.SplitHostPort(args[i])
+					if err != nil {
-						if e != nil && p == "" {
+						return &Etcd{}, false, err
-							args[i] = h + ":53"
 					}
-					}
+					etc.Proxy = proxy.New(ups)
-					etc.Proxy = proxy.New(args)
 				case "tls": // cert key cacertfile
 					args := c.RemainingArgs()
 					if len(args) != 3 {
@@ -133,13 +132,11 @@ func etcdParse(c *caddy.Controller) (*Etcd, bool, error) {
 						if len(args) == 0 {
 							return &Etcd{}, false, c.ArgErr()
 						}
-						for i := 0; i < len(args); i++ {
+						ups, err := dnsutil.ParseHostPortOrFile(args...)
-							h, p, e := net.SplitHostPort(args[i])
+						if err != nil {
-							if e != nil && p == "" {
+							return &Etcd{}, false, c.ArgErr()
-								args[i] = h + ":53"
 						}
-						}
+						etc.Proxy = proxy.New(ups)
-						etc.Proxy = proxy.New(args)
 					case "tls": // cert key cacertfile
 						args := c.RemainingArgs()
 						if len(args) != 3 {

middleware/file/setup.go

@@ -2,12 +2,12 @@ package file
 
 import (
 	"fmt"
-	"net"
 	"os"
 	"path"
 
 	"github.com/miekg/coredns/core/dnsserver"
 	"github.com/miekg/coredns/middleware"
+	"github.com/miekg/coredns/middleware/pkg/dnsutil"
 
 	"github.com/mholt/caddy"
 )
@@ -125,24 +125,26 @@ func TransferParse(c *caddy.Controller, secondary bool) (tos, froms []string, er
 			tos = c.RemainingArgs()
 			for i := range tos {
 				if tos[i] != "*" {
-					if x := net.ParseIP(tos[i]); x == nil {
+					normalized, err := dnsutil.ParseHostPort(tos[i], "53")
-						return nil, nil, fmt.Errorf("must specify an IP address: `%s'", tos[i])
+					if err != nil {
+						return nil, nil, err
 					}
-					tos[i] = middleware.Addr(tos[i]).Normalize()
+					tos[i] = normalized
 				}
 			}
 		}
 		if value == "from" {
 			if !secondary {
-				return nil, nil, fmt.Errorf("can't use `transfer from` when not being a seconary")
+				return nil, nil, fmt.Errorf("can't use `transfer from` when not being a secondary")
 			}
 			froms = c.RemainingArgs()
 			for i := range froms {
 				if froms[i] != "*" {
-					if x := net.ParseIP(froms[i]); x == nil {
+					normalized, err := dnsutil.ParseHostPort(froms[i], "53")
-						return nil, nil, fmt.Errorf("must specify an IP address: `%s'", froms[i])
+					if err != nil {
+						return nil, nil, err
 					}
-					froms[i] = middleware.Addr(froms[i]).Normalize()
+					froms[i] = normalized
 				} else {
 					return nil, nil, fmt.Errorf("can't use '*' in transfer from")
 				}

middleware/pkg/dnsutil/host.go

@@ -0,0 +1,82 @@
+package dnsutil
+
+import (
+	"fmt"
+	"net"
+	"os"
+
+	"github.com/miekg/dns"
+)
+
+// PorseHostPortOrFile parses the strings in s, each string can either be a address,
+// address:port or a filename. The address part is checked and the filename case a
+// resolv.conf like file is parsed and the nameserver found are returned.
+func ParseHostPortOrFile(s ...string) ([]string, error) {
+	var servers []string
+	for _, host := range s {
+		addr, _, err := net.SplitHostPort(host)
+		if err != nil {
+			// Parse didn't work, it is not a addr:port combo
+			if net.ParseIP(host) == nil {
+				// Not an IP address.
+				ss, err := tryFile(host)
+				if err == nil {
+					servers = append(servers, ss...)
+					continue
+				}
+				return servers, fmt.Errorf("not an IP address or file: %q", host)
+			}
+			ss := net.JoinHostPort(host, "53")
+			servers = append(servers, ss)
+			continue
+		}
+
+		if net.ParseIP(addr) == nil {
+			// No an IP address.
+			ss, err := tryFile(host)
+			if err == nil {
+				servers = append(servers, ss...)
+				continue
+			}
+			return servers, fmt.Errorf("not an IP address or file: %q", host)
+		}
+		servers = append(servers, host)
+	}
+	return servers, nil
+}
+
+// Try to open this is a file first.
+func tryFile(s string) ([]string, error) {
+	c, err := dns.ClientConfigFromFile(s)
+	if err == os.ErrNotExist {
+		return nil, fmt.Errorf("failed to open file %q: %q", s, err)
+	} else if err != nil {
+		return nil, err
+	}
+
+	servers := []string{}
+	for _, s := range c.Servers {
+		servers = append(servers, net.JoinHostPort(s, c.Port))
+	}
+	return servers, nil
+}
+
+// ParseHostPort will check if the host part is a valid IP address, if the
+// IP address is valid, but no port is found, defaultPort is added.
+func ParseHostPort(s, defaultPort string) (string, error) {
+	addr, port, err := net.SplitHostPort(s)
+	if port == "" {
+		port = defaultPort
+	}
+	if err != nil {
+		if net.ParseIP(s) == nil {
+			return "", fmt.Errorf("must specify an IP address: `%s'", s)
+		}
+		return net.JoinHostPort(s, port), nil
+	}
+
+	if net.ParseIP(addr) == nil {
+		return "", fmt.Errorf("must specify an IP address: `%s'", addr)
+	}
+	return net.JoinHostPort(addr, port), nil
+}

middleware/pkg/dnsutil/host_test.go

@@ -0,0 +1,85 @@
+package dnsutil
+
+import (
+	"io/ioutil"
+	"os"
+	"testing"
+)
+
+func TestParseHostPortOrFile(t *testing.T) {
+	tests := []struct {
+		in        string
+		expected  string
+		shouldErr bool
+	}{
+		{
+			"8.8.8.8",
+			"8.8.8.8:53",
+			false,
+		},
+		{
+			"8.8.8.8:153",
+			"8.8.8.8:153",
+			false,
+		},
+		{
+			"/etc/resolv.conf:53",
+			"",
+			true,
+		},
+		{
+			"resolv.conf",
+			"127.0.0.1:53",
+			false,
+		},
+	}
+
+	err := ioutil.WriteFile("resolv.conf", []byte("nameserver 127.0.0.1\n"), 0600)
+	if err != nil {
+		t.Fatalf("Failed to write test resolv.conf")
+	}
+	defer os.Remove("resolv.conf")
+
+	for i, tc := range tests {
+		got, err := ParseHostPortOrFile(tc.in)
+		if err == nil && tc.shouldErr {
+			t.Errorf("Test %d, expected error, got nil", i)
+			continue
+		}
+		if err != nil && tc.shouldErr {
+			continue
+		}
+		if got[0] != tc.expected {
+			t.Errorf("Test %d, expected %q, got %q", i, tc.expected, got[0])
+		}
+	}
+}
+
+func TestParseHostPort(t *testing.T) {
+	tests := []struct {
+		in        string
+		expected  string
+		shouldErr bool
+	}{
+		{"8.8.8.8:53", "8.8.8.8:53", false},
+		{"a.a.a.a:153", "", true},
+		{"8.8.8.8", "8.8.8.8:53", false},
+		{"8.8.8.8:", "8.8.8.8:53", false},
+		{"8.8.8.8::53", "", true},
+		{"resolv.conf", "", true},
+	}
+
+	for i, tc := range tests {
+		got, err := ParseHostPort(tc.in, "53")
+		if err == nil && tc.shouldErr {
+			t.Errorf("Test %d, expected error, got nil", i)
+			continue
+		}
+		if err != nil && !tc.shouldErr {
+			t.Errorf("Test %d, expected no error, got %q", i, err)
+		}
+		if got != tc.expected {
+			t.Errorf("Test %d, expected %q, got %q", i, tc.expected, got)
+		}
+	}
+}

middleware/proxy/README.md

@@ -10,7 +10,7 @@
 In its most basic form, a simple reverse proxy uses this syntax:
 
 ~~~
-proxy FROM To
+proxy FROM TO
 ~~~
 
 * **FROM** is the base path to match for the request to be proxied
@@ -68,13 +68,13 @@ proxy example.org localhost:9005
 Load-balance all requests between three backends (using random policy):
 
 ~~~
-proxy . web1.local:53 web2.local:1053 web3.local
+proxy . dns1.local:53 dns2.local:1053 dns3.local
 ~~~
 
 Same as above, but round-robin style:
 
 ~~~
-proxy . web1.local:53 web2.local:1053 web3.local {
+proxy . dns1.local:53 dns2.local:1053 dns3.local {
 	policy round_robin
 }
 ~~~
@@ -82,7 +82,7 @@ proxy . web1.local:53 web2.local:1053 web3.local {
 With health checks and proxy headers to pass hostname, IP, and scheme upstream:
 
 ~~~
-proxy . web1.local:53 web2.local:53 web3.local:53 {
+proxy . dns1.local:53 dns2.local:53 dns3.local:53 {
 	policy round_robin
 	health_check /health:8080
 }

middleware/proxy/upstream.go

@@ -1,18 +1,17 @@
 package proxy
 
 import (
-	"fmt"
 	"io"
 	"io/ioutil"
 	"net"
 	"net/http"
-	"os"
 	"strconv"
 	"strings"
 	"sync/atomic"
 	"time"
 
 	"github.com/miekg/coredns/middleware"
+	"github.com/miekg/coredns/middleware/pkg/dnsutil"
 
 	"github.com/mholt/caddy/caddyfile"
 	"github.com/miekg/dns"
@@ -68,27 +67,10 @@ func NewStaticUpstreams(c *caddyfile.Dispenser) ([]Upstream, error) {
 		}
 
 		// process the host list, substituting in any nameservers in files
-		var toHosts []string
+		toHosts, err := dnsutil.ParseHostPortOrFile(to...)
-		for _, host := range to {
-			h, _, err := net.SplitHostPort(host)
 		if err != nil {
-				h = host
-			}
-			if x := net.ParseIP(h); x == nil {
-				// it's a file, parse as resolv.conf
-				c, err := dns.ClientConfigFromFile(host)
-				if err == os.ErrNotExist {
-					return upstreams, fmt.Errorf("not an IP address or file: `%s'", h)
-				} else if err != nil {
 			return upstreams, err
 		}
-				for _, s := range c.Servers {
-					toHosts = append(toHosts, net.JoinHostPort(s, c.Port))
-				}
-			} else {
-				toHosts = append(toHosts, host)
-			}
-		}
 
 		for c.NextBlock() {
 			if err := parseBlock(c, upstream); err != nil {
@@ -99,7 +81,7 @@ func NewStaticUpstreams(c *caddyfile.Dispenser) ([]Upstream, error) {
 		upstream.Hosts = make([]*UpstreamHost, len(toHosts))
 		for i, host := range toHosts {
 			uh := &UpstreamHost{
-				Name:        defaultHostPort(host),
+				Name:        host,
 				Conns:       0,
 				Fails:       0,
 				FailTimeout: upstream.FailTimeout,
@@ -297,11 +279,3 @@ func (u *staticUpstream) IsAllowedPath(name string) bool {
 	}
 	return true
 }
-
-func defaultHostPort(s string) string {
-	_, _, e := net.SplitHostPort(s)
-	if e == nil {
-		return s
-	}
-	return net.JoinHostPort(s, "53")
-}