diff --git a/internal/dnsforward/dnsforward.go b/internal/dnsforward/dnsforward.go index 98c42a3c1..419dfb57a 100644 --- a/internal/dnsforward/dnsforward.go +++ b/internal/dnsforward/dnsforward.go @@ -376,7 +376,7 @@ func (s *Server) Resolve(ctx context.Context, net, host string) (addr []netip.Ad const ( // 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" + ErrRDNSNoData = rdns.ErrNoData // ErrRDNSFailed is returned by [RDNSExchanger.Exchange] if the received // response is not a NOERROR or NXDOMAIN. diff --git a/internal/rdns/rdns.go b/internal/rdns/rdns.go index af5b9ec86..9e9adbe65 100644 --- a/internal/rdns/rdns.go +++ b/internal/rdns/rdns.go @@ -12,6 +12,16 @@ import ( "github.com/bluele/gcache" ) +// maxErrorCacheTTL is the maximum period during which failed lookups aren't +// retried. It prevents request floods while allowing recovery from temporary +// resolver outages sooner than the regular cache TTL. +const maxErrorCacheTTL = 1 * time.Minute + +// ErrNoData is returned by an [Exchanger] when a successful DNS response has +// no PTR data. It is cached for the regular cache TTL rather than treated as a +// temporary resolver failure. +const ErrNoData errors.Error = "no ptr data in response" + // Interface processes rDNS queries. type Interface interface { // Process makes rDNS request and returns domain name. changed indicates @@ -96,10 +106,15 @@ func (r *Default) Process(ctx context.Context, ip netip.Addr) (host string, chan host, ttl, err := r.exchanger.Exchange(ctx, ip) if err != nil { r.logger.DebugContext(ctx, "resolving", "ip", ip, slogutil.KeyError, err) + if errors.Is(err, ErrNoData) { + ttl = max(ttl, r.cacheTTL) + } else { + ttl = min(r.cacheTTL, maxErrorCacheTTL) + } + } else { + ttl = max(ttl, r.cacheTTL) } - ttl = max(ttl, r.cacheTTL) - item := &cacheItem{ expiry: time.Now().Add(ttl), host: host, diff --git a/internal/rdns/rdns_internal_test.go b/internal/rdns/rdns_internal_test.go new file mode 100644 index 000000000..2b5ed1aa5 --- /dev/null +++ b/internal/rdns/rdns_internal_test.go @@ -0,0 +1,119 @@ +package rdns + +import ( + "context" + "net/netip" + "testing" + "time" + + "github.com/AdguardTeam/golibs/errors" + "github.com/AdguardTeam/golibs/logutil/slogutil" + "github.com/AdguardTeam/golibs/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type testExchanger func( + ctx context.Context, + ip netip.Addr, +) (host string, ttl time.Duration, err error) + +// Exchange implements the [Exchanger] interface for testExchanger. +func (f testExchanger) Exchange( + ctx context.Context, + ip netip.Addr, +) (host string, ttl time.Duration, err error) { + return f(ctx, ip) +} + +func TestDefault_Process_errorRetry(t *testing.T) { + const shortCacheTTL = 30 * time.Second + + testCases := []struct { + err error + name string + cacheTTL time.Duration + wantTTL time.Duration + }{ + { + name: "temporary_failure", + err: errors.Error("service unavailable"), + cacheTTL: time.Hour, + wantTTL: maxErrorCacheTTL, + }, + { + name: "temporary_failure_short_cache", + err: errors.Error("service unavailable"), + cacheTTL: shortCacheTTL, + wantTTL: shortCacheTTL, + }, + { + name: "no_data", + err: ErrNoData, + cacheTTL: time.Hour, + wantTTL: time.Hour, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var ( + available bool + hits int + ) + + wantHost := "router.example" + exchanger := testExchanger(func( + _ context.Context, + _ netip.Addr, + ) (host string, ttl time.Duration, err error) { + hits++ + if !available { + return "", 0, tc.err + } + + return wantHost, tc.cacheTTL, nil + }) + + r := New(&Config{ + Logger: slogutil.NewDiscardLogger(), + Exchanger: exchanger, + CacheSize: 1, + CacheTTL: tc.cacheTTL, + }) + + ctx := testutil.ContextWithTimeout(t, time.Second) + ip := netip.MustParseAddr("192.168.1.1") + started := time.Now() + + got, changed := r.Process(ctx, ip) + finished := time.Now() + require.True(t, changed) + assert.Empty(t, got) + require.Equal(t, 1, hits) + + val, err := r.cache.Get(ip) + require.NoError(t, err) + + item := val.(*cacheItem) + assert.False(t, item.expiry.Before(started.Add(tc.wantTTL))) + assert.False(t, item.expiry.After(finished.Add(tc.wantTTL))) + + // The failure is suppressed during the retry window. + got, changed = r.Process(ctx, ip) + assert.False(t, changed) + assert.Empty(t, got) + assert.Equal(t, 1, hits) + + // Simulate the retry delay passing and the private resolver becoming + // ready after AdGuard Home startup. + item.expiry = time.Now().Add(-time.Second) + available = true + + got, changed = r.Process(ctx, ip) + require.True(t, changed) + assert.Equal(t, wantHost, got) + assert.Equal(t, 2, hits) + }) + } +}