diff --git a/health/health_test.go b/health/health_test.go index 53f012ecf..22068263b 100644 --- a/health/health_test.go +++ b/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,169 @@ 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) { + ht := newTracker(nil) + + ht.SetUnhealthy(testWarnable, Args{}) + state := ht.CurrentState().Warnings[testWarnable.Code] + + // Set to healthy and back again to reset start time + ht.SetHealthy(testWarnable) + + ht.SetUnhealthy(testWarnable, Args{}) + 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("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") + } + }) +} diff --git a/health/state.go b/health/state.go index b5e6a8a38..ccf3b2ac5 100644 --- a/health/state.go +++ b/health/state.go @@ -4,6 +4,9 @@ package health import ( + "crypto/sha256" + "encoding/json" + "fmt" "time" "tailscale.com/tailcfg" @@ -35,6 +38,40 @@ 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 +} + +// hash computes a deep hash of UnhealthyState which will be stable across +// different runs of the same binary. +func (u UnhealthyState) hash() ([]byte, error) { + hasher := sha256.New() + enc := json.NewEncoder(hasher) + err := enc.Encode(u) + if err != nil { + return nil, err + } + return hasher.Sum(nil), 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 = "" + hash, err := u.hash() + if err != nil { + return u + } + u.ETag = fmt.Sprintf("%x", hash) + return u } // UnhealthyStateAction represents an action (URL and link) to be presented to @@ -107,7 +144,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 +165,7 @@ func (t *Tracker) CurrentState() *State { } } - wm[state.WarnableCode] = state + wm[state.WarnableCode] = state.withETag() } return &State{