test(cli): cover the DNS reset failure paths through the real code

The genuine-failure cases fed a fabricated error straight to
logIfaceLookupFailure, so they exercised only its own branching. A
restoration failure never reaches that helper, and no test reached the
enumeration classification in netInterface at all: discarding the
enumeration error, or downgrading an actual restoration failure to
debug, both kept the suite green.

The host boundaries of the reset path are now variables, in the style
the package already uses: the interface enumeration, the interface
lookup, the DNS setters and the NetworkManager restore. The tests stub
them, so a run changes no host DNS or NetworkManager state, and drive
the real netInterface and resetDNSForRunningIface to assert that a
failed enumeration keeps its cause and is not reported as a missing
interface, and that a failure to restore the saved static config or to
reset to DHCP on an interface that does exist stays at error level.
Both surviving mutations, and three more, now fail.

cmd/cli/cli.go

@@ -1148,12 +1148,16 @@ func processLogAndCacheFlags(v *viper.Viper, cfg *ctrld.Config) {
 // adapter, a torn down tether) is an expected outcome rather than a failure.
 var errInterfaceNotFound = errors.New("interface not found")
 
+// foreachInterface enumerates the host network interfaces. It is a variable so
+// tests can make enumeration fail without depending on the host.
+var foreachInterface = netmon.ForeachInterface
+
 func netInterface(ifaceName string) (*net.Interface, error) {
 	if ifaceName == autoIface {
 		ifaceName = defaultIfaceName()
 	}
 	var iface *net.Interface
-	err := netmon.ForeachInterface(func(i netmon.Interface, prefixes []netip.Prefix) {
+	err := foreachInterface(func(i netmon.Interface, prefixes []netip.Prefix) {
 		if i.Name == ifaceName {
 			iface = i.Interface
 		}

cmd/cli/dns_reset_missing_iface_test.go

@@ -2,12 +2,17 @@ package cli
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
+	"net"
+	"net/netip"
 	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 
 	"github.com/rs/zerolog"
+	"tailscale.com/net/netmon"
 )
 
 // Test_logIfaceLookupFailure is a regression test for issue-608: a macOS
@@ -17,7 +22,11 @@ import (
 //
 // because the DNS cleanup after the upgrade logged every interface-lookup
 // failure at error level, including the interface it had been bound to simply
-// being gone. Only that positively identified condition may be downgraded.
+// being gone.
+//
+// This covers the helper's own branching. The conditions it must tell apart
+// are exercised through the real code in Test_netInterfaceLookupOutcomes and
+// Test_resetDNSForRunningIfaceFailures.
 func Test_logIfaceLookupFailure(t *testing.T) {
 	for _, tc := range []struct {
 		name      string
@@ -50,21 +59,13 @@ func Test_logIfaceLookupFailure(t *testing.T) {
 			wantMsg:   "Skipping interface snapshot: previous interface is no longer present",
 		},
 		{
+			// What patchNetIfaceName reports for an interface that does exist.
 			name:      "the lookup failed for another reason",
 			skipping:  "DNS restoration",
 			err:       fmt.Errorf("patching interface name: %w", os.ErrPermission),
 			wantLevel: "error",
 			wantMsg:   "could not get interface",
 		},
-		{
-			// A restoration failure reaches the same logger, and must stay
-			// visible: the interface exists and its DNS was left wrong.
-			name:      "restoring DNS on an existing interface failed",
-			skipping:  "DNS restoration",
-			err:       fmt.Errorf("failed to restore static DNS config on interface %q: %w", "en0", os.ErrPermission),
-			wantLevel: "error",
-			wantMsg:   "could not get interface",
-		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			var buf syncBuffer
@@ -93,49 +94,250 @@ func Test_logIfaceLookupFailure(t *testing.T) {
 	}
 }
 
-// Test_resetDNSForRunningIfaceMissingIface drives the real cleanup path with an
-// interface that does not exist, which is what an upgrade sees after the
-// adapter ctrld was bound to went away. It changes no DNS settings: the lookup
-// fails before any restoration runs.
-func Test_resetDNSForRunningIfaceMissingIface(t *testing.T) {
+// Test_netInterfaceLookupOutcomes pins which lookup failures netInterface may
+// report as a missing interface. Only that error is quiet downstream, so an
+// enumeration that never ran must not be reported as one: the host may well
+// still have the interface.
+func Test_netInterfaceLookupOutcomes(t *testing.T) {
+	errEnumerate := errors.New("route socket closed")
+
+	t.Run("enumeration failed", func(t *testing.T) {
+		stubForeachInterface(t, func(fn func(netmon.Interface, []netip.Prefix)) error {
+			return errEnumerate
+		})
+
+		iface, err := netInterface("en5")
+
+		if iface != nil {
+			t.Errorf("iface = %v, want nil", iface)
+		}
+		if !errors.Is(err, errEnumerate) {
+			t.Errorf("err = %v, want it to keep the enumeration cause %v", err, errEnumerate)
+		}
+		if errors.Is(err, errInterfaceNotFound) {
+			t.Error("a failed enumeration must not report the interface as missing: " +
+				"the caller would then skip DNS restoration on an interface that still exists")
+		}
+	})
+
+	t.Run("enumeration ran and the interface is absent", func(t *testing.T) {
+		stubForeachInterface(t, func(fn func(netmon.Interface, []netip.Prefix)) error {
+			fn(netmon.Interface{Interface: &net.Interface{Index: 1, Name: "en0"}}, nil)
+			return nil
+		})
+
+		iface, err := netInterface("en5")
+
+		if iface != nil {
+			t.Errorf("iface = %v, want nil", iface)
+		}
+		if !errors.Is(err, errInterfaceNotFound) {
+			t.Errorf("err = %v, want errInterfaceNotFound", err)
+		}
+	})
+}
+
+// Test_resetDNSForRunningIfaceFailures drives the real cleanup path and asserts
+// how each outcome is reported: the interface being gone is a debug skip, while
+// a failure to restore DNS on an interface that does exist stays an error. The
+// OS boundaries are stubbed, so no host DNS or NetworkManager state is touched.
+func Test_resetDNSForRunningIfaceFailures(t *testing.T) {
+	errRestore := errors.New("permission denied")
+
+	t.Run("the previous interface is gone", func(t *testing.T) {
+		buf := captureDNSResetLogs(t)
+		// The real lookup over an enumeration that does not list the
+		// interface, which is what an upgrade sees after the adapter ctrld
+		// was bound to went away.
+		stubForeachInterface(t, func(fn func(netmon.Interface, []netip.Prefix)) error {
+			fn(netmon.Interface{Interface: &net.Interface{Index: 1, Name: "en0"}}, nil)
+			return nil
+		})
+		stubDNSResetBoundaries(t, func(iface *net.Interface, nameservers []string) error {
+			t.Fatalf("setDNS called for a missing interface: %v", nameservers)
+			return nil
+		}, func(iface *net.Interface) error {
+			t.Fatal("resetDNS called for a missing interface")
+			return nil
+		})
+		netInterfaceFn = netInterface
+
+		p := &prog{runningIface: "en5"}
+		if got := p.resetDNSForRunningIface(false, true); got != nil {
+			t.Errorf("resetDNSForRunningIface returned %v, want nil for a missing interface", got)
+		}
+
+		entry := findLogEntry(t, buf.String(), "previous interface is no longer present")
+		if got := entry["level"]; got != "debug" {
+			t.Errorf("level = %v, want %q: a missing interface must not make a successful upgrade look broken",
+				got, "debug")
+		}
+		if got := entry["iface"]; got != "en5" {
+			t.Errorf("iface = %v, want %q", got, "en5")
+		}
+	})
+
+	t.Run("restoring the saved static config failed", func(t *testing.T) {
+		buf := captureDNSResetLogs(t)
+		iface := stubExistingIface(t)
+		saveStaticNameservers(t, iface, "192.0.2.1")
+		called := false
+		stubDNSResetBoundaries(t, func(i *net.Interface, nameservers []string) error {
+			called = true
+			return errRestore
+		}, func(i *net.Interface) error {
+			t.Fatal("resetDNS called although a saved static config exists")
+			return nil
+		})
+
+		p := &prog{runningIface: iface.Name}
+		p.resetDNSForRunningIface(false, true)
+
+		if !called {
+			t.Fatal("setDNS was never called: the saved static config was not restored")
+		}
+		entry := findLogEntry(t, buf.String(), "failed to restore static DNS config")
+		if got := entry["level"]; got != "error" {
+			t.Errorf("level = %v, want %q: the interface exists and its DNS was left wrong",
+				got, "error")
+		}
+		if got := entry["error"]; got != errRestore.Error() {
+			t.Errorf("error = %v, want %q", got, errRestore)
+		}
+	})
+
+	t.Run("resetting to DHCP failed", func(t *testing.T) {
+		buf := captureDNSResetLogs(t)
+		iface := stubExistingIface(t)
+		// No saved static config, so the cleanup falls back to DHCP.
+		called := false
+		stubDNSResetBoundaries(t, func(i *net.Interface, nameservers []string) error {
+			t.Fatalf("setDNS called without a saved static config: %v", nameservers)
+			return nil
+		}, func(i *net.Interface) error {
+			called = true
+			return errRestore
+		})
+
+		p := &prog{runningIface: iface.Name}
+		p.resetDNSForRunningIface(false, true)
+
+		if !called {
+			t.Fatal("resetDNS was never called: the interface was not reset to DHCP")
+		}
+		entry := findLogEntry(t, buf.String(), "failed to reset DNS to DHCP")
+		if got := entry["level"]; got != "error" {
+			t.Errorf("level = %v, want %q: the interface exists and its DNS was left wrong",
+				got, "error")
+		}
+		if got := entry["error"]; got != errRestore.Error() {
+			t.Errorf("error = %v, want %q", got, errRestore)
+		}
+	})
+}
+
+// stubForeachInterface replaces the host interface enumeration for one test.
+func stubForeachInterface(t *testing.T, fn func(func(netmon.Interface, []netip.Prefix)) error) {
+	t.Helper()
+	old := foreachInterface
+	foreachInterface = fn
+	t.Cleanup(func() { foreachInterface = old })
+}
+
+// stubDNSResetBoundaries replaces the parts of the DNS reset path that change
+// host state: the DNS setters and the NetworkManager restore.
+func stubDNSResetBoundaries(t *testing.T, set func(*net.Interface, []string) error, reset func(*net.Interface) error) {
+	t.Helper()
+	oldSet, oldReset, oldNM := setIfaceDNSFn, resetIfaceDNSFn, restoreNetworkManagerFn
+	setIfaceDNSFn, resetIfaceDNSFn, restoreNetworkManagerFn = set, reset, func() error { return nil }
+	t.Cleanup(func() {
+		setIfaceDNSFn, resetIfaceDNSFn, restoreNetworkManagerFn = oldSet, oldReset, oldNM
+	})
+}
+
+// stubExistingIface makes the lookup report an interface that is present.
+func stubExistingIface(t *testing.T) *net.Interface {
+	t.Helper()
+	iface := &net.Interface{Index: 1, Name: "ctrld-test0"}
+	old := netInterfaceFn
+	netInterfaceFn = func(name string) (*net.Interface, error) {
+		if name != iface.Name {
+			return nil, errInterfaceNotFound
+		}
+		return iface, nil
+	}
+	t.Cleanup(func() { netInterfaceFn = old })
+	return iface
+}
+
+// saveStaticNameservers writes the saved static DNS config that the cleanup
+// restores, into a home directory private to this test.
+func saveStaticNameservers(t *testing.T, iface *net.Interface, nameservers ...string) {
+	t.Helper()
+	old := homedir
+	homedir = t.TempDir()
+	t.Cleanup(func() { homedir = old })
+	file := filepath.Join(homedir, ".dns_"+iface.Name)
+	if err := os.WriteFile(file, []byte(strings.Join(nameservers, ",")), 0600); err != nil {
+		t.Fatalf("writing %s: %v", file, err)
+	}
+}
+
+// captureDNSResetLogs redirects mainLog and enables debug output for one test.
+func captureDNSResetLogs(t *testing.T) *syncBuffer {
+	t.Helper()
 	level := zerolog.GlobalLevel()
 	zerolog.SetGlobalLevel(zerolog.DebugLevel)
-	t.Cleanup(func() { zerolog.SetGlobalLevel(level) })
-
 	old := mainLog.Load()
-	var buf syncBuffer
-	logger := zerolog.New(&buf)
+	buf := &syncBuffer{}
+	logger := zerolog.New(buf)
 	mainLog.Store(&logger)
-	t.Cleanup(func() { mainLog.Store(old) })
-
-	// A name no host assigns, so netInterface reports it as missing rather
-	// than restoring DNS on a real adapter.
-	p := &prog{runningIface: "ctrld-absent-iface0"}
-
-	if got := p.resetDNSForRunningIface(false, true); got != nil {
-		t.Errorf("resetDNSForRunningIface returned %v, want nil for a missing interface", got)
-	}
-
-	entry := soleLogEntry(t, buf.String())
-	if got := entry["level"]; got != "debug" {
-		t.Errorf("level = %v, want %q: a missing interface must not make a successful upgrade look broken (line: %s)",
-			got, "debug", strings.TrimSpace(buf.String()))
-	}
-	if got := entry["iface"]; got != p.runningIface {
-		t.Errorf("iface = %v, want %q", got, p.runningIface)
-	}
+	t.Cleanup(func() {
+		mainLog.Store(old)
+		zerolog.SetGlobalLevel(level)
+	})
+	return buf
 }
 
 // soleLogEntry decodes the single JSON log line in out.
 func soleLogEntry(t *testing.T, out string) map[string]any {
 	t.Helper()
-	lines := strings.Split(strings.TrimSpace(out), "\n")
-	if len(lines) != 1 || lines[0] == "" {
-		t.Fatalf("want exactly one log line, got %d:\n%s", len(lines), out)
+	entries := logEntries(t, out)
+	if len(entries) != 1 {
+		t.Fatalf("want exactly one log line, got %d:\n%s", len(entries), out)
 	}
-	var entry map[string]any
-	if err := json.Unmarshal([]byte(lines[0]), &entry); err != nil {
-		t.Fatalf("decoding log line %q: %v", lines[0], err)
+	return entries[0]
+}
+
+// findLogEntry returns the one log line whose message contains want.
+func findLogEntry(t *testing.T, out, want string) map[string]any {
+	t.Helper()
+	var found []map[string]any
+	for _, entry := range logEntries(t, out) {
+		if msg, _ := entry["message"].(string); strings.Contains(msg, want) {
+			found = append(found, entry)
+		}
 	}
-	return entry
+	if len(found) != 1 {
+		t.Fatalf("want exactly one log line containing %q, got %d:\n%s", want, len(found), out)
+	}
+	return found[0]
+}
+
+// logEntries decodes the JSON log lines in out.
+func logEntries(t *testing.T, out string) []map[string]any {
+	t.Helper()
+	out = strings.TrimSpace(out)
+	if out == "" {
+		t.Fatal("no log output")
+	}
+	var entries []map[string]any
+	for _, line := range strings.Split(out, "\n") {
+		var entry map[string]any
+		if err := json.Unmarshal([]byte(line), &entry); err != nil {
+			t.Fatalf("decoding log line %q: %v", line, err)
+		}
+		entries = append(entries, entry)
+	}
+	return entries
 }

cmd/cli/prog.go

@@ -1810,6 +1810,15 @@ func (p *prog) resetDNS(isStart bool, restoreStatic bool) {
 	}
 }
 
+// The OS boundaries of the DNS reset path, as variables so tests can exercise
+// its failure handling without changing the host's DNS or NetworkManager state.
+var (
+	netInterfaceFn          = netInterface
+	restoreNetworkManagerFn = restoreNetworkManager
+	setIfaceDNSFn           = setDNS
+	resetIfaceDNSFn         = resetDNS
+)
+
 // logIfaceLookupFailure reports a failed lookup of the interface the caller was
 // about to work on, naming that work in skipping, e.g. "DNS restoration". An
 // interface that no longer exists has nothing left to act on — an unplugged
@@ -1838,13 +1847,13 @@ func (p *prog) resetDNSForRunningIface(isStart bool, restoreStatic bool) (runnin
 		return
 	}
 	logger := mainLog.Load().With().Str("iface", p.runningIface).Logger()
-	netIface, err := netInterface(p.runningIface)
+	netIface, err := netInterfaceFn(p.runningIface)
 	if err != nil {
 		logIfaceLookupFailure(&logger, "DNS restoration", err)
 		return
 	}
 	runningIface = netIface
-	if err := restoreNetworkManager(); err != nil {
+	if err := restoreNetworkManagerFn(); err != nil {
 		logger.Error().Err(err).Msg("could not restore NetworkManager")
 		return
 	}
@@ -1874,13 +1883,13 @@ func (p *prog) resetDNSForRunningIface(isStart bool, restoreStatic bool) (runnin
 	saved := savedStaticNameservers(netIface)
 	if len(saved) > 0 && restoreStatic {
 		logger.Debug().Msgf("Restoring interface %q from saved static config: %v", netIface.Name, saved)
-		if err := setDNS(netIface, saved); err != nil {
+		if err := setIfaceDNSFn(netIface, saved); err != nil {
 			logger.Error().Err(err).Msgf("failed to restore static DNS config on interface %q", netIface.Name)
 			return
 		}
 	} else {
 		logger.Debug().Msgf("No saved static DNS config for interface %q; resetting to DHCP", netIface.Name)
-		if err := resetDNS(netIface); err != nil {
+		if err := resetIfaceDNSFn(netIface); err != nil {
 			logger.Error().Err(err).Msgf("failed to reset DNS to DHCP on interface %q", netIface.Name)
 			return
 		}