Pull request 1791: 4299-querylog-stats-clients

Merge in DNS/adguard-home from 4299-querylog-stats-clients to master

Squashed commit of the following:

commit 33b80b67224f7c1a15bee8e6a23d9d5bab6ac629
Merge: 61964fdd 5d5a7295
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Fri Apr 7 12:43:22 2023 +0300

    Merge branch 'master' into 4299-querylog-stats-clients

commit 61964fdd02221abbddedf2d6d02bb0bce6845362
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Fri Apr 7 12:42:01 2023 +0300

    dnsforward: imp code

commit 7382168500bab6ca7494d39aabfc2d7bfceb5d24
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Fri Apr 7 11:13:07 2023 +0300

    all: imp code, chlog

commit c7852902f6
Merge: aa4dc0a5 a55cbbe7
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Thu Apr 6 14:34:24 2023 +0300

    Merge branch 'master' into 4299-querylog-stats-clients

commit aa4dc0a54e
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Thu Apr 6 12:54:02 2023 +0300

    all: imp code

commit dd541f0cd7ecbf0afcf10ccbd130fd1d1fa4c1c4
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Fri Mar 31 13:01:53 2023 +0300

    querylog: fix typo

commit d2c8fdb35b04d27c8957fa027882fde704cc07be
Merge: 83d0baa1 2eb3bf6e
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Fri Mar 31 12:36:49 2023 +0300

    Merge branch 'master' into 4299-querylog-stats-clients

commit 83d0baa1f1202f9c62d4be2041d7aed12ee9ab2c
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Fri Mar 31 12:35:15 2023 +0300

    all: add tests

commit a459f19f25cf9646d145813fe7834b2d9979c516
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Wed Mar 29 16:51:53 2023 +0300

    all: add clients querylog stats ignore

CHANGELOG.md

@@ -25,6 +25,10 @@ NOTE: Add new changes BELOW THIS COMMENT.
 
 ### Added
 
+- The ability to exclude client activity from the query log or statistics by
+  using the new properties `ignore_querylog` and `ignore_statistics` of the
+  items of the `clients.persistent` array ([#1717], [#4299]).  The UI changes
+  are coming in the upcoming releases.
 - Better profiling information when `debug_pprof` is set to `true`.
 - IPv6 support in Safe Search for some services.
 - The ability to make bootstrap DNS lookups prefer IPv6 addresses to IPv4 ones
@@ -131,10 +135,12 @@ In this release, the schema version has changed from 17 to 20.
 
 [#1163]: https://github.com/AdguardTeam/AdGuardHome/issues/1163
 [#1333]: https://github.com/AdguardTeam/AdGuardHome/issues/1333
+[#1163]: https://github.com/AdguardTeam/AdGuardHome/issues/1717
 [#1472]: https://github.com/AdguardTeam/AdGuardHome/issues/1472
 [#3290]: https://github.com/AdguardTeam/AdGuardHome/issues/3290
 [#3459]: https://github.com/AdguardTeam/AdGuardHome/issues/3459
 [#4262]: https://github.com/AdguardTeam/AdGuardHome/issues/4262
+[#3290]: https://github.com/AdguardTeam/AdGuardHome/issues/4299
 [#5567]: https://github.com/AdguardTeam/AdGuardHome/issues/5567
 
 [rfc6761]:   https://www.rfc-editor.org/rfc/rfc6761

internal/dnsforward/dns.go

@@ -22,7 +22,7 @@ import (
 // To transfer information between modules
 //
 // TODO(s.chzhen):  Add lowercased, non-FQDN version of the hostname from the
-// question of the request.
+// question of the request.  Add persistent client.
 type dnsContext struct {
 	proxyCtx *proxy.DNSContext
 

internal/dnsforward/stats.go

@@ -40,12 +40,17 @@ func (s *Server) processQueryLogsAndStats(dctx *dnsContext) (rc resultCode) {
 
 	log.Debug("client ip: %s", ip)
 
+	ipStr := ip.String()
+	ids := []string{ipStr, dctx.clientID}
+
 	// Synchronize access to s.queryLog and s.stats so they won't be suddenly
 	// uninitialized while in use.  This can happen after proxy server has been
 	// stopped, but its workers haven't yet exited.
 	if shouldLog &&
 		s.queryLog != nil &&
-		s.queryLog.ShouldLog(host, q.Qtype, q.Qclass) {
+		// TODO(s.chzhen):  Use dnsforward.dnsContext when it will start
+		// containing persistent client.
+		s.queryLog.ShouldLog(host, q.Qtype, q.Qclass, ids) {
 		s.logQuery(dctx, pctx, elapsed, ip)
 	} else {
 		log.Debug(
@@ -56,8 +61,11 @@ func (s *Server) processQueryLogsAndStats(dctx *dnsContext) (rc resultCode) {
 		)
 	}
 
-	if s.stats != nil && s.stats.ShouldCount(host, q.Qtype, q.Qclass) {
-		s.updateStats(dctx, elapsed, *dctx.result, ip)
+	if s.stats != nil &&
+		// TODO(s.chzhen):  Use dnsforward.dnsContext when it will start
+		// containing persistent client.
+		s.stats.ShouldCount(host, q.Qtype, q.Qclass, ids) {
+		s.updateStats(dctx, elapsed, *dctx.result, ipStr)
 	}
 
 	return resultCodeSuccess
@@ -110,7 +118,7 @@ func (s *Server) updateStats(
 	ctx *dnsContext,
 	elapsed time.Duration,
 	res filtering.Result,
-	clientIP net.IP,
+	clientIP string,
 ) {
 	pctx := ctx.proxyCtx
 	e := stats.Entry{}
@@ -119,8 +127,8 @@ func (s *Server) updateStats(
 
 	if clientID := ctx.clientID; clientID != "" {
 		e.Client = clientID
-	} else if clientIP != nil {
-		e.Client = clientIP.String()
+	} else {
+		e.Client = clientIP
 	}
 
 	e.Time = uint32(elapsed / 1000)

internal/dnsforward/stats_test.go

@@ -31,7 +31,7 @@ func (l *testQueryLog) Add(p *querylog.AddParams) {
 }
 
 // ShouldLog implements the [querylog.QueryLog] interface for *testQueryLog.
-func (l *testQueryLog) ShouldLog(string, uint16, uint16) bool {
+func (l *testQueryLog) ShouldLog(string, uint16, uint16, []string) bool {
 	return true
 }
 
@@ -50,7 +50,7 @@ func (l *testStats) Update(e stats.Entry) {
 }
 
 // ShouldCount implements the [stats.Interface] interface for *testStats.
-func (l *testStats) ShouldCount(string, uint16, uint16) bool {
+func (l *testStats) ShouldCount(string, uint16, uint16, []string) bool {
 	return true
 }
 

internal/home/client.go

@@ -33,6 +33,8 @@ type Client struct {
 	SafeBrowsingEnabled   bool
 	ParentalEnabled       bool
 	UseOwnBlockedServices bool
+	IgnoreQueryLog        bool
+	IgnoreStatistics      bool
 }
 
 // closeUpstreams closes the client-specific upstream config of c if any.

internal/home/clients.go

@@ -51,7 +51,7 @@ type clientsContainer struct {
 	// lock protects all fields.
 	//
 	// TODO(a.garipov): Use a pointer and describe which fields are protected in
-	// more detail.
+	// more detail.  Use sync.RWMutex.
 	lock sync.Mutex
 
 	// safeSearchCacheSize is the size of the safe search cache to use for
@@ -159,6 +159,9 @@ type clientObject struct {
 	ParentalEnabled          bool `yaml:"parental_enabled"`
 	SafeBrowsingEnabled      bool `yaml:"safebrowsing_enabled"`
 	UseGlobalBlockedServices bool `yaml:"use_global_blocked_services"`
+
+	IgnoreQueryLog   bool `yaml:"ignore_querylog"`
+	IgnoreStatistics bool `yaml:"ignore_statistics"`
 }
 
 // addFromConfig initializes the clients container with objects from the
@@ -177,6 +180,8 @@ func (clients *clientsContainer) addFromConfig(objects []*clientObject, filterin
 			safeSearchConf:        o.SafeSearchConf,
 			SafeBrowsingEnabled:   o.SafeBrowsingEnabled,
 			UseOwnBlockedServices: !o.UseGlobalBlockedServices,
+			IgnoreQueryLog:        o.IgnoreQueryLog,
+			IgnoreStatistics:      o.IgnoreStatistics,
 		}
 
 		if o.SafeSearchConf.Enabled {
@@ -241,6 +246,8 @@ func (clients *clientsContainer) forConfig() (objs []*clientObject) {
 			SafeSearchConf:           cli.safeSearchConf,
 			SafeBrowsingEnabled:      cli.SafeBrowsingEnabled,
 			UseGlobalBlockedServices: !cli.UseOwnBlockedServices,
+			IgnoreQueryLog:           cli.IgnoreQueryLog,
+			IgnoreStatistics:         cli.IgnoreStatistics,
 		}
 
 		objs = append(objs, o)
@@ -353,6 +360,7 @@ func (clients *clientsContainer) clientOrArtificial(
 	if ok {
 		return &querylog.Client{
 			Name:           client.Name,
+			IgnoreQueryLog: client.IgnoreQueryLog,
 		}, false
 	}
 
@@ -387,6 +395,20 @@ func (clients *clientsContainer) Find(id string) (c *Client, ok bool) {
 	return c, true
 }
 
+// shouldCountClient is a wrapper around Find to make it a valid client
+// information finder for the statistics.  If no information about the client
+// is found, it returns true.
+func (clients *clientsContainer) shouldCountClient(ids []string) (y bool) {
+	for _, id := range ids {
+		client, ok := clients.Find(id)
+		if ok {
+			return !client.IgnoreStatistics
+		}
+	}
+
+	return true
+}
+
 // findUpstreams returns upstreams configured for the client, identified either
 // by its IP address or its ClientID.  upsConf is nil if the client isn't found
 // or if the client has no custom upstreams.

internal/home/dns.go

@@ -56,6 +56,7 @@ func initDNS() (err error) {
 		ConfigModified:    onConfigModified,
 		HTTPRegister:      httpRegister,
 		Enabled:           config.Stats.Enabled,
+		ShouldCountClient: Context.clients.shouldCountClient,
 	}
 
 	set, err := aghnet.NewDomainNameSet(config.Stats.Ignored)

internal/querylog/client.go

@@ -7,6 +7,7 @@ type Client struct {
 	Name           string       `json:"name"`
 	DisallowedRule string       `json:"disallowed_rule"`
 	Disallowed     bool         `json:"disallowed"`
+	IgnoreQueryLog bool         `json:"-"`
 }
 
 // ClientWHOIS is the filtered WHOIS data for the client.

internal/querylog/qlog.go

@@ -247,10 +247,19 @@ func (l *queryLog) Add(params *AddParams) {
 }
 
 // ShouldLog returns true if request for the host should be logged.
-func (l *queryLog) ShouldLog(host string, _, _ uint16) bool {
+func (l *queryLog) ShouldLog(host string, _, _ uint16, ids []string) bool {
 	l.confMu.RLock()
 	defer l.confMu.RUnlock()
 
+	c, err := l.findClient(ids)
+	if err != nil {
+		log.Error("querylog: finding client: %s", err)
+	}
+
+	if c != nil && c.IgnoreQueryLog {
+		return false
+	}
+
 	return !l.isIgnored(host)
 }
 

internal/querylog/qlog_test.go

@@ -258,36 +258,52 @@ func TestQueryLogShouldLog(t *testing.T) {
 	)
 	set := stringutil.NewSet(ignored1, ignored2)
 
+	findClient := func(ids []string) (c *Client, err error) {
+		log := ids[0] == "no_log"
+
+		return &Client{IgnoreQueryLog: log}, nil
+	}
+
 	l, err := newQueryLog(Config{
 		Ignored:     set,
 		Enabled:     true,
 		RotationIvl: timeutil.Day,
 		MemSize:     100,
 		BaseDir:     t.TempDir(),
+		FindClient:  findClient,
 	})
 	require.NoError(t, err)
 
 	testCases := []struct {
 		name    string
 		host    string
+		ids     []string
 		wantLog bool
 	}{{
 		name:    "log",
 		host:    "example.com",
+		ids:     []string{"whatever"},
 		wantLog: true,
 	}, {
 		name:    "no_log_ignored_1",
 		host:    ignored1,
+		ids:     []string{"whatever"},
 		wantLog: false,
 	}, {
 		name:    "no_log_ignored_2",
 		host:    ignored2,
+		ids:     []string{"whatever"},
+		wantLog: false,
+	}, {
+		name:    "no_log_client_ignore",
+		host:    "example.com",
+		ids:     []string{"no_log"},
 		wantLog: false,
 	}}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			res := l.ShouldLog(tc.host, dns.TypeA, dns.ClassINET)
+			res := l.ShouldLog(tc.host, dns.TypeA, dns.ClassINET, tc.ids)
 
 			assert.Equal(t, tc.wantLog, res)
 		})

internal/querylog/querylog.go

@@ -29,7 +29,7 @@ type QueryLog interface {
 	WriteDiskConfig(c *Config)
 
 	// ShouldLog returns true if request for the host should be logged.
-	ShouldLog(host string, qType, qClass uint16) bool
+	ShouldLog(host string, qType, qClass uint16, ids []string) bool
 }
 
 // Config is the query log configuration structure.

internal/stats/http_test.go

@@ -26,6 +26,7 @@ func TestHandleStatsConfig(t *testing.T) {
 	conf := Config{
 		UnitID:            func() (id uint32) { return 0 },
 		ConfigModified:    func() {},
+		ShouldCountClient: func([]string) bool { return true },
 		Filename:          filepath.Join(t.TempDir(), "stats.db"),
 		Limit:             time.Hour * 24,
 		Enabled:           true,
@@ -33,9 +34,9 @@ func TestHandleStatsConfig(t *testing.T) {
 
 	testCases := []struct {
 		name     string
+		wantErr  string
 		body     getConfigResp
 		wantCode int
-		wantErr  string
 	}{{
 		name: "set_ivl_1_minIvl",
 		body: getConfigResp{

internal/stats/stats.go

@@ -52,6 +52,9 @@ type Config struct {
 	// interface.
 	ConfigModified func()
 
+	// ShouldCountClient returns client's ignore setting.
+	ShouldCountClient func([]string) bool
+
 	// HTTPRegister is the function that registers handlers for the stats
 	// endpoints.
 	HTTPRegister aghhttp.RegisterFunc
@@ -87,7 +90,7 @@ type Interface interface {
 	WriteDiskConfig(dc *Config)
 
 	// ShouldCount returns true if request for the host should be counted.
-	ShouldCount(host string, qType, qClass uint16) bool
+	ShouldCount(host string, qType, qClass uint16, ids []string) bool
 }
 
 // StatsCtx collects the statistics and flushes it to the database.  Its default
@@ -118,6 +121,9 @@ type StatsCtx struct {
 	// ignored is the list of host names, which should not be counted.
 	ignored *stringutil.Set
 
+	// shouldCountClient returns client's ignore setting.
+	shouldCountClient func([]string) bool
+
 	// filename is the name of database file.
 	filename string
 
@@ -138,6 +144,10 @@ func New(conf Config) (s *StatsCtx, err error) {
 		return nil, fmt.Errorf("unsupported interval: %w", err)
 	}
 
+	if conf.ShouldCountClient == nil {
+		return nil, errors.Error("should count client is unspecified")
+	}
+
 	s = &StatsCtx{
 		currMu:         &sync.RWMutex{},
 		httpRegister:   conf.HTTPRegister,
@@ -146,6 +156,7 @@ func New(conf Config) (s *StatsCtx, err error) {
 
 		confMu:            &sync.RWMutex{},
 		ignored:           conf.Ignored,
+		shouldCountClient: conf.ShouldCountClient,
 		limit:             conf.Limit,
 		enabled:           conf.Enabled,
 	}
@@ -577,10 +588,14 @@ func (s *StatsCtx) loadUnits(limit uint32) (units []*unitDB, firstID uint32) {
 }
 
 // ShouldCount returns true if request for the host should be counted.
-func (s *StatsCtx) ShouldCount(host string, _, _ uint16) bool {
+func (s *StatsCtx) ShouldCount(host string, _, _ uint16, ids []string) bool {
 	s.confMu.RLock()
 	defer s.confMu.RUnlock()
 
+	if !s.shouldCountClient(ids) {
+		return false
+	}
+
 	return !s.isIgnored(host)
 }
 

internal/stats/stats_internal_test.go

@@ -36,6 +36,7 @@ func TestStats_races(t *testing.T) {
 	var r uint32
 	idGen := func() (id uint32) { return atomic.LoadUint32(&r) }
 	conf := Config{
+		ShouldCountClient: func([]string) bool { return true },
 		UnitID:            idGen,
 		Filename:          filepath.Join(t.TempDir(), "./stats.db"),
 		Limit:             timeutil.Day,

internal/stats/stats_test.go

@@ -12,8 +12,10 @@ import (
 
 	"github.com/AdguardTeam/AdGuardHome/internal/stats"
 	"github.com/AdguardTeam/golibs/netutil"
+	"github.com/AdguardTeam/golibs/stringutil"
 	"github.com/AdguardTeam/golibs/testutil"
 	"github.com/AdguardTeam/golibs/timeutil"
+	"github.com/miekg/dns"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -52,6 +54,7 @@ func TestStats(t *testing.T) {
 
 	handlers := map[string]http.Handler{}
 	conf := stats.Config{
+		ShouldCountClient: func([]string) bool { return true },
 		Filename:          filepath.Join(t.TempDir(), "stats.db"),
 		Limit:             timeutil.Day,
 		Enabled:           true,
@@ -158,6 +161,7 @@ func TestLargeNumbers(t *testing.T) {
 	handlers := map[string]http.Handler{}
 
 	conf := stats.Config{
+		ShouldCountClient: func([]string) bool { return true },
 		Filename:          filepath.Join(t.TempDir(), "stats.db"),
 		Limit:             timeutil.Day,
 		Enabled:           true,
@@ -197,3 +201,60 @@ func TestLargeNumbers(t *testing.T) {
 	assertSuccessAndUnmarshal(t, data, handlers["/control/stats"], req)
 	assert.Equal(t, hoursNum*cliNumPerHour, int(data.NumDNSQueries))
 }
+
+func TestShouldCount(t *testing.T) {
+	const (
+		ignored1 = "ignor.ed"
+		ignored2 = "ignored.to"
+	)
+	set := stringutil.NewSet(ignored1, ignored2)
+
+	s, err := stats.New(stats.Config{
+		Enabled:  true,
+		Filename: filepath.Join(t.TempDir(), "stats.db"),
+		Limit:    timeutil.Day,
+		Ignored:  set,
+		ShouldCountClient: func(ids []string) (a bool) {
+			return ids[0] != "no_count"
+		},
+	})
+	require.NoError(t, err)
+
+	s.Start()
+	testutil.CleanupAndRequireSuccess(t, s.Close)
+
+	testCases := []struct {
+		wantCount assert.BoolAssertionFunc
+		name      string
+		host      string
+		ids       []string
+	}{{
+		name:      "count",
+		host:      "example.com",
+		ids:       []string{"whatever"},
+		wantCount: assert.True,
+	}, {
+		name:      "no_count_ignored_1",
+		host:      ignored1,
+		ids:       []string{"whatever"},
+		wantCount: assert.False,
+	}, {
+		name:      "no_count_ignored_2",
+		host:      ignored2,
+		ids:       []string{"whatever"},
+		wantCount: assert.False,
+	}, {
+		name:      "no_count_client_ignore",
+		host:      "example.com",
+		ids:       []string{"no_count"},
+		wantCount: assert.False,
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			res := s.ShouldCount(tc.host, dns.TypeA, dns.ClassINET, tc.ids)
+
+			tc.wantCount(t, res)
+		})
+	}
+}