From 49808ae6eac374996a5f42e2c7492f3449576790 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sat, 17 Apr 2021 22:50:58 -0700 Subject: [PATCH] ipn{,/ipnlocal}, cmd/tailscale/cli: don't check pref reverts on initial up The ipn.NewPrefs func returns a populated ipn.Prefs for historical reasons. It's not used or as important as it once was, but it hasn't yet been removed. Meanwhile, it contains some default values that are used on some platforms. Notably, for this bug (#1725), Windows/Mac use its Prefs.RouteAll true value (to accept subnets), but Linux users have always gotten a "false" value for that, because that's what cmd/tailscale's CLI default flag is _for all operating systems_. That meant that "tailscale up" was rightfully reporting that the user was changing an implicit setting: RouteAll was changing from true with false with the user explicitly saying so. An obvious fix might be to change ipn.NewPrefs to return Prefs.RouteAll == false on some platforms, but the logic is complicated by darwin: we want RouteAll true on windows, android, ios, and the GUI mac app, but not the CLI tailscaled-on-macOS mode. But even if we used build tags (e.g. the "redo" build tag) to determine what the default is, that then means we have duplicated and differing "defaults" between both the CLI up flags and ipn.NewPrefs. Furthering that complication didn't seem like a good idea. So, changing the NewPrefs defaults is too invasive at this stage of the release, as is removing the NewPrefs func entirely. Instead, tweak slightly the semantics of the ipn.Prefs.ControlURL field. This now defines that a ControlURL of the empty string means both "we're uninitialized" and also "just use the default". Then, once we have the "empty-string-means-unintialized" semantics, use that to suppress "tailscale up"'s recent implicit-setting-revert checking safety net, if we've never initialized Tailscale yet. And update/add tests. Fixes #1725 Signed-off-by: Brad Fitzpatrick --- cmd/tailscale/cli/cli_test.go | 41 +++++++++++++++++++++++++++++++++++ cmd/tailscale/cli/up.go | 4 ++++ ipn/fake_test.go | 2 +- ipn/handle.go | 2 +- ipn/ipnlocal/local.go | 11 +++++++++- ipn/prefs.go | 28 ++++++++++++++++++++---- 6 files changed, 81 insertions(+), 7 deletions(-) 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.