From e0fcd596bf50556243c488f916d5128dccba6638 Mon Sep 17 00:00:00 2001 From: James Sanderson Date: Mon, 14 Jul 2025 17:54:56 +0100 Subject: [PATCH] tailcfg: send health update if DisplayMessage URL changes Updates tailscale/corp#27759 Signed-off-by: James Sanderson --- health/health_test.go | 158 +++++++++++++++++++--------------------- tailcfg/tailcfg.go | 5 +- tailcfg/tailcfg_test.go | 113 ++++++++++++++++++++-------- 3 files changed, 161 insertions(+), 115 deletions(-) diff --git a/health/health_test.go b/health/health_test.go index 0f1140f62..53f012ecf 100644 --- a/health/health_test.go +++ b/health/health_test.go @@ -555,98 +555,88 @@ func TestControlHealth(t *testing.T) { }) } -func TestControlHealthNotifiesOnSet(t *testing.T) { - ht := Tracker{} - ht.SetIPNState("NeedsLogin", true) - ht.GotStreamedMapResponse() - - gotNotified := false - ht.registerSyncWatcher(func(_ Change) { - gotNotified = true - }) - - ht.SetControlHealth(map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ - "test": {}, - }) - - if !gotNotified { - t.Errorf("watcher did not get called, want it to be called") +func TestControlHealthNotifies(t *testing.T) { + type test struct { + name string + initialState map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage + newState map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage + wantNotify bool } -} - -func TestControlHealthNotifiesOnChange(t *testing.T) { - ht := Tracker{} - ht.SetIPNState("NeedsLogin", true) - ht.GotStreamedMapResponse() - - ht.SetControlHealth(map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ - "test-1": {}, - }) - - gotNotified := false - ht.registerSyncWatcher(func(_ Change) { - gotNotified = true - }) - - ht.SetControlHealth(map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ - "test-2": {}, - }) - - if !gotNotified { - t.Errorf("watcher did not get called, want it to be called") - } -} - -func TestControlHealthNotifiesOnDetailsChange(t *testing.T) { - ht := Tracker{} - ht.SetIPNState("NeedsLogin", true) - ht.GotStreamedMapResponse() - - ht.SetControlHealth(map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ - "test-1": { - Title: "Title", + tests := []test{ + { + name: "no-change", + initialState: map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ + "test": {}, + }, + newState: map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ + "test": {}, + }, + wantNotify: false, }, - }) - - gotNotified := false - ht.registerSyncWatcher(func(_ Change) { - gotNotified = true - }) - - ht.SetControlHealth(map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ - "test-1": { - Title: "Updated title", + { + name: "on-set", + initialState: map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{}, + newState: map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ + "test": {}, + }, + wantNotify: true, + }, + { + name: "details-change", + initialState: map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ + "test": { + Title: "Title", + }, + }, + newState: map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ + "test": { + Title: "Updated title", + }, + }, + wantNotify: true, + }, + { + name: "action-changes", + initialState: map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ + "test": { + PrimaryAction: &tailcfg.DisplayMessageAction{ + URL: "http://www.example.com/a/123456", + Label: "Sign in", + }, + }, + }, + newState: map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ + "test": { + PrimaryAction: &tailcfg.DisplayMessageAction{ + URL: "http://www.example.com/a/abcdefg", + Label: "Sign in", + }, + }, + }, + wantNotify: true, }, - }) - - if !gotNotified { - t.Errorf("watcher did not get called, want it to be called") } -} + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ht := Tracker{} + ht.SetIPNState("NeedsLogin", true) + ht.GotStreamedMapResponse() -func TestControlHealthNoNotifyOnUnchanged(t *testing.T) { - ht := Tracker{} - ht.SetIPNState("NeedsLogin", true) - ht.GotStreamedMapResponse() + if len(test.initialState) != 0 { + ht.SetControlHealth(test.initialState) + } - // Set up an existing control health issue - ht.SetControlHealth(map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ - "test": {}, - }) + gotNotified := false + ht.registerSyncWatcher(func(_ Change) { + gotNotified = true + }) - // Now register our watcher - gotNotified := false - ht.registerSyncWatcher(func(_ Change) { - gotNotified = true - }) + ht.SetControlHealth(test.newState) - // Send the same control health message again - should not notify - ht.SetControlHealth(map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ - "test": {}, - }) - - if gotNotified { - t.Errorf("watcher got called, want it to not be called") + if gotNotified != test.wantNotify { + t.Errorf("notified: got %v, want %v", gotNotified, test.wantNotify) + } + }) } } diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 53c4683c1..0f13c725e 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -2171,7 +2171,10 @@ func (m DisplayMessage) Equal(o DisplayMessage) bool { return m.Title == o.Title && m.Text == o.Text && m.Severity == o.Severity && - m.ImpactsConnectivity == o.ImpactsConnectivity + m.ImpactsConnectivity == o.ImpactsConnectivity && + (m.PrimaryAction == nil) == (o.PrimaryAction == nil) && + (m.PrimaryAction == nil || (m.PrimaryAction.URL == o.PrimaryAction.URL && + m.PrimaryAction.Label == o.PrimaryAction.Label)) } // DisplayMessageSeverity represents how serious a [DisplayMessage] is. Analogous diff --git a/tailcfg/tailcfg_test.go b/tailcfg/tailcfg_test.go index e8e86cdb1..833314df8 100644 --- a/tailcfg/tailcfg_test.go +++ b/tailcfg/tailcfg_test.go @@ -881,76 +881,129 @@ func TestCheckTag(t *testing.T) { } func TestDisplayMessageEqual(t *testing.T) { - base := DisplayMessage{ - Title: "title", - Text: "text", - Severity: SeverityHigh, - ImpactsConnectivity: false, - } - type test struct { name string - value DisplayMessage + value1 DisplayMessage + value2 DisplayMessage wantEqual bool } for _, test := range []test{ { name: "same", - value: DisplayMessage{ + value1: DisplayMessage{ Title: "title", Text: "text", Severity: SeverityHigh, ImpactsConnectivity: false, + PrimaryAction: &DisplayMessageAction{ + URL: "https://example.com", + Label: "Open", + }, + }, + value2: DisplayMessage{ + Title: "title", + Text: "text", + Severity: SeverityHigh, + ImpactsConnectivity: false, + PrimaryAction: &DisplayMessageAction{ + URL: "https://example.com", + Label: "Open", + }, }, wantEqual: true, }, { name: "different-title", - value: DisplayMessage{ - Title: "different title", - Text: "text", - Severity: SeverityHigh, - ImpactsConnectivity: false, + value1: DisplayMessage{ + Title: "title", + }, + value2: DisplayMessage{ + Title: "different title", }, wantEqual: false, }, { name: "different-text", - value: DisplayMessage{ - Title: "title", - Text: "different text", - Severity: SeverityHigh, - ImpactsConnectivity: false, + value1: DisplayMessage{ + Text: "some text", + }, + value2: DisplayMessage{ + Text: "different text", }, wantEqual: false, }, { name: "different-severity", - value: DisplayMessage{ - Title: "title", - Text: "text", - Severity: SeverityMedium, - ImpactsConnectivity: false, + value1: DisplayMessage{ + Severity: SeverityHigh, + }, + value2: DisplayMessage{ + Severity: SeverityMedium, }, wantEqual: false, }, { name: "different-impactsConnectivity", - value: DisplayMessage{ - Title: "title", - Text: "text", - Severity: SeverityHigh, + value1: DisplayMessage{ ImpactsConnectivity: true, }, + value2: DisplayMessage{ + ImpactsConnectivity: false, + }, + wantEqual: false, + }, + { + name: "different-primaryAction-nil-non-nil", + value1: DisplayMessage{}, + value2: DisplayMessage{ + PrimaryAction: &DisplayMessageAction{ + URL: "https://example.com", + Label: "Open", + }, + }, + wantEqual: false, + }, + { + name: "different-primaryAction-url", + value1: DisplayMessage{ + PrimaryAction: &DisplayMessageAction{ + URL: "https://example.com", + Label: "Open", + }, + }, + value2: DisplayMessage{ + PrimaryAction: &DisplayMessageAction{ + URL: "https://zombo.com", + Label: "Open", + }, + }, + wantEqual: false, + }, + { + name: "different-primaryAction-label", + value1: DisplayMessage{ + PrimaryAction: &DisplayMessageAction{ + URL: "https://example.com", + Label: "Open", + }, + }, + value2: DisplayMessage{ + PrimaryAction: &DisplayMessageAction{ + URL: "https://example.com", + Label: "Learn more", + }, + }, wantEqual: false, }, } { t.Run(test.name, func(t *testing.T) { - got := base.Equal(test.value) + got := test.value1.Equal(test.value2) if got != test.wantEqual { - t.Errorf("Equal: got %t, want %t", got, test.wantEqual) + value1 := must.Get(json.MarshalIndent(test.value1, "", " ")) + value2 := must.Get(json.MarshalIndent(test.value2, "", " ")) + t.Errorf("value1.Equal(value2): got %t, want %t\nvalue1:\n%s\nvalue2:\n%s", got, test.wantEqual, value1, value2) } }) }