From b79098658a3edac2efed9725432d350e7c32cb8d Mon Sep 17 00:00:00 2001 From: Dev Scribe Date: Sat, 4 Jul 2026 15:56:26 -0400 Subject: [PATCH] fix: restore DNS during invalid-device uninstall --- cmd/cli/cli.go | 75 ++++++++++++++++++------------- cmd/cli/commands.go | 16 +++---- cmd/cli/main.go | 3 ++ cmd/cli/prog.go | 2 +- cmd/cli/uninstall_invalid_test.go | 61 +++++++++++++++++++++++++ 5 files changed, 118 insertions(+), 39 deletions(-) create mode 100644 cmd/cli/uninstall_invalid_test.go diff --git a/cmd/cli/cli.go b/cmd/cli/cli.go index b75856f..4428d10 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 := 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) - } - } - return nil - }) + restoreSavedStaticDNS("", false) }) close(waitCh) @@ -826,7 +815,7 @@ func processLogAndCacheFlags(v *viper.Viper, cfg *ctrld.Config) { } func netInterface(ifaceName string) (*net.Interface, error) { - if ifaceName == "auto" { + if ifaceName == autoIface { ifaceName = defaultIfaceName() } var iface *net.Interface @@ -1112,23 +1101,7 @@ func uninstall(p *prog, s service.Service) { } // 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 := 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) if router.Name() != "" { mainLog.Load().Debug().Msg("Router cleanup") @@ -1141,6 +1114,26 @@ func uninstall(p *prog, s service.Service) { } } +// 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 := 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 { if err := ctrld.ValidateConfig(validator.New(), cfg); err != nil { var ve validator.ValidationErrors @@ -2069,6 +2062,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 zerolog.Logger, doStop bool) bool { s, err := newService(p, svcConfig) @@ -2076,8 +2086,13 @@ func uninstallInvalidCdUID(p *prog, logger zerolog.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.go b/cmd/cli/commands.go index a03745f..ec7b47c 100644 --- a/cmd/cli/commands.go +++ b/cmd/cli/commands.go @@ -399,7 +399,7 @@ NOTE: running "ctrld start" without any arguments will start already installed c 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{} @@ -748,7 +748,7 @@ NOTE: running "ctrld start" without any arguments will start already installed c startCmd.Run(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()) rootCmd.AddCommand(startCmdAlias) @@ -833,7 +833,7 @@ func initStopCmd() *cobra.Command { stopCmd.Run(cmd, args) }, } - stopCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", "auto", `Reset DNS setting for iface, "auto" means the default interface gateway`) + stopCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", autoIface, `Reset DNS setting for iface, "auto" means the default interface gateway`) stopCmdAlias.Flags().AddFlagSet(stopCmd.Flags()) rootCmd.AddCommand(stopCmdAlias) @@ -865,7 +865,7 @@ func initRestartCmd() *cobra.Command { return } if iface == "" { - iface = "auto" + iface = autoIface } p.preRun() if ir := runningIface(s); ir != nil { @@ -1111,7 +1111,7 @@ NOTE: Uninstalling will set DNS to values provided by DHCP.`, return } if iface == "" { - iface = "auto" + iface = autoIface } p.preRun() if ir := runningIface(s); ir != nil { @@ -1207,7 +1207,7 @@ NOTE: Uninstalling will set DNS to values provided by DHCP.`, uninstallCmd.Run(cmd, args) }, } - uninstallCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", "auto", `Reset DNS setting for iface, "auto" means the default interface gateway`) + uninstallCmdAlias.Flags().StringVarP(&ifaceStartStop, "iface", "", autoIface, `Reset DNS setting for iface, "auto" means the default interface gateway`) uninstallCmdAlias.Flags().AddFlagSet(uninstallCmd.Flags()) rootCmd.AddCommand(uninstallCmdAlias) @@ -1400,7 +1400,7 @@ func initUpgradeCmd() *cobra.Command { return } if iface == "" { - iface = "auto" + iface = autoIface } p.preRun() if ir := runningIface(s); ir != nil { @@ -1595,7 +1595,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/main.go b/cmd/cli/main.go index 972c308..762777d 100644 --- a/cmd/cli/main.go +++ b/cmd/cli/main.go @@ -56,6 +56,9 @@ const ( cdOrgFlagName = "cd-org" customHostnameFlagName = "custom-hostname" nextdnsFlagName = "nextdns" + + // autoIface is the sentinel --iface value meaning "use the default gateway interface". + autoIface = "auto" ) func init() { diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index 27302f6..6272934 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -394,7 +394,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..3ba3657 --- /dev/null +++ b/cmd/cli/uninstall_invalid_test.go @@ -0,0 +1,61 @@ +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. + s, err := newService(&prog{}, svcConfig) + 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") + } + }) +}