Pull request: home: mv local domain name to dhcp setts

Closes #3367.

Squashed commit of the following:

commit e2cabcac2d91af24b9e5f4ac8f78daf5e8d339b9
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Jan 25 19:35:31 2022 +0300

    home: imp test, skip dhcp test on windows

commit e58053f11e081630ad4e8d1e77a7a74226029db0
Merge: ff2fe87d 0b72bcc5
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Jan 25 19:10:25 2022 +0300

    Merge branch 'master' into 3367-dhcp-local-domain-name

commit ff2fe87d8cab12e60d045be636e366e392d6d96f
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu Dec 30 20:55:34 2021 +0300

    home: mv local domain name to dhcp setts

CHANGELOG.md

@@ -21,9 +21,33 @@ and this project adheres to
 
 ### Changed
 
+- The setting `local_domain_name` is now in the `dhcp` block in the
+  configuration file to avoid confusion ([#3367]).
 - The `dns.bogus_nxdomain` configuration file parameter now supports CIDR
   notation alongside IP addresses ([#1730]).
 
+#### Configuration Changes
+
+In this release, the schema version has changed from 12 to 13.
+
+- Parameter `local_domain_name`, which in schema versions 12 and earlier used to
+  be a part of the `dns` object, is now a part of the `dhcp` object:
+
+  ```yaml
+  # BEFORE:
+  'dns':
+    # …
+    'local_domain_name': 'lan'
+
+  # AFTER:
+  'dhcp':
+    # …
+    'local_domain_name': 'lan'
+  ```
+
+  To rollback this change, move the parameter back into `dns` and change the
+  `schema_version` back to `12`.
+
 ### Deprecated
 
 <!--
@@ -38,6 +62,7 @@ and this project adheres to
 
 [#1730]: https://github.com/AdguardTeam/AdGuardHome/issues/1730
 [#3057]: https://github.com/AdguardTeam/AdGuardHome/issues/3057
+[#3367]: https://github.com/AdguardTeam/AdGuardHome/issues/3367
 
 
 

internal/aghnet/dhcp_unix.go

@@ -111,7 +111,7 @@ func discover4(iface *net.Interface, dstAddr *net.UDPAddr, hostname string) (ok
 	// is spoiled.
 	//
 	// It's also known that listening on the specified interface's address
-	// ignores broadcasted packets when reading.
+	// ignores broadcast packets when reading.
 	var c net.PacketConn
 	if c, err = listenPacketReusable(iface.Name, "udp4", ":68"); err != nil {
 		return false, fmt.Errorf("couldn't listen on :68: %w", err)

internal/dhcpd/dhcpd.go

@@ -119,23 +119,28 @@ func (l *Lease) UnmarshalJSON(data []byte) (err error) {
 	return nil
 }
 
-// ServerConfig - DHCP server configuration
-// field ordering is important -- yaml fields will mirror ordering from here
+// ServerConfig is the configuration for the DHCP server.  The order of YAML
+// fields is important, since the YAML configuration file follows it.
 type ServerConfig struct {
-	Enabled       bool   `yaml:"enabled"`
-	InterfaceName string `yaml:"interface_name"`
-
-	Conf4 V4ServerConf `yaml:"dhcpv4"`
-	Conf6 V6ServerConf `yaml:"dhcpv6"`
-
-	WorkDir    string `yaml:"-"`
-	DBFilePath string `yaml:"-"` // path to DB file
-
 	// Called when the configuration is changed by HTTP request
 	ConfigModified func() `yaml:"-"`
 
 	// Register an HTTP handler
 	HTTPRegister func(string, string, func(http.ResponseWriter, *http.Request)) `yaml:"-"`
+
+	Enabled       bool   `yaml:"enabled"`
+	InterfaceName string `yaml:"interface_name"`
+
+	// LocalDomainName is the domain name used for DHCP hosts.  For example,
+	// a DHCP client with the hostname "myhost" can be addressed as "myhost.lan"
+	// when LocalDomainName is "lan".
+	LocalDomainName string `yaml:"local_domain_name"`
+
+	Conf4 V4ServerConf `yaml:"dhcpv4"`
+	Conf6 V6ServerConf `yaml:"dhcpv6"`
+
+	WorkDir    string `yaml:"-"`
+	DBFilePath string `yaml:"-"`
 }
 
 // OnLeaseChangedT is a callback for lease changes.
@@ -156,7 +161,9 @@ type Server struct {
 	srv4 DHCPServer
 	srv6 DHCPServer
 
-	conf ServerConfig
+	// TODO(a.garipov): Either create a separate type for the internal config or
+	// just put the config values into Server.
+	conf *ServerConfig
 
 	// Called when the leases DB is modified
 	onLeaseChanged []OnLeaseChangedT
@@ -181,14 +188,21 @@ type ServerInterface interface {
 }
 
 // Create - create object
-func Create(conf ServerConfig) (s *Server, err error) {
-	s = &Server{}
+func Create(conf *ServerConfig) (s *Server, err error) {
+	s = &Server{
+		conf: &ServerConfig{
+			ConfigModified: conf.ConfigModified,
 
-	s.conf.Enabled = conf.Enabled
-	s.conf.InterfaceName = conf.InterfaceName
-	s.conf.HTTPRegister = conf.HTTPRegister
-	s.conf.ConfigModified = conf.ConfigModified
-	s.conf.DBFilePath = filepath.Join(conf.WorkDir, dbFilename)
+			HTTPRegister: conf.HTTPRegister,
+
+			Enabled:       conf.Enabled,
+			InterfaceName: conf.InterfaceName,
+
+			LocalDomainName: conf.LocalDomainName,
+
+			DBFilePath: filepath.Join(conf.WorkDir, dbFilename),
+		},
+	}
 
 	if !webHandlersRegistered && s.conf.HTTPRegister != nil {
 		if runtime.GOOS == "windows" {
@@ -305,6 +319,7 @@ func (s *Server) notify(flags int) {
 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

@@ -27,7 +27,7 @@ func testNotify(flags uint32) {
 func TestDB(t *testing.T) {
 	var err error
 	s := Server{
-		conf: ServerConfig{
+		conf: &ServerConfig{
 			DBFilePath: dbFilename,
 		},
 	}
@@ -140,27 +140,27 @@ func TestNormalizeLeases(t *testing.T) {
 func TestV4Server_badRange(t *testing.T) {
 	testCases := []struct {
 		name       string
+		wantErrMsg string
 		gatewayIP  net.IP
 		subnetMask net.IP
-		wantErrMsg string
 	}{{
-		name:       "gateway_in_range",
-		gatewayIP:  net.IP{192, 168, 10, 120},
-		subnetMask: net.IP{255, 255, 255, 0},
+		name: "gateway_in_range",
 		wantErrMsg: "dhcpv4: gateway ip 192.168.10.120 in the ip range: " +
 			"192.168.10.20-192.168.10.200",
+		gatewayIP:  net.IP{192, 168, 10, 120},
+		subnetMask: net.IP{255, 255, 255, 0},
 	}, {
-		name:       "outside_range_start",
-		gatewayIP:  net.IP{192, 168, 10, 1},
-		subnetMask: net.IP{255, 255, 255, 240},
+		name: "outside_range_start",
 		wantErrMsg: "dhcpv4: range start 192.168.10.20 is outside network " +
 			"192.168.10.1/28",
-	}, {
-		name:       "outside_range_end",
 		gatewayIP:  net.IP{192, 168, 10, 1},
-		subnetMask: net.IP{255, 255, 255, 224},
+		subnetMask: net.IP{255, 255, 255, 240},
+	}, {
+		name: "outside_range_end",
 		wantErrMsg: "dhcpv4: range end 192.168.10.200 is outside network " +
 			"192.168.10.1/27",
+		gatewayIP:  net.IP{192, 168, 10, 1},
+		subnetMask: net.IP{255, 255, 255, 224},
 	}}
 
 	for _, tc := range testCases {

internal/dhcpd/http.go

@@ -575,12 +575,15 @@ func (s *Server) handleReset(w http.ResponseWriter, r *http.Request) {
 		log.Error("dhcp: removing db: %s", err)
 	}
 
-	oldconf := s.conf
-	s.conf = ServerConfig{
-		WorkDir:        oldconf.WorkDir,
-		HTTPRegister:   oldconf.HTTPRegister,
-		ConfigModified: oldconf.ConfigModified,
-		DBFilePath:     oldconf.DBFilePath,
+	s.conf = &ServerConfig{
+		ConfigModified: s.conf.ConfigModified,
+
+		HTTPRegister: s.conf.HTTPRegister,
+
+		LocalDomainName: s.conf.LocalDomainName,
+
+		WorkDir:    s.conf.WorkDir,
+		DBFilePath: s.conf.DBFilePath,
 	}
 
 	v4conf := V4ServerConf{

internal/dhcpd/nullbool_test.go

@@ -12,33 +12,33 @@ import (
 func TestNullBool_UnmarshalJSON(t *testing.T) {
 	testCases := []struct {
 		name       string
-		data       []byte
 		wantErrMsg string
+		data       []byte
 		want       nullBool
 	}{{
 		name:       "empty",
-		data:       []byte{},
 		wantErrMsg: "",
+		data:       []byte{},
 		want:       nbNull,
 	}, {
 		name:       "null",
-		data:       []byte("null"),
 		wantErrMsg: "",
+		data:       []byte("null"),
 		want:       nbNull,
 	}, {
 		name:       "true",
-		data:       []byte("true"),
 		wantErrMsg: "",
+		data:       []byte("true"),
 		want:       nbTrue,
 	}, {
 		name:       "false",
-		data:       []byte("false"),
 		wantErrMsg: "",
+		data:       []byte("false"),
 		want:       nbFalse,
 	}, {
 		name:       "invalid",
-		data:       []byte("flase"),
-		wantErrMsg: `invalid nullBool value "flase"`,
+		wantErrMsg: `invalid nullBool value "invalid"`,
+		data:       []byte("invalid"),
 		want:       nbNull,
 	}}
 

internal/dhcpd/v4.go

@@ -969,11 +969,10 @@ func (s *v4Server) send(peer net.Addr, conn net.PacketConn, req, resp *dhcpv4.DH
 			Port: dhcpv4.ServerPort,
 		}
 		if mtype == dhcpv4.MessageTypeNak {
-			// Set the broadcast bit in the DHCPNAK, so that the
-			// relay agent broadcasted it to the client, because the
-			// client may not have a correct network address or
-			// subnet mask, and the client may not be answering ARP
-			// requests.
+			// Set the broadcast bit in the DHCPNAK, so that the relay agent
+			// broadcasts it to the client, because the client may not have
+			// a correct network address or subnet mask, and the client may not
+			// be answering ARP requests.
 			resp.SetBroadcast()
 		}
 	case mtype == dhcpv4.MessageTypeNak:

internal/dnsforward/dns.go

@@ -215,9 +215,8 @@ func (s *Server) onDHCPLeaseChanged(flags int) {
 		ipToHost = netutil.NewIPMap(len(ll))
 
 		for _, l := range ll {
-			// TODO(a.garipov): Remove this after we're finished
-			// with the client hostname validations in the DHCP
-			// server code.
+			// TODO(a.garipov): Remove this after we're finished with the client
+			// hostname validations in the DHCP server code.
 			err = netutil.ValidateDomainName(l.Hostname)
 			if err != nil {
 				log.Debug(
@@ -301,6 +300,8 @@ func (s *Server) processInternalHosts(dctx *dnsContext) (rc resultCode) {
 	}
 
 	reqHost := strings.ToLower(q.Name)
+	// TODO(a.garipov): Move everything related to DHCP local domain to the DHCP
+	// server.
 	host := strings.TrimSuffix(reqHost, s.localDomainSuffix)
 	if host == reqHost {
 		return resultCodeSuccess

internal/home/clients_test.go

@@ -3,10 +3,12 @@ package home
 import (
 	"net"
 	"os"
+	"runtime"
 	"testing"
 	"time"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/dhcpd"
+	"github.com/AdguardTeam/golibs/testutil"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -271,12 +273,18 @@ func TestClientsAddExisting(t *testing.T) {
 	})
 
 	t.Run("complicated", func(t *testing.T) {
+		// TODO(a.garipov): Properly decouple the DHCP server from the client
+		// storage.
+		if runtime.GOOS == "windows" {
+			t.Skip("skipping dhcp test on windows")
+		}
+
 		var err error
 
 		ip := net.IP{1, 2, 3, 4}
 
 		// First, init a DHCP server with a single static lease.
-		config := dhcpd.ServerConfig{
+		config := &dhcpd.ServerConfig{
 			Enabled:    true,
 			DBFilePath: "leases.db",
 			Conf4: dhcpd.V4ServerConf{
@@ -290,10 +298,9 @@ func TestClientsAddExisting(t *testing.T) {
 
 		clients.dhcpServer, err = dhcpd.Create(config)
 		require.NoError(t, err)
-		// TODO(e.burkov):  leases.db isn't created on Windows so removing it
-		// causes an error.  Split the test to make it run properly on different
-		// operating systems.
-		t.Cleanup(func() { _ = os.Remove("leases.db") })
+		testutil.CleanupAndRequireSuccess(t, func() (err error) {
+			return os.Remove("leases.db")
+		})
 
 		err = clients.dhcpServer.AddStaticLease(&dhcpd.Lease{
 			HWAddr:   net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA},

internal/home/config.go

@@ -83,7 +83,7 @@ type configuration struct {
 	WhitelistFilters []filter `yaml:"whitelist_filters"`
 	UserRules        []string `yaml:"user_rules"`
 
-	DHCP dhcpd.ServerConfig `yaml:"dhcp"`
+	DHCP *dhcpd.ServerConfig `yaml:"dhcp"`
 
 	// Clients contains the YAML representations of the persistent clients.
 	// This field is only used for reading and writing persistent client data.
@@ -123,11 +123,6 @@ type dnsConfig struct {
 	// UpstreamTimeout is the timeout for querying upstream servers.
 	UpstreamTimeout timeutil.Duration `yaml:"upstream_timeout"`
 
-	// LocalDomainName is the domain name used for known internal hosts.
-	// For example, a machine called "myhost" can be addressed as
-	// "myhost.lan" when LocalDomainName is "lan".
-	LocalDomainName string `yaml:"local_domain_name"`
-
 	// ResolveClients enables and disables resolving clients with RDNS.
 	ResolveClients bool `yaml:"resolve_clients"`
 
@@ -199,7 +194,6 @@ var config = &configuration{
 		FilteringEnabled:           true, // whether or not use filter lists
 		FiltersUpdateIntervalHours: 24,
 		UpstreamTimeout:            timeutil.Duration{Duration: dnsforward.DefaultTimeout},
-		LocalDomainName:            "lan",
 		ResolveClients:             true,
 		UsePrivateRDNS:             true,
 	},
@@ -208,6 +202,9 @@ var config = &configuration{
 		PortDNSOverTLS:  defaultPortTLS, // needs to be passed through to dnsproxy
 		PortDNSOverQUIC: defaultPortQUIC,
 	},
+	DHCP: &dhcpd.ServerConfig{
+		LocalDomainName: "lan",
+	},
 	logSettings: logSettings{
 		LogCompress:   false,
 		LogLocalTime:  false,
@@ -389,8 +386,8 @@ func (c *configuration) write() error {
 	}
 
 	if Context.dhcpServer != nil {
-		c := dhcpd.ServerConfig{}
-		Context.dhcpServer.WriteDiskConfig(&c)
+		c := &dhcpd.ServerConfig{}
+		Context.dhcpServer.WriteDiskConfig(c)
 		config.DHCP = c
 	}
 

internal/home/dns.go

@@ -83,7 +83,7 @@ func initDNSServer() (err error) {
 		QueryLog:       Context.queryLog,
 		SubnetDetector: Context.subnetDetector,
 		Anonymizer:     anonymizer,
-		LocalDomain:    config.DNS.LocalDomainName,
+		LocalDomain:    config.DHCP.LocalDomainName,
 	}
 	if Context.dhcpServer != nil {
 		p.DHCPServer = Context.dhcpServer

internal/home/upgrade.go

@@ -21,7 +21,7 @@ import (
 )
 
 // currentSchemaVersion is the current schema version.
-const currentSchemaVersion = 12
+const currentSchemaVersion = 13
 
 // These aliases are provided for convenience.
 type (
@@ -85,6 +85,7 @@ func upgradeConfigSchema(oldVersion int, diskConf yobj) (err error) {
 		upgradeSchema9to10,
 		upgradeSchema10to11,
 		upgradeSchema11to12,
+		upgradeSchema12to13,
 	}
 
 	n := 0
@@ -690,6 +691,52 @@ func upgradeSchema11to12(diskConf yobj) (err error) {
 	return nil
 }
 
+// upgradeSchema12to13 performs the following changes:
+//
+//   # BEFORE:
+//   'dns':
+//     # …
+//     'local_domain_name': 'lan'
+//
+//   # AFTER:
+//   'dhcp':
+//     # …
+//     'local_domain_name': 'lan'
+//
+func upgradeSchema12to13(diskConf yobj) (err error) {
+	log.Printf("Upgrade yaml: 12 to 13")
+	diskConf["schema_version"] = 13
+
+	dnsVal, ok := diskConf["dns"]
+	if !ok {
+		return nil
+	}
+
+	var dns yobj
+	dns, ok = dnsVal.(yobj)
+	if !ok {
+		return fmt.Errorf("unexpected type of dns: %T", dnsVal)
+	}
+
+	dhcpVal, ok := diskConf["dhcp"]
+	if !ok {
+		return nil
+	}
+
+	var dhcp yobj
+	dhcp, ok = dhcpVal.(yobj)
+	if !ok {
+		return fmt.Errorf("unexpected type of dhcp: %T", dnsVal)
+	}
+
+	const field = "local_domain_name"
+
+	dhcp[field] = dns[field]
+	delete(dns, field)
+
+	return nil
+}
+
 // TODO(a.garipov): Replace with log.Output when we port it to our logging
 // package.
 func funcName() string {

internal/home/upgrade_test.go

@@ -55,7 +55,7 @@ func TestUpgradeSchema2to3(t *testing.T) {
 		require.Len(t, v, 1)
 		require.Equal(t, "8.8.8.8:53", v[0])
 	default:
-		t.Fatalf("wrong type for bootsrap dns: %T", v)
+		t.Fatalf("wrong type for bootstrap dns: %T", v)
 	}
 
 	excludedEntries := []string{"bootstrap_dns"}
@@ -511,3 +511,48 @@ func TestUpgradeSchema11to12(t *testing.T) {
 		assert.Equal(t, 90*24*time.Hour, ivlVal.Duration)
 	})
 }
+
+func TestUpgradeSchema12to13(t *testing.T) {
+	t.Run("no_dns", func(t *testing.T) {
+		conf := yobj{}
+
+		err := upgradeSchema12to13(conf)
+		require.NoError(t, err)
+
+		assert.Equal(t, conf["schema_version"], 13)
+	})
+
+	t.Run("no_dhcp", func(t *testing.T) {
+		conf := yobj{
+			"dns": yobj{},
+		}
+
+		err := upgradeSchema12to13(conf)
+		require.NoError(t, err)
+
+		assert.Equal(t, conf["schema_version"], 13)
+	})
+
+	t.Run("good", func(t *testing.T) {
+		conf := yobj{
+			"dns": yobj{
+				"local_domain_name": "lan",
+			},
+			"dhcp":           yobj{},
+			"schema_version": 12,
+		}
+
+		wantConf := yobj{
+			"dns": yobj{},
+			"dhcp": yobj{
+				"local_domain_name": "lan",
+			},
+			"schema_version": 13,
+		}
+
+		err := upgradeSchema12to13(conf)
+		require.NoError(t, err)
+
+		assert.Equal(t, wantConf, conf)
+	})
+}