Pull request: all: do not use dhcp clients when server is off

Closes AdguardTeam#2934.

Squashed commit of the following:

commit 856ea4e
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu Apr 15 17:39:46 2021 +0300

    dnsforward: imp spacing

commit fa748e5
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu Apr 15 17:33:44 2021 +0300

    dnsforward: imp code

commit 771ba0d
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu Apr 15 17:06:03 2021 +0300

    all: do not use dhcp clients when server is off

CHANGELOG.md

@@ -47,6 +47,8 @@ and this project adheres to
 
 ### Fixed
 
+- Inconsistent resolving of DHCP clients when the DHCP server is disabled
+  ([#2934]).
 - Comment handling in clients' custom upstreams ([#2947]).
 - Overwriting of DHCPv4 options when using the HTTP API ([#2927]).
 - Assumption that MAC addresses always have the length of 6 octets ([#2828]).
@@ -75,6 +77,7 @@ and this project adheres to
 [#2838]: https://github.com/AdguardTeam/AdGuardHome/issues/2838
 [#2889]: https://github.com/AdguardTeam/AdGuardHome/issues/2889
 [#2927]: https://github.com/AdguardTeam/AdGuardHome/issues/2927
+[#2934]: https://github.com/AdguardTeam/AdGuardHome/issues/2934
 [#2945]: https://github.com/AdguardTeam/AdGuardHome/issues/2945
 [#2947]: https://github.com/AdguardTeam/AdGuardHome/issues/2947
 

internal/dhcpd/dhcpd.go

@@ -115,6 +115,7 @@ const (
 	LeaseChangedAdded = iota
 	LeaseChangedAddedStatic
 	LeaseChangedRemovedStatic
+	LeaseChangedRemovedAll
 
 	LeaseChangedDBStore
 )

internal/dhcpd/v4.go

@@ -801,6 +801,10 @@ func (s *v4Server) Start() error {
 		log.Debug("dhcpv4: srv.Serve: %s", err)
 	}()
 
+	// Signal to the clients containers in packages home and dnsforward that
+	// it should reload the DHCP clients.
+	s.conf.notify(LeaseChangedAdded)
+
 	return nil
 }
 
@@ -815,7 +819,11 @@ func (s *v4Server) Stop() {
 	if err != nil {
 		log.Error("dhcpv4: srv.Close: %s", err)
 	}
-	// now s.srv.Serve() will return
+
+	// Signal to the clients containers in packages home and dnsforward that
+	// it should remove all DHCP clients.
+	s.conf.notify(LeaseChangedRemovedAll)
+
 	s.srv = nil
 }
 

internal/dhcpd/v4_test.go

@@ -23,7 +23,7 @@ func TestV4_AddRemove_static(t *testing.T) {
 		SubnetMask: net.IP{255, 255, 255, 0},
 		notify:     notify4,
 	})
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	ls := s.GetLeases(LeasesStatic)
 	assert.Empty(t, ls)
@@ -33,23 +33,30 @@ func TestV4_AddRemove_static(t *testing.T) {
 		IP:     net.IP{192, 168, 10, 150},
 		HWAddr: net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA},
 	}
-	require.Nil(t, s.AddStaticLease(l))
-	assert.NotNil(t, s.AddStaticLease(l))
+
+	err = s.AddStaticLease(l)
+	require.NoError(t, err)
+
+	err = s.AddStaticLease(l)
+	assert.Error(t, err)
 
 	ls = s.GetLeases(LeasesStatic)
 	require.Len(t, ls, 1)
+
 	assert.True(t, l.IP.Equal(ls[0].IP))
 	assert.Equal(t, l.HWAddr, ls[0].HWAddr)
 	assert.True(t, ls[0].IsStatic())
 
 	// Try to remove static lease.
-	assert.NotNil(t, s.RemoveStaticLease(Lease{
+	err = s.RemoveStaticLease(Lease{
 		IP:     net.IP{192, 168, 10, 110},
 		HWAddr: net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA},
-	}))
+	})
+	assert.Error(t, err)
 
 	// Remove static lease.
-	require.Nil(t, s.RemoveStaticLease(l))
+	err = s.RemoveStaticLease(l)
+	require.NoError(t, err)
 	ls = s.GetLeases(LeasesStatic)
 	assert.Empty(t, ls)
 }
@@ -63,7 +70,7 @@ func TestV4_AddReplace(t *testing.T) {
 		SubnetMask: net.IP{255, 255, 255, 0},
 		notify:     notify4,
 	})
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	s, ok := sIface.(*v4Server)
 	require.True(t, ok)
@@ -78,7 +85,7 @@ func TestV4_AddReplace(t *testing.T) {
 
 	for i := range dynLeases {
 		err = s.addLease(&dynLeases[i])
-		require.Nil(t, err)
+		require.NoError(t, err)
 	}
 
 	stLeases := []Lease{{
@@ -90,7 +97,8 @@ func TestV4_AddReplace(t *testing.T) {
 	}}
 
 	for _, l := range stLeases {
-		require.Nil(t, s.AddStaticLease(l))
+		err = s.AddStaticLease(l)
+		require.NoError(t, err)
 	}
 
 	ls := s.GetLeases(LeasesStatic)
@@ -113,32 +121,35 @@ func TestV4StaticLease_Get(t *testing.T) {
 		SubnetMask: net.IP{255, 255, 255, 0},
 		notify:     notify4,
 	})
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	s, ok := sIface.(*v4Server)
 	require.True(t, ok)
+
 	s.conf.dnsIPAddrs = []net.IP{{192, 168, 10, 1}}
 
 	l := Lease{
 		IP:     net.IP{192, 168, 10, 150},
 		HWAddr: net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA},
 	}
-	require.Nil(t, s.AddStaticLease(l))
+	err = s.AddStaticLease(l)
+	require.NoError(t, err)
 
 	var req, resp *dhcpv4.DHCPv4
 	mac := net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}
 
 	t.Run("discover", func(t *testing.T) {
-		var terr error
+		req, err = dhcpv4.NewDiscovery(mac)
+		require.NoError(t, err)
 
-		req, terr = dhcpv4.NewDiscovery(mac)
-		require.Nil(t, terr)
+		resp, err = dhcpv4.NewReplyFromRequest(req)
+		require.NoError(t, err)
 
-		resp, terr = dhcpv4.NewReplyFromRequest(req)
-		require.Nil(t, terr)
 		assert.Equal(t, 1, s.process(req, resp))
 	})
-	require.Nil(t, err)
+
+	// Don't continue if we got any errors in the previous subtest.
+	require.NoError(t, err)
 
 	t.Run("offer", func(t *testing.T) {
 		assert.Equal(t, dhcpv4.MessageTypeOffer, resp.MessageType())
@@ -152,13 +163,15 @@ func TestV4StaticLease_Get(t *testing.T) {
 
 	t.Run("request", func(t *testing.T) {
 		req, err = dhcpv4.NewRequestFromOffer(resp)
-		require.Nil(t, err)
+		require.NoError(t, err)
 
 		resp, err = dhcpv4.NewReplyFromRequest(req)
-		require.Nil(t, err)
+		require.NoError(t, err)
+
 		assert.Equal(t, 1, s.process(req, resp))
 	})
-	require.Nil(t, err)
+
+	require.NoError(t, err)
 
 	t.Run("ack", func(t *testing.T) {
 		assert.Equal(t, dhcpv4.MessageTypeAck, resp.MessageType())
@@ -172,11 +185,13 @@ func TestV4StaticLease_Get(t *testing.T) {
 
 	dnsAddrs := resp.DNS()
 	require.Len(t, dnsAddrs, 1)
+
 	assert.True(t, s.conf.GatewayIP.Equal(dnsAddrs[0]))
 
 	t.Run("check_lease", func(t *testing.T) {
 		ls := s.GetLeases(LeasesStatic)
 		require.Len(t, ls, 1)
+
 		assert.True(t, l.IP.Equal(ls[0].IP))
 		assert.Equal(t, mac, ls[0].HWAddr)
 	})
@@ -196,26 +211,28 @@ func TestV4DynamicLease_Get(t *testing.T) {
 			"82 ip 1.2.3.4",
 		},
 	})
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	s, ok := sIface.(*v4Server)
 	require.True(t, ok)
+
 	s.conf.dnsIPAddrs = []net.IP{{192, 168, 10, 1}}
 
 	var req, resp *dhcpv4.DHCPv4
 	mac := net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}
 
 	t.Run("discover", func(t *testing.T) {
 		req, err = dhcpv4.NewDiscovery(mac)
-		require.Nil(t, err)
+		require.NoError(t, err)
 
 		resp, err = dhcpv4.NewReplyFromRequest(req)
-		require.Nil(t, err)
+		require.NoError(t, err)
+
 		assert.Equal(t, 1, s.process(req, resp))
 	})
 
 	// Don't continue if we got any errors in the previous subtest.
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	t.Run("offer", func(t *testing.T) {
 		assert.Equal(t, dhcpv4.MessageTypeOffer, resp.MessageType())
@@ -226,6 +243,7 @@ func TestV4DynamicLease_Get(t *testing.T) {
 
 		router := resp.Router()
 		require.Len(t, router, 1)
+
 		assert.Equal(t, s.conf.GatewayIP, router[0])
 
 		assert.Equal(t, s.conf.subnetMask, resp.SubnetMask())
@@ -236,16 +254,16 @@ func TestV4DynamicLease_Get(t *testing.T) {
 	})
 
 	t.Run("request", func(t *testing.T) {
-		var terr error
+		req, err = dhcpv4.NewRequestFromOffer(resp)
+		require.NoError(t, err)
 
-		req, terr = dhcpv4.NewRequestFromOffer(resp)
-		require.Nil(t, terr)
+		resp, err = dhcpv4.NewReplyFromRequest(req)
+		require.NoError(t, err)
 
-		resp, terr = dhcpv4.NewReplyFromRequest(req)
-		require.Nil(t, terr)
 		assert.Equal(t, 1, s.process(req, resp))
 	})
-	require.Nil(t, err)
+
+	require.NoError(t, err)
 
 	t.Run("ack", func(t *testing.T) {
 		assert.Equal(t, dhcpv4.MessageTypeAck, resp.MessageType())
@@ -259,12 +277,14 @@ func TestV4DynamicLease_Get(t *testing.T) {
 
 	dnsAddrs := resp.DNS()
 	require.Len(t, dnsAddrs, 1)
+
 	assert.True(t, net.IP{192, 168, 10, 1}.Equal(dnsAddrs[0]))
 
 	// check lease
 	t.Run("check_lease", func(t *testing.T) {
 		ls := s.GetLeases(LeasesDynamic)
-		assert.Len(t, ls, 1)
+		require.Len(t, ls, 1)
+
 		assert.True(t, net.IP{192, 168, 10, 100}.Equal(ls[0].IP))
 		assert.Equal(t, mac, ls[0].HWAddr)
 	})

internal/dnsforward/dns.go

@@ -154,44 +154,60 @@ func isHostnameOK(hostname string) bool {
 	return true
 }
 
+func (s *Server) setTableHostToIP(t hostToIPTable) {
+	s.tableHostToIPLock.Lock()
+	defer s.tableHostToIPLock.Unlock()
+
+	s.tableHostToIP = t
+}
+
+func (s *Server) setTableIPToHost(t ipToHostTable) {
+	s.tableIPToHostLock.Lock()
+	defer s.tableIPToHostLock.Unlock()
+
+	s.tableIPToHost = t
+}
+
 func (s *Server) onDHCPLeaseChanged(flags int) {
+	add := true
 	switch flags {
 	case dhcpd.LeaseChangedAdded,
 		dhcpd.LeaseChangedAddedStatic,
 		dhcpd.LeaseChangedRemovedStatic:
-		//
+		// Go on.
+	case dhcpd.LeaseChangedRemovedAll:
+		add = false
 	default:
 		return
 	}
 
-	hostToIP := make(map[string]net.IP)
-	m := make(map[string]string)
-
-	ll := s.dhcpServer.Leases(dhcpd.LeasesAll)
+	var hostToIP hostToIPTable
+	var ipToHost ipToHostTable
+	if add {
+		hostToIP = make(hostToIPTable)
+		ipToHost = make(ipToHostTable)
 
-	for _, l := range ll {
-		if len(l.Hostname) == 0 || !isHostnameOK(l.Hostname) {
-			continue
-		}
+		ll := s.dhcpServer.Leases(dhcpd.LeasesAll)
 
-		lowhost := strings.ToLower(l.Hostname)
+		for _, l := range ll {
+			if len(l.Hostname) == 0 || !isHostnameOK(l.Hostname) {
+				continue
+			}
 
-		m[l.IP.String()] = lowhost
+			lowhost := strings.ToLower(l.Hostname)
 
-		ip := make(net.IP, 4)
-		copy(ip, l.IP.To4())
-		hostToIP[lowhost] = ip
-	}
+			ipToHost[l.IP.String()] = lowhost
 
-	log.Debug("dns: added %d A/PTR entries from DHCP", len(m))
+			ip := make(net.IP, 4)
+			copy(ip, l.IP.To4())
+			hostToIP[lowhost] = ip
+		}
 
-	s.tableHostToIPLock.Lock()
-	s.tableHostToIP = hostToIP
-	s.tableHostToIPLock.Unlock()
+		log.Debug("dns: added %d A/PTR entries from DHCP", len(ipToHost))
+	}
 
-	s.tablePTRLock.Lock()
-	s.tablePTR = m
-	s.tablePTRLock.Unlock()
+	s.setTableHostToIP(hostToIP)
+	s.setTableIPToHost(ipToHost)
 }
 
 // processDetermineLocal determines if the client's IP address is from
@@ -336,14 +352,14 @@ func (s *Server) processRestrictLocal(ctx *dnsContext) (rc resultCode) {
 // ipToHost tries to get a hostname leased by DHCP.  It's safe for concurrent
 // use.
 func (s *Server) ipToHost(ip net.IP) (host string, ok bool) {
-	s.tablePTRLock.Lock()
-	defer s.tablePTRLock.Unlock()
+	s.tableIPToHostLock.Lock()
+	defer s.tableIPToHostLock.Unlock()
 
-	if s.tablePTR == nil {
+	if s.tableIPToHost == nil {
 		return "", false
 	}
 
-	host, ok = s.tablePTR[ip.String()]
+	host, ok = s.tableIPToHost[ip.String()]
 
 	return host, ok
 }

internal/dnsforward/dns_test.go

@@ -91,7 +91,7 @@ func TestServer_ProcessInternalHosts_localRestriction(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			s := &Server{
 				autohostSuffix: defaultAutohostSuffix,
-				tableHostToIP: map[string]net.IP{
+				tableHostToIP: hostToIPTable{
 					"example": knownIP,
 				},
 			}
@@ -202,7 +202,7 @@ func TestServer_ProcessInternalHosts(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			s := &Server{
 				autohostSuffix: tc.suffix,
-				tableHostToIP: map[string]net.IP{
+				tableHostToIP: hostToIPTable{
 					"example": knownIP,
 				},
 			}