diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index 6765a3cc9..9b3888052 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/cmd/tailscale/cli/cli_test.go @@ -13,9 +13,11 @@ "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)) + } + }) + } +} diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index 4dbb63510..fae65e2e8 100644 --- a/cmd/tailscale/cli/up.go +++ b/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") + simpleUp, justEditMP, err := updatePrefs(prefs, curPrefs, env) + if err != nil { + fatalf("%s", err) } - - // 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) + 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 @@ func updateMaskedPrefsFromUpFlag(mp *ipn.MaskedPrefs, flagName string) { // 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 }