Pull request: 4939 Client update

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

Updates #4939.

Squashed commit of the following:

commit 34f35822af
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

@@ -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
 
 <!--

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
+	// Exchange tries to resolve the ip in a suitable way, i.e. either as local
-	// local or as external.
+	// 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
+	// ErrRDNSNoData is returned by [RDNSExchanger.Exchange] when the answer
-	// section of respond is empty.
+	// section of response is either NODATA or has no PTR records.
-	rDNSEmptyAnswerErr errors.Error = "the answer section is empty"
+	ErrRDNSNoData errors.Error = "no ptr data in response"
 
-	// rDNSNotPTRErr is returned by Exchange method when the response is not
+	// ErrRDNSFailed is returned by [RDNSExchanger.Exchange] if the received
-	// of PTR type.
+	// response is not a NOERROR or NXDOMAIN.
-	rDNSNotPTRErr errors.Error = "the response is not a ptr"
+	ErrRDNSFailed errors.Error = "failed to resolve ptr"
 )
 
 // type check
@@ -324,18 +324,25 @@ 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 {
+	if resp.Rcode != dns.RcodeSuccess && resp.Rcode != dns.RcodeNameError {
-		return "", fmt.Errorf("lookup for %q: %w", arpa, rDNSEmptyAnswerErr)
+		return "", fmt.Errorf(
-	}
+			"received %s response: %w",
-
+			dns.RcodeToString[resp.Rcode],
-	ptr, ok := resp.Answer[0].(*dns.PTR)
+			ErrRDNSFailed,
-	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 "", ErrRDNSNoData
+}
 
 // ResolvesPrivatePTR implements the RDNSExchanger interface for *Server.
 func (s *Server) ResolvesPrivatePTR() (ok bool) {

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 {

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.
+// clientSource checks if client with this IP address already exists and returns
-func (clients *clientsContainer) exists(ip netip.Addr, source clientSource) (ok bool) {
+// 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 rc.Source
-	return source <= rc.Source
 }
 
 func toQueryLogWHOIS(wi *RuntimeClientWHOISInfo) (cw *querylog.ClientWHOIS) {

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.Equal(t, clients.clientSource(cliNoneIP), ClientSourceNone)
-		assert.True(t, clients.exists(cli1IP, ClientSourceHostsFile))
+		assert.Equal(t, clients.clientSource(cli1IP), ClientSourcePersistent)
-		assert.True(t, clients.exists(cli2IP, ClientSourceHostsFile))
+		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.Equal(t, clients.clientSource(cliOldIP), ClientSourceNone)
-		assert.True(t, clients.exists(cliNewIP, ClientSourceHostsFile))
+		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) {

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
+	revDNSCacheSize = 10000
-	defaultRDNSCacheTTL  = 1 * 60 * 60
+
-	defaultRDNSIPChSize  = 256
+	// 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 binary.BigEndian.Uint64(expire) > now
-			return true
 	}
-	}
-
-	// 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)
-
+			if errors.Is(err, dnsforward.ErrRDNSFailed) {
-			continue
+				// Cache failure for a less time.
-		} else if host == "" {
+				ttl = revDNSFailureCacheTTL
-			continue
+			}
 		}
 
+		r.cache(ip, ttl)
+
+		if host != "" {
 			_ = r.clients.AddHost(ip, host, ClientSourceRDNS)
 		}
 	}
+}

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{
@@ -204,21 +204,25 @@ func TestRDNS_WorkerLoop(t *testing.T) {
 		cliIP            netip.Addr
 		wantLog          string
 		name             string
+		wantClientSource clientSource
 	}{{
 		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",
+		wantClientSource: ClientSourceNone,
 	}, {
 		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))
 		})
 	}
 }