return an error for multiple use of some plugins (#1559)

* plugins: Return error for multiple use of some

Return plugin.ErrOnce when a plugin that doesn't support it, is called
mutliple times.

This now adds it for: cache, dnssec, errors, forward, hosts, nsid.
And changes it slightly in kubernetes, pprof, reload, root.

* more tests

plugin/cache/README.md

@@ -10,6 +10,8 @@ With *cache* enabled, all records except zone transfers and metadata records wil
 3600s. Caching is mostly useful in a scenario when fetching data from the backend (upstream,
 database, etc.) is expensive.
 
+This plugin can only be used once per Server Block.
+
 ## Syntax
 
 ~~~ txt

plugin/cache/setup.go

@@ -61,7 +61,13 @@ func setup(c *caddy.Controller) error {
 func cacheParse(c *caddy.Controller) (*Cache, error) {
 	ca := New()
 
+	j := 0
 	for c.Next() {
+		if j > 0 {
+			return nil, plugin.ErrOnce
+		}
+		j++
+
 		// cache [ttl] [zones..]
 		origins := make([]string, len(c.ServerBlockKeys))
 		copy(origins, c.ServerBlockKeys)
@@ -180,9 +186,7 @@ func cacheParse(c *caddy.Controller) (*Cache, error) {
 
 		ca.pcache = cache.New(ca.pcap)
 		ca.ncache = cache.New(ca.ncap)
-
-		return ca, nil
 	}
 
-	return nil, nil
+	return ca, nil
 }

plugin/cache/setup_test.go

@@ -20,46 +20,48 @@ func TestSetup(t *testing.T) {
 		{`cache`, false, defaultCap, defaultCap, maxNTTL, maxTTL, 0},
 		{`cache {}`, false, defaultCap, defaultCap, maxNTTL, maxTTL, 0},
 		{`cache example.nl {
-			success 10
-		}`, false, defaultCap, 10, maxNTTL, maxTTL, 0},
+				success 10
+			}`, false, defaultCap, 10, maxNTTL, maxTTL, 0},
 		{`cache example.nl {
-			success 10
-			denial 10 15
-		}`, false, 10, 10, 15 * time.Second, maxTTL, 0},
+				success 10
+				denial 10 15
+			}`, false, 10, 10, 15 * time.Second, maxTTL, 0},
 		{`cache 25 example.nl {
-			success 10
-			denial 10 15
-		}`, false, 10, 10, 15 * time.Second, 25 * time.Second, 0},
+				success 10
+				denial 10 15
+			}`, false, 10, 10, 15 * time.Second, 25 * time.Second, 0},
 		{`cache aaa example.nl`, false, defaultCap, defaultCap, maxNTTL, maxTTL, 0},
 		{`cache	{
-			prefetch 10
-		}`, false, defaultCap, defaultCap, maxNTTL, maxTTL, 10},
+				prefetch 10
+			}`, false, defaultCap, defaultCap, maxNTTL, maxTTL, 10},
 
 		// fails
 		{`cache example.nl {
-			success
-			denial 10 15
-		}`, true, defaultCap, defaultCap, maxTTL, maxTTL, 0},
+				success
+				denial 10 15
+			}`, true, defaultCap, defaultCap, maxTTL, maxTTL, 0},
 		{`cache example.nl {
-			success 15
-			denial aaa
-		}`, true, defaultCap, defaultCap, maxTTL, maxTTL, 0},
+				success 15
+				denial aaa
+			}`, true, defaultCap, defaultCap, maxTTL, maxTTL, 0},
 		{`cache example.nl {
-			positive 15
-			negative aaa
-		}`, true, defaultCap, defaultCap, maxTTL, maxTTL, 0},
+				positive 15
+				negative aaa
+			}`, true, defaultCap, defaultCap, maxTTL, maxTTL, 0},
 		{`cache 0 example.nl`, true, defaultCap, defaultCap, maxTTL, maxTTL, 0},
 		{`cache -1 example.nl`, true, defaultCap, defaultCap, maxTTL, maxTTL, 0},
 		{`cache 1 example.nl {
-			positive 0
-		}`, true, defaultCap, defaultCap, maxTTL, maxTTL, 0},
+				positive 0
+			}`, true, defaultCap, defaultCap, maxTTL, maxTTL, 0},
 		{`cache 1 example.nl {
-			positive 0
-			prefetch -1
-		}`, true, defaultCap, defaultCap, maxTTL, maxTTL, 0},
+				positive 0
+				prefetch -1
+			}`, true, defaultCap, defaultCap, maxTTL, maxTTL, 0},
 		{`cache 1 example.nl {
-			prefetch 0 blurp
-		}`, true, defaultCap, defaultCap, maxTTL, maxTTL, 0},
+				prefetch 0 blurp
+			}`, true, defaultCap, defaultCap, maxTTL, maxTTL, 0},
+		{`cache
+		  cache`, true, defaultCap, defaultCap, maxTTL, maxTTL, 0},
 	}
 	for i, test := range tests {
 		c := caddy.NewTestController("dns", test.input)

plugin/dnssec/README.md

@@ -10,6 +10,8 @@ With *dnssec* any reply that doesn't (or can't) do DNSSEC will get signed on the
 denial of existence is implemented with NSEC black lies. Using ECDSA as an algorithm is preferred as
 this leads to smaller signatures (compared to RSA). NSEC3 is *not* supported.
 
+This plugin can only be used once per Server Block.
+
 ## Syntax
 
 ~~~
@@ -74,20 +76,3 @@ cluster.local {
     }
 }
 ~~~
-
-## Bugs
-
-Multiple *dnssec* plugins inside one server stanza will silently overwrite earlier ones, here
-`example.org` will overwrite the one for `cluster.local`.
-
-~~~
-. {
-    kubernetes cluster.local
-    dnssec cluster.local {
-      key file Kcluster.local+013+45129
-    }
-    dnssec example.org {
-      key file Kexample.org.+013+45330
-    }
-}
-~~~

plugin/dnssec/setup.go

@@ -59,7 +59,14 @@ func dnssecParse(c *caddy.Controller) ([]string, []*DNSKEY, int, error) {
 	keys := []*DNSKEY{}
 
 	capacity := defaultCap
+
+	i := 0
 	for c.Next() {
+		if i > 0 {
+			return nil, nil, 0, plugin.ErrOnce
+		}
+		i++
+
 		// dnssec [zones...]
 		zones = make([]string, len(c.ServerBlockKeys))
 		copy(zones, c.ServerBlockKeys)
@@ -69,7 +76,8 @@ func dnssecParse(c *caddy.Controller) ([]string, []*DNSKEY, int, error) {
 		}
 
 		for c.NextBlock() {
-			switch c.Val() {
+
+			switch x := c.Val(); x {
 			case "key":
 				k, e := keyParse(c)
 				if e != nil {
@@ -86,6 +94,8 @@ func dnssecParse(c *caddy.Controller) ([]string, []*DNSKEY, int, error) {
 					return nil, nil, 0, err
 				}
 				capacity = cacheCap
+			default:
+				return nil, nil, 0, c.Errf("unknown property '%s'", x)
 			}
 
 		}

plugin/dnssec/setup_test.go

@@ -61,6 +61,8 @@ func TestSetupDnssec(t *testing.T) {
 				key file
 			}`, true, []string{"example.org."}, nil, defaultCap, "argument count",
 		},
+		{`dnssec
+		  dnssec`, true, nil, nil, defaultCap, ""},
 	}
 
 	for i, test := range tests {

plugin/errors/README.md

@@ -8,6 +8,8 @@
 
 Any errors encountered during the query processing will be printed to standard output.
 
+This plugin can only be used once per Server Block.
+
 ## Syntax
 
 ~~~

plugin/errors/setup.go

@@ -37,7 +37,13 @@ func setup(c *caddy.Controller) error {
 func errorsParse(c *caddy.Controller) (errorHandler, error) {
 	handler := errorHandler{}
 
+	i := 0
 	for c.Next() {
+		if i > 0 {
+			return handler, plugin.ErrOnce
+		}
+		i++
+
 		args := c.RemainingArgs()
 		switch len(args) {
 		case 0:

plugin/errors/setup_test.go

@@ -12,21 +12,14 @@ func TestErrorsParse(t *testing.T) {
 		shouldErr            bool
 		expectedErrorHandler errorHandler
 	}{
-		{`errors`, false, errorHandler{
-			LogFile: "stdout",
-		}},
-		{`errors stdout`, false, errorHandler{
-			LogFile: "stdout",
-		}},
-		{`errors errors.txt`, true, errorHandler{
-			LogFile: "",
-		}},
-		{`errors visible`, true, errorHandler{
-			LogFile: "",
-		}},
-		{`errors { log visible }`, true, errorHandler{
-			LogFile: "stdout",
-		}},
+		{`errors`, false, errorHandler{LogFile: "stdout"}},
+		{`errors stdout`, false, errorHandler{LogFile: "stdout"}},
+		{`errors errors.txt`, true, errorHandler{LogFile: ""}},
+		{`errors visible`, true, errorHandler{LogFile: ""}},
+		{`errors { log visible }`, true, errorHandler{LogFile: "stdout"}},
+		{`errors
+		errors `, true, errorHandler{LogFile: "stdout"}},
+		{`errors a b`, true, errorHandler{LogFile: ""}},
 	}
 	for i, test := range tests {
 		c := caddy.NewTestController("dns", test.inputErrorsRules)

plugin/forward/README.md

@@ -19,6 +19,8 @@ is performed and upstreams will always be considered healthy.
 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:

plugin/forward/setup.go

@@ -84,7 +84,13 @@ func parseForward(c *caddy.Controller) (*Forward, error) {
 
 	protocols := map[int]int{}
 
+	i := 0
 	for c.Next() {
+		if i > 0 {
+			return nil, plugin.ErrOnce
+		}
+		i++
+
 		if !c.Args(&f.from) {
 			return f, c.ArgErr()
 		}

plugin/forward/setup_test.go

@@ -30,6 +30,8 @@ func TestSetup(t *testing.T) {
 		// negative
 		{"forward . a27.0.0.1", true, "", nil, 0, false, "not an IP"},
 		{"forward . 127.0.0.1 {\nblaatl\n}\n", true, "", nil, 0, false, "unknown property"},
+		{`forward . ::1
+		forward com ::2`, true, "", nil, 0, false, "plugin"},
 	}
 
 	for i, test := range tests {

plugin/hosts/README.md

@@ -11,6 +11,8 @@ file that exists on disk. It checks the file for changes and updates the zones a
 plugin only supports A, AAAA, and PTR records. The hosts plugin can be used with readily
 available hosts files that block access to advertising servers.
 
+This plugin can only be used once per Server Block.
+
 ## Syntax
 
 ~~~

plugin/hosts/setup.go

@@ -69,7 +69,13 @@ func hostsParse(c *caddy.Controller) (Hosts, error) {
 	config := dnsserver.GetConfig(c)
 
 	inline := []string{}
+	i := 0
 	for c.Next() {
+		if i > 0 {
+			return h, plugin.ErrOnce
+		}
+		i++
+
 		args := c.RemainingArgs()
 		if len(args) >= 1 {
 			h.path = args[0]

plugin/hosts/setup_test.go

@@ -57,6 +57,15 @@ func TestHostsParse(t *testing.T) {
 			}`,
 			false, "/etc/hosts", []string{"miek.nl.", "10.in-addr.arpa."}, fall.Root,
 		},
+		{
+			`hosts /etc/hosts {
+				fallthrough
+			}
+			hosts /etc/hosts {
+				fallthrough
+			}`,
+			true, "/etc/hosts", nil, fall.Root,
+		},
 	}
 
 	for i, test := range tests {

plugin/kubernetes/README.md

@@ -16,6 +16,8 @@ to deploy CoreDNS in Kubernetes](https://github.com/coredns/deployment/tree/mast
 [stubDomains and upstreamNameservers](http://blog.kubernetes.io/2017/04/configuring-private-dns-zones-upstream-nameservers-kubernetes.html)
 are implemented via the *proxy* plugin and kubernetes *upstream*. See example below.
 
+This plugin can only be used once per Server Block.
+
 ## Syntax
 
 ~~~

plugin/kubernetes/setup.go

@@ -71,12 +71,18 @@ func (k *Kubernetes) RegisterKubeCache(c *caddy.Controller) {
 }
 
 func kubernetesParse(c *caddy.Controller) (*Kubernetes, error) {
-	var k8s *Kubernetes
-	var err error
-	for i := 1; c.Next(); i++ {
-		if i > 1 {
-			return nil, fmt.Errorf("only one kubernetes section allowed per server block")
+	var (
+		k8s *Kubernetes
+		err error
+	)
+
+	i := 0
+	for c.Next() {
+		if i > 0 {
+			return nil, plugin.ErrOnce
 		}
+		i++
+
 		k8s, err = ParseStanza(c)
 		if err != nil {
 			return k8s, err

plugin/kubernetes/setup_test.go

@@ -388,7 +388,7 @@ func TestKubernetesParse(t *testing.T) {
 			`kubernetes coredns.local
 kubernetes cluster.local`,
 			true,
-			"only one kubernetes section allowed per server block",
+			"this plugin",
 			-1,
 			0,
 			defaultResyncPeriod,

plugin/nsid/README.md

@@ -6,9 +6,12 @@
 
 ## Description
 
-This plugin implements RFC 5001 and adds an EDNS0 OPT resource record to replies that uniquely
-identify the server. This is useful in anycast setups to see which server was responsible for
-generating the reply and for debugging.
+This plugin implements [RFC 5001](https://tools.ietf.org/html/rfc5001) and adds an EDNS0 OPT
+resource record to replies that uniquely identify the server. This is useful in anycast setups to
+see which server was responsible for generating the reply and for debugging.
+
+This plugin can only be used once per Server Block.
+
 
 ## Syntax
 
@@ -48,3 +51,7 @@ And now a client with NSID support will see an OPT record with the NSID option:
 ;; QUESTION SECTION:
 ;whoami.example.org.		IN	A
 ~~~
+
+## Also See
+
+[RFC 5001](https://tools.ietf.org/html/rfc5001)

plugin/nsid/setup.go

@@ -36,13 +36,16 @@ func nsidParse(c *caddy.Controller) (string, error) {
 	if err != nil {
 		nsid = "localhost"
 	}
+	i := 0
 	for c.Next() {
+		if i > 0 {
+			return nsid, plugin.ErrOnce
+		}
+		i++
 		args := c.RemainingArgs()
-		if len(args) == 0 {
-			return nsid, nil
+		if len(args) > 0 {
+			nsid = strings.Join(args, " ")
 		}
-		nsid = strings.Join(args, " ")
-		return nsid, nil
 	}
 	return nsid, nil
 }

plugin/nsid/setup_test.go

@@ -19,18 +19,12 @@ func TestSetupNsid(t *testing.T) {
 		expectedData       string
 		expectedErrContent string // substring from the expected error. Empty for positive cases.
 	}{
-		{
-			`nsid`, false, defaultNsid, "",
-		},
-		{
-			`nsid "ps0"`, false, "ps0", "",
-		},
-		{
-			`nsid "worker1"`, false, "worker1", "",
-		},
-		{
-			`nsid "tf 2"`, false, "tf 2", "",
-		},
+		{`nsid`, false, defaultNsid, ""},
+		{`nsid "ps0"`, false, "ps0", ""},
+		{`nsid "worker1"`, false, "worker1", ""},
+		{`nsid "tf 2"`, false, "tf 2", ""},
+		{`nsid
+		nsid`, true, "", "plugin"},
 	}
 
 	for i, test := range tests {

plugin/plugin.go

@@ -104,3 +104,6 @@ const Namespace = "coredns"
 
 // TimeBuckets is based on Prometheus client_golang prometheus.DefBuckets
 var TimeBuckets = prometheus.ExponentialBuckets(0.00025, 2, 16) // from 0.25ms to 8 seconds
+
+// ErrOnce is returned when a plugin doesn't support multiple setups per server.
+var ErrOnce = errors.New("this plugin can only be used once per Server Block")

plugin/pprof/README.md

@@ -16,6 +16,8 @@ For more information, please see [Go's pprof
 documentation](https://golang.org/pkg/net/http/pprof/) and read
 [Profiling Go Programs](https://blog.golang.org/profiling-go-programs).
 
+This plugin can only be used once per Server Block.
+
 ## Syntax
 
 ~~~

plugin/pprof/setup.go

@@ -19,12 +19,15 @@ func init() {
 }
 
 func setup(c *caddy.Controller) error {
-	found := false
 	h := &handler{addr: defaultAddr}
+
+	i := 0
 	for c.Next() {
-		if found {
-			return plugin.Error("pprof", c.Err("pprof can only be specified once"))
+		if i > 0 {
+			return plugin.Error("pprof", plugin.ErrOnce)
 		}
+		i++
+
 		args := c.RemainingArgs()
 		if len(args) == 1 {
 			h.addr = args[0]
@@ -39,7 +42,6 @@ func setup(c *caddy.Controller) error {
 		if c.NextBlock() {
 			return plugin.Error("pprof", c.ArgErr())
 		}
-		found = true
 	}
 
 	pprofOnce.Do(func() {

plugin/reload/README.md

@@ -26,6 +26,8 @@ reloads are graceful, and can be disabled by setting the jitter to `0s`.
 
 Jitter is re-calculated whenever the Corefile is reloaded.
 
+This plugin can only be used once per Server Block.
+
 ## Syntax
 
 ~~~ txt

plugin/root/README.md

@@ -9,6 +9,8 @@
 The default root is the current working directory of CoreDNS. The *root* plugin allows you to change
 this. A relative root path is relative to the current working directory.
 
+This plugin can only be used once per Server Block.
+
 ## Syntax
 
 ~~~ txt

plugin/route53/README.md

@@ -6,9 +6,8 @@
 
 ## Description
 
-The route53 plugin is useful for serving zones from resource record sets in AWS route53.
-This plugin only supports A and AAAA records. The route53 plugin can be used when
-coredns is deployed on AWS.
+The route53 plugin is useful for serving zones from resource record sets in AWS route53. This plugin
+only supports A and AAAA records. The route53 plugin can be used when coredns is deployed on AWS.
 
 ## Syntax