plugin/forward: Enable multiple forward declarations (#5127)

* enable multiple declarations of forward plugin

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

5 files changed, 173 insertions, 70 deletions

diff --git a/plugin/forward/README.md b/plugin/forward/README.md
index d0c124e7f..5cc85b409 100644
--- a/plugin/forward/README.md
+++ b/plugin/forward/README.md
@@ -19,8 +19,6 @@ is taken as a healthy upstream. The health check uses the same protocol as speci
 When *all* upstreams are down it assumes health checking as a mechanism has failed and will try to
 connect to a random upstream (which may or may not work).
 
-This plugin can only be used once per Server Block.
-
 ## Syntax
 
 In its most basic form, a simple forwarder uses this syntax:
@@ -141,6 +139,40 @@ example.org {
 }
 ~~~
 
+Send all requests within `lab.example.local.` to `10.20.0.1`, all requests within `example.local.` (and not in
+`lab.example.local.`) to `10.0.0.1`, all others requests to the servers defined in `/etc/resolv.conf`, and
+caches results. Note that a CoreDNS server configured with multiple _forward_ plugins in a server block will evaluate those
+forward plugins in the order they are listed when serving a request.  Therefore, subdomains should be
+placed before parent domains otherwise subdomain requests will be forwarded to the parent domain's upstream.
+Accordingly, in this example `lab.example.local` is before `example.local`, and `example.local` is before `.`.
+
+~~~ corefile
+. {
+    cache
+    forward lab.example.local 10.20.0.1
+    forward example.local 10.0.0.1
+    forward . /etc/resolv.conf
+}
+~~~
+
+The example above is almost equivalent to the following example, except that example below defines three separate plugin
+chains (and thus 3 separate instances of _cache_).
+
+~~~ corefile
+lab.example.local {
+    cache
+    forward . 10.20.0.1
+}
+example.local {
+    cache
+    forward . 10.0.0.1
+}
+. {
+    cache
+    forward . /etc/resolv.conf
+}
+~~~
+
 Load balance all requests between three resolvers, one of which has a IPv6 address.
 
 ~~~ corefile
diff --git a/plugin/forward/proxy_test.go b/plugin/forward/proxy_test.go
index dc1f5fb18..74a0b5c4b 100644
--- a/plugin/forward/proxy_test.go
+++ b/plugin/forward/proxy_test.go
@@ -23,7 +23,8 @@ func TestProxy(t *testing.T) {
 	defer s.Close()
 
 	c := caddy.NewTestController("dns", "forward . "+s.Addr)
-	f, err := parseForward(c)
+	fs, err := parseForward(c)
+	f := fs[0]
 	if err != nil {
 		t.Errorf("Failed to create forwarder: %s", err)
 	}
@@ -53,7 +54,8 @@ func TestProxyTLSFail(t *testing.T) {
 	defer s.Close()
 
 	c := caddy.NewTestController("dns", "forward . tls://"+s.Addr)
-	f, err := parseForward(c)
+	fs, err := parseForward(c)
+	f := fs[0]
 	if err != nil {
 		t.Errorf("Failed to create forwarder: %s", err)
 	}
diff --git a/plugin/forward/setup.go b/plugin/forward/setup.go
index af8141898..0e317bf9f 100644
--- a/plugin/forward/setup.go
+++ b/plugin/forward/setup.go
@@ -19,34 +19,47 @@ import (
 func init() { plugin.Register("forward", setup) }
 
 func setup(c *caddy.Controller) error {
-	f, err := parseForward(c)
+	fs, err := parseForward(c)
 	if err != nil {
 		return plugin.Error("forward", err)
 	}
-	if f.Len() > max {
-		return plugin.Error("forward", fmt.Errorf("more than %d TOs configured: %d", max, f.Len()))
-	}
+	for i := range fs {
+		f := fs[i]
+		if f.Len() > max {
+			return plugin.Error("forward", fmt.Errorf("more than %d TOs configured: %d", max, f.Len()))
+		}
 
-	dnsserver.GetConfig(c).AddPlugin(func(next plugin.Handler) plugin.Handler {
-		f.Next = next
-		return f
-	})
+		if i == len(fs)-1 {
+			// last forward: point next to next plugin
+			dnsserver.GetConfig(c).AddPlugin(func(next plugin.Handler) plugin.Handler {
+				f.Next = next
+				return f
+			})
+		} else {
+			// middle forward: point next to next forward
+			nextForward := fs[i+1]
+			dnsserver.GetConfig(c).AddPlugin(func(plugin.Handler) plugin.Handler {
+				f.Next = nextForward
+				return f
+			})
+		}
 
-	c.OnStartup(func() error {
-		return f.OnStartup()
-	})
-	c.OnStartup(func() error {
-		if taph := dnsserver.GetConfig(c).Handler("dnstap"); taph != nil {
-			if tapPlugin, ok := taph.(dnstap.Dnstap); ok {
-				f.tapPlugin = &tapPlugin
+		c.OnStartup(func() error {
+			return f.OnStartup()
+		})
+		c.OnStartup(func() error {
+			if taph := dnsserver.GetConfig(c).Handler("dnstap"); taph != nil {
+				if tapPlugin, ok := taph.(dnstap.Dnstap); ok {
+					f.tapPlugin = &tapPlugin
+				}
 			}
-		}
-		return nil
-	})
+			return nil
+		})
 
-	c.OnShutdown(func() error {
-		return f.OnShutdown()
-	})
+		c.OnShutdown(func() error {
+			return f.OnShutdown()
+		})
+	}
 
 	return nil
 }
@@ -67,23 +80,16 @@ func (f *Forward) OnShutdown() error {
 	return nil
 }
 
-func parseForward(c *caddy.Controller) (*Forward, error) {
-	var (
-		f   *Forward
-		err error
-		i   int
-	)
+func parseForward(c *caddy.Controller) ([]*Forward, error) {
+	var fs = []*Forward{}
 	for c.Next() {
-		if i > 0 {
-			return nil, plugin.ErrOnce
-		}
-		i++
-		f, err = parseStanza(c)
+		f, err := parseStanza(c)
 		if err != nil {
 			return nil, err
 		}
+		fs = append(fs, f)
 	}
-	return f, nil
+	return fs, nil
 }
 
 func parseStanza(c *caddy.Controller) (*Forward, error) {
diff --git a/plugin/forward/setup_policy_test.go b/plugin/forward/setup_policy_test.go
index 2786f9a7a..13466d7a3 100644
--- a/plugin/forward/setup_policy_test.go
+++ b/plugin/forward/setup_policy_test.go
@@ -24,7 +24,7 @@ func TestSetupPolicy(t *testing.T) {
 
 	for i, test := range tests {
 		c := caddy.NewTestController("dns", test.input)
-		f, err := parseForward(c)
+		fs, err := parseForward(c)
 
 		if test.shouldErr && err == nil {
 			t.Errorf("Test %d: expected error but found %s for input %s", i, err, test.input)
@@ -40,8 +40,8 @@ func TestSetupPolicy(t *testing.T) {
 			}
 		}
 
-		if !test.shouldErr && f.p.String() != test.expectedPolicy {
-			t.Errorf("Test %d: expected: %s, got: %s", i, test.expectedPolicy, f.p.String())
+		if !test.shouldErr && (len(fs) == 0 || fs[0].p.String() != test.expectedPolicy) {
+			t.Errorf("Test %d: expected: %s, got: %s", i, test.expectedPolicy, fs[0].p.String())
 		}
 	}
 }
diff --git a/plugin/forward/setup_test.go b/plugin/forward/setup_test.go
index 2a5257e98..829e1f745 100644
--- a/plugin/forward/setup_test.go
+++ b/plugin/forward/setup_test.go
@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"github.com/coredns/caddy"
+	"github.com/coredns/coredns/core/dnsserver"
 )
 
 func TestSetup(t *testing.T) {
@@ -33,19 +34,19 @@ func TestSetup(t *testing.T) {
 		{"forward . [2003::1]:53", false, ".", nil, 2, options{hcRecursionDesired: true, hcDomain: "."}, ""},
 		{"forward . 127.0.0.1 \n", false, ".", nil, 2, options{hcRecursionDesired: true, hcDomain: "."}, ""},
 		{"forward 10.9.3.0/18 127.0.0.1", false, "0.9.10.in-addr.arpa.", nil, 2, options{hcRecursionDesired: true, hcDomain: "."}, ""},
+		{`forward . ::1
+		forward com ::2`, false, ".", nil, 2, options{hcRecursionDesired: true, hcDomain: "."}, "plugin"},
 		// negative
 		{"forward . a27.0.0.1", true, "", nil, 0, options{hcRecursionDesired: true, hcDomain: "."}, "not an IP"},
 		{"forward . 127.0.0.1 {\nblaatl\n}\n", true, "", nil, 0, options{hcRecursionDesired: true, hcDomain: "."}, "unknown property"},
 		{"forward . 127.0.0.1 {\nhealth_check 0.5s domain\n}\n", true, "", nil, 0, options{hcRecursionDesired: true, hcDomain: "."}, "Wrong argument count or unexpected line ending after 'domain'"},
-		{`forward . ::1
-		forward com ::2`, true, "", nil, 0, options{hcRecursionDesired: true, hcDomain: "."}, "plugin"},
 		{"forward . https://127.0.0.1 \n", true, ".", nil, 2, options{hcRecursionDesired: true, hcDomain: "."}, "'https' is not supported as a destination protocol in forward: https://127.0.0.1"},
 		{"forward xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx 127.0.0.1 \n", true, ".", nil, 2, options{hcRecursionDesired: true, hcDomain: "."}, "unable to normalize 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx'"},
 	}
 
 	for i, test := range tests {
 		c := caddy.NewTestController("dns", test.input)
-		f, err := parseForward(c)
+		fs, err := parseForward(c)
 
 		if test.shouldErr && err == nil {
 			t.Errorf("Test %d: expected error but found %s for input %s", i, err, test.input)
@@ -61,19 +62,22 @@ func TestSetup(t *testing.T) {
 			}
 		}
 
-		if !test.shouldErr && f.from != test.expectedFrom {
-			t.Errorf("Test %d: expected: %s, got: %s", i, test.expectedFrom, f.from)
-		}
-		if !test.shouldErr && test.expectedIgnored != nil {
-			if !reflect.DeepEqual(f.ignored, test.expectedIgnored) {
-				t.Errorf("Test %d: expected: %q, actual: %q", i, test.expectedIgnored, f.ignored)
+		if !test.shouldErr {
+			f := fs[0]
+			if f.from != test.expectedFrom {
+				t.Errorf("Test %d: expected: %s, got: %s", i, test.expectedFrom, f.from)
+			}
+			if test.expectedIgnored != nil {
+				if !reflect.DeepEqual(f.ignored, test.expectedIgnored) {
+					t.Errorf("Test %d: expected: %q, actual: %q", i, test.expectedIgnored, f.ignored)
+				}
+			}
+			if f.maxfails != test.expectedFails {
+				t.Errorf("Test %d: expected: %d, got: %d", i, test.expectedFails, f.maxfails)
+			}
+			if f.opts != test.expectedOpts {
+				t.Errorf("Test %d: expected: %v, got: %v", i, test.expectedOpts, f.opts)
 			}
-		}
-		if !test.shouldErr && f.maxfails != test.expectedFails {
-			t.Errorf("Test %d: expected: %d, got: %d", i, test.expectedFails, f.maxfails)
-		}
-		if !test.shouldErr && f.opts != test.expectedOpts {
-			t.Errorf("Test %d: expected: %v, got: %v", i, test.expectedOpts, f.opts)
 		}
 	}
 }
@@ -100,7 +104,8 @@ func TestSetupTLS(t *testing.T) {
 
 	for i, test := range tests {
 		c := caddy.NewTestController("dns", test.input)
-		f, err := parseForward(c)
+		fs, err := parseForward(c)
+		f := fs[0]
 
 		if test.shouldErr && err == nil {
 			t.Errorf("Test %d: expected error but found %s for input %s", i, err, test.input)
@@ -149,7 +154,7 @@ nameserver 10.10.255.253`), 0666); err != nil {
 
 	for i, test := range tests {
 		c := caddy.NewTestController("dns", test.input)
-		f, err := parseForward(c)
+		fs, err := parseForward(c)
 
 		if test.shouldErr && err == nil {
 			t.Errorf("Test %d: expected error but found %s for input %s", i, err, test.input)
@@ -166,17 +171,18 @@ nameserver 10.10.255.253`), 0666); err != nil {
 			}
 		}
 
-		if !test.shouldErr {
-			for j, n := range test.expectedNames {
-				addr := f.proxies[j].addr
-				if n != addr {
-					t.Errorf("Test %d, expected %q, got %q", j, n, addr)
-				}
-			}
-		}
 		if test.shouldErr {
 			continue
 		}
+
+		f := fs[0]
+		for j, n := range test.expectedNames {
+			addr := f.proxies[j].addr
+			if n != addr {
+				t.Errorf("Test %d, expected %q, got %q", j, n, addr)
+			}
+		}
+
 		for _, p := range f.proxies {
 			p.health.Check(p) // this should almost always err, we don't care it shouldn't crash
 		}
@@ -199,7 +205,7 @@ func TestSetupMaxConcurrent(t *testing.T) {
 
 	for i, test := range tests {
 		c := caddy.NewTestController("dns", test.input)
-		f, err := parseForward(c)
+		fs, err := parseForward(c)
 
 		if test.shouldErr && err == nil {
 			t.Errorf("Test %d: expected error but found %s for input %s", i, err, test.input)
@@ -215,7 +221,11 @@ func TestSetupMaxConcurrent(t *testing.T) {
 			}
 		}
 
-		if !test.shouldErr && f.maxConcurrent != test.expectedVal {
+		if test.shouldErr {
+			continue
+		}
+		f := fs[0]
+		if f.maxConcurrent != test.expectedVal {
 			t.Errorf("Test %d: expected: %d, got: %d", i, test.expectedVal, f.maxConcurrent)
 		}
 	}
@@ -244,7 +254,7 @@ func TestSetupHealthCheck(t *testing.T) {
 
 	for i, test := range tests {
 		c := caddy.NewTestController("dns", test.input)
-		f, err := parseForward(c)
+		fs, err := parseForward(c)
 
 		if test.shouldErr && err == nil {
 			t.Errorf("Test %d: expected error but found %s for input %s", i, err, test.input)
@@ -258,9 +268,62 @@ func TestSetupHealthCheck(t *testing.T) {
 				t.Errorf("Test %d: expected error to contain: %v, found error: %v, input: %s", i, test.expectedErr, err, test.input)
 			}
 		}
-		if !test.shouldErr && (f.opts.hcRecursionDesired != test.expectedRecVal || f.proxies[0].health.GetRecursionDesired() != test.expectedRecVal ||
-			f.opts.hcDomain != test.expectedDomain || f.proxies[0].health.GetDomain() != test.expectedDomain) {
+
+		if test.shouldErr {
+			continue
+		}
+
+		f := fs[0]
+		if f.opts.hcRecursionDesired != test.expectedRecVal || f.proxies[0].health.GetRecursionDesired() != test.expectedRecVal ||
+			f.opts.hcDomain != test.expectedDomain || f.proxies[0].health.GetDomain() != test.expectedDomain {
 			t.Errorf("Test %d: expectedRec: %v, got: %v. expectedDomain: %s, got: %s. ", i, test.expectedRecVal, f.opts.hcRecursionDesired, test.expectedDomain, f.opts.hcDomain)
 		}
 	}
 }
+
+func TestMultiForward(t *testing.T) {
+	input := `
+      forward 1st.example.org 10.0.0.1
+      forward 2nd.example.org 10.0.0.2
+      forward 3rd.example.org 10.0.0.3
+    `
+
+	c := caddy.NewTestController("dns", input)
+	setup(c)
+	dnsserver.NewServer("", []*dnsserver.Config{dnsserver.GetConfig(c)})
+
+	handlers := dnsserver.GetConfig(c).Handlers()
+	f1, ok := handlers[0].(*Forward)
+	if !ok {
+		t.Fatalf("expected first plugin to be Forward, got %v", reflect.TypeOf(f1.Next))
+	}
+
+	if f1.from != "1st.example.org." {
+		t.Errorf("expected first forward from \"1st.example.org.\", got %q", f1.from)
+	}
+	if f1.Next == nil {
+		t.Fatal("expected first forward to point to next forward instance, not nil")
+	}
+
+	f2, ok := f1.Next.(*Forward)
+	if !ok {
+		t.Fatalf("expected second plugin to be Forward, got %v", reflect.TypeOf(f1.Next))
+	}
+	if f2.from != "2nd.example.org." {
+		t.Errorf("expected second forward from \"2nd.example.org.\", got %q", f2.from)
+	}
+	if f2.Next == nil {
+		t.Fatal("expected second forward to point to third forward instance, got nil")
+	}
+
+	f3, ok := f2.Next.(*Forward)
+	if !ok {
+		t.Fatalf("expected third plugin to be Forward, got %v", reflect.TypeOf(f2.Next))
+	}
+	if f3.from != "3rd.example.org." {
+		t.Errorf("expected third forward from \"3rd.example.org.\", got %q", f3.from)
+	}
+	if f3.Next != nil {
+		t.Error("expected third plugin to be last, but Next is not nil")
+	}
+}