From 7b34154df221f5613b397226a0b2a9109d183ad7 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sat, 23 Mar 2024 16:23:59 -0700 Subject: [PATCH] all: deprecate Node.Capabilities (more), remove PeerChange.Capabilities [capver 89] First we had Capabilities []string. Then https://tailscale.com/blog/acl-grants (#4217) brought CapMap, a superset of Capabilities. Except we never really finished the transition inside the codebase to go all-in on CapMap. This does so. Notably, this coverts Capabilities on the wire early to CapMap internally so the code can only deal in CapMap, even against an old control server. In the process, this removes PeerChange.Capabilities support, which no known control plane sent anyway. They can and should use PeerChange.CapMap instead. Updates #11508 Updates #4217 Change-Id: I872074e226b873f9a578d9603897b831d50b25d9 Signed-off-by: Brad Fitzpatrick --- cmd/tailscale/cli/serve_legacy_test.go | 16 +++++++++----- control/controlclient/map.go | 22 ++++++++++--------- control/controlclient/map_test.go | 30 +------------------------- control/controlknobs/controlknobs.go | 8 ++----- ipn/ipnlocal/local.go | 7 +++--- ipn/ipnlocal/peerapi_test.go | 2 +- ipn/ipnlocal/serve_test.go | 3 +-- ipn/ipnstate/ipnstate.go | 6 +++++- ipn/serve.go | 2 +- tailcfg/tailcfg.go | 12 ++++------- 10 files changed, 42 insertions(+), 66 deletions(-) diff --git a/cmd/tailscale/cli/serve_legacy_test.go b/cmd/tailscale/cli/serve_legacy_test.go index 58139ecc2..5b70e6748 100644 --- a/cmd/tailscale/cli/serve_legacy_test.go +++ b/cmd/tailscale/cli/serve_legacy_test.go @@ -21,6 +21,7 @@ import ( "tailscale.com/ipn" "tailscale.com/ipn/ipnstate" "tailscale.com/tailcfg" + "tailscale.com/tstest" "tailscale.com/types/logger" ) @@ -807,9 +808,11 @@ func TestVerifyFunnelEnabled(t *testing.T) { lc.setQueryFeatureResponse(tt.queryFeatureResponse) if tt.caps != nil { - oldCaps := fakeStatus.Self.Capabilities - defer func() { fakeStatus.Self.Capabilities = oldCaps }() // reset after test - fakeStatus.Self.Capabilities = tt.caps + cm := make(tailcfg.NodeCapMap) + for _, c := range tt.caps { + cm[c] = nil + } + tstest.Replace(t, &fakeStatus.Self.CapMap, cm) } defer func() { @@ -853,8 +856,11 @@ type fakeLocalServeClient struct { var fakeStatus = &ipnstate.Status{ BackendState: ipn.Running.String(), Self: &ipnstate.PeerStatus{ - DNSName: "foo.test.ts.net", - Capabilities: []tailcfg.NodeCapability{tailcfg.NodeAttrFunnel, tailcfg.CapabilityFunnelPorts + "?ports=443,8443"}, + DNSName: "foo.test.ts.net", + CapMap: tailcfg.NodeCapMap{ + tailcfg.NodeAttrFunnel: nil, + tailcfg.CapabilityFunnelPorts + "?ports=443,8443": nil, + }, }, } diff --git a/control/controlclient/map.go b/control/controlclient/map.go index 482779b36..4f19e9753 100644 --- a/control/controlclient/map.go +++ b/control/controlclient/map.go @@ -172,7 +172,15 @@ func (ms *mapSession) HandleNonKeepAliveMapResponse(ctx context.Context, resp *t resp.Node.Capabilities = nil resp.Node.CapMap = nil } - ms.controlKnobs.UpdateFromNodeAttributes(resp.Node.Capabilities, resp.Node.CapMap) + // If the server is old and is still sending us Capabilities instead of + // CapMap, convert it to CapMap early so the rest of the client code can + // work only in terms of CapMap. + for _, c := range resp.Node.Capabilities { + if _, ok := resp.Node.CapMap[c]; !ok { + mak.Set(&resp.Node.CapMap, c, nil) + } + } + ms.controlKnobs.UpdateFromNodeAttributes(resp.Node.CapMap) } // Call Node.InitDisplayNames on any changed nodes. @@ -354,7 +362,6 @@ var ( patchOnline = clientmetric.NewCounter("controlclient_patch_online") patchLastSeen = clientmetric.NewCounter("controlclient_patch_lastseen") patchKeyExpiry = clientmetric.NewCounter("controlclient_patch_keyexpiry") - patchCapabilities = clientmetric.NewCounter("controlclient_patch_capabilities") patchCapMap = clientmetric.NewCounter("controlclient_patch_capmap") patchKeySignature = clientmetric.NewCounter("controlclient_patch_keysig") @@ -476,10 +483,6 @@ func (ms *mapSession) updatePeersStateFromResponse(resp *tailcfg.MapResponse) (s mut.KeyExpiry = *v patchKeyExpiry.Add(1) } - if v := pc.Capabilities; v != nil { - mut.Capabilities = *v - patchCapabilities.Add(1) - } if v := pc.KeySignature; v != nil { mut.KeySignature = v patchKeySignature.Add(1) @@ -601,6 +604,9 @@ func peerChangeDiff(was tailcfg.NodeView, n *tailcfg.Node) (_ *tailcfg.PeerChang continue case "DataPlaneAuditLogID": // Not sent for peers. + case "Capabilities": + // Deprecated; see https://github.com/tailscale/tailscale/issues/11508 + // And it was never sent by any known control server. case "ID": if was.ID() != n.ID { return nil, false @@ -722,10 +728,6 @@ func peerChangeDiff(was tailcfg.NodeView, n *tailcfg.Node) (_ *tailcfg.PeerChang if was.MachineAuthorized() != n.MachineAuthorized { return nil, false } - case "Capabilities": - if !views.SliceEqual(was.Capabilities(), views.SliceOf(n.Capabilities)) { - pc().Capabilities = ptr.To(n.Capabilities) - } case "UnsignedPeerAPIOnly": if was.UnsignedPeerAPIOnly() != n.UnsignedPeerAPIOnly { return nil, false diff --git a/control/controlclient/map_test.go b/control/controlclient/map_test.go index c3d05b44a..4a9002e17 100644 --- a/control/controlclient/map_test.go +++ b/control/controlclient/map_test.go @@ -331,23 +331,7 @@ func TestUpdatePeersStateFromResponse(t *testing.T) { }), wantStats: updateStats{changed: 1}, }, - { - name: "change_capabilities", - prev: peers(n(1, "foo")), - mapRes: &tailcfg.MapResponse{ - PeersChangedPatch: []*tailcfg.PeerChange{{ - NodeID: 1, - Capabilities: ptr.To([]tailcfg.NodeCapability{"foo"}), - }}, - }, - want: peers(&tailcfg.Node{ - ID: 1, - Name: "foo", - Capabilities: []tailcfg.NodeCapability{"foo"}, - }), - wantStats: updateStats{changed: 1}, - }} - + } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { if !tt.curTime.IsZero() { @@ -783,18 +767,6 @@ func TestPeerChangeDiff(t *testing.T) { b: &tailcfg.Node{ID: 1, LastSeen: ptr.To(time.Unix(2, 0))}, want: &tailcfg.PeerChange{NodeID: 1, LastSeen: ptr.To(time.Unix(2, 0))}, }, - { - name: "patch-capabilities-to-nonempty", - a: &tailcfg.Node{ID: 1, Capabilities: []tailcfg.NodeCapability{"foo"}}, - b: &tailcfg.Node{ID: 1, Capabilities: []tailcfg.NodeCapability{"bar"}}, - want: &tailcfg.PeerChange{NodeID: 1, Capabilities: ptr.To([]tailcfg.NodeCapability{"bar"})}, - }, - { - name: "patch-capabilities-to-empty", - a: &tailcfg.Node{ID: 1, Capabilities: []tailcfg.NodeCapability{"foo"}}, - b: &tailcfg.Node{ID: 1}, - want: &tailcfg.PeerChange{NodeID: 1, Capabilities: ptr.To([]tailcfg.NodeCapability(nil))}, - }, { name: "patch-online-to-true", a: &tailcfg.Node{ID: 1, Online: ptr.To(false)}, diff --git a/control/controlknobs/controlknobs.go b/control/controlknobs/controlknobs.go index 6a36c9261..6e3a62967 100644 --- a/control/controlknobs/controlknobs.go +++ b/control/controlknobs/controlknobs.go @@ -6,7 +6,6 @@ package controlknobs import ( - "slices" "sync/atomic" "tailscale.com/syncs" @@ -77,14 +76,11 @@ type Knobs struct { // UpdateFromNodeAttributes updates k (if non-nil) based on the provided self // node attributes (Node.Capabilities). -func (k *Knobs) UpdateFromNodeAttributes(selfNodeAttrs []tailcfg.NodeCapability, capMap tailcfg.NodeCapMap) { +func (k *Knobs) UpdateFromNodeAttributes(capMap tailcfg.NodeCapMap) { if k == nil { return } - has := func(attr tailcfg.NodeCapability) bool { - _, ok := capMap[attr] - return ok || slices.Contains(selfNodeAttrs, attr) - } + has := capMap.Contains var ( keepFullWG = has(tailcfg.NodeAttrDebugDisableWGTrim) disableDRPO = has(tailcfg.NodeAttrDebugDisableDRPO) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 9f6bcf17d..7dfac9595 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -823,15 +823,16 @@ func (b *LocalBackend) UpdateStatus(sb *ipnstate.StatusBuilder) { ss.UserID = b.netMap.User() if sn := b.netMap.SelfNode; sn.Valid() { peerStatusFromNode(ss, sn) - if c := sn.Capabilities(); c.Len() > 0 { - ss.Capabilities = c.AsSlice() - } if cm := sn.CapMap(); cm.Len() > 0 { + ss.Capabilities = make([]tailcfg.NodeCapability, 1, cm.Len()+1) + ss.Capabilities[0] = "HTTPS://TAILSCALE.COM/s/DEPRECATED-NODE-CAPS#see-https://github.com/tailscale/tailscale/issues/11508" ss.CapMap = make(tailcfg.NodeCapMap, sn.CapMap().Len()) cm.Range(func(k tailcfg.NodeCapability, v views.Slice[tailcfg.RawMessage]) bool { ss.CapMap[k] = v.AsSlice() + ss.Capabilities = append(ss.Capabilities, k) return true }) + slices.Sort(ss.Capabilities[1:]) } } for _, addr := range tailscaleIPs { diff --git a/ipn/ipnlocal/peerapi_test.go b/ipn/ipnlocal/peerapi_test.go index 85f5423e3..435ce5017 100644 --- a/ipn/ipnlocal/peerapi_test.go +++ b/ipn/ipnlocal/peerapi_test.go @@ -522,7 +522,7 @@ func TestHandlePeerAPI(t *testing.T) { }, } if tt.debugCap { - selfNode.Capabilities = append(selfNode.Capabilities, tailcfg.CapabilityDebug) + selfNode.CapMap = tailcfg.NodeCapMap{tailcfg.CapabilityDebug: nil} } var e peerAPITestEnv lb := &LocalBackend{ diff --git a/ipn/ipnlocal/serve_test.go b/ipn/ipnlocal/serve_test.go index f2b3e303c..45bd4bc2c 100644 --- a/ipn/ipnlocal/serve_test.go +++ b/ipn/ipnlocal/serve_test.go @@ -684,8 +684,7 @@ func newTestBackend(t *testing.T) *LocalBackend { b.netMap = &netmap.NetworkMap{ SelfNode: (&tailcfg.Node{ - Name: "example.ts.net", - Capabilities: []tailcfg.NodeCapability{tailcfg.NodeAttrsTailFSAccess}, + Name: "example.ts.net", }).View(), UserProfiles: map[tailcfg.UserID]tailcfg.UserProfile{ tailcfg.UserID(1): { diff --git a/ipn/ipnstate/ipnstate.go b/ipn/ipnstate/ipnstate.go index 94f1c4c79..c9ad0d0da 100644 --- a/ipn/ipnstate/ipnstate.go +++ b/ipn/ipnstate/ipnstate.go @@ -266,6 +266,10 @@ type PeerStatus struct { // "https://tailscale.com/cap/is-admin" // "https://tailscale.com/cap/file-sharing" // "funnel" + // + // Deprecated: use CapMap instead. See https://github.com/tailscale/tailscale/issues/11508 + // Every value is Capabilities is also a key in CapMap, even if it + // has no values in that map. Capabilities []tailcfg.NodeCapability `json:",omitempty"` // CapMap is a map of capabilities to their values. @@ -306,7 +310,7 @@ type PeerStatus struct { // HasCap reports whether ps has the given capability. func (ps *PeerStatus) HasCap(cap tailcfg.NodeCapability) bool { - return ps.CapMap.Contains(cap) || slices.Contains(ps.Capabilities, cap) + return ps.CapMap.Contains(cap) } // IsTagged reports whether ps is tagged. diff --git a/ipn/serve.go b/ipn/serve.go index 89ed6e556..b6e085b0c 100644 --- a/ipn/serve.go +++ b/ipn/serve.go @@ -445,7 +445,7 @@ func CheckFunnelPort(wantedPort uint16, node *ipnstate.PeerStatus) error { break } if portsStr == "" { - for _, attr := range node.Capabilities { + for attr := range node.CapMap { attr := string(attr) if !strings.HasPrefix(attr, string(tailcfg.CapabilityFunnelPorts)) { continue diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 877e0e384..353181748 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -129,7 +129,8 @@ type CapabilityVersion int // - 86: 2024-01-23: Client understands NodeAttrProbeUDPLifetime // - 87: 2024-02-11: UserProfile.Groups removed (added in 66) // - 88: 2024-03-05: Client understands NodeAttrSuggestExitNode -const CurrentCapabilityVersion CapabilityVersion = 88 +// - 89: 2024-03-23: Client no longer respects deleted PeerChange.Capabilities (use CapMap) +const CurrentCapabilityVersion CapabilityVersion = 89 type StableID string @@ -325,7 +326,7 @@ type Node struct { // "https://tailscale.com/cap/is-admin" // "https://tailscale.com/cap/file-sharing" // - // Deprecated: use CapMap instead. + // Deprecated: use CapMap instead. See https://github.com/tailscale/tailscale/issues/11508 Capabilities []NodeCapability `json:",omitempty"` // CapMap is a map of capabilities to their optional argument/data values. @@ -415,7 +416,7 @@ func (v NodeView) HasCap(cap NodeCapability) bool { // HasCap reports whether the node has the given capability. // It is safe to call on a nil Node. func (v *Node) HasCap(cap NodeCapability) bool { - return v != nil && (v.CapMap.Contains(cap) || slices.Contains(v.Capabilities, cap)) + return v != nil && v.CapMap.Contains(cap) } // DisplayName returns the user-facing name for a node which should @@ -2660,11 +2661,6 @@ type PeerChange struct { // KeyExpiry, if non-nil, changes the NodeID's key expiry. KeyExpiry *time.Time `json:",omitempty"` - - // Capabilities, if non-nil, means that the NodeID's capabilities changed. - // It's a pointer to a slice for "omitempty", to allow differentiating - // a change to empty from no change. - Capabilities *[]NodeCapability `json:",omitempty"` } // DerpMagicIP is a fake WireGuard endpoint IP address that means to