From f3d2fd22ef8c2d6df90a9d3ae7efc0105c9a00c1 Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@tailscale.com>
Date: Mon, 6 May 2024 14:10:17 -0700
Subject: [PATCH] cmd/tailscale/cli: don't start WatchIPNBus until after up's
 initial Start

The CLI "up" command is a historical mess, both on the CLI side and
the LocalBackend side. We're getting closer to cleaning it up, but in
the meantime it was again implicated in flaky tests.

In this case, the background goroutine running WatchIPNBus was very
occasionally running enough to get to its StartLoginInteractive call
before the original goroutine did its Start call. That meant
integration tests were very rarely but sometimes logging in with the
default control plane URL out on the internet
(controlplane.tailscale.com) instead of the localhost control server
for tests.

This also might've affected new Headscale etc users on initial "up".

Fixes #11960
Fixes #11962

Change-Id: I36f8817b69267a99271b5ee78cb7dbf0fcc0bd34
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
---
 cmd/tailscale/cli/up.go | 109 +++++++++++++++++-----------------------
 1 file changed, 45 insertions(+), 64 deletions(-)

diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go
index dc7fcb0c3..bc5fd0ab2 100644
--- a/cmd/tailscale/cli/up.go
+++ b/cmd/tailscale/cli/up.go
@@ -20,7 +20,6 @@ import (
 	"sort"
 	"strconv"
 	"strings"
-	"sync"
 	"syscall"
 	"time"
 
@@ -477,11 +476,6 @@ func runUp(ctx context.Context, cmd string, args []string, upArgs upArgsT) (retE
 
 	watchCtx, cancelWatch := context.WithCancel(ctx)
 	defer cancelWatch()
-	watcher, err := localClient.WatchIPNBus(watchCtx, 0)
-	if err != nil {
-		return err
-	}
-	defer watcher.Close()
 
 	go func() {
 		interrupt := make(chan os.Signal, 1)
@@ -494,29 +488,57 @@ func runUp(ctx context.Context, cmd string, args []string, upArgs upArgsT) (retE
 	}()
 
 	running := make(chan bool, 1) // gets value once in state ipn.Running
-	pumpErr := make(chan error, 1)
+	watchErr := make(chan error, 1)
 
-	// localAPIMu should be held while doing mutable LocalAPI calls
-	// to the backend. In particular, it prevents StartLoginInteractive from
-	// being called from the watcher goroutine while the Start call from
-	// the other goroutine is in progress.
-	// See https://github.com/tailscale/tailscale/issues/7036#issuecomment-2053771466
-	// TODO(bradfitz): simplify this once #11649 is cleaned up and Start is
-	// hopefully removed.
-	var localAPIMu sync.Mutex
+	// Special case: bare "tailscale up" means to just start
+	// running, if there's ever been a login.
+	if simpleUp {
+		_, err := localClient.EditPrefs(ctx, &ipn.MaskedPrefs{
+			Prefs: ipn.Prefs{
+				WantRunning: true,
+			},
+			WantRunningSet: true,
+		})
+		if err != nil {
+			return err
+		}
+	} else {
+		if err := localClient.CheckPrefs(ctx, prefs); err != nil {
+			return err
+		}
 
-	startLoginInteractive := sync.OnceFunc(func() {
-		localAPIMu.Lock()
-		defer localAPIMu.Unlock()
-		localClient.StartLoginInteractive(ctx)
-	})
+		authKey, err := upArgs.getAuthKey()
+		if err != nil {
+			return err
+		}
+		authKey, err = resolveAuthKey(ctx, authKey, upArgs.advertiseTags)
+		if err != nil {
+			return err
+		}
+		err = localClient.Start(ctx, ipn.Options{
+			AuthKey:     authKey,
+			UpdatePrefs: prefs,
+		})
+		if err != nil {
+			return err
+		}
+		if upArgs.forceReauth {
+			localClient.StartLoginInteractive(ctx)
+		}
+	}
+
+	watcher, err := localClient.WatchIPNBus(watchCtx, ipn.NotifyInitialState)
+	if err != nil {
+		return err
+	}
+	defer watcher.Close()
 
 	go func() {
 		var printed bool // whether we've yet printed anything to stdout or stderr
 		for {
 			n, err := watcher.Next()
 			if err != nil {
-				pumpErr <- err
+				watchErr <- err
 				return
 			}
 			if n.ErrMessage != nil {
@@ -526,7 +548,7 @@ func runUp(ctx context.Context, cmd string, args []string, upArgs upArgsT) (retE
 			if s := n.State; s != nil {
 				switch *s {
 				case ipn.NeedsLogin:
-					startLoginInteractive()
+					localClient.StartLoginInteractive(ctx)
 				case ipn.NeedsMachineAuth:
 					printed = true
 					if env.upArgs.json {
@@ -583,47 +605,6 @@ func runUp(ctx context.Context, cmd string, args []string, upArgs upArgsT) (retE
 		}
 	}()
 
-	// Special case: bare "tailscale up" means to just start
-	// running, if there's ever been a login.
-	if simpleUp {
-		localAPIMu.Lock()
-		_, err := localClient.EditPrefs(ctx, &ipn.MaskedPrefs{
-			Prefs: ipn.Prefs{
-				WantRunning: true,
-			},
-			WantRunningSet: true,
-		})
-		localAPIMu.Unlock()
-		if err != nil {
-			return err
-		}
-	} else {
-		if err := localClient.CheckPrefs(ctx, prefs); err != nil {
-			return err
-		}
-
-		authKey, err := upArgs.getAuthKey()
-		if err != nil {
-			return err
-		}
-		authKey, err = resolveAuthKey(ctx, authKey, upArgs.advertiseTags)
-		if err != nil {
-			return err
-		}
-		localAPIMu.Lock()
-		err = localClient.Start(ctx, ipn.Options{
-			AuthKey:     authKey,
-			UpdatePrefs: prefs,
-		})
-		localAPIMu.Unlock()
-		if err != nil {
-			return err
-		}
-		if upArgs.forceReauth {
-			startLoginInteractive()
-		}
-	}
-
 	// This whole 'up' mechanism is too complicated and results in
 	// hairy stuff like this select. We're ultimately waiting for
 	// 'running' to be done, but even in the case where
@@ -647,7 +628,7 @@ func runUp(ctx context.Context, cmd string, args []string, upArgs upArgsT) (retE
 		default:
 		}
 		return watchCtx.Err()
-	case err := <-pumpErr:
+	case err := <-watchErr:
 		select {
 		case <-running:
 			return nil