Pull request 2256: 4923 Better interfaces

Updates #4923.

Squashed commit of the following:

commit 0e40b41
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Jul 10 15:28:16 2024 +0300

    dhcpsvc: imp code, docs

commit 5463fdd
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Jul 9 20:31:20 2024 +0300

    dhcpsvc: imp ifaces

internal/dhcpsvc/dhcpsvc.go

@@ -50,7 +50,7 @@ type Interface interface {
 	IPByHost(host string) (ip netip.Addr)
 
 	// Leases returns all the active DHCP leases.  The returned slice should be
-	// a clone.
+	// a clone.  The order of leases is undefined.
 	//
 	// TODO(e.burkov):  Consider implementing iterating methods with appropriate
 	// signatures instead of cloning the whole list.

internal/dhcpsvc/interface.go

@@ -3,68 +3,102 @@ package dhcpsvc
 import (
 	"fmt"
 	"log/slog"
-	"slices"
+	"net"
 	"time"
 )
 
-// netInterface is a common part of any network interface within the DHCP
-// server.
+// macKey contains hardware address as byte array of 6, 8, or 20 bytes.
+//
+// TODO(e.burkov):  Move to aghnet or even to netutil.
+type macKey any
+
+// macToKey converts mac into macKey, which is used as the key for the lease
+// maps.  mac must be a valid hardware address of length 6, 8, or 20 bytes, see
+// [netutil.ValidateMAC].
+func macToKey(mac net.HardwareAddr) (key macKey) {
+	switch len(mac) {
+	case 6:
+		return [6]byte(mac)
+	case 8:
+		return [8]byte(mac)
+	case 20:
+		return [20]byte(mac)
+	default:
+		panic(fmt.Errorf("invalid mac address %#v", mac))
+	}
+}
+
+// netInterface is a common part of any interface within the DHCP server.
 //
 // TODO(e.burkov):  Add other methods as [DHCPServer] evolves.
 type netInterface struct {
 	// logger logs the events related to the network interface.
 	logger *slog.Logger
 
+	// leases is the set of DHCP leases assigned to this interface.
+	leases map[macKey]*Lease
+
 	// name is the name of the network interface.
 	name string
 
-	// leases is a set of leases sorted by hardware address.
-	leases []*Lease
-
 	// leaseTTL is the default Time-To-Live value for leases.
 	leaseTTL time.Duration
 }
 
+// newNetInterface creates a new netInterface with the given name, leaseTTL, and
+// logger.
+func newNetInterface(name string, l *slog.Logger, leaseTTL time.Duration) (iface *netInterface) {
+	return &netInterface{
+		logger:   l,
+		leases:   map[macKey]*Lease{},
+		name:     name,
+		leaseTTL: leaseTTL,
+	}
+}
+
 // reset clears all the slices in iface for reuse.
 func (iface *netInterface) reset() {
-	iface.leases = iface.leases[:0]
+	clear(iface.leases)
 }
 
-// insertLease inserts the given lease into iface.  It returns an error if the
+// addLease inserts the given lease into iface.  It returns an error if the
 // lease can't be inserted.
-func (iface *netInterface) insertLease(l *Lease) (err error) {
-	i, found := slices.BinarySearchFunc(iface.leases, l, compareLeaseMAC)
+func (iface *netInterface) addLease(l *Lease) (err error) {
+	mk := macToKey(l.HWAddr)
+	_, found := iface.leases[mk]
 	if found {
 		return fmt.Errorf("lease for mac %s already exists", l.HWAddr)
 	}
 
-	iface.leases = slices.Insert(iface.leases, i, l)
+	iface.leases[mk] = l
 
 	return nil
 }
 
 // updateLease replaces an existing lease within iface with the given one.  It
 // returns an error if there is no lease with such hardware address.
 func (iface *netInterface) updateLease(l *Lease) (prev *Lease, err error) {
-	i, found := slices.BinarySearchFunc(iface.leases, l, compareLeaseMAC)
+	mk := macToKey(l.HWAddr)
+	prev, found := iface.leases[mk]
 	if !found {
 		return nil, fmt.Errorf("no lease for mac %s", l.HWAddr)
 	}
 
-	prev, iface.leases[i] = iface.leases[i], l
+	iface.leases[mk] = l
 
 	return prev, nil
 }
 
 // removeLease removes an existing lease from iface.  It returns an error if
 // there is no lease equal to l.
 func (iface *netInterface) removeLease(l *Lease) (err error) {
-	i, found := slices.BinarySearchFunc(iface.leases, l, compareLeaseMAC)
+	mk := macToKey(l.HWAddr)
+	_, found := iface.leases[mk]
 	if !found {
 		return fmt.Errorf("no lease for mac %s", l.HWAddr)
 	}
 
-	iface.leases = slices.Delete(iface.leases, i, i+1)
+	delete(iface.leases, mk)
 
 	return nil
 }

internal/dhcpsvc/lease.go

@@ -1,7 +1,6 @@
 package dhcpsvc
 
 import (
-	"bytes"
 	"net"
 	"net/netip"
 	"slices"
@@ -45,8 +44,3 @@ func (l *Lease) Clone() (clone *Lease) {
 		IsStatic: l.IsStatic,
 	}
 }
-
-// compareLeaseMAC compares two [Lease]s by hardware address.
-func compareLeaseMAC(a, b *Lease) (res int) {
-	return bytes.Compare(a.HWAddr, b.HWAddr)
-}

internal/dhcpsvc/leaseindex.go

@@ -61,7 +61,7 @@ func (idx *leaseIndex) add(l *Lease, iface *netInterface) (err error) {
 		return fmt.Errorf("lease for hostname %s already exists", l.Hostname)
 	}
 
-	err = iface.insertLease(l)
+	err = iface.addLease(l)
 	if err != nil {
 		return err
 	}

internal/dhcpsvc/server.go

@@ -41,10 +41,10 @@ type DHCPServer struct {
 	leases *leaseIndex
 
 	// interfaces4 is the set of IPv4 interfaces sorted by interface name.
-	interfaces4 netInterfacesV4
+	interfaces4 dhcpInterfacesV4
 
 	// interfaces6 is the set of IPv6 interfaces sorted by interface name.
-	interfaces6 netInterfacesV6
+	interfaces6 dhcpInterfacesV6
 
 	// icmpTimeout is the timeout for checking another DHCP server's presence.
 	icmpTimeout time.Duration
@@ -63,28 +63,9 @@ func New(ctx context.Context, conf *Config) (srv *DHCPServer, err error) {
 		return nil, nil
 	}
 
-	// TODO(e.burkov):  Add validations scoped to the network interfaces set.
-	ifaces4 := make(netInterfacesV4, 0, len(conf.Interfaces))
-	ifaces6 := make(netInterfacesV6, 0, len(conf.Interfaces))
-	var errs []error
-
-	mapsutil.SortedRange(conf.Interfaces, func(name string, iface *InterfaceConfig) (cont bool) {
-		var i4 *netInterfaceV4
-		i4, err = newNetInterfaceV4(ctx, l, name, iface.IPv4)
-		if err != nil {
-			errs = append(errs, fmt.Errorf("interface %q: ipv4: %w", name, err))
-		} else if i4 != nil {
-			ifaces4 = append(ifaces4, i4)
-		}
-
-		i6 := newNetInterfaceV6(ctx, l, name, iface.IPv6)
-		if i6 != nil {
-			ifaces6 = append(ifaces6, i6)
-		}
-
-		return true
-	})
-	if err = errors.Join(errs...); err != nil {
+	ifaces4, ifaces6, err := newInterfaces(ctx, l, conf.Interfaces)
+	if err != nil {
+		// Don't wrap the error since it's informative enough as is.
 		return nil, err
 	}
 
@@ -112,6 +93,43 @@ func New(ctx context.Context, conf *Config) (srv *DHCPServer, err error) {
 	return srv, nil
 }
 
+// newInterfaces creates interfaces for the given map of interface names to
+// their configurations.
+func newInterfaces(
+	ctx context.Context,
+	l *slog.Logger,
+	ifaces map[string]*InterfaceConfig,
+) (v4 dhcpInterfacesV4, v6 dhcpInterfacesV6, err error) {
+	defer func() { err = errors.Annotate(err, "creating interfaces: %w") }()
+
+	// TODO(e.burkov):  Add validations scoped to the network interfaces set.
+	v4 = make(dhcpInterfacesV4, 0, len(ifaces))
+	v6 = make(dhcpInterfacesV6, 0, len(ifaces))
+
+	var errs []error
+	mapsutil.SortedRange(ifaces, func(name string, iface *InterfaceConfig) (cont bool) {
+		var i4 *dhcpInterfaceV4
+		i4, err = newDHCPInterfaceV4(ctx, l, name, iface.IPv4)
+		if err != nil {
+			errs = append(errs, fmt.Errorf("interface %q: ipv4: %w", name, err))
+		} else if i4 != nil {
+			v4 = append(v4, i4)
+		}
+
+		i6 := newDHCPInterfaceV6(ctx, l, name, iface.IPv6)
+		if i6 != nil {
+			v6 = append(v6, i6)
+		}
+
+		return true
+	})
+	if err = errors.Join(errs...); err != nil {
+		return nil, nil, err
+	}
+
+	return v4, v6, nil
+}
+
 // type check
 //
 // TODO(e.burkov):  Uncomment when the [Interface] interface is implemented.
@@ -127,16 +145,11 @@ func (srv *DHCPServer) Leases() (leases []*Lease) {
 	srv.leasesMu.RLock()
 	defer srv.leasesMu.RUnlock()
 
-	for _, iface := range srv.interfaces4 {
-		for _, lease := range iface.leases {
-			leases = append(leases, lease.Clone())
-		}
-	}
-	for _, iface := range srv.interfaces6 {
-		for _, lease := range iface.leases {
-			leases = append(leases, lease.Clone())
-		}
-	}
+	srv.leases.rangeLeases(func(l *Lease) (cont bool) {
+		leases = append(leases, l.Clone())
+
+		return true
+	})
 
 	return leases
 }
@@ -200,10 +213,10 @@ func (srv *DHCPServer) Reset(ctx context.Context) (err error) {
 // expects the DHCPServer.leasesMu to be locked.
 func (srv *DHCPServer) resetLeases() {
 	for _, iface := range srv.interfaces4 {
-		iface.reset()
+		iface.common.reset()
 	}
 	for _, iface := range srv.interfaces6 {
-		iface.reset()
+		iface.common.reset()
 	}
 	srv.leases.clear()
 }

internal/dhcpsvc/server_test.go

@@ -122,7 +122,7 @@ func TestNew(t *testing.T) {
 			DBFilePath: leasesPath,
 		},
 		name: "gateway_within_range",
-		wantErrMsg: `interface "eth0": ipv4: ` +
+		wantErrMsg: `creating interfaces: interface "eth0": ipv4: ` +
 			`gateway ip 192.168.0.100 in the ip range 192.168.0.1-192.168.0.254`,
 	}, {
 		conf: &dhcpsvc.Config{
@@ -138,7 +138,7 @@ func TestNew(t *testing.T) {
 			DBFilePath: leasesPath,
 		},
 		name: "bad_start",
-		wantErrMsg: `interface "eth0": ipv4: ` +
+		wantErrMsg: `creating interfaces: interface "eth0": ipv4: ` +
 			`range start 127.0.0.1 is not within 192.168.0.1/24`,
 	}}
 
@@ -568,5 +568,5 @@ func TestServer_Leases(t *testing.T) {
 		HWAddr:   mustParseMAC(t, "BB:BB:BB:BB:BB:BB"),
 		IsStatic: true,
 	}}
-	assert.Equal(t, wantLeases, srv.Leases())
+	assert.ElementsMatch(t, wantLeases, srv.Leases())
 }

internal/dhcpsvc/v4.go

@@ -82,8 +82,12 @@ func (c *IPv4Config) validate() (err error) {
 	return errors.Join(errs...)
 }
 
-// netInterfaceV4 is a DHCP interface for IPv4 address family.
-type netInterfaceV4 struct {
+// dhcpInterfaceV4 is a DHCP interface for IPv4 address family.
+type dhcpInterfaceV4 struct {
+	// common is the common part of any network interface within the DHCP
+	// server.
+	common *netInterface
+
 	// gateway is the IP address of the network gateway.
 	gateway netip.Addr
 
@@ -101,21 +105,17 @@ type netInterfaceV4 struct {
 	// explicitOpts are the user-configured options.  It must not have
 	// intersections with implicitOpts.
 	explicitOpts layers.DHCPOptions
-
-	// netInterface is embedded here to provide some common network interface
-	// logic.
-	netInterface
 }
 
-// newNetInterfaceV4 creates a new DHCP interface for IPv4 address family with
+// newDHCPInterfaceV4 creates a new DHCP interface for IPv4 address family with
 // the given configuration.  It returns an error if the given configuration
 // can't be used.
-func newNetInterfaceV4(
+func newDHCPInterfaceV4(
 	ctx context.Context,
 	l *slog.Logger,
 	name string,
 	conf *IPv4Config,
-) (i *netInterfaceV4, err error) {
+) (i *dhcpInterfaceV4, err error) {
 	l = l.With(
 		keyInterface, name,
 		keyFamily, netutil.AddrFamilyIPv4,
@@ -144,35 +144,31 @@ func newNetInterfaceV4(
 		return nil, fmt.Errorf("gateway ip %s in the ip range %s", conf.GatewayIP, addrSpace)
 	}
 
-	i = &netInterfaceV4{
+	i = &dhcpInterfaceV4{
 		gateway:   conf.GatewayIP,
 		subnet:    subnet,
 		addrSpace: addrSpace,
-		netInterface: netInterface{
-			name:     name,
-			leaseTTL: conf.LeaseDuration,
-			logger:   l,
-		},
+		common:    newNetInterface(name, l, conf.LeaseDuration),
 	}
 	i.implicitOpts, i.explicitOpts = conf.options(ctx, l)
 
 	return i, nil
 }
 
-// netInterfacesV4 is a slice of network interfaces of IPv4 address family.
-type netInterfacesV4 []*netInterfaceV4
+// dhcpInterfacesV4 is a slice of network interfaces of IPv4 address family.
+type dhcpInterfacesV4 []*dhcpInterfaceV4
 
 // find returns the first network interface within ifaces containing ip.  It
 // returns false if there is no such interface.
-func (ifaces netInterfacesV4) find(ip netip.Addr) (iface4 *netInterface, ok bool) {
-	i := slices.IndexFunc(ifaces, func(iface *netInterfaceV4) (contains bool) {
+func (ifaces dhcpInterfacesV4) find(ip netip.Addr) (iface4 *netInterface, ok bool) {
+	i := slices.IndexFunc(ifaces, func(iface *dhcpInterfaceV4) (contains bool) {
 		return iface.subnet.Contains(ip)
 	})
 	if i < 0 {
 		return nil, false
 	}
 
-	return &ifaces[i].netInterface, true
+	return ifaces[i].common, true
 }
 
 // options returns the implicit and explicit options for the interface.  The two