cmd/tailscale/cli: [set] handle selectively modifying routes/exit node

Noticed this while debugging something else, we would reset all routes if
either `--advertise-exit-node` or `--advertise-routes` were set. This handles
correctly updating them.

Also added tests.

Signed-off-by: Maisem Ali <maisem@tailscale.com>

cmd/derper/depaware.txt

@@ -97,6 +97,8 @@ tailscale.com/cmd/derper dependencies: (generated by github.com/tailscale/depawa
         golang.org/x/crypto/nacl/box                                 from tailscale.com/types/key
         golang.org/x/crypto/nacl/secretbox                           from golang.org/x/crypto/nacl/box
         golang.org/x/crypto/salsa20/salsa                            from golang.org/x/crypto/nacl/box+
+        golang.org/x/exp/constraints                                 from golang.org/x/exp/slices
+        golang.org/x/exp/slices                                      from tailscale.com/net/tsaddr
    L    golang.org/x/net/bpf                                         from github.com/mdlayher/netlink+
         golang.org/x/net/dns/dnsmessage                              from net+
         golang.org/x/net/http/httpguts                               from net/http

cmd/tailscale/cli/set.go

@@ -9,9 +9,11 @@ import (
 	"errors"
 	"flag"
 	"fmt"
+	"net/netip"
 
 	"github.com/peterbourgon/ff/v3/ffcli"
 	"tailscale.com/ipn"
+	"tailscale.com/net/tsaddr"
 	"tailscale.com/safesocket"
 )
 
@@ -77,11 +79,6 @@ func runSet(ctx context.Context, args []string) (retErr error) {
 		return err
 	}
 
-	routes, err := calcAdvertiseRoutes(setArgs.advertiseRoutes, setArgs.advertiseDefaultRoute)
-	if err != nil {
-		return err
-	}
-
 	maskedPrefs := &ipn.MaskedPrefs{
 		Prefs: ipn.Prefs{
 			RouteAll:               setArgs.acceptRoutes,
@@ -90,7 +87,6 @@ func runSet(ctx context.Context, args []string) (retErr error) {
 			ShieldsUp:              setArgs.shieldsUp,
 			RunSSH:                 setArgs.runSSH,
 			Hostname:               setArgs.hostname,
-			AdvertiseRoutes:        routes,
 			OperatorUser:           setArgs.opUser,
 		},
 	}
@@ -105,20 +101,32 @@ func runSet(ctx context.Context, args []string) (retErr error) {
 		}
 	}
 
+	var advertiseExitNodeSet, advertiseRoutesSet bool
 	setFlagSet.Visit(func(f *flag.Flag) {
 		updateMaskedPrefsFromUpOrSetFlag(maskedPrefs, f.Name)
+		switch f.Name {
+		case "advertise-exit-node":
+			advertiseExitNodeSet = true
+		case "advertise-routes":
+			advertiseRoutesSet = true
+		}
 	})
-
 	if maskedPrefs.IsEmpty() {
 		return flag.ErrHelp
 	}
 
-	if maskedPrefs.RunSSHSet {
+	curPrefs, err := localClient.GetPrefs(ctx)
-		curPrefs, err := localClient.GetPrefs(ctx)
+	if err != nil {
+		return err
+	}
+	if maskedPrefs.AdvertiseRoutesSet {
+		maskedPrefs.AdvertiseRoutes, err = calcAdvertiseRoutesForSet(advertiseExitNodeSet, advertiseRoutesSet, curPrefs, setArgs)
 		if err != nil {
 			return err
 		}
+	}
 
+	if maskedPrefs.RunSSHSet {
 		wantSSH, haveSSH := maskedPrefs.RunSSH, curPrefs.RunSSH
 		if err := presentSSHToggleRisk(wantSSH, haveSSH, setArgs.acceptedRisks); err != nil {
 			return err
@@ -128,3 +136,33 @@ func runSet(ctx context.Context, args []string) (retErr error) {
 	_, err = localClient.EditPrefs(ctx, maskedPrefs)
 	return err
 }
+
+// calcAdvertiseRoutesForSet returns the new value for Prefs.AdvertiseRoutes based on the
+// current value, the flags passed to "tailscale set".
+// advertiseExitNodeSet is whether the --advertise-exit-node flag was set.
+// advertiseRoutesSet is whether the --advertise-routes flag was set.
+// curPrefs is the current Prefs.
+// setArgs is the parsed command-line arguments.
+func calcAdvertiseRoutesForSet(advertiseExitNodeSet, advertiseRoutesSet bool, curPrefs *ipn.Prefs, setArgs setArgsT) (routes []netip.Prefix, err error) {
+	if advertiseExitNodeSet && advertiseRoutesSet {
+		return calcAdvertiseRoutes(setArgs.advertiseRoutes, setArgs.advertiseDefaultRoute)
+
+	}
+	if advertiseRoutesSet {
+		return calcAdvertiseRoutes(setArgs.advertiseRoutes, curPrefs.AdvertisesExitNode())
+	}
+	if advertiseExitNodeSet {
+		alreadyAdvertisesExitNode := curPrefs.AdvertisesExitNode()
+		if alreadyAdvertisesExitNode == setArgs.advertiseDefaultRoute {
+			return curPrefs.AdvertiseRoutes, nil
+		}
+		routes = tsaddr.FilterPrefixesCopy(curPrefs.AdvertiseRoutes, func(p netip.Prefix) bool {
+			return p.Bits() != 0
+		})
+		if setArgs.advertiseDefaultRoute {
+			routes = append(routes, tsaddr.AllIPv4(), tsaddr.AllIPv6())
+		}
+		return routes, nil
+	}
+	return nil, nil
+}

cmd/tailscale/cli/set_test.go

@@ -0,0 +1,133 @@
+// Copyright (c) 2022 Tailscale Inc & AUTHORS All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package cli
+
+import (
+	"net/netip"
+	"reflect"
+	"testing"
+
+	"tailscale.com/ipn"
+	"tailscale.com/net/tsaddr"
+)
+
+func ptrTo[T any](v T) *T { return &v }
+
+func TestCalcAdvertiseRoutesForSet(t *testing.T) {
+	pfx := netip.MustParsePrefix
+	tests := []struct {
+		name      string
+		setExit   *bool
+		setRoutes *string
+		was       []netip.Prefix
+		want      []netip.Prefix
+	}{
+		{
+			name: "empty",
+		},
+		{
+			name:    "advertise-exit",
+			setExit: ptrTo(true),
+			want:    tsaddr.ExitRoutes(),
+		},
+		{
+			name:    "advertise-exit/already-routes",
+			was:     []netip.Prefix{pfx("34.0.0.0/16")},
+			setExit: ptrTo(true),
+			want:    []netip.Prefix{pfx("34.0.0.0/16"), tsaddr.AllIPv4(), tsaddr.AllIPv6()},
+		},
+		{
+			name:    "advertise-exit/already-exit",
+			was:     tsaddr.ExitRoutes(),
+			setExit: ptrTo(true),
+			want:    tsaddr.ExitRoutes(),
+		},
+		{
+			name:    "stop-advertise-exit",
+			was:     tsaddr.ExitRoutes(),
+			setExit: ptrTo(false),
+			want:    nil,
+		},
+		{
+			name:    "stop-advertise-exit/with-routes",
+			was:     []netip.Prefix{pfx("34.0.0.0/16"), tsaddr.AllIPv4(), tsaddr.AllIPv6()},
+			setExit: ptrTo(false),
+			want:    []netip.Prefix{pfx("34.0.0.0/16")},
+		},
+		{
+			name:      "advertise-routes",
+			setRoutes: ptrTo("10.0.0.0/24,192.168.0.0/16"),
+			want:      []netip.Prefix{pfx("10.0.0.0/24"), pfx("192.168.0.0/16")},
+		},
+		{
+			name:      "advertise-routes/already-exit",
+			was:       tsaddr.ExitRoutes(),
+			setRoutes: ptrTo("10.0.0.0/24,192.168.0.0/16"),
+			want:      []netip.Prefix{pfx("10.0.0.0/24"), pfx("192.168.0.0/16"), tsaddr.AllIPv4(), tsaddr.AllIPv6()},
+		},
+		{
+			name:      "advertise-routes/already-diff-routes",
+			was:       []netip.Prefix{pfx("34.0.0.0/16")},
+			setRoutes: ptrTo("10.0.0.0/24,192.168.0.0/16"),
+			want:      []netip.Prefix{pfx("10.0.0.0/24"), pfx("192.168.0.0/16")},
+		},
+		{
+			name:      "stop-advertise-routes",
+			was:       []netip.Prefix{pfx("34.0.0.0/16")},
+			setRoutes: ptrTo(""),
+			want:      nil,
+		},
+		{
+			name:      "stop-advertise-routes/already-exit",
+			was:       []netip.Prefix{pfx("34.0.0.0/16"), tsaddr.AllIPv4(), tsaddr.AllIPv6()},
+			setRoutes: ptrTo(""),
+			want:      tsaddr.ExitRoutes(),
+		},
+		{
+			name:      "advertise-routes-and-exit",
+			setExit:   ptrTo(true),
+			setRoutes: ptrTo("10.0.0.0/24,192.168.0.0/16"),
+			want:      []netip.Prefix{pfx("10.0.0.0/24"), pfx("192.168.0.0/16"), tsaddr.AllIPv4(), tsaddr.AllIPv6()},
+		},
+		{
+			name:      "advertise-routes-and-exit/already-exit",
+			was:       tsaddr.ExitRoutes(),
+			setExit:   ptrTo(true),
+			setRoutes: ptrTo("10.0.0.0/24,192.168.0.0/16"),
+			want:      []netip.Prefix{pfx("10.0.0.0/24"), pfx("192.168.0.0/16"), tsaddr.AllIPv4(), tsaddr.AllIPv6()},
+		},
+		{
+			name:      "advertise-routes-and-exit/already-routes",
+			was:       []netip.Prefix{pfx("10.0.0.0/24"), pfx("192.168.0.0/16")},
+			setExit:   ptrTo(true),
+			setRoutes: ptrTo("10.0.0.0/24,192.168.0.0/16"),
+			want:      []netip.Prefix{pfx("10.0.0.0/24"), pfx("192.168.0.0/16"), tsaddr.AllIPv4(), tsaddr.AllIPv6()},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			curPrefs := &ipn.Prefs{
+				AdvertiseRoutes: tc.was,
+			}
+			sa := setArgsT{}
+			if tc.setExit != nil {
+				sa.advertiseDefaultRoute = *tc.setExit
+			}
+			if tc.setRoutes != nil {
+				sa.advertiseRoutes = *tc.setRoutes
+			}
+			got, err := calcAdvertiseRoutesForSet(tc.setExit != nil, tc.setRoutes != nil, curPrefs, sa)
+			if err != nil {
+				t.Fatal(err)
+			}
+			tsaddr.SortPrefixes(got)
+			tsaddr.SortPrefixes(tc.want)
+			if !reflect.DeepEqual(got, tc.want) {
+				t.Errorf("got %v, want %v", got, tc.want)
+			}
+		})
+	}
+}

cmd/tailscale/cli/up.go

@@ -261,6 +261,9 @@ func calcAdvertiseRoutes(advertiseRoutes string, advertiseDefaultRoute bool) ([]
 		routeMap[netip.MustParsePrefix("0.0.0.0/0")] = true
 		routeMap[netip.MustParsePrefix("::/0")] = true
 	}
+	if len(routeMap) == 0 {
+		return nil, nil
+	}
 	routes := make([]netip.Prefix, 0, len(routeMap))
 	for r := range routeMap {
 		routes = append(routes, r)

cmd/tailscale/depaware.txt

@@ -124,6 +124,8 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
         golang.org/x/crypto/nacl/secretbox                           from golang.org/x/crypto/nacl/box
         golang.org/x/crypto/pbkdf2                                   from software.sslmate.com/src/go-pkcs12
         golang.org/x/crypto/salsa20/salsa                            from golang.org/x/crypto/nacl/box+
+        golang.org/x/exp/constraints                                 from golang.org/x/exp/slices
+        golang.org/x/exp/slices                                      from tailscale.com/net/tsaddr
         golang.org/x/net/bpf                                         from github.com/mdlayher/netlink+
         golang.org/x/net/dns/dnsmessage                              from net+
         golang.org/x/net/http/httpguts                               from net/http+

ipn/ipnlocal/local.go

@@ -2930,19 +2930,10 @@ func peerRoutes(peers []wgcfg.Peer, cgnatThreshold int) (routes []netip.Prefix)
 		routes = append(routes, cgNATIPs...)
 	}
 
-	sort.Slice(routes, func(i, j int) bool {
+	tsaddr.SortPrefixes(routes)
-		return ipPrefixLess(routes[i], routes[j])
-	})
 	return routes
 }
 
-func ipPrefixLess(ri, rj netip.Prefix) bool {
-	if ri.Addr() == rj.Addr() {
-		return ri.Bits() < rj.Bits()
-	}
-	return ri.Addr().Less(rj.Addr())
-}
-
 // routerConfig produces a router.Config from a wireguard config and IPN prefs.
 func (b *LocalBackend) routerConfig(cfg *wgcfg.Config, prefs ipn.PrefsView, oneCGNATRoute bool) *router.Config {
 	singleRouteThreshold := 10_000

net/tsaddr/tsaddr.go

@@ -11,6 +11,7 @@ import (
 	"net/netip"
 	"sync"
 
+	"golang.org/x/exp/slices"
 	"tailscale.com/net/netaddr"
 )
 
@@ -266,6 +267,16 @@ func AllIPv6() netip.Prefix { return allIPv6 }
 // ExitRoutes returns a slice containing AllIPv4 and AllIPv6.
 func ExitRoutes() []netip.Prefix { return []netip.Prefix{allIPv4, allIPv6} }
 
+// SortPrefixes sorts the prefixes in place.
+func SortPrefixes(p []netip.Prefix) {
+	slices.SortFunc(p, func(ri, rj netip.Prefix) bool {
+		if ri.Addr() == rj.Addr() {
+			return ri.Bits() < rj.Bits()
+		}
+		return ri.Addr().Less(rj.Addr())
+	})
+}
+
 // FilterPrefixes returns a new slice, not aliasing in, containing elements of
 // in that match f.
 func FilterPrefixesCopy(in []netip.Prefix, f func(netip.Prefix) bool) []netip.Prefix {