all: imp code

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"`
@@ -470,7 +470,7 @@ func (s *Server) prepareIpsetListSettings() (ipsets []string, err error) {
 	}
 
 	ipsets = stringutil.SplitTrimmed(string(data), "\n")
-	ipsets = stringutil.FilterOut(ipsets, IsCommentOrEmpty)
+	slices.DeleteFunc(ipsets, IsCommentOrEmpty)
 
 	log.Debug("dns: using %d ipset rules from file %q", len(ipsets), fn)
 

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 processes DNS requests using ipset data.  It must not be nil after
-	ipset *ipsetCtx
+	// initialization.  See [newIpsetHandler].
+	ipset *ipsetHandler
 
 	// privateNets is the configured set of IP networks considered private.
 	privateNets netutil.SubnetSet
@@ -609,12 +610,13 @@ 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) {
-	ipsetConf, err := s.prepareIpsetListSettings()
+	ipsetList, err := s.prepareIpsetListSettings()
 	if err != nil {
 		return fmt.Errorf("preparing ipset settings: %w", err)
 	}
 
-	s.ipset, err = newIPSetCtx(s.logger, ipsetConf)
+	ipsetLogger := s.logger.With(slogutil.KeyPrefix, "ipset")
+	s.ipset, err = newIpsetHandler(ipsetLogger, ipsetList)
 	if err != nil {
 		// Don't wrap the error, because it's informative enough as is.
 		return err

internal/dnsforward/ipset.go

@@ -1,6 +1,7 @@
 package dnsforward
 
 import (
+	"context"
 	"fmt"
 	"log/slog"
 	"net"
@@ -13,21 +14,25 @@ import (
 	"github.com/miekg/dns"
 )
 
-// ipsetCtx is the ipset context.  ipsetMgr can be nil.
+// ipsetHandler is the ipset context.  ipsetMgr can be nil.
-type ipsetCtx struct {
+type ipsetHandler struct {
 	ipsetMgr ipset.Manager
 	logger   *slog.Logger
 }
 
-// newIPSetCtx returns a new initialized [ipsetCtx].  It is not safe for
+// newIpsetHandler returns a new initialized [ipsetHandler].  It is not safe for
-// concurrent use.
+// concurrent use.  c is always non-nil for [Server.Close].
-func newIPSetCtx(logger *slog.Logger, ipsetConf []string) (c *ipsetCtx, err error) {
+func newIpsetHandler(logger *slog.Logger, ipsetList []string) (c *ipsetHandler, err error) {
-	c = &ipsetCtx{
+	c = &ipsetHandler{
 		logger: logger,
 	}
-	ipsetLogger := logger.With(slogutil.KeyPrefix, "ipset")
+	c.ipsetMgr, err = ipset.NewManager(&ipset.Config{
-	c.ipsetMgr, err = ipset.NewManager(ipsetLogger, ipsetConf)
+		Logger:    logger,
-	if errors.Is(err, os.ErrInvalid) || errors.Is(err, os.ErrPermission) {
+		IpsetList: ipsetList,
+	})
+	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.
@@ -36,22 +41,18 @@ func newIPSetCtx(logger *slog.Logger, ipsetConf []string) (c *ipsetCtx, err erro
 		//
 		// TODO(a.garipov): The Snap problem can probably be solved if we add
 		// the netlink-connector interface plug.
-		logger.Warn("ipset: cannot initialize", slogutil.KeyError, err)
+		logger.Warn("cannot initialize", slogutil.KeyError, err)
-
-		return c, nil
-	} else if errors.Is(err, errors.ErrUnsupported) {
-		logger.Warn("ipset: cannot initialize", slogutil.KeyError, err)
 
 		return c, nil
 	} else if err != nil {
-		return nil, fmt.Errorf("initializing ipset: %w", err)
+		return c, fmt.Errorf("initializing ipset: %w", err)
 	}
 
 	return c, nil
 }
 
 // close closes the Linux Netfilter connections.
-func (c *ipsetCtx) close() (err error) {
+func (c *ipsetHandler) close() (err error) {
 	if c.ipsetMgr != nil {
 		return c.ipsetMgr.Close()
 	}
@@ -59,7 +60,7 @@ func (c *ipsetCtx) close() (err error) {
 	return nil
 }
 
-func (c *ipsetCtx) dctxIsfilled(dctx *dnsContext) (ok bool) {
+func (c *ipsetHandler) dctxIsfilled(dctx *dnsContext) (ok bool) {
 	return dctx != nil &&
 		dctx.responseFromUpstream &&
 		dctx.proxyCtx != nil &&
@@ -70,7 +71,7 @@ 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) {
+func (c *ipsetHandler) skipIpsetProcessing(dctx *dnsContext) (ok bool) {
 	if c == nil || c.ipsetMgr == nil || !c.dctxIsfilled(dctx) {
 		return true
 	}
@@ -113,9 +114,11 @@ 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) {
+func (c *ipsetHandler) process(dctx *dnsContext) (rc resultCode) {
-	c.logger.Debug("ipset: started processing")
+	c.logger.Debug("started processing")
-	defer c.logger.Debug("ipset: finished processing")
+	defer c.logger.Debug("finished processing")
+
+	ctx := context.TODO()
 
 	if c.skipIpsetProcessing(dctx) {
 		return resultCodeSuccess
@@ -127,15 +130,15 @@ func (c *ipsetCtx) process(dctx *dnsContext) (rc resultCode) {
 	host = strings.ToLower(host)
 
 	ip4s, ip6s := ipsFromAnswer(dctx.proxyCtx.Res.Answer)
-	n, err := c.ipsetMgr.Add(host, ip4s, ip6s)
+	n, err := c.ipsetMgr.Add(ctx, host, ip4s, ip6s)
 	if err != nil {
 		// Consider ipset errors non-critical to the request.
-		c.logger.Error("ipset: adding host ips", slogutil.KeyError, err)
+		c.logger.ErrorContext(ctx, "adding host ips", slogutil.KeyError, err)
 
 		return resultCodeSuccess
 	}
 
-	c.logger.Debug("ipset: added new ipset entries", "num", n)
+	c.logger.DebugContext(ctx, "added new ipset entries", "num", n)
 
 	return resultCodeSuccess
 }

internal/dnsforward/ipset_internal_test.go

@@ -1,6 +1,7 @@
 package dnsforward
 
 import (
+	"context"
 	"net"
 	"testing"
 
@@ -17,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...)
 
@@ -59,7 +60,7 @@ func TestIpsetCtx_process(t *testing.T) {
 			responseFromUpstream: true,
 		}
 
-		ictx := &ipsetCtx{
+		ictx := &ipsetHandler{
 			logger: slogutil.NewDiscardLogger(),
 		}
 		rc := ictx.process(dctx)
@@ -80,7 +81,7 @@ func TestIpsetCtx_process(t *testing.T) {
 		}
 
 		m := &fakeIpsetMgr{}
-		ictx := &ipsetCtx{
+		ictx := &ipsetHandler{
 			ipsetMgr: m,
 			logger:   slogutil.NewDiscardLogger(),
 		}
@@ -105,7 +106,7 @@ func TestIpsetCtx_process(t *testing.T) {
 		}
 
 		m := &fakeIpsetMgr{}
-		ictx := &ipsetCtx{
+		ictx := &ipsetHandler{
 			ipsetMgr: m,
 			logger:   slogutil.NewDiscardLogger(),
 		}
@@ -129,7 +130,7 @@ func TestIpsetCtx_SkipIpsetProcessing(t *testing.T) {
 	}
 
 	m := &fakeIpsetMgr{}
-	ictx := &ipsetCtx{
+	ictx := &ipsetHandler{
 		ipsetMgr: m,
 		logger:   slogutil.NewDiscardLogger(),
 	}

internal/ipset/ipset.go

@@ -2,6 +2,7 @@
 package ipset
 
 import (
+	"context"
 	"log/slog"
 	"net"
 )
@@ -11,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
+// Config is the configuration structure for the ipset manager.
-// ipset with an ipv4 family; IPv6 addresses, to an ipv6 ipset.  ipset must
+type Config struct {
-// exist.
+	// Logger is used for logging the operation of the ipset manager.  It must
+	// not be nil.
+	Logger *slog.Logger
+
+	// IpsetList is the ipset configuration with the following syntax:
+	//
+	//	DOMAIN[,DOMAIN].../IPSET_NAME[,IPSET_NAME]...
+	//
+	// IpsetList must not contain any blank lines or comments.
+	IpsetList []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:
+// If conf.IpsetList is empty, mgr and err are nil.  The error's chain contains
-//
-//	DOMAIN[,DOMAIN].../IPSET_NAME[,IPSET_NAME]...
-//
-// If ipsetConf is empty, msg and err are nil.  The error's chain contains
 // [errors.ErrUnsupported] if current OS is not supported.
-func NewManager(logger *slog.Logger, ipsetConf []string) (mgr Manager, err error) {
+func NewManager(conf *Config) (mgr Manager, err error) {
-	if len(ipsetConf) == 0 {
+	if len(conf.IpsetList) == 0 {
 		return nil, nil
 	}
 
-	return newManager(logger, ipsetConf)
+	return newManager(conf)
 }

internal/ipset/ipset_linux.go

@@ -4,6 +4,7 @@ package ipset
 
 import (
 	"bytes"
+	"context"
 	"fmt"
 	"log/slog"
 	"net"
@@ -35,8 +36,8 @@ import (
 //     resolved IP addresses.
 
 // newManager returns a new Linux ipset manager.
-func newManager(logger *slog.Logger, ipsetConf []string) (set Manager, err error) {
+func newManager(conf *Config) (set Manager, err error) {
-	return newManagerWithDialer(logger, ipsetConf, defaultDial)
+	return newManagerWithDialer(conf, defaultDial)
 }
 
 // defaultDial is the default netfilter dialing function.
@@ -339,8 +340,8 @@ func (m *manager) ipsets(names []string, currentlyKnown map[string]props) (sets
 		}
 
 		if p.family != netfilter.ProtoIPv4 && p.family != netfilter.ProtoIPv6 {
-			m.logger.Debug("getting properties",
+			m.logger.Debug(
-				slogutil.KeyError, "unexpected ipset family",
+				"got unexpected ipset family while getting set properties",
 				"set_name", p.name,
 				"set_type", p.typeName,
 				"set_family", p.family,
@@ -361,11 +362,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(
+func newManagerWithDialer(conf *Config, dial dialer) (mgr Manager, err error) {
-	logger *slog.Logger,
-	ipsetConf []string,
-	dial dialer,
-) (mgr Manager, err error) {
 	defer func() { err = errors.Annotate(err, "ipset: %w") }()
 
 	m := &manager{
@@ -374,7 +371,7 @@ func newManagerWithDialer(
 		nameToIpset:    make(map[string]props),
 		domainToIpsets: make(map[string][]props),
 
-		logger: logger,
+		logger: conf.Logger,
 
 		dial: dial,
 
@@ -386,7 +383,7 @@ func newManagerWithDialer(
 		if errors.Is(err, unix.EPROTONOSUPPORT) {
 			// The implementation doesn't support this protocol version.  Just
 			// issue a warning.
-			logger.Warn("dialing netfilter", slogutil.KeyError, err)
+			m.logger.Warn("dialing netfilter", slogutil.KeyError, err)
 
 			return nil, nil
 		}
@@ -394,12 +391,12 @@ func newManagerWithDialer(
 		return nil, fmt.Errorf("dialing netfilter: %w", err)
 	}
 
-	err = m.parseIpsetConfig(ipsetConf)
+	err = m.parseIpsetConfig(conf.IpsetList)
 	if err != nil {
 		return nil, fmt.Errorf("getting ipsets: %w", err)
 	}
 
-	logger.Debug("initialized")
+	m.logger.Debug("initialized")
 
 	return m, nil
 }
@@ -486,6 +483,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,
@@ -508,7 +506,9 @@ func (m *manager) addToSets(
 			return n, fmt.Errorf("%q %q unexpected family %q", set.name, set.typeName, set.family)
 		}
 
-		m.logger.Debug("added ips to set",
+		m.logger.DebugContext(
+			ctx,
+			"added ips to set",
 			"ips_num", nn,
 			"set_name", set.name,
 			"set_type", set.typeName,
@@ -521,7 +521,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()
 
@@ -530,9 +530,9 @@ func (m *manager) Add(host string, ip4s, ip6s []net.IP) (n int, err error) {
 		return 0, nil
 	}
 
-	m.logger.Debug("found sets", "set_num", 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.

internal/ipset/ipset_linux_internal_test.go

@@ -6,9 +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"
@@ -16,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
@@ -59,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",
 	}
@@ -90,7 +95,11 @@ func TestManager_Add(t *testing.T) {
 		}, nil
 	}
 
-	m, err := newManagerWithDialer(slogutil.NewDiscardLogger(), ipsetConf, fakeDial)
+	conf := &Config{
+		Logger:    slogutil.NewDiscardLogger(),
+		IpsetList: ipsetList,
+	}
+	m, err := newManagerWithDialer(conf, fakeDial)
 	require.NoError(t, err)
 
 	ip4 := net.IP{1, 2, 3, 4}
@@ -101,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)
@@ -111,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)

internal/ipset/ipset_others.go

@@ -3,11 +3,9 @@
 package ipset
 
 import (
-	"log/slog"
-
 	"github.com/AdguardTeam/AdGuardHome/internal/aghos"
 )
 
-func newManager(_ *slog.Logger, _ []string) (mgr Manager, err error) {
+func newManager(_ *Config) (mgr Manager, err error) {
 	return nil, aghos.Unsupported("ipset")
 }