From f9fe3172c45cd22a55982801b59906de85845401 Mon Sep 17 00:00:00 2001 From: Stanislav Chzhen Date: Fri, 7 Apr 2023 13:17:40 +0300 Subject: [PATCH] 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 Date: Fri Apr 7 12:43:22 2023 +0300 Merge branch 'master' into 4299-querylog-stats-clients commit 61964fdd02221abbddedf2d6d02bb0bce6845362 Author: Stanislav Chzhen Date: Fri Apr 7 12:42:01 2023 +0300 dnsforward: imp code commit 7382168500bab6ca7494d39aabfc2d7bfceb5d24 Author: Stanislav Chzhen Date: Fri Apr 7 11:13:07 2023 +0300 all: imp code, chlog commit c7852902f635af6c296dcb6735f7b0bfb83f4e87 Merge: aa4dc0a5 a55cbbe7 Author: Stanislav Chzhen Date: Thu Apr 6 14:34:24 2023 +0300 Merge branch 'master' into 4299-querylog-stats-clients commit aa4dc0a54e95bc5b24718ec158340b631a822801 Author: Stanislav Chzhen Date: Thu Apr 6 12:54:02 2023 +0300 all: imp code commit dd541f0cd7ecbf0afcf10ccbd130fd1d1fa4c1c4 Author: Stanislav Chzhen Date: Fri Mar 31 13:01:53 2023 +0300 querylog: fix typo commit d2c8fdb35b04d27c8957fa027882fde704cc07be Merge: 83d0baa1 2eb3bf6e Author: Stanislav Chzhen Date: Fri Mar 31 12:36:49 2023 +0300 Merge branch 'master' into 4299-querylog-stats-clients commit 83d0baa1f1202f9c62d4be2041d7aed12ee9ab2c Author: Stanislav Chzhen Date: Fri Mar 31 12:35:15 2023 +0300 all: add tests commit a459f19f25cf9646d145813fe7834b2d9979c516 Author: Stanislav Chzhen Date: Wed Mar 29 16:51:53 2023 +0300 all: add clients querylog stats ignore --- CHANGELOG.md | 6 ++ internal/dnsforward/dns.go | 2 +- internal/dnsforward/stats.go | 20 +++++-- internal/dnsforward/stats_test.go | 4 +- internal/home/client.go | 2 + internal/home/clients.go | 26 ++++++++- internal/home/dns.go | 11 ++-- internal/querylog/client.go | 1 + internal/querylog/qlog.go | 11 +++- internal/querylog/qlog_test.go | 18 +++++- internal/querylog/querylog.go | 2 +- internal/stats/http_test.go | 13 +++-- internal/stats/stats.go | 27 +++++++-- internal/stats/stats_internal_test.go | 7 ++- internal/stats/stats_test.go | 79 ++++++++++++++++++++++++--- 15 files changed, 186 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 353e559e..34094633 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/dnsforward/dns.go b/internal/dnsforward/dns.go index 8aafaad6..f8ce7d6f 100644 --- a/internal/dnsforward/dns.go +++ b/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 diff --git a/internal/dnsforward/stats.go b/internal/dnsforward/stats.go index 3fb6e245..8557205b 100644 --- a/internal/dnsforward/stats.go +++ b/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) diff --git a/internal/dnsforward/stats_test.go b/internal/dnsforward/stats_test.go index 6fa82a0e..3d793f7a 100644 --- a/internal/dnsforward/stats_test.go +++ b/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 } diff --git a/internal/home/client.go b/internal/home/client.go index c3946ffb..5e56df19 100644 --- a/internal/home/client.go +++ b/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. diff --git a/internal/home/clients.go b/internal/home/clients.go index 58be4bde..1ea0247a 100644 --- a/internal/home/clients.go +++ b/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) @@ -352,7 +359,8 @@ func (clients *clientsContainer) clientOrArtificial( client, ok := clients.Find(id) if ok { return &querylog.Client{ - Name: client.Name, + 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. diff --git a/internal/home/dns.go b/internal/home/dns.go index b14fa440..9c3b03cc 100644 --- a/internal/home/dns.go +++ b/internal/home/dns.go @@ -51,11 +51,12 @@ func initDNS() (err error) { anonymizer := config.anonymizer() statsConf := stats.Config{ - Filename: filepath.Join(baseDir, "stats.db"), - Limit: config.Stats.Interval.Duration, - ConfigModified: onConfigModified, - HTTPRegister: httpRegister, - Enabled: config.Stats.Enabled, + Filename: filepath.Join(baseDir, "stats.db"), + Limit: config.Stats.Interval.Duration, + ConfigModified: onConfigModified, + HTTPRegister: httpRegister, + Enabled: config.Stats.Enabled, + ShouldCountClient: Context.clients.shouldCountClient, } set, err := aghnet.NewDomainNameSet(config.Stats.Ignored) diff --git a/internal/querylog/client.go b/internal/querylog/client.go index 92199f6d..f25440cc 100644 --- a/internal/querylog/client.go +++ b/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. diff --git a/internal/querylog/qlog.go b/internal/querylog/qlog.go index da205c87..0a283299 100644 --- a/internal/querylog/qlog.go +++ b/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) } diff --git a/internal/querylog/qlog_test.go b/internal/querylog/qlog_test.go index 9c637776..d8395c89 100644 --- a/internal/querylog/qlog_test.go +++ b/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) }) diff --git a/internal/querylog/querylog.go b/internal/querylog/querylog.go index ac507e3a..603b6cf6 100644 --- a/internal/querylog/querylog.go +++ b/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. diff --git a/internal/stats/http_test.go b/internal/stats/http_test.go index d9572240..8388fc74 100644 --- a/internal/stats/http_test.go +++ b/internal/stats/http_test.go @@ -24,18 +24,19 @@ func TestHandleStatsConfig(t *testing.T) { ) conf := Config{ - UnitID: func() (id uint32) { return 0 }, - ConfigModified: func() {}, - Filename: filepath.Join(t.TempDir(), "stats.db"), - Limit: time.Hour * 24, - Enabled: true, + 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, } testCases := []struct { name string + wantErr string body getConfigResp wantCode int - wantErr string }{{ name: "set_ivl_1_minIvl", body: getConfigResp{ diff --git a/internal/stats/stats.go b/internal/stats/stats.go index df901447..851aea98 100644 --- a/internal/stats/stats.go +++ b/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,16 +144,21 @@ 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, configModified: conf.ConfigModified, filename: conf.Filename, - confMu: &sync.RWMutex{}, - ignored: conf.Ignored, - limit: conf.Limit, - enabled: conf.Enabled, + confMu: &sync.RWMutex{}, + ignored: conf.Ignored, + shouldCountClient: conf.ShouldCountClient, + limit: conf.Limit, + enabled: conf.Enabled, } if s.unitIDGen = newUnitID; conf.UnitID != nil { @@ -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) } diff --git a/internal/stats/stats_internal_test.go b/internal/stats/stats_internal_test.go index 115805fc..9a173f65 100644 --- a/internal/stats/stats_internal_test.go +++ b/internal/stats/stats_internal_test.go @@ -36,9 +36,10 @@ func TestStats_races(t *testing.T) { var r uint32 idGen := func() (id uint32) { return atomic.LoadUint32(&r) } conf := Config{ - UnitID: idGen, - Filename: filepath.Join(t.TempDir(), "./stats.db"), - Limit: timeutil.Day, + ShouldCountClient: func([]string) bool { return true }, + UnitID: idGen, + Filename: filepath.Join(t.TempDir(), "./stats.db"), + Limit: timeutil.Day, } s, err := New(conf) diff --git a/internal/stats/stats_test.go b/internal/stats/stats_test.go index 36f9d102..37b82163 100644 --- a/internal/stats/stats_test.go +++ b/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,10 +54,11 @@ func TestStats(t *testing.T) { handlers := map[string]http.Handler{} conf := stats.Config{ - Filename: filepath.Join(t.TempDir(), "stats.db"), - Limit: timeutil.Day, - Enabled: true, - UnitID: constUnitID, + ShouldCountClient: func([]string) bool { return true }, + Filename: filepath.Join(t.TempDir(), "stats.db"), + Limit: timeutil.Day, + Enabled: true, + UnitID: constUnitID, HTTPRegister: func(_, url string, handler http.HandlerFunc) { handlers[url] = handler }, @@ -158,11 +161,12 @@ func TestLargeNumbers(t *testing.T) { handlers := map[string]http.Handler{} conf := stats.Config{ - Filename: filepath.Join(t.TempDir(), "stats.db"), - Limit: timeutil.Day, - Enabled: true, - UnitID: func() (id uint32) { return atomic.LoadUint32(&curHour) }, - HTTPRegister: func(_, url string, handler http.HandlerFunc) { handlers[url] = handler }, + ShouldCountClient: func([]string) bool { return true }, + Filename: filepath.Join(t.TempDir(), "stats.db"), + Limit: timeutil.Day, + Enabled: true, + UnitID: func() (id uint32) { return atomic.LoadUint32(&curHour) }, + HTTPRegister: func(_, url string, handler http.HandlerFunc) { handlers[url] = handler }, } s, err := stats.New(conf) @@ -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) + }) + } +}