dhcpd: let v4 be unconfigured

internal/dhcpd/config.go

@@ -113,7 +113,8 @@ type V4ServerConf struct {
 // errNilConfig is an error returned by validation method if the config is nil.
 const errNilConfig errors.Error = "nil config"
 
-// TODO(e.burkov):  !! doc
+// tryTo4 returns a 4-byte version of ip.  An error is returned if the passed ip
+// is not an IPv4.
 func tryTo4(ip net.IP) (ip4 net.IP, err error) {
 	if ip == nil {
 		return nil, fmt.Errorf("%v is not an IP address", ip)
@@ -128,6 +129,9 @@ func tryTo4(ip net.IP) (ip4 net.IP, err error) {
 }
 
 // Validate returns an error if c is not a valid configuration.
+//
+// TODO(e.burkov):  Don't set the config fields when the server itself will stop
+// containing the config.
 func (c *V4ServerConf) Validate() (err error) {
 	defer func() { err = errors.Annotate(err, "dhcpv4: %w") }()
 
@@ -207,5 +211,3 @@ type V6ServerConf struct {
 	// Server calls this function when leases data changes
 	notify func(uint32)
 }
-
-// TODO(e.burkov):  !! validation

internal/dhcpd/dhcpd.go

@@ -203,11 +203,15 @@ func Create(conf ServerConfig) (s *server, err error) {
 	v4conf := conf.Conf4
 	v4conf.InterfaceName = s.conf.InterfaceName
 	v4conf.notify = s.onNotify
-	conf.Enabled = s.conf.Enabled && len(v4conf.RangeStart) != 0
+	v4conf.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)
+		if v4conf.Enabled {
+			return nil, fmt.Errorf("creating dhcpv4 srv: %w", err)
+		}
+
+		log.Error("creating dhcpv4 srv: %s", err)
 	}
 
 	v6conf := conf.Conf6
@@ -281,10 +285,6 @@ func (s *server) SetOnLeaseChanged(onLeaseChanged OnLeaseChangedT) {
 }
 
 func (s *server) notify(flags int) {
-	if len(s.onLeaseChanged) == 0 {
-		return
-	}
-
 	for _, f := range s.onLeaseChanged {
 		f(flags)
 	}

internal/dhcpd/http_others.go

@@ -150,7 +150,7 @@ type dhcpServerConfigJSON struct {
 
 func (s *server) handleDHCPSetConfigV4(
 	conf *dhcpServerConfigJSON,
-) (srv4 DHCPServer, enabled bool, err error) {
+) (srv DHCPServer, enabled bool, err error) {
 	if conf.V4 == nil {
 		return nil, false, nil
 	}
@@ -161,18 +161,23 @@ func (s *server) handleDHCPSetConfigV4(
 		v4Conf.Enabled = false
 	}
 
-	enabled = v4Conf.Enabled
 	v4Conf.InterfaceName = conf.InterfaceName
 
-	c4 := &V4ServerConf{}
+	// Set the default values for the fields not configurable via web API.
+	c4 := &V4ServerConf{
+		notify:      s.onNotify,
+		ICMPTimeout: s.conf.Conf4.ICMPTimeout,
+		Options:     s.conf.Conf4.Options,
+	}
+
 	s.srv4.WriteDiskConfig4(c4)
 	v4Conf.notify = c4.notify
 	v4Conf.ICMPTimeout = c4.ICMPTimeout
 	v4Conf.Options = c4.Options
 
-	srv4, err = v4Create(v4Conf)
+	srv4, err := v4Create(v4Conf)
 
-	return srv4, enabled, err
+	return srv4, srv4.enabled(), err
 }
 
 func (s *server) handleDHCPSetConfigV6(

internal/dhcpd/options_unix_test.go

@@ -251,7 +251,7 @@ func TestPrepareOptions(t *testing.T) {
 
 	for _, tc := range testCases {
 		s := &v4Server{
-			conf: V4ServerConf{
+			conf: &V4ServerConf{
 				// Just to avoid nil pointer dereference.
 				subnet:  &net.IPNet{},
 				Options: tc.opts,

internal/dhcpd/v4.go

@@ -30,7 +30,7 @@ import (
 //
 // TODO(a.garipov): Think about unifying this and v6Server.
 type v4Server struct {
-	conf V4ServerConf
+	conf *V4ServerConf
 
 	srv *server4.Server
 
@@ -56,9 +56,15 @@ type v4Server struct {
 	leases []*Lease
 }
 
+func (s *v4Server) enabled() (ok bool) {
+	return s.conf != nil && s.conf.Enabled
+}
+
 // WriteDiskConfig4 - write configuration
 func (s *v4Server) WriteDiskConfig4(c *V4ServerConf) {
-	*c = s.conf
+	if s.conf != nil {
+		*c = *s.conf
+	}
 }
 
 // WriteDiskConfig6 - write configuration
@@ -115,8 +121,8 @@ func (s *v4Server) validHostnameForClient(cliHostname string, ip net.IP) (hostna
 func (s *v4Server) ResetLeases(leases []*Lease) (err error) {
 	defer func() { err = errors.Annotate(err, "dhcpv4: %w") }()
 
-	if !s.conf.Enabled {
-		return
+	if s.conf == nil {
+		return nil
 	}
 
 	s.leasedOffsets = newBitSet()
@@ -130,12 +136,7 @@ func (s *v4Server) ResetLeases(leases []*Lease) (err error) {
 		err = s.addLease(l)
 		if err != nil {
 			// TODO(a.garipov): Wrap and bubble up the error.
-			log.Error(
-				"dhcpv4: reset: re-adding a lease for %s (%s): %s",
-				l.IP,
-				l.HWAddr,
-				err,
-			)
+			log.Error("dhcpv4: reset: re-adding a lease for %s (%s): %s", l.IP, l.HWAddr, err)
 
 			continue
 		}
@@ -337,11 +338,19 @@ func (s *v4Server) rmLease(lease *Lease) (err error) {
 	return errors.Error("lease not found")
 }
 
+// ErrUnconfigured is returned from the server's method when it requires the
+// server to be configured and it's not.
+const ErrUnconfigured errors.Error = "server is unconfigured"
+
 // AddStaticLease implements the DHCPServer interface for *v4Server.  It is safe
 // for concurrent use.
 func (s *v4Server) AddStaticLease(l *Lease) (err error) {
 	defer func() { err = errors.Annotate(err, "dhcpv4: adding static lease: %w") }()
 
+	if s.conf == nil {
+		return ErrUnconfigured
+	}
+
 	ip := l.IP.To4()
 	if ip == nil {
 		return fmt.Errorf("invalid ip %q, only ipv4 is supported", l.IP)
@@ -415,6 +424,10 @@ func (s *v4Server) AddStaticLease(l *Lease) (err error) {
 func (s *v4Server) RemoveStaticLease(l *Lease) (err error) {
 	defer func() { err = errors.Annotate(err, "dhcpv4: %w") }()
 
+	if s.conf == nil {
+		return ErrUnconfigured
+	}
+
 	if len(l.IP) != 4 {
 		return fmt.Errorf("invalid IP")
 	}
@@ -1141,7 +1154,7 @@ func (s *v4Server) send(peer net.Addr, conn net.PacketConn, req, resp *dhcpv4.DH
 func (s *v4Server) Start() (err error) {
 	defer func() { err = errors.Annotate(err, "dhcpv4: %w") }()
 
-	if !s.conf.Enabled {
+	if !s.enabled() {
 		return nil
 	}
 
@@ -1233,69 +1246,20 @@ func (s *v4Server) Stop() (err error) {
 }
 
 // Create DHCPv4 server
-func v4Create(conf *V4ServerConf) (srv DHCPServer, err error) {
+func v4Create(conf *V4ServerConf) (srv *v4Server, err error) {
 	s := &v4Server{
-		conf:       *conf,
 		leaseHosts: stringutil.NewSet(),
 	}
 
-	// TODO(a.garipov): Don't use a disabled server in other places or just use
-	// an interface.
-	if !conf.Enabled {
-		return s, nil
-	}
-
-	defer func() { err = errors.Annotate(err, "dhcpv4: %w") }()
-
-	var gatewayIP net.IP
-	gatewayIP, err = tryTo4(s.conf.GatewayIP)
-	if err != nil {
-		// Don't wrap an errors since it's inforative enough as is and there is
-		// an annotation deferred already.
-		return s, err
-	}
-
-	if s.conf.SubnetMask == nil {
-		return s, fmt.Errorf("invalid subnet mask: %v", s.conf.SubnetMask)
-	}
-
-	subnetMask := make([]byte, 4)
-	copy(subnetMask, s.conf.SubnetMask.To4())
-
-	s.conf.subnet = &net.IPNet{
-		IP:   gatewayIP,
-		Mask: subnetMask,
-	}
-	s.conf.broadcastIP = aghnet.BroadcastFromIPNet(s.conf.subnet)
-
-	s.conf.ipRange, err = newIPRange(conf.RangeStart, conf.RangeEnd)
+	err = conf.Validate()
 	if err != nil {
-		// Don't wrap an errors since it's inforative enough as is and there is
-		// an annotation deferred already.
+		// TODO(a.garipov): Don't use a disabled server in other places or just
+		// use an interface.
 		return s, err
 	}
 
-	if s.conf.ipRange.contains(gatewayIP) {
-		return s, fmt.Errorf("gateway ip %v in the ip range: %v-%v",
-			gatewayIP,
-			conf.RangeStart,
-			conf.RangeEnd,
-		)
-	}
-
-	if !s.conf.subnet.Contains(conf.RangeStart) {
-		return s, fmt.Errorf("range start %v is outside network %v",
-			conf.RangeStart,
-			s.conf.subnet,
-		)
-	}
-
-	if !s.conf.subnet.Contains(conf.RangeEnd) {
-		return s, fmt.Errorf("range end %v is outside network %v",
-			conf.RangeEnd,
-			s.conf.subnet,
-		)
-	}
+	s.conf = &V4ServerConf{}
+	*s.conf = *conf
 
 	// TODO(a.garipov, d.seregin): Check that every lease is inside the IPRange.
 	s.leasedOffsets = newBitSet()

internal/dhcpd/v4_test.go

@@ -30,9 +30,6 @@ var (
 	DefaultSubnetMask = net.IP{255, 255, 255, 0}
 )
 
-func notify4(flags uint32) {
-}
-
 // defaultV4ServerConf returns the default configuration for *v4Server to use in
 // tests.
 func defaultV4ServerConf() (conf *V4ServerConf) {
@@ -42,7 +39,7 @@ func defaultV4ServerConf() (conf *V4ServerConf) {
 		RangeEnd:   DefaultRangeEnd,
 		GatewayIP:  DefaultGatewayIP,
 		SubnetMask: DefaultSubnetMask,
-		notify:     notify4,
+		notify:     testNotify,
 		dnsIPAddrs: []net.IP{DefaultSelfIP},
 	}
 }
@@ -350,13 +347,10 @@ func TestV4Server_handle_optionsPriority(t *testing.T) {
 			defer func() { s.implicitOpts.Update(dhcpv4.OptDNS(defaultIP)) }()
 		}
 
-		ss, err := v4Create(conf)
+		var err error
+		s, err = v4Create(conf)
 		require.NoError(t, err)
 
-		var ok bool
-		s, ok = ss.(*v4Server)
-		require.True(t, ok)
-
 		s.conf.dnsIPAddrs = []net.IP{defaultIP}
 
 		return s
@@ -490,10 +484,9 @@ func TestV4Server_updateOptions(t *testing.T) {
 		require.NoError(t, err)
 
 		require.IsType(t, (*v4Server)(nil), s)
-		s4, _ := s.(*v4Server)
 
 		t.Run(tc.name, func(t *testing.T) {
-			s4.updateOptions(req, resp)
+			s.updateOptions(req, resp)
 
 			for c, v := range tc.wantOpts {
 				if v == nil {
@@ -596,13 +589,9 @@ func TestV4DynamicLease_Get(t *testing.T) {
 		"82 ip 1.2.3.4",
 	}
 
-	var err error
-	sIface, err := v4Create(conf)
+	s, err := v4Create(conf)
 	require.NoError(t, err)
 
-	s, ok := sIface.(*v4Server)
-	require.True(t, ok)
-
 	s.conf.dnsIPAddrs = []net.IP{{192, 168, 10, 1}}
 	s.implicitOpts.Update(dhcpv4.OptDNS(s.conf.dnsIPAddrs...))
 

internal/dhcpd/v6.go

@@ -173,7 +173,7 @@ func (s *v6Server) rmDynamicLease(lease *Lease) (err error) {
 func (s *v6Server) AddStaticLease(l *Lease) (err error) {
 	defer func() { err = errors.Annotate(err, "dhcpv6: %w") }()
 
-	if len(l.IP) != 16 {
+	if len(l.IP) != net.IPv6len {
 		return fmt.Errorf("invalid IP")
 	}
 

internal/home/config.go

@@ -433,9 +433,7 @@ func (c *configuration) write() (err error) {
 	}
 
 	if Context.dhcpServer != nil {
-		c := &dhcpd.ServerConfig{}
-		Context.dhcpServer.WriteDiskConfig(c)
-		config.DHCP = *c
+		Context.dhcpServer.WriteDiskConfig(&config.DHCP)
 	}
 
 	config.Clients.Persistent = Context.clients.forConfig()