Various cleanups and fixes (#88)

Add port number to health check. Add tests the rewrite

middleware.



Fixes #36

5 files changed, 153 insertions, 143 deletions

diff --git a/middleware/proxy/README.md b/middleware/proxy/README.md
index 4315fbf6c..c6d45b51a 100644
--- a/middleware/proxy/README.md
+++ b/middleware/proxy/README.md
@@ -22,9 +22,9 @@ proxy from to... {
 	policy random | least_conn | round_robin
 	fail_timeout duration
 	max_fails integer
-	health_check path [duration]
+	health_check path:port [duration]
 	except ignored_names...
-	preset
+    tcp
 }
 ~~~
 
@@ -33,8 +33,10 @@ proxy from to... {
 * `policy` is the load balancing policy to use; applies only with multiple backends. May be one of random, least_conn, or round_robin. Default is random.
 * `fail_timeout` specifies how long to consider a backend as down after it has failed. While it is down, requests will not be routed to that backend. A backend is "down" if Caddy fails to communicate with it. The default value is 10 seconds ("10s").
 * `max_fails` is the number of failures within fail_timeout that are needed before considering a backend to be down. If 0, the backend will never be marked as down. Default is 1.
-* `health_check` will check path on each backend. If a backend returns a status code of 200-399, then that backend is healthy. If it doesn't, the backend is marked as unhealthy for duration and no requests are routed to it. If this option is not provided then health checks are disabled. The default duration is 10 seconds ("10s").
+* `health_check` will check path (on port) on each backend. If a backend returns a status code of 200-399, then that backend is healthy. If it doesn't, the backend is marked as unhealthy for duration and no requests are routed to it. If this option is not provided then health checks are disabled. The default duration is 10 seconds ("10s").
 * `ignored_names...` is a space-separated list of paths to exclude from proxying. Requests that match any of these paths will be passed thru.
+* `tcp` use TCP for all upstream queries, otherwise it depends on the transport of the incoming
+  query. TODO(miek): implement.
 
 ## Policies
 
@@ -68,9 +70,9 @@ 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:80 web2.local:90 web3.local:100 {
+proxy . web1.local:53 web2.local:53 web3.local:53 {
 	policy round_robin
-	health_check /health
+	health_check /health:8080
 }
 ~~~
 
diff --git a/middleware/proxy/upstream.go b/middleware/proxy/upstream.go
index 3ebac2f06..52375323e 100644
--- a/middleware/proxy/upstream.go
+++ b/middleware/proxy/upstream.go
@@ -3,6 +3,7 @@ package proxy
 import (
 	"io"
 	"io/ioutil"
+	"net"
 	"net/http"
 	"strconv"
 	"strings"
@@ -28,6 +29,7 @@ type staticUpstream struct {
 	MaxFails    int32
 	HealthCheck struct {
 		Path     string
+		Port     string
 		Interval time.Duration
 	}
 	WithoutPathPrefix string
@@ -138,7 +140,11 @@ func parseBlock(c *parse.Dispenser, u *staticUpstream) error {
 		if !c.NextArg() {
 			return c.ArgErr()
 		}
-		u.HealthCheck.Path = c.Val()
+		var err error
+		u.HealthCheck.Path, u.HealthCheck.Port, err = net.SplitHostPort(c.Val())
+		if err != nil {
+			return err
+		}
 		u.HealthCheck.Interval = 30 * time.Second
 		if c.NextArg() {
 			dur, err := time.ParseDuration(c.Val())
@@ -175,7 +181,11 @@ func parseBlock(c *parse.Dispenser, u *staticUpstream) error {
 
 func (u *staticUpstream) healthCheck() {
 	for _, host := range u.Hosts {
-		hostURL := host.Name + u.HealthCheck.Path
+		port := ""
+		if u.HealthCheck.Port != "" {
+			port = ":" + u.HealthCheck.Port
+		}
+		hostURL := host.Name + port + u.HealthCheck.Path
 		if r, err := http.Get(hostURL); err == nil {
 			io.Copy(ioutil.Discard, r.Body)
 			r.Body.Close()
diff --git a/middleware/rewrite/README.md b/middleware/rewrite/README.md
index 8e5a460f5..e7e66e90f 100644
--- a/middleware/rewrite/README.md
+++ b/middleware/rewrite/README.md
@@ -22,8 +22,8 @@ rewritten. I.e. to rewrite CH queries to IN use `rewrite CH IN`.
 If it does not look like a type a name is assumed and the qname in the message is rewritten, this
 needs to be a full match of the name `rewrite miek.nl example.org`.
 
-Advanced users may open a block and make a complex rewrite rule:
-TODO(miek): this has not yet been implemented.
+If you specify multiple rules and an incoming query matches on multiple (simple) rules only one
+the first rewrite is applied.
 
 > Everything below this line has not been implemented, yet.
 
diff --git a/middleware/rewrite/rewrite.go b/middleware/rewrite/rewrite.go
index 591598dda..b7d775133 100644
--- a/middleware/rewrite/rewrite.go
+++ b/middleware/rewrite/rewrite.go
@@ -1,11 +1,11 @@
-// Package rewrite is middleware for rewriting requests internally to
-// something different.
+// Package rewrite is middleware for rewriting requests internally to something different.
 package rewrite
 
 import (
 	"strings"
 
 	"github.com/miekg/coredns/middleware"
+
 	"github.com/miekg/dns"
 	"golang.org/x/net/context"
 )
@@ -25,16 +25,20 @@ const (
 
 // Rewrite is middleware to rewrite requests internally before being handled.
 type Rewrite struct {
-	Next  middleware.Handler
-	Rules []Rule
+	Next     middleware.Handler
+	Rules    []Rule
+	noRevert bool
 }
 
 // ServeHTTP implements the middleware.Handler interface.
 func (rw Rewrite) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
-	wr := NewResponseReverter(w, r)
 	for _, rule := range rw.Rules {
 		switch result := rule.Rewrite(r); result {
 		case RewriteDone:
+			if rw.noRevert {
+				return rw.Next.ServeDNS(ctx, w, r)
+			}
+			wr := NewResponseReverter(w, r)
 			return rw.Next.ServeDNS(ctx, wr, r)
 		case RewriteIgnored:
 			break
@@ -67,8 +71,16 @@ type SimpleRule struct {
 func NewSimpleRule(from, to string) SimpleRule {
 	tpf := dns.StringToType[from]
 	tpt := dns.StringToType[to]
+
+	// ANY is both a type and class, ANY class rewritting is way more less frequent
+	// so we default to ANY as a type.
 	clf := dns.StringToClass[from]
 	clt := dns.StringToClass[to]
+	if from == "ANY" {
+		clf = 0
+		clt = 0
+	}
+
 	// It's only a type/class if uppercase is used.
 	if from != strings.ToUpper(from) {
 		tpf = 0
@@ -85,25 +97,20 @@ func NewSimpleRule(from, to string) SimpleRule {
 
 // Rewrite rewrites the the current request.
 func (s SimpleRule) Rewrite(r *dns.Msg) Result {
-	// type rewrite
 	if s.fromType > 0 && s.toType > 0 {
 		if r.Question[0].Qtype == s.fromType {
 			r.Question[0].Qtype = s.toType
 			return RewriteDone
 		}
-		return RewriteIgnored
 	}
 
-	// class rewrite
 	if s.fromClass > 0 && s.toClass > 0 {
 		if r.Question[0].Qclass == s.fromClass {
 			r.Question[0].Qclass = s.toClass
 			return RewriteDone
 		}
-		return RewriteIgnored
 	}
 
-	// name rewite
 	if s.From == r.Question[0].Name {
 		r.Question[0].Name = s.To
 		return RewriteDone
diff --git a/middleware/rewrite/rewrite_test.go b/middleware/rewrite/rewrite_test.go
index b6b01fc94..6dafdf3a6 100644
--- a/middleware/rewrite/rewrite_test.go
+++ b/middleware/rewrite/rewrite_test.go
@@ -1,151 +1,142 @@
 package rewrite
 
-/*
+import (
+	"testing"
+
+	"github.com/miekg/coredns/middleware"
+	coretest "github.com/miekg/coredns/middleware/testing"
+
+	"github.com/miekg/dns"
+	"golang.org/x/net/context"
+)
+
+func msgPrinter(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
+	w.WriteMsg(r)
+	return 0, nil
+}
+
 func TestRewrite(t *testing.T) {
 	rw := Rewrite{
-		Next: middleware.HandlerFunc(urlPrinter),
+		Next: middleware.HandlerFunc(msgPrinter),
 		Rules: []Rule{
-			NewSimpleRule("/from", "/to"),
-			NewSimpleRule("/a", "/b"),
-			NewSimpleRule("/b", "/b{uri}"),
+			NewSimpleRule("from.nl.", "to.nl."),
+			NewSimpleRule("CH", "IN"),
+			NewSimpleRule("ANY", "HINFO"),
 		},
-		FileSys: http.Dir("."),
-	}
-
-	regexps := [][]string{
-		{"/reg/", ".*", "/to", ""},
-		{"/r/", "[a-z]+", "/toaz", "!.html|"},
-		{"/url/", "a([a-z0-9]*)s([A-Z]{2})", "/to/{path}", ""},
-		{"/ab/", "ab", "/ab?{query}", ".txt|"},
-		{"/ab/", "ab", "/ab?type=html&{query}", ".html|"},
-		{"/abc/", "ab", "/abc/{file}", ".html|"},
-		{"/abcd/", "ab", "/a/{dir}/{file}", ".html|"},
-		{"/abcde/", "ab", "/a#{fragment}", ".html|"},
-		{"/ab/", `.*\.jpg`, "/ajpg", ""},
-		{"/reggrp", `/ad/([0-9]+)([a-z]*)`, "/a{1}/{2}", ""},
-		{"/reg2grp", `(.*)`, "/{1}", ""},
-		{"/reg3grp", `(.*)/(.*)/(.*)`, "/{1}{2}{3}", ""},
-	}
-
-	for _, regexpRule := range regexps {
-		var ext []string
-		if s := strings.Split(regexpRule[3], "|"); len(s) > 1 {
-			ext = s[:len(s)-1]
-		}
-		rule, err := NewComplexRule(regexpRule[0], regexpRule[1], regexpRule[2], 0, ext, nil)
-		if err != nil {
-			t.Fatal(err)
-		}
-		rw.Rules = append(rw.Rules, rule)
+		noRevert: true,
 	}
 
 	tests := []struct {
-		from       string
-		expectedTo string
+		from  string
+		fromT uint16
+		fromC uint16
+		to    string
+		toT   uint16
+		toC   uint16
 	}{
-		{"/from", "/to"},
-		{"/a", "/b"},
-		{"/b", "/b/b"},
-		{"/aa", "/aa"},
-		{"/", "/"},
-		{"/a?foo=bar", "/b?foo=bar"},
-		{"/asdf?foo=bar", "/asdf?foo=bar"},
-		{"/foo#bar", "/foo#bar"},
-		{"/a#foo", "/b#foo"},
-		{"/reg/foo", "/to"},
-		{"/re", "/re"},
-		{"/r/", "/r/"},
-		{"/r/123", "/r/123"},
-		{"/r/a123", "/toaz"},
-		{"/r/abcz", "/toaz"},
-		{"/r/z", "/toaz"},
-		{"/r/z.html", "/r/z.html"},
-		{"/r/z.js", "/toaz"},
-		{"/url/asAB", "/to/url/asAB"},
-		{"/url/aBsAB", "/url/aBsAB"},
-		{"/url/a00sAB", "/to/url/a00sAB"},
-		{"/url/a0z0sAB", "/to/url/a0z0sAB"},
-		{"/ab/aa", "/ab/aa"},
-		{"/ab/ab", "/ab/ab"},
-		{"/ab/ab.txt", "/ab"},
-		{"/ab/ab.txt?name=name", "/ab?name=name"},
-		{"/ab/ab.html?name=name", "/ab?type=html&name=name"},
-		{"/abc/ab.html", "/abc/ab.html"},
-		{"/abcd/abcd.html", "/a/abcd/abcd.html"},
-		{"/abcde/abcde.html", "/a"},
-		{"/abcde/abcde.html#1234", "/a#1234"},
-		{"/ab/ab.jpg", "/ajpg"},
-		{"/reggrp/ad/12", "/a12"},
-		{"/reggrp/ad/124a", "/a124/a"},
-		{"/reggrp/ad/124abc", "/a124/abc"},
-		{"/reg2grp/ad/124abc", "/ad/124abc"},
-		{"/reg3grp/ad/aa/66", "/adaa66"},
-		{"/reg3grp/ad612/n1n/ab", "/ad612n1nab"},
+		{"from.nl.", dns.TypeA, dns.ClassINET, "to.nl.", dns.TypeA, dns.ClassINET},
+		{"a.nl.", dns.TypeA, dns.ClassINET, "a.nl.", dns.TypeA, dns.ClassINET},
+		{"a.nl.", dns.TypeA, dns.ClassCHAOS, "a.nl.", dns.TypeA, dns.ClassINET},
+		{"a.nl.", dns.TypeANY, dns.ClassINET, "a.nl.", dns.TypeHINFO, dns.ClassINET},
+		// name is rewritten, type is not.
+		{"from.nl.", dns.TypeANY, dns.ClassINET, "to.nl.", dns.TypeANY, dns.ClassINET},
+		// name is not, type is, but class is, because class is the 2nd rule.
+		{"a.nl.", dns.TypeANY, dns.ClassCHAOS, "a.nl.", dns.TypeANY, dns.ClassINET},
 	}
 
+	ctx := context.TODO()
 	for i, test := range tests {
-		req, err := http.NewRequest("GET", test.from, nil)
-		if err != nil {
-			t.Fatalf("Test %d: Could not create HTTP request: %v", i, err)
-		}
+		m := new(dns.Msg)
+		m.SetQuestion(test.from, test.fromT)
+		m.Question[0].Qclass = test.fromC
 
-		rec := httptest.NewRecorder()
-		rw.ServeHTTP(rec, req)
+		rec := middleware.NewResponseRecorder(&coretest.ResponseWriter{})
+		rw.ServeDNS(ctx, rec, m)
+		resp := rec.Msg()
 
-		if rec.Body.String() != test.expectedTo {
-			t.Errorf("Test %d: Expected URL to be '%s' but was '%s'",
-				i, test.expectedTo, rec.Body.String())
+		if resp.Question[0].Name != test.to {
+			t.Errorf("Test %d: Expected Name to be '%s' but was '%s'", i, test.to, resp.Question[0].Name)
+		}
+		if resp.Question[0].Qtype != test.toT {
+			t.Errorf("Test %d: Expected Type to be '%d' but was '%d'", i, test.toT, resp.Question[0].Qtype)
+		}
+		if resp.Question[0].Qclass != test.toC {
+			t.Errorf("Test %d: Expected Class to be '%d' but was '%d'", i, test.toC, resp.Question[0].Qclass)
 		}
 	}
 
-	statusTests := []struct {
-		status         int
-		base           string
-		to             string
-		regexp         string
-		statusExpected bool
-	}{
-		{400, "/status", "", "", true},
-		{400, "/ignore", "", "", false},
-		{400, "/", "", "^/ignore", false},
-		{400, "/", "", "(.*)", true},
-		{400, "/status", "", "", true},
-	}
+	/*
+		regexps := [][]string{
+			{"/reg/", ".*", "/to", ""},
+			{"/r/", "[a-z]+", "/toaz", "!.html|"},
+			{"/url/", "a([a-z0-9]*)s([A-Z]{2})", "/to/{path}", ""},
+			{"/ab/", "ab", "/ab?{query}", ".txt|"},
+			{"/ab/", "ab", "/ab?type=html&{query}", ".html|"},
+			{"/abc/", "ab", "/abc/{file}", ".html|"},
+			{"/abcd/", "ab", "/a/{dir}/{file}", ".html|"},
+			{"/abcde/", "ab", "/a#{fragment}", ".html|"},
+			{"/ab/", `.*\.jpg`, "/ajpg", ""},
+			{"/reggrp", `/ad/([0-9]+)([a-z]*)`, "/a{1}/{2}", ""},
+			{"/reg2grp", `(.*)`, "/{1}", ""},
+			{"/reg3grp", `(.*)/(.*)/(.*)`, "/{1}{2}{3}", ""},
+		}
 
-	for i, s := range statusTests {
-		urlPath := fmt.Sprintf("/status%d", i)
-		rule, err := NewComplexRule(s.base, s.regexp, s.to, s.status, nil, nil)
-		if err != nil {
-			t.Fatalf("Test %d: No error expected for rule but found %v", i, err)
+		for _, regexpRule := range regexps {
+			var ext []string
+			if s := strings.Split(regexpRule[3], "|"); len(s) > 1 {
+				ext = s[:len(s)-1]
+			}
+			rule, err := NewComplexRule(regexpRule[0], regexpRule[1], regexpRule[2], 0, ext, nil)
+			if err != nil {
+				t.Fatal(err)
+			}
+			rw.Rules = append(rw.Rules, rule)
 		}
-		rw.Rules = []Rule{rule}
-		req, err := http.NewRequest("GET", urlPath, nil)
-		if err != nil {
-			t.Fatalf("Test %d: Could not create HTTP request: %v", i, err)
+	*/
+	/*
+		statusTests := []struct {
+			status         int
+			base           string
+			to             string
+			regexp         string
+			statusExpected bool
+		}{
+			{400, "/status", "", "", true},
+			{400, "/ignore", "", "", false},
+			{400, "/", "", "^/ignore", false},
+			{400, "/", "", "(.*)", true},
+			{400, "/status", "", "", true},
 		}
 
-		rec := httptest.NewRecorder()
-		code, err := rw.ServeHTTP(rec, req)
-		if err != nil {
-			t.Fatalf("Test %d: No error expected for handler but found %v", i, err)
-		}
-		if s.statusExpected {
-			if rec.Body.String() != "" {
-				t.Errorf("Test %d: Expected empty body but found %s", i, rec.Body.String())
+		for i, s := range statusTests {
+			urlPath := fmt.Sprintf("/status%d", i)
+			rule, err := NewComplexRule(s.base, s.regexp, s.to, s.status, nil, nil)
+			if err != nil {
+				t.Fatalf("Test %d: No error expected for rule but found %v", i, err)
 			}
-			if code != s.status {
-				t.Errorf("Test %d: Expected status code %d found %d", i, s.status, code)
+			rw.Rules = []Rule{rule}
+			req, err := http.NewRequest("GET", urlPath, nil)
+			if err != nil {
+				t.Fatalf("Test %d: Could not create HTTP request: %v", i, err)
 			}
-		} else {
-			if code != 0 {
-				t.Errorf("Test %d: Expected no status code found %d", i, code)
+
+			rec := httptest.NewRecorder()
+			code, err := rw.ServeHTTP(rec, req)
+			if err != nil {
+				t.Fatalf("Test %d: No error expected for handler but found %v", i, err)
+			}
+			if s.statusExpected {
+				if rec.Body.String() != "" {
+					t.Errorf("Test %d: Expected empty body but found %s", i, rec.Body.String())
+				}
+				if code != s.status {
+					t.Errorf("Test %d: Expected status code %d found %d", i, s.status, code)
+				}
+			} else {
+				if code != 0 {
+					t.Errorf("Test %d: Expected no status code found %d", i, code)
+				}
 			}
 		}
-	}
-}
-
-func urlPrinter(w http.ResponseWriter, r *http.Request) (int, error) {
-	fmt.Fprintf(w, r.URL.String())
-	return 0, nil
+	*/
 }
-*/