cmd/tailscale/cli: factor out more up code for testing

In theory, some of the other table-driven tests could be moved into this
form now but I didn't want to disturb too much good test code.

Includes a commented-out test for #2384 that is currently failing.

Signed-off-by: David Crawshaw <crawshaw@tailscale.com>

cmd/tailscale/cli/cli_test.go

@@ -13,9 +13,11 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"inet.af/netaddr"
 	"tailscale.com/ipn"
 	"tailscale.com/ipn/ipnstate"
+	"tailscale.com/types/persist"
 	"tailscale.com/types/preftype"
 )
 
@@ -440,8 +442,9 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 			}
 			applyImplicitPrefs(newPrefs, tt.curPrefs, tt.curUser)
 			var got string
-			if err := checkForAccidentalSettingReverts(flagSet, tt.curPrefs, newPrefs, upCheckEnv{
+			if err := checkForAccidentalSettingReverts(newPrefs, tt.curPrefs, upCheckEnv{
 				goos:          goos,
+				flagSet:       flagSet,
 				curExitNodeIP: tt.curExitNodeIP,
 			}); err != nil {
 				got = err.Error()
@@ -688,3 +691,93 @@ func TestFlagAppliesToOS(t *testing.T) {
 		})
 	}
 }
+
+func TestUpdatePrefs(t *testing.T) {
+	tests := []struct {
+		name     string
+		flags    []string // argv to be parsed into env.flagSet and env.upArgs
+		curPrefs *ipn.Prefs
+		env      upCheckEnv // empty goos means "linux"
+
+		wantSimpleUp   bool
+		wantJustEditMP *ipn.MaskedPrefs
+		wantErrSubtr   string
+	}{
+		{
+			name:  "bare_up_means_up",
+			flags: []string{},
+			curPrefs: &ipn.Prefs{
+				ControlURL:  ipn.DefaultControlURL,
+				WantRunning: false,
+				Hostname:    "foo",
+			},
+		},
+		{
+			name:  "just_up",
+			flags: []string{},
+			curPrefs: &ipn.Prefs{
+				ControlURL: ipn.DefaultControlURL,
+				Persist:    &persist.Persist{LoginName: "crawshaw.github"},
+			},
+			env: upCheckEnv{
+				backendState: "Stopped",
+			},
+			wantSimpleUp: true,
+		},
+		{
+			name:  "just_edit",
+			flags: []string{},
+			curPrefs: &ipn.Prefs{
+				ControlURL: ipn.DefaultControlURL,
+				Persist:    &persist.Persist{LoginName: "crawshaw.github"},
+			},
+			env:            upCheckEnv{backendState: "Running"},
+			wantSimpleUp:   true,
+			wantJustEditMP: &ipn.MaskedPrefs{WantRunningSet: true},
+		},
+		/* TODO(crawshaw): fix, #2384 {
+			name:  "control_synonym",
+			flags: []string{},
+			curPrefs: &ipn.Prefs{
+				ControlURL: "https://login.tailscale.com",
+				Persist:    &persist.Persist{LoginName: "crawshaw.github"},
+			},
+			env:            upCheckEnv{backendState: "Running"},
+			wantSimpleUp:   true,
+			wantJustEditMP: &ipn.MaskedPrefs{WantRunningSet: true},
+		},*/
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.env.goos == "" {
+				tt.env.goos = "linux"
+			}
+			tt.env.flagSet = newUpFlagSet(tt.env.goos, &tt.env.upArgs)
+			tt.env.flagSet.Parse(tt.flags)
+
+			newPrefs, err := prefsFromUpArgs(tt.env.upArgs, t.Logf, new(ipnstate.Status), tt.env.goos)
+			if err != nil {
+				t.Fatal(err)
+			}
+			simpleUp, justEditMP, err := updatePrefs(newPrefs, tt.curPrefs, tt.env)
+			if err != nil {
+				if tt.wantErrSubtr != "" {
+					if !strings.Contains(err.Error(), tt.wantErrSubtr) {
+						t.Fatalf("want error %q, got: %v", tt.wantErrSubtr, err)
+					}
+					return
+				}
+				t.Fatal(err)
+			}
+			if simpleUp != tt.wantSimpleUp {
+				t.Fatalf("simpleUp=%v, want %v", simpleUp, tt.wantSimpleUp)
+			}
+			if justEditMP != nil {
+				justEditMP.Prefs = ipn.Prefs{} // uninteresting
+			}
+			if !reflect.DeepEqual(justEditMP, tt.wantJustEditMP) {
+				t.Fatalf("justEditMP: %v", cmp.Diff(justEditMP, tt.wantJustEditMP))
+			}
+		})
+	}
+}

cmd/tailscale/cli/up.go

@@ -240,6 +240,52 @@ func prefsFromUpArgs(upArgs upArgsT, warnf logger.Logf, st *ipnstate.Status, goo
 	return prefs, nil
 }
 
+// updatePrefs updates prefs based on curPrefs
+//
+// It returns a non-nil justEditMP if we're already running and none of
+// the flags require a restart, so we can just do an EditPrefs call and
+// change the prefs at runtime (e.g. changing hostname, changing
+// advertised tags, routes, etc).
+//
+// It returns simpleUp if we're running a simple "tailscale up" to
+// transition to running from a previously-logged-in but down state,
+// without changing any settings.
+func updatePrefs(prefs, curPrefs *ipn.Prefs, env upCheckEnv) (simpleUp bool, justEditMP *ipn.MaskedPrefs, err error) {
+	if !env.upArgs.reset {
+		applyImplicitPrefs(prefs, curPrefs, env.user)
+
+		if err := checkForAccidentalSettingReverts(prefs, curPrefs, env); err != nil {
+			return false, nil, err
+		}
+	}
+
+	controlURLChanged := curPrefs.ControlURL != prefs.ControlURL
+	if controlURLChanged && env.backendState == ipn.Running.String() && !env.upArgs.forceReauth {
+		return false, nil, fmt.Errorf("can't change --login-server without --force-reauth")
+	}
+
+	simpleUp = env.flagSet.NFlag() == 0 &&
+		curPrefs.Persist != nil &&
+		curPrefs.Persist.LoginName != "" &&
+		env.backendState != ipn.NeedsLogin.String()
+
+	justEdit := env.backendState == ipn.Running.String() &&
+		!env.upArgs.forceReauth &&
+		!env.upArgs.reset &&
+		env.upArgs.authKey == "" &&
+		!controlURLChanged
+	if justEdit {
+		justEditMP = new(ipn.MaskedPrefs)
+		justEditMP.WantRunningSet = true
+		justEditMP.Prefs = *prefs
+		env.flagSet.Visit(func(f *flag.Flag) {
+			updateMaskedPrefsFromUpFlag(justEditMP, f.Name)
+		})
+	}
+
+	return simpleUp, justEditMP, nil
+}
+
 func runUp(ctx context.Context, args []string) error {
 	if len(args) > 0 {
 		fatalf("too many non-flag arguments: %q", args)
@@ -296,51 +342,23 @@ func runUp(ctx context.Context, args []string) error {
 		return err
 	}
 
-	if !upArgs.reset {
-		applyImplicitPrefs(prefs, curPrefs, os.Getenv("USER"))
-
-		if err := checkForAccidentalSettingReverts(upFlagSet, curPrefs, prefs, upCheckEnv{
-			goos:          runtime.GOOS,
-			curExitNodeIP: exitNodeIP(prefs, st),
-		}); err != nil {
-			fatalf("%s", err)
-		}
+	env := upCheckEnv{
+		goos:          runtime.GOOS,
+		user:          os.Getenv("USER"),
+		flagSet:       upFlagSet,
+		upArgs:        upArgs,
+		backendState:  st.BackendState,
+		curExitNodeIP: exitNodeIP(prefs, st),
 	}
-
-	controlURLChanged := curPrefs.ControlURL != prefs.ControlURL
-	if controlURLChanged && st.BackendState == ipn.Running.String() && !upArgs.forceReauth {
-		fatalf("can't change --login-server without --force-reauth")
-	}
-
-	// If we're already running and none of the flags require a
-	// restart, we can just do an EditPrefs call and change the
-	// prefs at runtime (e.g. changing hostname, changing
-	// advertised tags, routes, etc)
-	justEdit := st.BackendState == ipn.Running.String() &&
-		!upArgs.forceReauth &&
-		!upArgs.reset &&
-		upArgs.authKey == "" &&
-		!controlURLChanged
-	if justEdit {
-		mp := new(ipn.MaskedPrefs)
-		mp.WantRunningSet = true
-		mp.Prefs = *prefs
-		upFlagSet.Visit(func(f *flag.Flag) {
-			updateMaskedPrefsFromUpFlag(mp, f.Name)
-		})
-
-		_, err := tailscale.EditPrefs(ctx, mp)
+	simpleUp, justEditMP, err := updatePrefs(prefs, curPrefs, env)
+	if err != nil {
+		fatalf("%s", err)
+	}
+	if justEditMP != nil {
+		_, err := tailscale.EditPrefs(ctx, justEditMP)
 		return err
 	}
 
-	// simpleUp is whether we're running a simple "tailscale up"
-	// to transition to running from a previously-logged-in but
-	// down state, without changing any settings.
-	simpleUp := upFlagSet.NFlag() == 0 &&
-		curPrefs.Persist != nil &&
-		curPrefs.Persist.LoginName != "" &&
-		st.BackendState != ipn.NeedsLogin.String()
-
 	// At this point we need to subscribe to the IPN bus to watch
 	// for state transitions and possible need to authenticate.
 	c, bc, pumpCtx, cancel := connect(ctx)
@@ -538,6 +556,10 @@ const accidentalUpPrefix = "Error: changing settings via 'tailscale up' requires
 // needed by checkForAccidentalSettingReverts and friends.
 type upCheckEnv struct {
 	goos          string
+	user          string
+	flagSet       *flag.FlagSet
+	upArgs        upArgsT
+	backendState  string
 	curExitNodeIP netaddr.IP
 }
 
@@ -555,14 +577,14 @@ type upCheckEnv struct {
 //
 // mp is the mask of settings actually set, where mp.Prefs is the new
 // preferences to set, including any values set from implicit flags.
-func checkForAccidentalSettingReverts(flagSet *flag.FlagSet, curPrefs, newPrefs *ipn.Prefs, env upCheckEnv) error {
+func checkForAccidentalSettingReverts(newPrefs, curPrefs *ipn.Prefs, env upCheckEnv) error {
 	if curPrefs.ControlURL == "" {
 		// Don't validate things on initial "up" before a control URL has been set.
 		return nil
 	}
 
 	flagIsSet := map[string]bool{}
-	flagSet.Visit(func(f *flag.Flag) {
+	env.flagSet.Visit(func(f *flag.Flag) {
 		flagIsSet[f.Name] = true
 	})
 
@@ -599,7 +621,7 @@ func checkForAccidentalSettingReverts(flagSet *flag.FlagSet, curPrefs, newPrefs
 	// Compute the stringification of the explicitly provided args in flagSet
 	// to prepend to the command to run.
 	var explicit []string
-	flagSet.Visit(func(f *flag.Flag) {
+	env.flagSet.Visit(func(f *flag.Flag) {
 		type isBool interface {
 			IsBoolFlag() bool
 		}