From 123da4c8443f19a9a9e427a95fe6f47160cd0c1b Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Thu, 5 Nov 2020 14:37:16 +0100 Subject: [PATCH] plugin/dnstap: remove config struct (#4258) * plugin/dnstap: remove config struct this struct is an uneeded intermidiate to get a dnstap it can be removed. Remove the dnstapio subpkg: it's also not needed. Make *many* functions and structs private now that we can. Signed-off-by: Miek Gieben * correct logging Signed-off-by: Miek Gieben --- plugin/dnstap/README.md | 2 +- plugin/dnstap/{dnstapio => }/encoder.go | 19 +++++---- plugin/dnstap/handler.go | 7 ++-- plugin/dnstap/{dnstapio => }/io.go | 36 ++++++++--------- plugin/dnstap/{dnstapio => }/io_test.go | 34 ++++++++-------- plugin/dnstap/setup.go | 52 ++++++++++++------------- plugin/dnstap/setup_test.go | 40 ++++++++++++------- plugin/dnstap/writer.go | 14 +++---- 8 files changed, 102 insertions(+), 102 deletions(-) rename plugin/dnstap/{dnstapio => }/encoder.go (57%) rename plugin/dnstap/{dnstapio => }/io.go (74%) rename plugin/dnstap/{dnstapio => }/io_test.go (85%) diff --git a/plugin/dnstap/README.md b/plugin/dnstap/README.md index 8dc9b5674..095d33e24 100644 --- a/plugin/dnstap/README.md +++ b/plugin/dnstap/README.md @@ -18,7 +18,7 @@ Every message is sent to the socket as soon as it comes in, the *dnstap* plugin dnstap SOCKET [full] ~~~ -* **SOCKET** is the socket path supplied to the dnstap command line tool. +* **SOCKET** is the socket (path) supplied to the dnstap command line tool. * `full` to include the wire-format DNS message. ## Examples diff --git a/plugin/dnstap/dnstapio/encoder.go b/plugin/dnstap/encoder.go similarity index 57% rename from plugin/dnstap/dnstapio/encoder.go rename to plugin/dnstap/encoder.go index 2b4a76cd5..09b1e2e6f 100644 --- a/plugin/dnstap/dnstapio/encoder.go +++ b/plugin/dnstap/encoder.go @@ -1,5 +1,4 @@ -// Package dnstapio is a small wrapper around golang-framestream -package dnstapio +package dnstap import ( "io" @@ -10,12 +9,12 @@ import ( "github.com/golang/protobuf/proto" ) -// Encoder wraps a fs.Encoder. -type Encoder struct { +// encoder wraps a golang-framestream.Encoder. +type encoder struct { fs *fs.Encoder } -func newEncoder(w io.Writer, timeout time.Duration) (*Encoder, error) { +func newEncoder(w io.Writer, timeout time.Duration) (*encoder, error) { fs, err := fs.NewEncoder(w, &fs.EncoderOptions{ ContentType: []byte("protobuf:dnstap.Dnstap"), Bidirectional: true, @@ -24,18 +23,18 @@ func newEncoder(w io.Writer, timeout time.Duration) (*Encoder, error) { if err != nil { return nil, err } - return &Encoder{fs}, nil + return &encoder{fs}, nil } -func (e *Encoder) writeMsg(msg *tap.Dnstap) error { +func (e *encoder) writeMsg(msg *tap.Dnstap) error { buf, err := proto.Marshal(msg) if err != nil { return err } - _, err = e.fs.Write(buf) // n < len(buf) should return an error + _, err = e.fs.Write(buf) // n < len(buf) should return an error? return err } -func (e *Encoder) flush() error { return e.fs.Flush() } -func (e *Encoder) close() error { return e.fs.Close() } +func (e *encoder) flush() error { return e.fs.Flush() } +func (e *encoder) close() error { return e.fs.Close() } diff --git a/plugin/dnstap/handler.go b/plugin/dnstap/handler.go index 7451d63f0..b31508b81 100644 --- a/plugin/dnstap/handler.go +++ b/plugin/dnstap/handler.go @@ -5,7 +5,6 @@ import ( "time" "github.com/coredns/coredns/plugin" - "github.com/coredns/coredns/plugin/dnstap/dnstapio" tap "github.com/dnstap/golang-dnstap" "github.com/miekg/dns" @@ -14,7 +13,7 @@ import ( // Dnstap is the dnstap handler. type Dnstap struct { Next plugin.Handler - io dnstapio.Tapper + io tapper // IncludeRawMessage will include the raw DNS message into the dnstap messages if true. IncludeRawMessage bool @@ -31,8 +30,8 @@ func (h Dnstap) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) rw := &ResponseWriter{ ResponseWriter: w, Dnstap: h, - Query: r, - QueryTime: time.Now(), + query: r, + queryTime: time.Now(), } return plugin.NextOrFailure(h.Name(), h.Next, ctx, rw, r) diff --git a/plugin/dnstap/dnstapio/io.go b/plugin/dnstap/io.go similarity index 74% rename from plugin/dnstap/dnstapio/io.go rename to plugin/dnstap/io.go index d85196cc8..6823fa8a6 100644 --- a/plugin/dnstap/dnstapio/io.go +++ b/plugin/dnstap/io.go @@ -1,26 +1,23 @@ -package dnstapio +package dnstap import ( "net" "sync/atomic" "time" - clog "github.com/coredns/coredns/plugin/pkg/log" - tap "github.com/dnstap/golang-dnstap" ) -var log = clog.NewWithPlugin("dnstap") - const ( - tcpWriteBufSize = 1024 * 1024 // there is no good explanation for why this number (see #xxx) - queueSize = 10000 // see #xxxx - tcpTimeout = 4 * time.Second - flushTimeout = 1 * time.Second + tcpWriteBufSize = 1024 * 1024 // there is no good explanation for why this number has this value. + queueSize = 10000 // idem. + + tcpTimeout = 4 * time.Second + flushTimeout = 1 * time.Second ) -// Tapper interface is used in testing to mock the Dnstap method. -type Tapper interface { +// tapper interface is used in testing to mock the Dnstap method. +type tapper interface { Dnstap(tap.Dnstap) } @@ -29,7 +26,7 @@ type dio struct { endpoint string proto string conn net.Conn - enc *Encoder + enc *encoder queue chan tap.Dnstap dropped uint32 quit chan struct{} @@ -37,8 +34,8 @@ type dio struct { tcpTimeout time.Duration } -// New returns a new and initialized pointer to a dio. -func New(proto, endpoint string) *dio { +// newIO returns a new and initialized pointer to a dio. +func newIO(proto, endpoint string) *dio { return &dio{ endpoint: endpoint, proto: proto, @@ -64,11 +61,10 @@ func (d *dio) dial() error { } // Connect connects to the dnstap endpoint. -func (d *dio) Connect() { - if err := d.dial(); err != nil { - log.Errorf("No connection to dnstap endpoint: %s", err) - } +func (d *dio) connect() error { + err := d.dial() go d.serve() + return err } // Dnstap enqueues the payload for log. @@ -80,8 +76,8 @@ func (d *dio) Dnstap(payload tap.Dnstap) { } } -// Close waits until the I/O routine is finished to return. -func (d *dio) Close() { close(d.quit) } +// close waits until the I/O routine is finished to return. +func (d *dio) close() { close(d.quit) } func (d *dio) write(payload *tap.Dnstap) error { if d.enc == nil { diff --git a/plugin/dnstap/dnstapio/io_test.go b/plugin/dnstap/io_test.go similarity index 85% rename from plugin/dnstap/dnstapio/io_test.go rename to plugin/dnstap/io_test.go index 6870734e4..30f0c75fb 100644 --- a/plugin/dnstap/dnstapio/io_test.go +++ b/plugin/dnstap/io_test.go @@ -1,4 +1,4 @@ -package dnstapio +package dnstap import ( "net" @@ -14,7 +14,7 @@ import ( var ( msgType = tap.Dnstap_MESSAGE - msg = tap.Dnstap{Type: &msgType} + tmsg = tap.Dnstap{Type: &msgType} ) func accept(t *testing.T, l net.Listener, count int) { @@ -42,14 +42,12 @@ func accept(t *testing.T, l net.Listener, count int) { } func TestTransport(t *testing.T) { - transport := [2][2]string{ {"tcp", ":0"}, {"unix", "dnstap.sock"}, } for _, param := range transport { - // Start TCP listener l, err := reuseport.Listen(param[0], param[1]) if err != nil { t.Fatalf("Cannot start listener: %s", err) @@ -62,16 +60,16 @@ func TestTransport(t *testing.T) { wg.Done() }() - dio := New(param[0], l.Addr().String()) + dio := newIO(param[0], l.Addr().String()) dio.tcpTimeout = 10 * time.Millisecond dio.flushTimeout = 30 * time.Millisecond - dio.Connect() + dio.connect() - dio.Dnstap(msg) + dio.Dnstap(tmsg) wg.Wait() l.Close() - dio.Close() + dio.close() } } @@ -91,17 +89,17 @@ func TestRace(t *testing.T) { wg.Done() }() - dio := New("tcp", l.Addr().String()) + dio := newIO("tcp", l.Addr().String()) dio.tcpTimeout = 10 * time.Millisecond dio.flushTimeout = 30 * time.Millisecond - dio.Connect() - defer dio.Close() + dio.connect() + defer dio.close() wg.Add(count) for i := 0; i < count; i++ { go func() { - msg := tap.Dnstap_MESSAGE - dio.Dnstap(tap.Dnstap{Type: &msg}) + tmsg := tap.Dnstap_MESSAGE + dio.Dnstap(tap.Dnstap{Type: &tmsg}) wg.Done() }() } @@ -124,13 +122,13 @@ func TestReconnect(t *testing.T) { }() addr := l.Addr().String() - dio := New("tcp", addr) + dio := newIO("tcp", addr) dio.tcpTimeout = 10 * time.Millisecond dio.flushTimeout = 30 * time.Millisecond - dio.Connect() - defer dio.Close() + dio.connect() + defer dio.close() - dio.Dnstap(msg) + dio.Dnstap(tmsg) wg.Wait() @@ -151,7 +149,7 @@ func TestReconnect(t *testing.T) { for i := 0; i < count; i++ { time.Sleep(100 * time.Millisecond) - dio.Dnstap(msg) + dio.Dnstap(tmsg) } wg.Wait() } diff --git a/plugin/dnstap/setup.go b/plugin/dnstap/setup.go index a863639ad..4324087dd 100644 --- a/plugin/dnstap/setup.go +++ b/plugin/dnstap/setup.go @@ -6,64 +6,62 @@ import ( "github.com/coredns/caddy" "github.com/coredns/coredns/core/dnsserver" "github.com/coredns/coredns/plugin" - "github.com/coredns/coredns/plugin/dnstap/dnstapio" + clog "github.com/coredns/coredns/plugin/pkg/log" "github.com/coredns/coredns/plugin/pkg/parse" ) +var log = clog.NewWithPlugin("dnstap") + func init() { plugin.Register("dnstap", setup) } -type config struct { - proto string - target string - full bool -} +func parseConfig(c *caddy.Controller) (Dnstap, error) { + c.Next() // directive name + d := Dnstap{} + endpoint := "" -func parseConfig(d *caddy.Controller) (c config, err error) { - d.Next() // directive name - - if !d.Args(&c.target) { - return c, d.ArgErr() + if !c.Args(&endpoint) { + return d, c.ArgErr() } - if strings.HasPrefix(c.target, "tcp://") { + if strings.HasPrefix(endpoint, "tcp://") { // remote IP endpoint - servers, err := parse.HostPortOrFile(c.target[6:]) + servers, err := parse.HostPortOrFile(endpoint[6:]) if err != nil { - return c, d.ArgErr() + return d, c.ArgErr() } - c.target = servers[0] - c.proto = "tcp" + dio := newIO("tcp", servers[0]) + d = Dnstap{io: dio} } else { - c.target = strings.TrimPrefix(c.target, "unix://") - c.proto = "unix" + endpoint = strings.TrimPrefix(endpoint, "unix://") + dio := newIO("unix", endpoint) + d = Dnstap{io: dio} } - c.full = d.NextArg() && d.Val() == "full" + d.IncludeRawMessage = c.NextArg() && c.Val() == "full" - return + return d, nil } func setup(c *caddy.Controller) error { - conf, err := parseConfig(c) + dnstap, err := parseConfig(c) if err != nil { return plugin.Error("dnstap", err) } - dio := dnstapio.New(conf.proto, conf.target) - dnstap := Dnstap{io: dio, IncludeRawMessage: conf.full} - c.OnStartup(func() error { - dio.Connect() + if err := dnstap.io.(*dio).connect(); err != nil { + log.Errorf("No connection to dnstap endpoint: %s", err) + } return nil }) c.OnRestart(func() error { - dio.Close() + dnstap.io.(*dio).close() return nil }) c.OnFinalShutdown(func() error { - dio.Close() + dnstap.io.(*dio).close() return nil }) diff --git a/plugin/dnstap/setup_test.go b/plugin/dnstap/setup_test.go index 129107efd..6b9ad284b 100644 --- a/plugin/dnstap/setup_test.go +++ b/plugin/dnstap/setup_test.go @@ -8,26 +8,38 @@ import ( func TestConfig(t *testing.T) { tests := []struct { - file string - path string - full bool - proto string - fail bool + in string + endpoint string + full bool + proto string + fail bool }{ {"dnstap dnstap.sock full", "dnstap.sock", true, "unix", false}, {"dnstap unix://dnstap.sock", "dnstap.sock", false, "unix", false}, {"dnstap tcp://127.0.0.1:6000", "127.0.0.1:6000", false, "tcp", false}, {"dnstap", "fail", false, "tcp", true}, } - for _, c := range tests { - cad := caddy.NewTestController("dns", c.file) - conf, err := parseConfig(cad) - if c.fail { - if err == nil { - t.Errorf("%s: %s", c.file, err) - } - } else if err != nil || conf.target != c.path || conf.full != c.full || conf.proto != c.proto { - t.Errorf("Expected: %+v\nhave: %+v\nerror: %s", c, conf, err) + for i, tc := range tests { + c := caddy.NewTestController("dns", tc.in) + tap, err := parseConfig(c) + if tc.fail && err == nil { + t.Fatalf("Test %d: expected test to fail: %s: %s", i, tc.in, err) + } + if tc.fail { + continue + } + + if err != nil { + t.Fatalf("Test %d: expected no error, got %s", i, err) + } + if x := tap.io.(*dio).endpoint; x != tc.endpoint { + t.Errorf("Test %d: expected endpoint %s, got %s", i, tc.endpoint, x) + } + if x := tap.io.(*dio).proto; x != tc.proto { + t.Errorf("Test %d: expected proto %s, got %s", i, tc.proto, x) + } + if x := tap.IncludeRawMessage; x != tc.full { + t.Errorf("Test %d: expected IncludeRawMessage %t, got %t", i, tc.full, x) } } } diff --git a/plugin/dnstap/writer.go b/plugin/dnstap/writer.go index 315a3a790..1683508a8 100644 --- a/plugin/dnstap/writer.go +++ b/plugin/dnstap/writer.go @@ -9,25 +9,23 @@ import ( ) // ResponseWriter captures the client response and logs the query to dnstap. -// Single request use. type ResponseWriter struct { - QueryTime time.Time - Query *dns.Msg + queryTime time.Time + query *dns.Msg dns.ResponseWriter Dnstap } -// WriteMsg writes back the response to the client and THEN works on logging the request -// and response to dnstap. +// WriteMsg writes back the response to the client and THEN works on logging the request and response to dnstap. func (w *ResponseWriter) WriteMsg(resp *dns.Msg) error { err := w.ResponseWriter.WriteMsg(resp) q := new(tap.Message) - msg.SetQueryTime(q, w.QueryTime) + msg.SetQueryTime(q, w.queryTime) msg.SetQueryAddress(q, w.RemoteAddr()) if w.IncludeRawMessage { - buf, _ := w.Query.Pack() + buf, _ := w.query.Pack() q.QueryMessage = buf } msg.SetType(q, tap.Message_CLIENT_QUERY) @@ -38,7 +36,7 @@ func (w *ResponseWriter) WriteMsg(resp *dns.Msg) error { } r := new(tap.Message) - msg.SetQueryTime(r, w.QueryTime) + msg.SetQueryTime(r, w.queryTime) msg.SetResponseTime(r, time.Now()) msg.SetQueryAddress(r, w.RemoteAddr())