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 <bradfitz@tailscale.com>

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

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)
 

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")
 	}

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() {

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" {

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.