diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index 3da21ba1d..266afbe9f 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/cmd/tailscale/cli/cli_test.go @@ -15,6 +15,7 @@ "inet.af/netaddr" "tailscale.com/ipn" "tailscale.com/ipn/ipnstate" + "tailscale.com/types/logger" "tailscale.com/types/preftype" ) @@ -46,6 +47,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { name: "bare_up_means_up", flagSet: f(), curPrefs: &ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, WantRunning: false, Hostname: "foo", }, @@ -61,15 +63,18 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { name: "losing_hostname", flagSet: f("accept-dns"), curPrefs: &ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, WantRunning: false, Hostname: "foo", CorpDNS: true, }, mp: &ipn.MaskedPrefs{ Prefs: ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, WantRunning: true, CorpDNS: true, }, + ControlURLSet: true, WantRunningSet: true, CorpDNSSet: true, }, @@ -79,14 +84,17 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { name: "hostname_changing_explicitly", flagSet: f("hostname"), curPrefs: &ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, WantRunning: false, Hostname: "foo", }, mp: &ipn.MaskedPrefs{ Prefs: ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, WantRunning: true, Hostname: "bar", }, + ControlURLSet: true, WantRunningSet: true, HostnameSet: true, }, @@ -96,14 +104,17 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { name: "hostname_changing_empty_explicitly", flagSet: f("hostname"), curPrefs: &ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, WantRunning: false, Hostname: "foo", }, mp: &ipn.MaskedPrefs{ Prefs: ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, WantRunning: true, Hostname: "", }, + ControlURLSet: true, WantRunningSet: true, HostnameSet: true, }, @@ -113,12 +124,27 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { name: "empty_slice_equals_nil_slice", flagSet: f("hostname"), curPrefs: &ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, AdvertiseRoutes: []netaddr.IPPrefix{}, }, mp: &ipn.MaskedPrefs{ Prefs: ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, AdvertiseRoutes: nil, }, + ControlURLSet: true, + }, + want: "", + }, + { + // Issue 1725: "tailscale up --authkey=..." (or other non-empty flags) works from + // a fresh server's initial prefs. + name: "up_with_default_prefs", + flagSet: f("authkey"), + curPrefs: ipn.NewPrefs(), + mp: &ipn.MaskedPrefs{ + Prefs: *defaultPrefsFromUpArgs(t), + WantRunningSet: true, }, want: "", }, @@ -136,6 +162,21 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { } } +func defaultPrefsFromUpArgs(t testing.TB) *ipn.Prefs { + upFlagSet.Parse(nil) // populates upArgs + if upFlagSet.Lookup("netfilter-mode") == nil && upArgs.netfilterMode == "" { + // This flag is not compiled on on-Linux platforms, + // but prefsFromUpArgs requires it be populated. + upArgs.netfilterMode = defaultNetfilterMode() + } + prefs, err := prefsFromUpArgs(upArgs, logger.Discard, new(ipnstate.Status), "linux") + if err != nil { + t.Fatalf("defaultPrefsFromUpArgs: %v", err) + } + prefs.WantRunning = true + return prefs +} + func TestPrefsFromUpArgs(t *testing.T) { tests := []struct { name string diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index 0f399d260..30b4b1ac9 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -499,6 +499,10 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre // mean bringing the network up without any changes. return nil } + if curPrefs.ControlURL == "" { + // Don't validate things on initial "up" before a control URL has been set. + return nil + } curWithExplicitEdits := curPrefs.Clone() curWithExplicitEdits.ApplyEdits(mp) diff --git a/ipn/fake_test.go b/ipn/fake_test.go index cacb13caa..4f6c9273b 100644 --- a/ipn/fake_test.go +++ b/ipn/fake_test.go @@ -19,7 +19,7 @@ type FakeBackend struct { } func (b *FakeBackend) Start(opts Options) error { - b.serverURL = opts.Prefs.ControlURL + b.serverURL = opts.Prefs.ControlURLOrDefault() if b.notify == nil { panic("FakeBackend.Start: SetNotifyCallback not called") } diff --git a/ipn/handle.go b/ipn/handle.go index 756dca5b0..7f1eb8ea3 100644 --- a/ipn/handle.go +++ b/ipn/handle.go @@ -156,7 +156,7 @@ func (h *Handle) Expiry() time.Time { } func (h *Handle) AdminPageURL() string { - return h.prefsCache.ControlURL + "/admin/machines" + return h.prefsCache.ControlURLOrDefault() + "/admin/machines" } func (h *Handle) StartLoginInteractive() { diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 18d8b310f..6fa920c7f 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -436,6 +436,15 @@ func (b *LocalBackend) setClientStatus(st controlclient.Status) { netMap := b.netMap interact := b.interact + if prefs.ControlURL == "" { + // Once we get a message from the control plane, set + // our ControlURL pref explicitly. This causes a + // future "tailscale up" to start checking for + // implicit setting reverts, which it doesn't do when + // ControlURL is blank. + prefs.ControlURL = prefs.ControlURLOrDefault() + prefsChanged = true + } if st.Persist != nil { if !b.prefs.Persist.Equals(st.Persist) { prefsChanged = true @@ -637,7 +646,7 @@ func (b *LocalBackend) Start(opts ipn.Options) error { } b.inServerMode = b.prefs.ForceDaemon - b.serverURL = b.prefs.ControlURL + b.serverURL = b.prefs.ControlURLOrDefault() hostinfo.RoutableIPs = append(hostinfo.RoutableIPs, b.prefs.AdvertiseRoutes...) hostinfo.RequestTags = append(hostinfo.RequestTags, b.prefs.AdvertiseTags...) if b.inServerMode || runtime.GOOS == "windows" { diff --git a/ipn/prefs.go b/ipn/prefs.go index 35b4a49b9..cd3cc2cda 100644 --- a/ipn/prefs.go +++ b/ipn/prefs.go @@ -33,6 +33,10 @@ // Prefs are the user modifiable settings of the Tailscale node agent. type Prefs struct { // ControlURL is the URL of the control server to use. + // + // 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. ControlURL string // RouteAll specifies whether to accept subnets advertised by @@ -340,12 +344,19 @@ func compareStrings(a, b []string) bool { return true } +// NewPrefs returns the default preferences to use. func NewPrefs() *Prefs { + // Provide default values for options which might be missing + // from the json data for any reason. The json can still + // override them to false. return &Prefs{ - // Provide default values for options which might be missing - // from the json data for any reason. The json can still - // override them to false. - ControlURL: DefaultControlURL, + // ControlURL is explicitly not set to signal that + // it's not yet configured, which relaxes the CLI "up" + // safety net features. It will get set to DefaultControlURL + // on first up. Or, if not, DefaultControlURL will be used + // later anyway. + ControlURL: "", + RouteAll: true, AllowSingleHosts: true, CorpDNS: true, @@ -354,6 +365,15 @@ func NewPrefs() *Prefs { } } +// ControlURLOrDefault returns the coordination server's URL base. +// If not configured, DefaultControlURL is returned instead. +func (p *Prefs) ControlURLOrDefault() string { + if p.ControlURL != "" { + return p.ControlURL + } + return DefaultControlURL +} + // PrefsFromBytes deserializes Prefs from a JSON blob. If // enforceDefaults is true, Prefs.RouteAll and Prefs.AllowSingleHosts // are forced on.