health: add an ETag to UnhealthyState for change detection

Updates tailscale/corp#30596

Signed-off-by: James Sanderson <jsanderson@tailscale.com>

control/controlclient/map_test.go

@@ -1361,7 +1361,7 @@ func TestNetmapHealthIntegration(t *testing.T) {
 		}
 	}
 
-	if d := cmp.Diff(want, got); d != "" {
+	if d := cmp.Diff(want, got, cmpopts.IgnoreFields(health.UnhealthyState{}, "ETag")); d != "" {
 		t.Fatalf("CurrentStatus().Warnings[\"control-health*\"] different than expected (-want +got)\n%s", d)
 	}
 }
@@ -1414,7 +1414,7 @@ func TestNetmapDisplayMessageIntegration(t *testing.T) {
 		},
 	}
 
-	if diff := cmp.Diff(want, state.Warnings); diff != "" {
+	if diff := cmp.Diff(want, state.Warnings, cmpopts.IgnoreFields(health.UnhealthyState{}, "ETag")); diff != "" {
 		t.Errorf("unexpected message contents (-want +got):\n%s", diff)
 	}
 }

health/health_test.go

@@ -13,8 +13,10 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"tailscale.com/tailcfg"
 	"tailscale.com/tstest"
+	"tailscale.com/tstime"
 	"tailscale.com/types/opt"
 	"tailscale.com/util/usermetric"
 	"tailscale.com/version"
@@ -517,7 +519,7 @@ func TestControlHealth(t *testing.T) {
 				delete(gotWarns, k)
 			}
 		}
-		if diff := cmp.Diff(wantWarns, gotWarns); diff != "" {
+		if diff := cmp.Diff(wantWarns, gotWarns, cmpopts.IgnoreFields(UnhealthyState{}, "ETag")); diff != "" {
 			t.Fatalf(`CurrentState().Warnings["control-health-*"] wrong (-want +got):\n%s`, diff)
 		}
 	})
@@ -664,3 +666,175 @@ func TestControlHealthIgnoredOutsideMapPoll(t *testing.T) {
 		t.Error("watcher got called, want it to not be called")
 	}
 }
+
+// TestCurrentStateETagControlHealth tests that the ETag on an [UnhealthyState]
+// created from Control health & returned by [Tracker.CurrentState] is different
+// when the details of the [tailcfg.DisplayMessage] are different.
+func TestCurrentStateETagControlHealth(t *testing.T) {
+	ht := Tracker{}
+	ht.SetIPNState("NeedsLogin", true)
+	ht.GotStreamedMapResponse()
+
+	msg := tailcfg.DisplayMessage{
+		Title:               "Test Warning",
+		Text:                "This is a test warning.",
+		Severity:            tailcfg.SeverityHigh,
+		ImpactsConnectivity: true,
+		PrimaryAction: &tailcfg.DisplayMessageAction{
+			URL:   "https://example.com/",
+			Label: "open",
+		},
+	}
+
+	type test struct {
+		name            string
+		change          func(tailcfg.DisplayMessage) tailcfg.DisplayMessage
+		wantChangedETag bool
+	}
+	tests := []test{
+		{
+			name:            "same_value",
+			change:          func(m tailcfg.DisplayMessage) tailcfg.DisplayMessage { return m },
+			wantChangedETag: false,
+		},
+		{
+			name: "different_severity",
+			change: func(m tailcfg.DisplayMessage) tailcfg.DisplayMessage {
+				m.Severity = tailcfg.SeverityLow
+				return m
+			},
+			wantChangedETag: true,
+		},
+		{
+			name: "different_title",
+			change: func(m tailcfg.DisplayMessage) tailcfg.DisplayMessage {
+				m.Title = "Different Title"
+				return m
+			},
+			wantChangedETag: true,
+		},
+		{
+			name: "different_text",
+			change: func(m tailcfg.DisplayMessage) tailcfg.DisplayMessage {
+				m.Text = "This is a different text."
+				return m
+			},
+			wantChangedETag: true,
+		},
+		{
+			name: "different_impacts_connectivity",
+			change: func(m tailcfg.DisplayMessage) tailcfg.DisplayMessage {
+				m.ImpactsConnectivity = false
+				return m
+			},
+			wantChangedETag: true,
+		},
+		{
+			name: "different_primary_action_label",
+			change: func(m tailcfg.DisplayMessage) tailcfg.DisplayMessage {
+				m.PrimaryAction.Label = "new_label"
+				return m
+			},
+			wantChangedETag: true,
+		},
+		{
+			name: "different_primary_action_url",
+			change: func(m tailcfg.DisplayMessage) tailcfg.DisplayMessage {
+				m.PrimaryAction.URL = "https://new.example.com/"
+				return m
+			},
+			wantChangedETag: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			ht.SetControlHealth(map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{
+				"test-message": msg,
+			})
+			state := ht.CurrentState().Warnings["control-health.test-message"]
+
+			newMsg := test.change(msg)
+			ht.SetControlHealth(map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{
+				"test-message": newMsg,
+			})
+			newState := ht.CurrentState().Warnings["control-health.test-message"]
+
+			if (state.ETag != newState.ETag) != test.wantChangedETag {
+				if test.wantChangedETag {
+					t.Errorf("got unchanged ETag, want changed (ETag was %q)", newState.ETag)
+				} else {
+					t.Errorf("got changed ETag, want unchanged")
+				}
+			}
+		})
+	}
+}
+
+// TestCurrentStateETagWarnable tests that the ETag on an [UnhealthyState]
+// created from a Warnable & returned by [Tracker.CurrentState] is different
+// when the details of the Warnable are different.
+func TestCurrentStateETagWarnable(t *testing.T) {
+	newTracker := func(clock tstime.Clock) *Tracker {
+		ht := &Tracker{
+			testClock: clock,
+		}
+		ht.SetIPNState("NeedsLogin", true)
+		ht.GotStreamedMapResponse()
+		return ht
+	}
+
+	t.Run("new_args", func(t *testing.T) {
+		ht := newTracker(nil)
+
+		ht.SetUnhealthy(testWarnable, Args{ArgError: "initial value"})
+		state := ht.CurrentState().Warnings[testWarnable.Code]
+
+		ht.SetUnhealthy(testWarnable, Args{ArgError: "new value"})
+		newState := ht.CurrentState().Warnings[testWarnable.Code]
+
+		if state.ETag == newState.ETag {
+			t.Errorf("got unchanged ETag, want changed (ETag was %q)", newState.ETag)
+		}
+	})
+
+	t.Run("new_broken_since", func(t *testing.T) {
+		clock1 := tstest.NewClock(tstest.ClockOpts{
+			Start: time.Unix(123, 0),
+		})
+		ht1 := newTracker(clock1)
+
+		ht1.SetUnhealthy(testWarnable, Args{})
+		state := ht1.CurrentState().Warnings[testWarnable.Code]
+
+		// Use a second tracker to get a different broken since time
+		clock2 := tstest.NewClock(tstest.ClockOpts{
+			Start: time.Unix(456, 0),
+		})
+		ht2 := newTracker(clock2)
+
+		ht2.SetUnhealthy(testWarnable, Args{})
+		newState := ht2.CurrentState().Warnings[testWarnable.Code]
+
+		if state.ETag == newState.ETag {
+			t.Errorf("got unchanged ETag, want changed (ETag was %q)", newState.ETag)
+		}
+	})
+
+	t.Run("no_change", func(t *testing.T) {
+		clock := tstest.NewClock(tstest.ClockOpts{})
+		ht1 := newTracker(clock)
+
+		ht1.SetUnhealthy(testWarnable, Args{})
+		state := ht1.CurrentState().Warnings[testWarnable.Code]
+
+		// Using a second tracker because SetUnhealthy with no changes is a no-op
+		ht2 := newTracker(clock)
+		ht2.SetUnhealthy(testWarnable, Args{})
+		newState := ht2.CurrentState().Warnings[testWarnable.Code]
+
+		if state.ETag != newState.ETag {
+			t.Errorf("got changed ETag, want unchanged")
+		}
+	})
+}

health/state.go

@@ -4,6 +4,9 @@
 package health
 
 import (
+	"crypto/sha256"
+	"encoding/hex"
+	"encoding/json"
 	"time"
 
 	"tailscale.com/tailcfg"
@@ -35,6 +38,36 @@ type UnhealthyState struct {
 	DependsOn           []WarnableCode        `json:",omitempty"`
 	ImpactsConnectivity bool                  `json:",omitempty"`
 	PrimaryAction       *UnhealthyStateAction `json:",omitempty"`
+
+	// ETag identifies a specific version of an UnhealthyState. If the contents
+	// of the other fields of two UnhealthyStates are the same, the ETags will
+	// be the same. If the contents differ, the ETags will also differ. The
+	// implementation is not defined and the value is opaque: it might be a
+	// hash, it might be a simple counter. Implementations should not rely on
+	// any specific implementation detail or format of the ETag string other
+	// than string (in)equality.
+	ETag string `json:",omitzero"`
+}
+
+// hash computes a deep hash of UnhealthyState which will be stable across
+// different runs of the same binary.
+func (u UnhealthyState) hash() []byte {
+	hasher := sha256.New()
+	enc := json.NewEncoder(hasher)
+
+	// hash.Hash.Write never returns an error, so this will only fail if u is
+	// not marshalable, in which case we have much bigger problems.
+	_ = enc.Encode(u)
+	return hasher.Sum(nil)
+}
+
+// withETag returns a copy of UnhealthyState with an ETag set. The ETag will be
+// the same for all UnhealthyState instances that are equal. If calculating the
+// ETag errors, it returns a copy of the UnhealthyState with an empty ETag.
+func (u UnhealthyState) withETag() UnhealthyState {
+	u.ETag = ""
+	u.ETag = hex.EncodeToString(u.hash())
+	return u
 }
 
 // UnhealthyStateAction represents an action (URL and link) to be presented to
@@ -107,7 +140,8 @@ func (t *Tracker) CurrentState() *State {
 			// that are unhealthy.
 			continue
 		}
-		wm[w.Code] = *w.unhealthyState(ws)
+		state := w.unhealthyState(ws)
+		wm[w.Code] = state.withETag()
 	}
 
 	for id, msg := range t.lastNotifiedControlMessages {
@@ -127,7 +161,7 @@ func (t *Tracker) CurrentState() *State {
 			}
 		}
 
-		wm[state.WarnableCode] = state
+		wm[state.WarnableCode] = state.withETag()
 	}
 
 	return &State{

ipn/ipnlocal/local_test.go

@@ -6807,7 +6807,7 @@ func TestDisplayMessagesURLFilter(t *testing.T) {
 		Severity:     health.SeverityHigh,
 	}
 
-	if diff := cmp.Diff(want, got); diff != "" {
+	if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(health.UnhealthyState{}, "ETag")); diff != "" {
 		t.Errorf("Unexpected message content (-want/+got):\n%s", diff)
 	}
 }
@@ -6879,7 +6879,7 @@ func TestDisplayMessageIPNBus(t *testing.T) {
 					}
 					got, ok := n.Health.Warnings[wantID]
 					if ok {
-						if diff := cmp.Diff(tt.wantWarning, got); diff != "" {
+						if diff := cmp.Diff(tt.wantWarning, got, cmpopts.IgnoreFields(health.UnhealthyState{}, "ETag")); diff != "" {
 							t.Errorf("unexpected warning details (-want/+got):\n%s", diff)
 							return true // we failed the test so tell the watcher we've seen what we need to to stop it waiting
 						}