From 738958d90a6fc3e413c5e6b7a521c5bf2c1c6084 Mon Sep 17 00:00:00 2001 From: Stanislav Chzhen Date: Mon, 26 Aug 2024 13:30:00 +0300 Subject: [PATCH] Pull request 2270: AGDNS-2374-slog-ipset Squashed commit of the following: commit 51ff7d8c49d174d057b4f508f3e113e1ca86bd1a Author: Stanislav Chzhen Date: Thu Aug 22 13:50:10 2024 +0300 dnsforward: imp code commit a1c0011273fc83ec1b509a9d930bca5e278e1e2c Author: Stanislav Chzhen Date: Wed Aug 21 21:53:01 2024 +0300 dnsforward: imp code commit a64fd6b3f037712927a583d04296fcaf821f6442 Author: Stanislav Chzhen Date: Wed Aug 21 21:28:48 2024 +0300 dnsforward: imp code commit 37ccae4e923a7e688e79a135b0e49a746e9b2a06 Author: Stanislav Chzhen Date: Wed Aug 21 20:23:58 2024 +0300 all: imp code commit 03c69ab2729eb424d768def986cba83731ad3e3b Author: Stanislav Chzhen Date: Wed Aug 21 19:08:30 2024 +0300 all: imp code commit 72adfb101fcdb42635702c1f1c4e13ddcc95bfdc Author: Stanislav Chzhen Date: Wed Aug 21 16:42:44 2024 +0300 all: slog ipset --- internal/dnsforward/config.go | 14 ++-- internal/dnsforward/dnsforward.go | 14 +++- internal/dnsforward/ipset.go | 87 ++++++++++++--------- internal/dnsforward/ipset_internal_test.go | 17 ++-- internal/ipset/ipset.go | 35 ++++++--- internal/ipset/ipset_linux.go | 55 ++++++++----- internal/ipset/ipset_linux_internal_test.go | 18 ++++- internal/ipset/ipset_others.go | 4 +- 8 files changed, 157 insertions(+), 87 deletions(-) diff --git a/internal/dnsforward/config.go b/internal/dnsforward/config.go index 7b1fe1f7..7e12e8c3 100644 --- a/internal/dnsforward/config.go +++ b/internal/dnsforward/config.go @@ -159,7 +159,7 @@ type Config struct { // IpsetList is the ipset configuration that allows AdGuard Home to add IP // addresses of the specified domain names to an ipset list. Syntax: // - // DOMAIN[,DOMAIN].../IPSET_NAME + // DOMAIN[,DOMAIN].../IPSET_NAME[,IPSET_NAME]... // // This field is ignored if [IpsetListFileName] is set. IpsetList []string `yaml:"ipset"` @@ -457,24 +457,24 @@ func (s *Server) initDefaultSettings() { // prepareIpsetListSettings reads and prepares the ipset configuration either // from a file or from the data in the configuration file. -func (s *Server) prepareIpsetListSettings() (err error) { +func (s *Server) prepareIpsetListSettings() (ipsets []string, err error) { fn := s.conf.IpsetListFileName if fn == "" { - return s.ipset.init(s.conf.IpsetList) + return s.conf.IpsetList, nil } // #nosec G304 -- Trust the path explicitly given by the user. data, err := os.ReadFile(fn) if err != nil { - return err + return nil, err } - ipsets := stringutil.SplitTrimmed(string(data), "\n") - ipsets = stringutil.FilterOut(ipsets, IsCommentOrEmpty) + ipsets = stringutil.SplitTrimmed(string(data), "\n") + ipsets = slices.DeleteFunc(ipsets, IsCommentOrEmpty) log.Debug("dns: using %d ipset rules from file %q", len(ipsets), fn) - return s.ipset.init(ipsets) + return ipsets, nil } // loadUpstreams parses upstream DNS servers from the configured file or from diff --git a/internal/dnsforward/dnsforward.go b/internal/dnsforward/dnsforward.go index 9ae6fc69..30d7e731 100644 --- a/internal/dnsforward/dnsforward.go +++ b/internal/dnsforward/dnsforward.go @@ -133,8 +133,9 @@ type Server struct { // must be a valid domain name plus dots on each side. localDomainSuffix string - // ipset processes DNS requests using ipset data. - ipset ipsetCtx + // ipset processes DNS requests using ipset data. It must not be nil after + // initialization. See [newIpsetHandler]. + ipset *ipsetHandler // privateNets is the configured set of IP networks considered private. privateNets netutil.SubnetSet @@ -609,11 +610,18 @@ func (s *Server) prepareLocalResolvers() (uc *proxy.UpstreamConfig, err error) { // the primary DNS proxy instance. It assumes s.serverLock is locked or the // Server not running. func (s *Server) prepareInternalDNS() (err error) { - err = s.prepareIpsetListSettings() + ipsetList, err := s.prepareIpsetListSettings() if err != nil { return fmt.Errorf("preparing ipset settings: %w", err) } + ipsetLogger := s.logger.With(slogutil.KeyPrefix, "ipset") + s.ipset, err = newIpsetHandler(context.TODO(), ipsetLogger, ipsetList) + if err != nil { + // Don't wrap the error, because it's informative enough as is. + return err + } + bootOpts := &upstream.Options{ Timeout: DefaultTimeout, HTTPVersions: UpstreamHTTPVersions(s.conf.UseHTTP3Upstreams), diff --git a/internal/dnsforward/ipset.go b/internal/dnsforward/ipset.go index cd841c34..7347890a 100644 --- a/internal/dnsforward/ipset.go +++ b/internal/dnsforward/ipset.go @@ -1,28 +1,43 @@ package dnsforward import ( + "context" "fmt" + "log/slog" "net" "os" "strings" "github.com/AdguardTeam/AdGuardHome/internal/ipset" "github.com/AdguardTeam/golibs/errors" - "github.com/AdguardTeam/golibs/log" + "github.com/AdguardTeam/golibs/logutil/slogutil" "github.com/miekg/dns" ) -// ipsetCtx is the ipset context. ipsetMgr can be nil. -type ipsetCtx struct { +// ipsetHandler is the ipset context. ipsetMgr can be nil. +type ipsetHandler struct { ipsetMgr ipset.Manager + logger *slog.Logger } -// init initializes the ipset context. It is not safe for concurrent use. -// -// TODO(a.garipov): Rewrite into a simple constructor? -func (c *ipsetCtx) init(ipsetConf []string) (err error) { - c.ipsetMgr, err = ipset.NewManager(ipsetConf) - if errors.Is(err, os.ErrInvalid) || errors.Is(err, os.ErrPermission) { +// newIpsetHandler returns a new initialized [ipsetHandler]. It is not safe for +// concurrent use. +func newIpsetHandler( + ctx context.Context, + logger *slog.Logger, + ipsetList []string, +) (h *ipsetHandler, err error) { + h = &ipsetHandler{ + logger: logger, + } + conf := &ipset.Config{ + Logger: logger, + Lines: ipsetList, + } + h.ipsetMgr, err = ipset.NewManager(ctx, conf) + if errors.Is(err, os.ErrInvalid) || + errors.Is(err, os.ErrPermission) || + errors.Is(err, errors.ErrUnsupported) { // ipset cannot currently be initialized if the server was installed // from Snap or when the user or the binary doesn't have the required // permissions, or when the kernel doesn't support netfilter. @@ -31,30 +46,28 @@ func (c *ipsetCtx) init(ipsetConf []string) (err error) { // // TODO(a.garipov): The Snap problem can probably be solved if we add // the netlink-connector interface plug. - log.Info("ipset: warning: cannot initialize: %s", err) + logger.WarnContext(ctx, "cannot initialize", slogutil.KeyError, err) - return nil - } else if errors.Is(err, errors.ErrUnsupported) { - log.Info("ipset: warning: %s", err) - - return nil + return h, nil } else if err != nil { - return fmt.Errorf("initializing ipset: %w", err) + return nil, fmt.Errorf("initializing ipset: %w", err) + } + + return h, nil +} + +// close closes the Linux Netfilter connections. close can be called on a nil +// handler. +func (h *ipsetHandler) close() (err error) { + if h != nil && h.ipsetMgr != nil { + return h.ipsetMgr.Close() } return nil } -// close closes the Linux Netfilter connections. -func (c *ipsetCtx) close() (err error) { - if c.ipsetMgr != nil { - return c.ipsetMgr.Close() - } - - return nil -} - -func (c *ipsetCtx) dctxIsfilled(dctx *dnsContext) (ok bool) { +// dctxIsFilled returns true if dctx has enough information to process. +func dctxIsFilled(dctx *dnsContext) (ok bool) { return dctx != nil && dctx.responseFromUpstream && dctx.proxyCtx != nil && @@ -65,8 +78,8 @@ func (c *ipsetCtx) dctxIsfilled(dctx *dnsContext) (ok bool) { // skipIpsetProcessing returns true when the ipset processing can be skipped for // this request. -func (c *ipsetCtx) skipIpsetProcessing(dctx *dnsContext) (ok bool) { - if c == nil || c.ipsetMgr == nil || !c.dctxIsfilled(dctx) { +func (h *ipsetHandler) skipIpsetProcessing(dctx *dnsContext) (ok bool) { + if h == nil || h.ipsetMgr == nil || !dctxIsFilled(dctx) { return true } @@ -108,31 +121,31 @@ func ipsFromAnswer(ans []dns.RR) (ip4s, ip6s []net.IP) { } // process adds the resolved IP addresses to the domain's ipsets, if any. -func (c *ipsetCtx) process(dctx *dnsContext) (rc resultCode) { - log.Debug("dnsforward: ipset: started processing") - defer log.Debug("dnsforward: ipset: finished processing") +func (h *ipsetHandler) process(dctx *dnsContext) (rc resultCode) { + // TODO(s.chzhen): Use passed context. + ctx := context.TODO() + h.logger.DebugContext(ctx, "started processing") + defer h.logger.DebugContext(ctx, "finished processing") - if c.skipIpsetProcessing(dctx) { + if h.skipIpsetProcessing(dctx) { return resultCodeSuccess } - log.Debug("ipset: starting processing") - req := dctx.proxyCtx.Req host := req.Question[0].Name host = strings.TrimSuffix(host, ".") host = strings.ToLower(host) ip4s, ip6s := ipsFromAnswer(dctx.proxyCtx.Res.Answer) - n, err := c.ipsetMgr.Add(host, ip4s, ip6s) + n, err := h.ipsetMgr.Add(ctx, host, ip4s, ip6s) if err != nil { // Consider ipset errors non-critical to the request. - log.Error("dnsforward: ipset: adding host ips: %s", err) + h.logger.ErrorContext(ctx, "adding host ips", slogutil.KeyError, err) return resultCodeSuccess } - log.Debug("dnsforward: ipset: added %d new ipset entries", n) + h.logger.DebugContext(ctx, "added new ipset entries", "num", n) return resultCodeSuccess } diff --git a/internal/dnsforward/ipset_internal_test.go b/internal/dnsforward/ipset_internal_test.go index cdeed21b..09601ac6 100644 --- a/internal/dnsforward/ipset_internal_test.go +++ b/internal/dnsforward/ipset_internal_test.go @@ -1,10 +1,12 @@ package dnsforward import ( + "context" "net" "testing" "github.com/AdguardTeam/dnsproxy/proxy" + "github.com/AdguardTeam/golibs/logutil/slogutil" "github.com/miekg/dns" "github.com/stretchr/testify/assert" ) @@ -16,7 +18,7 @@ type fakeIpsetMgr struct { } // Add implements the aghnet.IpsetManager interface for *fakeIpsetMgr. -func (m *fakeIpsetMgr) Add(host string, ip4s, ip6s []net.IP) (n int, err error) { +func (m *fakeIpsetMgr) Add(_ context.Context, host string, ip4s, ip6s []net.IP) (n int, err error) { m.ip4s = append(m.ip4s, ip4s...) m.ip6s = append(m.ip6s, ip6s...) @@ -58,7 +60,9 @@ func TestIpsetCtx_process(t *testing.T) { responseFromUpstream: true, } - ictx := &ipsetCtx{} + ictx := &ipsetHandler{ + logger: slogutil.NewDiscardLogger(), + } rc := ictx.process(dctx) assert.Equal(t, resultCodeSuccess, rc) @@ -77,8 +81,9 @@ func TestIpsetCtx_process(t *testing.T) { } m := &fakeIpsetMgr{} - ictx := &ipsetCtx{ + ictx := &ipsetHandler{ ipsetMgr: m, + logger: slogutil.NewDiscardLogger(), } rc := ictx.process(dctx) @@ -101,8 +106,9 @@ func TestIpsetCtx_process(t *testing.T) { } m := &fakeIpsetMgr{} - ictx := &ipsetCtx{ + ictx := &ipsetHandler{ ipsetMgr: m, + logger: slogutil.NewDiscardLogger(), } rc := ictx.process(dctx) @@ -124,8 +130,9 @@ func TestIpsetCtx_SkipIpsetProcessing(t *testing.T) { } m := &fakeIpsetMgr{} - ictx := &ipsetCtx{ + ictx := &ipsetHandler{ ipsetMgr: m, + logger: slogutil.NewDiscardLogger(), } testCases := []struct { diff --git a/internal/ipset/ipset.go b/internal/ipset/ipset.go index 44f519f1..839497df 100644 --- a/internal/ipset/ipset.go +++ b/internal/ipset/ipset.go @@ -2,6 +2,8 @@ package ipset import ( + "context" + "log/slog" "net" ) @@ -10,24 +12,33 @@ import ( // TODO(a.garipov): Perhaps generalize this into some kind of a NetFilter type, // since ipset is exclusive to Linux? type Manager interface { - Add(host string, ip4s, ip6s []net.IP) (n int, err error) + Add(ctx context.Context, host string, ip4s, ip6s []net.IP) (n int, err error) Close() (err error) } -// NewManager returns a new ipset manager. IPv4 addresses are added to an -// ipset with an ipv4 family; IPv6 addresses, to an ipv6 ipset. ipset must -// exist. +// Config is the configuration structure for the ipset manager. +type Config struct { + // Logger is used for logging the operation of the ipset manager. It must + // not be nil. + Logger *slog.Logger + + // Lines is the ipset configuration with the following syntax: + // + // DOMAIN[,DOMAIN].../IPSET_NAME[,IPSET_NAME]... + // + // Lines must not contain any blank lines or comments. + Lines []string +} + +// NewManager returns a new ipset manager. IPv4 addresses are added to an ipset +// with an ipv4 family; IPv6 addresses, to an ipv6 ipset. ipset must exist. // -// The syntax of the ipsetConf is: -// -// DOMAIN[,DOMAIN].../IPSET_NAME[,IPSET_NAME]... -// -// If ipsetConf is empty, msg and err are nil. The error's chain contains +// If conf.Lines is empty, mgr and err are nil. The error's chain contains // [errors.ErrUnsupported] if current OS is not supported. -func NewManager(ipsetConf []string) (mgr Manager, err error) { - if len(ipsetConf) == 0 { +func NewManager(ctx context.Context, conf *Config) (mgr Manager, err error) { + if len(conf.Lines) == 0 { return nil, nil } - return newManager(ipsetConf) + return newManager(ctx, conf) } diff --git a/internal/ipset/ipset_linux.go b/internal/ipset/ipset_linux.go index 12dcecea..1d15428e 100644 --- a/internal/ipset/ipset_linux.go +++ b/internal/ipset/ipset_linux.go @@ -4,14 +4,16 @@ package ipset import ( "bytes" + "context" "fmt" + "log/slog" "net" "strings" "sync" "github.com/AdguardTeam/golibs/container" "github.com/AdguardTeam/golibs/errors" - "github.com/AdguardTeam/golibs/log" + "github.com/AdguardTeam/golibs/logutil/slogutil" "github.com/digineo/go-ipset/v2" "github.com/mdlayher/netlink" "github.com/ti-mo/netfilter" @@ -34,8 +36,8 @@ import ( // resolved IP addresses. // newManager returns a new Linux ipset manager. -func newManager(ipsetConf []string) (set Manager, err error) { - return newManagerWithDialer(ipsetConf, defaultDial) +func newManager(ctx context.Context, conf *Config) (set Manager, err error) { + return newManagerWithDialer(ctx, conf, defaultDial) } // defaultDial is the default netfilter dialing function. @@ -180,6 +182,8 @@ type manager struct { nameToIpset map[string]props domainToIpsets map[string][]props + logger *slog.Logger + dial dialer // mu protects all properties below. @@ -254,7 +258,7 @@ func parseIpsetConfigLine(confStr string) (hosts, ipsetNames []string, err error // parseIpsetConfig parses the ipset configuration and stores ipsets. It // returns an error if the configuration can't be used. -func (m *manager) parseIpsetConfig(ipsetConf []string) (err error) { +func (m *manager) parseIpsetConfig(ctx context.Context, ipsetConf []string) (err error) { // The family doesn't seem to matter when we use a header query, so query // only the IPv4 one. // @@ -278,7 +282,7 @@ func (m *manager) parseIpsetConfig(ipsetConf []string) (err error) { } var ipsets []props - ipsets, err = m.ipsets(ipsetNames, currentlyKnown) + ipsets, err = m.ipsets(ctx, ipsetNames, currentlyKnown) if err != nil { return fmt.Errorf("getting ipsets from config line at idx %d: %w", i, err) } @@ -328,7 +332,11 @@ func (m *manager) ipsetProps(name string) (p props, err error) { // ipsets returns ipset properties of currently known ipsets. It also makes an // additional ipset header data query if needed. -func (m *manager) ipsets(names []string, currentlyKnown map[string]props) (sets []props, err error) { +func (m *manager) ipsets( + ctx context.Context, + names []string, + currentlyKnown map[string]props, +) (sets []props, err error) { for _, n := range names { p, ok := currentlyKnown[n] if !ok { @@ -336,10 +344,12 @@ func (m *manager) ipsets(names []string, currentlyKnown map[string]props) (sets } if p.family != netfilter.ProtoIPv4 && p.family != netfilter.ProtoIPv6 { - log.Debug("ipset: getting properties: %q %q unexpected ipset family %q", - p.name, - p.typeName, - p.family, + m.logger.DebugContext( + ctx, + "got unexpected ipset family while getting set properties", + "set_name", p.name, + "set_type", p.typeName, + "set_family", p.family, ) p, err = m.ipsetProps(n) @@ -357,7 +367,7 @@ func (m *manager) ipsets(names []string, currentlyKnown map[string]props) (sets // newManagerWithDialer returns a new Linux ipset manager using the provided // dialer. -func newManagerWithDialer(ipsetConf []string, dial dialer) (mgr Manager, err error) { +func newManagerWithDialer(ctx context.Context, conf *Config, dial dialer) (mgr Manager, err error) { defer func() { err = errors.Annotate(err, "ipset: %w") }() m := &manager{ @@ -366,6 +376,8 @@ func newManagerWithDialer(ipsetConf []string, dial dialer) (mgr Manager, err err nameToIpset: make(map[string]props), domainToIpsets: make(map[string][]props), + logger: conf.Logger, + dial: dial, addedIPs: container.NewMapSet[ipInIpsetEntry](), @@ -376,7 +388,7 @@ func newManagerWithDialer(ipsetConf []string, dial dialer) (mgr Manager, err err if errors.Is(err, unix.EPROTONOSUPPORT) { // The implementation doesn't support this protocol version. Just // issue a warning. - log.Info("ipset: dialing netfilter: warning: %s", err) + m.logger.WarnContext(ctx, "dialing netfilter", slogutil.KeyError, err) return nil, nil } @@ -384,12 +396,12 @@ func newManagerWithDialer(ipsetConf []string, dial dialer) (mgr Manager, err err return nil, fmt.Errorf("dialing netfilter: %w", err) } - err = m.parseIpsetConfig(ipsetConf) + err = m.parseIpsetConfig(ctx, conf.Lines) if err != nil { return nil, fmt.Errorf("getting ipsets: %w", err) } - log.Debug("ipset: initialized") + m.logger.DebugContext(ctx, "initialized") return m, nil } @@ -476,6 +488,7 @@ func (m *manager) addIPs(host string, set props, ips []net.IP) (n int, err error // addToSets adds the IP addresses to the corresponding ipset. func (m *manager) addToSets( + ctx context.Context, host string, ip4s []net.IP, ip6s []net.IP, @@ -498,7 +511,13 @@ func (m *manager) addToSets( return n, fmt.Errorf("%q %q unexpected family %q", set.name, set.typeName, set.family) } - log.Debug("ipset: added %d ips to set %q %q", nn, set.name, set.typeName) + m.logger.DebugContext( + ctx, + "added ips to set", + "ips_num", nn, + "set_name", set.name, + "set_type", set.typeName, + ) n += nn } @@ -507,7 +526,7 @@ func (m *manager) addToSets( } // Add implements the [Manager] interface for *manager. -func (m *manager) Add(host string, ip4s, ip6s []net.IP) (n int, err error) { +func (m *manager) Add(ctx context.Context, host string, ip4s, ip6s []net.IP) (n int, err error) { m.mu.Lock() defer m.mu.Unlock() @@ -516,9 +535,9 @@ func (m *manager) Add(host string, ip4s, ip6s []net.IP) (n int, err error) { return 0, nil } - log.Debug("ipset: found %d sets", len(sets)) + m.logger.DebugContext(ctx, "found sets", "set_num", len(sets)) - return m.addToSets(host, ip4s, ip6s, sets) + return m.addToSets(ctx, host, ip4s, ip6s, sets) } // Close implements the [Manager] interface for *manager. diff --git a/internal/ipset/ipset_linux_internal_test.go b/internal/ipset/ipset_linux_internal_test.go index 4d727ee7..632f0d0f 100644 --- a/internal/ipset/ipset_linux_internal_test.go +++ b/internal/ipset/ipset_linux_internal_test.go @@ -6,8 +6,11 @@ import ( "net" "strings" "testing" + "time" "github.com/AdguardTeam/golibs/errors" + "github.com/AdguardTeam/golibs/logutil/slogutil" + "github.com/AdguardTeam/golibs/testutil" "github.com/digineo/go-ipset/v2" "github.com/mdlayher/netlink" "github.com/stretchr/testify/assert" @@ -15,6 +18,9 @@ import ( "github.com/ti-mo/netfilter" ) +// testTimeout is a common timeout for tests and contexts. +const testTimeout = 1 * time.Second + // fakeConn is a fake ipsetConn for tests. type fakeConn struct { ipv4Header *ipset.HeaderPolicy @@ -58,7 +64,7 @@ func (c *fakeConn) listAll() (sets []props, err error) { } func TestManager_Add(t *testing.T) { - ipsetConf := []string{ + ipsetList := []string{ "example.com,example.net/ipv4set", "example.org,example.biz/ipv6set", } @@ -89,7 +95,11 @@ func TestManager_Add(t *testing.T) { }, nil } - m, err := newManagerWithDialer(ipsetConf, fakeDial) + conf := &Config{ + Logger: slogutil.NewDiscardLogger(), + Lines: ipsetList, + } + m, err := newManagerWithDialer(testutil.ContextWithTimeout(t, testTimeout), conf, fakeDial) require.NoError(t, err) ip4 := net.IP{1, 2, 3, 4} @@ -100,7 +110,7 @@ func TestManager_Add(t *testing.T) { 0x00, 0x00, 0x56, 0x78, } - n, err := m.Add("example.net", []net.IP{ip4}, nil) + n, err := m.Add(testutil.ContextWithTimeout(t, testTimeout), "example.net", []net.IP{ip4}, nil) require.NoError(t, err) assert.Equal(t, 1, n) @@ -110,7 +120,7 @@ func TestManager_Add(t *testing.T) { gotIP4 := ipv4Entries[0].IP.Value assert.Equal(t, ip4, gotIP4) - n, err = m.Add("example.biz", nil, []net.IP{ip6}) + n, err = m.Add(testutil.ContextWithTimeout(t, testTimeout), "example.biz", nil, []net.IP{ip6}) require.NoError(t, err) assert.Equal(t, 1, n) diff --git a/internal/ipset/ipset_others.go b/internal/ipset/ipset_others.go index 8b75912f..29038b9d 100644 --- a/internal/ipset/ipset_others.go +++ b/internal/ipset/ipset_others.go @@ -3,9 +3,11 @@ package ipset import ( + "context" + "github.com/AdguardTeam/AdGuardHome/internal/aghos" ) -func newManager(_ []string) (mgr Manager, err error) { +func newManager(_ context.Context, _ *Config) (mgr Manager, err error) { return nil, aghos.Unsupported("ipset") }