diff --git a/control/controlclient/map.go b/control/controlclient/map.go index abfc5eb17..f346e19d4 100644 --- a/control/controlclient/map.go +++ b/control/controlclient/map.go @@ -90,6 +90,7 @@ type mapSession struct { lastDomain string lastDomainAuditLogID string lastHealth []string + lastDisplayMessages map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage lastPopBrowserURL string lastTKAInfo *tailcfg.TKAInfo lastNetmapSummary string // from NetworkMap.VeryConcise @@ -412,6 +413,21 @@ func (ms *mapSession) updateStateFromResponse(resp *tailcfg.MapResponse) { if resp.Health != nil { ms.lastHealth = resp.Health } + if resp.DisplayMessages != nil { + if v, ok := resp.DisplayMessages["*"]; ok && v == nil { + ms.lastDisplayMessages = nil + } + for k, v := range resp.DisplayMessages { + if k == "*" { + continue + } + if v != nil { + mak.Set(&ms.lastDisplayMessages, k, *v) + } else { + delete(ms.lastDisplayMessages, k) + } + } + } if resp.TKAInfo != nil { ms.lastTKAInfo = resp.TKAInfo } @@ -831,14 +847,19 @@ func (ms *mapSession) sortedPeers() []tailcfg.NodeView { func (ms *mapSession) netmap() *netmap.NetworkMap { peerViews := ms.sortedPeers() - // Convert all ms.lastHealth to the new [netmap.NetworkMap.DisplayMessages]. var msgs map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage - for _, h := range ms.lastHealth { - mak.Set(&msgs, tailcfg.DisplayMessageID("control-health-"+strhash(h)), tailcfg.DisplayMessage{ - Title: "Coordination server reports an issue", - Severity: tailcfg.SeverityMedium, - Text: "The coordination server is reporting a health issue: " + h, - }) + if len(ms.lastDisplayMessages) != 0 { + msgs = ms.lastDisplayMessages + } else if len(ms.lastHealth) > 0 { + // Convert all ms.lastHealth to the new [netmap.NetworkMap.DisplayMessages] + for _, h := range ms.lastHealth { + id := "control-health-" + strhash(h) // Unique ID in case there is more than one health message + mak.Set(&msgs, tailcfg.DisplayMessageID(id), tailcfg.DisplayMessage{ + Title: "Coordination server reports an issue", + Severity: tailcfg.SeverityMedium, + Text: "The coordination server is reporting a health issue: " + h, + }) + } } nm := &netmap.NetworkMap{ diff --git a/control/controlclient/map_test.go b/control/controlclient/map_test.go index 9abaae923..013640f47 100644 --- a/control/controlclient/map_test.go +++ b/control/controlclient/map_test.go @@ -16,6 +16,7 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "go4.org/mem" "tailscale.com/control/controlknobs" "tailscale.com/health" @@ -1139,8 +1140,190 @@ func BenchmarkMapSessionDelta(b *testing.B) { } } +// TestNetmapDisplayMessage checks that the various diff operations +// (add/update/delete/clear) for [tailcfg.DisplayMessage] in a +// [tailcfg.MapResponse] work as expected. +func TestNetmapDisplayMessage(t *testing.T) { + type test struct { + name string + initialState *tailcfg.MapResponse + mapResponse tailcfg.MapResponse + wantMessages map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage + } + + tests := []test{ + { + name: "basic-set", + mapResponse: tailcfg.MapResponse{ + DisplayMessages: map[tailcfg.DisplayMessageID]*tailcfg.DisplayMessage{ + "test-message": { + Title: "Testing", + Text: "This is a test message", + Severity: tailcfg.SeverityHigh, + ImpactsConnectivity: true, + PrimaryAction: &tailcfg.DisplayMessageAction{ + URL: "https://www.example.com", + Label: "Learn more", + }, + }, + }, + }, + wantMessages: map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ + "test-message": { + Title: "Testing", + Text: "This is a test message", + Severity: tailcfg.SeverityHigh, + ImpactsConnectivity: true, + PrimaryAction: &tailcfg.DisplayMessageAction{ + URL: "https://www.example.com", + Label: "Learn more", + }, + }, + }, + }, + { + name: "delete-one", + initialState: &tailcfg.MapResponse{ + DisplayMessages: map[tailcfg.DisplayMessageID]*tailcfg.DisplayMessage{ + "message-a": { + Title: "Message A", + }, + "message-b": { + Title: "Message B", + }, + }, + }, + mapResponse: tailcfg.MapResponse{ + DisplayMessages: map[tailcfg.DisplayMessageID]*tailcfg.DisplayMessage{ + "message-a": nil, + }, + }, + wantMessages: map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ + "message-b": { + Title: "Message B", + }, + }, + }, + { + name: "update-one", + initialState: &tailcfg.MapResponse{ + DisplayMessages: map[tailcfg.DisplayMessageID]*tailcfg.DisplayMessage{ + "message-a": { + Title: "Message A", + }, + "message-b": { + Title: "Message B", + }, + }, + }, + mapResponse: tailcfg.MapResponse{ + DisplayMessages: map[tailcfg.DisplayMessageID]*tailcfg.DisplayMessage{ + "message-a": { + Title: "Message A updated", + }, + }, + }, + wantMessages: map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ + "message-a": { + Title: "Message A updated", + }, + "message-b": { + Title: "Message B", + }, + }, + }, + { + name: "add-one", + initialState: &tailcfg.MapResponse{ + DisplayMessages: map[tailcfg.DisplayMessageID]*tailcfg.DisplayMessage{ + "message-a": { + Title: "Message A", + }, + }, + }, + mapResponse: tailcfg.MapResponse{ + DisplayMessages: map[tailcfg.DisplayMessageID]*tailcfg.DisplayMessage{ + "message-b": { + Title: "Message B", + }, + }, + }, + wantMessages: map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ + "message-a": { + Title: "Message A", + }, + "message-b": { + Title: "Message B", + }, + }, + }, + { + name: "delete-all", + initialState: &tailcfg.MapResponse{ + DisplayMessages: map[tailcfg.DisplayMessageID]*tailcfg.DisplayMessage{ + "message-a": { + Title: "Message A", + }, + "message-b": { + Title: "Message B", + }, + }, + }, + mapResponse: tailcfg.MapResponse{ + DisplayMessages: map[tailcfg.DisplayMessageID]*tailcfg.DisplayMessage{ + "*": nil, + }, + }, + wantMessages: map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{}, + }, + { + name: "delete-all-and-add", + initialState: &tailcfg.MapResponse{ + DisplayMessages: map[tailcfg.DisplayMessageID]*tailcfg.DisplayMessage{ + "message-a": { + Title: "Message A", + }, + "message-b": { + Title: "Message B", + }, + }, + }, + mapResponse: tailcfg.MapResponse{ + DisplayMessages: map[tailcfg.DisplayMessageID]*tailcfg.DisplayMessage{ + "*": nil, + "message-c": { + Title: "Message C", + }, + }, + }, + wantMessages: map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ + "message-c": { + Title: "Message C", + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ms := newTestMapSession(t, nil) + + if test.initialState != nil { + ms.netmapForResponse(test.initialState) + } + + nm := ms.netmapForResponse(&test.mapResponse) + + if diff := cmp.Diff(test.wantMessages, nm.DisplayMessages, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("unexpected warnings (-want +got):\n%s", diff) + } + }) + } +} + // TestNetmapHealthIntegration checks that we get the expected health warnings -// from processing a map response and passing the NetworkMap to a health tracker +// from processing a [tailcfg.MapResponse] containing health messages and passing the +// [netmap.NetworkMap] to a [health.Tracker]. func TestNetmapHealthIntegration(t *testing.T) { ms := newTestMapSession(t, nil) ht := health.Tracker{} @@ -1182,3 +1365,56 @@ func TestNetmapHealthIntegration(t *testing.T) { t.Fatalf("CurrentStatus().Warnings[\"control-health*\"] different than expected (-want +got)\n%s", d) } } + +// TestNetmapDisplayMessageIntegration checks that we get the expected health +// warnings from processing a [tailcfg.MapResponse] that contains DisplayMessages and +// passing the [netmap.NetworkMap] to a [health.Tracker]. +func TestNetmapDisplayMessageIntegration(t *testing.T) { + ms := newTestMapSession(t, nil) + ht := health.Tracker{} + + ht.SetIPNState("NeedsLogin", true) + ht.GotStreamedMapResponse() + baseWarnings := ht.CurrentState().Warnings + + nm := ms.netmapForResponse(&tailcfg.MapResponse{ + DisplayMessages: map[tailcfg.DisplayMessageID]*tailcfg.DisplayMessage{ + "test-message": { + Title: "Testing", + Text: "This is a test message", + Severity: tailcfg.SeverityHigh, + ImpactsConnectivity: true, + PrimaryAction: &tailcfg.DisplayMessageAction{ + URL: "https://www.example.com", + Label: "Learn more", + }, + }, + }, + }) + ht.SetControlHealth(nm.DisplayMessages) + + state := ht.CurrentState() + + // Ignore warnings that aren't from the netmap + for k := range baseWarnings { + delete(state.Warnings, k) + } + + want := map[health.WarnableCode]health.UnhealthyState{ + "test-message": { + WarnableCode: "test-message", + Title: "Testing", + Text: "This is a test message", + Severity: health.SeverityHigh, + ImpactsConnectivity: true, + PrimaryAction: &health.UnhealthyStateAction{ + URL: "https://www.example.com", + Label: "Learn more", + }, + }, + } + + if diff := cmp.Diff(want, state.Warnings); diff != "" { + t.Errorf("unexpected message contents (-want +got):\n%s", diff) + } +} diff --git a/health/state.go b/health/state.go index cf4f922d7..cec967931 100644 --- a/health/state.go +++ b/health/state.go @@ -30,10 +30,19 @@ type UnhealthyState struct { Severity Severity Title string Text string - BrokenSince *time.Time `json:",omitempty"` - Args Args `json:",omitempty"` - DependsOn []WarnableCode `json:",omitempty"` - ImpactsConnectivity bool `json:",omitempty"` + BrokenSince *time.Time `json:",omitempty"` + Args Args `json:",omitempty"` + DependsOn []WarnableCode `json:",omitempty"` + ImpactsConnectivity bool `json:",omitempty"` + PrimaryAction *UnhealthyStateAction `json:",omitempty"` +} + +// UnhealthyStateAction represents an action (URL and link) to be presented to +// the user associated with an [UnhealthyState]. Analogous to +// [tailcfg.DisplayMessageAction]. +type UnhealthyStateAction struct { + URL string + Label string } // unhealthyState returns a unhealthyState of the Warnable given its current warningState. @@ -102,15 +111,23 @@ func (t *Tracker) CurrentState() *State { } for id, msg := range t.lastNotifiedControlMessages { - code := WarnableCode(id) - wm[code] = UnhealthyState{ - WarnableCode: code, + state := UnhealthyState{ + WarnableCode: WarnableCode(id), Severity: severityFromTailcfg(msg.Severity), Title: msg.Title, Text: msg.Text, ImpactsConnectivity: msg.ImpactsConnectivity, // TODO(tailscale/corp#27759): DependsOn? } + + if msg.PrimaryAction != nil { + state.PrimaryAction = &UnhealthyStateAction{ + URL: msg.PrimaryAction.URL, + Label: msg.PrimaryAction.Label, + } + } + + wm[state.WarnableCode] = state } return &State{ diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index d69c07a9f..05f026631 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -5828,7 +5828,14 @@ func (b *LocalBackend) setNetMapLocked(nm *netmap.NetworkMap) { b.pauseOrResumeControlClientLocked() if nm != nil { - b.health.SetControlHealth(nm.DisplayMessages) + messages := make(map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage) + for id, msg := range nm.DisplayMessages { + if msg.PrimaryAction != nil && !b.validPopBrowserURL(msg.PrimaryAction.URL) { + msg.PrimaryAction = nil + } + messages[id] = msg + } + b.health.SetControlHealth(messages) } else { b.health.SetControlHealth(nil) } diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 19cfd9195..1ad3225a5 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -5339,3 +5339,68 @@ func TestSrcCapPacketFilter(t *testing.T) { t.Error("IsDrop() for node without cap = false, want true") } } + +func TestDisplayMessages(t *testing.T) { + b := newTestLocalBackend(t) + + // Pretend we're in a map poll so health updates get processed + ht := b.HealthTracker() + ht.SetIPNState("NeedsLogin", true) + ht.GotStreamedMapResponse() + + b.setNetMapLocked(&netmap.NetworkMap{ + DisplayMessages: map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ + "test-message": { + Title: "Testing", + }, + }, + }) + + state := ht.CurrentState() + _, ok := state.Warnings["test-message"] + + if !ok { + t.Error("no warning found with id 'test-message'") + } +} + +// TestDisplayMessagesURLFilter tests that we filter out any URLs that are not +// valid as a pop browser URL (see [LocalBackend.validPopBrowserURL]). +func TestDisplayMessagesURLFilter(t *testing.T) { + b := newTestLocalBackend(t) + + // Pretend we're in a map poll so health updates get processed + ht := b.HealthTracker() + ht.SetIPNState("NeedsLogin", true) + ht.GotStreamedMapResponse() + + b.setNetMapLocked(&netmap.NetworkMap{ + DisplayMessages: map[tailcfg.DisplayMessageID]tailcfg.DisplayMessage{ + "test-message": { + Title: "Testing", + Severity: tailcfg.SeverityHigh, + PrimaryAction: &tailcfg.DisplayMessageAction{ + URL: "https://www.evil.com", + Label: "Phishing Link", + }, + }, + }, + }) + + state := ht.CurrentState() + got, ok := state.Warnings["test-message"] + + if !ok { + t.Fatal("no warning found with id 'test-message'") + } + + want := health.UnhealthyState{ + WarnableCode: "test-message", + Title: "Testing", + Severity: health.SeverityHigh, + } + + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("Unexpected message content (-want/+got):\n%s", diff) + } +} diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 7e2fa3ffc..4679609f3 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -161,7 +161,8 @@ type CapabilityVersion int // - 114: 2025-01-30: NodeAttrMaxKeyDuration CapMap defined, clients might use it (no tailscaled code change) (#14829) // - 115: 2025-03-07: Client understands DERPRegion.NoMeasureNoHome. // - 116: 2025-05-05: Client serves MagicDNS "AAAA" if NodeAttrMagicDNSPeerAAAA set on self node -const CurrentCapabilityVersion CapabilityVersion = 116 +// - 117: 2025-05-28: Client understands DisplayMessages (structured health messages), but not necessarily PrimaryAction. +const CurrentCapabilityVersion CapabilityVersion = 117 // ID is an integer ID for a user, node, or login allocated by the // control plane. @@ -2030,11 +2031,29 @@ type MapResponse struct { // known problems). A non-zero length slice are the list of problems that // the control plane sees. // + // Either this will be set, or DisplayMessages will be set, but not both. + // // Note that this package's type, due its use of a slice and omitempty, is // unable to marshal a zero-length non-nil slice. The control server needs // to marshal this type using a separate type. See MapResponse docs. Health []string `json:",omitempty"` + // DisplayMessages sets the health state of the node from the control + // plane's perspective. + // + // Either this will be set, or Health will be set, but not both. + // + // The map keys are IDs that uniquely identify the type of health issue. The + // map values are the messages. If the server sends down a map with entries, + // the client treats it as a patch: new entries are added, keys with a value + // of nil are deleted, existing entries with new values are updated. A nil + // map and an empty map both mean no change has occurred since the last + // update. + // + // As a special case, the map key "*" with a value of nil means to clear all + // prior display messages before processing the other map entries. + DisplayMessages map[DisplayMessageID]*DisplayMessage `json:",omitempty"` + // SSHPolicy, if non-nil, updates the SSH policy for how incoming // SSH connections should be handled. SSHPolicy *SSHPolicy `json:",omitempty"` @@ -2079,24 +2098,53 @@ type MapResponse struct { } // DisplayMessage represents a health state of the node from the control plane's -// perspective. It is deliberately similar to health.Warnable as both get -// converted into health.UnhealthyState to be sent to the GUI. +// perspective. It is deliberately similar to [health.Warnable] as both get +// converted into [health.UnhealthyState] to be sent to the GUI. type DisplayMessage struct { // Title is a string that the GUI uses as title for this message. The title - // should be short and fit in a single line. + // should be short and fit in a single line. It should not end in a period. + // + // Example: "Network may be blocking Tailscale". + // + // See the various instantiations of [health.Warnable] for more examples. Title string - // Text is an extended string that the GUI will display to the user. + // Text is an extended string that the GUI will display to the user. This + // could be multiple sentences explaining the issue in more detail. + // + // Example: "macOS Screen Time seems to be blocking Tailscale. Try disabling + // Screen Time in System Settings > Screen Time > Content & Privacy > Access + // to Web Content." + // + // See the various instantiations of [health.Warnable] for more examples. Text string // Severity is the severity of the DisplayMessage, which the GUI can use to - // determine how to display it. Maps to health.Severity. + // determine how to display it. Maps to [health.Severity]. Severity DisplayMessageSeverity // ImpactsConnectivity is whether the health problem will impact the user's // ability to connect to the Internet or other nodes on the tailnet, which // the GUI can use to determine how to display it. ImpactsConnectivity bool `json:",omitempty"` + + // Primary action, if present, represents the action to allow the user to + // take when interacting with this message. For example, if the + // DisplayMessage is shown via a notification, the action label might be a + // button on that notification and clicking the button would open the URL. + PrimaryAction *DisplayMessageAction `json:",omitempty"` +} + +// DisplayMessageAction represents an action (URL and link) to be presented to +// the user associated with a [DisplayMessage]. +type DisplayMessageAction struct { + // URL is the URL to navigate to when the user interacts with this action + URL string + + // Label is the call to action for the UI to display on the UI element that + // will open the URL (such as a button or link). For example, "Sign in" or + // "Learn more". + Label string } // DisplayMessageID is a string that uniquely identifies the kind of health diff --git a/types/netmap/nodemut.go b/types/netmap/nodemut.go index e31c731be..ccbdeae3f 100644 --- a/types/netmap/nodemut.go +++ b/types/netmap/nodemut.go @@ -163,6 +163,7 @@ func mapResponseContainsNonPatchFields(res *tailcfg.MapResponse) bool { res.PacketFilters != nil || res.UserProfiles != nil || res.Health != nil || + res.DisplayMessages != nil || res.SSHPolicy != nil || res.TKAInfo != nil || res.DomainDataPlaneAuditLogID != "" ||