From bfde8079a0919269fd9f435ba0c6e1bfc35988c2 Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@tailscale.com>
Date: Mon, 27 Jan 2025 13:37:49 +0000
Subject: [PATCH] health: do Warnable dependency filtering in tailscaled

Previously we were depending on the GUI(s) to do it.
By doing it in tailscaled, GUIs can be simplified and be
guaranteed to render consistent results.

If warnable A depends on warnable B, if both A & B are unhealhy, only
B will be shown to the GUI as unhealthy. Once B clears up, only then
will A be presented as unhealthy.

Updates #14687

Change-Id: Id8566f2672d8d2d699740fa053d4e2a2c8009e83
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
---
 health/health.go      |  9 +++++++--
 health/health_test.go | 10 ++++++++--
 health/state.go       | 25 +++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/health/health.go b/health/health.go
index 079b3195c..fa608ea73 100644
--- a/health/health.go
+++ b/health/health.go
@@ -214,9 +214,11 @@ type Warnable struct {
 	// TODO(angott): turn this into a SeverityFunc, which allows the Warnable to change its severity based on
 	// the Args of the unhappy state, just like we do in the Text function.
 	Severity Severity
-	// DependsOn is a set of Warnables that this Warnable depends, on and need to be healthy
-	// before this Warnable can also be healthy again. The GUI can use this information to ignore
+	// DependsOn is a set of Warnables that this Warnable depends on and need to be healthy
+	// before this Warnable is relevant. The GUI can use this information to ignore
 	// this Warnable if one of its dependencies is unhealthy.
+	// That is, if any of these Warnables are unhealthy, then this Warnable is not relevant
+	// and should be considered healthy to bother the user about.
 	DependsOn []*Warnable
 
 	// MapDebugFlag is a MapRequest.DebugFlag that is sent to control when this Warnable is unhealthy
@@ -940,6 +942,9 @@ func (t *Tracker) stringsLocked() []string {
 			// Do not append invisible warnings.
 			continue
 		}
+		if t.isEffectivelyHealthyLocked(w) {
+			continue
+		}
 		if ws.Args == nil {
 			result = append(result, w.Text(Args{}))
 		} else {
diff --git a/health/health_test.go b/health/health_test.go
index ebdddc988..cc7b9d5aa 100644
--- a/health/health_test.go
+++ b/health/health_test.go
@@ -257,9 +257,15 @@ func TestCheckDependsOnAppearsInUnhealthyState(t *testing.T) {
 	}
 	ht.SetUnhealthy(w2, Args{ArgError: "w2 is also unhealthy now"})
 	us2, ok := ht.CurrentState().Warnings[w2.Code]
-	if !ok {
-		t.Fatalf("Expected an UnhealthyState for w2, got nothing")
+	if ok {
+		t.Fatalf("Saw w2 being unhealthy but it shouldn't be, as it depends on unhealthy w1")
 	}
+	ht.SetHealthy(w1)
+	us2, ok = ht.CurrentState().Warnings[w2.Code]
+	if !ok {
+		t.Fatalf("w2 wasn't unhealthy; want it to be unhealthy now that w1 is back healthy")
+	}
+
 	wantDependsOn = slices.Concat([]WarnableCode{w1.Code}, wantDependsOn)
 	if !reflect.DeepEqual(us2.DependsOn, wantDependsOn) {
 		t.Fatalf("Expected DependsOn = %v in the unhealthy state, got: %v", wantDependsOn, us2.DependsOn)
diff --git a/health/state.go b/health/state.go
index 17a646794..3bfa6f99b 100644
--- a/health/state.go
+++ b/health/state.go
@@ -90,6 +90,11 @@ func (t *Tracker) CurrentState() *State {
 			// Skip invisible Warnables.
 			continue
 		}
+		if t.isEffectivelyHealthyLocked(w) {
+			// Skip Warnables that are unhealthy if they have dependencies
+			// that are unhealthy.
+			continue
+		}
 		wm[w.Code] = *w.unhealthyState(ws)
 	}
 
@@ -97,3 +102,23 @@ func (t *Tracker) CurrentState() *State {
 		Warnings: wm,
 	}
 }
+
+// isEffectivelyHealthyLocked reports whether w is effectively healthy.
+// That means it's either actually healthy or it has a dependency that
+// that's unhealthy, so we should treat w as healthy to not spam users
+// with multiple warnings when only the root cause is relevant.
+func (t *Tracker) isEffectivelyHealthyLocked(w *Warnable) bool {
+	if _, ok := t.warnableVal[w]; !ok {
+		// Warnable not found in the tracker. So healthy.
+		return true
+	}
+	for _, d := range w.DependsOn {
+		if !t.isEffectivelyHealthyLocked(d) {
+			// If one of our deps is unhealthy, we're healthy.
+			return true
+		}
+	}
+	// If we have no unhealthy deps and had warnableVal set,
+	// we're unhealthy.
+	return false
+}