diff --git a/cmd/cli/cli.go b/cmd/cli/cli.go index de78e8e..3d5f534 100644 --- a/cmd/cli/cli.go +++ b/cmd/cli/cli.go @@ -451,18 +451,7 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { p.onStopped = append(p.onStopped, func() { // restore static DNS settings or DHCP p.resetDNS(false, true) - // Iterate over all physical interfaces and restore static DNS if a saved static config exists. - withEachPhysicalInterfaces("", "restore static DNS", func(i *net.Interface) error { - file := ctrld.SavedStaticDnsSettingsFilePath(i) - if _, err := os.Stat(file); err == nil { - if err := restoreDNS(i); err != nil { - p.Error().Err(err).Msgf("Could not restore static dns on interface %s", i.Name) - } else { - p.Debug().Msgf("Restored static dns on interface %s successfully", i.Name) - } - } - return nil - }) + restoreSavedStaticDNS("", false) }) close(waitCh) @@ -886,7 +875,7 @@ func processLogAndCacheFlags() { // netInterface returns the network interface by name func netInterface(ifaceName string) (*net.Interface, error) { - if ifaceName == "auto" { + if ifaceName == autoIface { ifaceName = defaultIfaceName() } var iface *net.Interface @@ -1142,29 +1131,33 @@ func uninstall(p *prog, s service.Service) { if doTasks(tasks) { // restore static DNS settings or DHCP p.resetDNS(false, true) - - // Iterate over all physical interfaces and restore DNS if a saved static config exists. - withEachPhysicalInterfaces(p.runningIface, "restore static DNS", func(i *net.Interface) error { - file := ctrld.SavedStaticDnsSettingsFilePath(i) - if _, err := os.Stat(file); err == nil { - if err := restoreDNS(i); err != nil { - mainLog.Load().Error().Err(err).Msgf("Could not restore static dns on interface %s", i.Name) - } else { - mainLog.Load().Debug().Msgf("Restored static dns on interface %s successfully", i.Name) - err = os.Remove(file) - if err != nil { - mainLog.Load().Debug().Err(err).Msgf("Could not remove saved static dns file for interface %s", i.Name) - } - } - } - return nil - }) + restoreSavedStaticDNS(p.runningIface, true) mainLog.Load().Notice().Msg("Service uninstalled") return } } +// restoreSavedStaticDNS restores DNS from saved static config files on physical interfaces. +func restoreSavedStaticDNS(excludeIfaceName string, removeSaved bool) { + withEachPhysicalInterfaces(excludeIfaceName, "restore static DNS", func(i *net.Interface) error { + file := ctrld.SavedStaticDnsSettingsFilePath(i) + if _, err := os.Stat(file); err == nil { + if err := restoreDNS(i); err != nil { + mainLog.Load().Error().Err(err).Msgf("Could not restore static DNS on interface %s", i.Name) + } else { + mainLog.Load().Debug().Msgf("Restored static DNS on interface %s successfully", i.Name) + if removeSaved { + if err := os.Remove(file); err != nil { + mainLog.Load().Debug().Err(err).Msgf("Could not remove saved static DNS file for interface %s", i.Name) + } + } + } + } + return nil + }) +} + func validateConfig(cfg *ctrld.Config) error { mainLog.Load().Debug().Msg("Validating configuration") @@ -2052,6 +2045,23 @@ func doValidateCdRemoteConfig(cdUID string, fatal bool) error { return nil } +// ensureRunningIfaceForInvalidUninstall populates p.runningIface before the +// invalid-device self-uninstall resets DNS. This path can run early during +// service startup (e.g. right after a reboot) before the running interface is +// otherwise known. resetDNS, via resetDNSForRunningIface, silently skips DNS +// restoration when p.runningIface is empty, which would leave the OS pointed at +// ctrld's local listener after the service is removed. See issue-556. +func ensureRunningIfaceForInvalidUninstall(p *prog, s service.Service) { + if iface == "" { + iface = autoIface + } + p.preRun() + if ir := runningIface(s); ir != nil { + p.runningIface = ir.Name + p.requiredMultiNICsConfig = ir.All + } +} + // uninstallInvalidCdUID performs self-uninstallation because the ControlD device does not exist. func uninstallInvalidCdUID(p *prog, logger *ctrld.Logger, doStop bool) bool { svcCmd := NewServiceCommand() @@ -2060,8 +2070,13 @@ func uninstallInvalidCdUID(p *prog, logger *ctrld.Logger, doStop bool) bool { logger.Warn().Err(err).Msg("Failed to create new service") return false } + ensureRunningIfaceForInvalidUninstall(p, s) // restore static DNS settings or DHCP p.resetDNS(false, true) + // The invalid-device path may run early during service startup before runningIface + // is known. Restore every saved static DNS file so uninstalling does not leave the + // OS pointed at ctrld's local listener after the service is removed. + restoreSavedStaticDNS("", true) tasks := []task{{s.Uninstall, true, "Uninstall"}} if doTasks(tasks) { diff --git a/cmd/cli/commands_service.go b/cmd/cli/commands_service.go index 9153119..fb5c661 100644 --- a/cmd/cli/commands_service.go +++ b/cmd/cli/commands_service.go @@ -305,7 +305,7 @@ func onlyInterceptFlags(args []string) bool { } else { return false } - case arg == "--iface=auto" || arg == "--iface" || arg == "auto": + case arg == "--iface="+autoIface || arg == "--iface" || arg == autoIface: // Auto-added by startCmdAlias or its value; safe to ignore. continue default: diff --git a/cmd/cli/commands_service_restart.go b/cmd/cli/commands_service_restart.go index 02e5a69..dac1550 100644 --- a/cmd/cli/commands_service_restart.go +++ b/cmd/cli/commands_service_restart.go @@ -32,7 +32,7 @@ func (sc *ServiceCommand) Restart(cmd *cobra.Command, args []string) error { p.cfg = &cfg if iface == "" { - iface = "auto" + iface = autoIface } p.preRun() if ir := runningIface(s); ir != nil { diff --git a/cmd/cli/commands_service_start.go b/cmd/cli/commands_service_start.go index 12f25b7..436cc6f 100644 --- a/cmd/cli/commands_service_start.go +++ b/cmd/cli/commands_service_start.go @@ -124,7 +124,7 @@ func (sc *ServiceCommand) Start(cmd *cobra.Command, args []string) error { reportSetDnsOk := func(sockDir string) { if cc := newSocketControlClient(ctx, s, sockDir); cc != nil { if resp, _ := cc.post(ifacePath, nil); resp != nil && resp.StatusCode == http.StatusOK { - if iface == "auto" { + if iface == autoIface { iface = defaultIfaceName() } res := &ifaceResponse{} @@ -456,7 +456,7 @@ NOTE: running "ctrld start" without any arguments will start already installed c return startCmd.RunE(cmd, args) }, } - startCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", "auto", `Update DNS setting for iface, "auto" means the default interface gateway`) + startCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", autoIface, `Update DNS setting for iface, "auto" means the default interface gateway`) startCmdAlias.Flags().AddFlagSet(startCmd.Flags()) return startCmd, startCmdAlias diff --git a/cmd/cli/commands_service_stop.go b/cmd/cli/commands_service_stop.go index 0f47e46..c388fcd 100644 --- a/cmd/cli/commands_service_stop.go +++ b/cmd/cli/commands_service_stop.go @@ -24,7 +24,7 @@ func (sc *ServiceCommand) Stop(cmd *cobra.Command, args []string) error { p.cfg = &cfg if iface == "" { - iface = "auto" + iface = autoIface } p.preRun() if ir := runningIface(s); ir != nil { diff --git a/cmd/cli/commands_service_uninstall.go b/cmd/cli/commands_service_uninstall.go index 78a3d5e..c3c1941 100644 --- a/cmd/cli/commands_service_uninstall.go +++ b/cmd/cli/commands_service_uninstall.go @@ -26,7 +26,7 @@ func (sc *ServiceCommand) Uninstall(cmd *cobra.Command, args []string) error { p.cfg = &cfg if iface == "" { - iface = "auto" + iface = autoIface } p.preRun() if ir := runningIface(s); ir != nil { diff --git a/cmd/cli/commands_upgrade.go b/cmd/cli/commands_upgrade.go index a6ab304..48f5ce1 100644 --- a/cmd/cli/commands_upgrade.go +++ b/cmd/cli/commands_upgrade.go @@ -55,7 +55,7 @@ func (uc *UpgradeCommand) Upgrade(cmd *cobra.Command, args []string) error { } if iface == "" { - iface = "auto" + iface = autoIface } p.preRun() if ir := runningIface(s); ir != nil { diff --git a/cmd/cli/main.go b/cmd/cli/main.go index 01c2c05..e51052c 100644 --- a/cmd/cli/main.go +++ b/cmd/cli/main.go @@ -64,6 +64,9 @@ const ( cdOrgFlagName = "cd-org" customHostnameFlagName = "custom-hostname" nextdnsFlagName = "nextdns" + + // autoIface is the sentinel --iface value meaning "use the default gateway interface". + autoIface = "auto" ) // init initializes the default logger before any CLI commands are executed diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index a13ab1e..dcad439 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -384,7 +384,7 @@ func preserveBoundListeners(newListeners, curListeners map[string]*ctrld.Listene } func (p *prog) preRun() { - if iface == "auto" { + if iface == autoIface { iface = defaultIfaceName() p.requiredMultiNICsConfig = requiredMultiNICsConfig() } diff --git a/cmd/cli/uninstall_invalid_test.go b/cmd/cli/uninstall_invalid_test.go new file mode 100644 index 0000000..664e97b --- /dev/null +++ b/cmd/cli/uninstall_invalid_test.go @@ -0,0 +1,62 @@ +package cli + +import "testing" + +// Test_ensureRunningIfaceForInvalidUninstall is a regression test for issue-556: +// after a reboot, the invalid-device self-uninstall path could run before the +// running interface was known. Because resetDNS (via resetDNSForRunningIface) +// silently skips DNS restoration when p.runningIface is empty, the OS was left +// pointed at ctrld's local listener with no internet after the service was +// removed. ensureRunningIfaceForInvalidUninstall must populate p.runningIface +// before resetDNS runs. +func Test_ensureRunningIfaceForInvalidUninstall(t *testing.T) { + // preRun mutates the package-level iface global; restore it after the test. + origIface := iface + t.Cleanup(func() { iface = origIface }) + + // newService needs the package service config; it is safe to build here + // because ensureRunningIfaceForInvalidUninstall only queries the (absent) + // control socket via runningIface, which returns nil when ctrld is not + // running, and performs no DNS or service mutation. + sc := NewServiceCommand() + s, err := sc.newService(&prog{}, sc.createServiceConfig()) + if err != nil { + t.Fatalf("newService: %v", err) + } + + t.Run("iface flag already resolved but not yet copied", func(t *testing.T) { + iface = "eth-test" + p := &prog{} + + // Precondition mirrors the buggy post-reboot state: an empty running + // interface would make resetDNS skip restoration entirely. + if p.runningIface != "" { + t.Fatalf("precondition: runningIface = %q, want empty", p.runningIface) + } + + ensureRunningIfaceForInvalidUninstall(p, s) + + if p.runningIface == "" { + t.Fatal("runningIface still empty after prepare: resetDNS would skip DNS " + + "restoration and leave the OS pointed at ctrld's local listener") + } + if p.runningIface != "eth-test" { + t.Fatalf("runningIface = %q, want the resolved iface %q", p.runningIface, "eth-test") + } + }) + + t.Run("iface unset falls back to auto-detected interface", func(t *testing.T) { + iface = "" + p := &prog{} + + ensureRunningIfaceForInvalidUninstall(p, s) + + // With iface unset the prep resolves "auto" to the default interface + // (defaultIfaceName never returns empty on the supported platforms), so + // resetDNS has a concrete interface to restore. + if p.runningIface == "" { + t.Fatal("runningIface still empty after prepare with iface unset: " + + "resetDNS would skip DNS restoration") + } + }) +}