From 42c7cd6f8e1272d8b67a52632b9a44f2569d4fe3 Mon Sep 17 00:00:00 2001
From: Eugene Burkov <e.burkov@adguard.com>
Date: Wed, 10 Jul 2024 16:17:56 +0300
Subject: [PATCH] Pull request 2256: 4923 Better interfaces
Updates #4923.
Squashed commit of the following:
commit 0e40b41aa1e517a62d6076c4e7a57c607792ef01
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date: Wed Jul 10 15:28:16 2024 +0300
dhcpsvc: imp code, docs
commit 5463fdde473f84caaca229b53027e8183d5c6bdc
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date: Tue Jul 9 20:31:20 2024 +0300
dhcpsvc: imp ifaces
---
internal/dhcpsvc/dhcpsvc.go | 2 +-
internal/dhcpsvc/interface.go | 68 +++++++++++++++++++-------
internal/dhcpsvc/lease.go | 6 ---
internal/dhcpsvc/leaseindex.go | 2 +-
internal/dhcpsvc/server.go | 85 +++++++++++++++++++--------------
internal/dhcpsvc/server_test.go | 6 +--
internal/dhcpsvc/v4.go | 36 +++++++-------
internal/dhcpsvc/v6.go | 40 +++++++---------
8 files changed, 138 insertions(+), 107 deletions(-)
diff --git a/internal/dhcpsvc/dhcpsvc.go b/internal/dhcpsvc/dhcpsvc.go
index a7d76ab5..b6c77786 100644
--- a/internal/dhcpsvc/dhcpsvc.go
+++ b/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.
diff --git a/internal/dhcpsvc/interface.go b/internal/dhcpsvc/interface.go
index 13dadb4a..87c3de4d 100644
--- a/internal/dhcpsvc/interface.go
+++ b/internal/dhcpsvc/interface.go
@@ -3,42 +3,74 @@ 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
}
-// reset clears all the slices in iface for reuse.
-func (iface *netInterface) reset() {
- iface.leases = iface.leases[:0]
+// 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,
+ }
}
-// insertLease inserts the given lease into iface. It returns an error if the
+// reset clears all the slices in iface for reuse.
+func (iface *netInterface) reset() {
+ clear(iface.leases)
+}
+
+// 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
}
@@ -46,12 +78,13 @@ func (iface *netInterface) insertLease(l *Lease) (err error) {
// 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
}
@@ -59,12 +92,13 @@ func (iface *netInterface) updateLease(l *Lease) (prev *Lease, err error) {
// 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
}
diff --git a/internal/dhcpsvc/lease.go b/internal/dhcpsvc/lease.go
index a920a4f2..a855b7d5 100644
--- a/internal/dhcpsvc/lease.go
+++ b/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)
-}
diff --git a/internal/dhcpsvc/leaseindex.go b/internal/dhcpsvc/leaseindex.go
index 855d6b84..5502d2cf 100644
--- a/internal/dhcpsvc/leaseindex.go
+++ b/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
}
diff --git a/internal/dhcpsvc/server.go b/internal/dhcpsvc/server.go
index 745895ff..c8bab6e6 100644
--- a/internal/dhcpsvc/server.go
+++ b/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()
}
diff --git a/internal/dhcpsvc/server_test.go b/internal/dhcpsvc/server_test.go
index 181161a6..94509e37 100644
--- a/internal/dhcpsvc/server_test.go
+++ b/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())
}
diff --git a/internal/dhcpsvc/v4.go b/internal/dhcpsvc/v4.go
index 10624105..b5194a9f 100644
--- a/internal/dhcpsvc/v4.go
+++ b/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
diff --git a/internal/dhcpsvc/v6.go b/internal/dhcpsvc/v6.go
index a1ee56ac..dd75184e 100644
--- a/internal/dhcpsvc/v6.go
+++ b/internal/dhcpsvc/v6.go
@@ -62,10 +62,12 @@ func (c *IPv6Config) validate() (err error) {
return errors.Join(errs...)
}
-// netInterfaceV6 is a DHCP interface for IPv6 address family.
-//
-// TODO(e.burkov): Add options.
-type netInterfaceV6 struct {
+// dhcpInterfaceV6 is a DHCP interface for IPv6 address family.
+type dhcpInterfaceV6 struct {
+ // common is the common part of any network interface within the DHCP
+ // server.
+ common *netInterface
+
// rangeStart is the first IP address in the range.
rangeStart netip.Addr
@@ -78,10 +80,6 @@ type netInterfaceV6 struct {
// intersections with implicitOpts.
explicitOpts layers.DHCPv6Options
- // netInterface is embedded here to provide some common network interface
- // logic.
- netInterface
-
// raSLAACOnly defines if DHCP should send ICMPv6.RA packets without MO
// flags.
raSLAACOnly bool
@@ -90,16 +88,16 @@ type netInterfaceV6 struct {
raAllowSLAAC bool
}
-// newNetInterfaceV6 creates a new DHCP interface for IPv6 address family with
+// newDHCPInterfaceV6 creates a new DHCP interface for IPv6 address family with
// the given configuration.
//
// TODO(e.burkov): Validate properly.
-func newNetInterfaceV6(
+func newDHCPInterfaceV6(
ctx context.Context,
l *slog.Logger,
name string,
conf *IPv6Config,
-) (i *netInterfaceV6) {
+) (i *dhcpInterfaceV6) {
l = l.With(keyInterface, name, keyFamily, netutil.AddrFamilyIPv6)
if !conf.Enabled {
l.DebugContext(ctx, "disabled")
@@ -107,13 +105,9 @@ func newNetInterfaceV6(
return nil
}
- i = &netInterfaceV6{
- rangeStart: conf.RangeStart,
- netInterface: netInterface{
- name: name,
- leaseTTL: conf.LeaseDuration,
- logger: l,
- },
+ i = &dhcpInterfaceV6{
+ rangeStart: conf.RangeStart,
+ common: newNetInterface(name, l, conf.LeaseDuration),
raSLAACOnly: conf.RASLAACOnly,
raAllowSLAAC: conf.RAAllowSLAAC,
}
@@ -122,12 +116,12 @@ func newNetInterfaceV6(
return i
}
-// netInterfacesV4 is a slice of network interfaces of IPv4 address family.
-type netInterfacesV6 []*netInterfaceV6
+// dhcpInterfacesV6 is a slice of network interfaces of IPv6 address family.
+type dhcpInterfacesV6 []*dhcpInterfaceV6
// find returns the first network interface within ifaces containing ip. It
// returns false if there is no such interface.
-func (ifaces netInterfacesV6) find(ip netip.Addr) (iface6 *netInterface, ok bool) {
+func (ifaces dhcpInterfacesV6) find(ip netip.Addr) (iface6 *netInterface, ok bool) {
// prefLen is the length of prefix to match ip against.
//
// TODO(e.burkov): DHCPv6 inherits the weird behavior of legacy
@@ -136,7 +130,7 @@ func (ifaces netInterfacesV6) find(ip netip.Addr) (iface6 *netInterface, ok bool
// be used instead.
const prefLen = netutil.IPv6BitLen - 8
- i := slices.IndexFunc(ifaces, func(iface *netInterfaceV6) (contains bool) {
+ i := slices.IndexFunc(ifaces, func(iface *dhcpInterfaceV6) (contains bool) {
return !ip.Less(iface.rangeStart) &&
netip.PrefixFrom(iface.rangeStart, prefLen).Contains(ip)
})
@@ -144,7 +138,7 @@ func (ifaces netInterfacesV6) find(ip netip.Addr) (iface6 *netInterface, ok bool
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