ipnlocal: accept a new opts.UpdatePrefs field.

This is needed because the original opts.Prefs field was at some point
subverted for use in frontend->backend state migration for backward
compatibility on some platforms. We still need that feature, but we
also need the feature of providing the full set of prefs from
`tailscale up`, *not* including overwriting the prefs.Persist keys, so
we can't use the original field from `tailscale up`.

`tailscale up` had attempted to compensate for that by doing SetPrefs()
before Start(), but that violates the ipn.Backend contract, which says
you should call Start() before anything else (that's why it's called
Start()). As a result, doing SetPrefs({ControlURL=...,
WantRunning=true}) would cause a connection to the *previous* control
server (because WantRunning=true), and then connect to the *new*
control server only after running Start().

This problem may have been avoided before, but only by pure luck.

It turned out to be relatively harmless since the connection to the old
control server was immediately closed and replaced anyway, but it
created a race condition that could have caused spurious notifications
or rejected keys if the server responded quickly.

As already covered by existing TODOs, a better fix would be to have
Start() get out of the business of state migration altogether. But
we're approaching a release so I want to make the minimum possible fix.

Fixes #1840.

Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>

cmd/tailscale/cli/up.go

@@ -422,11 +422,10 @@ func runUp(ctx context.Context, args []string) error {
 			return err
 		}
 	} else {
-		bc.SetPrefs(prefs)
-
 		opts := ipn.Options{
 			StateKey: ipn.GlobalDaemonStateKey,
 			AuthKey:  upArgs.authKey,
+			UpdatePrefs: prefs,
 		}
 		// On Windows, we still run in mostly the "legacy" way that
 		// predated the server's StateStore. That is, we send an empty

ipn/backend.go

@@ -185,8 +185,30 @@ type Options struct {
 	//    state and use/update that.
 	//  - StateKey!="" && Prefs!=nil: like the previous case, but do
 	//    an initial overwrite of backend state with Prefs.
+	//
+	// NOTE(apenwarr): The above means that this Prefs field does not do
+	// what you probably think it does. It will overwrite your encryption
+	// keys. Do not use unless you know what you're doing.
 	StateKey StateKey
 	Prefs    *Prefs
+	// UpdatePrefs, if provided, overrides Options.Prefs *and* the Prefs
+	// already stored in the backend state, *except* for the Persist
+	// Persist member. If you just want to provide prefs, this is
+	// probably what you want.
+	//
+	// UpdatePrefs.Persist is always ignored. Prefs.Persist will still
+	// be used even if UpdatePrefs is provided. Other than Persist,
+	// UpdatePrefs takes precedence over Prefs.
+	//
+	// This is intended as a purely temporary workaround for the
+	// currently unexpected behaviour of Options.Prefs.
+	//
+	// TODO(apenwarr): Remove this, or rename Prefs to something else
+	//   and rename this to Prefs. Or, move Prefs.Persist elsewhere
+	//   entirely (as it always should have been), and then we wouldn't
+	//   need two separate fields at all. Or, move the fancy state
+	//   migration stuff out of Start().
+	UpdatePrefs *Prefs
 	// AuthKey is an optional node auth key used to authorize a
 	// new node key without user interaction.
 	AuthKey string

ipn/ipnlocal/local.go

@@ -633,16 +633,18 @@ func (b *LocalBackend) getNewControlClientFunc() clientGen {
 //
 // b.mu must be held.
 func (b *LocalBackend) startIsNoopLocked(opts ipn.Options) bool {
-	// Options has 4 fields; check all of them:
+	// Options has 5 fields; check all of them:
 	//   * FrontendLogID
 	//   * StateKey
 	//   * Prefs
+	//   * UpdatePrefs
 	//   * AuthKey
 	return b.state == ipn.Running &&
 		b.hostinfo != nil &&
 		b.hostinfo.FrontendLogID == opts.FrontendLogID &&
 		b.stateKey == opts.StateKey &&
 		opts.Prefs == nil &&
+		opts.UpdatePrefs == nil &&
 		opts.AuthKey == ""
 }
 
@@ -717,6 +719,12 @@ func (b *LocalBackend) Start(opts ipn.Options) error {
 		return fmt.Errorf("loading requested state: %v", err)
 	}
 
+	if opts.UpdatePrefs != nil {
+		newPrefs := opts.UpdatePrefs
+		newPrefs.Persist = b.prefs.Persist
+		b.prefs = newPrefs
+	}
+
 	wantRunning := b.prefs.WantRunning
 	if wantRunning {
 		if err := b.initMachineKeyLocked(); err != nil {
@@ -779,6 +787,10 @@ func (b *LocalBackend) Start(opts ipn.Options) error {
 		debugFlags = append([]string{"netstack"}, debugFlags...)
 	}
 
+	// TODO(apenwarr): The only way to change the ServerURL is to
+	// re-run b.Start(), because this is the only place we create a
+	// new controlclient. SetPrefs() allows you to overwrite ServerURL,
+	// but it won't take effect until the next Start().
 	cc, err := b.getNewControlClientFunc()(controlclient.Options{
 		GetMachinePrivateKey: b.createGetMachinePrivateKeyFunc(),
 		Logf:                 logger.WithPrefix(b.logf, "control: "),

ipn/prefs.go

@@ -37,6 +37,15 @@ type Prefs struct {
 	// If empty, the default for new installs, DefaultControlURL
 	// is used. It's set non-empty once the daemon has been started
 	// for the first time.
+	//
+	// TODO(apenwarr): Make it safe to update this with SetPrefs().
+	// Right now, you have to pass it in the initial prefs in Start(),
+	// which is the only code that actually uses the ControlURL value.
+	// It would be more consistent to restart controlclient
+	// automatically whenever this variable changes.
+	//
+	// Meanwhile, you have to provide this as part of Options.Prefs or
+	// Options.UpdatePrefs when calling Backend.Start().
 	ControlURL string
 
 	// RouteAll specifies whether to accept subnets advertised by