net/dns, ipn/ipnlocal: fix regressions from change moving away from deephash

I got sidetracked apparently and never finished writing this Clone
code in 316afe7d02babc (#17448). (It really should use views instead.)

And then I missed one of the users of "routerChanged" that was broken up
into "routerChanged" vs "dnsChanged".

This broke integration tests elsewhere.

Fixes #17506

Change-Id: I533bf0fcf3da9ac6eb4a6cdef03b8df2c1fb4c8e
Signed-off-by: Brad Fitzpatrick <[email protected]>

net/dns/config.go

@@ -7,6 +7,7 @@ package dns
 import (
 	"bufio"
 	"fmt"
+	"maps"
 	"net/netip"
 	"reflect"
 	"slices"
@@ -190,15 +191,21 @@ func sameResolverNames(a, b []*dnstype.Resolver) bool {
 	return true
 }
 
+// Clone makes a shallow clone of c.
+//
+// The returned Config still references slices and maps from c.
+//
+// TODO(bradfitz): use cmd/{viewer,cloner} for these and make the
+// caller use views instead.
 func (c *Config) Clone() *Config {
 	if c == nil {
 		return nil
 	}
 	return &Config{
 		DefaultResolvers: slices.Clone(c.DefaultResolvers),
-		Routes:           make(map[dnsname.FQDN][]*dnstype.Resolver, len(c.Routes)),
+		Routes:           maps.Clone(c.Routes),
 		SearchDomains:    slices.Clone(c.SearchDomains),
-		Hosts:            make(map[dnsname.FQDN][]netip.Addr, len(c.Hosts)),
+		Hosts:            maps.Clone(c.Hosts),
 		OnlyIPv6:         c.OnlyIPv6,
 	}
 }

net/dns/config_test.go

@@ -0,0 +1,66 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+package dns
+
+import (
+	"net/netip"
+	"reflect"
+	"testing"
+
+	"tailscale.com/types/dnstype"
+	"tailscale.com/util/dnsname"
+)
+
+func TestConfigClone(t *testing.T) {
+	tests := []struct {
+		name string
+		conf *Config
+	}{
+		{
+			name: "nil",
+			conf: nil,
+		},
+		{
+			name: "empty",
+			conf: &Config{},
+		},
+		{
+			name: "full",
+			conf: &Config{
+				DefaultResolvers: []*dnstype.Resolver{
+					{
+						Addr:                "abc",
+						BootstrapResolution: []netip.Addr{netip.MustParseAddr("1.2.3.4")},
+						UseWithExitNode:     true,
+					},
+				},
+				Routes: map[dnsname.FQDN][]*dnstype.Resolver{
+					"foo.bar.": {
+						{
+							Addr:                "abc",
+							BootstrapResolution: []netip.Addr{netip.MustParseAddr("1.2.3.4")},
+							UseWithExitNode:     true,
+						},
+					},
+				},
+				SearchDomains: []dnsname.FQDN{"bar.baz."},
+				Hosts: map[dnsname.FQDN][]netip.Addr{
+					"host.bar.": {netip.MustParseAddr("5.6.7.8")},
+				},
+				OnlyIPv6: true,
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := tt.conf.Clone()
+			if !reflect.DeepEqual(got, tt.conf) {
+				t.Error("Cloned result is not reflect.DeepEqual")
+			}
+			if !got.Equal(tt.conf) {
+				t.Error("Cloned result is not Equal")
+			}
+		})
+	}
+}

util/checkchange/checkchange.go

@@ -17,7 +17,7 @@ type EqualCloner[T any] interface {
 //
 // It only modifies *old if they are different. old must be non-nil.
 func Update[T EqualCloner[T]](old *T, new T) (changed bool) {
-	if new.Equal(*old) {
+	if (*old).Equal(new) {
 		return false
 	}
 	*old = new.Clone()

wgengine/userspace.go

@@ -965,8 +965,9 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config,
 	isSubnetRouterChanged := isSubnetRouter != e.lastIsSubnetRouter
 
 	engineChanged := checkchange.Update(&e.lastEngineFull, cfg)
-	dnsChanged := checkchange.Update(&e.lastDNSConfig, dnsCfg)
+	dnsChanged := buildfeatures.HasDNS && checkchange.Update(&e.lastDNSConfig, dnsCfg)
 	routerChanged := checkchange.Update(&e.lastRouter, routerCfg)
+
 	listenPortChanged := listenPort != e.magicConn.LocalPort()
 	peerMTUChanged := peerMTUEnable != e.magicConn.PeerMTUEnabled()
 	if !engineChanged && !routerChanged && !dnsChanged && !listenPortChanged && !isSubnetRouterChanged && !peerMTUChanged {
@@ -987,7 +988,9 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config,
 	// instead have ipnlocal populate a map of DNS IP => linkName and
 	// put that in the *dns.Config instead, and plumb it down to the
 	// dns.Manager. Maybe also with isLocalAddr above.
-	e.isDNSIPOverTailscale.Store(ipset.NewContainsIPFunc(views.SliceOf(dnsIPsOverTailscale(dnsCfg, routerCfg))))
+	if buildfeatures.HasDNS {
+		e.isDNSIPOverTailscale.Store(ipset.NewContainsIPFunc(views.SliceOf(dnsIPsOverTailscale(dnsCfg, routerCfg))))
+	}
 
 	// See if any peers have changed disco keys, which means they've restarted.
 	// If so, we need to update the wireguard-go/device.Device in two phases:
@@ -1063,7 +1066,18 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config,
 		if err != nil {
 			return err
 		}
+	}
 
+	// We've historically re-set DNS even after just a router change. While
+	// refactoring in tailscale/tailscale#17448 and and
+	// tailscale/tailscale#17499, I'm erring on the side of keeping that
+	// historical quirk for now (2025-10-08), lest it's load bearing in
+	// unexpected ways
+	//
+	// TODO(bradfitz): try to do the "configuring DNS" part below only if
+	// dnsChanged, not routerChanged. The "resolver.ShouldUseRoutes" part
+	// probably needs to keep happening for both.
+	if buildfeatures.HasDNS && (routerChanged || dnsChanged) {
 		if resolver.ShouldUseRoutes(e.controlKnobs) {
 			e.logf("wgengine: Reconfig: user dialer")
 			e.dialer.SetRoutes(routerCfg.Routes, routerCfg.LocalRoutes)
@@ -1075,7 +1089,7 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config,
 		// DNS managers refuse to apply settings if the device has no
 		// assigned address.
 		e.logf("wgengine: Reconfig: configuring DNS")
-		err = e.dns.Set(*dnsCfg)
+		err := e.dns.Set(*dnsCfg)
 		e.health.SetDNSHealth(err)
 		if err != nil {
 			return err
@@ -1097,7 +1111,7 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config,
 		}
 	}
 
-	if isSubnetRouterChanged && e.birdClient != nil {
+	if buildfeatures.HasBird && isSubnetRouterChanged && e.birdClient != nil {
 		e.logf("wgengine: Reconfig: configuring BIRD")
 		var err error
 		if isSubnetRouter {