Plugin/BIND - extend the syntax to allow multiple addresses (#1512)

* Extend bind to allow multiple addresses. UTs added. Changes the log for server starting, adding address when available

* update readme for bind

* fixes after review

* minor fix on readme

* accept multiple BIND directives in blocserver, consolidate the addresses

* fixes after review - format logging server address, variable names

core/dnsserver/address.go

 package dnsserver
 
 import (
+	"fmt"
 	"net"
 	"strings"
 
@@ -72,6 +73,21 @@ func normalizeZone(str string) (zoneAddr, error) {
 	return zoneAddr{Zone: dns.Fqdn(host), Port: port, Transport: trans, IPNet: ipnet}, nil
 }
 
+// SplitProtocolHostPort - split a full formed address like "dns://[::1}:53" into parts
+func SplitProtocolHostPort(address string) (protocol string, ip string, port string, err error) {
+	parts := strings.Split(address, "://")
+	switch len(parts) {
+	case 1:
+		ip, port, err := net.SplitHostPort(parts[0])
+		return "", ip, port, err
+	case 2:
+		ip, port, err := net.SplitHostPort(parts[1])
+		return parts[0], ip, port, err
+	default:
+		return "", "", "", fmt.Errorf("provided value is not in an address format : %s", address)
+	}
+}
+
 // Supported transports.
 const (
 	TransportDNS  = "dns"

core/dnsserver/address_test.go

@@ -63,3 +63,48 @@ func TestNormalizeZoneReverse(t *testing.T) {
 		}
 	}
 }
+
+func TestSplitProtocolHostPort(t *testing.T) {
+	for i, test := range []struct {
+		input     string
+		proto     string
+		ip        string
+		port      string
+		shouldErr bool
+	}{
+		{"dns://:53", "dns", "", "53", false},
+		{"dns://127.0.0.1:4005", "dns", "127.0.0.1", "4005", false},
+		{"[ffe0:34ab:1]:4005", "", "ffe0:34ab:1", "4005", false},
+
+		// port part is mandatory
+		{"dns://", "dns", "", "", true},
+		{"dns://127.0.0.1", "dns", "127.0.0.1", "", true},
+		// cannot be empty
+		{"", "", "", "", true},
+		// invalid format with twice ://
+		{"dns://127.0.0.1://53", "", "", "", true},
+	} {
+		proto, ip, port, err := SplitProtocolHostPort(test.input)
+		if test.shouldErr && err == nil {
+			t.Errorf("Test %d: (address = %s) expected error, but there wasn't any", i, test.input)
+			continue
+		}
+		if !test.shouldErr && err != nil {
+			t.Errorf("Test %d: (address = %s) expected no error, but there was one: %v", i, test.input, err)
+			continue
+		}
+		if err == nil || test.shouldErr {
+			continue
+		}
+		if proto != test.proto {
+			t.Errorf("Test %d: (address = %s) expected protocol with value %s but got %s", i, test.input, test.proto, proto)
+		}
+		if ip != test.ip {
+			t.Errorf("Test %d: (address = %s) expected ip with value %s but got %s", i, test.input, test.ip, ip)
+		}
+		if port != test.port {
+			t.Errorf("Test %d: (address = %s) expected port with value %s but got %s", i, test.input, test.port, port)
+		}
+
+	}
+}

core/dnsserver/config.go

@@ -2,6 +2,7 @@ package dnsserver
 
 import (
 	"crypto/tls"
+	"net"
 
 	"github.com/coredns/coredns/plugin"
 
@@ -13,8 +14,9 @@ type Config struct {
 	// The zone of the site.
 	Zone string
 
-	// The hostname to bind listener to, defaults to the wildcard address
-	ListenHost string
+	// one or several hostnames to bind the server to.
+	// defaults to a single empty string that denote the wildcard address
+	ListenHosts []string
 
 	// The port to listen on.
 	Port string
@@ -50,6 +52,22 @@ type Config struct {
 	registry map[string]plugin.Handler
 }
 
+//HostAddresses builds a representation of the addresses of this Config
+//after server is started ONLY, can be used as a Key for identifing that config
+// :53 or 127.0.0.1:53 or 127.0.0.1:53/::1:53
+func (c *Config) HostAddresses() string {
+	all := ""
+	for _, h := range c.ListenHosts {
+		addr := net.JoinHostPort(h, c.Port)
+		if all == "" {
+			all = addr
+			continue
+		}
+		all = all + "/" + addr
+	}
+	return all
+}
+
 // GetConfig gets the Config that corresponds to c.
 // If none exist nil is returned.
 func GetConfig(c *caddy.Controller) *Config {
@@ -60,6 +78,6 @@ func GetConfig(c *caddy.Controller) *Config {
 	// we should only get here during tests because directive
 	// actions typically skip the server blocks where we make
 	// the configs.
-	ctx.saveConfig(c.Key, &Config{})
+	ctx.saveConfig(c.Key, &Config{ListenHosts: []string{""}})
 	return GetConfig(c)
 }

core/dnsserver/register.go

@@ -73,6 +73,7 @@ func (h *dnsContext) InspectServerBlocks(sourceFile string, serverBlocks []caddy
 				Zone:        za.Zone,
 				Port:        za.Port,
 				Transport:   za.Transport,
+				ListenHosts: []string{""},
 			}
 			if za.IPNet == nil {
 				h.saveConfig(za.String(), cfg)
@@ -191,14 +192,15 @@ func groupConfigsByListenAddr(configs []*Config) (map[string][]*Config, error) {
 	groups := make(map[string][]*Config)
 
 	for _, conf := range configs {
-		addr, err := net.ResolveTCPAddr("tcp", net.JoinHostPort(conf.ListenHost, conf.Port))
+		for _, h := range conf.ListenHosts {
+			addr, err := net.ResolveTCPAddr("tcp", net.JoinHostPort(h, conf.Port))
 			if err != nil {
 				return nil, err
 			}
 			addrstr := conf.Transport + "://" + addr.String()
 			groups[addrstr] = append(groups[addrstr], conf)
 		}
-
+	}
 	return groups, nil
 }
 

core/dnsserver/register_test.go

@@ -29,3 +29,93 @@ func TestHandlers(t *testing.T) {
 		t.Errorf("Expected [testPlugin] from Handlers, got %v", hs)
 	}
 }
+
+func TestGroupingServers(t *testing.T) {
+	for i, test := range []struct {
+		configs        []*Config
+		expectedGroups []string
+		failing        bool
+	}{
+		// single config -> one group
+		{configs: []*Config{
+			{Transport: "dns", Zone: ".", Port: "53", ListenHosts: []string{""}},
+		},
+			expectedGroups: []string{"dns://:53"},
+			failing:        false},
+
+		// 2 configs on different port -> 2 groups
+		{configs: []*Config{
+			{Transport: "dns", Zone: ".", Port: "53", ListenHosts: []string{""}},
+			{Transport: "dns", Zone: ".", Port: "54", ListenHosts: []string{""}},
+		},
+			expectedGroups: []string{"dns://:53", "dns://:54"},
+			failing:        false},
+
+		// 2 configs on same port, same broadcast address, diff zones -> 1 group
+		{configs: []*Config{
+			{Transport: "dns", Zone: ".", Port: "53", ListenHosts: []string{""}},
+			{Transport: "dns", Zone: "com.", Port: "53", ListenHosts: []string{""}},
+		},
+			expectedGroups: []string{"dns://:53"},
+			failing:        false},
+
+		// 2 configs on same port, same address, diff zones -> 1 group
+		{configs: []*Config{
+			{Transport: "dns", Zone: ".", Port: "53", ListenHosts: []string{"127.0.0.1"}},
+			{Transport: "dns", Zone: ".", Port: "54", ListenHosts: []string{""}},
+		},
+			expectedGroups: []string{"dns://127.0.0.1:53", "dns://:54"},
+			failing:        false},
+
+		// 2 configs on diff ports, 3 different address, diff zones -> 3 group
+		{configs: []*Config{
+			{Transport: "dns", Zone: ".", Port: "53", ListenHosts: []string{"127.0.0.1", "::1"}},
+			{Transport: "dns", Zone: ".", Port: "54", ListenHosts: []string{""}}},
+			expectedGroups: []string{"dns://127.0.0.1:53", "dns://[::1]:53", "dns://:54"},
+			failing:        false},
+
+		// 2 configs on same port, same unicast address, diff zones -> 1 group
+		{configs: []*Config{
+			{Transport: "dns", Zone: ".", Port: "53", ListenHosts: []string{"127.0.0.1", "::1"}},
+			{Transport: "dns", Zone: "com.", Port: "53", ListenHosts: []string{"127.0.0.1", "::1"}},
+		},
+			expectedGroups: []string{"dns://127.0.0.1:53", "dns://[::1]:53"},
+			failing:        false},
+
+		// 2 configs on same port, total 2 diff addresses, diff zones -> 2 groups
+		{configs: []*Config{
+			{Transport: "dns", Zone: ".", Port: "53", ListenHosts: []string{"127.0.0.1"}},
+			{Transport: "dns", Zone: "com.", Port: "53", ListenHosts: []string{"::1"}},
+		},
+			expectedGroups: []string{"dns://127.0.0.1:53", "dns://[::1]:53"},
+			failing:        false},
+
+		// 2 configs on same port, total 3 diff addresses, diff zones -> 3 groups
+		{configs: []*Config{
+			{Transport: "dns", Zone: ".", Port: "53", ListenHosts: []string{"127.0.0.1", "::1"}},
+			{Transport: "dns", Zone: "com.", Port: "53", ListenHosts: []string{""}}},
+			expectedGroups: []string{"dns://127.0.0.1:53", "dns://[::1]:53", "dns://:53"},
+			failing:        false},
+	} {
+		groups, err := groupConfigsByListenAddr(test.configs)
+		if err != nil {
+			if !test.failing {
+				t.Fatalf("test %d, expected no errors, but got: %v", i, err)
+			}
+			continue
+		}
+		if test.failing {
+			t.Fatalf("test %d, expected to failed but did not, returned values", i)
+		}
+		if len(groups) != len(test.expectedGroups) {
+			t.Errorf("test %d : expected the group's size to be %d, was %d", i, len(test.expectedGroups), len(groups))
+			continue
+		}
+		for _, v := range test.expectedGroups {
+			if _, ok := groups[v]; !ok {
+				t.Errorf("test %d : expected value %v to be in the group, was not", i, v)
+
+			}
+		}
+	}
+}

core/dnsserver/server.go

@@ -287,8 +287,22 @@ func (s *Server) OnStartupComplete() {
 		return
 	}
 
-	for zone, config := range s.zones {
-		fmt.Println(zone + ":" + config.Port)
+	for zone := range s.zones {
+		// split addr into protocol, IP and Port
+		_, ip, port, err := SplitProtocolHostPort(s.Addr)
+
+		if err != nil {
+			// this should not happen, but we need to take care of it anyway
+			fmt.Println(zone + ":" + s.Addr)
+			return
+		}
+		if ip == "" {
+			fmt.Println(zone + ":" + port)
+			return
+		}
+		// if the server is listening on a specific address let's make it visible in the log,
+		// so one can differentiate between all active listeners
+		fmt.Println(zone + ":" + port + " on " + ip)
 	}
 }
 

core/dnsserver/server_test.go

@@ -22,7 +22,7 @@ func testConfig(transport string, p plugin.Handler) *Config {
 	c := &Config{
 		Zone:        "example.com.",
 		Transport:   transport,
-		ListenHost: "127.0.0.1",
+		ListenHosts: []string{"127.0.0.1"},
 		Port:        "53",
 		Debug:       false,
 	}

plugin/bind/README.md

@@ -6,23 +6,46 @@
 
 ## Description
 
-Normally, the listener binds to the wildcard host. However, you may force the listener to bind to
-another IP instead. This directive accepts only an address, not a port.
+Normally, the listener binds to the wildcard host. However, you may want the listener to bind to
+another IP instead.      
+
+If several addresses are provided, a listener will be open on each of the IP provided.
+
+Each address has to be an IP of one of the interfaces of the host.
 
 ## Syntax
 
 ~~~ txt
-bind ADDRESS
+bind ADDRESS  ...
 ~~~
 
-**ADDRESS** is the IP address to bind to.
+**ADDRESS** is an IP address to bind to.
+When several addresses are provided a listener will be opened on each of the addresses.
 
 ## Examples
 
 To make your socket accessible only to that machine, bind to IP 127.0.0.1 (localhost):
 
+~~~ corefile
+. {
+    bind 127.0.0.1
+}
+~~~
+
+To allow processing DNS requests only local host on both IPv4 and IPv6 stacks, use the syntax:
+
+~~~ corefile
+. {
+    bind 127.0.0.1 ::1
+}
 ~~~
+
+If the configuration comes up with several *bind* directives, all addresses are consolidated together:
+The following sample is equivalent to the preceding:
+ 
+~~~ corefile
 . {
     bind 127.0.0.1 
+    bind ::1
 }
 ~~~

plugin/bind/bind_test.go

@@ -9,22 +9,38 @@ import (
 )
 
 func TestSetupBind(t *testing.T) {
-	c := caddy.NewTestController("dns", `bind 1.2.3.4`)
+	for i, test := range []struct {
+		config   string
+		expected []string
+		failing  bool
+	}{
+		{`bind 1.2.3.4`, []string{"1.2.3.4"}, false},
+		{`bind`, nil, true},
+		{`bind 1.2.3.invalid`, nil, true},
+		{`bind 1.2.3.4 ::5`, []string{"1.2.3.4", "::5"}, false},
+		{`bind ::1 1.2.3.4 ::5 127.9.9.0`, []string{"::1", "1.2.3.4", "::5", "127.9.9.0"}, false},
+		{`bind ::1 1.2.3.4 ::5 127.9.9.0 noone`, nil, true},
+	} {
+		c := caddy.NewTestController("dns", test.config)
 		err := setupBind(c)
 		if err != nil {
-		t.Fatalf("Expected no errors, but got: %v", err)
+			if !test.failing {
+				t.Fatalf("test %d, expected no errors, but got: %v", i, err)
+			}
+			continue
+		}
+		if test.failing {
+			t.Fatalf("test %d, expected to failed but did not, returned values", i)
 		}
-
 		cfg := dnsserver.GetConfig(c)
-	if got, want := cfg.ListenHost, "1.2.3.4"; got != want {
-		t.Errorf("Expected the config's ListenHost to be %s, was %s", want, got)
+		if len(cfg.ListenHosts) != len(test.expected) {
+			t.Errorf("test %d : expected the config's ListenHosts size to be %d, was %d", i, len(test.expected), len(cfg.ListenHosts))
+			continue
+		}
+		for i, v := range test.expected {
+			if got, want := cfg.ListenHosts[i], v; got != want {
+				t.Errorf("test %d : expected the config's ListenHost to be %s, was %s", i, want, got)
+			}
 		}
-}
-
-func TestBindAddress(t *testing.T) {
-	c := caddy.NewTestController("dns", `bind 1.2.3.bla`)
-	err := setupBind(c)
-	if err == nil {
-		t.Fatalf("Expected errors, but got none")
 	}
 }

plugin/bind/setup.go

@@ -12,13 +12,21 @@ import (
 
 func setupBind(c *caddy.Controller) error {
 	config := dnsserver.GetConfig(c)
+
+	// addresses will be consolidated over all BIND directives available in that BlocServer
+	all := []string{}
 	for c.Next() {
-		if !c.Args(&config.ListenHost) {
-			return plugin.Error("bind", c.ArgErr())
+		addrs := c.RemainingArgs()
+		if len(addrs) == 0 {
+			return plugin.Error("bind", fmt.Errorf("at least one address is expected"))
+		}
+		for _, addr := range addrs {
+			if net.ParseIP(addr) == nil {
+				return plugin.Error("bind", fmt.Errorf("not a valid IP address: %s", addr))
 			}
 		}
-	if net.ParseIP(config.ListenHost) == nil {
-		return plugin.Error("bind", fmt.Errorf("not a valid IP address: %s", config.ListenHost))
+		all = append(all, addrs...)
 	}
+	config.ListenHosts = all
 	return nil
 }

plugin/trace/setup.go

@@ -41,7 +41,7 @@ func traceParse(c *caddy.Controller) (*trace, error) {
 	)
 
 	cfg := dnsserver.GetConfig(c)
-	tr.ServiceEndpoint = cfg.ListenHost + ":" + cfg.Port
+	tr.ServiceEndpoint = cfg.HostAddresses()
 	for c.Next() { // trace
 		var err error
 		args := c.RemainingArgs()