Pull request 1967: AG-24794-imp-arpdb

Squashed commit of the following: commit 6f6f6cc Merge: 9aa3ac5 8fb7670 Author: Stanislav Chzhen <[email protected]> Date: Thu Aug 24 13:29:47 2023 +0300 Merge branch 'master' into AG-24794-imp-arpdb commit 9aa3ac5 Author: Stanislav Chzhen <[email protected]> Date: Wed Aug 23 16:14:02 2023 +0300 scripts: gocognit: add arpdb commit e99b053 Merge: 84893bc 3722c28 Author: Stanislav Chzhen <[email protected]> Date: Wed Aug 23 16:08:25 2023 +0300 Merge branch 'master' into AG-24794-imp-arpdb commit 84893bc Author: Stanislav Chzhen <[email protected]> Date: Wed Aug 23 16:07:43 2023 +0300 arpdb: add todo commit ad4b368 Author: Stanislav Chzhen <[email protected]> Date: Wed Aug 23 14:02:07 2023 +0300 arpdb: imp code commit 9cdd17d Author: Stanislav Chzhen <[email protected]> Date: Fri Aug 18 19:05:10 2023 +0300 all: imp arpdb
AdguardTeam · Aug 24, 2023 · 6fea709 · 6fea709
1 parent 8fb7670
commit 6fea709
Show file tree

Hide file tree

Showing 15 changed files with 203 additions and 136 deletions.
diff --git a/internal/aghnet/net_internal_test.go b/internal/aghnet/net_internal_test.go
@@ -7,7 +7,6 @@ import (
 	"io/fs"
 	"net"
 	"net/netip"
-	"os"
 	"strings"
 	"testing"
 
@@ -18,9 +17,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// testdata is the filesystem containing data for testing the package.
-var testdata fs.FS = os.DirFS("./testdata")
-
 // substRootDirFS replaces the aghos.RootDirFS function used throughout the
 // package with fsys for tests ran under t.
 func substRootDirFS(t testing.TB, fsys fs.FS) {

diff --git a/internal/aghnet/arpdb.go → internal/arpdb/arpdb.go b/internal/aghnet/arpdb.go → internal/arpdb/arpdb.go
@@ -1,4 +1,5 @@
-package aghnet
+// Package arpdb implements the Network Neighborhood Database.
+package arpdb
 
 import (
 	"bufio"
@@ -8,15 +9,25 @@ import (
 	"net/netip"
 	"sync"
 
+	"github.com/AdguardTeam/AdGuardHome/internal/aghos"
 	"github.com/AdguardTeam/golibs/errors"
+	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/netutil"
 	"golang.org/x/exp/slices"
 )
 
-// ARPDB: The Network Neighborhood Database
+// Variables and functions to substitute in tests.
+var (
+	// aghosRunCommand is the function to run shell commands.
+	aghosRunCommand = aghos.RunCommand
 
-// ARPDB stores and refreshes the network neighborhood reported by ARP (Address
-// Resolution Protocol).
-type ARPDB interface {
+	// rootDirFS is the filesystem pointing to the root directory.
+	rootDirFS = aghos.RootDirFS()
+)
+
+// Interface stores and refreshes the network neighborhood reported by ARP
+// (Address Resolution Protocol).
+type Interface interface {
 	// Refresh updates the stored data.  It must be safe for concurrent use.
 	Refresh() (err error)
 
@@ -25,28 +36,24 @@ type ARPDB interface {
 	Neighbors() (ns []Neighbor)
 }
 
-// NewARPDB returns the ARPDB properly initialized for the OS.
-func NewARPDB() (arp ARPDB) {
+// New returns the [Interface] properly initialized for the OS.
+func New() (arp Interface) {
 	return newARPDB()
 }
 
-// Empty ARPDB implementation
-
-// EmptyARPDB is the ARPDB implementation that does nothing.
-type EmptyARPDB struct{}
+// Empty is the [Interface] implementation that does nothing.
+type Empty struct{}
 
 // type check
-var _ ARPDB = EmptyARPDB{}
+var _ Interface = Empty{}
 
-// Refresh implements the ARPDB interface for EmptyARPContainer.  It does
+// Refresh implements the [Interface] interface for EmptyARPContainer.  It does
 // nothing and always returns nil error.
-func (EmptyARPDB) Refresh() (err error) { return nil }
+func (Empty) Refresh() (err error) { return nil }
 
-// Neighbors implements the ARPDB interface for EmptyARPContainer.  It always
-// returns nil.
-func (EmptyARPDB) Neighbors() (ns []Neighbor) { return nil }
-
-// ARPDB Helper Types
+// Neighbors implements the [Interface] interface for EmptyARPContainer.  It
+// always returns nil.
+func (Empty) Neighbors() (ns []Neighbor) { return nil }
 
 // Neighbor is the pair of IP address and MAC address reported by ARP.
 type Neighbor struct {
@@ -70,8 +77,21 @@ func (n Neighbor) Clone() (clone Neighbor) {
 	}
 }
 
+// validatedHostname returns valid hostname.  Otherwise returns empty string and
+// logs the error if hostname is not valid.
+func validatedHostname(h string) (host string) {
+	err := netutil.ValidateHostname(h)
+	if err != nil {
+		log.Debug("arpdb: parsing arp output: host: %s", err)
+
+		return ""
+	}
+
+	return h
+}
+
 // neighs is the helper type that stores neighbors to avoid copying its methods
-// among all the ARPDB implementations.
+// among all the [Interface] implementations.
 type neighs struct {
 	mu *sync.RWMutex
 	ns []Neighbor
@@ -108,14 +128,12 @@ func (ns *neighs) reset(with []Neighbor) {
 	ns.ns = with
 }
 
-// Command ARPDB
-
 // 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)
 
-// cmdARPDB is the implementation of the ARPDB that uses command line to
+// cmdARPDB is the implementation of the [Interface] that uses command line to
 // retrieve data.
 type cmdARPDB struct {
 	parse parseNeighsFunc
@@ -125,9 +143,9 @@ type cmdARPDB struct {
 }
 
 // type check
-var _ ARPDB = (*cmdARPDB)(nil)
+var _ Interface = (*cmdARPDB)(nil)
 
-// Refresh implements the ARPDB interface for *cmdARPDB.
+// Refresh implements the [Interface] interface for *cmdARPDB.
 func (arp *cmdARPDB) Refresh() (err error) {
 	defer func() { err = errors.Annotate(err, "cmd arpdb: %w") }()
 
@@ -150,24 +168,22 @@ func (arp *cmdARPDB) Refresh() (err error) {
 	return nil
 }
 
-// Neighbors implements the ARPDB interface for *cmdARPDB.
+// Neighbors implements the [Interface] interface for *cmdARPDB.
 func (arp *cmdARPDB) Neighbors() (ns []Neighbor) {
 	return arp.ns.clone()
 }
 
-// Composite ARPDB
-
-// arpdbs is the ARPDB that combines several ARPDB implementations and
-// consequently switches between those.
+// arpdbs is the [Interface] that combines several [Interface] implementations
+// and consequently switches between those.
 type arpdbs struct {
-	// arps is the set of ARPDB implementations to range through.
-	arps []ARPDB
+	// arps is the set of [Interface] implementations to range through.
+	arps []Interface
 	neighs
 }
 
 // newARPDBs returns a properly initialized *arpdbs.  It begins refreshing from
 // the first of arps.
-func newARPDBs(arps ...ARPDB) (arp *arpdbs) {
+func newARPDBs(arps ...Interface) (arp *arpdbs) {
 	return &arpdbs{
 		arps: arps,
 		neighs: neighs{
@@ -178,9 +194,9 @@ func newARPDBs(arps ...ARPDB) (arp *arpdbs) {
 }
 
 // type check
-var _ ARPDB = (*arpdbs)(nil)
+var _ Interface = (*arpdbs)(nil)
 
-// Refresh implements the ARPDB interface for *arpdbs.
+// Refresh implements the [Interface] interface for *arpdbs.
 func (arp *arpdbs) Refresh() (err error) {
 	var errs []error
 
@@ -200,7 +216,7 @@ func (arp *arpdbs) Refresh() (err error) {
 	return errors.Annotate(errors.Join(errs...), "each arpdb failed: %w")
 }
 
-// Neighbors implements the ARPDB interface for *arpdbs.
+// Neighbors implements the [Interface] interface for *arpdbs.
 //
 // TODO(e.burkov):  Think of a way to avoid cloning the slice twice.
 func (arp *arpdbs) Neighbors() (ns []Neighbor) {

diff --git a/internal/aghnet/arpdb_bsd.go → internal/arpdb/arpdb_bsd.go b/internal/aghnet/arpdb_bsd.go → internal/arpdb/arpdb_bsd.go
@@ -1,6 +1,6 @@
 //go:build darwin || freebsd
 
-package aghnet
+package arpdb
 
 import (
 	"bufio"
@@ -10,7 +10,6 @@ import (
 	"sync"
 
 	"github.com/AdguardTeam/golibs/log"
-	"github.com/AdguardTeam/golibs/netutil"
 )
 
 func newARPDB() (arp *cmdARPDB) {
@@ -44,16 +43,16 @@ func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 			continue
 		}
 
-		n := Neighbor{}
-
-		if ipStr := fields[1]; len(ipStr) < 2 {
+		ipStr := fields[1]
+		if len(ipStr) < 2 {
 			continue
-		} else if ip, err := netip.ParseAddr(ipStr[1 : len(ipStr)-1]); err != nil {
+		}
+
+		ip, err := netip.ParseAddr(ipStr[1 : len(ipStr)-1])
+		if err != nil {
 			log.Debug("arpdb: parsing arp output: ip: %s", err)
 
 			continue
-		} else {
-			n.IP = ip
 		}
 
 		hwStr := fields[3]
@@ -62,19 +61,13 @@ func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 			log.Debug("arpdb: parsing arp output: mac: %s", err)
 
 			continue
-		} else {
-			n.MAC = mac
-		}
-
-		host := fields[0]
-		err = netutil.ValidateHostname(host)
-		if err != nil {
-			log.Debug("arpdb: parsing arp output: host: %s", err)
-		} else {
-			n.Name = host
 		}
 
-		ns = append(ns, n)
+		ns = append(ns, Neighbor{
+			IP:   ip,
+			MAC:  mac,
+			Name: validatedHostname(fields[0]),
+		})
 	}
 
 	return ns

diff --git a/internal/aghnet/arpdb_bsd_test.go → internal/arpdb/arpdb_bsd_internal_test.go b/internal/aghnet/arpdb_bsd_test.go → internal/arpdb/arpdb_bsd_internal_test.go
@@ -1,6 +1,6 @@
 //go:build darwin || freebsd
 
-package aghnet
+package arpdb
 
 import (
 	"net"

diff --git a/internal/aghnet/arpdb_test.go → internal/arpdb/arpdb_internal_test.go b/internal/aghnet/arpdb_test.go → internal/arpdb/arpdb_internal_test.go
@@ -1,8 +1,12 @@
-package aghnet
+package arpdb
 
 import (
+	"fmt"
+	"io/fs"
 	"net"
 	"net/netip"
+	"os"
+	"strings"
 	"sync"
 	"testing"
 
@@ -12,30 +16,78 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestNewARPDB(t *testing.T) {
-	var a ARPDB
-	require.NotPanics(t, func() { a = NewARPDB() })
+// testdata is the filesystem containing data for testing the package.
+var testdata fs.FS = os.DirFS("./testdata")
+
+// RunCmdFunc is the signature of aghos.RunCommand function.
+type RunCmdFunc func(cmd string, args ...string) (code int, out []byte, err error)
+
+// substShell replaces the the aghos.RunCommand function used throughout the
+// package with rc for tests ran under t.
+func substShell(t testing.TB, rc RunCmdFunc) {
+	t.Helper()
+
+	prev := aghosRunCommand
+	t.Cleanup(func() { aghosRunCommand = prev })
+	aghosRunCommand = rc
+}
+
+// mapShell is a substitution of aghos.RunCommand that maps the command to it's
+// execution result.  It's only needed to simplify testing.
+//
+// TODO(e.burkov):  Perhaps put all the shell interactions behind an interface.
+type mapShell map[string]struct {
+	err  error
+	out  string
+	code int
+}
+
+// theOnlyCmd returns mapShell that only handles a single command and arguments
+// combination from cmd.
+func theOnlyCmd(cmd string, code int, out string, err error) (s mapShell) {
+	return mapShell{cmd: {code: code, out: out, err: err}}
+}
+
+// RunCmd is a RunCmdFunc handled by s.
+func (s mapShell) RunCmd(cmd string, args ...string) (code int, out []byte, err error) {
+	key := strings.Join(append([]string{cmd}, args...), " ")
+	ret, ok := s[key]
+	if !ok {
+		return 0, nil, fmt.Errorf("unexpected shell command %q", key)
+	}
+
+	return ret.code, []byte(ret.out), ret.err
+}
+
+func Test_New(t *testing.T) {
+	var a Interface
+	require.NotPanics(t, func() { a = New() })
 
 	assert.NotNil(t, a)
 }
 
-// TestARPDB is the mock implementation of ARPDB to use in tests.
+// TODO(s.chzhen):  Consider moving mocks into aghtest.
+
+// TestARPDB is the mock implementation of [Interface] to use in tests.
 type TestARPDB struct {
 	OnRefresh   func() (err error)
 	OnNeighbors func() (ns []Neighbor)
 }
 
-// Refresh implements the ARPDB interface for *TestARPDB.
+// type check
+var _ Interface = (*TestARPDB)(nil)
+
+// Refresh implements the [Interface] interface for *TestARPDB.
 func (arp *TestARPDB) Refresh() (err error) {
 	return arp.OnRefresh()
 }
 
-// Neighbors implements the ARPDB interface for *TestARPDB.
+// Neighbors implements the [Interface] interface for *TestARPDB.
 func (arp *TestARPDB) Neighbors() (ns []Neighbor) {
 	return arp.OnNeighbors()
 }
 
-func TestARPDBS(t *testing.T) {
+func Test_NewARPDBs(t *testing.T) {
 	knownIP := netip.MustParseAddr("1.2.3.4")
 	knownMAC := net.HardwareAddr{0xAB, 0xCD, 0xEF, 0xAB, 0xCD, 0xEF}
 
@@ -195,7 +247,7 @@ func TestCmdARPDB_arpa(t *testing.T) {
 }
 
 func TestEmptyARPDB(t *testing.T) {
-	a := EmptyARPDB{}
+	a := Empty{}
 
 	t.Run("refresh", func(t *testing.T) {
 		var err error