cmd/tailscale/cli: warn if a simple up would change prefs (#17877)

Updates tailscale/corp#21570

Signed-off-by: James Sanderson <jsanderson@tailscale.com>

cmd/tailscale/cli/cli_test.go

@@ -174,6 +174,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 		curUser       string // os.Getenv("USER") on the client side
 		goos          string // empty means "linux"
 		distro        distro.Distro
+		backendState  string // empty means "Running"
 
 		want string
 	}{
@@ -188,6 +189,28 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 			},
 			want: "",
 		},
+		{
+			name:         "bare_up_needs_login_default_prefs",
+			flags:        []string{},
+			curPrefs:     ipn.NewPrefs(),
+			backendState: ipn.NeedsLogin.String(),
+			want:         "",
+		},
+		{
+			name:  "bare_up_needs_login_losing_prefs",
+			flags: []string{},
+			curPrefs: &ipn.Prefs{
+				// defaults:
+				ControlURL:          ipn.DefaultControlURL,
+				WantRunning:         false,
+				NetfilterMode:       preftype.NetfilterOn,
+				NoStatefulFiltering: opt.NewBool(true),
+				// non-default:
+				CorpDNS: false,
+			},
+			backendState: ipn.NeedsLogin.String(),
+			want:         accidentalUpPrefix + " --accept-dns=false",
+		},
 		{
 			name:  "losing_hostname",
 			flags: []string{"--accept-dns"},
@@ -620,9 +643,13 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			goos := "linux"
+			goos := stdcmp.Or(tt.goos, "linux")
-			if tt.goos != "" {
+			backendState := stdcmp.Or(tt.backendState, ipn.Running.String())
-				goos = tt.goos
+			// Needs to match the other conditions in checkForAccidentalSettingReverts
+			tt.curPrefs.Persist = &persist.Persist{
+				UserProfile: tailcfg.UserProfile{
+					LoginName: "janet",
+				},
 			}
 			var upArgs upArgsT
 			flagSet := newUpFlagSet(goos, &upArgs, "up")
@@ -638,10 +665,11 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 				curExitNodeIP: tt.curExitNodeIP,
 				distro:        tt.distro,
 				user:          tt.curUser,
+				backendState:  backendState,
 			}
 			applyImplicitPrefs(newPrefs, tt.curPrefs, upEnv)
 			var got string
-			if err := checkForAccidentalSettingReverts(newPrefs, tt.curPrefs, upEnv); err != nil {
+			if _, err := checkForAccidentalSettingReverts(newPrefs, tt.curPrefs, upEnv); err != nil {
 				got = err.Error()
 			}
 			if strings.TrimSpace(got) != tt.want {
@@ -1013,11 +1041,8 @@ func TestUpdatePrefs(t *testing.T) {
 		{
 			name:         "bare_up_means_up",
 			flags:        []string{},
-			curPrefs: &ipn.Prefs{
+			curPrefs:     ipn.NewPrefs(),
-				ControlURL:  ipn.DefaultControlURL,
+			wantSimpleUp: false, // user profile not set, so no simple up
-				WantRunning: false,
-				Hostname:    "foo",
-			},
 		},
 		{
 			name:  "just_up",
@@ -1031,6 +1056,32 @@ func TestUpdatePrefs(t *testing.T) {
 			},
 			wantSimpleUp: true,
 		},
+		{
+			name:     "just_up_needs_login_default_prefs",
+			flags:    []string{},
+			curPrefs: ipn.NewPrefs(),
+			env: upCheckEnv{
+				backendState: "NeedsLogin",
+			},
+			wantSimpleUp: false,
+		},
+		{
+			name:  "just_up_needs_login_losing_prefs",
+			flags: []string{},
+			curPrefs: &ipn.Prefs{
+				// defaults:
+				ControlURL:    ipn.DefaultControlURL,
+				WantRunning:   false,
+				NetfilterMode: preftype.NetfilterOn,
+				// non-default:
+				CorpDNS: false,
+			},
+			env: upCheckEnv{
+				backendState: "NeedsLogin",
+			},
+			wantSimpleUp: false,
+			wantErrSubtr: "tailscale up --accept-dns=false",
+		},
 		{
 			name:  "just_edit",
 			flags: []string{},

cmd/tailscale/cli/up.go

@@ -388,7 +388,8 @@ func updatePrefs(prefs, curPrefs *ipn.Prefs, env upCheckEnv) (simpleUp bool, jus
 	if !env.upArgs.reset {
 		applyImplicitPrefs(prefs, curPrefs, env)
 
-		if err := checkForAccidentalSettingReverts(prefs, curPrefs, env); err != nil {
+		simpleUp, err = checkForAccidentalSettingReverts(prefs, curPrefs, env)
+		if err != nil {
 			return false, nil, err
 		}
 	}
@@ -420,11 +421,6 @@ func updatePrefs(prefs, curPrefs *ipn.Prefs, env upCheckEnv) (simpleUp bool, jus
 
 	tagsChanged := !reflect.DeepEqual(curPrefs.AdvertiseTags, prefs.AdvertiseTags)
 
-	simpleUp = env.flagSet.NFlag() == 0 &&
-		curPrefs.Persist != nil &&
-		curPrefs.Persist.UserProfile.LoginName != "" &&
-		env.backendState != ipn.NeedsLogin.String()
-
 	justEdit := env.backendState == ipn.Running.String() &&
 		!env.upArgs.forceReauth &&
 		env.upArgs.authKeyOrFile == "" &&
@@ -968,10 +964,10 @@ 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(newPrefs, curPrefs *ipn.Prefs, env upCheckEnv) error {
+func checkForAccidentalSettingReverts(newPrefs, curPrefs *ipn.Prefs, env upCheckEnv) (simpleUp bool, err error) {
 	if curPrefs.ControlURL == "" {
 		// Don't validate things on initial "up" before a control URL has been set.
-		return nil
+		return false, nil
 	}
 
 	flagIsSet := map[string]bool{}
@@ -979,10 +975,13 @@ func checkForAccidentalSettingReverts(newPrefs, curPrefs *ipn.Prefs, env upCheck
 		flagIsSet[f.Name] = true
 	})
 
-	if len(flagIsSet) == 0 {
+	if len(flagIsSet) == 0 &&
+		curPrefs.Persist != nil &&
+		curPrefs.Persist.UserProfile.LoginName != "" &&
+		env.backendState != ipn.NeedsLogin.String() {
 		// A bare "tailscale up" is a special case to just
 		// mean bringing the network up without any changes.
-		return nil
+		return true, nil
 	}
 
 	// flagsCur is what flags we'd need to use to keep the exact
@@ -1024,7 +1023,7 @@ func checkForAccidentalSettingReverts(newPrefs, curPrefs *ipn.Prefs, env upCheck
 		missing = append(missing, fmtFlagValueArg(flagName, valCur))
 	}
 	if len(missing) == 0 {
-		return nil
+		return false, nil
 	}
 
 	// Some previously provided flags are missing. This run of 'tailscale
@@ -1057,7 +1056,7 @@ func checkForAccidentalSettingReverts(newPrefs, curPrefs *ipn.Prefs, env upCheck
 		fmt.Fprintf(&sb, " %s", a)
 	}
 	sb.WriteString("\n\n")
-	return errors.New(sb.String())
+	return false, errors.New(sb.String())
 }
 
 // applyImplicitPrefs mutates prefs to add implicit preferences for the user operator.