home: make rdns great again

internal/home/dns.go

@@ -90,7 +90,7 @@ func initDNSServer() error {
 		return fmt.Errorf("dnsServer.Prepare: %w", err)
 	}
 
-	Context.rdns = InitRDNS(Context.dnsServer, &Context.clients)
+	Context.rdns = NewRDNS(Context.dnsServer, &Context.clients, Context.ipDetector, Context.localResolvers)
 	Context.whois = initWhois(&Context.clients)
 
 	Context.filters.Init()
@@ -108,11 +108,10 @@ func onDNSRequest(d *proxy.DNSContext) {
 		return
 	}
 
-	ipd := Context.ipDetector
-	if !ipd.IsLocallyServedNetwork(ip) {
+	if !ip.IsLoopback() {
 		Context.rdns.Begin(ip)
 	}
-	if !ipd.IsSpecialNetwork(ip) {
+	if !Context.ipDetector.IsSpecialNetwork(ip) {
 		Context.whois.Begin(ip)
 	}
 }

internal/home/rdns.go

@@ -2,129 +2,169 @@ package home
 
 import (
 	"encoding/binary"
+	"fmt"
 	"net"
 	"strings"
 	"time"
 
+	"github.com/AdguardTeam/AdGuardHome/internal/agherr"
+	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/AdGuardHome/internal/dnsforward"
 	"github.com/AdguardTeam/golibs/cache"
 	"github.com/AdguardTeam/golibs/log"
 	"github.com/miekg/dns"
 )
 
-// RDNS - module context
+// RDNS resolves clients' addresses to enrich their metadata.
 type RDNS struct {
-	dnsServer *dnsforward.Server
-	clients   *clientsContainer
-	ipChannel chan net.IP // pass data from DNS request handling thread to rDNS thread
-
-	// Contains IP addresses of clients to be resolved by rDNS
-	// If IP address is resolved, it stays here while it's inside Clients.
-	//  If it's removed from Clients, this IP address will be resolved once again.
-	// If IP address couldn't be resolved, it stays here for some time to prevent further attempts to resolve the same IP.
-	ipAddrs cache.Cache
+	dnsServer      *dnsforward.Server
+	clients        *clientsContainer
+	ipDetector     *aghnet.IPDetector
+	localResolvers aghnet.LocalResolvers
+
+	// ipChan used to pass client's IP to rDNS workerLoop.
+	ipChan chan net.IP
+
+	// ipCache caches the IP addresses to be resolved by rDNS.  The resolved
+	// address stays here while it's inside clients.  After leaving clients
+	// the address will be resolved once again.  If the address couldn't be
+	// resolved, cache prevents further attempts to resolve it for some
+	// time.
+	ipCache cache.Cache
 }
 
-// InitRDNS - create module context
-func InitRDNS(dnsServer *dnsforward.Server, clients *clientsContainer) *RDNS {
-	r := &RDNS{
-		dnsServer: dnsServer,
-		clients:   clients,
-		ipAddrs: cache.New(cache.Config{
+// Default rDNS values.
+const (
+	defaultRDNSCacheSize  = 10000
+	defaultRDNSCacheTTL   = 1 * 60 * 60
+	defaultRDNSipChanSize = 256
+)
+
+// NewRDNS creates and returns initialized RDNS.
+func NewRDNS(
+	dnsServer *dnsforward.Server,
+	clients *clientsContainer,
+	ipd *aghnet.IPDetector,
+	lr aghnet.LocalResolvers,
+) (rDNS *RDNS) {
+	rDNS = &RDNS{
+		dnsServer:      dnsServer,
+		clients:        clients,
+		ipDetector:     ipd,
+		localResolvers: lr,
+		ipCache: cache.New(cache.Config{
 			EnableLRU: true,
-			MaxCount:  10000,
+			MaxCount:  defaultRDNSCacheSize,
 		}),
-		ipChannel: make(chan net.IP, 256),
+		ipChan: make(chan net.IP, defaultRDNSipChanSize),
 	}
 
-	go r.workerLoop()
-	return r
+	go rDNS.workerLoop()
+
+	return rDNS
 }
 
-// Begin - add IP address to rDNS queue
+// Begin adds the ip to the resolving queue if it is not cached or already
+// resolved.
 func (r *RDNS) Begin(ip net.IP) {
 	now := uint64(time.Now().Unix())
-	expire := r.ipAddrs.Get(ip)
-	if len(expire) != 0 {
-		exp := binary.BigEndian.Uint64(expire)
-		if exp > now {
+	if expire := r.ipCache.Get(ip); len(expire) != 0 {
+		if binary.BigEndian.Uint64(expire) > now {
 			return
 		}
-		// TTL expired
 	}
-	expire = make([]byte, 8)
-	const ttl = 1 * 60 * 60
-	binary.BigEndian.PutUint64(expire, now+ttl)
-	_ = r.ipAddrs.Set(ip, expire)
+
+	// The cache entry either expired or doesn't exist.
+	ttl := make([]byte, 8)
+	binary.BigEndian.PutUint64(ttl, now+defaultRDNSCacheTTL)
+	r.ipCache.Set(ip, ttl)
 
 	id := ip.String()
 	if r.clients.Exists(id, ClientSourceRDNS) {
 		return
 	}
 
-	log.Tracef("rDNS: adding %s", ip)
 	select {
-	case r.ipChannel <- ip:
-		//
+	case r.ipChan <- ip:
+		log.Tracef("rdns: %q added to queue", ip)
 	default:
-		log.Tracef("rDNS: queue is full")
+		log.Tracef("rdns: queue is full")
 	}
 }
 
-// Use rDNS to get hostname by IP address
-func (r *RDNS) resolve(ip net.IP) string {
-	log.Tracef("Resolving host for %s", ip)
+const (
+	// rDNSEmptyAnswerErr is returned by RDNS resolve method when the answer
+	// section of respond is empty.
+	rDNSEmptyAnswerErr agherr.Error = "the answer section is empty"
 
-	name, err := dns.ReverseAddr(ip.String())
+	// rDNSNotPTRErr is returned by RDNS resolve method when the response is
+	// not of PTR type.
+	rDNSNotPTRErr agherr.Error = "the response is not a ptr"
+)
+
+// resolve tries to resolve the ip in a suitable way.
+func (r *RDNS) resolve(ip net.IP) (host string, err error) {
+	log.Tracef("rdns: resolving host for %q", ip)
+
+	var arpa string
+	arpa, err = dns.ReverseAddr(ip.String())
 	if err != nil {
-		log.Debug("Error while calling dns.ReverseAddr(%s): %s", ip, err)
-		return ""
+		return "", fmt.Errorf("reversing %q: %w", ip, err)
 	}
-
-	resp, err := r.dnsServer.Exchange(&dns.Msg{
+	msg := &dns.Msg{
 		MsgHdr: dns.MsgHdr{
 			Id:               dns.Id(),
 			RecursionDesired: true,
 		},
 		Question: []dns.Question{{
-			Name:   name,
+			Name:   arpa,
 			Qtype:  dns.TypePTR,
 			Qclass: dns.ClassINET,
 		}},
-	})
+	}
+
+	var resp *dns.Msg
+	if r.ipDetector.IsLocallyServedNetwork(ip) {
+		resp, err = r.localResolvers.Exchange(msg)
+	} else {
+		resp, err = r.dnsServer.Exchange(msg)
+	}
 	if err != nil {
-		log.Debug("Error while making an rDNS lookup for %s: %s", ip, err)
-		return ""
+		return "", fmt.Errorf("performing lookup for %q: %w", ip, err)
 	}
+
 	if len(resp.Answer) == 0 {
-		log.Debug("No answer for rDNS lookup of %s", ip)
-		return ""
+		return "", fmt.Errorf("lookup for %q: %w", ip, rDNSEmptyAnswerErr)
 	}
+
 	ptr, ok := resp.Answer[0].(*dns.PTR)
 	if !ok {
-		log.Debug("not a PTR response for %s", ip)
-		return ""
+		return "", fmt.Errorf("type checking: %w", rDNSNotPTRErr)
 	}
 
-	log.Tracef("PTR response for %s: %s", ip, ptr.String())
-	if strings.HasSuffix(ptr.Ptr, ".") {
-		ptr.Ptr = ptr.Ptr[:len(ptr.Ptr)-1]
-	}
+	log.Tracef("rdns: ptr response for %q: %s", ip, ptr.String())
 
-	return ptr.Ptr
+	return strings.TrimSuffix(ptr.Ptr, "."), nil
 }
 
-// Wait for a signal and then synchronously resolve hostname by IP address
-// Add the hostname:IP pair to "Clients" array
+// workerLoop handles incoming IP addresses from ipChan and adds it into
+// clients.
 func (r *RDNS) workerLoop() {
-	for {
-		ip := <-r.ipChannel
+	defer agherr.LogPanic("rdns")
+
+	for ip := range r.ipChan {
+		host, err := r.resolve(ip)
+		if err != nil {
+			log.Error("rdns: resolving %q: %s", ip, err)
 
-		host := r.resolve(ip)
-		if len(host) == 0 {
 			continue
 		}
 
-		_, _ = r.clients.AddHost(ip.String(), host, ClientSourceRDNS)
+		_, err = r.clients.AddHost(ip.String(), host, ClientSourceRDNS)
+		// AddHost always returns nil error for now but may begin to
+		// return some non-nil errors in the future.
+		if err != nil {
+			log.Error("rdns: adding %q into clients: %s", ip, err)
+		}
 	}
 }

internal/home/rdns_test.go

@@ -4,17 +4,20 @@ import (
 	"net"
 	"testing"
 
+	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghtest"
 	"github.com/AdguardTeam/AdGuardHome/internal/dnsforward"
 	"github.com/AdguardTeam/dnsproxy/proxy"
 	"github.com/AdguardTeam/dnsproxy/upstream"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
-func TestResolveRDNS(t *testing.T) {
+func TestRDNS_Resolve(t *testing.T) {
 	ups := &aghtest.TestUpstream{
 		Reverse: map[string][]string{
-			"1.1.1.1.in-addr.arpa.": {"one.one.one.one"},
+			"1.1.1.1.in-addr.arpa.":     {"one.one.one.one"},
+			"1.1.168.192.in-addr.arpa.": {"local.domain"},
 		},
 	}
 	dns := dnsforward.NewCustomServer(&proxy.Proxy{
@@ -26,7 +29,34 @@ func TestResolveRDNS(t *testing.T) {
 	})
 
 	clients := &clientsContainer{}
-	rdns := InitRDNS(dns, clients)
-	r := rdns.resolve(net.IP{1, 1, 1, 1})
-	assert.Equal(t, "one.one.one.one", r, r)
+
+	ipd, err := aghnet.NewIPDetector()
+	require.NoError(t, err)
+
+	lr := &aghtest.LocalResolvers{
+		Ups: ups,
+	}
+	rdns := NewRDNS(dns, clients, ipd, lr)
+
+	testCases := []struct {
+		name string
+		want string
+		req  net.IP
+	}{{
+		name: "external",
+		want: "one.one.one.one",
+		req:  net.IP{1, 1, 1, 1},
+	}, {
+		name: "local",
+		want: "local.domain",
+		req:  net.IP{192, 168, 1, 1},
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			r, rerr := rdns.resolve(tc.req)
+			require.Nil(t, rerr)
+			assert.Equal(t, tc.want, r)
+		})
+	}
 }