plugin/dnstap: Fix behavior when multiple dnstap plugins specified (#5773)

* fix multiple dnstap plugins behavior

Signed-off-by: Chris O'Haver <cohaver@infoblox.com>

6 files changed, 218 insertions, 119 deletions

diff --git a/plugin/dnstap/README.md b/plugin/dnstap/README.md
index 8e5615772..7405ddd77 100644
--- a/plugin/dnstap/README.md
+++ b/plugin/dnstap/README.md
@@ -61,6 +61,15 @@ dnstap /tmp/dnstap.sock {
 }
 ~~~
 
+You can use _dnstap_ more than once to define multiple taps. The following logs information including the
+wire-format DNS message about client requests and responses to */tmp/dnstap.sock*,
+and also sends client requests and responses without wire-format DNS messages to a remote FQDN.
+
+~~~ txt
+dnstap /tmp/dnstap.sock full
+dnstap tcp://example.com:6000
+~~~
+
 ## Command Line Tool
 
 Dnstap has a command line tool that can be used to inspect the logging. The tool can be found
@@ -86,13 +95,15 @@ $ dnstap -l 127.0.0.1:6000
 
 ## Using Dnstap in your plugin
 
-In your setup function, check to see if the *dnstap* plugin is loaded:
+In your setup function, collect and store a list of all *dnstap* plugins loaded in the config:
 
 ~~~ go
+x :=  &ExamplePlugin{}
+
 c.OnStartup(func() error {
     if taph := dnsserver.GetConfig(c).Handler("dnstap"); taph != nil {
         if tapPlugin, ok := taph.(dnstap.Dnstap); ok {
-            f.tapPlugin = &tapPlugin
+            x.tapPlugins = append(x.tapPlugins, &tapPlugin)
         }
     }
     return nil
@@ -102,8 +113,13 @@ c.OnStartup(func() error {
 And then in your plugin:
 
 ~~~ go
-func (x RandomPlugin) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
-    if tapPlugin != nil {
+import (
+  github.com/coredns/coredns/plugin/dnstap/msg
+  tap "github.com/dnstap/golang-dnstap"
+)
+
+func (x ExamplePlugin) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
+    for _, tapPlugin := range x.tapPlugins {
         q := new(msg.Msg)
         msg.SetQueryTime(q, time.Now())
         msg.SetQueryAddress(q, w.RemoteAddr())
diff --git a/plugin/dnstap/setup.go b/plugin/dnstap/setup.go
index d7d1cdc1b..ff9b3d89e 100644
--- a/plugin/dnstap/setup.go
+++ b/plugin/dnstap/setup.go
@@ -15,89 +15,104 @@ var log = clog.NewWithPlugin("dnstap")
 
 func init() { plugin.Register("dnstap", setup) }
 
-func parseConfig(c *caddy.Controller) (Dnstap, error) {
-	c.Next() // directive name
-	d := Dnstap{}
-	endpoint := ""
+func parseConfig(c *caddy.Controller) ([]*Dnstap, error) {
+	dnstaps := []*Dnstap{}
 
-	args := c.RemainingArgs()
+	for c.Next() { // directive name
+		d := Dnstap{}
+		endpoint := ""
 
-	if len(args) == 0 {
-		return d, c.ArgErr()
-	}
+		args := c.RemainingArgs()
+
+		if len(args) == 0 {
+			return nil, c.ArgErr()
+		}
 
-	endpoint = args[0]
+		endpoint = args[0]
 
-	if strings.HasPrefix(endpoint, "tcp://") {
-		// remote network endpoint
-		endpointURL, err := url.Parse(endpoint)
-		if err != nil {
-			return d, c.ArgErr()
+		if strings.HasPrefix(endpoint, "tcp://") {
+			// remote network endpoint
+			endpointURL, err := url.Parse(endpoint)
+			if err != nil {
+				return nil, c.ArgErr()
+			}
+			dio := newIO("tcp", endpointURL.Host)
+			d = Dnstap{io: dio}
+		} else {
+			endpoint = strings.TrimPrefix(endpoint, "unix://")
+			dio := newIO("unix", endpoint)
+			d = Dnstap{io: dio}
 		}
-		dio := newIO("tcp", endpointURL.Host)
-		d = Dnstap{io: dio}
-	} else {
-		endpoint = strings.TrimPrefix(endpoint, "unix://")
-		dio := newIO("unix", endpoint)
-		d = Dnstap{io: dio}
-	}
 
-	d.IncludeRawMessage = len(args) == 2 && args[1] == "full"
+		d.IncludeRawMessage = len(args) == 2 && args[1] == "full"
 
-	hostname, _ := os.Hostname()
-	d.Identity = []byte(hostname)
-	d.Version = []byte(caddy.AppName + "-" + caddy.AppVersion)
+		hostname, _ := os.Hostname()
+		d.Identity = []byte(hostname)
+		d.Version = []byte(caddy.AppName + "-" + caddy.AppVersion)
 
-	for c.NextBlock() {
-		switch c.Val() {
-		case "identity":
-			{
-				if !c.NextArg() {
-					return d, c.ArgErr()
+		for c.NextBlock() {
+			switch c.Val() {
+			case "identity":
+				{
+					if !c.NextArg() {
+						return nil, c.ArgErr()
+					}
+					d.Identity = []byte(c.Val())
 				}
-				d.Identity = []byte(c.Val())
-			}
-		case "version":
-			{
-				if !c.NextArg() {
-					return d, c.ArgErr()
+			case "version":
+				{
+					if !c.NextArg() {
+						return nil, c.ArgErr()
+					}
+					d.Version = []byte(c.Val())
 				}
-				d.Version = []byte(c.Val())
 			}
 		}
+		dnstaps = append(dnstaps, &d)
 	}
-
-	return d, nil
+	return dnstaps, nil
 }
 
 func setup(c *caddy.Controller) error {
-	dnstap, err := parseConfig(c)
+	dnstaps, err := parseConfig(c)
 	if err != nil {
 		return plugin.Error("dnstap", err)
 	}
 
-	c.OnStartup(func() error {
-		if err := dnstap.io.(*dio).connect(); err != nil {
-			log.Errorf("No connection to dnstap endpoint: %s", err)
-		}
-		return nil
-	})
-
-	c.OnRestart(func() error {
-		dnstap.io.(*dio).close()
-		return nil
-	})
-
-	c.OnFinalShutdown(func() error {
-		dnstap.io.(*dio).close()
-		return nil
-	})
-
-	dnsserver.GetConfig(c).AddPlugin(
-		func(next plugin.Handler) plugin.Handler {
-			dnstap.Next = next
-			return dnstap
+	for i := range dnstaps {
+		dnstap := dnstaps[i]
+		c.OnStartup(func() error {
+			if err := dnstap.io.(*dio).connect(); err != nil {
+				log.Errorf("No connection to dnstap endpoint: %s", err)
+			}
+			return nil
+		})
+
+		c.OnRestart(func() error {
+			dnstap.io.(*dio).close()
+			return nil
+		})
+
+		c.OnFinalShutdown(func() error {
+			dnstap.io.(*dio).close()
+			return nil
 		})
 
+		if i == len(dnstaps)-1 {
+			// last dnstap plugin in block: point next to next plugin
+			dnsserver.GetConfig(c).AddPlugin(func(next plugin.Handler) plugin.Handler {
+				dnstap.Next = next
+				return dnstap
+			})
+		} else {
+			// not last dnstap plugin in block: point next to next dnstap
+			nextDnstap := dnstaps[i+1]
+			dnsserver.GetConfig(c).AddPlugin(func(plugin.Handler) plugin.Handler {
+				dnstap.Next = nextDnstap
+				return dnstap
+			})
+		}
+	}
+
 	return nil
 }
diff --git a/plugin/dnstap/setup_test.go b/plugin/dnstap/setup_test.go
index 9d5f20a92..92ce5f055 100644
--- a/plugin/dnstap/setup_test.go
+++ b/plugin/dnstap/setup_test.go
@@ -2,35 +2,52 @@ package dnstap
 
 import (
 	"os"
+	"reflect"
 	"testing"
 
 	"github.com/coredns/caddy"
+	"github.com/coredns/coredns/core/dnsserver"
 )
 
+type results struct {
+	endpoint string
+	full     bool
+	proto    string
+	identity []byte
+	version  []byte
+}
+
 func TestConfig(t *testing.T) {
 	hostname, _ := os.Hostname()
 	tests := []struct {
-		in       string
-		endpoint string
-		full     bool
-		proto    string
-		fail     bool
-		identity []byte
-		version  []byte
+		in     string
+		fail   bool
+		expect []results
 	}{
-		{"dnstap dnstap.sock full", "dnstap.sock", true, "unix", false, []byte(hostname), []byte("-")},
-		{"dnstap unix://dnstap.sock", "dnstap.sock", false, "unix", false, []byte(hostname), []byte("-")},
-		{"dnstap tcp://127.0.0.1:6000", "127.0.0.1:6000", false, "tcp", false, []byte(hostname), []byte("-")},
-		{"dnstap tcp://[::1]:6000", "[::1]:6000", false, "tcp", false, []byte(hostname), []byte("-")},
-		{"dnstap tcp://example.com:6000", "example.com:6000", false, "tcp", false, []byte(hostname), []byte("-")},
-		{"dnstap", "fail", false, "tcp", true, []byte(hostname), []byte("-")},
-		{"dnstap dnstap.sock full {\nidentity NAME\nversion VER\n}\n", "dnstap.sock", true, "unix", false, []byte("NAME"), []byte("VER")},
-		{"dnstap dnstap.sock {\nidentity NAME\nversion VER\n}\n", "dnstap.sock", false, "unix", false, []byte("NAME"), []byte("VER")},
-		{"dnstap {\nidentity NAME\nversion VER\n}\n", "fail", false, "tcp", true, []byte("NAME"), []byte("VER")},
+		{"dnstap dnstap.sock full", false, []results{{"dnstap.sock", true, "unix", []byte(hostname), []byte("-")}}},
+		{"dnstap unix://dnstap.sock", false, []results{{"dnstap.sock", false, "unix", []byte(hostname), []byte("-")}}},
+		{"dnstap tcp://127.0.0.1:6000", false, []results{{"127.0.0.1:6000", false, "tcp", []byte(hostname), []byte("-")}}},
+		{"dnstap tcp://[::1]:6000", false, []results{{"[::1]:6000", false, "tcp", []byte(hostname), []byte("-")}}},
+		{"dnstap tcp://example.com:6000", false, []results{{"example.com:6000", false, "tcp", []byte(hostname), []byte("-")}}},
+		{"dnstap", true, []results{{"fail", false, "tcp", []byte(hostname), []byte("-")}}},
+		{"dnstap dnstap.sock full {\nidentity NAME\nversion VER\n}\n", false, []results{{"dnstap.sock", true, "unix", []byte("NAME"), []byte("VER")}}},
+		{"dnstap dnstap.sock {\nidentity NAME\nversion VER\n}\n", false, []results{{"dnstap.sock", false, "unix", []byte("NAME"), []byte("VER")}}},
+		{"dnstap {\nidentity NAME\nversion VER\n}\n", true, []results{{"fail", false, "tcp", []byte("NAME"), []byte("VER")}}},
+		{`dnstap dnstap.sock full {
+                identity NAME
+                version VER
+              }
+              dnstap tcp://127.0.0.1:6000 {
+                identity NAME2
+                version VER2
+              }`, false, []results{
+			{"dnstap.sock", true, "unix", []byte("NAME"), []byte("VER")},
+			{"127.0.0.1:6000", false, "tcp", []byte("NAME2"), []byte("VER2")},
+		}},
 	}
 	for i, tc := range tests {
 		c := caddy.NewTestController("dns", tc.in)
-		tap, err := parseConfig(c)
+		taps, err := parseConfig(c)
 		if tc.fail && err == nil {
 			t.Fatalf("Test %d: expected test to fail: %s: %s", i, tc.in, err)
 		}
@@ -41,20 +58,69 @@ func TestConfig(t *testing.T) {
 		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)
-		}
-		if x := string(tap.Identity); x != string(tc.identity) {
-			t.Errorf("Test %d: expected identity %s, got %s", i, tc.identity, x)
-		}
-		if x := string(tap.Version); x != string(tc.version) {
-			t.Errorf("Test %d: expected version %s, got %s", i, tc.version, x)
+		for i, tap := range taps {
+			if x := tap.io.(*dio).endpoint; x != tc.expect[i].endpoint {
+				t.Errorf("Test %d: expected endpoint %s, got %s", i, tc.expect[i].endpoint, x)
+			}
+			if x := tap.io.(*dio).proto; x != tc.expect[i].proto {
+				t.Errorf("Test %d: expected proto %s, got %s", i, tc.expect[i].proto, x)
+			}
+			if x := tap.IncludeRawMessage; x != tc.expect[i].full {
+				t.Errorf("Test %d: expected IncludeRawMessage %t, got %t", i, tc.expect[i].full, x)
+			}
+			if x := string(tap.Identity); x != string(tc.expect[i].identity) {
+				t.Errorf("Test %d: expected identity %s, got %s", i, tc.expect[i].identity, x)
+			}
+			if x := string(tap.Version); x != string(tc.expect[i].version) {
+				t.Errorf("Test %d: expected version %s, got %s", i, tc.expect[i].version, x)
+			}
 		}
 	}
 }
+
+func TestMultiDnstap(t *testing.T) {
+	input := `
+      dnstap dnstap1.sock
+      dnstap dnstap2.sock
+      dnstap dnstap3.sock
+    `
+
+	c := caddy.NewTestController("dns", input)
+	setup(c)
+	dnsserver.NewServer("", []*dnsserver.Config{dnsserver.GetConfig(c)})
+
+	handlers := dnsserver.GetConfig(c).Handlers()
+	d1, ok := handlers[0].(*Dnstap)
+	if !ok {
+		t.Fatalf("expected first plugin to be Dnstap, got %v", reflect.TypeOf(d1.Next))
+	}
+
+	if d1.io.(*dio).endpoint != "dnstap1.sock" {
+		t.Errorf("expected first dnstap to \"dnstap1.sock\", got %q", d1.io.(*dio).endpoint)
+	}
+	if d1.Next == nil {
+		t.Fatal("expected first dnstap to point to next dnstap instance")
+	}
+
+	d2, ok := d1.Next.(*Dnstap)
+	if !ok {
+		t.Fatalf("expected second plugin to be Dnstap, got %v", reflect.TypeOf(d1.Next))
+	}
+	if d2.io.(*dio).endpoint != "dnstap2.sock" {
+		t.Errorf("expected second dnstap to \"dnstap2.sock\", got %q", d2.io.(*dio).endpoint)
+	}
+	if d2.Next == nil {
+		t.Fatal("expected second dnstap to point to third dnstap instance")
+	}
+
+	d3, ok := d2.Next.(*Dnstap)
+	if !ok {
+		t.Fatalf("expected third plugin to be Dnstap, got %v", reflect.TypeOf(d2.Next))
+	}
+	if d3.io.(*dio).endpoint != "dnstap3.sock" {
+		t.Errorf("expected third dnstap to \"dnstap3.sock\", got %q", d3.io.(*dio).endpoint)
+	}
+	if d3.Next != nil {
+		t.Error("expected third plugin to be last, but Next is not nil")
+	}
+}
diff --git a/plugin/forward/dnstap.go b/plugin/forward/dnstap.go
index 4e06ac1ff..95ef90acf 100644
--- a/plugin/forward/dnstap.go
+++ b/plugin/forward/dnstap.go
@@ -39,25 +39,27 @@ func toDnstap(f *Forward, host string, state request.Request, opts options, repl
 	msg.SetQueryAddress(q, state.W.RemoteAddr())
 	msg.SetResponseAddress(q, ta)
 
-	if f.tapPlugin.IncludeRawMessage {
-		buf, _ := state.Req.Pack()
-		q.QueryMessage = buf
-	}
-	msg.SetType(q, tap.Message_FORWARDER_QUERY)
-	f.tapPlugin.TapMessage(q)
-
-	// Response
-	if reply != nil {
-		r := new(tap.Message)
-		if f.tapPlugin.IncludeRawMessage {
-			buf, _ := reply.Pack()
-			r.ResponseMessage = buf
+	for _, t := range f.tapPlugins {
+		if t.IncludeRawMessage {
+			buf, _ := state.Req.Pack()
+			q.QueryMessage = buf
+		}
+		msg.SetType(q, tap.Message_FORWARDER_QUERY)
+		t.TapMessage(q)
+
+		// Response
+		if reply != nil {
+			r := new(tap.Message)
+			if t.IncludeRawMessage {
+				buf, _ := reply.Pack()
+				r.ResponseMessage = buf
+			}
+			msg.SetQueryTime(r, start)
+			msg.SetQueryAddress(r, state.W.RemoteAddr())
+			msg.SetResponseAddress(r, ta)
+			msg.SetResponseTime(r, time.Now())
+			msg.SetType(r, tap.Message_FORWARDER_RESPONSE)
+			t.TapMessage(r)
 		}
-		msg.SetQueryTime(r, start)
-		msg.SetQueryAddress(r, state.W.RemoteAddr())
-		msg.SetResponseAddress(r, ta)
-		msg.SetResponseTime(r, time.Now())
-		msg.SetType(r, tap.Message_FORWARDER_RESPONSE)
-		f.tapPlugin.TapMessage(r)
 	}
 }
diff --git a/plugin/forward/forward.go b/plugin/forward/forward.go
index 90ae1aef8..b32e8b3b4 100644
--- a/plugin/forward/forward.go
+++ b/plugin/forward/forward.go
@@ -49,7 +49,7 @@ type Forward struct {
 	// the maximum allowed (maxConcurrent)
 	ErrLimitExceeded error
 
-	tapPlugin *dnstap.Dnstap // when the dnstap plugin is loaded, we use to this to send messages out.
+	tapPlugins []*dnstap.Dnstap // when dnstap plugins are loaded, we use to this to send messages out.
 
 	Next plugin.Handler
 }
@@ -150,7 +150,7 @@ func (f *Forward) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg
 			child.Finish()
 		}
 
-		if f.tapPlugin != nil {
+		if len(f.tapPlugins) != 0 {
 			toDnstap(f, proxy.addr, state, opts, ret, start)
 		}
 
diff --git a/plugin/forward/setup.go b/plugin/forward/setup.go
index dfae70d37..39709301e 100644
--- a/plugin/forward/setup.go
+++ b/plugin/forward/setup.go
@@ -52,7 +52,7 @@ func setup(c *caddy.Controller) error {
 		c.OnStartup(func() error {
 			if taph := dnsserver.GetConfig(c).Handler("dnstap"); taph != nil {
 				if tapPlugin, ok := taph.(dnstap.Dnstap); ok {
-					f.tapPlugin = &tapPlugin
+					f.tapPlugins = append(f.tapPlugins, &tapPlugin)
 				}
 			}
 			return nil