From beeb8f0522a756205e4c8d6c1ff909ba8ccafb0d Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Wed, 3 Jul 2024 15:29:54 +0300 Subject: [PATCH] Pull request 2245: 4923 gopacket DHCP vol.8 Updates #4923. Squashed commit of the following: commit 0bfccf8bc1e63c4f5a01ce7f268e767969b368a0 Merge: 305f9fe2f 0e5e8e4dd Author: Eugene Burkov Date: Wed Jul 3 15:12:43 2024 +0300 Merge branch 'master' into 4923-gopacket-dhcp-vol.8 commit 305f9fe2fec033f28385dfe2bbee6a27a83b9702 Author: Eugene Burkov Date: Tue Jul 2 17:03:01 2024 +0300 dhcpsvc: adjust interface commit f05b9f42e2f50325ddaf09b5fed84b62e48c5120 Author: Eugene Burkov Date: Tue Jul 2 16:59:39 2024 +0300 dhcpsvc: use logger commit 4779f945baf9c8722d07d589928a86290a37d3ab Author: Eugene Burkov Date: Tue Jul 2 14:59:22 2024 +0300 dhcpsvc: add todo commit ae1713e5f717a66863eb0289e3aa66c7069ac8bf Author: Eugene Burkov Date: Mon Jul 1 15:49:22 2024 +0300 dhcpsvc: use slog --- internal/dhcpsvc/config.go | 40 +++-- internal/dhcpsvc/config_test.go | 3 +- internal/dhcpsvc/dhcpsvc.go | 24 ++- internal/dhcpsvc/interface.go | 4 + internal/dhcpsvc/server.go | 95 +++++++--- internal/dhcpsvc/server_test.go | 58 ++++-- internal/dhcpsvc/v4.go | 253 +++++++++++++++------------ internal/dhcpsvc/v4_internal_test.go | 8 +- internal/dhcpsvc/v6.go | 95 +++++----- 9 files changed, 368 insertions(+), 212 deletions(-) diff --git a/internal/dhcpsvc/config.go b/internal/dhcpsvc/config.go index bb5c46a0..c1d7910d 100644 --- a/internal/dhcpsvc/config.go +++ b/internal/dhcpsvc/config.go @@ -2,11 +2,12 @@ package dhcpsvc import ( "fmt" - "slices" + "log/slog" "time" + "github.com/AdguardTeam/golibs/errors" + "github.com/AdguardTeam/golibs/mapsutil" "github.com/AdguardTeam/golibs/netutil" - "golang.org/x/exp/maps" ) // Config is the configuration for the DHCP service. @@ -15,6 +16,9 @@ type Config struct { // interface identified by its name. Interfaces map[string]*InterfaceConfig + // Logger will be used to log the DHCP events. + Logger *slog.Logger + // LocalDomainName is the top-level domain name to use for resolving DHCP // clients' hostnames. LocalDomainName string @@ -38,36 +42,44 @@ type InterfaceConfig struct { } // Validate returns an error in conf if any. +// +// TODO(e.burkov): Unexport and rewrite the test. func (conf *Config) Validate() (err error) { switch { case conf == nil: return errNilConfig case !conf.Enabled: return nil - case conf.ICMPTimeout < 0: - return newMustErr("icmp timeout", "be non-negative", conf.ICMPTimeout) + } + + var errs []error + if conf.ICMPTimeout < 0 { + err = newMustErr("icmp timeout", "be non-negative", conf.ICMPTimeout) + errs = append(errs, err) } err = netutil.ValidateDomainName(conf.LocalDomainName) if err != nil { // Don't wrap the error since it's informative enough as is. - return err + errs = append(errs, err) } if len(conf.Interfaces) == 0 { - return errNoInterfaces + errs = append(errs, errNoInterfaces) + + return errors.Join(errs...) } - ifaces := maps.Keys(conf.Interfaces) - slices.Sort(ifaces) - - for _, iface := range ifaces { - if err = conf.Interfaces[iface].validate(); err != nil { - return fmt.Errorf("interface %q: %w", iface, err) + mapsutil.SortedRange(conf.Interfaces, func(iface string, ic *InterfaceConfig) (ok bool) { + err = ic.validate() + if err != nil { + errs = append(errs, fmt.Errorf("interface %q: %w", iface, err)) } - } - return nil + return true + }) + + return errors.Join(errs...) } // validate returns an error in ic, if any. diff --git a/internal/dhcpsvc/config_test.go b/internal/dhcpsvc/config_test.go index 6663d378..aa87b0d6 100644 --- a/internal/dhcpsvc/config_test.go +++ b/internal/dhcpsvc/config_test.go @@ -23,7 +23,8 @@ func TestConfig_Validate(t *testing.T) { }, { name: "empty", conf: &dhcpsvc.Config{ - Enabled: true, + Enabled: true, + Interfaces: testInterfaceConf, }, wantErrMsg: `bad domain name "": domain name is empty`, }, { diff --git a/internal/dhcpsvc/dhcpsvc.go b/internal/dhcpsvc/dhcpsvc.go index 41e0037e..a7d76ab5 100644 --- a/internal/dhcpsvc/dhcpsvc.go +++ b/internal/dhcpsvc/dhcpsvc.go @@ -11,6 +11,14 @@ import ( "github.com/AdguardTeam/AdGuardHome/internal/next/agh" ) +const ( + // keyInterface is the key for logging the network interface name. + keyInterface = "iface" + + // keyFamily is the key for logging the handled address family. + keyFamily = "family" +) + // Interface is a DHCP service. // // TODO(e.burkov): Separate HostByIP, MACByIP, IPByHost into a separate @@ -50,21 +58,21 @@ type Interface interface { // AddLease adds a new DHCP lease. l must be valid. It returns an error if // l already exists. - AddLease(l *Lease) (err error) + AddLease(ctx context.Context, l *Lease) (err error) // UpdateStaticLease replaces an existing static DHCP lease. l must be // valid. It returns an error if the lease with the given hardware address // doesn't exist or if other values match another existing lease. - UpdateStaticLease(l *Lease) (err error) + UpdateStaticLease(ctx context.Context, l *Lease) (err error) // RemoveLease removes an existing DHCP lease. l must be valid. It returns // an error if there is no lease equal to l. - RemoveLease(l *Lease) (err error) + RemoveLease(ctx context.Context, l *Lease) (err error) // Reset removes all the DHCP leases. // // TODO(e.burkov): If it's really needed? - Reset() (err error) + Reset(ctx context.Context) (err error) } // Empty is an [Interface] implementation that does nothing. @@ -101,13 +109,13 @@ func (Empty) IPByHost(_ string) (ip netip.Addr) { return netip.Addr{} } func (Empty) Leases() (leases []*Lease) { return nil } // AddLease implements the [Interface] interface for Empty. -func (Empty) AddLease(_ *Lease) (err error) { return nil } +func (Empty) AddLease(_ context.Context, _ *Lease) (err error) { return nil } // UpdateStaticLease implements the [Interface] interface for Empty. -func (Empty) UpdateStaticLease(_ *Lease) (err error) { return nil } +func (Empty) UpdateStaticLease(_ context.Context, _ *Lease) (err error) { return nil } // RemoveLease implements the [Interface] interface for Empty. -func (Empty) RemoveLease(_ *Lease) (err error) { return nil } +func (Empty) RemoveLease(_ context.Context, _ *Lease) (err error) { return nil } // Reset implements the [Interface] interface for Empty. -func (Empty) Reset() (err error) { return nil } +func (Empty) Reset(_ context.Context) (err error) { return nil } diff --git a/internal/dhcpsvc/interface.go b/internal/dhcpsvc/interface.go index ebb225e6..13dadb4a 100644 --- a/internal/dhcpsvc/interface.go +++ b/internal/dhcpsvc/interface.go @@ -2,6 +2,7 @@ package dhcpsvc import ( "fmt" + "log/slog" "slices" "time" ) @@ -11,6 +12,9 @@ import ( // // TODO(e.burkov): Add other methods as [DHCPServer] evolves. type netInterface struct { + // logger logs the events related to the network interface. + logger *slog.Logger + // name is the name of the network interface. name string diff --git a/internal/dhcpsvc/server.go b/internal/dhcpsvc/server.go index bc354b00..cd1e93b2 100644 --- a/internal/dhcpsvc/server.go +++ b/internal/dhcpsvc/server.go @@ -1,16 +1,17 @@ package dhcpsvc import ( + "context" "fmt" + "log/slog" "net" "net/netip" - "slices" "sync" "sync/atomic" "time" "github.com/AdguardTeam/golibs/errors" - "golang.org/x/exp/maps" + "github.com/AdguardTeam/golibs/mapsutil" ) // DHCPServer is a DHCP server for both IPv4 and IPv6 address families. @@ -19,6 +20,9 @@ type DHCPServer struct { // information about its clients. enabled *atomic.Bool + // logger logs common DHCP events. + logger *slog.Logger + // localTLD is the top-level domain name to use for resolving DHCP clients' // hostnames. localTLD string @@ -43,8 +47,11 @@ type DHCPServer struct { // error if the given configuration can't be used. // // TODO(e.burkov): Use. -func New(conf *Config) (srv *DHCPServer, err error) { +func New(ctx context.Context, conf *Config) (srv *DHCPServer, err error) { + l := conf.Logger if !conf.Enabled { + l.DebugContext(ctx, "disabled") + // TODO(e.burkov): Perhaps return [Empty]? return nil, nil } @@ -52,27 +59,26 @@ func New(conf *Config) (srv *DHCPServer, err error) { // 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 - ifaceNames := maps.Keys(conf.Interfaces) - slices.Sort(ifaceNames) - - var i4 *netInterfaceV4 - var i6 *netInterfaceV6 - - for _, ifaceName := range ifaceNames { - iface := conf.Interfaces[ifaceName] - - i4, err = newNetInterfaceV4(ifaceName, iface.IPv4) + 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 { - return nil, fmt.Errorf("interface %q: ipv4: %w", ifaceName, err) + errs = append(errs, fmt.Errorf("interface %q: ipv4: %w", name, err)) } else if i4 != nil { ifaces4 = append(ifaces4, i4) } - i6 = newNetInterfaceV6(ifaceName, iface.IPv6) + i6 := newNetInterfaceV6(ctx, l, name, iface.IPv6) if i6 != nil { ifaces6 = append(ifaces6, i6) } + + return true + }) + if err = errors.Join(errs...); err != nil { + return nil, err } enabled := &atomic.Bool{} @@ -80,6 +86,7 @@ func New(conf *Config) (srv *DHCPServer, err error) { srv = &DHCPServer{ enabled: enabled, + logger: l, localTLD: conf.LocalDomainName, leasesMu: &sync.RWMutex{}, leases: newLeaseIndex(), @@ -159,7 +166,7 @@ func (srv *DHCPServer) IPByHost(host string) (ip netip.Addr) { } // Reset implements the [Interface] interface for *DHCPServer. -func (srv *DHCPServer) Reset() (err error) { +func (srv *DHCPServer) Reset(ctx context.Context) (err error) { srv.leasesMu.Lock() defer srv.leasesMu.Unlock() @@ -171,11 +178,13 @@ func (srv *DHCPServer) Reset() (err error) { } srv.leases.clear() + srv.logger.DebugContext(ctx, "reset leases") + return nil } // AddLease implements the [Interface] interface for *DHCPServer. -func (srv *DHCPServer) AddLease(l *Lease) (err error) { +func (srv *DHCPServer) AddLease(ctx context.Context, l *Lease) (err error) { defer func() { err = errors.Annotate(err, "adding lease: %w") }() addr := l.IP @@ -188,13 +197,27 @@ func (srv *DHCPServer) AddLease(l *Lease) (err error) { srv.leasesMu.Lock() defer srv.leasesMu.Unlock() - return srv.leases.add(l, iface) + err = srv.leases.add(l, iface) + if err != nil { + // Don't wrap the error since there is already an annotation deferred. + return err + } + + iface.logger.DebugContext( + ctx, "added lease", + "hostname", l.Hostname, + "ip", l.IP, + "mac", l.HWAddr, + "static", l.IsStatic, + ) + + return nil } // UpdateStaticLease implements the [Interface] interface for *DHCPServer. // // TODO(e.burkov): Support moving leases between interfaces. -func (srv *DHCPServer) UpdateStaticLease(l *Lease) (err error) { +func (srv *DHCPServer) UpdateStaticLease(ctx context.Context, l *Lease) (err error) { defer func() { err = errors.Annotate(err, "updating static lease: %w") }() addr := l.IP @@ -207,11 +230,25 @@ func (srv *DHCPServer) UpdateStaticLease(l *Lease) (err error) { srv.leasesMu.Lock() defer srv.leasesMu.Unlock() - return srv.leases.update(l, iface) + err = srv.leases.update(l, iface) + if err != nil { + // Don't wrap the error since there is already an annotation deferred. + return err + } + + iface.logger.DebugContext( + ctx, "updated lease", + "hostname", l.Hostname, + "ip", l.IP, + "mac", l.HWAddr, + "static", l.IsStatic, + ) + + return nil } // RemoveLease implements the [Interface] interface for *DHCPServer. -func (srv *DHCPServer) RemoveLease(l *Lease) (err error) { +func (srv *DHCPServer) RemoveLease(ctx context.Context, l *Lease) (err error) { defer func() { err = errors.Annotate(err, "removing lease: %w") }() addr := l.IP @@ -224,7 +261,21 @@ func (srv *DHCPServer) RemoveLease(l *Lease) (err error) { srv.leasesMu.Lock() defer srv.leasesMu.Unlock() - return srv.leases.remove(l, iface) + err = srv.leases.remove(l, iface) + if err != nil { + // Don't wrap the error since there is already an annotation deferred. + return err + } + + iface.logger.DebugContext( + ctx, "removed lease", + "hostname", l.Hostname, + "ip", l.IP, + "mac", l.HWAddr, + "static", l.IsStatic, + ) + + return nil } // ifaceForAddr returns the handled network interface for the given IP address, diff --git a/internal/dhcpsvc/server_test.go b/internal/dhcpsvc/server_test.go index 6d5bc9d8..5f6f002f 100644 --- a/internal/dhcpsvc/server_test.go +++ b/internal/dhcpsvc/server_test.go @@ -8,6 +8,7 @@ import ( "time" "github.com/AdguardTeam/AdGuardHome/internal/dhcpsvc" + "github.com/AdguardTeam/golibs/logutil/slogutil" "github.com/AdguardTeam/golibs/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -16,6 +17,12 @@ import ( // testLocalTLD is a common local TLD for tests. const testLocalTLD = "local" +// testTimeout is a common timeout for tests and contexts. +const testTimeout time.Duration = 10 * time.Second + +// discardLog is a logger to discard test output. +var discardLog = slogutil.NewDiscardLogger() + // testInterfaceConf is a common set of interface configurations for tests. var testInterfaceConf = map[string]*dhcpsvc.InterfaceConfig{ "eth0": { @@ -103,6 +110,7 @@ func TestNew(t *testing.T) { }{{ conf: &dhcpsvc.Config{ Enabled: true, + Logger: discardLog, LocalDomainName: testLocalTLD, Interfaces: map[string]*dhcpsvc.InterfaceConfig{ "eth0": { @@ -116,6 +124,7 @@ func TestNew(t *testing.T) { }, { conf: &dhcpsvc.Config{ Enabled: true, + Logger: discardLog, LocalDomainName: testLocalTLD, Interfaces: map[string]*dhcpsvc.InterfaceConfig{ "eth0": { @@ -129,6 +138,7 @@ func TestNew(t *testing.T) { }, { conf: &dhcpsvc.Config{ Enabled: true, + Logger: discardLog, LocalDomainName: testLocalTLD, Interfaces: map[string]*dhcpsvc.InterfaceConfig{ "eth0": { @@ -143,6 +153,7 @@ func TestNew(t *testing.T) { }, { conf: &dhcpsvc.Config{ Enabled: true, + Logger: discardLog, LocalDomainName: testLocalTLD, Interfaces: map[string]*dhcpsvc.InterfaceConfig{ "eth0": { @@ -156,17 +167,22 @@ func TestNew(t *testing.T) { `range start 127.0.0.1 is not within 192.168.0.1/24`, }} + ctx := testutil.ContextWithTimeout(t, testTimeout) + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - _, err := dhcpsvc.New(tc.conf) + _, err := dhcpsvc.New(ctx, tc.conf) testutil.AssertErrorMsg(t, tc.wantErrMsg, err) }) } } func TestDHCPServer_AddLease(t *testing.T) { - srv, err := dhcpsvc.New(&dhcpsvc.Config{ + ctx := testutil.ContextWithTimeout(t, testTimeout) + + srv, err := dhcpsvc.New(ctx, &dhcpsvc.Config{ Enabled: true, + Logger: discardLog, LocalDomainName: testLocalTLD, Interfaces: testInterfaceConf, }) @@ -186,7 +202,7 @@ func TestDHCPServer_AddLease(t *testing.T) { mac2 := mustParseMAC(t, "06:05:04:03:02:01") mac3 := mustParseMAC(t, "02:03:04:05:06:07") - require.NoError(t, srv.AddLease(&dhcpsvc.Lease{ + require.NoError(t, srv.AddLease(ctx, &dhcpsvc.Lease{ Hostname: host1, IP: ip1, HWAddr: mac1, @@ -261,14 +277,17 @@ func TestDHCPServer_AddLease(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - testutil.AssertErrorMsg(t, tc.wantErrMsg, srv.AddLease(tc.lease)) + testutil.AssertErrorMsg(t, tc.wantErrMsg, srv.AddLease(ctx, tc.lease)) }) } } func TestDHCPServer_index(t *testing.T) { - srv, err := dhcpsvc.New(&dhcpsvc.Config{ + ctx := testutil.ContextWithTimeout(t, testTimeout) + + srv, err := dhcpsvc.New(ctx, &dhcpsvc.Config{ Enabled: true, + Logger: discardLog, LocalDomainName: testLocalTLD, Interfaces: testInterfaceConf, }) @@ -313,7 +332,7 @@ func TestDHCPServer_index(t *testing.T) { IsStatic: true, }} for _, l := range leases { - require.NoError(t, srv.AddLease(l)) + require.NoError(t, srv.AddLease(ctx, l)) } t.Run("ip_idx", func(t *testing.T) { @@ -342,8 +361,11 @@ func TestDHCPServer_index(t *testing.T) { } func TestDHCPServer_UpdateStaticLease(t *testing.T) { - srv, err := dhcpsvc.New(&dhcpsvc.Config{ + ctx := testutil.ContextWithTimeout(t, testTimeout) + + srv, err := dhcpsvc.New(ctx, &dhcpsvc.Config{ Enabled: true, + Logger: discardLog, LocalDomainName: testLocalTLD, Interfaces: testInterfaceConf, }) @@ -386,7 +408,7 @@ func TestDHCPServer_UpdateStaticLease(t *testing.T) { IsStatic: true, }} for _, l := range leases { - require.NoError(t, srv.AddLease(l)) + require.NoError(t, srv.AddLease(ctx, l)) } testCases := []struct { @@ -456,14 +478,17 @@ func TestDHCPServer_UpdateStaticLease(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - testutil.AssertErrorMsg(t, tc.wantErrMsg, srv.UpdateStaticLease(tc.lease)) + testutil.AssertErrorMsg(t, tc.wantErrMsg, srv.UpdateStaticLease(ctx, tc.lease)) }) } } func TestDHCPServer_RemoveLease(t *testing.T) { - srv, err := dhcpsvc.New(&dhcpsvc.Config{ + ctx := testutil.ContextWithTimeout(t, testTimeout) + + srv, err := dhcpsvc.New(ctx, &dhcpsvc.Config{ Enabled: true, + Logger: discardLog, LocalDomainName: testLocalTLD, Interfaces: testInterfaceConf, }) @@ -495,7 +520,7 @@ func TestDHCPServer_RemoveLease(t *testing.T) { IsStatic: true, }} for _, l := range leases { - require.NoError(t, srv.AddLease(l)) + require.NoError(t, srv.AddLease(ctx, l)) } testCases := []struct { @@ -546,7 +571,7 @@ func TestDHCPServer_RemoveLease(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - testutil.AssertErrorMsg(t, tc.wantErrMsg, srv.RemoveLease(tc.lease)) + testutil.AssertErrorMsg(t, tc.wantErrMsg, srv.RemoveLease(ctx, tc.lease)) }) } @@ -554,8 +579,11 @@ func TestDHCPServer_RemoveLease(t *testing.T) { } func TestDHCPServer_Reset(t *testing.T) { - srv, err := dhcpsvc.New(&dhcpsvc.Config{ + ctx := testutil.ContextWithTimeout(t, testTimeout) + + srv, err := dhcpsvc.New(ctx, &dhcpsvc.Config{ Enabled: true, + Logger: discardLog, LocalDomainName: testLocalTLD, Interfaces: testInterfaceConf, }) @@ -584,12 +612,12 @@ func TestDHCPServer_Reset(t *testing.T) { }} for _, l := range leases { - require.NoError(t, srv.AddLease(l)) + require.NoError(t, srv.AddLease(ctx, l)) } require.Len(t, srv.Leases(), len(leases)) - require.NoError(t, srv.Reset()) + require.NoError(t, srv.Reset(ctx)) assert.Empty(t, srv.Leases()) } diff --git a/internal/dhcpsvc/v4.go b/internal/dhcpsvc/v4.go index a93f1103..09df8013 100644 --- a/internal/dhcpsvc/v4.go +++ b/internal/dhcpsvc/v4.go @@ -1,13 +1,15 @@ package dhcpsvc import ( + "context" "fmt" + "log/slog" "net" "net/netip" "slices" "time" - "github.com/AdguardTeam/golibs/log" + "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/netutil" "github.com/google/gopacket/layers" ) @@ -43,25 +45,133 @@ type IPv4Config struct { } // validate returns an error in conf if any. -func (conf *IPv4Config) validate() (err error) { - switch { - case conf == nil: +func (c *IPv4Config) validate() (err error) { + if c == nil { return errNilConfig - case !conf.Enabled: - return nil - case !conf.GatewayIP.Is4(): - return newMustErr("gateway ip", "be a valid ipv4", conf.GatewayIP) - case !conf.SubnetMask.Is4(): - return newMustErr("subnet mask", "be a valid ipv4 cidr mask", conf.SubnetMask) - case !conf.RangeStart.Is4(): - return newMustErr("range start", "be a valid ipv4", conf.RangeStart) - case !conf.RangeEnd.Is4(): - return newMustErr("range end", "be a valid ipv4", conf.RangeEnd) - case conf.LeaseDuration <= 0: - return newMustErr("lease duration", "be less than %d", conf.LeaseDuration) - default: + } else if !c.Enabled { return nil } + + var errs []error + + if !c.GatewayIP.Is4() { + err = newMustErr("gateway ip", "be a valid ipv4", c.GatewayIP) + errs = append(errs, err) + } + + if !c.SubnetMask.Is4() { + err = newMustErr("subnet mask", "be a valid ipv4 cidr mask", c.SubnetMask) + errs = append(errs, err) + } + + if !c.RangeStart.Is4() { + err = newMustErr("range start", "be a valid ipv4", c.RangeStart) + errs = append(errs, err) + } + + if !c.RangeEnd.Is4() { + err = newMustErr("range end", "be a valid ipv4", c.RangeEnd) + errs = append(errs, err) + } + + if c.LeaseDuration <= 0 { + err = newMustErr("icmp timeout", "be positive", c.LeaseDuration) + errs = append(errs, err) + } + + return errors.Join(errs...) +} + +// netInterfaceV4 is a DHCP interface for IPv4 address family. +type netInterfaceV4 struct { + // gateway is the IP address of the network gateway. + gateway netip.Addr + + // subnet is the network subnet. + subnet netip.Prefix + + // addrSpace is the IPv4 address space allocated for leasing. + addrSpace ipRange + + // implicitOpts are the options listed in Appendix A of RFC 2131 and + // initialized with default values. It must not have intersections with + // explicitOpts. + implicitOpts layers.DHCPOptions + + // 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 +// the given configuration. It returns an error if the given configuration +// can't be used. +func newNetInterfaceV4( + ctx context.Context, + l *slog.Logger, + name string, + conf *IPv4Config, +) (i *netInterfaceV4, err error) { + l = l.With( + keyInterface, name, + keyFamily, netutil.AddrFamilyIPv4, + ) + if !conf.Enabled { + l.DebugContext(ctx, "disabled") + + return nil, nil + } + + maskLen, _ := net.IPMask(conf.SubnetMask.AsSlice()).Size() + subnet := netip.PrefixFrom(conf.GatewayIP, maskLen) + + switch { + case !subnet.Contains(conf.RangeStart): + return nil, fmt.Errorf("range start %s is not within %s", conf.RangeStart, subnet) + case !subnet.Contains(conf.RangeEnd): + return nil, fmt.Errorf("range end %s is not within %s", conf.RangeEnd, subnet) + } + + addrSpace, err := newIPRange(conf.RangeStart, conf.RangeEnd) + if err != nil { + return nil, err + } else if addrSpace.contains(conf.GatewayIP) { + return nil, fmt.Errorf("gateway ip %s in the ip range %s", conf.GatewayIP, addrSpace) + } + + i = &netInterfaceV4{ + gateway: conf.GatewayIP, + subnet: subnet, + addrSpace: addrSpace, + netInterface: netInterface{ + name: name, + leaseTTL: conf.LeaseDuration, + logger: l, + }, + } + 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 + +// 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) { + return iface.subnet.Contains(ip) + }) + if i < 0 { + return nil, false + } + + return &ifaces[i].netInterface, true } // options returns the implicit and explicit options for the interface. The two @@ -69,14 +179,14 @@ func (conf *IPv4Config) validate() (err error) { // values. // // TODO(e.burkov): DRY with the IPv6 version. -func (conf *IPv4Config) options() (implicit, explicit layers.DHCPOptions) { +func (c *IPv4Config) options(ctx context.Context, l *slog.Logger) (imp, exp layers.DHCPOptions) { // Set default values of host configuration parameters listed in Appendix A // of RFC-2131. - implicit = layers.DHCPOptions{ + imp = layers.DHCPOptions{ // Values From Configuration - layers.NewDHCPOption(layers.DHCPOptSubnetMask, conf.SubnetMask.AsSlice()), - layers.NewDHCPOption(layers.DHCPOptRouter, conf.GatewayIP.AsSlice()), + layers.NewDHCPOption(layers.DHCPOptSubnetMask, c.SubnetMask.AsSlice()), + layers.NewDHCPOption(layers.DHCPOptRouter, c.GatewayIP.AsSlice()), // IP-Layer Per Host @@ -228,110 +338,29 @@ func (conf *IPv4Config) options() (implicit, explicit layers.DHCPOptions) { // See https://datatracker.ietf.org/doc/html/rfc1122#section-4.2.3.6. layers.NewDHCPOption(layers.DHCPOptTCPKeepAliveGarbage, []byte{0x1}), } - slices.SortFunc(implicit, compareV4OptionCodes) + slices.SortFunc(imp, compareV4OptionCodes) // Set values for explicitly configured options. - for _, exp := range conf.Options { - i, found := slices.BinarySearchFunc(implicit, exp, compareV4OptionCodes) + for _, o := range c.Options { + i, found := slices.BinarySearchFunc(imp, o, compareV4OptionCodes) if found { - implicit = slices.Delete(implicit, i, i+1) + imp = slices.Delete(imp, i, i+1) } - i, found = slices.BinarySearchFunc(explicit, exp, compareV4OptionCodes) - if exp.Length > 0 { - explicit = slices.Insert(explicit, i, exp) + i, found = slices.BinarySearchFunc(exp, o, compareV4OptionCodes) + if o.Length > 0 { + exp = slices.Insert(exp, i, o) } else if found { - explicit = slices.Delete(explicit, i, i+1) + exp = slices.Delete(exp, i, i+1) } } - log.Debug("dhcpsvc: v4: implicit options: %s", implicit) - log.Debug("dhcpsvc: v4: explicit options: %s", explicit) + l.DebugContext(ctx, "options", "implicit", imp, "explicit", exp) - return implicit, explicit + return imp, exp } // compareV4OptionCodes compares option codes of a and b. func compareV4OptionCodes(a, b layers.DHCPOption) (res int) { return int(a.Type) - int(b.Type) } - -// netInterfaceV4 is a DHCP interface for IPv4 address family. -type netInterfaceV4 struct { - // gateway is the IP address of the network gateway. - gateway netip.Addr - - // subnet is the network subnet. - subnet netip.Prefix - - // addrSpace is the IPv4 address space allocated for leasing. - addrSpace ipRange - - // implicitOpts are the options listed in Appendix A of RFC 2131 and - // initialized with default values. It must not have intersections with - // explicitOpts. - implicitOpts layers.DHCPOptions - - // 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 -// the given configuration. It returns an error if the given configuration -// can't be used. -func newNetInterfaceV4(name string, conf *IPv4Config) (i *netInterfaceV4, err error) { - if !conf.Enabled { - return nil, nil - } - - maskLen, _ := net.IPMask(conf.SubnetMask.AsSlice()).Size() - subnet := netip.PrefixFrom(conf.GatewayIP, maskLen) - - switch { - case !subnet.Contains(conf.RangeStart): - return nil, fmt.Errorf("range start %s is not within %s", conf.RangeStart, subnet) - case !subnet.Contains(conf.RangeEnd): - return nil, fmt.Errorf("range end %s is not within %s", conf.RangeEnd, subnet) - } - - addrSpace, err := newIPRange(conf.RangeStart, conf.RangeEnd) - if err != nil { - return nil, err - } else if addrSpace.contains(conf.GatewayIP) { - return nil, fmt.Errorf("gateway ip %s in the ip range %s", conf.GatewayIP, addrSpace) - } - - i = &netInterfaceV4{ - gateway: conf.GatewayIP, - subnet: subnet, - addrSpace: addrSpace, - netInterface: netInterface{ - name: name, - leaseTTL: conf.LeaseDuration, - }, - } - i.implicitOpts, i.explicitOpts = conf.options() - - return i, nil -} - -// netInterfacesV4 is a slice of network interfaces of IPv4 address family. -type netInterfacesV4 []*netInterfaceV4 - -// 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) { - return iface.subnet.Contains(ip) - }) - if i < 0 { - return nil, false - } - - return &ifaces[i].netInterface, true -} diff --git a/internal/dhcpsvc/v4_internal_test.go b/internal/dhcpsvc/v4_internal_test.go index 0b65366e..7aa64505 100644 --- a/internal/dhcpsvc/v4_internal_test.go +++ b/internal/dhcpsvc/v4_internal_test.go @@ -3,7 +3,10 @@ package dhcpsvc import ( "net/netip" "testing" + "time" + "github.com/AdguardTeam/golibs/logutil/slogutil" + "github.com/AdguardTeam/golibs/testutil" "github.com/google/gopacket/layers" "github.com/stretchr/testify/assert" ) @@ -75,9 +78,12 @@ func TestIPv4Config_Options(t *testing.T) { wantExplicit: layers.DHCPOptions{opt1}, }} + ctx := testutil.ContextWithTimeout(t, time.Second) + l := slogutil.NewDiscardLogger() + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - imp, exp := tc.conf.options() + imp, exp := tc.conf.options(ctx, l) assert.Equal(t, tc.wantExplicit, exp) for c := range exp { diff --git a/internal/dhcpsvc/v6.go b/internal/dhcpsvc/v6.go index 09342569..a1ee56ac 100644 --- a/internal/dhcpsvc/v6.go +++ b/internal/dhcpsvc/v6.go @@ -1,12 +1,14 @@ package dhcpsvc import ( + "context" "fmt" + "log/slog" "net/netip" "slices" "time" - "github.com/AdguardTeam/golibs/log" + "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/netutil" "github.com/google/gopacket/layers" ) @@ -38,50 +40,26 @@ type IPv6Config struct { } // validate returns an error in conf if any. -func (conf *IPv6Config) validate() (err error) { - switch { - case conf == nil: +func (c *IPv6Config) validate() (err error) { + if c == nil { return errNilConfig - case !conf.Enabled: - return nil - case !conf.RangeStart.Is6(): - return fmt.Errorf("range start %s should be a valid ipv6", conf.RangeStart) - case conf.LeaseDuration <= 0: - return fmt.Errorf("lease duration %s must be positive", conf.LeaseDuration) - default: + } else if !c.Enabled { return nil } -} -// options returns the implicit and explicit options for the interface. The two -// lists are disjoint and the implicit options are initialized with default -// values. -// -// TODO(e.burkov): Add implicit options according to RFC. -func (conf *IPv6Config) options() (implicit, explicit layers.DHCPv6Options) { - // Set default values of host configuration parameters listed in RFC 8415. - implicit = layers.DHCPv6Options{} - slices.SortFunc(implicit, compareV6OptionCodes) + var errs []error - // Set values for explicitly configured options. - for _, exp := range conf.Options { - i, found := slices.BinarySearchFunc(implicit, exp, compareV6OptionCodes) - if found { - implicit = slices.Delete(implicit, i, i+1) - } - - explicit = append(explicit, exp) + if !c.RangeStart.Is6() { + err = fmt.Errorf("range start %s should be a valid ipv6", c.RangeStart) + errs = append(errs, err) } - log.Debug("dhcpsvc: v6: implicit options: %s", implicit) - log.Debug("dhcpsvc: v6: explicit options: %s", explicit) + if c.LeaseDuration <= 0 { + err = fmt.Errorf("lease duration %s must be positive", c.LeaseDuration) + errs = append(errs, err) + } - return implicit, explicit -} - -// compareV6OptionCodes compares option codes of a and b. -func compareV6OptionCodes(a, b layers.DHCPv6Option) (res int) { - return int(a.Code) - int(b.Code) + return errors.Join(errs...) } // netInterfaceV6 is a DHCP interface for IPv6 address family. @@ -116,8 +94,16 @@ type netInterfaceV6 struct { // the given configuration. // // TODO(e.burkov): Validate properly. -func newNetInterfaceV6(name string, conf *IPv6Config) (i *netInterfaceV6) { +func newNetInterfaceV6( + ctx context.Context, + l *slog.Logger, + name string, + conf *IPv6Config, +) (i *netInterfaceV6) { + l = l.With(keyInterface, name, keyFamily, netutil.AddrFamilyIPv6) if !conf.Enabled { + l.DebugContext(ctx, "disabled") + return nil } @@ -126,11 +112,12 @@ func newNetInterfaceV6(name string, conf *IPv6Config) (i *netInterfaceV6) { netInterface: netInterface{ name: name, leaseTTL: conf.LeaseDuration, + logger: l, }, raSLAACOnly: conf.RASLAACOnly, raAllowSLAAC: conf.RAAllowSLAAC, } - i.implicitOpts, i.explicitOpts = conf.options() + i.implicitOpts, i.explicitOpts = conf.options(ctx, l) return i } @@ -159,3 +146,33 @@ func (ifaces netInterfacesV6) find(ip netip.Addr) (iface6 *netInterface, ok bool return &ifaces[i].netInterface, true } + +// options returns the implicit and explicit options for the interface. The two +// lists are disjoint and the implicit options are initialized with default +// values. +// +// TODO(e.burkov): Add implicit options according to RFC. +func (c *IPv6Config) options(ctx context.Context, l *slog.Logger) (imp, exp layers.DHCPv6Options) { + // Set default values of host configuration parameters listed in RFC 8415. + imp = layers.DHCPv6Options{} + slices.SortFunc(imp, compareV6OptionCodes) + + // Set values for explicitly configured options. + for _, e := range c.Options { + i, found := slices.BinarySearchFunc(imp, e, compareV6OptionCodes) + if found { + imp = slices.Delete(imp, i, i+1) + } + + exp = append(exp, e) + } + + l.DebugContext(ctx, "options", "implicit", imp, "explicit", exp) + + return imp, exp +} + +// compareV6OptionCodes compares option codes of a and b. +func compareV6OptionCodes(a, b layers.DHCPv6Option) (res int) { + return int(a.Code) - int(b.Code) +}