cmd/tailscale: make the new 'up' errors prettier and more helpful

Fixes #1746

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

cmd/tailscale/cli/cli_test.go

@@ -79,7 +79,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 				WantRunningSet: true,
 				CorpDNSSet:     true,
 			},
-			want: `'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --hostname is not specified but its default value of "" differs from current value "foo"`,
+			want: accidentalUpPrefix + " --accept-dns --hostname=foo",
 		},
 		{
 			name:    "hostname_changing_explicitly",
@@ -163,7 +163,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 				},
 				ControlURLSet: true,
 			},
-			want: `'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --operator is not specified but its default value of "" differs from current value "alice"`,
+			want: accidentalUpPrefix + " --hostname= --operator=alice",
 		},
 		{
 			name:    "implicit_operator_matches_shell_user",
@@ -201,7 +201,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 				},
 				AdvertiseRoutesSet: true,
 			},
-			want: "'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --advertise-exit-node flag not mentioned but currently advertised routes are an exit node",
+			want: accidentalUpPrefix + " --advertise-routes=10.0.42.0/24 --advertise-exit-node",
 		},
 		{
 			name:    "advertised_routes_exit_node_removed",
@@ -270,7 +270,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 				},
 				AdvertiseRoutesSet: true,
 			},
-			want: "'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --advertise-exit-node flag not mentioned but currently advertised routes are an exit node",
+			want: accidentalUpPrefix + " --advertise-routes=11.1.43.0/24,0.0.0.0/0 --advertise-exit-node",
 		},
 		{
 			name:    "exit_node_clearing", // Issue 1777
@@ -288,6 +288,66 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 			},
 			want: "",
 		},
+		{
+			name:    "remove_all_implicit",
+			flagSet: f("force-reauth"),
+			curPrefs: &ipn.Prefs{
+				WantRunning:      true,
+				ControlURL:       ipn.DefaultControlURL,
+				RouteAll:         true,
+				AllowSingleHosts: false,
+				ExitNodeIP:       netaddr.MustParseIP("100.64.5.6"),
+				CorpDNS:          true,
+				ShieldsUp:        true,
+				AdvertiseTags:    []string{"tag:foo", "tag:bar"},
+				Hostname:         "myhostname",
+				ForceDaemon:      true,
+				AdvertiseRoutes: []netaddr.IPPrefix{
+					netaddr.MustParseIPPrefix("10.0.0.0/16"),
+				},
+				NetfilterMode: preftype.NetfilterNoDivert,
+				OperatorUser:  "alice",
+			},
+			curUser: "eve",
+			mp: &ipn.MaskedPrefs{
+				Prefs: ipn.Prefs{
+					ControlURL:  ipn.DefaultControlURL,
+					WantRunning: true,
+				},
+			},
+			want: accidentalUpPrefix + " --accept-routes --exit-node=100.64.5.6 --accept-dns --shields-up --advertise-tags=tag:foo,tag:bar --hostname=myhostname --unattended --advertise-routes=10.0.0.0/16 --netfilter-mode=nodivert --operator=alice",
+		},
+		{
+			name:    "remove_all_implicit_except_hostname",
+			flagSet: f("hostname"),
+			curPrefs: &ipn.Prefs{
+				WantRunning:      true,
+				ControlURL:       ipn.DefaultControlURL,
+				RouteAll:         true,
+				AllowSingleHosts: false,
+				ExitNodeIP:       netaddr.MustParseIP("100.64.5.6"),
+				CorpDNS:          true,
+				ShieldsUp:        true,
+				AdvertiseTags:    []string{"tag:foo", "tag:bar"},
+				Hostname:         "myhostname",
+				ForceDaemon:      true,
+				AdvertiseRoutes: []netaddr.IPPrefix{
+					netaddr.MustParseIPPrefix("10.0.0.0/16"),
+				},
+				NetfilterMode: preftype.NetfilterNoDivert,
+				OperatorUser:  "alice",
+			},
+			curUser: "eve",
+			mp: &ipn.MaskedPrefs{
+				Prefs: ipn.Prefs{
+					ControlURL:  ipn.DefaultControlURL,
+					WantRunning: true,
+					Hostname:    "newhostname",
+				},
+				HostnameSet: true,
+			},
+			want: accidentalUpPrefix + " --hostname=newhostname --accept-routes --exit-node=100.64.5.6 --accept-dns --shields-up --advertise-tags=tag:foo,tag:bar --unattended --advertise-routes=10.0.0.0/16 --netfilter-mode=nodivert --operator=alice",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -295,7 +355,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 			if err := checkForAccidentalSettingReverts(tt.flagSet, tt.curPrefs, tt.mp, tt.curUser); err != nil {
 				got = err.Error()
 			}
-			if got != tt.want {
+			if strings.TrimSpace(got) != tt.want {
 				t.Errorf("unexpected result\n got: %s\nwant: %s\n", got, tt.want)
 			}
 		})

cmd/tailscale/cli/up.go

@@ -17,7 +17,7 @@ import (
 	"strings"
 	"sync"
 
-	"github.com/go-multierror/multierror"
+	shellquote "github.com/kballard/go-shellquote"
 	"github.com/peterbourgon/ff/v2/ffcli"
 	"inet.af/netaddr"
 	"tailscale.com/client/tailscale"
@@ -508,6 +508,12 @@ func updateMaskedPrefsFromUpFlag(mp *ipn.MaskedPrefs, flagName string) {
 	}
 }
 
+const accidentalUpPrefix = "Error: changing settings via 'tailscale up' requires mentioning all\n" +
+	"non-default flags. To proceed, either re-run your command with --reset or\n" +
+	"specify use the command below to explicitly mention the current value of\n" +
+	"all non-default settings:\n\n" +
+	"\ttailscale up"
+
 // checkForAccidentalSettingReverts checks for people running
 // "tailscale up" with a subset of the flags they originally ran it
 // with.
@@ -541,8 +547,9 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre
 	ev := reflect.ValueOf(curWithExplicitEdits).Elem()
 	// Implicit values (what we'd get if we replaced everything with flag defaults):
 	iv := reflect.ValueOf(&mp.Prefs).Elem()
-	var errs []error
+
-	var didExitNodeErr bool
+	var missing []string
+	flagExplicitValue := map[string]interface{}{} // e.g. "accept-dns" => true (from flagSet)
 	for i := 0; i < prefType.NumField(); i++ {
 		prefName := prefType.Field(i).Name
 		if prefName == "Persist" {
@@ -558,10 +565,11 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre
 			hasExitNodeRoutes(curPrefs.AdvertiseRoutes) &&
 			!hasExitNodeRoutes(curWithExplicitEdits.AdvertiseRoutes) &&
 			!flagSet["advertise-exit-node"] {
-			errs = append(errs, errors.New("'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --advertise-exit-node flag not mentioned but currently advertised routes are an exit node"))
+			missing = append(missing, "--advertise-exit-node")
 		}
 
 		if hasFlag && flagSet[flagName] {
+			flagExplicitValue[flagName] = ev.Field(i).Interface()
 			continue
 		}
 		// Get explicit value and implicit value
@@ -585,28 +593,68 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre
 		}
 		switch flagName {
 		case "":
-			errs = append(errs, fmt.Errorf("'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; this command would change the value of flagless pref %q", prefName))
+			return fmt.Errorf("'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; this command would change the value of flagless pref %q", prefName)
 		case "exit-node":
-			if !didExitNodeErr {
+			if prefName == "ExitNodeIP" {
-				didExitNodeErr = true
+				missing = append(missing, fmtFlagValueArg("exit-node", fmtSettingVal(exi)))
-				errs = append(errs, errors.New("'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --exit-node is not specified but an exit node is currently configured"))
 			}
 		default:
-			errs = append(errs, fmt.Errorf("'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --%s is not specified but its default value of %v differs from current value %v",
+			missing = append(missing, fmtFlagValueArg(flagName, fmtSettingVal(exi)))
-				flagName, fmtSettingVal(imi), fmtSettingVal(exi)))
 		}
 	}
-	return multierror.New(errs)
+	if len(missing) == 0 {
+		return nil
+	}
+	var sb strings.Builder
+	sb.WriteString(accidentalUpPrefix)
+
+	var flagSetSorted []string
+	for f := range flagSet {
+		flagSetSorted = append(flagSetSorted, f)
+	}
+	sort.Strings(flagSetSorted)
+	for _, flagName := range flagSetSorted {
+		if ev, ok := flagExplicitValue[flagName]; ok {
+			fmt.Fprintf(&sb, " %s", fmtFlagValueArg(flagName, fmtSettingVal(ev)))
+		}
+	}
+	for _, a := range missing {
+		fmt.Fprintf(&sb, " %s", a)
+	}
+	sb.WriteString("\n\n")
+	return errors.New(sb.String())
+}
+
+func fmtFlagValueArg(flagName, val string) string {
+	if val == "true" {
+		// TODO: check flagName's type to see if its Pref is of type bool
+		return "--" + flagName
+	}
+	if val == "" {
+		return "--" + flagName + "="
+	}
+	return fmt.Sprintf("--%s=%v", flagName, shellquote.Join(val))
 }
 
 func fmtSettingVal(v interface{}) string {
 	switch v := v.(type) {
 	case bool:
 		return strconv.FormatBool(v)
-	case string, preftype.NetfilterMode:
+	case string:
-		return fmt.Sprintf("%q", v)
+		return v
+	case preftype.NetfilterMode:
+		return v.String()
 	case []string:
 		return strings.Join(v, ",")
+	case []netaddr.IPPrefix:
+		var sb strings.Builder
+		for i, r := range v {
+			if i > 0 {
+				sb.WriteByte(',')
+			}
+			sb.WriteString(r.String())
+		}
+		return sb.String()
 	}
 	return fmt.Sprint(v)
 }

cmd/tailscale/depaware.txt

@@ -2,7 +2,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
 
    W 💣 github.com/alexbrainman/sspi                                 from github.com/alexbrainman/sspi/negotiate
    W 💣 github.com/alexbrainman/sspi/negotiate                       from tailscale.com/net/tshttpproxy
-        github.com/go-multierror/multierror                          from tailscale.com/cmd/tailscale/cli
+        github.com/kballard/go-shellquote                            from tailscale.com/cmd/tailscale/cli
         github.com/peterbourgon/ff/v2                                from github.com/peterbourgon/ff/v2/ffcli
         github.com/peterbourgon/ff/v2/ffcli                          from tailscale.com/cmd/tailscale/cli
         github.com/tcnksm/go-httpstat                                from tailscale.com/net/netcheck

go.mod

@@ -15,6 +15,7 @@ require (
 	github.com/google/go-cmp v0.5.4
 	github.com/goreleaser/nfpm v1.1.10
 	github.com/jsimonetti/rtnetlink v0.0.0-20210212075122-66c871082f2b
+	github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 // indirect
 	github.com/klauspost/compress v1.10.10
 	github.com/kr/pty v1.1.8
 	github.com/mdlayher/netlink v1.3.2

go.sum

@@ -61,6 +61,8 @@ github.com/jsimonetti/rtnetlink v0.0.0-20201220180245-69540ac93943/go.mod h1:z4c
 github.com/jsimonetti/rtnetlink v0.0.0-20210122163228-8d122574c736/go.mod h1:ZXpIyOK59ZnN7J0BV99cZUPmsqDRZ3eq5X+st7u/oSA=
 github.com/jsimonetti/rtnetlink v0.0.0-20210212075122-66c871082f2b h1:c3NTyLNozICy8B4mlMXemD3z/gXgQzVXZS/HqT+i3do=
 github.com/jsimonetti/rtnetlink v0.0.0-20210212075122-66c871082f2b/go.mod h1:8w9Rh8m+aHZIG69YPGGem1i5VzoyRC8nw2kA8B+ik5U=
+github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 h1:Z9n2FFNUXsshfwJMBgNA0RU6/i7WVaAegv3PtuIHPMs=
+github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51/go.mod h1:CzGEWj7cYgsdH8dAjBGEr58BoE7ScuLd+fwFZ44+/x8=
 github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck=
 github.com/klauspost/compress v1.10.10 h1:a/y8CglcM7gLGYmlbP/stPE5sR3hbhFRUjCBfd/0B3I=
 github.com/klauspost/compress v1.10.10/go.mod h1:aoV0uJVorq1K+umq18yTdKaF57EivdYsUV+/s2qKfXs=