Allow overlapping Zones if binding addresses are different (#1530)

* add OverlapChecker, move the test of overlap AFTER the directive setup process, change key of configs to allow multiple same key

* glitch when rebase. init of Config should include the default host

* add tests for the registering of configuration
rename multicast in 'unbound'.
add comments on the validator

* - merged zoneAddr and addrKey that are very similar
- move maps of Validator to zoneAddr, avoinding need to have string representation of zoneaddr
- moving key build for saving Config at Config side instead of dnsContext

* - UT on saving config is now useless.

* - cannot cleanup access to Configs after setup. Deferred function to Start, use it

* - cleanup register unit tests. remove useless function

* - address comments of review. name of validator, comments, simplify registerAndCheck

* - fixes after review. renaming a function and a comment

6 files changed, 164 insertions, 19 deletions

diff --git a/core/dnsserver/address.go b/core/dnsserver/address.go
index 75f56ded9..39d656eff 100644
--- a/core/dnsserver/address.go
+++ b/core/dnsserver/address.go
@@ -15,10 +15,17 @@ type zoneAddr struct {
 	Port      string
 	Transport string     // dns, tls or grpc
 	IPNet     *net.IPNet // if reverse zone this hold the IPNet
+	Address   string     // used for bound zoneAddr - validation of overlapping
 }
 
 // String return the string representation of z.
-func (z zoneAddr) String() string { return z.Transport + "://" + z.Zone + ":" + z.Port }
+func (z zoneAddr) String() string {
+	s := z.Transport + "://" + z.Zone + ":" + z.Port
+	if z.Address != "" {
+		s += " on " + z.Address
+	}
+	return s
+}
 
 // Transport returns the protocol of the string s
 func Transport(s string) string {
@@ -94,3 +101,37 @@ const (
 	TransportTLS  = "tls"
 	TransportGRPC = "grpc"
 )
+
+type zoneOverlap struct {
+	registeredAddr map[zoneAddr]zoneAddr // each zoneAddr is registered once by its key
+	unboundOverlap map[zoneAddr]zoneAddr // the "no bind" equiv ZoneAdddr is registered by its original key
+}
+
+func newOverlapZone() *zoneOverlap {
+	return &zoneOverlap{registeredAddr: make(map[zoneAddr]zoneAddr), unboundOverlap: make(map[zoneAddr]zoneAddr)}
+}
+
+// registerAndCheck adds a new zoneAddr for validation, it returns information about existing or overlapping with already registered
+// we consider that an unbound address is overlapping all bound addresses for same zone, same port
+func (zo *zoneOverlap) registerAndCheck(z zoneAddr) (existingZone *zoneAddr, overlappingZone *zoneAddr) {
+
+	if exist, ok := zo.registeredAddr[z]; ok {
+		// exact same zone already registered
+		return &exist, nil
+	}
+	uz := zoneAddr{Zone: z.Zone, Address: "", Port: z.Port, Transport: z.Transport}
+	if already, ok := zo.unboundOverlap[uz]; ok {
+		if z.Address == "" {
+			// current is not bound to an address, but there is already another zone with a bind address registered
+			return nil, &already
+		}
+		if _, ok := zo.registeredAddr[uz]; ok {
+			// current zone is bound to an address, but there is already an overlapping zone+port with no bind address
+			return nil, &uz
+		}
+	}
+	// there is no overlap, keep the current zoneAddr for future checks
+	zo.registeredAddr[z] = z
+	zo.unboundOverlap[uz] = z
+	return nil, nil
+}
diff --git a/core/dnsserver/address_test.go b/core/dnsserver/address_test.go
index 33b8fd898..137bc8e44 100644
--- a/core/dnsserver/address_test.go
+++ b/core/dnsserver/address_test.go
@@ -108,3 +108,72 @@ func TestSplitProtocolHostPort(t *testing.T) {
 
 	}
 }
+
+type checkCall struct {
+	zone       zoneAddr
+	same       bool
+	overlap    bool
+	overlapKey string
+}
+
+type checkTest struct {
+	sequence []checkCall
+}
+
+func TestOverlapAddressChecker(t *testing.T) {
+	for i, test := range []checkTest{
+		{sequence: []checkCall{
+			{zoneAddr{Transport: "dns", Zone: ".", Address: "", Port: "53"}, false, false, ""},
+			{zoneAddr{Transport: "dns", Zone: ".", Address: "", Port: "53"}, true, false, ""},
+		},
+		},
+		{sequence: []checkCall{
+			{zoneAddr{Transport: "dns", Zone: ".", Address: "", Port: "53"}, false, false, ""},
+			{zoneAddr{Transport: "dns", Zone: ".", Address: "", Port: "54"}, false, false, ""},
+			{zoneAddr{Transport: "dns", Zone: ".", Address: "127.0.0.1", Port: "53"}, false, true, "dns://.:53"},
+		},
+		},
+		{sequence: []checkCall{
+			{zoneAddr{Transport: "dns", Zone: ".", Address: "127.0.0.1", Port: "53"}, false, false, ""},
+			{zoneAddr{Transport: "dns", Zone: ".", Address: "", Port: "54"}, false, false, ""},
+			{zoneAddr{Transport: "dns", Zone: ".", Address: "127.0.0.1", Port: "53"}, true, false, ""},
+		},
+		},
+		{sequence: []checkCall{
+			{zoneAddr{Transport: "dns", Zone: ".", Address: "127.0.0.1", Port: "53"}, false, false, ""},
+			{zoneAddr{Transport: "dns", Zone: ".", Address: "", Port: "54"}, false, false, ""},
+			{zoneAddr{Transport: "dns", Zone: ".", Address: "128.0.0.1", Port: "53"}, false, false, ""},
+			{zoneAddr{Transport: "dns", Zone: ".", Address: "129.0.0.1", Port: "53"}, false, false, ""},
+			{zoneAddr{Transport: "dns", Zone: ".", Address: "", Port: "53"}, false, true, "dns://.:53 on 129.0.0.1"},
+		},
+		},
+		{sequence: []checkCall{
+			{zoneAddr{Transport: "dns", Zone: ".", Address: "127.0.0.1", Port: "53"}, false, false, ""},
+			{zoneAddr{Transport: "dns", Zone: "com.", Address: "127.0.0.1", Port: "53"}, false, false, ""},
+			{zoneAddr{Transport: "dns", Zone: "com.", Address: "", Port: "53"}, false, true, "dns://com.:53 on 127.0.0.1"},
+		},
+		},
+	} {
+
+		checker := newOverlapZone()
+		for _, call := range test.sequence {
+			same, overlap := checker.registerAndCheck(call.zone)
+			sZone := call.zone.String()
+			if (same != nil) != call.same {
+				t.Errorf("Test %d: error, for zone %s, 'same' (%v) has not the expected value (%v)", i, sZone, same != nil, call.same)
+			}
+			if same == nil {
+				if (overlap != nil) != call.overlap {
+					t.Errorf("Test %d: error, for zone %s, 'overlap' (%v) has not the expected value (%v)", i, sZone, overlap != nil, call.overlap)
+				}
+				if overlap != nil {
+					if overlap.String() != call.overlapKey {
+						t.Errorf("Test %d: error, for zone %s, 'overlap Key' (%v) has not the expected value (%v)", i, sZone, overlap.String(), call.overlapKey)
+					}
+
+				}
+			}
+
+		}
+	}
+}
diff --git a/core/dnsserver/config.go b/core/dnsserver/config.go
index 1e952b153..d21a52380 100644
--- a/core/dnsserver/config.go
+++ b/core/dnsserver/config.go
@@ -2,6 +2,7 @@ package dnsserver
 
 import (
 	"crypto/tls"
+	"fmt"
 	"net"
 
 	"github.com/coredns/coredns/plugin"
@@ -68,16 +69,22 @@ func (c *Config) HostAddresses() string {
 	return all
 }
 
+// keyForConfig build a key for identifying the configs during setup time
+func keyForConfig(blocIndex int, blocKeyIndex int) string {
+	return fmt.Sprintf("%d:%d", blocIndex, blocKeyIndex)
+}
+
 // GetConfig gets the Config that corresponds to c.
 // If none exist nil is returned.
 func GetConfig(c *caddy.Controller) *Config {
 	ctx := c.Context().(*dnsContext)
-	if cfg, ok := ctx.keysToConfigs[c.Key]; ok {
+	key := keyForConfig(c.ServerBlockIndex, c.ServerBlockKeyIndex)
+	if cfg, ok := ctx.keysToConfigs[key]; ok {
 		return cfg
 	}
 	// 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{ListenHosts: []string{""}})
+	ctx.saveConfig(key, &Config{ListenHosts: []string{""}})
 	return GetConfig(c)
 }
diff --git a/core/dnsserver/register.go b/core/dnsserver/register.go
index 8be36cd22..779dc6b5f 100644
--- a/core/dnsserver/register.go
+++ b/core/dnsserver/register.go
@@ -55,28 +55,23 @@ func (h *dnsContext) saveConfig(key string, cfg *Config) {
 // be parsed and executed.
 func (h *dnsContext) InspectServerBlocks(sourceFile string, serverBlocks []caddyfile.ServerBlock) ([]caddyfile.ServerBlock, error) {
 	// Normalize and check all the zone names and check for duplicates
-	dups := map[string]string{}
-	for _, s := range serverBlocks {
-		for i, k := range s.Keys {
+	for ib, s := range serverBlocks {
+		for ik, k := range s.Keys {
 			za, err := normalizeZone(k)
 			if err != nil {
 				return nil, err
 			}
-			s.Keys[i] = za.String()
-			if v, ok := dups[za.String()]; ok {
-				return nil, fmt.Errorf("cannot serve %s - zone already defined for %v", za, v)
-			}
-			dups[za.String()] = za.String()
-
+			s.Keys[ik] = za.String()
 			// Save the config to our master list, and key it for lookups.
 			cfg := &Config{
 				Zone:        za.Zone,
+				ListenHosts: []string{""},
 				Port:        za.Port,
 				Transport:   za.Transport,
-				ListenHosts: []string{""},
 			}
+			keyConfig := keyForConfig(ib, ik)
 			if za.IPNet == nil {
-				h.saveConfig(za.String(), cfg)
+				h.saveConfig(keyConfig, cfg)
 				continue
 			}
 
@@ -91,7 +86,7 @@ func (h *dnsContext) InspectServerBlocks(sourceFile string, serverBlocks []caddy
 					return za.IPNet.Contains(net.ParseIP(addr))
 				}
 			}
-			h.saveConfig(za.String(), cfg)
+			h.saveConfig(keyConfig, cfg)
 		}
 	}
 	return serverBlocks, nil
@@ -100,6 +95,13 @@ func (h *dnsContext) InspectServerBlocks(sourceFile string, serverBlocks []caddy
 // MakeServers uses the newly-created siteConfigs to create and return a list of server instances.
 func (h *dnsContext) MakeServers() ([]caddy.Server, error) {
 
+	// Now that all Keys and Directives are parsed and initialized
+	// lets verify that there is no overlap on the zones and addresses to listen for
+	errValid := h.validateZonesAndListeningAddresses()
+	if errValid != nil {
+		return nil, errValid
+	}
+
 	// we must map (group) each config to a bind address
 	groups, err := groupConfigsByListenAddr(h.configs)
 	if err != nil {
@@ -183,14 +185,35 @@ func (c *Config) Handlers() []plugin.Handler {
 	return hs
 }
 
+func (h *dnsContext) validateZonesAndListeningAddresses() error {
+	//Validate Zone and addresses
+	checker := newOverlapZone()
+	for _, conf := range h.configs {
+		for _, h := range conf.ListenHosts {
+			// Validate the overlapping of ZoneAddr
+			akey := zoneAddr{Transport: conf.Transport, Zone: conf.Zone, Address: h, Port: conf.Port}
+			existZone, overlapZone := checker.registerAndCheck(akey)
+			if existZone != nil {
+				return fmt.Errorf("cannot serve %s - it is already defined", akey.String())
+			}
+			if overlapZone != nil {
+				return fmt.Errorf("cannot serve %s - zone overlap listener capacity with %v", akey.String(), overlapZone.String())
+			}
+
+		}
+	}
+	return nil
+
+}
+
 // groupSiteConfigsByListenAddr groups site configs by their listen
 // (bind) address, so sites that use the same listener can be served
 // on the same server instance. The return value maps the listen
 // address (what you pass into net.Listen) to the list of site configs.
 // This function does NOT vet the configs to ensure they are compatible.
 func groupConfigsByListenAddr(configs []*Config) (map[string][]*Config, error) {
-	groups := make(map[string][]*Config)
 
+	groups := make(map[string][]*Config)
 	for _, conf := range configs {
 		for _, h := range conf.ListenHosts {
 			addr, err := net.ResolveTCPAddr("tcp", net.JoinHostPort(h, conf.Port))
@@ -201,6 +224,7 @@ func groupConfigsByListenAddr(configs []*Config) (map[string][]*Config, error) {
 			groups[addrstr] = append(groups[addrstr], conf)
 		}
 	}
+
 	return groups, nil
 }
 
diff --git a/core/dnsserver/register_test.go b/core/dnsserver/register_test.go
index 626fdc710..1bf279a79 100644
--- a/core/dnsserver/register_test.go
+++ b/core/dnsserver/register_test.go
@@ -51,7 +51,7 @@ func TestGroupingServers(t *testing.T) {
 			expectedGroups: []string{"dns://:53", "dns://:54"},
 			failing:        false},
 
-		// 2 configs on same port, same broadcast address, diff zones -> 1 group
+		// 2 configs on same port, both not using bind, diff zones -> 1 group
 		{configs: []*Config{
 			{Transport: "dns", Zone: ".", Port: "53", ListenHosts: []string{""}},
 			{Transport: "dns", Zone: "com.", Port: "53", ListenHosts: []string{""}},
@@ -59,7 +59,7 @@ func TestGroupingServers(t *testing.T) {
 			expectedGroups: []string{"dns://:53"},
 			failing:        false},
 
-		// 2 configs on same port, same address, diff zones -> 1 group
+		// 2 configs on same port, one addressed - one not using bind, diff zones -> 1 group
 		{configs: []*Config{
 			{Transport: "dns", Zone: ".", Port: "53", ListenHosts: []string{"127.0.0.1"}},
 			{Transport: "dns", Zone: ".", Port: "54", ListenHosts: []string{""}},
@@ -74,7 +74,7 @@ func TestGroupingServers(t *testing.T) {
 			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
+		// 2 configs on same port, same 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"}},
diff --git a/core/dnsserver/server.go b/core/dnsserver/server.go
index f10bdb78b..07137494f 100644
--- a/core/dnsserver/server.go
+++ b/core/dnsserver/server.go
@@ -65,6 +65,10 @@ func NewServer(addr string, group []*Config) (*Server, error) {
 		// set the config per zone
 		s.zones[site.Zone] = site
 		// compile custom plugin for everything
+		if site.registry != nil {
+			// this config is already computed with the chain of plugin
+			continue
+		}
 		var stack plugin.Handler
 		for i := len(site.Plugin) - 1; i >= 0; i-- {
 			stack = site.Plugin[i](stack)