health: use Warnables for system DNS configuration errorsangott/dns-warnables

Updates tailscale/tailscale#4136

Begins to use Warnables to replace the deprecated DNS health subsystem.

Signed-off-by: Andrea Gottardo <andrea@gottardo.me>

6 files changed, 35 insertions, 48 deletions

diff --git a/health/health.go b/health/health.go
index c1740b1f1..a8a7c2f7f 100644
--- a/health/health.go
+++ b/health/health.go
@@ -110,15 +110,6 @@ const (
 	// SysRouter is the name of the wgengine/router subsystem.
 	SysRouter = Subsystem("router")
 
-	// SysDNS is the name of the net/dns subsystem.
-	SysDNS = Subsystem("dns")
-
-	// SysDNSOS is the name of the net/dns OSConfigurator subsystem.
-	SysDNSOS = Subsystem("dns-os")
-
-	// SysDNSManager is the name of the net/dns manager subsystem.
-	SysDNSManager = Subsystem("dns-manager")
-
 	// SysTKA is the name of the tailnet key authority subsystem.
 	SysTKA = Subsystem("tailnet-lock")
 )
@@ -126,7 +117,7 @@ const (
 var subsystemsWarnables = map[Subsystem]*Warnable{}
 
 func init() {
-	for _, s := range []Subsystem{SysRouter, SysDNS, SysDNSOS, SysDNSManager, SysTKA} {
+	for _, s := range []Subsystem{SysRouter, SysTKA} {
 		w := Register(&Warnable{
 			Code:     WarnableCode(s),
 			Severity: SeverityMedium,
@@ -406,32 +397,6 @@ func (t *Tracker) SetRouterHealth(err error) { t.setErr(SysRouter, err) }
 // Deprecated: Warnables should be preferred over Subsystem errors.
 func (t *Tracker) RouterHealth() error { return t.get(SysRouter) }
 
-// SetDNSHealth sets the state of the net/dns.Manager
-//
-// Deprecated: Warnables should be preferred over Subsystem errors.
-func (t *Tracker) SetDNSHealth(err error) { t.setErr(SysDNS, err) }
-
-// DNSHealth returns the net/dns.Manager error state.
-//
-// Deprecated: Warnables should be preferred over Subsystem errors.
-func (t *Tracker) DNSHealth() error { return t.get(SysDNS) }
-
-// SetDNSOSHealth sets the state of the net/dns.OSConfigurator
-//
-// Deprecated: Warnables should be preferred over Subsystem errors.
-func (t *Tracker) SetDNSOSHealth(err error) { t.setErr(SysDNSOS, err) }
-
-// SetDNSManagerHealth sets the state of the Linux net/dns manager's
-// discovery of the /etc/resolv.conf situation.
-//
-// Deprecated: Warnables should be preferred over Subsystem errors.
-func (t *Tracker) SetDNSManagerHealth(err error) { t.setErr(SysDNSManager, err) }
-
-// DNSOSHealth returns the net/dns.OSConfigurator error state.
-//
-// Deprecated: Warnables should be preferred over Subsystem errors.
-func (t *Tracker) DNSOSHealth() error { return t.get(SysDNSOS) }
-
 // SetTKAHealth sets the health of the tailnet key authority.
 //
 // Deprecated: Warnables should be preferred over Subsystem errors.
diff --git a/net/dns/manager.go b/net/dns/manager.go
index 88fe94fac..7040be7e9 100644
--- a/net/dns/manager.go
+++ b/net/dns/manager.go
@@ -147,20 +147,28 @@ func (m *Manager) setLocked(cfg Config) error {
 		ocfg.WriteToBufioWriter(w)
 	}))
 
-	if err := m.resolver.SetConfig(rcfg); err != nil {
-		return err
-	}
+	m.resolver.SetConfig(rcfg)
+
 	if err := m.os.SetDNS(ocfg); err != nil {
-		m.health.SetDNSOSHealth(err)
+		m.health.SetUnhealthy(OSConfigWarnable, health.Args{health.ArgError: err.Error()})
 		return err
 	}
 
-	m.health.SetDNSOSHealth(nil)
+	m.health.SetHealthy(OSConfigWarnable)
 	m.config = &cfg
 
 	return nil
 }
 
+var OSConfigWarnable = health.Register(&health.Warnable{
+	Code:     "dns-failed-osconfig",
+	Severity: health.SeverityMedium,
+	Title:    "System DNS configuration failed",
+	Text: func(args health.Args) string {
+		return "Could not configure this device to use the Tailscale DNS resolver. Error: " + args[health.ArgError]
+	},
+})
+
 // compileHostEntries creates a list of single-label resolutions possible
 // from the configured hosts and search domains.
 // The entries are compiled in the order of the search domains, then the hosts.
@@ -315,9 +323,10 @@ func (m *Manager) compileConfig(cfg Config) (rcfg resolver.Config, ocfg OSConfig
 			// This is currently (2022-10-13) expected on certain iOS and macOS
 			// builds.
 		} else {
-			m.health.SetDNSOSHealth(err)
+			m.health.SetUnhealthy(OSConfigWarnable, health.Args{health.ArgError: err.Error()})
 			return resolver.Config{}, OSConfig{}, err
 		}
+		m.health.SetHealthy(OSConfigWarnable)
 	}
 
 	if baseCfg == nil {
diff --git a/net/dns/manager_linux.go b/net/dns/manager_linux.go
index 3ba3022b6..dcb178179 100644
--- a/net/dns/manager_linux.go
+++ b/net/dns/manager_linux.go
@@ -275,7 +275,7 @@ func dnsMode(logf logger.Logf, health *health.Tracker, env newOSConfigEnv) (ret
 			return "direct", nil
 		}
 
-		health.SetDNSManagerHealth(errors.New("systemd-resolved and NetworkManager are wired together incorrectly; MagicDNS will probably not work. For more info, see https://tailscale.com/s/resolved-nm"))
+		health.SetUnhealthy(resolvedNetworkManagerConflictWarnable, nil)
 		dbg("nm-safe", "no")
 		return "systemd-resolved", nil
 	default:
@@ -284,6 +284,16 @@ func dnsMode(logf logger.Logf, health *health.Tracker, env newOSConfigEnv) (ret
 	}
 }
 
+// resolvedNetworkManagerConflictWarnable reports whether the system is in a
+// state where NetworkManager and systemd-resolved are in conflict, and we should
+// warn the user about it.
+var resolvedNetworkManagerConflictWarnable = health.Register(&health.Warnable{
+	Code:     "resolved-nm-conflict",
+	Title:    "NetworkManager and systemd-resolved conflict",
+	Text:     health.StaticMessage("systemd-resolved and NetworkManager are wired together incorrectly; MagicDNS will probably not work. For more info, see https://tailscale.com/s/resolved-nm"),
+	Severity: health.SeverityMedium,
+})
+
 func nmVersionBetween(first, last string) (bool, error) {
 	conn, err := dbus.SystemBus()
 	if err != nil {
diff --git a/net/dns/resolved.go b/net/dns/resolved.go
index d82d3fc31..fe7345fba 100644
--- a/net/dns/resolved.go
+++ b/net/dns/resolved.go
@@ -165,7 +165,7 @@ func (m *resolvedManager) run(ctx context.Context) {
 
 		// Reset backoff and SetNSOSHealth after successful on reconnect.
 		bo.BackOff(ctx, nil)
-		m.health.SetDNSOSHealth(nil)
+		m.health.SetHealthy(OSConfigWarnable)
 		return nil
 	}
 
@@ -243,9 +243,11 @@ func (m *resolvedManager) run(ctx context.Context) {
 			// Set health while holding the lock, because this will
 			// graciously serialize the resync's health outcome with a
 			// concurrent SetDNS call.
-			m.health.SetDNSOSHealth(err)
 			if err != nil {
 				m.logf("failed to configure systemd-resolved: %v", err)
+				m.health.SetUnhealthy(OSConfigWarnable, health.Args{health.ArgError: err.Error()})
+			} else {
+				m.health.SetHealthy(OSConfigWarnable)
 			}
 		}
 	}
diff --git a/net/dns/resolver/tsdns.go b/net/dns/resolver/tsdns.go
index a140c7e93..6036cf99f 100644
--- a/net/dns/resolver/tsdns.go
+++ b/net/dns/resolver/tsdns.go
@@ -253,7 +253,7 @@ func (r *Resolver) SetMissingUpstreamRecovery(f func()) {
 
 func (r *Resolver) TestOnlySetHook(hook func(Config)) { r.saveConfigForTests = hook }
 
-func (r *Resolver) SetConfig(cfg Config) error {
+func (r *Resolver) SetConfig(cfg Config) {
 	if r.saveConfigForTests != nil {
 		r.saveConfigForTests(cfg)
 	}
@@ -273,7 +273,6 @@ func (r *Resolver) SetConfig(cfg Config) error {
 	r.localDomains = cfg.LocalDomains
 	r.hostToIP = cfg.Hosts
 	r.ipToHost = reverse
-	return nil
 }
 
 // Close shuts down the resolver and ensures poll goroutines have exited.
diff --git a/wgengine/userspace.go b/wgengine/userspace.go
index 6399476c8..50600f647 100644
--- a/wgengine/userspace.go
+++ b/wgengine/userspace.go
@@ -994,13 +994,15 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config,
 		// assigned address.
 		e.logf("wgengine: Reconfig: configuring DNS")
 		err = e.dns.Set(*dnsCfg)
-		e.health.SetDNSHealth(err)
 		if err != nil {
+			e.health.SetUnhealthy(dns.OSConfigWarnable, health.Args{health.ArgError: err.Error()})
 			return err
 		}
 		if err := e.reconfigureVPNIfNecessary(); err != nil {
+			e.health.SetUnhealthy(dns.OSConfigWarnable, health.Args{health.ArgError: err.Error()})
 			return err
 		}
+		e.health.SetHealthy(dns.OSConfigWarnable)
 	}
 
 	// Shutdown the network logger.