From ec19a85ed02e17001e06134c9d5ed2258c37ec08 Mon Sep 17 00:00:00 2001
From: Eugene Burkov <e.burkov@adguard.com>
Date: Fri, 10 Feb 2023 16:40:36 +0300
Subject: [PATCH] Pull request: 4939 Client update

Merge in DNS/adguard-home from 4939-client-upd to master

Updates #4939.

Squashed commit of the following:

commit 34f35822afcc8020a674cd023a5907b5b3edcb65
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Feb 10 14:01:57 2023 +0300

    all: imp code, docs

commit 1cd8767a38f6494c92fb5ceff26abe228fcca638
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Feb 9 17:20:56 2023 +0300

    all: different ttls

commit 66d951ba3dd72cb698b89b432cbbbdd65cb421a2
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Feb 9 14:24:47 2023 +0300

    all: imp code

commit 3fb8d08310296dad90783f13ba46a1d0ea11da2e
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Feb 8 19:35:29 2023 +0300

    home: fix rdns check logic
---
 CHANGELOG.md                           |  2 +
 internal/dnsforward/dnsforward.go      | 35 ++++++++++-------
 internal/dnsforward/dnsforward_test.go | 15 +++++--
 internal/home/clients.go               | 23 ++++++-----
 internal/home/clients_test.go          | 18 ++++-----
 internal/home/rdns.go                  | 54 ++++++++++++++++----------
 internal/home/rdns_test.go             | 48 +++++++++++++----------
 7 files changed, 119 insertions(+), 76 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 30e280f2..47168a00 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -62,6 +62,7 @@ In this release, the schema version has changed from 14 to 15.
 
 ### Fixed
 
+- Client names resolved via reverse DNS not being updated ([#4939]).
 - The icon for League Of Legends on the Blocked services page ([#5433]).
 
 ### Removed
@@ -70,6 +71,7 @@ In this release, the schema version has changed from 14 to 15.
 
 [#1717]: https://github.com/AdguardTeam/AdGuardHome/issues/1717
 [#4299]: https://github.com/AdguardTeam/AdGuardHome/issues/4299
+[#4939]: https://github.com/AdguardTeam/AdGuardHome/issues/4939
 [#5433]: https://github.com/AdguardTeam/AdGuardHome/issues/5433
 
 <!--
diff --git a/internal/dnsforward/dnsforward.go b/internal/dnsforward/dnsforward.go
index a5f07e62..ffec33f2 100644
--- a/internal/dnsforward/dnsforward.go
+++ b/internal/dnsforward/dnsforward.go
@@ -253,8 +253,8 @@ func (s *Server) Resolve(host string) ([]net.IPAddr, error) {
 
 // RDNSExchanger is a resolver for clients' addresses.
 type RDNSExchanger interface {
-	// Exchange tries to resolve the ip in a suitable way, e.g. either as
-	// local or as external.
+	// Exchange tries to resolve the ip in a suitable way, i.e. either as local
+	// or as external.
 	Exchange(ip net.IP) (host string, err error)
 
 	// ResolvesPrivatePTR returns true if the RDNSExchanger is able to
@@ -263,13 +263,13 @@ type RDNSExchanger interface {
 }
 
 const (
-	// rDNSEmptyAnswerErr is returned by Exchange method when the answer
-	// section of respond is empty.
-	rDNSEmptyAnswerErr errors.Error = "the answer section is empty"
+	// ErrRDNSNoData is returned by [RDNSExchanger.Exchange] when the answer
+	// section of response is either NODATA or has no PTR records.
+	ErrRDNSNoData errors.Error = "no ptr data in response"
 
-	// rDNSNotPTRErr is returned by Exchange method when the response is not
-	// of PTR type.
-	rDNSNotPTRErr errors.Error = "the response is not a ptr"
+	// ErrRDNSFailed is returned by [RDNSExchanger.Exchange] if the received
+	// response is not a NOERROR or NXDOMAIN.
+	ErrRDNSFailed errors.Error = "failed to resolve ptr"
 )
 
 // type check
@@ -324,17 +324,24 @@ func (s *Server) Exchange(ip net.IP) (host string, err error) {
 		return "", err
 	}
 
+	// Distinguish between NODATA response and a failed request.
 	resp := ctx.Res
-	if len(resp.Answer) == 0 {
-		return "", fmt.Errorf("lookup for %q: %w", arpa, rDNSEmptyAnswerErr)
+	if resp.Rcode != dns.RcodeSuccess && resp.Rcode != dns.RcodeNameError {
+		return "", fmt.Errorf(
+			"received %s response: %w",
+			dns.RcodeToString[resp.Rcode],
+			ErrRDNSFailed,
+		)
 	}
 
-	ptr, ok := resp.Answer[0].(*dns.PTR)
-	if !ok {
-		return "", fmt.Errorf("type checking: %w", rDNSNotPTRErr)
+	for _, ans := range resp.Answer {
+		ptr, ok := ans.(*dns.PTR)
+		if ok {
+			return strings.TrimSuffix(ptr.Ptr, "."), nil
+		}
 	}
 
-	return strings.TrimSuffix(ptr.Ptr, "."), nil
+	return "", ErrRDNSNoData
 }
 
 // ResolvesPrivatePTR implements the RDNSExchanger interface for *Server.
diff --git a/internal/dnsforward/dnsforward_test.go b/internal/dnsforward/dnsforward_test.go
index 91239826..a95b03df 100644
--- a/internal/dnsforward/dnsforward_test.go
+++ b/internal/dnsforward/dnsforward_test.go
@@ -1221,6 +1221,9 @@ func TestServer_Exchange(t *testing.T) {
 
 	errUpstream := aghtest.NewErrorUpstream()
 	nonPtrUpstream := aghtest.NewBlockUpstream("some-host", true)
+	refusingUpstream := aghtest.NewUpstreamMock(func(req *dns.Msg) (resp *dns.Msg, err error) {
+		return new(dns.Msg).SetRcode(req, dns.RcodeRefused), nil
+	})
 
 	srv := &Server{
 		recDetector: newRecursionDetector(0, 1),
@@ -1265,15 +1268,21 @@ func TestServer_Exchange(t *testing.T) {
 	}, {
 		name:        "empty_answer_error",
 		want:        "",
-		wantErr:     rDNSEmptyAnswerErr,
+		wantErr:     ErrRDNSNoData,
 		locUpstream: locUpstream,
 		req:         net.IP{192, 168, 1, 2},
 	}, {
-		name:        "not_ptr_error",
+		name:        "invalid_answer",
 		want:        "",
-		wantErr:     rDNSNotPTRErr,
+		wantErr:     ErrRDNSNoData,
 		locUpstream: nonPtrUpstream,
 		req:         localIP,
+	}, {
+		name:        "refused",
+		want:        "",
+		wantErr:     ErrRDNSFailed,
+		locUpstream: refusingUpstream,
+		req:         localIP,
 	}}
 
 	for _, tc := range testCases {
diff --git a/internal/home/clients.go b/internal/home/clients.go
index f46c616e..5d0a9ef2 100644
--- a/internal/home/clients.go
+++ b/internal/home/clients.go
@@ -67,15 +67,18 @@ func (c *Client) closeUpstreams() (err error) {
 
 type clientSource uint
 
-// Client sources.  The order determines the priority.
+// Clients information sources.  The order determines the priority.
 const (
-	ClientSourceWHOIS clientSource = iota
+	ClientSourceNone clientSource = iota
+	ClientSourceWHOIS
 	ClientSourceARP
 	ClientSourceRDNS
 	ClientSourceDHCP
 	ClientSourceHostsFile
+	ClientSourcePersistent
 )
 
+// type check
 var _ fmt.Stringer = clientSource(0)
 
 // String returns a human-readable name of cs.
@@ -96,6 +99,7 @@ func (cs clientSource) String() (s string) {
 	}
 }
 
+// type check
 var _ encoding.TextMarshaler = clientSource(0)
 
 // MarshalText implements encoding.TextMarshaler for the clientSource.
@@ -332,23 +336,24 @@ func (clients *clientsContainer) onDHCPLeaseChanged(flags int) {
 	}
 }
 
-// exists checks if client with this IP address already exists.
-func (clients *clientsContainer) exists(ip netip.Addr, source clientSource) (ok bool) {
+// clientSource checks if client with this IP address already exists and returns
+// the source which updated it last.  It returns [ClientSourceNone] if the
+// client doesn't exist.
+func (clients *clientsContainer) clientSource(ip netip.Addr) (src clientSource) {
 	clients.lock.Lock()
 	defer clients.lock.Unlock()
 
-	_, ok = clients.findLocked(ip.String())
+	_, ok := clients.findLocked(ip.String())
 	if ok {
-		return true
+		return ClientSourcePersistent
 	}
 
 	rc, ok := clients.ipToRC[ip]
 	if !ok {
-		return false
+		return ClientSourceNone
 	}
 
-	// Return false if the new source has higher priority.
-	return source <= rc.Source
+	return rc.Source
 }
 
 func toQueryLogWHOIS(wi *RuntimeClientWHOISInfo) (cw *querylog.ClientWHOIS) {
diff --git a/internal/home/clients_test.go b/internal/home/clients_test.go
index ca98352a..612fe7db 100644
--- a/internal/home/clients_test.go
+++ b/internal/home/clients_test.go
@@ -67,9 +67,9 @@ func TestClients(t *testing.T) {
 
 		assert.Equal(t, "client2", c.Name)
 
-		assert.False(t, clients.exists(cliNoneIP, ClientSourceHostsFile))
-		assert.True(t, clients.exists(cli1IP, ClientSourceHostsFile))
-		assert.True(t, clients.exists(cli2IP, ClientSourceHostsFile))
+		assert.Equal(t, clients.clientSource(cliNoneIP), ClientSourceNone)
+		assert.Equal(t, clients.clientSource(cli1IP), ClientSourcePersistent)
+		assert.Equal(t, clients.clientSource(cli2IP), ClientSourcePersistent)
 	})
 
 	t.Run("add_fail_name", func(t *testing.T) {
@@ -127,8 +127,8 @@ func TestClients(t *testing.T) {
 		})
 		require.NoError(t, err)
 
-		assert.False(t, clients.exists(cliOldIP, ClientSourceHostsFile))
-		assert.True(t, clients.exists(cliNewIP, ClientSourceHostsFile))
+		assert.Equal(t, clients.clientSource(cliOldIP), ClientSourceNone)
+		assert.Equal(t, clients.clientSource(cliNewIP), ClientSourcePersistent)
 
 		err = clients.Update("client1", &Client{
 			IDs:            []string{cliNew},
@@ -157,7 +157,7 @@ func TestClients(t *testing.T) {
 		ok := clients.Del("client1-renamed")
 		require.True(t, ok)
 
-		assert.False(t, clients.exists(netip.MustParseAddr("1.1.1.2"), ClientSourceHostsFile))
+		assert.Equal(t, clients.clientSource(netip.MustParseAddr("1.1.1.2")), ClientSourceNone)
 	})
 
 	t.Run("del_fail", func(t *testing.T) {
@@ -176,18 +176,18 @@ func TestClients(t *testing.T) {
 		ok = clients.AddHost(ip, "host3", ClientSourceHostsFile)
 		assert.True(t, ok)
 
-		assert.True(t, clients.exists(ip, ClientSourceHostsFile))
+		assert.Equal(t, clients.clientSource(ip), ClientSourceHostsFile)
 	})
 
 	t.Run("dhcp_replaces_arp", func(t *testing.T) {
 		ip := netip.MustParseAddr("1.2.3.4")
 		ok := clients.AddHost(ip, "from_arp", ClientSourceARP)
 		assert.True(t, ok)
-		assert.True(t, clients.exists(ip, ClientSourceARP))
+		assert.Equal(t, clients.clientSource(ip), ClientSourceARP)
 
 		ok = clients.AddHost(ip, "from_dhcp", ClientSourceDHCP)
 		assert.True(t, ok)
-		assert.True(t, clients.exists(ip, ClientSourceDHCP))
+		assert.Equal(t, clients.clientSource(ip), ClientSourceDHCP)
 	})
 
 	t.Run("addhost_fail", func(t *testing.T) {
diff --git a/internal/home/rdns.go b/internal/home/rdns.go
index 0d7227f7..cae7a9c3 100644
--- a/internal/home/rdns.go
+++ b/internal/home/rdns.go
@@ -8,6 +8,7 @@ import (
 
 	"github.com/AdguardTeam/AdGuardHome/internal/dnsforward"
 	"github.com/AdguardTeam/golibs/cache"
+	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
 )
 
@@ -30,11 +31,15 @@ type RDNS struct {
 	usePrivate atomic.Bool
 }
 
-// Default rDNS values.
+// Default AdGuard Home reverse DNS values.
 const (
-	defaultRDNSCacheSize = 10000
-	defaultRDNSCacheTTL  = 1 * 60 * 60
-	defaultRDNSIPChSize  = 256
+	revDNSCacheSize = 10000
+
+	// TODO(e.burkov):  Make these values configurable.
+	revDNSCacheTTL        = 24 * 60 * 60
+	revDNSFailureCacheTTL = 1 * 60 * 60
+
+	revDNSQueueSize = 256
 )
 
 // NewRDNS creates and returns initialized RDNS.
@@ -48,9 +53,9 @@ func NewRDNS(
 		clients:   clients,
 		ipCache: cache.New(cache.Config{
 			EnableLRU: true,
-			MaxCount:  defaultRDNSCacheSize,
+			MaxCount:  revDNSCacheSize,
 		}),
-		ipCh: make(chan netip.Addr, defaultRDNSIPChSize),
+		ipCh: make(chan netip.Addr, revDNSQueueSize),
 	}
 
 	rDNS.usePrivate.Store(usePrivate)
@@ -79,25 +84,28 @@ func (r *RDNS) isCached(ip netip.Addr) (ok bool) {
 	ipBytes := ip.AsSlice()
 	now := uint64(time.Now().Unix())
 	if expire := r.ipCache.Get(ipBytes); len(expire) != 0 {
-		if binary.BigEndian.Uint64(expire) > now {
-			return true
-		}
+		return binary.BigEndian.Uint64(expire) > now
 	}
 
-	// The cache entry either expired or doesn't exist.
-	ttl := make([]byte, 8)
-	binary.BigEndian.PutUint64(ttl, now+defaultRDNSCacheTTL)
-	r.ipCache.Set(ipBytes, ttl)
-
 	return false
 }
 
+// cache caches the ip address for ttl seconds.
+func (r *RDNS) cache(ip netip.Addr, ttl uint64) {
+	ipData := ip.AsSlice()
+
+	ttlData := [8]byte{}
+	binary.BigEndian.PutUint64(ttlData[:], uint64(time.Now().Unix())+ttl)
+
+	r.ipCache.Set(ipData, ttlData[:])
+}
+
 // Begin adds the ip to the resolving queue if it is not cached or already
 // resolved.
 func (r *RDNS) Begin(ip netip.Addr) {
 	r.ensurePrivateCache()
 
-	if r.isCached(ip) || r.clients.exists(ip, ClientSourceRDNS) {
+	if r.isCached(ip) || r.clients.clientSource(ip) > ClientSourceRDNS {
 		return
 	}
 
@@ -115,15 +123,21 @@ func (r *RDNS) workerLoop() {
 	defer log.OnPanic("rdns")
 
 	for ip := range r.ipCh {
+		ttl := uint64(revDNSCacheTTL)
+
 		host, err := r.exchanger.Exchange(ip.AsSlice())
 		if err != nil {
 			log.Debug("rdns: resolving %q: %s", ip, err)
-
-			continue
-		} else if host == "" {
-			continue
+			if errors.Is(err, dnsforward.ErrRDNSFailed) {
+				// Cache failure for a less time.
+				ttl = revDNSFailureCacheTTL
+			}
 		}
 
-		_ = r.clients.AddHost(ip, host, ClientSourceRDNS)
+		r.cache(ip, ttl)
+
+		if host != "" {
+			_ = r.clients.AddHost(ip, host, ClientSourceRDNS)
+		}
 	}
 }
diff --git a/internal/home/rdns_test.go b/internal/home/rdns_test.go
index 8dc675b5..4e424273 100644
--- a/internal/home/rdns_test.go
+++ b/internal/home/rdns_test.go
@@ -76,7 +76,7 @@ func TestRDNS_Begin(t *testing.T) {
 
 		ipCache := cache.New(cache.Config{
 			EnableLRU: true,
-			MaxCount:  defaultRDNSCacheSize,
+			MaxCount:  revDNSCacheSize,
 		})
 		ttl := make([]byte, binary.Size(uint64(0)))
 		binary.BigEndian.PutUint64(ttl, uint64(time.Now().Add(100*time.Hour).Unix()))
@@ -153,7 +153,7 @@ func TestRDNS_ensurePrivateCache(t *testing.T) {
 
 	ipCache := cache.New(cache.Config{
 		EnableLRU: true,
-		MaxCount:  defaultRDNSCacheSize,
+		MaxCount:  revDNSCacheSize,
 	})
 
 	ex := &rDNSExchanger{
@@ -200,25 +200,29 @@ func TestRDNS_WorkerLoop(t *testing.T) {
 	errUpstream := aghtest.NewErrorUpstream()
 
 	testCases := []struct {
-		ups     upstream.Upstream
-		cliIP   netip.Addr
-		wantLog string
-		name    string
+		ups              upstream.Upstream
+		cliIP            netip.Addr
+		wantLog          string
+		name             string
+		wantClientSource clientSource
 	}{{
-		ups:     locUpstream,
-		cliIP:   localIP,
-		wantLog: "",
-		name:    "all_good",
+		ups:              locUpstream,
+		cliIP:            localIP,
+		wantLog:          "",
+		name:             "all_good",
+		wantClientSource: ClientSourceRDNS,
 	}, {
-		ups:     errUpstream,
-		cliIP:   netip.MustParseAddr("192.168.1.2"),
-		wantLog: `rdns: resolving "192.168.1.2": test upstream error`,
-		name:    "resolve_error",
+		ups:              errUpstream,
+		cliIP:            netip.MustParseAddr("192.168.1.2"),
+		wantLog:          `rdns: resolving "192.168.1.2": test upstream error`,
+		name:             "resolve_error",
+		wantClientSource: ClientSourceNone,
 	}, {
-		ups:     locUpstream,
-		cliIP:   netip.MustParseAddr("2a00:1450:400c:c06::93"),
-		wantLog: "",
-		name:    "ipv6_good",
+		ups:              locUpstream,
+		cliIP:            netip.MustParseAddr("2a00:1450:400c:c06::93"),
+		wantLog:          "",
+		name:             "ipv6_good",
+		wantClientSource: ClientSourceRDNS,
 	}}
 
 	for _, tc := range testCases {
@@ -237,6 +241,10 @@ func TestRDNS_WorkerLoop(t *testing.T) {
 			},
 			clients: cc,
 			ipCh:    ch,
+			ipCache: cache.New(cache.Config{
+				EnableLRU: true,
+				MaxCount:  revDNSCacheSize,
+			}),
 		}
 
 		t.Run(tc.name, func(t *testing.T) {
@@ -253,11 +261,9 @@ func TestRDNS_WorkerLoop(t *testing.T) {
 
 			if tc.wantLog != "" {
 				assert.Contains(t, w.String(), tc.wantLog)
-
-				return
 			}
 
-			assert.True(t, cc.exists(tc.cliIP, ClientSourceRDNS))
+			assert.Equal(t, tc.wantClientSource, cc.clientSource(tc.cliIP))
 		})
 	}
 }