From 02bdc654d5d7d066b6abea957b36a48961ffad60 Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@tailscale.com>
Date: Thu, 27 Jan 2022 14:20:51 -0800
Subject: [PATCH] cmd/tailscale: fix up --reset, again

Also fix a somewhat related printing bug in the process where
some paths would print "Success." inconsistently even
when there otherwise was no output (in the EditPrefs path)

Fixes #3830
Updates #3702 (which broke it once while trying to fix it)

Change-Id: Ic51e14526ad75be61ba00084670aa6a98221daa5
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
---
 cmd/tailscale/cli/cli_test.go      | 30 +++++++++++++++++++++++++++---
 cmd/tailscale/cli/up.go            | 13 +++++++++----
 tstest/integration/vms/vms_test.go |  1 +
 3 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go
index 7f00539e5..a7b2b5ce9 100644
--- a/cmd/tailscale/cli/cli_test.go
+++ b/cmd/tailscale/cli/cli_test.go
@@ -793,8 +793,25 @@ func TestUpdatePrefs(t *testing.T) {
 				ControlURL: ipn.DefaultControlURL,
 				Persist:    &persist.Persist{LoginName: "crawshaw.github"},
 			},
-			env:            upCheckEnv{backendState: "Running"},
-			wantJustEditMP: &ipn.MaskedPrefs{WantRunningSet: true},
+			env: upCheckEnv{backendState: "Running"},
+			wantJustEditMP: &ipn.MaskedPrefs{
+				AdvertiseRoutesSet:        true,
+				AdvertiseTagsSet:          true,
+				AllowSingleHostsSet:       true,
+				ControlURLSet:             true,
+				CorpDNSSet:                true,
+				ExitNodeAllowLANAccessSet: true,
+				ExitNodeIDSet:             true,
+				ExitNodeIPSet:             true,
+				HostnameSet:               true,
+				NetfilterModeSet:          true,
+				NoSNATSet:                 true,
+				OperatorUserSet:           true,
+				RouteAllSet:               true,
+				RunSSHSet:                 true,
+				ShieldsUpSet:              true,
+				WantRunningSet:            true,
+			},
 		},
 		{
 			name:  "control_synonym",
@@ -860,16 +877,23 @@ func TestUpdatePrefs(t *testing.T) {
 			if simpleUp != tt.wantSimpleUp {
 				t.Fatalf("simpleUp=%v, want %v", simpleUp, tt.wantSimpleUp)
 			}
+			var oldEditPrefs ipn.Prefs
 			if justEditMP != nil {
+				oldEditPrefs = justEditMP.Prefs
 				justEditMP.Prefs = ipn.Prefs{} // uninteresting
 			}
 			if !reflect.DeepEqual(justEditMP, tt.wantJustEditMP) {
-				t.Fatalf("justEditMP: %v", cmp.Diff(justEditMP, tt.wantJustEditMP))
+				t.Logf("justEditMP != wantJustEditMP; following diff omits the Prefs field, which was %+v", oldEditPrefs)
+				t.Fatalf("justEditMP: %v\n\n: ", cmp.Diff(justEditMP, tt.wantJustEditMP, cmpIP))
 			}
 		})
 	}
 }
 
+var cmpIP = cmp.Comparer(func(a, b netaddr.IP) bool {
+	return a == b
+})
+
 func TestExitNodeIPOfArg(t *testing.T) {
 	mustIP := netaddr.MustParseIP
 	tests := []struct {
diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go
index 6eb720de2..c6e1296c9 100644
--- a/cmd/tailscale/cli/up.go
+++ b/cmd/tailscale/cli/up.go
@@ -387,7 +387,8 @@ func prefsFromUpArgs(upArgs upArgsT, warnf logger.Logf, st *ipnstate.Status, goo
 	return prefs, nil
 }
 
-// updatePrefs updates prefs based on curPrefs
+// updatePrefs returns how to edit preferences based on the
+// flag-provided 'prefs' and the currently active 'curPrefs'.
 //
 // It returns a non-nil justEditMP if we're already running and none of
 // the flags require a restart, so we can just do an EditPrefs call and
@@ -424,11 +425,16 @@ func updatePrefs(prefs, curPrefs *ipn.Prefs, env upCheckEnv) (simpleUp bool, jus
 		env.upArgs.authKeyOrFile == "" &&
 		!controlURLChanged &&
 		!tagsChanged
+
 	if justEdit {
 		justEditMP = new(ipn.MaskedPrefs)
 		justEditMP.WantRunningSet = true
 		justEditMP.Prefs = *prefs
-		env.flagSet.Visit(func(f *flag.Flag) {
+		visitFlags := env.flagSet.Visit
+		if env.upArgs.reset {
+			visitFlags = env.flagSet.VisitAll
+		}
+		visitFlags(func(f *flag.Flag) {
 			updateMaskedPrefsFromUpFlag(justEditMP, f.Name)
 		})
 	}
@@ -520,7 +526,7 @@ func runUp(ctx context.Context, args []string) error {
 	pumpErr := make(chan error, 1)
 	go func() { pumpErr <- pump(pumpCtx, bc, c) }()
 
-	printed := !simpleUp
+	var printed bool // whether we've yet printed anything to stdout or stderr
 	var loginOnce sync.Once
 	startLoginInteractive := func() { loginOnce.Do(func() { bc.StartLoginInteractive() }) }
 
@@ -546,7 +552,6 @@ func runUp(ctx context.Context, args []string) error {
 		if s := n.State; s != nil {
 			switch *s {
 			case ipn.NeedsLogin:
-				printed = true
 				startLoginInteractive()
 			case ipn.NeedsMachineAuth:
 				printed = true
diff --git a/tstest/integration/vms/vms_test.go b/tstest/integration/vms/vms_test.go
index b6df16f80..f0aef3e22 100644
--- a/tstest/integration/vms/vms_test.go
+++ b/tstest/integration/vms/vms_test.go
@@ -376,6 +376,7 @@ func (h *Harness) testDistro(t *testing.T, d Distro, ipm ipMapping) {
 	t.Run("login", func(t *testing.T) {
 		runTestCommands(t, timeout, cli, []expect.Batcher{
 			&expect.BSnd{S: fmt.Sprintf("tailscale up --login-server=%s\n", loginServer)},
+			&expect.BSnd{S: "echo Success.\n"},
 			&expect.BExp{R: `Success.`},
 		})
 	})