cmd/tailscale/cli: pull out prefsFromUpArgs for testability, add tests

Will add more tests later but this locks in all the existing warnings
and errors at least, and some of the existing non-error behavior.

Mostly I want this to exist before I actually fix #1725.

Updates #1725

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

cmd/tailscale/cli/cli_test.go

@@ -5,11 +5,17 @@
 package cli
 
 import (
+	"bytes"
+	"encoding/json"
 	"flag"
+	"fmt"
+	"strings"
 	"testing"
 
 	"inet.af/netaddr"
 	"tailscale.com/ipn"
+	"tailscale.com/ipn/ipnstate"
+	"tailscale.com/types/preftype"
 )
 
 // Test that checkForAccidentalSettingReverts's updateMaskedPrefsFromUpFlag can handle
@@ -129,3 +135,161 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 		})
 	}
 }
+
+func TestPrefsFromUpArgs(t *testing.T) {
+	tests := []struct {
+		name     string
+		args     upArgsT
+		goos     string           // runtime.GOOS; empty means linux
+		st       *ipnstate.Status // or nil
+		want     *ipn.Prefs
+		wantErr  string
+		wantWarn string
+	}{
+		{
+			name: "zero",
+			goos: "windows",
+			args: upArgsT{},
+			want: &ipn.Prefs{
+				WantRunning:   true,
+				NoSNAT:        true,
+				NetfilterMode: preftype.NetfilterOn, // silly, but default from ipn.NewPref currently
+			},
+		},
+		{
+			name: "error_advertise_route_invalid_ip",
+			args: upArgsT{
+				advertiseRoutes: "foo",
+			},
+			wantErr: `"foo" is not a valid IP address or CIDR prefix`,
+		},
+		{
+			name: "error_advertise_route_unmasked_bits",
+			args: upArgsT{
+				advertiseRoutes: "1.2.3.4/16",
+			},
+			wantErr: `1.2.3.4/16 has non-address bits set; expected 1.2.0.0/16`,
+		},
+		{
+			name: "error_exit_node_bad_ip",
+			args: upArgsT{
+				exitNodeIP: "foo",
+			},
+			wantErr: `invalid IP address "foo" for --exit-node: unable to parse IP`,
+		},
+		{
+			name: "error_exit_node_allow_lan_without_exit_node",
+			args: upArgsT{
+				exitNodeAllowLANAccess: true,
+			},
+			wantErr: `--exit-node-allow-lan-access can only be used with --exit-node`,
+		},
+		{
+			name: "error_tag_prefix",
+			args: upArgsT{
+				advertiseTags: "foo",
+			},
+			wantErr: `tag: "foo": tags must start with 'tag:'`,
+		},
+		{
+			name: "error_long_hostname",
+			args: upArgsT{
+				hostname: strings.Repeat("a", 300),
+			},
+			wantErr: `hostname too long: 300 bytes (max 256)`,
+		},
+		{
+			name: "error_linux_netfilter_empty",
+			args: upArgsT{
+				netfilterMode: "",
+			},
+			wantErr: `invalid value --netfilter-mode=""`,
+		},
+		{
+			name: "error_linux_netfilter_bogus",
+			args: upArgsT{
+				netfilterMode: "bogus",
+			},
+			wantErr: `invalid value --netfilter-mode="bogus"`,
+		},
+		{
+			name: "error_exit_node_ip_is_self_ip",
+			args: upArgsT{
+				exitNodeIP: "100.105.106.107",
+			},
+			st: &ipnstate.Status{
+				TailscaleIPs: []netaddr.IP{netaddr.MustParseIP("100.105.106.107")},
+			},
+			wantErr: `cannot use 100.105.106.107 as the exit node as it is a local IP address to this machine, did you mean --advertise-exit-node?`,
+		},
+		{
+			name: "warn_linux_netfilter_nodivert",
+			goos: "linux",
+			args: upArgsT{
+				netfilterMode: "nodivert",
+			},
+			wantWarn: "netfilter=nodivert; add iptables calls to ts-* chains manually.",
+			want: &ipn.Prefs{
+				WantRunning:   true,
+				NetfilterMode: preftype.NetfilterNoDivert,
+				NoSNAT:        true,
+			},
+		},
+		{
+			name: "warn_linux_netfilter_off",
+			goos: "linux",
+			args: upArgsT{
+				netfilterMode: "off",
+			},
+			wantWarn: "netfilter=off; configure iptables yourself.",
+			want: &ipn.Prefs{
+				WantRunning:   true,
+				NetfilterMode: preftype.NetfilterOff,
+				NoSNAT:        true,
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var warnBuf bytes.Buffer
+			warnf := func(format string, a ...interface{}) {
+				fmt.Fprintf(&warnBuf, format, a...)
+			}
+			goos := tt.goos
+			if goos == "" {
+				goos = "linux"
+			}
+			st := tt.st
+			if st == nil {
+				st = new(ipnstate.Status)
+			}
+			got, err := prefsFromUpArgs(tt.args, warnf, st, goos)
+			gotErr := fmt.Sprint(err)
+			if tt.wantErr != "" {
+				if tt.wantErr != gotErr {
+					t.Errorf("wrong error.\n got error: %v\nwant error: %v\n", gotErr, tt.wantErr)
+				}
+				return
+			}
+			if err != nil {
+				t.Fatal(err)
+			}
+			if tt.want == nil {
+				t.Fatal("tt.want is nil")
+			}
+			if !got.Equals(tt.want) {
+				jgot, _ := json.MarshalIndent(got, "", "\t")
+				jwant, _ := json.MarshalIndent(tt.want, "", "\t")
+				if bytes.Equal(jgot, jwant) {
+					t.Logf("prefs differ only in non-JSON-visible ways (nil/non-nil zero-length arrays)")
+				}
+				t.Errorf("wrong prefs\n got: %s\nwant: %s\n\ngot: %s\nwant: %s\n",
+					got.Pretty(), tt.want.Pretty(),
+					jgot, jwant,
+				)
+
+			}
+		})
+	}
+
+}

cmd/tailscale/cli/up.go

@@ -22,7 +22,9 @@ import (
 	"inet.af/netaddr"
 	"tailscale.com/client/tailscale"
 	"tailscale.com/ipn"
+	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/tailcfg"
+	"tailscale.com/types/logger"
 	"tailscale.com/types/preftype"
 	"tailscale.com/version/distro"
 )
@@ -84,7 +86,7 @@ func defaultNetfilterMode() string {
 	return "on"
 }
 
-var upArgs struct {
+type upArgsT struct {
 	reset                  bool
 	server                 string
 	acceptRoutes           bool
@@ -104,6 +106,8 @@ var upArgs struct {
 	hostname               string
 }
 
+var upArgs upArgsT
+
 func warnf(format string, args ...interface{}) {
 	fmt.Printf("Warning: "+format+"\n", args...)
 }
@@ -113,29 +117,13 @@ var (
 	ipv6default = netaddr.MustParseIPPrefix("::/0")
 )
 
-func runUp(ctx context.Context, args []string) error {
+// prefsFromUpArgs returns the ipn.Prefs for the provided args.
-	if len(args) > 0 {
+//
-		fatalf("too many non-flag arguments: %q", args)
+// Note that the parameters upArgs and warnf are named intentionally
-	}
+// to shadow the globals to prevent accidental misuse of them. This
-
+// function exists for testing and should have no side effects or
-	st, err := tailscale.Status(ctx)
+// outside interactions (e.g. no making Tailscale local API calls).
-	if err != nil {
+func prefsFromUpArgs(upArgs upArgsT, warnf logger.Logf, st *ipnstate.Status, goos string) (*ipn.Prefs, error) {
-		fatalf("can't fetch status from tailscaled: %v", err)
-	}
-
-	if distro.Get() == distro.Synology {
-		notSupported := "not yet supported on Synology; see https://github.com/tailscale/tailscale/issues/451"
-		if upArgs.acceptRoutes {
-			return errors.New("--accept-routes is " + notSupported)
-		}
-		if upArgs.exitNodeIP != "" {
-			return errors.New("--exit-node is " + notSupported)
-		}
-		if upArgs.netfilterMode != "off" {
-			return errors.New("--netfilter-mode values besides \"off\" " + notSupported)
-		}
-	}
-
 	routeMap := map[netaddr.IPPrefix]bool{}
 	var default4, default6 bool
 	if upArgs.advertiseRoutes != "" {
@@ -143,10 +131,10 @@ func runUp(ctx context.Context, args []string) error {
 		for _, s := range advroutes {
 			ipp, err := netaddr.ParseIPPrefix(s)
 			if err != nil {
-				fatalf("%q is not a valid IP address or CIDR prefix", s)
+				return nil, fmt.Errorf("%q is not a valid IP address or CIDR prefix", s)
 			}
 			if ipp != ipp.Masked() {
-				fatalf("%s has non-address bits set; expected %s", ipp, ipp.Masked())
+				return nil, fmt.Errorf("%s has non-address bits set; expected %s", ipp, ipp.Masked())
 			}
 			if ipp == ipv4default {
 				default4 = true
@@ -156,20 +144,15 @@ func runUp(ctx context.Context, args []string) error {
 			routeMap[ipp] = true
 		}
 		if default4 && !default6 {
-			fatalf("%s advertised without its IPv6 counterpart, please also advertise %s", ipv4default, ipv6default)
+			return nil, fmt.Errorf("%s advertised without its IPv6 counterpart, please also advertise %s", ipv4default, ipv6default)
 		} else if default6 && !default4 {
-			fatalf("%s advertised without its IPv6 counterpart, please also advertise %s", ipv6default, ipv4default)
+			return nil, fmt.Errorf("%s advertised without its IPv6 counterpart, please also advertise %s", ipv6default, ipv4default)
 		}
 	}
 	if upArgs.advertiseDefaultRoute {
 		routeMap[netaddr.MustParseIPPrefix("0.0.0.0/0")] = true
 		routeMap[netaddr.MustParseIPPrefix("::/0")] = true
 	}
-	if len(routeMap) > 0 {
-		if err := tailscale.CheckIPForwarding(context.Background()); err != nil {
-			warnf("%v", err)
-		}
-	}
 	routes := make([]netaddr.IPPrefix, 0, len(routeMap))
 	for r := range routeMap {
 		routes = append(routes, r)
@@ -186,16 +169,16 @@ func runUp(ctx context.Context, args []string) error {
 		var err error
 		exitNodeIP, err = netaddr.ParseIP(upArgs.exitNodeIP)
 		if err != nil {
-			fatalf("invalid IP address %q for --exit-node: %v", upArgs.exitNodeIP, err)
+			return nil, fmt.Errorf("invalid IP address %q for --exit-node: %v", upArgs.exitNodeIP, err)
 		}
 	} else if upArgs.exitNodeAllowLANAccess {
-		fatalf("--exit-node-allow-lan-access can only be used with --exit-node")
+		return nil, fmt.Errorf("--exit-node-allow-lan-access can only be used with --exit-node")
 	}
 
-	if !exitNodeIP.IsZero() {
+	if upArgs.exitNodeIP != "" {
 		for _, ip := range st.TailscaleIPs {
 			if exitNodeIP == ip {
-				fatalf("cannot use %s as the exit node as it is a local IP address to this machine, did you mean --advertise-exit-node?", exitNodeIP)
+				return nil, fmt.Errorf("cannot use %s as the exit node as it is a local IP address to this machine, did you mean --advertise-exit-node?", upArgs.exitNodeIP)
 			}
 		}
 	}
@@ -206,13 +189,13 @@ func runUp(ctx context.Context, args []string) error {
 		for _, tag := range tags {
 			err := tailcfg.CheckTag(tag)
 			if err != nil {
-				fatalf("tag: %q: %s", tag, err)
+				return nil, fmt.Errorf("tag: %q: %s", tag, err)
 			}
 		}
 	}
 
 	if len(upArgs.hostname) > 256 {
-		fatalf("hostname too long: %d bytes (max 256)", len(upArgs.hostname))
+		return nil, fmt.Errorf("hostname too long: %d bytes (max 256)", len(upArgs.hostname))
 	}
 
 	prefs := ipn.NewPrefs()
@@ -230,7 +213,7 @@ func runUp(ctx context.Context, args []string) error {
 	prefs.Hostname = upArgs.hostname
 	prefs.ForceDaemon = upArgs.forceDaemon
 
-	if runtime.GOOS == "linux" {
+	if goos == "linux" {
 		switch upArgs.netfilterMode {
 		case "on":
 			prefs.NetfilterMode = preftype.NetfilterOn
@@ -241,7 +224,43 @@ func runUp(ctx context.Context, args []string) error {
 			prefs.NetfilterMode = preftype.NetfilterOff
 			warnf("netfilter=off; configure iptables yourself.")
 		default:
-			fatalf("invalid value --netfilter-mode: %q", upArgs.netfilterMode)
+			return nil, fmt.Errorf("invalid value --netfilter-mode=%q", upArgs.netfilterMode)
+		}
+	}
+	return prefs, nil
+}
+
+func runUp(ctx context.Context, args []string) error {
+	if len(args) > 0 {
+		fatalf("too many non-flag arguments: %q", args)
+	}
+
+	st, err := tailscale.Status(ctx)
+	if err != nil {
+		fatalf("can't fetch status from tailscaled: %v", err)
+	}
+
+	if distro.Get() == distro.Synology {
+		notSupported := "not yet supported on Synology; see https://github.com/tailscale/tailscale/issues/451"
+		if upArgs.acceptRoutes {
+			return errors.New("--accept-routes is " + notSupported)
+		}
+		if upArgs.exitNodeIP != "" {
+			return errors.New("--exit-node is " + notSupported)
+		}
+		if upArgs.netfilterMode != "off" {
+			return errors.New("--netfilter-mode values besides \"off\" " + notSupported)
+		}
+	}
+
+	prefs, err := prefsFromUpArgs(upArgs, warnf, st, runtime.GOOS)
+	if err != nil {
+		fatalf("%s", err)
+	}
+
+	if len(prefs.AdvertiseRoutes) > 0 {
+		if err := tailscale.CheckIPForwarding(context.Background()); err != nil {
+			warnf("%v", err)
 		}
 	}