From 4ef207833b556ec6f01fc8670ab3f5f9f4c4c1c6 Mon Sep 17 00:00:00 2001
From: Avery Pennarun <apenwarr@tailscale.com>
Date: Fri, 30 Apr 2021 05:27:37 -0400
Subject: [PATCH] ipn: !WantRunning + !LoggedOut should not be idle on startup.

There was logic that would make a "down" tailscale backend (ie.
!WantRunning) refuse to do any network activity. Unfortunately, this
makes the macOS and iOS UI unable to render correctly if they start
while !WantRunning.

Now that we have Prefs.LoggedOut, use that instead. So `tailscale down`
will still allow the controlclient to connect its authroutine, but
pause the maproutine. `tailscale logout` will entirely stop all
activity.

This new behaviour is not obviously correct; it's a bit annoying that
`tailsale down` doesn't terminate all activity like you might expect.
Maybe we should redesign the UI code to render differently when
disconnected, and then revert this change.

Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>
---
 ipn/ipnlocal/local.go      | 27 +++++++++++++++------------
 ipn/ipnlocal/state_test.go | 20 +++++++++++---------
 2 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go
index c5a9413ba..9f88a3f37 100644
--- a/ipn/ipnlocal/local.go
+++ b/ipn/ipnlocal/local.go
@@ -809,17 +809,13 @@ func (b *LocalBackend) Start(opts ipn.Options) error {
 	b.send(ipn.Notify{BackendLogID: &blid})
 	b.send(ipn.Notify{Prefs: prefs})
 
-	if wantRunning && !loggedOut {
+	if !loggedOut && b.hasNodeKey() {
+		// Even if !WantRunning, we should verify our key, if there
+		// is one. If you want tailscaled to be completely idle,
+		// use logout instead.
 		cc.Login(nil, controlclient.LoginDefault)
-
-		b.mu.Lock()
-		b.state = ipn.Starting
-		b.mu.Unlock()
-
-		b.send(ipn.Notify{State: &b.state})
-	} else {
-		b.stateMachine()
 	}
+	b.stateMachine()
 	return nil
 }
 
@@ -2125,6 +2121,15 @@ func (b *LocalBackend) enterState(newState ipn.State) {
 
 }
 
+func (b *LocalBackend) hasNodeKey() bool {
+	// we can't use b.Prefs(), because it strips the keys, oops!
+	b.mu.Lock()
+	p := b.prefs
+	b.mu.Unlock()
+
+	return p.Persist != nil && !p.Persist.PrivateNodeKey.IsZero()
+}
+
 // nextState returns the state the backend seems to be in, based on
 // its internal state.
 func (b *LocalBackend) nextState() ipn.State {
@@ -2136,13 +2141,11 @@ func (b *LocalBackend) nextState() ipn.State {
 		state       = b.state
 		wantRunning = b.prefs.WantRunning
 		loggedOut   = b.prefs.LoggedOut
-		hasNodeKey  = b.prefs.Persist != nil &&
-			!b.prefs.Persist.PrivateNodeKey.IsZero()
 	)
 	b.mu.Unlock()
 
 	switch {
-	case !wantRunning && !loggedOut && hasNodeKey:
+	case !wantRunning && !loggedOut && b.hasNodeKey():
 		return ipn.Stopped
 	case netMap == nil:
 		if cc.AuthCantContinue() || loggedOut {
diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go
index a2c1b2e9a..cb2015c16 100644
--- a/ipn/ipnlocal/state_test.go
+++ b/ipn/ipnlocal/state_test.go
@@ -283,8 +283,10 @@ func TestStateMachine(t *testing.T) {
 		cc.opts = opts
 		cc.logf = opts.Logf
 		cc.authBlocked = true
+		cc.persist = cc.opts.Persist
 		cc.mu.Unlock()
 
+		cc.logf("ccGen: new mockControl.")
 		cc.called("New")
 		return cc, nil
 	}
@@ -749,7 +751,7 @@ func TestStateMachine(t *testing.T) {
 		// NOTE: cc.Shutdown() is correct here, since we didn't call
 		// b.Shutdown() explicitly ourselves.
 		// BUG: UpdateEndpoints should be called here since we're not WantRunning.
-		assert.Equal([]string{"Shutdown", "New", "UpdateEndpoints", "pause"}, cc.getCalls())
+		assert.Equal([]string{"Shutdown", "New", "UpdateEndpoints", "Login", "pause"}, cc.getCalls())
 
 		nn := notifies.drain(2)
 		assert.Equal([]string{}, cc.getCalls())
@@ -780,7 +782,7 @@ func TestStateMachine(t *testing.T) {
 	// The last test case is the most common one: restarting when both
 	// logged in and WantRunning.
 	t.Logf("\n\nStart5")
-	notifies.expect(2)
+	notifies.expect(1)
 	assert.Nil(b.Start(ipn.Options{
 		StateKey: ipn.GlobalDaemonStateKey,
 	}))
@@ -789,27 +791,27 @@ func TestStateMachine(t *testing.T) {
 		// b.Shutdown() ourselves.
 		assert.Equal([]string{"Shutdown", "New", "UpdateEndpoints", "Login"}, cc.getCalls())
 
-		nn := notifies.drain(2)
+		nn := notifies.drain(1)
 		assert.Equal([]string{}, cc.getCalls())
 		assert.NotNil(nn[0].Prefs)
 		assert.Equal(false, nn[0].Prefs.LoggedOut)
 		assert.Equal(true, nn[0].Prefs.WantRunning)
-		assert.NotNil(nn[1].State)
-		assert.Equal(ipn.Starting, *nn[1].State)
-		assert.Equal(ipn.Starting, b.State())
+		assert.Equal(ipn.NoState, b.State())
 	}
 
 	// Control server accepts our valid key from before.
 	t.Logf("\n\nLoginFinished5")
-	notifies.expect(1)
+	notifies.expect(2)
 	cc.setAuthBlocked(false)
 	cc.send(nil, "", true, &netmap.NetworkMap{
 		MachineStatus: tailcfg.MachineAuthorized,
 	})
 	{
-		nn := notifies.drain(1)
-		assert.Equal([]string{}, cc.getCalls())
+		nn := notifies.drain(2)
+		assert.Equal([]string{"unpause"}, cc.getCalls())
 		assert.NotNil(nn[0].LoginFinished)
+		assert.NotNil(nn[1].State)
+		assert.Equal(ipn.Starting, *nn[1].State)
 		// NOTE: No prefs change this time. WantRunning stays true.
 		// We were in Starting in the first place, so that doesn't
 		// change either.