Pull request: dhcpd: do not override ra-slaac settings

Merge in DNS/adguard-home from 2653-ra-slaac to master

Updates #2653.

Squashed commit of the following:

commit f261413a58dc813e37cc848606ed490b8c0ac9f3
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu Feb 11 20:37:13 2021 +0300

    all: doc changes, rm debug

commit 4a8c6e4897579493c1ca242fb8f0f440c3b51a74
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu Feb 11 20:11:46 2021 +0300

    dhcpd: do not override ra-slaac settings

CHANGELOG.md

@@ -26,13 +26,15 @@ and this project adheres to
 
 ### Fixed
 
+- DHCPv6 server's `ra_slaac_only` and `ra_allow_slaac` settings aren't reset to
+  `false` on update any more ([#2653]).
 - The `Vary` header is now added along with `Access-Control-Allow-Origin` to
   prevent cache-related and other issues in browsers ([#2658]).
-  domain, but with an HTTP scheme as opposed to `*` ([#2484]).
 - The request body size limit is now set for HTTPS requests as well.
 - Incorrect version tag in the Docker release ([#2663]).
 - DNSCrypt queries weren't marked as such in logs ([#2662]).
 
+[#2653]: https://github.com/AdguardTeam/AdGuardHome/issues/2653
 [#2658]: https://github.com/AdguardTeam/AdGuardHome/issues/2658
 [#2662]: https://github.com/AdguardTeam/AdGuardHome/issues/2662
 [#2663]: https://github.com/AdguardTeam/AdGuardHome/issues/2663

internal/dhcpd/dhcpd.go

@@ -117,14 +117,14 @@ type ServerInterface interface {
 }
 
 // Create - create object
-func Create(config ServerConfig) *Server {
+func Create(conf ServerConfig) *Server {
 	s := &Server{}
 
-	s.conf.Enabled = config.Enabled
+	s.conf.Enabled = conf.Enabled
-	s.conf.InterfaceName = config.InterfaceName
+	s.conf.InterfaceName = conf.InterfaceName
-	s.conf.HTTPRegister = config.HTTPRegister
+	s.conf.HTTPRegister = conf.HTTPRegister
-	s.conf.ConfigModified = config.ConfigModified
+	s.conf.ConfigModified = conf.ConfigModified
-	s.conf.DBFilePath = filepath.Join(config.WorkDir, dbFilename)
+	s.conf.DBFilePath = filepath.Join(conf.WorkDir, dbFilename)
 
 	if !webHandlersRegistered && s.conf.HTTPRegister != nil {
 		if runtime.GOOS == "windows" {
@@ -145,7 +145,7 @@ func Create(config ServerConfig) *Server {
 	}
 
 	var err4, err6 error
-	v4conf := config.Conf4
+	v4conf := conf.Conf4
 	v4conf.Enabled = s.conf.Enabled
 	if len(v4conf.RangeStart) == 0 {
 		v4conf.Enabled = false
@@ -154,7 +154,7 @@ func Create(config ServerConfig) *Server {
 	v4conf.notify = s.onNotify
 	s.srv4, err4 = v4Create(v4conf)
 
-	v6conf := config.Conf6
+	v6conf := conf.Conf6
 	v6conf.Enabled = s.conf.Enabled
 	if len(v6conf.RangeStart) == 0 {
 		v6conf.Enabled = false
@@ -172,6 +172,9 @@ func Create(config ServerConfig) *Server {
 		return nil
 	}
 
+	s.conf.Conf4 = conf.Conf4
+	s.conf.Conf6 = conf.Conf6
+
 	if s.conf.Enabled && !v4conf.Enabled && !v6conf.Enabled {
 		log.Error("Can't enable DHCP server because neither DHCPv4 nor DHCPv6 servers are configured")
 		return nil

internal/dhcpd/dhcphttp.go

@@ -11,7 +11,6 @@ import (
 
 	"github.com/AdguardTeam/AdGuardHome/internal/sysutil"
 	"github.com/AdguardTeam/AdGuardHome/internal/util"
-	"github.com/AdguardTeam/golibs/jsonutil"
 	"github.com/AdguardTeam/golibs/log"
 )
 
@@ -29,7 +28,11 @@ type v4ServerConfJSON struct {
 	LeaseDuration uint32 `json:"lease_duration"`
 }
 
-func v4JSONToServerConf(j v4ServerConfJSON) V4ServerConf {
+func v4JSONToServerConf(j *v4ServerConfJSON) V4ServerConf {
+	if j == nil {
+		return V4ServerConf{}
+	}
+
 	return V4ServerConf{
 		GatewayIP:     j.GatewayIP,
 		SubnetMask:    j.SubnetMask,
@@ -44,7 +47,11 @@ type v6ServerConfJSON struct {
 	LeaseDuration uint32 `json:"lease_duration"`
 }
 
-func v6JSONToServerConf(j v6ServerConfJSON) V6ServerConf {
+func v6JSONToServerConf(j *v6ServerConfJSON) V6ServerConf {
+	if j == nil {
+		return V6ServerConf{}
+	}
+
 	return V6ServerConf{
 		RangeStart:    j.RangeStart,
 		LeaseDuration: j.LeaseDuration,
@@ -83,13 +90,6 @@ func (s *Server) handleDHCPStatus(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-type dhcpServerConfigJSON struct {
-	Enabled       bool             `json:"enabled"`
-	InterfaceName string           `json:"interface_name"`
-	V4            v4ServerConfJSON `json:"v4"`
-	V6            v6ServerConfJSON `json:"v6"`
-}
-
 func (s *Server) enableDHCP(ifaceName string) (code int, err error) {
 	var hasStaticIP bool
 	hasStaticIP, err = sysutil.IfaceHasStaticIP(ifaceName)
@@ -112,14 +112,22 @@ func (s *Server) enableDHCP(ifaceName string) (code int, err error) {
 	return 0, nil
 }
 
-func (s *Server) handleDHCPSetConfig(w http.ResponseWriter, r *http.Request) {
+type dhcpServerConfigJSON struct {
-	newconfig := dhcpServerConfigJSON{}
+	V4            *v4ServerConfJSON `json:"v4"`
-	newconfig.Enabled = s.conf.Enabled
+	V6            *v6ServerConfJSON `json:"v6"`
-	newconfig.InterfaceName = s.conf.InterfaceName
+	InterfaceName string            `json:"interface_name"`
+	Enabled       nullBool          `json:"enabled"`
+}
 
-	js, err := jsonutil.DecodeObject(&newconfig, r.Body)
+func (s *Server) handleDHCPSetConfig(w http.ResponseWriter, r *http.Request) {
+	conf := dhcpServerConfigJSON{}
+	conf.Enabled = boolToNullBool(s.conf.Enabled)
+	conf.InterfaceName = s.conf.InterfaceName
+
+	err := json.NewDecoder(r.Body).Decode(&conf)
 	if err != nil {
-		httpError(r, w, http.StatusBadRequest, "Failed to parse new DHCP config json: %s", err)
+		httpError(r, w, http.StatusBadRequest,
+			"failed to parse new dhcp config json: %s", err)
 
 		return
 	}
@@ -129,62 +137,72 @@ func (s *Server) handleDHCPSetConfig(w http.ResponseWriter, r *http.Request) {
 	v4Enabled := false
 	v6Enabled := false
 
-	if js.Exists("v4") {
+	if conf.V4 != nil {
-		v4conf := v4JSONToServerConf(newconfig.V4)
+		v4Conf := v4JSONToServerConf(conf.V4)
-		v4conf.Enabled = newconfig.Enabled
+		v4Conf.Enabled = conf.Enabled == nbTrue
-		if len(v4conf.RangeStart) == 0 {
+		if len(v4Conf.RangeStart) == 0 {
-			v4conf.Enabled = false
+			v4Conf.Enabled = false
 		}
 
-		v4Enabled = v4conf.Enabled
+		v4Enabled = v4Conf.Enabled
-		v4conf.InterfaceName = newconfig.InterfaceName
+		v4Conf.InterfaceName = conf.InterfaceName
 
 		c4 := V4ServerConf{}
 		s.srv4.WriteDiskConfig4(&c4)
-		v4conf.notify = c4.notify
+		v4Conf.notify = c4.notify
-		v4conf.ICMPTimeout = c4.ICMPTimeout
+		v4Conf.ICMPTimeout = c4.ICMPTimeout
 
-		s4, err = v4Create(v4conf)
+		s4, err = v4Create(v4Conf)
 		if err != nil {
-			httpError(r, w, http.StatusBadRequest, "invalid dhcpv4 configuration: %s", err)
+			httpError(r, w, http.StatusBadRequest,
+				"invalid dhcpv4 configuration: %s", err)
 
 			return
 		}
 	}
 
-	if js.Exists("v6") {
+	if conf.V6 != nil {
-		v6conf := v6JSONToServerConf(newconfig.V6)
+		v6Conf := v6JSONToServerConf(conf.V6)
-		v6conf.Enabled = newconfig.Enabled
+		v6Conf.Enabled = conf.Enabled == nbTrue
-		if len(v6conf.RangeStart) == 0 {
+		if len(v6Conf.RangeStart) == 0 {
-			v6conf.Enabled = false
+			v6Conf.Enabled = false
 		}
 
-		v6Enabled = v6conf.Enabled
+		// Don't overwrite the RA/SLAAC settings from the config file.
-		v6conf.InterfaceName = newconfig.InterfaceName
+		//
-		v6conf.notify = s.onNotify
+		// TODO(a.garipov): Perhaps include them into the request to
+		// allow changing them from the HTTP API?
+		v6Conf.RASLAACOnly = s.conf.Conf6.RASLAACOnly
+		v6Conf.RAAllowSLAAC = s.conf.Conf6.RAAllowSLAAC
 
-		s6, err = v6Create(v6conf)
+		v6Enabled = v6Conf.Enabled
+		v6Conf.InterfaceName = conf.InterfaceName
+		v6Conf.notify = s.onNotify
+
+		s6, err = v6Create(v6Conf)
 		if err != nil {
-			httpError(r, w, http.StatusBadRequest, "invalid dhcpv6 configuration: %s", err)
+			httpError(r, w, http.StatusBadRequest,
+				"invalid dhcpv6 configuration: %s", err)
 
 			return
 		}
 	}
 
-	if newconfig.Enabled && !v4Enabled && !v6Enabled {
+	if conf.Enabled == nbTrue && !v4Enabled && !v6Enabled {
-		httpError(r, w, http.StatusBadRequest, "dhcpv4 or dhcpv6 configuration must be complete")
+		httpError(r, w, http.StatusBadRequest,
+			"dhcpv4 or dhcpv6 configuration must be complete")
 
 		return
 	}
 
 	s.Stop()
 
-	if js.Exists("enabled") {
+	if conf.Enabled != nbNull {
-		s.conf.Enabled = newconfig.Enabled
+		s.conf.Enabled = conf.Enabled == nbTrue
 	}
 
-	if js.Exists("interface_name") {
+	if conf.InterfaceName != "" {
-		s.conf.InterfaceName = newconfig.InterfaceName
+		s.conf.InterfaceName = conf.InterfaceName
 	}
 
 	if s4 != nil {
@@ -200,7 +218,7 @@ func (s *Server) handleDHCPSetConfig(w http.ResponseWriter, r *http.Request) {
 
 	if s.conf.Enabled {
 		var code int
-		code, err = s.enableDHCP(newconfig.InterfaceName)
+		code, err = s.enableDHCP(conf.InterfaceName)
 		if err != nil {
 			httpError(r, w, code, "enabling dhcp: %s", err)
 

internal/dhcpd/nullbool.go

@@ -0,0 +1,58 @@
+package dhcpd
+
+import (
+	"bytes"
+	"fmt"
+)
+
+// nullBool is a nullable boolean.  Use these in JSON requests and responses
+// instead of pointers to bool.
+//
+// TODO(a.garipov): Inspect uses of *bool, move this type into some new package
+// if we need it somewhere else.
+type nullBool uint8
+
+// nullBool values
+const (
+	nbNull nullBool = iota
+	nbTrue
+	nbFalse
+)
+
+// String implements the fmt.Stringer interface for nullBool.
+func (nb nullBool) String() (s string) {
+	switch nb {
+	case nbNull:
+		return "null"
+	case nbTrue:
+		return "true"
+	case nbFalse:
+		return "false"
+	}
+
+	return fmt.Sprintf("!invalid nullBool %d", uint8(nb))
+}
+
+// boolToNullBool converts a bool into a nullBool.
+func boolToNullBool(cond bool) (nb nullBool) {
+	if cond {
+		return nbTrue
+	}
+
+	return nbFalse
+}
+
+// UnmarshalJSON implements the json.Unmarshaler interface for *nullBool.
+func (nb *nullBool) UnmarshalJSON(b []byte) (err error) {
+	if len(b) == 0 || bytes.Equal(b, []byte("null")) {
+		*nb = nbNull
+	} else if bytes.Equal(b, []byte("true")) {
+		*nb = nbTrue
+	} else if bytes.Equal(b, []byte("false")) {
+		*nb = nbFalse
+	} else {
+		return fmt.Errorf("invalid nullBool value %q", b)
+	}
+
+	return nil
+}

internal/dhcpd/nullbool_test.go

@@ -0,0 +1,69 @@
+package dhcpd
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestNullBool_UnmarshalText(t *testing.T) {
+	testCases := []struct {
+		name       string
+		data       []byte
+		wantErrMsg string
+		want       nullBool
+	}{{
+		name:       "empty",
+		data:       []byte{},
+		wantErrMsg: "",
+		want:       nbNull,
+	}, {
+		name:       "null",
+		data:       []byte("null"),
+		wantErrMsg: "",
+		want:       nbNull,
+	}, {
+		name:       "true",
+		data:       []byte("true"),
+		wantErrMsg: "",
+		want:       nbTrue,
+	}, {
+		name:       "false",
+		data:       []byte("false"),
+		wantErrMsg: "",
+		want:       nbFalse,
+	}, {
+		name:       "invalid",
+		data:       []byte("flase"),
+		wantErrMsg: `invalid nullBool value "flase"`,
+		want:       nbNull,
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			var got nullBool
+			err := got.UnmarshalJSON(tc.data)
+			if tc.wantErrMsg == "" {
+				assert.Nil(t, err)
+			} else {
+				require.NotNil(t, err)
+				assert.Equal(t, tc.wantErrMsg, err.Error())
+			}
+
+			assert.Equal(t, tc.want, got)
+		})
+	}
+
+	t.Run("json", func(t *testing.T) {
+		want := nbTrue
+		var got struct {
+			A nullBool
+		}
+
+		err := json.Unmarshal([]byte(`{"A":true}`), &got)
+		require.Nil(t, err)
+		assert.Equal(t, want, got.A)
+	})
+}

internal/dhcpd/routeradv.go

@@ -13,8 +13,8 @@ import (
 )
 
 type raCtx struct {
-	raAllowSlaac     bool   // send RA packets without MO flags
+	raAllowSLAAC     bool   // send RA packets without MO flags
-	raSlaacOnly      bool   // send RA packets with MO flags
+	raSLAACOnly      bool   // send RA packets with MO flags
 	ipAddr           net.IP // source IP address (link-local-unicast)
 	dnsIPAddr        net.IP // IP address for DNS Server option
 	prefixIPAddr     net.IP // IP address for Prefix option
@@ -159,7 +159,7 @@ func createICMPv6RAPacket(params icmpv6RA) []byte {
 func (ra *raCtx) Init() error {
 	ra.stop.Store(0)
 	ra.conn = nil
-	if !(ra.raAllowSlaac || ra.raSlaacOnly) {
+	if !(ra.raAllowSLAAC || ra.raSLAACOnly) {
 		return nil
 	}
 
@@ -167,8 +167,8 @@ func (ra *raCtx) Init() error {
 		ra.ipAddr, ra.dnsIPAddr)
 
 	params := icmpv6RA{
-		managedAddressConfiguration: !ra.raSlaacOnly,
+		managedAddressConfiguration: !ra.raSLAACOnly,
-		otherConfiguration:          !ra.raSlaacOnly,
+		otherConfiguration:          !ra.raSLAACOnly,
 		mtu:                         uint32(ra.iface.MTU),
 		prefixLen:                   64,
 		recursiveDNSServer:          ra.dnsIPAddr,

internal/dhcpd/server.go

@@ -83,8 +83,8 @@ type V6ServerConf struct {
 
 	LeaseDuration uint32 `yaml:"lease_duration" json:"lease_duration"` // in seconds
 
-	RaSlaacOnly  bool `yaml:"ra_slaac_only" json:"-"`  // send ICMPv6.RA packets without MO flags
+	RASLAACOnly  bool `yaml:"ra_slaac_only" json:"-"`  // send ICMPv6.RA packets without MO flags
-	RaAllowSlaac bool `yaml:"ra_allow_slaac" json:"-"` // send ICMPv6.RA packets with MO flags
+	RAAllowSLAAC bool `yaml:"ra_allow_slaac" json:"-"` // send ICMPv6.RA packets with MO flags
 
 	ipStart    net.IP        // starting IP address for dynamic leases
 	leaseTime  time.Duration // the time during which a dynamic lease is considered valid

internal/dhcpd/v6.go

@@ -552,8 +552,8 @@ func (s *v6Server) initRA(iface *net.Interface) error {
 		}
 	}
 
-	s.ra.raAllowSlaac = s.conf.RaAllowSlaac
+	s.ra.raAllowSLAAC = s.conf.RAAllowSLAAC
-	s.ra.raSlaacOnly = s.conf.RaSlaacOnly
+	s.ra.raSLAACOnly = s.conf.RASLAACOnly
 	s.ra.dnsIPAddr = s.ra.ipAddr
 	s.ra.prefixIPAddr = s.conf.ipStart
 	s.ra.ifaceName = s.conf.InterfaceName
@@ -594,7 +594,7 @@ func (s *v6Server) Start() error {
 	}
 
 	// don't initialize DHCPv6 server if we must force the clients to use SLAAC
-	if s.conf.RaSlaacOnly {
+	if s.conf.RASLAACOnly {
 		log.Debug("DHCPv6: not starting DHCPv6 server due to ra_slaac_only=true")
 		return nil
 	}

internal/sysutil/net_linux.go

@@ -33,15 +33,19 @@ func ifaceHasStaticIP(ifaceName string) (has bool, err error) {
 		filePath: "/etc/network/interfaces",
 	}} {
 		f, err = os.Open(check.filePath)
-		if errors.Is(err, os.ErrNotExist) {
-			continue
-		}
 		if err != nil {
+			if errors.Is(err, os.ErrNotExist) {
+				err = nil
+
+				continue
+			}
+
 			return false, err
 		}
 		defer f.Close()
 
-		fileReadCloser, err := aghio.LimitReadCloser(f, maxConfigFileSize)
+		var fileReadCloser io.ReadCloser
+		fileReadCloser, err = aghio.LimitReadCloser(f, maxConfigFileSize)
 		if err != nil {
 			return false, err
 		}

staticcheck.conf

@@ -10,7 +10,9 @@ initialisms = [
 , "MX"
 , "PTR"
 , "QUIC"
+, "RA"
 , "SDNS"
+, "SLAAC"
 , "SVCB"
 ]
 dot_import_whitelist = []