From f8f53bb6d47526cd5819039d6fa52a050eabc22c Mon Sep 17 00:00:00 2001
From: Andrea Gottardo <andrea@tailscale.com>
Date: Mon, 21 Oct 2024 13:40:43 -0700
Subject: [PATCH] health: remove SysDNSOS, add two Warnables for read+set
 system DNS config (#13874)

---
 health/health.go    | 15 +--------------
 net/dns/manager.go  | 28 +++++++++++++++++++++++++---
 net/dns/resolved.go |  9 ++++++---
 3 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/health/health.go b/health/health.go
index 216535d17..16b41f075 100644
--- a/health/health.go
+++ b/health/health.go
@@ -128,9 +128,6 @@ const (
 	// 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")
 
@@ -141,7 +138,7 @@ const (
 var subsystemsWarnables = map[Subsystem]*Warnable{}
 
 func init() {
-	for _, s := range []Subsystem{SysRouter, SysDNS, SysDNSOS, SysDNSManager, SysTKA} {
+	for _, s := range []Subsystem{SysRouter, SysDNS, SysDNSManager, SysTKA} {
 		w := Register(&Warnable{
 			Code:     WarnableCode(s),
 			Severity: SeverityMedium,
@@ -510,22 +507,12 @@ func (t *Tracker) SetDNSHealth(err error) { t.setErr(SysDNS, err) }
 // 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 51a0fa12c..13cb2d84e 100644
--- a/net/dns/manager.go
+++ b/net/dns/manager.go
@@ -8,6 +8,7 @@ import (
 	"context"
 	"encoding/binary"
 	"errors"
+	"fmt"
 	"io"
 	"net"
 	"net/netip"
@@ -156,11 +157,11 @@ func (m *Manager) setLocked(cfg Config) error {
 		return err
 	}
 	if err := m.os.SetDNS(ocfg); err != nil {
-		m.health.SetDNSOSHealth(err)
+		m.health.SetUnhealthy(osConfigurationSetWarnable, health.Args{health.ArgError: err.Error()})
 		return err
 	}
 
-	m.health.SetDNSOSHealth(nil)
+	m.health.SetHealthy(osConfigurationSetWarnable)
 	m.config = &cfg
 
 	return nil
@@ -217,6 +218,26 @@ func compileHostEntries(cfg Config) (hosts []*HostEntry) {
 	return hosts
 }
 
+var osConfigurationReadWarnable = health.Register(&health.Warnable{
+	Code:  "dns-read-os-config-failed",
+	Title: "Failed to read system DNS configuration",
+	Text: func(args health.Args) string {
+		return fmt.Sprintf("Tailscale failed to fetch the DNS configuration of your device: %v", args[health.ArgError])
+	},
+	Severity:  health.SeverityLow,
+	DependsOn: []*health.Warnable{health.NetworkStatusWarnable},
+})
+
+var osConfigurationSetWarnable = health.Register(&health.Warnable{
+	Code:  "dns-set-os-config-failed",
+	Title: "Failed to set system DNS configuration",
+	Text: func(args health.Args) string {
+		return fmt.Sprintf("Tailscale failed to set the DNS configuration of your device: %v", args[health.ArgError])
+	},
+	Severity:  health.SeverityMedium,
+	DependsOn: []*health.Warnable{health.NetworkStatusWarnable},
+})
+
 // compileConfig converts cfg into a quad-100 resolver configuration
 // and an OS-level configuration.
 func (m *Manager) compileConfig(cfg Config) (rcfg resolver.Config, ocfg OSConfig, err error) {
@@ -320,9 +341,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(osConfigurationReadWarnable, health.Args{health.ArgError: err.Error()})
 			return resolver.Config{}, OSConfig{}, err
 		}
+		m.health.SetHealthy(osConfigurationReadWarnable)
 	}
 
 	if baseCfg == nil {
diff --git a/net/dns/resolved.go b/net/dns/resolved.go
index d82d3fc31..1a7c86041 100644
--- a/net/dns/resolved.go
+++ b/net/dns/resolved.go
@@ -163,9 +163,9 @@ func (m *resolvedManager) run(ctx context.Context) {
 		}
 		conn.Signal(signals)
 
-		// Reset backoff and SetNSOSHealth after successful on reconnect.
+		// Reset backoff and set osConfigurationSetWarnable to healthy after a successful reconnect.
 		bo.BackOff(ctx, nil)
-		m.health.SetDNSOSHealth(nil)
+		m.health.SetHealthy(osConfigurationSetWarnable)
 		return nil
 	}
 
@@ -243,9 +243,12 @@ 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(osConfigurationSetWarnable, health.Args{health.ArgError: err.Error()})
+			} else {
+				m.health.SetHealthy(osConfigurationSetWarnable)
 			}
 		}
 	}