From db760d0bacd351a77f4f8940467bb4ad1b00240b Mon Sep 17 00:00:00 2001 From: James Tucker Date: Tue, 2 Apr 2024 19:52:19 -0700 Subject: [PATCH] cmd/tailscaled: move cleanup to an implicit action during startup This removes a potentially increased boot delay for certain boot topologies where they block on ExecStartPre that may have socket activation dependencies on other system services (such as systemd-resolved and NetworkManager). Also rename cleanup to clean up in affected/immediately nearby places per code review commentary. Fixes #11599 Signed-off-by: James Tucker --- cmd/tailscaled/tailscaled.go | 22 +++++++++++++--------- cmd/tailscaled/tailscaled.service | 1 - net/dns/manager.go | 4 ++-- util/linuxfw/iptables_runner.go | 4 ++-- wgengine/router/router.go | 6 +++--- wgengine/router/router_darwin.go | 2 +- wgengine/router/router_default.go | 2 +- wgengine/router/router_freebsd.go | 2 +- wgengine/router/router_linux.go | 10 +++++----- wgengine/router/router_openbsd.go | 4 ++-- wgengine/router/router_windows.go | 2 +- 11 files changed, 31 insertions(+), 28 deletions(-) diff --git a/cmd/tailscaled/tailscaled.go b/cmd/tailscaled/tailscaled.go index b78827fef..f93538ad5 100644 --- a/cmd/tailscaled/tailscaled.go +++ b/cmd/tailscaled/tailscaled.go @@ -116,7 +116,7 @@ var args struct { // or comma-separated list thereof. tunname string - cleanup bool + cleanUp bool confFile string debug string port uint16 @@ -156,7 +156,7 @@ func main() { printVersion := false flag.IntVar(&args.verbose, "verbose", 0, "log verbosity level; 0 is default, 1 or higher are increasingly verbose") - flag.BoolVar(&args.cleanup, "cleanup", false, "clean up system state and exit") + flag.BoolVar(&args.cleanUp, "cleanup", false, "clean up system state and exit") flag.StringVar(&args.debug, "debug", "", "listen address ([ip]:port) of optional debug server") flag.StringVar(&args.socksAddr, "socks5-server", "", `optional [ip]:port to run a SOCK5 server (e.g. "localhost:1080")`) flag.StringVar(&args.httpProxyAddr, "outbound-http-proxy-listen", "", `optional [ip]:port to run an outbound HTTP proxy (e.g. "localhost:8080")`) @@ -207,7 +207,7 @@ func main() { os.Exit(0) } - if runtime.GOOS == "darwin" && os.Getuid() != 0 && !strings.Contains(args.tunname, "userspace-networking") && !args.cleanup { + if runtime.GOOS == "darwin" && os.Getuid() != 0 && !strings.Contains(args.tunname, "userspace-networking") && !args.cleanUp { log.SetFlags(0) log.Fatalf("tailscaled requires root; use sudo tailscaled (or use --tun=userspace-networking)") } @@ -387,12 +387,16 @@ func run() (err error) { } logf = logger.RateLimitedFn(logf, 5*time.Second, 5, 100) - if args.cleanup { - if envknob.Bool("TS_PLEASE_PANIC") { - panic("TS_PLEASE_PANIC asked us to panic") - } - dns.Cleanup(logf, args.tunname) - router.Cleanup(logf, args.tunname) + if envknob.Bool("TS_PLEASE_PANIC") { + panic("TS_PLEASE_PANIC asked us to panic") + } + // Always clean up, even if we're going to run the server. This covers cases + // such as when a system was rebooted without shutting down, or tailscaled + // crashed, and would for example restore system DNS configuration. + dns.CleanUp(logf, args.tunname) + router.CleanUp(logf, args.tunname) + // If the cleanUp flag was passed, then exit. + if args.cleanUp { return nil } diff --git a/cmd/tailscaled/tailscaled.service b/cmd/tailscaled/tailscaled.service index d5a65a42e..719a3c0c9 100644 --- a/cmd/tailscaled/tailscaled.service +++ b/cmd/tailscaled/tailscaled.service @@ -6,7 +6,6 @@ After=network-pre.target NetworkManager.service systemd-resolved.service [Service] EnvironmentFile=/etc/default/tailscaled -ExecStartPre=/usr/sbin/tailscaled --cleanup ExecStart=/usr/sbin/tailscaled --state=/var/lib/tailscale/tailscaled.state --socket=/run/tailscale/tailscaled.sock --port=${PORT} $FLAGS ExecStopPost=/usr/sbin/tailscaled --cleanup diff --git a/net/dns/manager.go b/net/dns/manager.go index 69185909c..32b372b4f 100644 --- a/net/dns/manager.go +++ b/net/dns/manager.go @@ -449,10 +449,10 @@ func (m *Manager) FlushCaches() error { return flushCaches() } -// Cleanup restores the system DNS configuration to its original state +// CleanUp restores the system DNS configuration to its original state // in case the Tailscale daemon terminated without closing the router. // No other state needs to be instantiated before this runs. -func Cleanup(logf logger.Logf, interfaceName string) { +func CleanUp(logf logger.Logf, interfaceName string) { oscfg, err := NewOSConfigurator(logf, interfaceName) if err != nil { logf("creating dns cleanup: %v", err) diff --git a/util/linuxfw/iptables_runner.go b/util/linuxfw/iptables_runner.go index f5996346d..83c069af4 100644 --- a/util/linuxfw/iptables_runner.go +++ b/util/linuxfw/iptables_runner.go @@ -561,9 +561,9 @@ func (i *iptablesRunner) DelMagicsockPortRule(port uint16, network string) error return nil } -// IPTablesCleanup removes all Tailscale added iptables rules. +// IPTablesCleanUp removes all Tailscale added iptables rules. // Any errors that occur are logged to the provided logf. -func IPTablesCleanup(logf logger.Logf) { +func IPTablesCleanUp(logf logger.Logf) { err := clearRules(iptables.ProtocolIPv4, logf) if err != nil { logf("linuxfw: clear iptables: %v", err) diff --git a/wgengine/router/router.go b/wgengine/router/router.go index da0b39739..485d058f3 100644 --- a/wgengine/router/router.go +++ b/wgengine/router/router.go @@ -49,11 +49,11 @@ func New(logf logger.Logf, tundev tun.Device, netMon *netmon.Monitor) (Router, e return newUserspaceRouter(logf, tundev, netMon) } -// Cleanup restores the system network configuration to its original state +// CleanUp restores the system network configuration to its original state // in case the Tailscale daemon terminated without closing the router. // No other state needs to be instantiated before this runs. -func Cleanup(logf logger.Logf, interfaceName string) { - cleanup(logf, interfaceName) +func CleanUp(logf logger.Logf, interfaceName string) { + cleanUp(logf, interfaceName) } // Config is the subset of Tailscale configuration that is relevant to diff --git a/wgengine/router/router_darwin.go b/wgengine/router/router_darwin.go index 38fc78f82..9c3cdac29 100644 --- a/wgengine/router/router_darwin.go +++ b/wgengine/router/router_darwin.go @@ -13,6 +13,6 @@ func newUserspaceRouter(logf logger.Logf, tundev tun.Device, netMon *netmon.Moni return newUserspaceBSDRouter(logf, tundev, netMon) } -func cleanup(logger.Logf, string) { +func cleanUp(logger.Logf, string) { // Nothing to do. } diff --git a/wgengine/router/router_default.go b/wgengine/router/router_default.go index 9fee5d8f2..61f6eddae 100644 --- a/wgengine/router/router_default.go +++ b/wgengine/router/router_default.go @@ -18,6 +18,6 @@ func newUserspaceRouter(logf logger.Logf, tunDev tun.Device, netMon *netmon.Moni return nil, fmt.Errorf("unsupported OS %q", runtime.GOOS) } -func cleanup(logf logger.Logf, interfaceName string) { +func cleanUp(logf logger.Logf, interfaceName string) { // Nothing to do here. } diff --git a/wgengine/router/router_freebsd.go b/wgengine/router/router_freebsd.go index 84dccbca0..41fcd2606 100644 --- a/wgengine/router/router_freebsd.go +++ b/wgengine/router/router_freebsd.go @@ -18,7 +18,7 @@ func newUserspaceRouter(logf logger.Logf, tundev tun.Device, netMon *netmon.Moni return newUserspaceBSDRouter(logf, tundev, netMon) } -func cleanup(logf logger.Logf, interfaceName string) { +func cleanUp(logf logger.Logf, interfaceName string) { // If the interface was left behind, ifconfig down will not remove it. // In fact, this will leave a system in a tainted state where starting tailscaled // will result in "interface tailscale0 already exists" diff --git a/wgengine/router/router_linux.go b/wgengine/router/router_linux.go index 53b24a2b6..36e95d297 100644 --- a/wgengine/router/router_linux.go +++ b/wgengine/router/router_linux.go @@ -1396,12 +1396,12 @@ func normalizeCIDR(cidr netip.Prefix) string { return cidr.Masked().String() } -// cleanup removes all the rules and routes that were added by the linux router. -// The function calls cleanup for both iptables and nftables since which ever -// netfilter runner is used, the cleanup function for the other one doesn't do anything. -func cleanup(logf logger.Logf, interfaceName string) { +// cleanUp removes all the rules and routes that were added by the linux router. +// The function calls cleanUp for both iptables and nftables since which ever +// netfilter runner is used, the cleanUp function for the other one doesn't do anything. +func cleanUp(logf logger.Logf, interfaceName string) { if interfaceName != "userspace-networking" { - linuxfw.IPTablesCleanup(logf) + linuxfw.IPTablesCleanUp(logf) linuxfw.NfTablesCleanUp(logf) } } diff --git a/wgengine/router/router_openbsd.go b/wgengine/router/router_openbsd.go index b71a01cc7..7c0316cf1 100644 --- a/wgengine/router/router_openbsd.go +++ b/wgengine/router/router_openbsd.go @@ -236,11 +236,11 @@ func (r *openbsdRouter) UpdateMagicsockPort(_ uint16, _ string) error { } func (r *openbsdRouter) Close() error { - cleanup(r.logf, r.tunname) + cleanUp(r.logf, r.tunname) return nil } -func cleanup(logf logger.Logf, interfaceName string) { +func cleanUp(logf logger.Logf, interfaceName string) { out, err := cmd("ifconfig", interfaceName, "down").CombinedOutput() if err != nil { logf("ifconfig down: %v\n%s", err, out) diff --git a/wgengine/router/router_windows.go b/wgengine/router/router_windows.go index 73d219bce..4d685a9a9 100644 --- a/wgengine/router/router_windows.go +++ b/wgengine/router/router_windows.go @@ -120,7 +120,7 @@ func (r *winRouter) Close() error { return nil } -func cleanup(logf logger.Logf, interfaceName string) { +func cleanUp(logf logger.Logf, interfaceName string) { // Nothing to do here. }