net/dns: exhaustively test DNS selection paths for linux.

Signed-off-by: David Anderson <danderson@tailscale.com>

net/dns/manager_freebsd.go

@@ -23,7 +23,14 @@ func NewOSConfigurator(logf logger.Logf, _ string) (OSConfigurator, error) {
 
 	switch resolvOwner(bs) {
 	case "resolvconf":
-		return newResolvconfManager(logf, getResolvConfVersion)
+		switch resolvconfStyle() {
+		case "":
+			return newDirectManager(), nil
+		case "debian":
+			return newDebianResolvconfManager(logf)
+		case "openresolv":
+			return newOpenresolvManager()
+		}
 	default:
 		return newDirectManager(), nil
 	}

net/dns/manager_linux.go

@@ -5,11 +5,11 @@
 package dns
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
 	"os"
-	"os/exec"
 	"time"
 
 	"github.com/godbus/dbus/v5"
@@ -27,36 +27,52 @@ func (kv kv) String() string {
 }
 
 func NewOSConfigurator(logf logger.Logf, interfaceName string) (ret OSConfigurator, err error) {
-	return newOSConfigurator(logf, interfaceName, newOSConfigEnv{
-		fs:                         directFS{},
-		resolvOwner:                resolvOwner,
-		resolvedIsActuallyResolver: resolvedIsActuallyResolver,
-		dbusPing:                   dbusPing,
-		nmIsUsingResolved:          nmIsUsingResolved,
-		nmVersionBetween:           nmVersionBetween,
-		getResolvConfVersion:       getResolvConfVersion,
-	})
+	env := newOSConfigEnv{
+		fs:                directFS{},
+		dbusPing:          dbusPing,
+		nmIsUsingResolved: nmIsUsingResolved,
+		nmVersionBetween:  nmVersionBetween,
+		resolvconfStyle:   resolvconfStyle,
+	}
+	mode, err := dnsMode(logf, env)
+	if err != nil {
+		return nil, err
+	}
+	switch mode {
+	case "direct":
+		return newDirectManagerOnFS(env.fs), nil
+	case "systemd-resolved":
+		return newResolvedManager(logf, interfaceName)
+	case "network-manager":
+		return newNMManager(interfaceName)
+	case "debian-resolvconf":
+		return newDebianResolvconfManager(logf)
+	case "openresolv":
+		return newOpenresolvManager()
+	default:
+		logf("[unexpected] detected unknown DNS mode %q, using direct manager as last resort", mode)
+		return newDirectManagerOnFS(env.fs), nil
+	}
 }
 
 // newOSConfigEnv are the funcs newOSConfigurator needs, pulled out for testing.
 type newOSConfigEnv struct {
-	fs                         wholeFileFS
-	resolvOwner                func(resolvConfContents []byte) string
-	resolvedIsActuallyResolver func(wholeFileFS) error
-	dbusPing                   func(string, string) error
-	nmIsUsingResolved          func() error
-	nmVersionBetween           func(v1, v2 string) (safe bool, err error)
-	getResolvConfVersion       func() ([]byte, error)
+	fs                        wholeFileFS
+	dbusPing                  func(string, string) error
+	nmIsUsingResolved         func() error
+	nmVersionBetween          func(v1, v2 string) (safe bool, err error)
+	resolvconfStyle           func() string
+	isResolvconfDebianVersion func() bool
 }
 
-func newOSConfigurator(logf logger.Logf, interfaceName string, env newOSConfigEnv) (ret OSConfigurator, err error) {
+func dnsMode(logf logger.Logf, env newOSConfigEnv) (ret string, err error) {
 	var debug []kv
 	dbg := func(k, v string) {
 		debug = append(debug, kv{k, v})
 	}
 	defer func() {
-		if ret != nil {
-			dbg("ret", fmt.Sprintf("%T", ret))
+		if ret != "" {
+			dbg("ret", ret)
 		}
 		logf("dns: %v", debug)
 	}()
@@ -64,13 +80,13 @@ func newOSConfigurator(logf logger.Logf, interfaceName string, env newOSConfigEn
 	bs, err := env.fs.ReadFile(resolvConf)
 	if os.IsNotExist(err) {
 		dbg("rc", "missing")
-		return newDirectManager(), nil
+		return "direct", nil
 	}
 	if err != nil {
-		return nil, fmt.Errorf("reading /etc/resolv.conf: %w", err)
+		return "", fmt.Errorf("reading /etc/resolv.conf: %w", err)
 	}
 
-	switch env.resolvOwner(bs) {
+	switch resolvOwner(bs) {
 	case "systemd-resolved":
 		dbg("rc", "resolved")
 		// Some systems, for reasons known only to them, have a
@@ -78,22 +94,22 @@ func newOSConfigurator(logf logger.Logf, interfaceName string, env newOSConfigEn
 		// header, but doesn't actually point to resolved. We mustn't
 		// try to program resolved in that case.
 		// https://github.com/tailscale/tailscale/issues/2136
-		if err := env.resolvedIsActuallyResolver(env.fs); err != nil {
+		if err := resolvedIsActuallyResolver(bs); err != nil {
 			dbg("resolved", "not-in-use")
-			return newDirectManagerOnFS(env.fs), nil
+			return "direct", nil
 		}
 		if err := env.dbusPing("org.freedesktop.resolve1", "/org/freedesktop/resolve1"); err != nil {
 			dbg("resolved", "no")
-			return newDirectManagerOnFS(env.fs), nil
+			return "direct", nil
 		}
 		if err := env.dbusPing("org.freedesktop.NetworkManager", "/org/freedesktop/NetworkManager/DnsManager"); err != nil {
 			dbg("nm", "no")
-			return newResolvedManager(logf, interfaceName)
+			return "systemd-resolved", nil
 		}
 		dbg("nm", "yes")
 		if err := env.nmIsUsingResolved(); err != nil {
 			dbg("nm-resolved", "no")
-			return newResolvedManager(logf, interfaceName)
+			return "systemd-resolved", nil
 		}
 		dbg("nm-resolved", "yes")
 
@@ -131,26 +147,38 @@ func newOSConfigurator(logf logger.Logf, interfaceName string, env newOSConfigEn
 		// that comes with it (see
 		// https://github.com/tailscale/tailscale/issues/1699,
 		// https://github.com/tailscale/tailscale/pull/1945)
-		safe, err := nmVersionBetween("1.26.0", "1.26.5")
+		safe, err := env.nmVersionBetween("1.26.0", "1.26.5")
 		if err != nil {
 			// Failed to figure out NM's version, can't make a correct
 			// decision.
-			return nil, fmt.Errorf("checking NetworkManager version: %v", err)
+			return "", fmt.Errorf("checking NetworkManager version: %v", err)
 		}
 		if safe {
 			dbg("nm-safe", "yes")
-			return newNMManager(interfaceName)
+			return "network-manager", nil
 		}
 		dbg("nm-safe", "no")
-		return newResolvedManager(logf, interfaceName)
+		return "systemd-resolved", nil
 	case "resolvconf":
 		dbg("rc", "resolvconf")
-		if _, err := exec.LookPath("resolvconf"); err != nil {
+		style := env.resolvconfStyle()
+		switch style {
+		case "":
 			dbg("resolvconf", "no")
-			return newDirectManagerOnFS(env.fs), nil
+			return "direct", nil
+		case "debian":
+			dbg("resolvconf", "debian")
+			return "debian-resolvconf", nil
+		case "openresolv":
+			dbg("resolvconf", "openresolv")
+			return "openresolv", nil
+		default:
+			// Shouldn't happen, that means we updated flavors of
+			// resolvconf without updating here.
+			dbg("resolvconf", style)
+			logf("[unexpected] got unknown flavor of resolvconf %q, falling back to direct manager", env.resolvconfStyle())
+			return "direct", nil
 		}
-		dbg("resolvconf", "yes")
-		return newResolvconfManager(logf, env.getResolvConfVersion)
 	case "NetworkManager":
 		// You'd think we would use newNMManager somewhere in
 		// here. However, as explained in
@@ -165,10 +193,10 @@ func newOSConfigurator(logf logger.Logf, interfaceName string, env newOSConfigEn
 		// anyway, so you still need a fallback path that uses
 		// directManager.
 		dbg("rc", "nm")
-		return newDirectManagerOnFS(env.fs), nil
+		return "direct", nil
 	default:
 		dbg("rc", "unknown")
-		return newDirectManagerOnFS(env.fs), nil
+		return "direct", nil
 	}
 }
 
@@ -216,8 +244,8 @@ func nmIsUsingResolved() error {
 	return nil
 }
 
-func resolvedIsActuallyResolver(fs wholeFileFS) error {
-	cfg, err := newDirectManagerOnFS(fs).readResolvConf()
+func resolvedIsActuallyResolver(bs []byte) error {
+	cfg, err := readResolv(bytes.NewBuffer(bs))
 	if err != nil {
 		return err
 	}

net/dns/manager_linux_test.go

@@ -6,29 +6,142 @@ package dns
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"io/fs"
 	"os"
+	"strings"
 	"testing"
+
+	"tailscale.com/util/cmpver"
 )
 
-func TestLinuxNewOSConfigurator(t *testing.T) {
+func TestLinuxDNSMode(t *testing.T) {
 	tests := []struct {
 		name    string
 		env     newOSConfigEnv
 		wantLog string
-		want    string // reflect type string
+		want    string
 	}{
 		{
-			name: "no_obvious_resolv.conf_owner",
-			env: newOSConfigEnv{
-				fs: memFS{
-					"/etc/resolv.conf": "nameserver 10.0.0.1\n",
-				},
-				resolvOwner: resolvOwner,
-			},
-			wantLog: "dns: [rc=unknown ret=dns.directManager]\n",
-			want:    "dns.directManager",
+			name:    "no_obvious_resolv.conf_owner",
+			env:     env(resolvDotConf("nameserver 10.0.0.1")),
+			wantLog: "dns: [rc=unknown ret=direct]",
+			want:    "direct",
+		},
+		{
+			name: "network_manager",
+			env: env(
+				resolvDotConf(
+					"# Managed by NetworkManager",
+					"nameserver 10.0.0.1")),
+			wantLog: "dns: [rc=nm ret=direct]",
+			want:    "direct",
+		},
+		{
+			name:    "resolvconf_but_no_resolvconf_binary",
+			env:     env(resolvDotConf("# Managed by resolvconf", "nameserver 10.0.0.1")),
+			wantLog: "dns: [rc=resolvconf resolvconf=no ret=direct]",
+			want:    "direct",
+		},
+		{
+			name: "debian_resolvconf",
+			env: env(
+				resolvDotConf("# Managed by resolvconf", "nameserver 10.0.0.1"),
+				resolvconf("debian")),
+			wantLog: "dns: [rc=resolvconf resolvconf=debian ret=debian-resolvconf]",
+			want:    "debian-resolvconf",
+		},
+		{
+			name: "openresolv",
+			env: env(
+				resolvDotConf("# Managed by resolvconf", "nameserver 10.0.0.1"),
+				resolvconf("openresolv")),
+			wantLog: "dns: [rc=resolvconf resolvconf=openresolv ret=openresolv]",
+			want:    "openresolv",
+		},
+		{
+			name: "unknown_resolvconf_flavor",
+			env: env(
+				resolvDotConf("# Managed by resolvconf", "nameserver 10.0.0.1"),
+				resolvconf("daves-discount-resolvconf")),
+			wantLog: "[unexpected] got unknown flavor of resolvconf \"daves-discount-resolvconf\", falling back to direct manager\ndns: [rc=resolvconf resolvconf=daves-discount-resolvconf ret=direct]",
+			want:    "direct",
+		},
+		{
+			name:    "resolved_not_running",
+			env:     env(resolvDotConf("# Managed by systemd-resolved", "nameserver 127.0.0.53")),
+			wantLog: "dns: [rc=resolved resolved=no ret=direct]",
+			want:    "direct",
+		},
+		{
+			name: "resolved_alone",
+			env: env(
+				resolvDotConf("# Managed by systemd-resolved", "nameserver 127.0.0.53"),
+				resolvedRunning()),
+			wantLog: "dns: [rc=resolved nm=no ret=systemd-resolved]",
+			want:    "systemd-resolved",
+		},
+		{
+			name: "resolved_and_networkmanager_not_using_resolved",
+			env: env(
+				resolvDotConf("# Managed by systemd-resolved", "nameserver 127.0.0.53"),
+				resolvedRunning(),
+				nmRunning("1.2.3", false)),
+			wantLog: "dns: [rc=resolved nm=yes nm-resolved=no ret=systemd-resolved]",
+			want:    "systemd-resolved",
+		},
+		{
+			name: "resolved_and_mid_2020_networkmanager",
+			env: env(
+				resolvDotConf("# Managed by systemd-resolved", "nameserver 127.0.0.53"),
+				resolvedRunning(),
+				nmRunning("1.26.2", true)),
+			wantLog: "dns: [rc=resolved nm=yes nm-resolved=yes nm-safe=yes ret=network-manager]",
+			want:    "network-manager",
+		},
+		{
+			name: "resolved_and_2021_networkmanager",
+			env: env(
+				resolvDotConf("# Managed by systemd-resolved", "nameserver 127.0.0.53"),
+				resolvedRunning(),
+				nmRunning("1.27.0", true)),
+			wantLog: "dns: [rc=resolved nm=yes nm-resolved=yes nm-safe=no ret=systemd-resolved]",
+			want:    "systemd-resolved",
+		},
+		{
+			name: "resolved_and_ancient_networkmanager",
+			env: env(
+				resolvDotConf("# Managed by systemd-resolved", "nameserver 127.0.0.53"),
+				resolvedRunning(),
+				nmRunning("1.22.0", true)),
+			wantLog: "dns: [rc=resolved nm=yes nm-resolved=yes nm-safe=no ret=systemd-resolved]",
+			want:    "systemd-resolved",
+		},
+		// Regression tests for extreme corner cases below.
+		{
+			// One user reported a configuration whose comment string
+			// alleged that it was managed by systemd-resolved, but it
+			// was actually a completely static config file pointing
+			// elsewhere.
+			name:    "allegedly_resolved_but_not_in_resolv.conf",
+			env:     env(resolvDotConf("# Managed by systemd-resolved", "nameserver 10.0.0.1")),
+			wantLog: "dns: [rc=resolved resolved=not-in-use ret=direct]",
+			want:    "direct",
+		},
+		{
+			// We used to incorrectly decide that resolved wasn't in
+			// charge when handed this (admittedly weird and bugged)
+			// resolv.conf.
+			name: "resolved_with_duplicates_in_resolv.conf",
+			env: env(
+				resolvDotConf(
+					"# Managed by systemd-resolved",
+					"nameserver 127.0.0.53",
+					"nameserver 127.0.0.53"),
+				resolvedRunning()),
+			wantLog: "dns: [rc=resolved nm=no ret=systemd-resolved]",
+			want:    "systemd-resolved",
 		},
 	}
 	for _, tt := range tests {
@@ -38,15 +151,15 @@ func TestLinuxNewOSConfigurator(t *testing.T) {
 				fmt.Fprintf(&logBuf, format, a...)
 				logBuf.WriteByte('\n')
 			}
-			osc, err := newOSConfigurator(logf, "unused_if_name0", tt.env)
+			got, err := dnsMode(logf, tt.env)
 			if err != nil {
 				t.Fatal(err)
 			}
-			if got := fmt.Sprintf("%T", osc); got != tt.want {
+			if got != tt.want {
 				t.Errorf("got %s; want %s", got, tt.want)
 			}
-			if tt.wantLog != string(logBuf.Bytes()) {
-				t.Errorf("log output mismatch:\n got: %q\nwant: %q\n", logBuf.Bytes(), tt.wantLog)
+			if got := strings.TrimSpace(logBuf.String()); got != tt.wantLog {
+				t.Errorf("log output mismatch:\n got: %q\nwant: %q\n", got, tt.wantLog)
 			}
 		})
 	}
@@ -82,3 +195,79 @@ func (fs memFS) WriteFile(name string, contents []byte, perm os.FileMode) error
 	fs[name] = string(contents)
 	return nil
 }
+
+type envBuilder struct {
+	fs              memFS
+	dbus            []struct{ name, path string }
+	nmUsingResolved bool
+	nmVersion       string
+	resolvconfStyle string
+}
+
+type envOption interface {
+	apply(*envBuilder)
+}
+
+type envOpt func(*envBuilder)
+
+func (e envOpt) apply(b *envBuilder) {
+	e(b)
+}
+
+func env(opts ...envOption) newOSConfigEnv {
+	b := &envBuilder{
+		fs: memFS{},
+	}
+	for _, opt := range opts {
+		opt.apply(b)
+	}
+
+	return newOSConfigEnv{
+		fs: b.fs,
+		dbusPing: func(name, path string) error {
+			for _, svc := range b.dbus {
+				if svc.name == name && svc.path == path {
+					return nil
+				}
+			}
+			return errors.New("dbus service not found")
+		},
+		nmIsUsingResolved: func() error {
+			if !b.nmUsingResolved {
+				return errors.New("networkmanager not using resolved")
+			}
+			return nil
+		},
+		nmVersionBetween: func(first, last string) (bool, error) {
+			outside := cmpver.Compare(b.nmVersion, first) < 0 || cmpver.Compare(b.nmVersion, last) > 0
+			return !outside, nil
+		},
+		resolvconfStyle: func() string { return b.resolvconfStyle },
+	}
+}
+
+func resolvDotConf(ss ...string) envOption {
+	return envOpt(func(b *envBuilder) {
+		b.fs["/etc/resolv.conf"] = strings.Join(ss, "\n")
+	})
+}
+
+func resolvedRunning() envOption {
+	return envOpt(func(b *envBuilder) {
+		b.dbus = append(b.dbus, struct{ name, path string }{"org.freedesktop.resolve1", "/org/freedesktop/resolve1"})
+	})
+}
+
+func nmRunning(version string, usingResolved bool) envOption {
+	return envOpt(func(b *envBuilder) {
+		b.nmUsingResolved = usingResolved
+		b.nmVersion = version
+		b.dbus = append(b.dbus, struct{ name, path string }{"org.freedesktop.NetworkManager", "/org/freedesktop/NetworkManager/DnsManager"})
+	})
+}
+
+func resolvconf(s string) envOption {
+	return envOpt(func(b *envBuilder) {
+		b.resolvconfStyle = s
+	})
+}

net/dns/resolvconf.go

@@ -9,26 +9,19 @@ package dns
 
 import (
 	"os/exec"
-
-	"tailscale.com/types/logger"
 )
 
-func getResolvConfVersion() ([]byte, error) {
-	return exec.Command("resolvconf", "--version").CombinedOutput()
-}
-
-func newResolvconfManager(logf logger.Logf, getResolvConfVersion func() ([]byte, error)) (OSConfigurator, error) {
-	_, err := getResolvConfVersion()
-	if err != nil {
+func resolvconfStyle() string {
+	if _, err := exec.LookPath("resolvconf"); err != nil {
+		return ""
+	}
+	if _, err := exec.Command("resolvconf", "--version").CombinedOutput(); err != nil {
+		// Debian resolvconf doesn't understand --version, and
+		// exits with a specific error code.
 		if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 99 {
-			// Debian resolvconf doesn't understand --version, and
-			// exits with a specific error code.
-			return newDebianResolvconfManager(logf)
+			return "debian"
 		}
 	}
-	// If --version works, or we got some surprising error while
-	// probing, use openresolv. It's the more common implementation,
-	// so in cases where we can't figure things out, it's the least
-	// likely to misbehave.
-	return newOpenresolvManager()
+	// Treat everything else as openresolv, by far the more popular implementation.
+	return "openresolv"
 }