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 <miek@miek.nl>

* correct logging

Signed-off-by: Miek Gieben <miek@miek.nl>

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

plugin/dnstap/encoder.go

@@ -1,5 +1,4 @@
-// Package dnstapio is a small wrapper around golang-framestream
+package dnstap
-package dnstapio
 
 import (
 	"io"
@@ -10,12 +9,12 @@ import (
 	"github.com/golang/protobuf/proto"
 )
 
-// Encoder wraps a fs.Encoder.
+// encoder wraps a golang-framestream.Encoder.
-type Encoder struct {
+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) flush() error { return e.fs.Flush() }
-func (e *Encoder) close() error { return e.fs.Close() }
+func (e *encoder) close() error { return e.fs.Close() }

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,
+		query:          r,
-		QueryTime:      time.Now(),
+		queryTime:      time.Now(),
 	}
 
 	return plugin.NextOrFailure(h.Name(), h.Next, ctx, rw, r)

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)
+	tcpWriteBufSize = 1024 * 1024 // there is no good explanation for why this number has this value.
-	queueSize       = 10000       // see #xxxx
+	queueSize       = 10000       // idem.
-	tcpTimeout      = 4 * time.Second
+
-	flushTimeout    = 1 * time.Second
+	tcpTimeout   = 4 * time.Second
+	flushTimeout = 1 * time.Second
 )
 
-// Tapper interface is used in testing to mock the Dnstap method.
+// tapper interface is used in testing to mock the Dnstap method.
-type Tapper interface {
+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.
+// newIO returns a new and initialized pointer to a dio.
-func New(proto, endpoint string) *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() {
+func (d *dio) connect() error {
-	if err := d.dial(); err != nil {
+	err := d.dial()
-		log.Errorf("No connection to dnstap endpoint: %s", err)
-	}
 	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.
+// close waits until the I/O routine is finished to return.
-func (d *dio) Close() { close(d.quit) }
+func (d *dio) close() { close(d.quit) }
 
 func (d *dio) write(payload *tap.Dnstap) error {
 	if d.enc == nil {

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()
+	dio.connect()
-	defer dio.Close()
+	defer dio.close()
 
 	wg.Add(count)
 	for i := 0; i < count; i++ {
 		go func() {
-			msg := tap.Dnstap_MESSAGE
+			tmsg := tap.Dnstap_MESSAGE
-			dio.Dnstap(tap.Dnstap{Type: &msg})
+			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()
+	dio.connect()
-	defer dio.Close()
+	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()
 }

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 {
+func parseConfig(c *caddy.Controller) (Dnstap, error) {
-	proto  string
+	c.Next() // directive name
-	target string
+	d := Dnstap{}
-	full   bool
+	endpoint := ""
-}
 
-func parseConfig(d *caddy.Controller) (c config, err error) {
+	if !c.Args(&endpoint) {
-	d.Next() // directive name
+		return d, c.ArgErr()
-
-	if !d.Args(&c.target) {
-		return c, d.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]
+		dio := newIO("tcp", servers[0])
-		c.proto = "tcp"
+		d = Dnstap{io: dio}
 	} else {
-		c.target = strings.TrimPrefix(c.target, "unix://")
+		endpoint = strings.TrimPrefix(endpoint, "unix://")
-		c.proto = "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
 	})
 

plugin/dnstap/setup_test.go

@@ -8,26 +8,38 @@ import (
 
 func TestConfig(t *testing.T) {
 	tests := []struct {
-		file  string
+		in       string
-		path  string
+		endpoint string
-		full  bool
+		full     bool
-		proto string
+		proto    string
-		fail  bool
+		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 {
+	for i, tc := range tests {
-		cad := caddy.NewTestController("dns", c.file)
+		c := caddy.NewTestController("dns", tc.in)
-		conf, err := parseConfig(cad)
+		tap, err := parseConfig(c)
-		if c.fail {
+		if tc.fail && err == nil {
-			if err == nil {
+			t.Fatalf("Test %d: expected test to fail: %s: %s", i, tc.in, err)
-				t.Errorf("%s: %s", c.file, err)
+		}
-			}
+		if tc.fail {
-		} else if err != nil || conf.target != c.path || conf.full != c.full || conf.proto != c.proto {
+			continue
-			t.Errorf("Expected: %+v\nhave: %+v\nerror: %s", c, conf, err)
+		}
+
+		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)
 		}
 	}
 }

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
+	queryTime time.Time
-	Query     *dns.Msg
+	query     *dns.Msg
 	dns.ResponseWriter
 	Dnstap
 }
 
-// WriteMsg writes back the response to the client and THEN works on logging the request
+// WriteMsg writes back the response to the client and THEN works on logging the request and response to dnstap.
-// 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())