tailcfg: send health update if DisplayMessage URL changes

Updates tailscale/corp#27759 Signed-off-by: James Sanderson <jsanderson@tailscale.com>
2025-08-14 06:57:31 +00:00 · 2025-07-14 17:54:56 +01:00
parent 7a3221177e
commit e0fcd596bf
3 changed files with 161 additions and 115 deletions
--- 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)
+			}
+		})
 	}
 }

--- 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
--- 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)
 			}
 		})
 	}