From db6c9a3f01c9bbef12fd2a1b43e5a84608044ee0 Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Tue, 4 Oct 2016 11:05:04 +0100 Subject: [PATCH] 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 | 2 +- middleware/metrics/metrics.go | 2 +- middleware/metrics/setup.go | 25 ++++++++++++++++--------- middleware/metrics/setup_test.go | 23 ++++++++++++++++++----- test/metrics_test.go | 17 +++++++++++++++++ 5 files changed, 53 insertions(+), 16 deletions(-) create mode 100644 test/metrics_test.go diff --git a/middleware/metrics/handler.go b/middleware/metrics/handler.go index 2856621b5..a0247c517 100644 --- a/middleware/metrics/handler.go +++ b/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() diff --git a/middleware/metrics/metrics.go b/middleware/metrics/metrics.go index d8af7f7c5..b82a2be86 100644 --- a/middleware/metrics/metrics.go +++ b/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 } diff --git a/middleware/metrics/setup.go b/middleware/metrics/setup.go index fd3f4a80c..8c8dd1a75 100644 --- a/middleware/metrics/setup.go +++ b/middleware/metrics/setup.go @@ -1,6 +1,7 @@ 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 } diff --git a/middleware/metrics/setup_test.go b/middleware/metrics/setup_test.go index 231830c89..2971634f9 100644 --- a/middleware/metrics/setup_test.go +++ b/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) } } } diff --git a/test/metrics_test.go b/test/metrics_test.go new file mode 100644 index 000000000..85cb2a824 --- /dev/null +++ b/test/metrics_test.go @@ -0,0 +1,17 @@ +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() +}