all: deprecate Node.Capabilities (more), remove PeerChange.Capabiliti…

…es [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>
tailscale · Mar 24, 2024 · f4814c7 · f4814c7
1 parent b104688
commit f4814c7
Show file tree

Hide file tree

Showing 10 changed files with 42 additions and 65 deletions.
diff --git 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
@@ -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.
@@ -476,10 +484,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 +605,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 +729,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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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