From 5ecc7c7200bda43f02f9a04fb684ad4f3614c48a Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 26 Apr 2021 14:39:49 -0700 Subject: [PATCH] cmd/tailscale: make the new 'up' errors prettier and more helpful Fixes #1746 Signed-off-by: Brad Fitzpatrick --- cmd/tailscale/cli/cli_test.go | 70 ++++++++++++++++++++++++++++++--- cmd/tailscale/cli/up.go | 74 +++++++++++++++++++++++++++++------ cmd/tailscale/depaware.txt | 2 +- go.mod | 1 + go.sum | 2 + 5 files changed, 130 insertions(+), 19 deletions(-) diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index 70b163824..fbfa4cffc 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/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) } }) diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index bdfde2d3c..c53ce0946 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -17,7 +17,7 @@ "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 { - didExitNodeErr = true - 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")) + if prefName == "ExitNodeIP" { + missing = append(missing, fmtFlagValueArg("exit-node", fmtSettingVal(exi))) } 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", - flagName, fmtSettingVal(imi), fmtSettingVal(exi))) + missing = append(missing, fmtFlagValueArg(flagName, 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: - return fmt.Sprintf("%q", v) + case string: + 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) } diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index 2b2fa40ee..c95c3b495 100644 --- a/cmd/tailscale/depaware.txt +++ b/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 diff --git a/go.mod b/go.mod index 0c20cbe70..2653e203b 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 5d8b47883..7718353be 100644 --- a/go.sum +++ b/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=