Pull request: 3744 DNS server DHCP option

Merge in DNS/adguard-home from 3744-options-priority to master

Updates #3744.

Squashed commit of the following:

commit 30f1d483bebd92348250573d2edd708247081b45
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Oct 18 15:22:49 2021 +0300

    dhcpd: imp tests more

commit 9a8194e2f259ac7a88b23a1480c74decfef587b3
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Oct 18 15:09:20 2021 +0300

    dhcpd: imp tests

commit d915e0b407adcfd24df6e28be22f095909749aa3
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Oct 18 14:46:20 2021 +0300

    dhcpd: fix options priority

CHANGELOG.md

@@ -114,6 +114,7 @@ In this release, the schema version has changed from 10 to 12.
 
 ### Fixed
 
+- Incorrect assignment of explicitly configured DHCP options ([#3744]).
 - Occasional panic during shutdown ([#3655]).
 - Addition of IPs into only one as opposed to all matching ipsets on Linux
   ([#3638]).
@@ -204,6 +205,7 @@ In this release, the schema version has changed from 10 to 12.
 [#3607]: https://github.com/AdguardTeam/AdGuardHome/issues/3607
 [#3638]: https://github.com/AdguardTeam/AdGuardHome/issues/3638
 [#3655]: https://github.com/AdguardTeam/AdGuardHome/issues/3655
+[#3744]: https://github.com/AdguardTeam/AdGuardHome/issues/3744
 
 
 

internal/dhcpd/v4.go

@@ -900,9 +900,10 @@ func (s *v4Server) process(req, resp *dhcpv4.DHCPv4) int {
 			resp.UpdateOption(dhcpv4.OptGeneric(code, configured.Get(code)))
 		}
 	}
-	// Update the value of Domain Name Server option separately from others
+	// Update the value of Domain Name Server option separately from others if
-	// since its value is set after server's creating.
+	// not assigned yet since its value is set after server's creating.
-	if requested.Has(dhcpv4.OptionDomainNameServer) {
+	if requested.Has(dhcpv4.OptionDomainNameServer) &&
+		!resp.Options.Has(dhcpv4.OptionDomainNameServer) {
 		resp.UpdateOption(dhcpv4.OptDNS(s.conf.dnsIPAddrs...))
 	}
 

internal/dhcpd/v4_test.go

@@ -5,8 +5,10 @@ package dhcpd
 
 import (
 	"net"
+	"strings"
 	"testing"
 
+	"github.com/AdguardTeam/golibs/stringutil"
 	"github.com/insomniacslk/dhcp/dhcpv4"
 	"github.com/mdlayher/raw"
 	"github.com/stretchr/testify/assert"
@@ -16,17 +18,34 @@ import (
 func notify4(flags uint32) {
 }
 
-func TestV4_AddRemove_static(t *testing.T) {
+// defaultV4ServerConf returns the default configuration for *v4Server to use in
-	s, err := v4Create(V4ServerConf{
+// tests.
+func defaultV4ServerConf() (conf V4ServerConf) {
+	return V4ServerConf{
 		Enabled:    true,
 		RangeStart: net.IP{192, 168, 10, 100},
 		RangeEnd:   net.IP{192, 168, 10, 200},
 		GatewayIP:  net.IP{192, 168, 10, 1},
 		SubnetMask: net.IP{255, 255, 255, 0},
 		notify:     notify4,
-	})
+	}
+}
+
+// defaultSrv prepares the default DHCPServer to use in tests.  The underlying
+// type of s is *v4Server.
+func defaultSrv(t *testing.T) (s DHCPServer) {
+	t.Helper()
+
+	var err error
+	s, err = v4Create(defaultV4ServerConf())
 	require.NoError(t, err)
 
+	return s
+}
+
+func TestV4_AddRemove_static(t *testing.T) {
+	s := defaultSrv(t)
+
 	ls := s.GetLeases(LeasesStatic)
 	assert.Empty(t, ls)
 
@@ -37,7 +56,7 @@ func TestV4_AddRemove_static(t *testing.T) {
 		IP:       net.IP{192, 168, 10, 150},
 	}
 
-	err = s.AddStaticLease(l)
+	err := s.AddStaticLease(l)
 	require.NoError(t, err)
 
 	err = s.AddStaticLease(l)
@@ -65,15 +84,7 @@ func TestV4_AddRemove_static(t *testing.T) {
 }
 
 func TestV4_AddReplace(t *testing.T) {
-	sIface, err := v4Create(V4ServerConf{
+	sIface := defaultSrv(t)
-		Enabled:    true,
-		RangeStart: net.IP{192, 168, 10, 100},
-		RangeEnd:   net.IP{192, 168, 10, 200},
-		GatewayIP:  net.IP{192, 168, 10, 1},
-		SubnetMask: net.IP{255, 255, 255, 0},
-		notify:     notify4,
-	})
-	require.NoError(t, err)
 
 	s, ok := sIface.(*v4Server)
 	require.True(t, ok)
@@ -89,7 +100,7 @@ func TestV4_AddReplace(t *testing.T) {
 	}}
 
 	for i := range dynLeases {
-		err = s.addLease(&dynLeases[i])
+		err := s.addLease(&dynLeases[i])
 		require.NoError(t, err)
 	}
 
@@ -104,7 +115,7 @@ func TestV4_AddReplace(t *testing.T) {
 	}}
 
 	for _, l := range stLeases {
-		err = s.AddStaticLease(l)
+		err := s.AddStaticLease(l)
 		require.NoError(t, err)
 	}
 
@@ -118,18 +129,81 @@ func TestV4_AddReplace(t *testing.T) {
 	}
 }
 
-func TestV4StaticLease_Get(t *testing.T) {
+func TestV4Server_Process_optionsPriority(t *testing.T) {
-	var err error
+	defaultIP := net.IP{192, 168, 1, 1}
-	sIface, err := v4Create(V4ServerConf{
+	knownIP := net.IP{1, 2, 3, 4}
-		Enabled:    true,
+
-		RangeStart: net.IP{192, 168, 10, 100},
+	// prepareSrv creates a *v4Server and sets the opt6IPs in the initial
-		RangeEnd:   net.IP{192, 168, 10, 200},
+	// configuration of the server as the value for DHCP option 6.
-		GatewayIP:  net.IP{192, 168, 10, 1},
+	prepareSrv := func(t *testing.T, opt6IPs []net.IP) (s *v4Server) {
-		SubnetMask: net.IP{255, 255, 255, 0},
+		t.Helper()
-		notify:     notify4,
+
-	})
+		conf := defaultV4ServerConf()
+		if len(opt6IPs) > 0 {
+			b := &strings.Builder{}
+			stringutil.WriteToBuilder(b, "6 ips ", opt6IPs[0].String())
+			for _, ip := range opt6IPs[1:] {
+				stringutil.WriteToBuilder(b, ",", ip.String())
+			}
+			conf.Options = []string{b.String()}
+		}
+
+		ss, 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
+	}
+
+	// checkResp creates a discovery message with DHCP option 6 requested amd
+	// asserts the response to contain wantIPs in this option.
+	checkResp := func(t *testing.T, s *v4Server, wantIPs []net.IP) {
+		t.Helper()
+
+		mac := net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA}
+		req, err := dhcpv4.NewDiscovery(mac, dhcpv4.WithRequestedOptions(
+			dhcpv4.OptionDomainNameServer,
+		))
+		require.NoError(t, err)
+
+		var resp *dhcpv4.DHCPv4
+		resp, err = dhcpv4.NewReplyFromRequest(req)
+		require.NoError(t, err)
+
+		res := s.process(req, resp)
+		require.Equal(t, 1, res)
+
+		o := resp.GetOneOption(dhcpv4.OptionDomainNameServer)
+		require.NotEmpty(t, o)
+
+		wantData := []byte{}
+		for _, ip := range wantIPs {
+			wantData = append(wantData, ip...)
+		}
+		assert.Equal(t, o, wantData)
+	}
+
+	t.Run("default", func(t *testing.T) {
+		s := prepareSrv(t, nil)
+
+		checkResp(t, s, []net.IP{defaultIP})
+	})
+
+	t.Run("explicitly_configured", func(t *testing.T) {
+		s := prepareSrv(t, []net.IP{knownIP, knownIP})
+
+		checkResp(t, s, []net.IP{knownIP, knownIP})
+	})
+}
+
+func TestV4StaticLease_Get(t *testing.T) {
+	sIface := defaultSrv(t)
+
 	s, ok := sIface.(*v4Server)
 	require.True(t, ok)
 
@@ -140,7 +214,7 @@ func TestV4StaticLease_Get(t *testing.T) {
 		HWAddr:   net.HardwareAddr{0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA},
 		IP:       net.IP{192, 168, 10, 150},
 	}
-	err = s.AddStaticLease(l)
+	err := s.AddStaticLease(l)
 	require.NoError(t, err)
 
 	var req, resp *dhcpv4.DHCPv4
@@ -208,19 +282,14 @@ func TestV4StaticLease_Get(t *testing.T) {
 }
 
 func TestV4DynamicLease_Get(t *testing.T) {
-	var err error
+	conf := defaultV4ServerConf()
-	sIface, err := v4Create(V4ServerConf{
+	conf.Options = []string{
-		Enabled:    true,
-		RangeStart: net.IP{192, 168, 10, 100},
-		RangeEnd:   net.IP{192, 168, 10, 200},
-		GatewayIP:  net.IP{192, 168, 10, 1},
-		SubnetMask: net.IP{255, 255, 255, 0},
-		notify:     notify4,
-		Options: []string{
 		"81 hex 303132",
 		"82 ip 1.2.3.4",
-		},
+	}
-	})
+
+	var err error
+	sIface, err := v4Create(conf)
 	require.NoError(t, err)
 
 	s, ok := sIface.(*v4Server)