Pull request: dhcpd: wait for interfaces' ip addresses to appear

Merge in DNS/adguard-home from 2304-dncp-backoff to master

Updates #2304.

Squashed commit of the following:

commit c9bff8b27c6b031d43a7dd98152adcde7f49fff1
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Fri Nov 27 14:08:03 2020 +0300

    dhcpd: try for 5s instead of 10s

commit 983cf471832de0e7762b8b6e0a4ba9bb76ecadfc
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Nov 25 19:58:41 2020 +0300

    dhcpd: wait for interfaces' ip addresses to appear

HACKING.md

@@ -1,8 +1,9 @@
  #  *AdGuardHome* Developer Guidelines
 
-As of **2020-11-20**, this document is still a work-in-progress.  Some of the
+As of **2020-11-27**, this document is a work-in-progress, but should still be
-rules aren't enforced, and others might change.  Still, this is a good place to
+followed.  Some of the rules aren't enforced as thoroughly or remain broken in
-find out about how we **want** our code to look like.
+old code, but this is still the place to find out about what we **want** our
+code to look like.
 
 The rules are mostly sorted in the alphabetical order.
 
@@ -31,27 +32,17 @@ The rules are mostly sorted in the alphabetical order.
 
 ##  *Go*
 
- *  <https://github.com/golang/go/wiki/CodeReviewComments>.
+ ###  Code And Naming
 
- *  <https://github.com/golang/go/wiki/TestComments>.
+ *  Avoid `goto`.
-
- *  <https://go-proverbs.github.io/>
-
- *  Add an empty line before `break`, `continue`, and `return`, unless it's the
-    only statement in that block.
 
  *  Avoid `init` and use explicit initialization functions instead.
 
  *  Avoid `new`, especially with structs.
 
- *  Document everything, including unexported top-level identifiers, to build
-    a habit of writing documentation.
-
  *  Constructors should validate their arguments and return meaningful errors.
     As a corollary, avoid lazy initialization.
 
- *  Don't put variable names into any kind of quotes.
-
  *  Don't use naked `return`s.
 
  *  Don't use underscores in file and package names, unless they're build tags
@@ -76,25 +67,34 @@ The rules are mostly sorted in the alphabetical order.
 
  *  Name the deferred errors (e.g. when closing something) `cerr`.
 
- *  No `goto`.
-
  *  No shadowing, since it can often lead to subtle bugs, especially with
     errors.
 
  *  Prefer constants to variables where possible.  Reduce global variables.  Use
     [constant errors] instead of `errors.New`.
 
- *  Put comments above the documented entity, **not** to the side, to improve
-    readability.
-
- *  Use `gofumpt --extra -s`.
-
-    **TODO(a.garipov):** Add to the linters.
-
  *  Use linters.
 
  *  Use named returns to improve readability of function signatures.
 
+ *  Write logs and error messages in lowercase only to make it easier to `grep`
+    logs and error messages without using the `-i` flag.
+
+[constant errors]: https://dave.cheney.net/2016/04/07/constant-errors
+[Linus said]:      https://www.kernel.org/doc/html/v4.17/process/coding-style.html#indentation
+
+ ###  Commenting
+
+ *  See also the *Text, Including Comments* section below.
+
+ *  Document everything, including unexported top-level identifiers, to build
+    a habit of writing documentation.
+
+ *  Don't put identifiers into any kind of quotes.
+
+ *  Put comments above the documented entity, **not** to the side, to improve
+    readability.
+
  *  When a method implements an interface, start the doc comment with the
     standard template:
 
@@ -105,8 +105,14 @@ The rules are mostly sorted in the alphabetical order.
     }
     ```
 
- *  Write logs and error messages in lowercase only to make it easier to `grep`
+ ###  Formatting
-    logs and error messages without using the `-i` flag.
+
+ *  Add an empty line before `break`, `continue`, `fallthrough`, and `return`,
+    unless it's the only statement in that block.
+
+ *  Use `gofumpt --extra -s`.
+
+    **TODO(a.garipov):** Add to the linters.
 
  *  Write slices of struct like this:
 
@@ -123,8 +129,13 @@ The rules are mostly sorted in the alphabetical order.
     }}
     ```
 
-[constant errors]: https://dave.cheney.net/2016/04/07/constant-errors
+ ###  Recommended Reading
-[Linus said]:      https://www.kernel.org/doc/html/v4.17/process/coding-style.html#indentation
+
+ *  <https://github.com/golang/go/wiki/CodeReviewComments>.
+
+ *  <https://github.com/golang/go/wiki/TestComments>.
+
+ *  <https://go-proverbs.github.io/>
 
 ##  *Markdown*
 

internal/dhcpd/check_other_dhcp.go

@@ -26,7 +26,7 @@ func CheckIfOtherDHCPServersPresentV4(ifaceName string) (bool, error) {
 		return false, fmt.Errorf("couldn't find interface by name %s: %w", ifaceName, err)
 	}
 
-	ifaceIPNet, err := ifaceIPv4Addrs(iface)
+	ifaceIPNet, err := ifaceIPAddrs(iface, ipVersion4)
 	if err != nil {
 		return false, fmt.Errorf("getting ipv4 addrs for iface %s: %w", ifaceName, err)
 	}
@@ -161,7 +161,7 @@ func CheckIfOtherDHCPServersPresentV6(ifaceName string) (bool, error) {
 		return false, fmt.Errorf("dhcpv6: net.InterfaceByName: %s: %w", ifaceName, err)
 	}
 
-	ifaceIPNet, err := ifaceIPv6Addrs(iface)
+	ifaceIPNet, err := ifaceIPAddrs(iface, ipVersion6)
 	if err != nil {
 		return false, fmt.Errorf("getting ipv6 addrs for iface %s: %w", ifaceName, err)
 	}

internal/dhcpd/v4.go

@@ -16,7 +16,9 @@ import (
 	"github.com/insomniacslk/dhcp/dhcpv4/server4"
 )
 
-// v4Server - DHCPv4 server
+// v4Server is a DHCPv4 server.
+//
+// TODO(a.garipov): Think about unifying this and v6Server.
 type v4Server struct {
 	srv        *server4.Server
 	leasesLock sync.Mutex
@@ -560,27 +562,6 @@ func (s *v4Server) packetHandler(conn net.PacketConn, peer net.Addr, req *dhcpv4
 	}
 }
 
-// ifaceIPv4Addrs returns the interface's IPv4 addresses.
-func ifaceIPv4Addrs(iface *net.Interface) (ips []net.IP, err error) {
-	addrs, err := iface.Addrs()
-	if err != nil {
-		return nil, err
-	}
-
-	for _, a := range addrs {
-		ipnet, ok := a.(*net.IPNet)
-		if !ok {
-			continue
-		}
-
-		if ip := ipnet.IP.To4(); ip != nil {
-			ips = append(ips, ip)
-		}
-	}
-
-	return ips, nil
-}
-
 // Start starts the IPv4 DHCP server.
 func (s *v4Server) Start() error {
 	if !s.conf.Enabled {
@@ -595,26 +576,9 @@ func (s *v4Server) Start() error {
 
 	log.Debug("dhcpv4: starting...")
 
-	dnsIPAddrs, err := ifaceIPv4Addrs(iface)
+	dnsIPAddrs, err := ifaceDNSIPAddrs(iface, ipVersion4, defaultMaxAttempts, defaultBackoff)
 	if err != nil {
-		return fmt.Errorf("dhcpv4: getting ipv4 addrs for iface %s: %w", ifaceName, err)
+		return fmt.Errorf("dhcpv4: interface %s: %w", ifaceName, err)
-	}
-
-	switch len(dnsIPAddrs) {
-	case 0:
-		log.Debug("dhcpv4: no ipv4 address for interface %s", iface.Name)
-
-		return nil
-	case 1:
-		// Some Android devices use 8.8.8.8 if there is no secondary DNS
-		// server.  Fix that by setting the secondary DNS address to our
-		// address as well.
-		//
-		// See https://github.com/AdguardTeam/AdGuardHome/issues/1708.
-		log.Debug("dhcpv4: setting secondary dns ip to iself for interface %s", iface.Name)
-		dnsIPAddrs = append(dnsIPAddrs, dnsIPAddrs[0])
-	default:
-		// Go on.
 	}
 
 	s.conf.dnsIPAddrs = dnsIPAddrs

internal/dhcpd/v46.go

@@ -0,0 +1,123 @@
+package dhcpd
+
+import (
+	"fmt"
+	"net"
+	"time"
+
+	"github.com/AdguardTeam/golibs/log"
+)
+
+// ipVersion is a documentational alias for int.  Use it when the integer means
+// IP version.
+type ipVersion = int
+
+// IP version constants.
+const (
+	ipVersion4 ipVersion = 4
+	ipVersion6 ipVersion = 6
+)
+
+// netIface is the interface for network interface methods.
+type netIface interface {
+	Addrs() ([]net.Addr, error)
+}
+
+// ifaceIPAddrs returns the interface's IP addresses.
+func ifaceIPAddrs(iface netIface, ipv ipVersion) (ips []net.IP, err error) {
+	addrs, err := iface.Addrs()
+	if err != nil {
+		return nil, err
+	}
+
+	for _, a := range addrs {
+		var ip net.IP
+		switch a := a.(type) {
+		case *net.IPAddr:
+			ip = a.IP
+		case *net.IPNet:
+			ip = a.IP
+		default:
+			continue
+		}
+
+		// Assume that net.(*Interface).Addrs can only return valid IPv4
+		// and IPv6 addresses.  Thus, if it isn't an IPv4 address, it
+		// must be an IPv6 one.
+		switch ipv {
+		case ipVersion4:
+			if ip4 := ip.To4(); ip4 != nil {
+				ips = append(ips, ip4)
+			}
+		case ipVersion6:
+			if ip6 := ip.To4(); ip6 == nil {
+				ips = append(ips, ip)
+			}
+		default:
+			return nil, fmt.Errorf("invalid ip version %d", ipv)
+		}
+	}
+
+	return ips, nil
+}
+
+// Currently used defaults for ifaceDNSAddrs.
+const (
+	defaultMaxAttempts int = 10
+
+	defaultBackoff time.Duration = 500 * time.Millisecond
+)
+
+// ifaceDNSIPAddrs returns IP addresses of the interface suitable to send to
+// clients as DNS addresses.  If err is nil, addrs contains either no addresses
+// or at least two.
+//
+// It makes up to maxAttempts attempts to get the addresses if there are none,
+// each time using the provided backoff.  Sometimes an interface needs a few
+// seconds to really ititialize.
+//
+// See https://github.com/AdguardTeam/AdGuardHome/issues/2304.
+func ifaceDNSIPAddrs(
+	iface netIface,
+	ipv ipVersion,
+	maxAttempts int,
+	backoff time.Duration,
+) (addrs []net.IP, err error) {
+	var n int
+waitForIP:
+	for n = 1; n <= maxAttempts; n++ {
+		addrs, err = ifaceIPAddrs(iface, ipv)
+		if err != nil {
+			return nil, fmt.Errorf("getting ip addrs: %w", err)
+		}
+
+		switch len(addrs) {
+		case 0:
+			log.Debug("dhcpv%d: attempt %d: no ip addresses", ipv, n)
+
+			time.Sleep(backoff)
+		case 1:
+			// Some Android devices use 8.8.8.8 if there is not
+			// a secondary DNS server.  Fix that by setting the
+			// secondary DNS address to the same address.
+			//
+			// See https://github.com/AdguardTeam/AdGuardHome/issues/1708.
+			log.Debug("dhcpv%d: setting secondary dns ip to itself", ipv)
+			addrs = append(addrs, addrs[0])
+
+			fallthrough
+		default:
+			break waitForIP
+		}
+	}
+
+	if len(addrs) == 0 {
+		// Don't return errors in case the users want to try and enable
+		// the DHCP server later.
+		log.Error("dhcpv%d: no ip address for interface after %d attempts and %s", ipv, n, time.Duration(n)*backoff)
+	} else {
+		log.Debug("dhcpv%d: got addresses %s after %d attempts", ipv, addrs, n)
+	}
+
+	return addrs, nil
+}

internal/dhcpd/v46_test.go

@@ -0,0 +1,189 @@
+package dhcpd
+
+import (
+	"errors"
+	"net"
+	"testing"
+
+	"github.com/AdguardTeam/AdGuardHome/internal/agherr"
+	"github.com/stretchr/testify/assert"
+)
+
+type fakeIface struct {
+	addrs []net.Addr
+	err   error
+}
+
+// Addrs implements the netIface interface for *fakeIface.
+func (iface *fakeIface) Addrs() (addrs []net.Addr, err error) {
+	if iface.err != nil {
+		return nil, iface.err
+	}
+
+	return iface.addrs, nil
+}
+
+func TestIfaceIPAddrs(t *testing.T) {
+	const errTest agherr.Error = "test error"
+
+	ip4 := net.IP{1, 2, 3, 4}
+	addr4 := &net.IPNet{IP: ip4}
+
+	ip6 := net.IP{1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5, 6}
+	addr6 := &net.IPNet{IP: ip6}
+
+	testCases := []struct {
+		name    string
+		iface   netIface
+		ipv     ipVersion
+		want    []net.IP
+		wantErr error
+	}{{
+		name:    "ipv4_success",
+		iface:   &fakeIface{addrs: []net.Addr{addr4}, err: nil},
+		ipv:     ipVersion4,
+		want:    []net.IP{ip4},
+		wantErr: nil,
+	}, {
+		name:    "ipv4_success_with_ipv6",
+		iface:   &fakeIface{addrs: []net.Addr{addr6, addr4}, err: nil},
+		ipv:     ipVersion4,
+		want:    []net.IP{ip4},
+		wantErr: nil,
+	}, {
+		name:    "ipv4_error",
+		iface:   &fakeIface{addrs: []net.Addr{addr4}, err: errTest},
+		ipv:     ipVersion4,
+		want:    nil,
+		wantErr: errTest,
+	}, {
+		name:    "ipv6_success",
+		iface:   &fakeIface{addrs: []net.Addr{addr6}, err: nil},
+		ipv:     ipVersion6,
+		want:    []net.IP{ip6},
+		wantErr: nil,
+	}, {
+		name:    "ipv6_success_with_ipv4",
+		iface:   &fakeIface{addrs: []net.Addr{addr6, addr4}, err: nil},
+		ipv:     ipVersion6,
+		want:    []net.IP{ip6},
+		wantErr: nil,
+	}, {
+		name:    "ipv6_error",
+		iface:   &fakeIface{addrs: []net.Addr{addr6}, err: errTest},
+		ipv:     ipVersion6,
+		want:    nil,
+		wantErr: errTest,
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			got, gotErr := ifaceIPAddrs(tc.iface, tc.ipv)
+			assert.Equal(t, tc.want, got)
+			assert.True(t, errors.Is(gotErr, tc.wantErr))
+		})
+	}
+}
+
+type waitingFakeIface struct {
+	addrs []net.Addr
+	err   error
+	n     int
+}
+
+// Addrs implements the netIface interface for *waitingFakeIface.
+func (iface *waitingFakeIface) Addrs() (addrs []net.Addr, err error) {
+	if iface.err != nil {
+		return nil, iface.err
+	}
+
+	if iface.n == 0 {
+		return iface.addrs, nil
+	}
+
+	iface.n--
+
+	return nil, nil
+}
+
+func TestIfaceDNSIPAddrs(t *testing.T) {
+	const errTest agherr.Error = "test error"
+
+	ip4 := net.IP{1, 2, 3, 4}
+	addr4 := &net.IPNet{IP: ip4}
+
+	ip6 := net.IP{1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5, 6}
+	addr6 := &net.IPNet{IP: ip6}
+
+	testCases := []struct {
+		name    string
+		iface   netIface
+		ipv     ipVersion
+		want    []net.IP
+		wantErr error
+	}{{
+		name:    "ipv4_success",
+		iface:   &fakeIface{addrs: []net.Addr{addr4}, err: nil},
+		ipv:     ipVersion4,
+		want:    []net.IP{ip4, ip4},
+		wantErr: nil,
+	}, {
+		name:    "ipv4_success_with_ipv6",
+		iface:   &fakeIface{addrs: []net.Addr{addr6, addr4}, err: nil},
+		ipv:     ipVersion4,
+		want:    []net.IP{ip4, ip4},
+		wantErr: nil,
+	}, {
+		name:    "ipv4_error",
+		iface:   &fakeIface{addrs: []net.Addr{addr4}, err: errTest},
+		ipv:     ipVersion4,
+		want:    nil,
+		wantErr: errTest,
+	}, {
+		name: "ipv4_wait",
+		iface: &waitingFakeIface{
+			addrs: []net.Addr{addr4},
+			err:   nil,
+			n:     1,
+		},
+		ipv:     ipVersion4,
+		want:    []net.IP{ip4, ip4},
+		wantErr: nil,
+	}, {
+		name:    "ipv6_success",
+		iface:   &fakeIface{addrs: []net.Addr{addr6}, err: nil},
+		ipv:     ipVersion6,
+		want:    []net.IP{ip6, ip6},
+		wantErr: nil,
+	}, {
+		name:    "ipv6_success_with_ipv4",
+		iface:   &fakeIface{addrs: []net.Addr{addr6, addr4}, err: nil},
+		ipv:     ipVersion6,
+		want:    []net.IP{ip6, ip6},
+		wantErr: nil,
+	}, {
+		name:    "ipv6_error",
+		iface:   &fakeIface{addrs: []net.Addr{addr6}, err: errTest},
+		ipv:     ipVersion6,
+		want:    nil,
+		wantErr: errTest,
+	}, {
+		name: "ipv6_wait",
+		iface: &waitingFakeIface{
+			addrs: []net.Addr{addr6},
+			err:   nil,
+			n:     1,
+		},
+		ipv:     ipVersion6,
+		want:    []net.IP{ip6, ip6},
+		wantErr: nil,
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			got, gotErr := ifaceDNSIPAddrs(tc.iface, tc.ipv, 2, 0)
+			assert.Equal(t, tc.want, got)
+			assert.True(t, errors.Is(gotErr, tc.wantErr))
+		})
+	}
+}

internal/dhcpd/v6.go

@@ -17,7 +17,9 @@ import (
 
 const valueIAID = "ADGH" // value for IANA.ID
 
-// v6Server - DHCPv6 server
+// v6Server is a DHCPv6 server.
+//
+// TODO(a.garipov): Think about unifying this and v4Server.
 type v6Server struct {
 	srv        *server6.Server
 	leasesLock sync.Mutex
@@ -537,34 +539,6 @@ func (s *v6Server) packetHandler(conn net.PacketConn, peer net.Addr, req dhcpv6.
 	}
 }
 
-type netIface interface {
-	Addrs() ([]net.Addr, error)
-}
-
-// ifaceIPv6Addrs returns the interface's IPv6 addresses.
-func ifaceIPv6Addrs(iface netIface) (ips []net.IP, err error) {
-	addrs, err := iface.Addrs()
-	if err != nil {
-		return nil, err
-	}
-
-	for _, a := range addrs {
-		ipnet, ok := a.(*net.IPNet)
-		if !ok {
-			continue
-		}
-
-		if ip := ipnet.IP.To4(); ip == nil {
-			// Assume that net.(*Interface).Addrs can only return
-			// valid IPv4 and IPv6 addresses.  Since this isn't an
-			// IPv4 address, it must be an IPv6 one.
-			ips = append(ips, ipnet.IP)
-		}
-	}
-
-	return ips, nil
-}
-
 // initialize RA module
 func (s *v6Server) initRA(iface *net.Interface) error {
 	// choose the source IP address - should be link-local-unicast
@@ -598,24 +572,11 @@ func (s *v6Server) Start() error {
 		return fmt.Errorf("dhcpv6: finding interface %s by name: %w", ifaceName, err)
 	}
 
-	log.Debug("dhcpv4: starting...")
+	log.Debug("dhcpv6: starting...")
 
-	dnsIPAddrs, err := ifaceIPv6Addrs(iface)
+	dnsIPAddrs, err := ifaceDNSIPAddrs(iface, ipVersion6, defaultMaxAttempts, defaultBackoff)
 	if err != nil {
-		return fmt.Errorf("dhcpv6: getting ipv6 addrs for iface %s: %w", ifaceName, err)
+		return fmt.Errorf("dhcpv6: interface %s: %w", ifaceName, err)
-	}
-
-	switch len(dnsIPAddrs) {
-	case 0:
-		log.Debug("dhcpv6: no ipv6 address for interface %s", iface.Name)
-
-		return nil
-	case 1:
-		// See the comment in (*v4Server).Start.
-		log.Debug("dhcpv6: setting secondary dns ip to iself for interface %s", iface.Name)
-		dnsIPAddrs = append(dnsIPAddrs, dnsIPAddrs[0])
-	default:
-		// Go on.
 	}
 
 	s.conf.dnsIPAddrs = dnsIPAddrs
@@ -631,7 +592,7 @@ func (s *v6Server) Start() error {
 		return nil
 	}
 
-	log.Debug("DHCPv6: starting...")
+	log.Debug("dhcpv6: listening...")
 
 	if len(iface.HardwareAddr) != 6 {
 		return fmt.Errorf("dhcpv6: invalid MAC %s", iface.HardwareAddr)

internal/dhcpd/v6_test.go

@@ -6,7 +6,6 @@ import (
 	"net"
 	"testing"
 
-	"github.com/AdguardTeam/AdGuardHome/internal/agherr"
 	"github.com/insomniacslk/dhcp/dhcpv6"
 	"github.com/insomniacslk/dhcp/iana"
 	"github.com/stretchr/testify/assert"
@@ -224,57 +223,3 @@ func TestV6GetDynamicLease(t *testing.T) {
 	assert.True(t, ip6InRange(net.ParseIP("2001::2"), net.ParseIP("2001::2")))
 	assert.True(t, ip6InRange(net.ParseIP("2001::2"), net.ParseIP("2001::3")))
 }
-
-type fakeIface struct {
-	addrs []net.Addr
-	err   error
-}
-
-// Addrs implements the netIface interface for *fakeIface.
-func (iface *fakeIface) Addrs() (addrs []net.Addr, err error) {
-	if iface.err != nil {
-		return nil, iface.err
-	}
-
-	return iface.addrs, nil
-}
-
-func TestIfaceIPv6Addrs(t *testing.T) {
-	ip := net.IP{1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5, 6}
-	ip4 := net.IP{1, 2, 3, 4}
-	addr := &net.IPNet{IP: ip}
-	errTest := agherr.Error("test error")
-
-	testCases := []struct {
-		name    string
-		iface   netIface
-		want    []net.IP
-		wantErr error
-	}{{
-		name:    "success",
-		iface:   &fakeIface{addrs: []net.Addr{addr}, err: nil},
-		want:    []net.IP{ip},
-		wantErr: nil,
-	}, {
-		name: "success_with_ipv4",
-		iface: &fakeIface{
-			addrs: []net.Addr{addr, &net.IPNet{IP: ip4}},
-			err:   nil,
-		},
-		want:    []net.IP{ip},
-		wantErr: nil,
-	}, {
-		name:    "error",
-		iface:   &fakeIface{addrs: []net.Addr{addr}, err: errTest},
-		want:    nil,
-		wantErr: errTest,
-	}}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			got, gotErr := ifaceIPv6Addrs(tc.iface)
-			assert.Equal(t, tc.want, got)
-			assert.Equal(t, tc.wantErr, gotErr)
-		})
-	}
-}