From 10547d989d5fcb1007952a6a089d104d2e80f715 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Sat, 4 Sep 2021 23:40:48 -0700 Subject: [PATCH] net/dns: exhaustively test DNS selection paths for linux. Signed-off-by: David Anderson --- net/dns/manager_freebsd.go | 9 +- net/dns/manager_linux.go | 108 ++++++++++------- net/dns/manager_linux_test.go | 219 +++++++++++++++++++++++++++++++--- net/dns/resolvconf.go | 27 ++--- 4 files changed, 290 insertions(+), 73 deletions(-) diff --git a/net/dns/manager_freebsd.go b/net/dns/manager_freebsd.go index 624866f23..181b17b19 100644 --- a/net/dns/manager_freebsd.go +++ b/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 } diff --git a/net/dns/manager_linux.go b/net/dns/manager_linux.go index 63f06276b..fd701a83b 100644 --- a/net/dns/manager_linux.go +++ b/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 } diff --git a/net/dns/manager_linux_test.go b/net/dns/manager_linux_test.go index 62034f4e2..c1891822b 100644 --- a/net/dns/manager_linux_test.go +++ b/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 + }) +} diff --git a/net/dns/resolvconf.go b/net/dns/resolvconf.go index b70355166..d59f2117d 100644 --- a/net/dns/resolvconf.go +++ b/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" }