From 8a62785e62e1d802dddc4c7e61853533122781fe Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Thu, 10 Jun 2021 14:54:47 +0300 Subject: [PATCH] Pull request: 3166 fix dhcp Updates #3166. Squashed commit of the following: commit cac62f842886f06a70fb50d251938e0feaa06846 Author: Ildar Kamalov Date: Thu Jun 10 14:35:40 2021 +0300 client: get dhcp status on static lease submit commit 2ae4e3ab40041fbc0f53f4902a00263425409a6a Merge: 1e0f9c15 299ea88c Author: Eugene Burkov Date: Thu Jun 10 14:09:05 2021 +0300 Merge branch 'master' into 3166-fix-dhcp commit 1e0f9c15d98f40a499c75b74bca48fe58512e300 Author: Eugene Burkov Date: Thu Jun 10 14:03:58 2021 +0300 dhcpd: imp code commit e4b9626b60a11cd2491c675a297f527f7e12f9b4 Author: Eugene Burkov Date: Thu Jun 10 13:03:47 2021 +0300 dhcpd: fix static lease load commit dc449f21986a8ea300d07c081f1e416b2f08bc25 Author: Eugene Burkov Date: Tue Jun 1 17:21:10 2021 +0300 home: change clients sources priority --- CHANGELOG.md | 3 ++ client/src/actions/index.js | 1 + internal/aghnet/addr.go | 2 +- internal/dhcpd/v4.go | 65 ++++++++++++++++++----------------- internal/home/clients.go | 4 +-- internal/home/clients_test.go | 14 ++++++++ 6 files changed, 54 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7097ce6a..e1fe5cb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,8 @@ released by then. ### Fixed +- Redundant hostname generating while loading static leases with empty hostname + ([#3166]). - Domain name case in responses ([#3194]). - Custom upstreams selection for clients with client IDs in DNS-over-TLS and DNS-over-HTTP ([#3186]). @@ -61,6 +63,7 @@ released by then. [#2443]: https://github.com/AdguardTeam/AdGuardHome/issues/2443 [#2763]: https://github.com/AdguardTeam/AdGuardHome/issues/2763 [#3136]: https://github.com/AdguardTeam/AdGuardHome/issues/3136 +[#3166]: https://github.com/AdguardTeam/AdGuardHome/issues/3166 [#3172]: https://github.com/AdguardTeam/AdGuardHome/issues/3172 [#3184]: https://github.com/AdguardTeam/AdGuardHome/issues/3184 [#3185]: https://github.com/AdguardTeam/AdGuardHome/issues/3185 diff --git a/client/src/actions/index.js b/client/src/actions/index.js index 31179de7..8b2ccf9e 100644 --- a/client/src/actions/index.js +++ b/client/src/actions/index.js @@ -561,6 +561,7 @@ export const addStaticLease = (config) => async (dispatch) => { dispatch(addStaticLeaseSuccess(config)); dispatch(addSuccessToast(i18next.t('dhcp_lease_added', { key: name }))); dispatch(toggleLeaseModal()); + dispatch(getDhcpStatus()); } catch (error) { dispatch(addErrorToast({ error })); dispatch(addStaticLeaseFailure()); diff --git a/internal/aghnet/addr.go b/internal/aghnet/addr.go index 336cd136..c76ef093 100644 --- a/internal/aghnet/addr.go +++ b/internal/aghnet/addr.go @@ -127,7 +127,7 @@ func ValidateDomainName(name string) (err error) { // The maximum lengths of generated hostnames for different IP versions. const ( - ipv4HostnameMaxLen = len("192-168-100-10-") + ipv4HostnameMaxLen = len("192-168-100-100") ipv6HostnameMaxLen = len("ff80-f076-0000-0000-0000-0000-0000-0010") ) diff --git a/internal/dhcpd/v4.go b/internal/dhcpd/v4.go index ee37d097..21345153 100644 --- a/internal/dhcpd/v4.go +++ b/internal/dhcpd/v4.go @@ -110,7 +110,9 @@ func (s *v4Server) ResetLeases(leases []*Lease) { s.leases = nil for _, l := range leases { - l.Hostname = s.validHostnameForClient(l.Hostname, l.IP) + if !l.IsStatic() { + l.Hostname = s.validHostnameForClient(l.Hostname, l.IP) + } err = s.addLease(l) if err != nil { // TODO(a.garipov): Wrap and bubble up the error. @@ -339,9 +341,8 @@ func (s *v4Server) AddStaticLease(l Lease) (err error) { return err } - if l.Hostname != "" { - var hostname string - hostname, err = normalizeHostname(l.Hostname) + if hostname := l.Hostname; hostname != "" { + hostname, err = normalizeHostname(hostname) if err != nil { return err } @@ -635,6 +636,33 @@ func (o *optFQDN) ToBytes() []byte { return b } +// checkLease checks if the pair of mac and ip is already leased. The mismatch +// is true when the existing lease has the same hardware address but differs in +// its IP address. +func (s *v4Server) checkLease(mac net.HardwareAddr, ip net.IP) (lease *Lease, mismatch bool) { + s.leasesLock.Lock() + defer s.leasesLock.Unlock() + + for _, l := range s.leases { + if !bytes.Equal(l.HWAddr, mac) { + continue + } + + if l.IP.Equal(ip) { + return l, false + } + + log.Debug( + `dhcpv4: mismatched OptionRequestedIPAddress in req msg for %s`, + mac, + ) + + return nil, true + } + + return nil, false +} + // processRequest is the handler for the DHCP Request request. func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, needsReply bool) { mac := req.ClientHWAddr @@ -656,33 +684,8 @@ func (s *v4Server) processRequest(req, resp *dhcpv4.DHCPv4) (lease *Lease, needs return nil, false } - mismatch := false - func() { - s.leasesLock.Lock() - defer s.leasesLock.Unlock() - - for _, l := range s.leases { - if !bytes.Equal(l.HWAddr, mac) { - continue - } - - if l.IP.Equal(reqIP) { - lease = l - - return - } - - log.Debug( - `dhcpv4: mismatched OptionRequestedIPAddress in req msg for %s`, - mac, - ) - - mismatch = true - - return - } - }() - if mismatch { + var mismatch bool + if lease, mismatch = s.checkLease(mac, reqIP); mismatch { return nil, true } diff --git a/internal/home/clients.go b/internal/home/clients.go index 30c508e4..c1a7d499 100644 --- a/internal/home/clients.go +++ b/internal/home/clients.go @@ -53,13 +53,11 @@ type Client struct { type clientSource uint // Client sources. The order determines the priority. -// -// TODO(e.burkov): Is ARP a higher priority source than DHCP? const ( ClientSourceWHOIS clientSource = iota ClientSourceRDNS - ClientSourceDHCP ClientSourceARP + ClientSourceDHCP ClientSourceHostsFile ) diff --git a/internal/home/clients_test.go b/internal/home/clients_test.go index f8c34fbb..c609721c 100644 --- a/internal/home/clients_test.go +++ b/internal/home/clients_test.go @@ -151,6 +151,20 @@ func TestClients(t *testing.T) { assert.True(t, clients.Exists("1.1.1.1", ClientSourceHostsFile)) }) + t.Run("dhcp_replaces_arp", func(t *testing.T) { + ok, err := clients.AddHost("1.2.3.4", "from_arp", ClientSourceARP) + require.NoError(t, err) + assert.True(t, ok) + + assert.True(t, clients.Exists("1.2.3.4", ClientSourceARP)) + + ok, err = clients.AddHost("1.2.3.4", "from_dhcp", ClientSourceDHCP) + require.NoError(t, err) + assert.True(t, ok) + + assert.True(t, clients.Exists("1.2.3.4", ClientSourceDHCP)) + }) + t.Run("addhost_fail", func(t *testing.T) { ok, err := clients.AddHost("1.1.1.1", "host1", ClientSourceRDNS) require.NoError(t, err)