middleware/metrics: fix crash on startup (#318)

Make the methods that handle Metrics all use pointer receivers to fix
sync.Once not being initialized.

Finish the setup_test to test for failures. And make the check for the
address more strict and return an error when it does not have a port
number.

Add a toplevel test that starts a CoreDNS server with metrics enabled
so we catch these errors in the future.

middleware/metrics/handler.go

@@ -13,7 +13,7 @@ import (
 )
 
 // ServeDNS implements the Handler interface.
-func (m Metrics) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
+func (m *Metrics) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
 	state := request.Request{W: w, Req: r}
 
 	qname := state.QName()

middleware/metrics/metrics.go

@@ -32,7 +32,7 @@ type Metrics struct {
 	Addr      string
 	ln        net.Listener
 	mux       *http.ServeMux
-	Once      *sync.Once
+	Once      sync.Once
 	ZoneNames []string
 }
 

middleware/metrics/setup.go

 package metrics
 
 import (
+	"net"
 	"sync"
 
 	"github.com/miekg/coredns/core/dnsserver"
@@ -35,15 +36,15 @@ func setup(c *caddy.Controller) error {
 	return nil
 }
 
-func prometheusParse(c *caddy.Controller) (Metrics, error) {
+func prometheusParse(c *caddy.Controller) (*Metrics, error) {
 	var (
-		met Metrics
+		met = &Metrics{Addr: addr}
 		err error
 	)
 
 	for c.Next() {
 		if len(met.ZoneNames) > 0 {
-			return Metrics{}, c.Err("metrics: can only have one metrics module per server")
+			return met, c.Err("metrics: can only have one metrics module per server")
 		}
 		met.ZoneNames = make([]string, len(c.ServerBlockKeys))
 		copy(met.ZoneNames, c.ServerBlockKeys)
@@ -56,26 +57,32 @@ func prometheusParse(c *caddy.Controller) (Metrics, error) {
 		case 0:
 		case 1:
 			met.Addr = args[0]
+			_, _, e := net.SplitHostPort(met.Addr)
+			if e != nil {
+				return met, e
+			}
 		default:
-			return Metrics{}, c.ArgErr()
+			return met, c.ArgErr()
 		}
 		for c.NextBlock() {
 			switch c.Val() {
 			case "address":
 				args = c.RemainingArgs()
 				if len(args) != 1 {
-					return Metrics{}, c.ArgErr()
+					return met, c.ArgErr()
 				}
 				met.Addr = args[0]
+				// expecting something that resembles a host-port
+				_, _, e := net.SplitHostPort(met.Addr)
+				if e != nil {
+					return met, e
+				}
 			default:
-				return Metrics{}, c.Errf("metrics: unknown item: %s", c.Val())
+				return met, c.Errf("metrics: unknown item: %s", c.Val())
 			}
 
 		}
 	}
-	if met.Addr == "" {
-		met.Addr = addr
-	}
 	return met, err
 }
 

middleware/metrics/setup_test.go

@@ -10,19 +10,32 @@ func TestPrometheus(t *testing.T) {
 	tests := []struct {
 		input     string
 		shouldErr bool
+		addr      string
 	}{
-		{`prometheus`, false},
-		{`prometheus {}`, false},   // TODO(miek): should be true
-		{`prometheus /foo`, false}, // TODO(miek): should be true
-		{`prometheus localhost:53`, false},
+		// oks
+		{`prometheus`, false, "localhost:9153"},
+		{`prometheus localhost:53`, false, "localhost:53"},
+		// fails
+		{`prometheus {}`, true, ""},
+		{`prometheus /foo`, true, ""},
 	}
 	for i, test := range tests {
 		c := caddy.NewTestController("dns", test.input)
-		err := setup(c)
+		m, err := prometheusParse(c)
 		if test.shouldErr && err == nil {
 			t.Errorf("Test %v: Expected error but found nil", i)
+			continue
 		} else if !test.shouldErr && err != nil {
 			t.Errorf("Test %v: Expected no error but found error: %v", i, err)
+			continue
+		}
+
+		if test.shouldErr {
+			continue
+		}
+
+		if test.addr != m.Addr {
+			t.Errorf("Test %v: Expected address %s but found: %s", i, test.addr, m.Addr)
 		}
 	}
 }

test/metrics_test.go

+package test
+
+import "testing"
+
+// Start test server that has metrics enabled. Then tear it down again.
+func TestMetricsServer(t *testing.T) {
+	corefile := `.:0 {
+	chaos CoreDNS-001 miek@miek.nl
+	prometheus localhost:0
+}
+`
+	srv, err := CoreDNSServer(corefile)
+	if err != nil {
+		t.Fatalf("Could not get CoreDNS serving instance: %s", err)
+	}
+	defer srv.Stop()
+}