diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index 911656ad5..30edac5cb 100644 --- a/cmd/tailscale/cli/up.go +++ b/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 diff --git a/ipn/backend.go b/ipn/backend.go index e186b45f7..1dd301cad 100644 --- a/ipn/backend.go +++ b/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 diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 582f13cac..3603e2e0e 100644 --- a/ipn/ipnlocal/local.go +++ b/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: "), diff --git a/ipn/prefs.go b/ipn/prefs.go index 61f837ae0..912262d1c 100644 --- a/ipn/prefs.go +++ b/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