diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index c5546187c..c25059db4 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/cmd/tailscale/cli/cli_test.go @@ -493,14 +493,16 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { if err != nil { t.Fatal(err) } - applyImplicitPrefs(newPrefs, tt.curPrefs, tt.curUser) - var got string - if err := checkForAccidentalSettingReverts(newPrefs, tt.curPrefs, upCheckEnv{ + upEnv := upCheckEnv{ goos: goos, flagSet: flagSet, curExitNodeIP: tt.curExitNodeIP, distro: tt.distro, - }); err != nil { + user: tt.curUser, + } + applyImplicitPrefs(newPrefs, tt.curPrefs, upEnv) + var got string + if err := checkForAccidentalSettingReverts(newPrefs, tt.curPrefs, upEnv); err != nil { got = err.Error() } if strings.TrimSpace(got) != tt.want { @@ -784,6 +786,10 @@ func TestUpdatePrefs(t *testing.T) { curPrefs *ipn.Prefs env upCheckEnv // empty goos means "linux" + // checkUpdatePrefsMutations, if non-nil, is run with the new prefs after + // updatePrefs might've mutated them (from applyImplicitPrefs). + checkUpdatePrefsMutations func(t *testing.T, newPrefs *ipn.Prefs) + wantSimpleUp bool wantJustEditMP *ipn.MaskedPrefs wantErrSubtr string @@ -885,6 +891,28 @@ func TestUpdatePrefs(t *testing.T) { }, env: upCheckEnv{backendState: "Running"}, }, + { + // Issue 3808: explicitly empty --operator= should clear value. + name: "explicit_empty_operator", + flags: []string{"--operator="}, + curPrefs: &ipn.Prefs{ + ControlURL: "https://login.tailscale.com", + CorpDNS: true, + AllowSingleHosts: true, + NetfilterMode: preftype.NetfilterOn, + OperatorUser: "somebody", + }, + env: upCheckEnv{user: "somebody", backendState: "Running"}, + wantJustEditMP: &ipn.MaskedPrefs{ + OperatorUserSet: true, + WantRunningSet: true, + }, + checkUpdatePrefsMutations: func(t *testing.T, prefs *ipn.Prefs) { + if prefs.OperatorUser != "" { + t.Errorf("operator sent to backend should be empty; got %q", prefs.OperatorUser) + } + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -909,6 +937,9 @@ func TestUpdatePrefs(t *testing.T) { } t.Fatal(err) } + if tt.checkUpdatePrefsMutations != nil { + tt.checkUpdatePrefsMutations(t, newPrefs) + } if simpleUp != tt.wantSimpleUp { t.Fatalf("simpleUp=%v, want %v", simpleUp, tt.wantSimpleUp) } diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index f768971f3..9a2fd6ab0 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -362,7 +362,7 @@ func prefsFromUpArgs(upArgs upArgsT, warnf logger.Logf, st *ipnstate.Status, goo // 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) + applyImplicitPrefs(prefs, curPrefs, env) if err := checkForAccidentalSettingReverts(prefs, curPrefs, env); err != nil { return false, nil, err @@ -881,13 +881,20 @@ func checkForAccidentalSettingReverts(newPrefs, curPrefs *ipn.Prefs, env upCheck return errors.New(sb.String()) } -// applyImplicitPrefs mutates prefs to add implicit preferences. Currently -// this is just the operator user, which only needs to be set if it doesn't +// applyImplicitPrefs mutates prefs to add implicit preferences for the user operator. +// If the operator flag is passed no action is taken, otherwise this only needs to be set if it doesn't // match the current user. // // curUser is os.Getenv("USER"). It's pulled out for testability. -func applyImplicitPrefs(prefs, oldPrefs *ipn.Prefs, curUser string) { - if prefs.OperatorUser == "" && oldPrefs.OperatorUser == curUser { +func applyImplicitPrefs(prefs, oldPrefs *ipn.Prefs, env upCheckEnv) { + explicitOperator := false + env.flagSet.Visit(func(f *flag.Flag) { + if f.Name == "operator" { + explicitOperator = true + } + }) + + if prefs.OperatorUser == "" && oldPrefs.OperatorUser == env.user && !explicitOperator { prefs.OperatorUser = oldPrefs.OperatorUser } }