dhcpd: imp conf

internal/dhcpd/config.go

@@ -1,10 +1,14 @@
 package dhcpd
 
 import (
+	"fmt"
 	"net"
 	"time"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
+	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
+	"github.com/AdguardTeam/golibs/errors"
+	"github.com/AdguardTeam/golibs/netutil"
 )
 
 // ServerConfig is the configuration for the DHCP server.  The order of YAML
@@ -61,7 +65,7 @@ type V4ServerConf struct {
 	Enabled       bool   `yaml:"-" json:"-"`
 	InterfaceName string `yaml:"-" json:"-"`
 
-	GatewayIP  net.IP `yaml:"gateway_ip" json:"gateway_ip"`
+	GatewayIP  net.IP `yaml:"gateway_ip"  json:"gateway_ip"`
 	SubnetMask net.IP `yaml:"subnet_mask" json:"subnet_mask"`
 	// broadcastIP is the broadcasting address pre-calculated from the
 	// configured gateway IP and subnet mask.
@@ -70,7 +74,7 @@ type V4ServerConf struct {
 	// The first & the last IP address for dynamic leases
 	// Bytes [0..2] of the last allowed IP address must match the first IP
 	RangeStart net.IP `yaml:"range_start" json:"range_start"`
-	RangeEnd   net.IP `yaml:"range_end" json:"range_end"`
+	RangeEnd   net.IP `yaml:"range_end"   json:"range_end"`
 
 	LeaseDuration uint32 `yaml:"lease_duration" json:"lease_duration"` // in seconds
 
@@ -106,7 +110,81 @@ type V4ServerConf struct {
 	notify func(uint32)
 }
 
-// TODO(e.burkov):  !! validation
+// errNilConfig is an error returned by validation method if the config is nil.
+const errNilConfig errors.Error = "nil config"
+
+// TODO(e.burkov):  !! doc
+func tryTo4(ip net.IP) (ip4 net.IP, err error) {
+	if ip == nil {
+		return nil, fmt.Errorf("%v is not an IP address", ip)
+	}
+
+	ip4 = ip.To4()
+	if ip4 == nil {
+		return nil, fmt.Errorf("%v is not an IPv4 address", ip)
+	}
+
+	return ip4, nil
+}
+
+// Validate returns an error if c is not a valid configuration.
+func (c *V4ServerConf) Validate() (err error) {
+	defer func() { err = errors.Annotate(err, "dhcpv4: %w") }()
+
+	if c == nil {
+		return errNilConfig
+	}
+
+	var gatewayIP net.IP
+	gatewayIP, err = tryTo4(c.GatewayIP)
+	if err != nil {
+		// Don't wrap an errors since it's inforative enough as is and there is
+		// an annotation deferred already.
+		return err
+	}
+
+	if c.SubnetMask == nil {
+		return fmt.Errorf("invalid subnet mask: %v", c.SubnetMask)
+	}
+
+	subnetMask := net.IPMask(netutil.CloneIP(c.SubnetMask.To4()))
+	c.subnet = &net.IPNet{
+		IP:   gatewayIP,
+		Mask: subnetMask,
+	}
+	c.broadcastIP = aghnet.BroadcastFromIPNet(c.subnet)
+
+	c.ipRange, err = newIPRange(c.RangeStart, c.RangeEnd)
+	if err != nil {
+		// Don't wrap an errors since it's inforative enough as is and there is
+		// an annotation deferred already.
+		return err
+	}
+
+	if c.ipRange.contains(gatewayIP) {
+		return fmt.Errorf("gateway ip %v in the ip range: %v-%v",
+			gatewayIP,
+			c.RangeStart,
+			c.RangeEnd,
+		)
+	}
+
+	if !c.subnet.Contains(c.RangeStart) {
+		return fmt.Errorf("range start %v is outside network %v",
+			c.RangeStart,
+			c.subnet,
+		)
+	}
+
+	if !c.subnet.Contains(c.RangeEnd) {
+		return fmt.Errorf("range end %v is outside network %v",
+			c.RangeEnd,
+			c.subnet,
+		)
+	}
+
+	return nil
+}
 
 // V6ServerConf - server configuration
 type V6ServerConf struct {

internal/dhcpd/dhcpd.go

@@ -182,7 +182,7 @@ type Interface interface {
 }
 
 // Create - create object
-func Create(conf *ServerConfig) (s *server, err error) {
+func Create(conf ServerConfig) (s *server, err error) {
 	s = &server{
 		conf: &ServerConfig{
 			ConfigModified: conf.ConfigModified,
@@ -201,14 +201,11 @@ func Create(conf *ServerConfig) (s *server, err error) {
 	s.registerHandlers()
 
 	v4conf := conf.Conf4
-	v4conf.Enabled = s.conf.Enabled
-	if len(v4conf.RangeStart) == 0 {
-		v4conf.Enabled = false
-	}
-
 	v4conf.InterfaceName = s.conf.InterfaceName
 	v4conf.notify = s.onNotify
-	s.srv4, err = v4Create(v4conf)
+	conf.Enabled = s.conf.Enabled && len(v4conf.RangeStart) != 0
+
+	s.srv4, err = v4Create(&v4conf)
 	if err != nil {
 		return nil, fmt.Errorf("creating dhcpv4 srv: %w", err)
 	}
@@ -298,6 +295,7 @@ func (s *server) WriteDiskConfig(c *ServerConfig) {
 	c.Enabled = s.conf.Enabled
 	c.InterfaceName = s.conf.InterfaceName
 	c.LocalDomainName = s.conf.LocalDomainName
+
 	s.srv4.WriteDiskConfig4(&c.Conf4)
 	s.srv6.WriteDiskConfig6(&c.Conf6)
 }

internal/dhcpd/dhcpd_test.go

@@ -32,7 +32,7 @@ func TestDB(t *testing.T) {
 		},
 	}
 
-	s.srv4, err = v4Create(V4ServerConf{
+	s.srv4, err = v4Create(&V4ServerConf{
 		Enabled:    true,
 		RangeStart: net.IP{192, 168, 10, 100},
 		RangeEnd:   net.IP{192, 168, 10, 200},
@@ -88,32 +88,6 @@ func TestDB(t *testing.T) {
 	assert.Equal(t, leases[0].Expiry.Unix(), ll[1].Expiry.Unix())
 }
 
-func TestIsValidSubnetMask(t *testing.T) {
-	testCases := []struct {
-		mask net.IP
-		want bool
-	}{{
-		mask: net.IP{255, 255, 255, 0},
-		want: true,
-	}, {
-		mask: net.IP{255, 255, 254, 0},
-		want: true,
-	}, {
-		mask: net.IP{255, 255, 252, 0},
-		want: true,
-	}, {
-		mask: net.IP{255, 255, 253, 0},
-	}, {
-		mask: net.IP{255, 255, 255, 1},
-	}}
-
-	for _, tc := range testCases {
-		t.Run(tc.mask.String(), func(t *testing.T) {
-			assert.Equal(t, tc.want, isValidSubnetMask(tc.mask))
-		})
-	}
-}
-
 func TestNormalizeLeases(t *testing.T) {
 	dynLeases := []*Lease{{
 		HWAddr: net.HardwareAddr{1, 2, 3, 4},
@@ -174,7 +148,7 @@ func TestV4Server_badRange(t *testing.T) {
 				notify:     testNotify,
 			}
 
-			_, err := v4Create(conf)
+			_, err := v4Create(&conf)
 			testutil.AssertErrorMsg(t, tc.wantErrMsg, err)
 		})
 	}

internal/dhcpd/http_others.go

@@ -26,12 +26,12 @@ type v4ServerConfJSON struct {
 	LeaseDuration uint32 `json:"lease_duration"`
 }
 
-func v4JSONToServerConf(j *v4ServerConfJSON) V4ServerConf {
+func (j *v4ServerConfJSON) toServerConf() *V4ServerConf {
 	if j == nil {
-		return V4ServerConf{}
+		return &V4ServerConf{}
 	}
 
-	return V4ServerConf{
+	return &V4ServerConf{
 		GatewayIP:     j.GatewayIP,
 		SubnetMask:    j.SubnetMask,
 		RangeStart:    j.RangeStart,
@@ -99,23 +99,21 @@ func (s *server) enableDHCP(ifaceName string) (code int, err error) {
 	hasStaticIP, err = aghnet.IfaceHasStaticIP(ifaceName)
 	if err != nil {
 		if errors.Is(err, os.ErrPermission) {
-			// ErrPermission may happen here on Linux systems where
-			// AdGuard Home is installed using Snap.  That doesn't
-			// necessarily mean that the machine doesn't have
-			// a static IP, so we can assume that it has and go on.
-			// If the machine doesn't, we'll get an error later.
+			// ErrPermission may happen here on Linux systems where AdGuard Home
+			// is installed using Snap.  That doesn't necessarily mean that the
+			// machine doesn't have a static IP, so we can assume that it has
+			// and go on.  If the machine doesn't, we'll get an error later.
 			//
 			// See https://github.com/AdguardTeam/AdGuardHome/issues/2667.
 			//
-			// TODO(a.garipov): I was thinking about moving this
-			// into IfaceHasStaticIP, but then we wouldn't be able
-			// to log it.  Think about it more.
+			// TODO(a.garipov): I was thinking about moving this into
+			// IfaceHasStaticIP, but then we wouldn't be able to log it.  Think
+			// about it more.
 			log.Info("error while checking static ip: %s; "+
 				"assuming machine has static ip and going on", err)
 			hasStaticIP = true
 		} else if errors.Is(err, aghnet.ErrNoStaticIPInfo) {
-			// Couldn't obtain a definitive answer.  Assume static
-			// IP an go on.
+			// Couldn't obtain a definitive answer.  Assume static IP an go on.
 			log.Info("can't check for static ip; " +
 				"assuming machine has static ip and going on")
 			hasStaticIP = true
@@ -157,7 +155,7 @@ func (s *server) handleDHCPSetConfigV4(
 		return nil, false, nil
 	}
 
-	v4Conf := v4JSONToServerConf(conf.V4)
+	v4Conf := conf.V4.toServerConf()
 	v4Conf.Enabled = conf.Enabled == aghalg.NBTrue
 	if len(v4Conf.RangeStart) == 0 {
 		v4Conf.Enabled = false
@@ -166,8 +164,8 @@ func (s *server) handleDHCPSetConfigV4(
 	enabled = v4Conf.Enabled
 	v4Conf.InterfaceName = conf.InterfaceName
 
-	c4 := V4ServerConf{}
-	s.srv4.WriteDiskConfig4(&c4)
+	c4 := &V4ServerConf{}
+	s.srv4.WriteDiskConfig4(c4)
 	v4Conf.notify = c4.notify
 	v4Conf.ICMPTimeout = c4.ICMPTimeout
 	v4Conf.Options = c4.Options
@@ -576,7 +574,7 @@ func (s *server) handleReset(w http.ResponseWriter, r *http.Request) {
 		DBFilePath: s.conf.DBFilePath,
 	}
 
-	v4conf := V4ServerConf{
+	v4conf := &V4ServerConf{
 		LeaseDuration: DefaultDHCPLeaseTTL,
 		ICMPTimeout:   DefaultDHCPTimeoutICMP,
 		notify:        s.onNotify,

internal/dhcpd/options_unix.go

@@ -197,10 +197,10 @@ func parseDHCPOption(s string) (code dhcpv4.OptionCode, val dhcpv4.OptionValue,
 
 // prepareOptions builds the set of DHCP options according to host requirements
 // document and values from conf.
-func prepareOptions(conf V4ServerConf) (implicit, explicit dhcpv4.Options) {
+func (s *v4Server) prepareOptions() {
 	// Set default values of host configuration parameters listed in Appendix A
 	// of RFC-2131.
-	implicit = dhcpv4.OptionsFromList(
+	s.implicitOpts = dhcpv4.OptionsFromList(
 		// IP-Layer Per Host
 
 		// An Internet host that includes embedded gateway code MUST have a
@@ -376,32 +376,30 @@ func prepareOptions(conf V4ServerConf) (implicit, explicit dhcpv4.Options) {
 
 		// Set the Router Option to working subnet's IP since it's initialized
 		// with the address of the gateway.
-		dhcpv4.OptRouter(conf.subnet.IP),
+		dhcpv4.OptRouter(s.conf.subnet.IP),
 
-		dhcpv4.OptSubnetMask(conf.subnet.Mask),
+		dhcpv4.OptSubnetMask(s.conf.subnet.Mask),
 	)
 
 	// Set values for explicitly configured options.
-	explicit = dhcpv4.Options{}
-	for i, o := range conf.Options {
+	s.explicitOpts = dhcpv4.Options{}
+	for i, o := range s.conf.Options {
 		code, val, err := parseDHCPOption(o)
 		if err != nil {
 			log.Error("dhcpv4: bad option string at index %d: %s", i, err)
 
 			continue
 		}
 
-		explicit.Update(dhcpv4.Option{Code: code, Value: val})
+		s.explicitOpts.Update(dhcpv4.Option{Code: code, Value: val})
 		// Remove those from the implicit options.
-		delete(implicit, code.Code())
+		delete(s.implicitOpts, code.Code())
 	}
 
-	log.Debug("dhcpv4: implicit options:\n%s", implicit.Summary(nil))
-	log.Debug("dhcpv4: explicit options:\n%s", explicit.Summary(nil))
+	log.Debug("dhcpv4: implicit options:\n%s", s.implicitOpts.Summary(nil))
+	log.Debug("dhcpv4: explicit options:\n%s", s.explicitOpts.Summary(nil))
 
-	if len(explicit) == 0 {
-		explicit = nil
+	if len(s.explicitOpts) == 0 {
+		s.explicitOpts = nil
 	}
-
-	return implicit, explicit
 }

internal/dhcpd/options_unix_test.go

@@ -250,17 +250,21 @@ func TestPrepareOptions(t *testing.T) {
 	}}
 
 	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			implicit, explicit := prepareOptions(V4ServerConf{
+		s := &v4Server{
+			conf: V4ServerConf{
 				// Just to avoid nil pointer dereference.
 				subnet:  &net.IPNet{},
 				Options: tc.opts,
-			})
+			},
+		}
+
+		t.Run(tc.name, func(t *testing.T) {
+			s.prepareOptions()
 
-			assert.Equal(t, tc.wantExplicit, explicit)
+			assert.Equal(t, tc.wantExplicit, s.explicitOpts)
 
-			for c := range explicit {
-				assert.NotContains(t, implicit, c)
+			for c := range s.explicitOpts {
+				assert.NotContains(t, s.implicitOpts, c)
 			}
 		})
 	}