net/dns: don't set MatchDomains on Apple platforms when no upstream nameservers available (#12334)

This PR addresses a DNS issue on macOS as discussed this morning.

Signed-off-by: Andrea Gottardo <andrea@gottardo.me>

net/dns/manager.go

@@ -52,10 +52,11 @@ type Manager struct {
 
 	resolver *resolver.Resolver
 	os       OSConfigurator
+	goos     string // if empty, gets set to runtime.GOOS
 }
 
 // NewManagers created a new manager from the given config.
-func NewManager(logf logger.Logf, oscfg OSConfigurator, health *health.Tracker, dialer *tsdial.Dialer, linkSel resolver.ForwardLinkSelector, knobs *controlknobs.Knobs) *Manager {
+func NewManager(logf logger.Logf, oscfg OSConfigurator, health *health.Tracker, dialer *tsdial.Dialer, linkSel resolver.ForwardLinkSelector, knobs *controlknobs.Knobs, goos string) *Manager {
 	if dialer == nil {
 		panic("nil Dialer")
 	}
@@ -63,11 +64,15 @@ func NewManager(logf logger.Logf, oscfg OSConfigurator, health *health.Tracker,
 		panic("Dialer has nil NetMon")
 	}
 	logf = logger.WithPrefix(logf, "dns: ")
+	if goos == "" {
+		goos = runtime.GOOS
+	}
 	m := &Manager{
 		logf:     logf,
 		resolver: resolver.New(logf, linkSel, dialer, knobs),
 		os:       oscfg,
 		health:   health,
+		goos:     goos,
 	}
 	m.ctx, m.ctxCancel = context.WithCancel(context.Background())
 	m.logf("using %T", m.os)
@@ -166,7 +171,7 @@ func (m *Manager) compileConfig(cfg Config) (rcfg resolver.Config, ocfg OSConfig
 
 	// Similarly, the OS always gets search paths.
 	ocfg.SearchDomains = cfg.SearchDomains
-	if runtime.GOOS == "windows" {
+	if m.goos == "windows" {
 		ocfg.Hosts = compileHostEntries(cfg)
 	}
 
@@ -219,8 +224,9 @@ func (m *Manager) compileConfig(cfg Config) (rcfg resolver.Config, ocfg OSConfig
 	//
 	// This bool is used in a couple of places below to implement this
 	// workaround.
-	isWindows := runtime.GOOS == "windows"
-	if len(cfg.singleResolverSet()) > 0 && m.os.SupportsSplitDNS() && !isWindows {
+	isWindows := m.goos == "windows"
+	isApple := (m.goos == "darwin" || m.goos == "ios")
+	if len(cfg.singleResolverSet()) > 0 && m.os.SupportsSplitDNS() && !isWindows && !isApple {
 		// Split DNS configuration requested, where all split domains
 		// go to the same resolvers. We can let the OS do it.
 		ocfg.Nameservers = toIPsOnly(cfg.singleResolverSet())
@@ -240,8 +246,6 @@ func (m *Manager) compileConfig(cfg Config) (rcfg resolver.Config, ocfg OSConfig
 	// selectively answer ExtraRecords, and ignore other DNS traffic. As a
 	// workaround, we read the existing default resolver configuration and use
 	// that as the forwarder for all DNS traffic that quad-100 doesn't handle.
-	const isApple = runtime.GOOS == "darwin" || runtime.GOOS == "ios"
-
 	if isApple || !m.os.SupportsSplitDNS() {
 		// If the OS can't do native split-dns, read out the underlying
 		// resolver config and blend it into our config.
@@ -257,9 +261,8 @@ func (m *Manager) compileConfig(cfg Config) (rcfg resolver.Config, ocfg OSConfig
 		}
 	}
 
-	if baseCfg == nil || isApple && len(baseCfg.Nameservers) == 0 {
-		// If there was no base config, or if we're on Apple and the base
-		// config is empty, then we need to fallback to SplitDNS mode.
+	if baseCfg == nil {
+		// If there was no base config, then we need to fallback to SplitDNS mode.
 		ocfg.MatchDomains = cfg.matchDomains()
 	} else {
 		// On iOS only (for now), check if all route names point to resources inside the tailnet.
@@ -269,7 +272,7 @@ func (m *Manager) compileConfig(cfg Config) (rcfg resolver.Config, ocfg OSConfig
 		// we have any Routes outside the tailnet. Otherwise when app connectors are enabled,
 		// a query for 'work-laptop' might lead to search domain expansion, resolving
 		// as 'work-laptop.aws.com' for example.
-		if runtime.GOOS == "ios" && rcfg.RoutesRequireNoCustomResolvers() {
+		if m.goos == "ios" && rcfg.RoutesRequireNoCustomResolvers() {
 			for r := range rcfg.Routes {
 				ocfg.MatchDomains = append(ocfg.MatchDomains, r)
 			}
@@ -476,7 +479,7 @@ func CleanUp(logf logger.Logf, netMon *netmon.Monitor, interfaceName string) {
 	}
 	d := &tsdial.Dialer{Logf: logf}
 	d.SetNetMon(netMon)
-	dns := NewManager(logf, oscfg, nil, d, nil, nil)
+	dns := NewManager(logf, oscfg, nil, d, nil, nil, runtime.GOOS)
 	if err := dns.Down(); err != nil {
 		logf("dns down: %v", err)
 	}

net/dns/manager_tcp_test.go

@@ -88,7 +88,7 @@ func TestDNSOverTCP(t *testing.T) {
 			SearchDomains: fqdns("coffee.shop"),
 		},
 	}
-	m := NewManager(t.Logf, &f, nil, tsdial.NewDialer(netmon.NewStatic()), nil, nil)
+	m := NewManager(t.Logf, &f, nil, tsdial.NewDialer(netmon.NewStatic()), nil, nil, "")
 	m.resolver.TestOnlySetHook(f.SetResolver)
 	m.Set(Config{
 		Hosts: hosts(
@@ -173,7 +173,7 @@ func TestDNSOverTCP_TooLarge(t *testing.T) {
 			SearchDomains: fqdns("coffee.shop"),
 		},
 	}
-	m := NewManager(log, &f, nil, tsdial.NewDialer(netmon.NewStatic()), nil, nil)
+	m := NewManager(log, &f, nil, tsdial.NewDialer(netmon.NewStatic()), nil, nil, "")
 	m.resolver.TestOnlySetHook(f.SetResolver)
 	m.Set(Config{
 		Hosts:         hosts("andrew.ts.com.", "1.2.3.4"),

net/dns/manager_test.go

@@ -172,6 +172,7 @@ func TestManager(t *testing.T) {
 		bs    OSConfig
 		os    OSConfig
 		rs    resolver.Config
+		goos  string // empty means "linux"
 	}{
 		{
 			name: "empty",
@@ -424,7 +425,7 @@ func TestManager(t *testing.T) {
 			},
 		},
 		{
-			name: "routes-multi-split",
+			name: "routes-multi-split-linux",
 			in: Config{
 				Routes: upstreams(
 					"corp.com", "2.2.2.2",
@@ -442,6 +443,57 @@ func TestManager(t *testing.T) {
 					"corp.com.", "2.2.2.2",
 					"bigco.net.", "3.3.3.3"),
 			},
+			goos: "linux",
+		},
+		{
+			// The `routes-multi-split-linux` test case above on Darwin should NOT result in a split
+			// DNS configuration.
+			// Check that MatchDomains is empty. Due to Apple limitations, we cannot set MatchDomains
+			// without those domains also being SearchDomains.
+			name: "routes-multi-does-not-split-on-darwin",
+			in: Config{
+				Routes: upstreams(
+					"corp.com", "2.2.2.2",
+					"bigco.net", "3.3.3.3"),
+				SearchDomains: fqdns("tailscale.com", "universe.tf"),
+			},
+			split: false,
+			os: OSConfig{
+				Nameservers:   mustIPs("100.100.100.100"),
+				SearchDomains: fqdns("tailscale.com", "universe.tf"),
+			},
+			rs: resolver.Config{
+				Routes: upstreams(
+					".", "",
+					"corp.com.", "2.2.2.2",
+					"bigco.net.", "3.3.3.3"),
+			},
+			goos: "darwin",
+		},
+		{
+			// The `routes-multi-split-linux` test case above on iOS should NOT result in a split
+			// DNS configuration.
+			// Check that MatchDomains is empty. Due to Apple limitations, we cannot set MatchDomains
+			// without those domains also being SearchDomains.
+			name: "routes-multi-does-not-split-on-ios",
+			in: Config{
+				Routes: upstreams(
+					"corp.com", "2.2.2.2",
+					"bigco.net", "3.3.3.3"),
+				SearchDomains: fqdns("tailscale.com", "universe.tf"),
+			},
+			split: false,
+			os: OSConfig{
+				Nameservers:   mustIPs("100.100.100.100"),
+				SearchDomains: fqdns("tailscale.com", "universe.tf"),
+			},
+			rs: resolver.Config{
+				Routes: upstreams(
+					".", "",
+					"corp.com.", "2.2.2.2",
+					"bigco.net.", "3.3.3.3"),
+			},
+			goos: "ios",
 		},
 		{
 			name: "magic",
@@ -489,6 +541,59 @@ func TestManager(t *testing.T) {
 					"bradfitz.ts.com.", "2.3.4.5"),
 				LocalDomains: fqdns("ts.com."),
 			},
+			goos: "linux",
+		},
+		{
+			// The `magic-split` test case above on Darwin should NOT result in a split DNS configuration.
+			// Check that MatchDomains is empty. Due to Apple limitations, we cannot set MatchDomains
+			// without those domains also being SearchDomains.
+			name: "magic-split-does-not-split-on-darwin",
+			in: Config{
+				Hosts: hosts(
+					"dave.ts.com.", "1.2.3.4",
+					"bradfitz.ts.com.", "2.3.4.5"),
+				Routes:        upstreams("ts.com", ""),
+				SearchDomains: fqdns("tailscale.com", "universe.tf"),
+			},
+			split: false,
+			os: OSConfig{
+				Nameservers:   mustIPs("100.100.100.100"),
+				SearchDomains: fqdns("tailscale.com", "universe.tf"),
+			},
+			rs: resolver.Config{
+				Routes: upstreams(".", ""),
+				Hosts: hosts(
+					"dave.ts.com.", "1.2.3.4",
+					"bradfitz.ts.com.", "2.3.4.5"),
+				LocalDomains: fqdns("ts.com."),
+			},
+			goos: "darwin",
+		},
+		{
+			// The `magic-split` test case above on iOS should NOT result in a split DNS configuration.
+			// Check that MatchDomains is empty. Due to Apple limitations, we cannot set MatchDomains
+			// without those domains also being SearchDomains.
+			name: "magic-split-does-not-split-on-ios",
+			in: Config{
+				Hosts: hosts(
+					"dave.ts.com.", "1.2.3.4",
+					"bradfitz.ts.com.", "2.3.4.5"),
+				Routes:        upstreams("ts.com", ""),
+				SearchDomains: fqdns("tailscale.com", "universe.tf"),
+			},
+			split: false,
+			os: OSConfig{
+				Nameservers:   mustIPs("100.100.100.100"),
+				SearchDomains: fqdns("tailscale.com", "universe.tf"),
+			},
+			rs: resolver.Config{
+				Routes: upstreams(".", ""),
+				Hosts: hosts(
+					"dave.ts.com.", "1.2.3.4",
+					"bradfitz.ts.com.", "2.3.4.5"),
+				LocalDomains: fqdns("ts.com."),
+			},
+			goos: "ios",
 		},
 		{
 			name: "routes-magic",
@@ -518,7 +623,7 @@ func TestManager(t *testing.T) {
 			},
 		},
 		{
-			name: "routes-magic-split",
+			name: "routes-magic-split-linux",
 			in: Config{
 				Routes: upstreams(
 					"corp.com", "2.2.2.2",
@@ -541,6 +646,71 @@ func TestManager(t *testing.T) {
 					"bradfitz.ts.com.", "2.3.4.5"),
 				LocalDomains: fqdns("ts.com."),
 			},
+			goos: "linux",
+		},
+		{
+			// The `routes-magic-split-linux` test case above on Darwin should NOT result in a
+			// split DNS configuration.
+			// Check that MatchDomains is empty. Due to Apple limitations, we cannot set MatchDomains
+			// without those domains also being SearchDomains.
+			name: "routes-magic-does-not-split-on-darwin",
+			in: Config{
+				Routes: upstreams(
+					"corp.com", "2.2.2.2",
+					"ts.com", ""),
+				Hosts: hosts(
+					"dave.ts.com.", "1.2.3.4",
+					"bradfitz.ts.com.", "2.3.4.5"),
+				SearchDomains: fqdns("tailscale.com", "universe.tf"),
+			},
+			split: true,
+			os: OSConfig{
+				Nameservers:   mustIPs("100.100.100.100"),
+				SearchDomains: fqdns("tailscale.com", "universe.tf"),
+			},
+			rs: resolver.Config{
+				Routes: upstreams(
+					".", "",
+					"corp.com.", "2.2.2.2",
+				),
+				Hosts: hosts(
+					"dave.ts.com.", "1.2.3.4",
+					"bradfitz.ts.com.", "2.3.4.5"),
+				LocalDomains: fqdns("ts.com."),
+			},
+			goos: "darwin",
+		},
+		{
+			// The `routes-magic-split-linux` test case above on Darwin should NOT result in a
+			// split DNS configuration.
+			// Check that MatchDomains is empty. Due to Apple limitations, we cannot set MatchDomains
+			// without those domains also being SearchDomains.
+			name: "routes-magic-does-not-split-on-ios",
+			in: Config{
+				Routes: upstreams(
+					"corp.com", "2.2.2.2",
+					"ts.com", ""),
+				Hosts: hosts(
+					"dave.ts.com.", "1.2.3.4",
+					"bradfitz.ts.com.", "2.3.4.5"),
+				SearchDomains: fqdns("tailscale.com", "universe.tf"),
+			},
+			split: true,
+			os: OSConfig{
+				Nameservers:   mustIPs("100.100.100.100"),
+				SearchDomains: fqdns("tailscale.com", "universe.tf"),
+			},
+			rs: resolver.Config{
+				Routes: upstreams(
+					".", "",
+					"corp.com.", "2.2.2.2",
+				),
+				Hosts: hosts(
+					"dave.ts.com.", "1.2.3.4",
+					"bradfitz.ts.com.", "2.3.4.5"),
+				LocalDomains: fqdns("ts.com."),
+			},
+			goos: "ios",
 		},
 		{
 			name: "exit-node-forward",
@@ -598,6 +768,76 @@ func TestManager(t *testing.T) {
 				Routes: upstreams(".", "https://dns.nextdns.io/c3a884"),
 			},
 		},
+		{
+			// on iOS exclusively, tests the split DNS behavior for battery life optimization added in
+			// https://github.com/tailscale/tailscale/pull/10576
+			name: "ios-use-split-dns-when-no-custom-resolvers",
+			in: Config{
+				Routes:        upstreams("ts.net", "199.247.155.52", "optimistic-display.ts.net", ""),
+				SearchDomains: fqdns("optimistic-display.ts.net"),
+			},
+			split: true,
+			os: OSConfig{
+				Nameservers:   mustIPs("100.100.100.100"),
+				SearchDomains: fqdns("optimistic-display.ts.net"),
+				MatchDomains:  fqdns("ts.net"),
+			},
+			rs: resolver.Config{
+				Routes: upstreams(
+					".", "",
+					"ts.net", "199.247.155.52",
+				),
+				LocalDomains: fqdns("optimistic-display.ts.net."),
+			},
+			goos: "ios",
+		},
+		{
+			// if using app connectors, the battery life optimization above should not be applied
+			name: "ios-dont-use-split-dns-when-app-connector-resolver-needed",
+			in: Config{
+				Routes: upstreams(
+					"ts.net", "199.247.155.52",
+					"optimistic-display.ts.net", "",
+					"github.com", "https://dnsresolver.bigcorp.com/2f143"),
+				SearchDomains: fqdns("optimistic-display.ts.net"),
+			},
+			split: true,
+			os: OSConfig{
+				Nameservers:   mustIPs("100.100.100.100"),
+				SearchDomains: fqdns("optimistic-display.ts.net"),
+			},
+			rs: resolver.Config{
+				Routes: upstreams(
+					".", "",
+					"github.com", "https://dnsresolver.bigcorp.com/2f143",
+					"ts.net", "199.247.155.52",
+				),
+				LocalDomains: fqdns("optimistic-display.ts.net."),
+			},
+			goos: "ios",
+		},
+		{
+			// on darwin, verify that with the same config as in ios-use-split-dns-when-no-custom-resolvers,
+			// MatchDomains are NOT set.
+			name: "darwin-dont-use-split-dns-when-no-custom-resolvers",
+			in: Config{
+				Routes:        upstreams("ts.net", "199.247.155.52", "optimistic-display.ts.net", ""),
+				SearchDomains: fqdns("optimistic-display.ts.net"),
+			},
+			split: true,
+			os: OSConfig{
+				Nameservers:   mustIPs("100.100.100.100"),
+				SearchDomains: fqdns("optimistic-display.ts.net"),
+			},
+			rs: resolver.Config{
+				Routes: upstreams(
+					".", "",
+					"ts.net", "199.247.155.52",
+				),
+				LocalDomains: fqdns("optimistic-display.ts.net."),
+			},
+			goos: "darwin",
+		},
 	}
 
 	trIP := cmp.Transformer("ipStr", func(ip netip.Addr) string { return ip.String() })
@@ -614,7 +854,11 @@ func TestManager(t *testing.T) {
 				SplitDNS:   test.split,
 				BaseConfig: test.bs,
 			}
-			m := NewManager(t.Logf, &f, nil, tsdial.NewDialer(netmon.NewStatic()), nil, nil)
+			goos := test.goos
+			if goos == "" {
+				goos = "linux"
+			}
+			m := NewManager(t.Logf, &f, nil, tsdial.NewDialer(netmon.NewStatic()), nil, nil, goos)
 			m.resolver.TestOnlySetHook(f.SetResolver)
 
 			if err := m.Set(test.in); err != nil {

wgengine/userspace.go

@@ -348,7 +348,7 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error)
 	tunName, _ := conf.Tun.Name()
 	conf.Dialer.SetTUNName(tunName)
 	conf.Dialer.SetNetMon(e.netMon)
-	e.dns = dns.NewManager(logf, conf.DNS, e.health, conf.Dialer, fwdDNSLinkSelector{e, tunName}, conf.ControlKnobs)
+	e.dns = dns.NewManager(logf, conf.DNS, e.health, conf.Dialer, fwdDNSLinkSelector{e, tunName}, conf.ControlKnobs, runtime.GOOS)
 
 	// TODO: there's probably a better place for this
 	sockstats.SetNetMon(e.netMon)