Pull request 2271: AGDNS-2374-slog-arpdb

Squashed commit of the following: commit 355136e6e2f3e77b483d97fbc01fbef562c319eb Author: Stanislav Chzhen <s.chzhen@adguard.com> Date: Tue Aug 27 18:09:51 2024 +0300 arpdb: imp docs commit 2738383303 Author: Stanislav Chzhen <s.chzhen@adguard.com> Date: Mon Aug 26 19:11:10 2024 +0300 all: slog arpdb
2024-08-27 20:42:10 +03:00 · 2024-08-27 20:42:10 +03:00 · 0b8bf13453
parent 738958d90a
commit 0b8bf13453
8 changed files with 110 additions and 142 deletions
--- a/internal/arpdb/arpdb.go
+++ b/internal/arpdb/arpdb.go
@ -5,6 +5,7 @@ import (
 	"bufio"
 	"bytes"
 	"fmt"
+	"log/slog"
 	"net"
 	"net/netip"
 	"slices"
@ -12,7 +13,7 @@ import (

 	"github.com/AdguardTeam/AdGuardHome/internal/aghos"
 	"github.com/AdguardTeam/golibs/errors"
-	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/logutil/slogutil"
 	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/AdguardTeam/golibs/osutil"
 )
@ -38,8 +39,8 @@ type Interface interface {
 }

 // New returns the [Interface] properly initialized for the OS.
-func New() (arp Interface) {
-	return newARPDB()
+func New(logger *slog.Logger) (arp Interface) {
+	return newARPDB(logger)
 }

 // Empty is the [Interface] implementation that does nothing.
@ -69,6 +70,30 @@ type Neighbor struct {
 	MAC net.HardwareAddr
 }

+// newNeighbor returns the new initialized [Neighbor] by parsing string
+// representations of IP and MAC addresses.
+func newNeighbor(host, ipStr, macStr string) (n *Neighbor, err error) {
+	defer func() { err = errors.Annotate(err, "getting arp neighbor: %w") }()
+
+	ip, err := netip.ParseAddr(ipStr)
+	if err != nil {
+		// Don't wrap the error, as it will get annotated.
+		return nil, err
+	}
+
+	mac, err := net.ParseMAC(macStr)
+	if err != nil {
+		// Don't wrap the error, as it will get annotated.
+		return nil, err
+	}
+
+	return &Neighbor{
+		Name: host,
+		IP:   ip,
+		MAC:  mac,
+	}, nil
+}
+
 // Clone returns the deep copy of n.
 func (n Neighbor) Clone() (clone Neighbor) {
 	return Neighbor{
@ -80,10 +105,10 @@ func (n Neighbor) Clone() (clone Neighbor) {

 // validatedHostname returns h if it's a valid hostname, or an empty string
 // otherwise, logging the validation error.
-func validatedHostname(h string) (host string) {
+func validatedHostname(logger *slog.Logger, h string) (host string) {
 	err := netutil.ValidateHostname(h)
 	if err != nil {
-		log.Debug("arpdb: parsing arp output: host: %s", err)
+		logger.Debug("parsing host of arp output", slogutil.KeyError, err)

 		return ""
 	}
@ -132,15 +157,18 @@ func (ns *neighs) reset(with []Neighbor) {
 // parseNeighsFunc parses the text from sc as if it'd be an output of some
 // ARP-related command.  lenHint is a hint for the size of the allocated slice
 // of Neighbors.
-type parseNeighsFunc func(sc *bufio.Scanner, lenHint int) (ns []Neighbor)
+//
+// TODO(s.chzhen):  Return []*Neighbor instead.
+type parseNeighsFunc func(logger *slog.Logger, sc *bufio.Scanner, lenHint int) (ns []Neighbor)

 // cmdARPDB is the implementation of the [Interface] that uses command line to
 // retrieve data.
 type cmdARPDB struct {
-	parse parseNeighsFunc
-	ns    *neighs
-	cmd   string
-	args  []string
+	logger *slog.Logger
+	parse  parseNeighsFunc
+	ns     *neighs
+	cmd    string
+	args   []string
 }

 // type check
@ -158,7 +186,7 @@ func (arp *cmdARPDB) Refresh() (err error) {
 	}

 	sc := bufio.NewScanner(bytes.NewReader(out))
-	ns := arp.parse(sc, arp.ns.len())
+	ns := arp.parse(arp.logger, sc, arp.ns.len())
 	if err = sc.Err(); err != nil {
 		// TODO(e.burkov):  This error seems unreachable.  Investigate.
 		return fmt.Errorf("scanning the output: %w", err)
--- a/internal/arpdb/arpdb_bsd.go
+++ b/internal/arpdb/arpdb_bsd.go
@ -4,17 +4,17 @@ package arpdb

 import (
 	"bufio"
-	"net"
-	"net/netip"
+	"log/slog"
 	"strings"
 	"sync"

-	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/logutil/slogutil"
 )

-func newARPDB() (arp *cmdARPDB) {
+func newARPDB(logger *slog.Logger) (arp *cmdARPDB) {
 	return &cmdARPDB{
-		parse: parseArpA,
+		logger: logger,
+		parse:  parseArpA,
 		ns: &neighs{
 			mu: &sync.RWMutex{},
 			ns: make([]Neighbor, 0),
@ -33,7 +33,7 @@ func newARPDB() (arp *cmdARPDB) {
 // The expected input format:
 //
 //	host.name (192.168.0.1) at ff:ff:ff:ff:ff:ff on en0 ifscope [ethernet]
-func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
+func parseArpA(logger *slog.Logger, sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 	ns = make([]Neighbor, 0, lenHint)
 	for sc.Scan() {
 		ln := sc.Text()
@ -48,26 +48,15 @@ func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 			continue
 		}

-		ip, err := netip.ParseAddr(ipStr[1 : len(ipStr)-1])
+		host := validatedHostname(logger, fields[0])
+		n, err := newNeighbor(host, ipStr[1:len(ipStr)-1], fields[3])
 		if err != nil {
-			log.Debug("arpdb: parsing arp output: ip: %s", err)
+			logger.Debug("parsing arp output", "line", ln, slogutil.KeyError, err)

 			continue
 		}

-		hwStr := fields[3]
-		mac, err := net.ParseMAC(hwStr)
-		if err != nil {
-			log.Debug("arpdb: parsing arp output: mac: %s", err)
-
-			continue
-		}
-
-		ns = append(ns, Neighbor{
-			IP:   ip,
-			MAC:  mac,
-			Name: validatedHostname(fields[0]),
-		})
+		ns = append(ns, *n)
 	}

 	return ns
--- a/internal/arpdb/arpdb_internal_test.go
+++ b/internal/arpdb/arpdb_internal_test.go
@ -11,6 +11,7 @@ import (
 	"testing"

 	"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"
@ -61,7 +62,7 @@ func (s mapShell) RunCmd(cmd string, args ...string) (code int, out []byte, err

 func Test_New(t *testing.T) {
 	var a Interface
-	require.NotPanics(t, func() { a = New() })
+	require.NotPanics(t, func() { a = New(slogutil.NewDiscardLogger()) })

 	assert.NotNil(t, a)
 }
@ -201,8 +202,9 @@ func Test_NewARPDBs(t *testing.T) {

 func TestCmdARPDB_arpa(t *testing.T) {
 	a := &cmdARPDB{
-		cmd:   "cmd",
-		parse: parseArpA,
+		logger: slogutil.NewDiscardLogger(),
+		cmd:    "cmd",
+		parse:  parseArpA,
 		ns: &neighs{
 			mu: &sync.RWMutex{},
 			ns: make([]Neighbor, 0),
--- a/internal/arpdb/arpdb_linux.go
+++ b/internal/arpdb/arpdb_linux.go
@ -6,17 +6,18 @@ import (
 	"bufio"
 	"fmt"
 	"io/fs"
+	"log/slog"
 	"net"
 	"net/netip"
 	"strings"
 	"sync"

 	"github.com/AdguardTeam/AdGuardHome/internal/aghos"
-	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/logutil/slogutil"
 	"github.com/AdguardTeam/golibs/stringutil"
 )

-func newARPDB() (arp *arpdbs) {
+func newARPDB(logger *slog.Logger) (arp *arpdbs) {
 	// Use the common storage among the implementations.
 	ns := &neighs{
 		mu: &sync.RWMutex{},
@ -39,9 +40,10 @@ func newARPDB() (arp *arpdbs) {
 		},
 		// Then, try "arp -a -n".
 		&cmdARPDB{
-			parse: parseF,
-			ns:    ns,
-			cmd:   "arp",
+			logger: logger,
+			parse:  parseF,
+			ns:     ns,
+			cmd:    "arp",
 			// Use -n flag to avoid resolving the hostnames of the neighbors.
 			// By default ARP attempts to resolve the hostnames via DNS.  See
 			// man 8 arp.
@ -51,10 +53,11 @@ func newARPDB() (arp *arpdbs) {
 		},
 		// Finally, try "ip neigh".
 		&cmdARPDB{
-			parse: parseIPNeigh,
-			ns:    ns,
-			cmd:   "ip",
-			args:  []string{"neigh"},
+			logger: logger,
+			parse:  parseIPNeigh,
+			ns:     ns,
+			cmd:    "ip",
+			args:   []string{"neigh"},
 		},
 	)
 }
@ -131,7 +134,7 @@ func (arp *fsysARPDB) Neighbors() (ns []Neighbor) {
 //
 //	IP address     HW type  Flags  HW address         Mask  Device
 //	192.168.11.98  0x1      0x2    5a:92:df:a9:7e:28  *     wan
-func parseArpAWrt(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
+func parseArpAWrt(logger *slog.Logger, sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 	if !sc.Scan() {
 		// Skip the header.
 		return
@ -146,25 +149,14 @@ func parseArpAWrt(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 			continue
 		}

-		ip, err := netip.ParseAddr(fields[0])
+		n, err := newNeighbor("", fields[0], fields[3])
 		if err != nil {
-			log.Debug("arpdb: parsing arp output: ip: %s", err)
+			logger.Debug("parsing arp output", "line", ln, slogutil.KeyError, err)

 			continue
 		}

-		hwStr := fields[3]
-		mac, err := net.ParseMAC(hwStr)
-		if err != nil {
-			log.Debug("arpdb: parsing arp output: mac: %s", err)
-
-			continue
-		}
-
-		ns = append(ns, Neighbor{
-			IP:  ip,
-			MAC: mac,
-		})
+		ns = append(ns, *n)
 	}

 	return ns
@ -174,7 +166,7 @@ func parseArpAWrt(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 // expected input format:
 //
 //	hostname (192.168.1.1) at ab:cd:ef:ab:cd:ef [ether] on enp0s3
-func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
+func parseArpA(logger *slog.Logger, sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 	ns = make([]Neighbor, 0, lenHint)
 	for sc.Scan() {
 		ln := sc.Text()
@ -189,26 +181,15 @@ func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 			continue
 		}

-		ip, err := netip.ParseAddr(ipStr[1 : len(ipStr)-1])
+		host := validatedHostname(logger, fields[0])
+		n, err := newNeighbor(host, ipStr[1:len(ipStr)-1], fields[3])
 		if err != nil {
-			log.Debug("arpdb: parsing arp output: ip: %s", err)
+			logger.Debug("parsing arp output", "line", ln, slogutil.KeyError, err)

 			continue
 		}

-		hwStr := fields[3]
-		mac, err := net.ParseMAC(hwStr)
-		if err != nil {
-			log.Debug("arpdb: parsing arp output: mac: %s", err)
-
-			continue
-		}
-
-		ns = append(ns, Neighbor{
-			IP:   ip,
-			MAC:  mac,
-			Name: validatedHostname(fields[0]),
-		})
+		ns = append(ns, *n)
 	}

 	return ns
@ -218,7 +199,7 @@ func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 // expected input format:
 //
 //	192.168.1.1 dev enp0s3 lladdr ab:cd:ef:ab:cd:ef REACHABLE
-func parseIPNeigh(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
+func parseIPNeigh(logger *slog.Logger, sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 	ns = make([]Neighbor, 0, lenHint)
 	for sc.Scan() {
 		ln := sc.Text()
@ -228,27 +209,14 @@ func parseIPNeigh(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 			continue
 		}

-		n := Neighbor{}
-
-		ip, err := netip.ParseAddr(fields[0])
+		n, err := newNeighbor("", fields[0], fields[4])
 		if err != nil {
-			log.Debug("arpdb: parsing arp output: ip: %s", err)
+			logger.Debug("parsing arp output", "line", ln, slogutil.KeyError, err)

 			continue
-		} else {
-			n.IP = ip
 		}

-		mac, err := net.ParseMAC(fields[4])
-		if err != nil {
-			log.Debug("arpdb: parsing arp output: mac: %s", err)
-
-			continue
-		} else {
-			n.MAC = mac
-		}
-
-		ns = append(ns, n)
+		ns = append(ns, *n)
 	}

 	return ns
--- a/internal/arpdb/arpdb_linux_internal_test.go
+++ b/internal/arpdb/arpdb_linux_internal_test.go
@ -9,6 +9,7 @@ import (
 	"testing"
 	"testing/fstest"

+	"github.com/AdguardTeam/golibs/logutil/slogutil"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@ -69,9 +70,10 @@ func TestCmdARPDB_linux(t *testing.T) {

 	t.Run("wrt", func(t *testing.T) {
 		a := &cmdARPDB{
-			parse: parseArpAWrt,
-			cmd:   "arp",
-			args:  []string{"-a"},
+			logger: slogutil.NewDiscardLogger(),
+			parse:  parseArpAWrt,
+			cmd:    "arp",
+			args:   []string{"-a"},
 			ns: &neighs{
 				mu: &sync.RWMutex{},
 				ns: make([]Neighbor, 0),
@ -86,9 +88,10 @@ func TestCmdARPDB_linux(t *testing.T) {

 	t.Run("ip_neigh", func(t *testing.T) {
 		a := &cmdARPDB{
-			parse: parseIPNeigh,
-			cmd:   "ip",
-			args:  []string{"neigh"},
+			logger: slogutil.NewDiscardLogger(),
+			parse:  parseIPNeigh,
+			cmd:    "ip",
+			args:   []string{"neigh"},
 			ns: &neighs{
 				mu: &sync.RWMutex{},
 				ns: make([]Neighbor, 0),
--- a/internal/arpdb/arpdb_openbsd.go
+++ b/internal/arpdb/arpdb_openbsd.go
@ -4,17 +4,17 @@ package arpdb

 import (
 	"bufio"
-	"net"
-	"net/netip"
+	"log/slog"
 	"strings"
 	"sync"

-	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/logutil/slogutil"
 )

-func newARPDB() (arp *cmdARPDB) {
+func newARPDB(logger *slog.Logger) (arp *cmdARPDB) {
 	return &cmdARPDB{
-		parse: parseArpA,
+		logger: logger,
+		parse:  parseArpA,
 		ns: &neighs{
 			mu: &sync.RWMutex{},
 			ns: make([]Neighbor, 0),
@ -34,7 +34,7 @@ func newARPDB() (arp *cmdARPDB) {
 //
 //	Host        Ethernet Address  Netif Expire    Flags
 //	192.168.1.1 ab:cd:ef:ab:cd:ef   em0 19m59s
-func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
+func parseArpA(logger *slog.Logger, sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 	// Skip the header.
 	if !sc.Scan() {
 		return nil
@ -49,27 +49,14 @@ func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 			continue
 		}

-		n := Neighbor{}
-
-		ip, err := netip.ParseAddr(fields[0])
+		n, err := newNeighbor("", fields[0], fields[1])
 		if err != nil {
-			log.Debug("arpdb: parsing arp output: ip: %s", err)
+			logger.Debug("parsing arp output", "line", ln, slogutil.KeyError, err)

 			continue
-		} else {
-			n.IP = ip
 		}

-		mac, err := net.ParseMAC(fields[1])
-		if err != nil {
-			log.Debug("arpdb: parsing arp output: mac: %s", err)
-
-			continue
-		} else {
-			n.MAC = mac
-		}
-
-		ns = append(ns, n)
+		ns = append(ns, *n)
 	}

 	return ns
--- a/internal/arpdb/arpdb_windows.go
+++ b/internal/arpdb/arpdb_windows.go
@ -4,17 +4,17 @@ package arpdb

 import (
 	"bufio"
-	"net"
-	"net/netip"
+	"log/slog"
 	"strings"
 	"sync"

-	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/logutil/slogutil"
 )

-func newARPDB() (arp *cmdARPDB) {
+func newARPDB(logger *slog.Logger) (arp *cmdARPDB) {
 	return &cmdARPDB{
-		parse: parseArpA,
+		logger: logger,
+		parse:  parseArpA,
 		ns: &neighs{
 			mu: &sync.RWMutex{},
 			ns: make([]Neighbor, 0),
@ -31,7 +31,7 @@ func newARPDB() (arp *cmdARPDB) {
 //	  Internet Address      Physical Address      Type
 //	  192.168.56.1          0a-00-27-00-00-00     dynamic
 //	  192.168.56.255        ff-ff-ff-ff-ff-ff     static
-func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
+func parseArpA(logger *slog.Logger, sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 	ns = make([]Neighbor, 0, lenHint)
 	for sc.Scan() {
 		ln := sc.Text()
@ -44,24 +44,14 @@ func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 			continue
 		}

-		ip, err := netip.ParseAddr(fields[0])
+		n, err := newNeighbor("", fields[0], fields[1])
 		if err != nil {
-			log.Debug("arpdb: parsing arp output: ip: %s", err)
+			logger.Debug("parsing arp output", "line", ln, slogutil.KeyError, err)

 			continue
 		}

-		mac, err := net.ParseMAC(fields[1])
-		if err != nil {
-			log.Debug("arpdb: parsing arp output: mac: %s", err)
-
-			continue
-		}
-
-		ns = append(ns, Neighbor{
-			IP:  ip,
-			MAC: mac,
-		})
+		ns = append(ns, *n)
 	}

 	return ns
--- a/internal/home/home.go
+++ b/internal/home/home.go
@ -39,6 +39,7 @@ import (
 	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/hostsfile"
 	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/logutil/slogutil"
 	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/AdguardTeam/golibs/osutil"
 )
@ -276,7 +277,7 @@ func setupOpts(opts options) (err error) {
 }

 // initContextClients initializes Context clients and related fields.
-func initContextClients() (err error) {
+func initContextClients(logger *slog.Logger) (err error) {
 	err = setupDNSFilteringConf(config.Filtering)
 	if err != nil {
 		// Don't wrap the error, because it's informative enough as is.
@ -300,7 +301,7 @@ func initContextClients() (err error) {

 	var arpDB arpdb.Interface
 	if config.Clients.Sources.ARP {
-		arpDB = arpdb.New()
+		arpDB = arpdb.New(logger.With(slogutil.KeyError, "arpdb"))
 	}

 	return Context.clients.Init(
@ -582,7 +583,7 @@ func run(opts options, clientBuildFS fs.FS, done chan struct{}) {
 	// data first, but also to avoid relying on automatic Go init() function.
 	filtering.InitModule()

-	err = initContextClients()
+	err = initContextClients(slogLogger)
 	fatalOnError(err)

 	err = setupOpts(opts)