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 <bradfitz@tailscale.com>

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
+				cm := make(tailcfg.NodeCapMap)
-				defer func() { fakeStatus.Self.Capabilities = oldCaps }() // reset after test
+				for _, c := range tt.caps {
-				fakeStatus.Self.Capabilities = 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",
+		DNSName: "foo.test.ts.net",
-		Capabilities: []tailcfg.NodeCapability{tailcfg.NodeAttrFunnel, tailcfg.CapabilityFunnelPorts + "?ports=443,8443"},
+		CapMap: tailcfg.NodeCapMap{
+			tailcfg.NodeAttrFunnel:                            nil,
+			tailcfg.CapabilityFunnelPorts + "?ports=443,8443": nil,
+		},
 	},
 }
 

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

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

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 {
+	has := capMap.Contains
-		_, ok := capMap[attr]
-		return ok || slices.Contains(selfNodeAttrs, attr)
-	}
 	var (
 		keepFullWG                    = has(tailcfg.NodeAttrDebugDisableWGTrim)
 		disableDRPO                   = has(tailcfg.NodeAttrDebugDisableDRPO)

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 {

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{

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",
+			Name: "example.ts.net",
-			Capabilities: []tailcfg.NodeCapability{tailcfg.NodeAttrsTailFSAccess},
 		}).View(),
 		UserProfiles: map[tailcfg.UserID]tailcfg.UserProfile{
 			tailcfg.UserID(1): {

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.

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

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