config: Extend getListenAddress for ip%zone

This removes the `interface` configuration statement and makes coredhcp
accept a wider range of specifications in the `listen` configuration
statement, notably:

 * Omitting the statement entirely yields the default behaviour:
   listening on `[::]:547`(and joining `ff02::1:2` and `ff05::1:3`) or
   `0.0.0.0:67`
 * Omitting the `:port` is accepted and means the default port for the
   protocol version. Conversely specifying `:port` means `[::]:port` or
   `0.0.0.0:port`
 * `ip%iface` and `ip%iface:port` are accepted, and result in binding to the
   specified interface. This is the general replacement for the
   interface statement
 * `%iface` and `%iface:port` are also accepted and mean the wildcard
   address, for a direct replacement of having interface specified but
   listen empty

Signed-off-by: Anatole Denis <anatole@unverle.fr>

config/config.go

@@ -111,59 +111,77 @@ func parsePlugins(pluginList []interface{}) ([]*PluginConfig, error) {
 	return plugins, nil
 }
 
-func (c *Config) getInterface(ver protocolVersion) (string, error) {
-	if err := protoVersionCheck(ver); err != nil {
-		return "", err
-	}
-	directive := fmt.Sprintf("server%d.interface", ver)
-	if exists := c.v.Get(directive); exists == nil {
-		return "", ConfigErrorFromString("dhcpv%d: missing `%s` directive", ver, directive)
+// BUG(Natolumin): listen specifications of the form `[ip6]%iface:port` or
+// `[ip6]%iface` are not supported, even though they are the default format of
+// the `ss` utility in linux. Use `[ip6%iface]:port` instead
+
+// splitHostPort splits an address of the form ip%zone:port into ip,zone and port.
+// It still returns if any of these are unset (unlike net.SplitHostPort which
+// returns an error if there is no port)
+func splitHostPort(hostport string) (ip string, zone string, port string, err error) {
+	ip, port, err = net.SplitHostPort(hostport)
+	if err != nil {
+		// Either there is no port, or a more serious error.
+		// Supply a synthetic port to differentiate cases
+		var altErr error
+		if ip, _, altErr = net.SplitHostPort(hostport + ":0"); altErr != nil {
+			// Invalid even with a fake port. Return the original error
+			return
+		}
+		err = nil
 	}
-	ifname := c.v.GetString(directive)
-	if ifname == "" {
-		return "", ConfigErrorFromString("dhcpv%d: missing `%s` directive", ver, directive)
+	if i := strings.LastIndexByte(ip, '%'); i >= 0 {
+		ip, zone = ip[:i], ip[i+1:]
 	}
-	return ifname, nil
+	return
 }
 
 func (c *Config) getListenAddress(ver protocolVersion) (*net.UDPAddr, error) {
 	if err := protoVersionCheck(ver); err != nil {
 		return nil, err
 	}
+
 	addr := c.v.GetString(fmt.Sprintf("server%d.listen", ver))
-	if addr == "" {
-		// return default listener
-		if ver == protocolV6 {
-			return &net.UDPAddr{
-				IP:   net.IPv6unspecified,
-				Port: dhcpv6.DefaultServerPort,
-			}, nil
-		}
-		return &net.UDPAddr{
-			IP:   net.IPv4zero,
-			Port: dhcpv4.ServerPort,
-		}, nil
-	}
-	ipStr, portStr, err := net.SplitHostPort(addr)
+	ipStr, ifname, portStr, err := splitHostPort(addr)
 	if err != nil {
 		return nil, ConfigErrorFromString("dhcpv%d: %v", ver, err)
 	}
+
 	ip := net.ParseIP(ipStr)
+	if ipStr == "" {
+		switch ver {
+		case protocolV4:
+			ip = net.IPv4zero
+		case protocolV6:
+			ip = net.IPv6unspecified
+		}
+	}
 	if ip == nil {
-		return nil, ConfigErrorFromString("dhcpv%d: invalid IP address in `listen` directive", ver)
+		return nil, ConfigErrorFromString("dhcpv%d: invalid IP address in `listen` directive: %s", ver, ipStr)
 	}
-	if ver == protocolV6 && ip.To4() != nil {
-		return nil, ConfigErrorFromString("dhcpv%d: not a valid IPv6 address in `listen` directive", ver)
-	} else if ver == protocolV4 && ip.To4() == nil {
-		return nil, ConfigErrorFromString("dhcpv%d: not a valid IPv4 address in `listen` directive", ver)
+	if ip4 := ip.To4(); (ver == protocolV6 && ip4 != nil) || (ver == protocolV4 && ip4 == nil) {
+		return nil, ConfigErrorFromString("dhcpv%d: not a valid IPv%d address in `listen` directive", ver, ver)
 	}
-	port, err := strconv.Atoi(portStr)
-	if err != nil {
-		return nil, ConfigErrorFromString("dhcpv%d: invalid `listen` port", ver)
+
+	var port int
+	if portStr == "" {
+		switch ver {
+		case protocolV4:
+			port = dhcpv4.ServerPort
+		case protocolV6:
+			port = dhcpv6.DefaultServerPort
+		}
+	} else {
+		port, err = strconv.Atoi(portStr)
+		if err != nil {
+			return nil, ConfigErrorFromString("dhcpv%d: invalid `listen` port", ver)
+		}
 	}
+
 	listener := net.UDPAddr{
 		IP:   ip,
 		Port: port,
+		Zone: ifname,
 	}
 	return &listener, nil
 }
@@ -187,10 +205,6 @@ func (c *Config) parseConfig(ver protocolVersion) error {
 		// it is valid to have no server configuration defined
 		return nil
 	}
-	ifname, err := c.getInterface(ver)
-	if err != nil {
-		return err
-	}
 	listenAddr, err := c.getListenAddress(ver)
 	if err != nil {
 		return err
@@ -198,6 +212,7 @@ func (c *Config) parseConfig(ver protocolVersion) error {
 	if listenAddr == nil {
 		// no listener is configured, so `c.Server6` (or `c.Server4` if v4)
 		// will stay nil.
+		log.Warnf("DHCPv%d: server%d present but no listen address defined. The server will not be started", ver, ver)
 		return nil
 	}
 	// read plugin configuration
@@ -210,7 +225,7 @@ func (c *Config) parseConfig(ver protocolVersion) error {
 	}
 	sc := ServerConfig{
 		Listener:  listenAddr,
-		Interface: ifname,
+		Interface: listenAddr.Zone,
 		Plugins:   plugins,
 	}
 	if ver == protocolV6 {

config/config_test.go

@@ -0,0 +1,52 @@
+// Copyright 2018-present the CoreDHCP Authors. All rights reserved
+// This source code is licensed under the MIT license found in the
+// LICENSE file in the root directory of this source tree.
+
+package config
+
+import "testing"
+
+func TestSplitHostPort(t *testing.T) {
+	testcases := []struct {
+		hostport string
+		ip       string
+		zone     string
+		port     string
+		err      bool // Should return an error (ie true for err != nil)
+	}{
+		{"0.0.0.0:67", "0.0.0.0", "", "67", false},
+		{"192.0.2.0", "192.0.2.0", "", "", false},
+		{"192.0.2.9%eth0", "192.0.2.9", "eth0", "", false},
+		{"0.0.0.0%eth0:67", "0.0.0.0", "eth0", "67", false},
+		{"0.0.0.0:20%eth0:67", "0.0.0.0", "eth0", "67", true},
+		{"2001:db8::1:547", "", "", "547", true}, // [] mandatory for v6
+		{"[::]:547", "::", "", "547", false},
+		{"[fe80::1%eth0]", "fe80::1", "eth0", "", false},
+		{"[fe80::1]:eth1", "fe80::1", "", "eth1", false},             // no validation of ports in this function
+		{"fe80::1%eth0:547", "fe80::1", "eth0", "547", true},         // [] mandatory for v6 even with %zone
+		{"fe80::1%eth0", "fe80::1", "eth0", "547", true},             // [] mandatory for v6 even without port
+		{"[2001:db8::2]47", "fe80::1", "eth0", "547", true},          // garbage after []
+		{"[ff02::1:2]%srv_u:547", "ff02::1:2", "srv_u", "547", true}, // FIXME: Linux `ss` format, we should accept but net.SplitHostPort doesn't
+		{":http", "", "", "http", false},
+		{"%eth0:80", "", "eth0", "80", false},          // janky, but looks valid enough for "[::%eth0]:80" imo
+		{"%eth0", "", "eth0", "", false},               // janky
+		{"fe80::1]:80", "fe80::1", "", "80", true},     // unbalanced ]
+		{"fe80::1%eth0]", "fe80::1", "eth0", "", true}, // unbalanced ], no port
+		{"", "", "", "", false},                        // trivial case, still valid
+	}
+
+	for _, tc := range testcases {
+		ip, zone, port, err := splitHostPort(tc.hostport)
+		if tc.err != (err != nil) {
+			errState := "not "
+			if tc.err {
+				errState = ""
+			}
+			t.Errorf("Mismatched error state: %s should %serror (got err: %v)\n", tc.hostport, errState, err)
+			continue
+		}
+		if err == nil && (ip != tc.ip || zone != tc.zone || port != tc.port) {
+			t.Errorf("%s => \"%s\", \"%s\", \"%s\" expected \"%s\",\"%s\",\"%s\"\n", tc.hostport, ip, zone, port, tc.ip, tc.zone, tc.port)
+		}
+	}
+}