From 5190435d6e7914c656c0a9cb93dd98e0ee35727c Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 6 May 2021 21:25:16 -0700 Subject: [PATCH] cmd/tailscale: rewrite the "up" checker, fix bugs The old way was way too fragile and had felt like it had more special cases than normal cases. (see #1874, #1860, #1834, etc) It became very obvious the old algorithm didn't work when we made the output be pretty and try to show the user the command they need to run in 5ecc7c7200bda43f02f9a04fb684ad4f3614c48a for #1746) The new algorithm is to map the prefs (current and new) back to flags and then compare flags. This nicely handles the OS-specific flags and the n:1 and 1:n flag:pref cases. No change in the existing already-massive test suite, except some ordering differences (the missing items are now sorted), but some new tests are added for behavior that was broken before. In particular, it now: * preserves non-pref boolean flags set to false, and preserves exit node IPs (mapping them back from the ExitNodeID pref, as well as ExitNodeIP), * doesn't ignore --advertise-exit-node when doing an EditPrefs call (#1880) * doesn't lose the --operator on the non-EditPrefs paths (e.g. with --force-reauth, or when the backend was not in state Running). Fixes #1880 Signed-off-by: Brad Fitzpatrick --- cmd/tailscale/cli/cli_test.go | 364 ++++++++++++++++----------------- cmd/tailscale/cli/up.go | 369 +++++++++++++++++++--------------- 2 files changed, 375 insertions(+), 358 deletions(-) diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index ed42013c9..f8372d642 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/cmd/tailscale/cli/cli_test.go @@ -16,43 +16,46 @@ import ( "inet.af/netaddr" "tailscale.com/ipn" "tailscale.com/ipn/ipnstate" - "tailscale.com/types/logger" "tailscale.com/types/preftype" ) +// geese is a collection of gooses. It need not be complete. +// But it should include anything handled specially (e.g. linux, windows) +// and at least one thing that's not (darwin, freebsd). +var geese = []string{"linux", "darwin", "windows", "freebsd"} + // Test that checkForAccidentalSettingReverts's updateMaskedPrefsFromUpFlag can handle // all flags. This will panic if a new flag creeps in that's unhandled. +// +// Also, issue 1880: advertise-exit-node was being ignored. Verify that all flags cause an edit. func TestUpdateMaskedPrefsFromUpFlag(t *testing.T) { - mp := new(ipn.MaskedPrefs) - upFlagSet.VisitAll(func(f *flag.Flag) { - updateMaskedPrefsFromUpFlag(mp, f.Name) - }) + for _, goos := range geese { + var upArgs upArgsT + fs := newUpFlagSet(goos, &upArgs) + fs.VisitAll(func(f *flag.Flag) { + mp := new(ipn.MaskedPrefs) + updateMaskedPrefsFromUpFlag(mp, f.Name) + got := mp.Pretty() + wantEmpty := preflessFlag(f.Name) + isEmpty := got == "MaskedPrefs{}" + if isEmpty != wantEmpty { + t.Errorf("flag %q created MaskedPrefs %s; want empty=%v", f.Name, got, wantEmpty) + } + }) + } } func TestCheckForAccidentalSettingReverts(t *testing.T) { - f := func(flags ...string) map[string]bool { - m := make(map[string]bool) - for _, f := range flags { - m[f] = true - } - return m - } tests := []struct { - name string - - // flags, if non-nil populates flagSet and mp. - flags []string // if non-nil, + name string + flags []string // argv to be parsed by FlagSet + curPrefs *ipn.Prefs - // flagSet and mp are required if flags is nil. - // flagSet is the set of flags that were explicitly set in flags. - // mp is the mutation to apply to server. - flagSet map[string]bool - mp *ipn.MaskedPrefs + curExitNodeIP netaddr.IP + curUser string // os.Getenv("USER") on the client side + goos string // empty means "linux" - curPrefs *ipn.Prefs - curUser string // os.Getenv("USER") on the client side - goos string // empty means "linux" - want string + want string }{ { name: "bare_up_means_up", @@ -78,58 +81,26 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { want: accidentalUpPrefix + " --accept-dns --hostname=foo", }, { - name: "hostname_changing_explicitly", - flagSet: f("hostname"), + name: "hostname_changing_explicitly", + flags: []string{"--hostname=bar"}, curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - WantRunning: false, - Hostname: "foo", - }, - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - WantRunning: true, - Hostname: "bar", - }, - ControlURLSet: true, - WantRunningSet: true, - HostnameSet: true, - }, - want: "", - }, - { - name: "hostname_changing_empty_explicitly", - flagSet: f("hostname"), - curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - WantRunning: false, - Hostname: "foo", - }, - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - WantRunning: true, - Hostname: "", - }, - ControlURLSet: true, - WantRunningSet: true, - HostnameSet: true, + ControlURL: ipn.DefaultControlURL, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, + AllowSingleHosts: true, + Hostname: "foo", }, want: "", }, { - name: "empty_slice_equals_nil_slice", - flagSet: f("hostname"), + name: "hostname_changing_empty_explicitly", + flags: []string{"--hostname="}, curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - AdvertiseRoutes: []netaddr.IPPrefix{}, - }, - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - AdvertiseRoutes: nil, - }, - ControlURLSet: true, + ControlURL: ipn.DefaultControlURL, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, + AllowSingleHosts: true, + Hostname: "foo", }, want: "", }, @@ -137,45 +108,35 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { // Issue 1725: "tailscale up --authkey=..." (or other non-empty flags) works from // a fresh server's initial prefs. name: "up_with_default_prefs", - flagSet: f("authkey"), + flags: []string{"--authkey=foosdlkfjskdljf"}, curPrefs: ipn.NewPrefs(), - mp: &ipn.MaskedPrefs{ - Prefs: *defaultPrefsFromUpArgs(t, "linux"), - WantRunningSet: true, - }, - want: "", + want: "", }, { - name: "implicit_operator_change", - flagSet: f("hostname"), + name: "implicit_operator_change", + flags: []string{"--hostname=foo"}, curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - OperatorUser: "alice", + ControlURL: ipn.DefaultControlURL, + OperatorUser: "alice", + AllowSingleHosts: true, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, }, curUser: "eve", - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - }, - ControlURLSet: true, - }, - want: accidentalUpPrefix + " --hostname= --operator=alice", + want: accidentalUpPrefix + " --hostname=foo --operator=alice", }, { - name: "implicit_operator_matches_shell_user", - flagSet: f("hostname"), + name: "implicit_operator_matches_shell_user", + flags: []string{"--hostname=foo"}, curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - OperatorUser: "alice", + ControlURL: ipn.DefaultControlURL, + AllowSingleHosts: true, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, + OperatorUser: "alice", }, curUser: "alice", - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - }, - ControlURLSet: true, - }, - want: "", + want: "", }, { name: "error_advertised_routes_exit_node_removed", @@ -194,7 +155,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { want: accidentalUpPrefix + " --advertise-routes=10.0.42.0/24 --advertise-exit-node", }, { - name: "advertised_routes_exit_node_removed", + name: "advertised_routes_exit_node_removed_explicit", flags: []string{"--advertise-routes=10.0.42.0/24", "--advertise-exit-node=false"}, curPrefs: &ipn.Prefs{ ControlURL: ipn.DefaultControlURL, @@ -210,27 +171,19 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { want: "", }, { - name: "advertised_routes_includes_the_0_routes", // but no --advertise-exit-node - flagSet: f("advertise-routes"), + name: "advertised_routes_includes_the_0_routes", // but no --advertise-exit-node + flags: []string{"--advertise-routes=11.1.43.0/24,0.0.0.0/0,::/0"}, curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, + ControlURL: ipn.DefaultControlURL, + AllowSingleHosts: true, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, AdvertiseRoutes: []netaddr.IPPrefix{ netaddr.MustParseIPPrefix("10.0.42.0/24"), netaddr.MustParseIPPrefix("0.0.0.0/0"), netaddr.MustParseIPPrefix("::/0"), }, }, - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - AdvertiseRoutes: []netaddr.IPPrefix{ - netaddr.MustParseIPPrefix("11.1.43.0/24"), - netaddr.MustParseIPPrefix("0.0.0.0/0"), - netaddr.MustParseIPPrefix("::/0"), - }, - }, - AdvertiseRoutesSet: true, - }, want: "", }, { @@ -252,6 +205,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { AllowSingleHosts: true, CorpDNS: true, NetfilterMode: preftype.NetfilterOn, + AdvertiseRoutes: []netaddr.IPPrefix{ netaddr.MustParseIPPrefix("1.2.0.0/16"), }, @@ -275,19 +229,16 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { want: accidentalUpPrefix + " --advertise-exit-node --advertise-routes=1.2.0.0/16", }, { - name: "exit_node_clearing", // Issue 1777 - flagSet: f("exit-node"), + name: "exit_node_clearing", // Issue 1777 + flags: []string{"--exit-node="}, curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, + ControlURL: ipn.DefaultControlURL, + AllowSingleHosts: true, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, + ExitNodeID: "fooID", }, - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - ExitNodeIP: netaddr.IP{}, - }, - ExitNodeIPSet: true, - }, want: "", }, { @@ -306,12 +257,14 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { ForceDaemon: true, AdvertiseRoutes: []netaddr.IPPrefix{ netaddr.MustParseIPPrefix("10.0.0.0/16"), + netaddr.MustParseIPPrefix("0.0.0.0/0"), + netaddr.MustParseIPPrefix("::/0"), }, NetfilterMode: preftype.NetfilterNoDivert, OperatorUser: "alice", }, curUser: "eve", - want: accidentalUpPrefix + " --force-reauth --accept-routes --host-routes=false --exit-node=100.64.5.6 --accept-dns=false --shields-up --advertise-tags=tag:foo,tag:bar --hostname=myhostname --unattended --advertise-routes=10.0.0.0/16 --netfilter-mode=nodivert --operator=alice", + want: accidentalUpPrefix + " --force-reauth --accept-dns=false --accept-routes --advertise-exit-node --advertise-routes=10.0.0.0/16 --advertise-tags=tag:foo,tag:bar --exit-node=100.64.5.6 --host-routes=false --hostname=myhostname --netfilter-mode=nodivert --operator=alice --shields-up", }, { name: "remove_all_implicit_except_hostname", @@ -334,63 +287,46 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { OperatorUser: "alice", }, curUser: "eve", - want: accidentalUpPrefix + " --hostname=newhostname --accept-routes --host-routes=false --exit-node=100.64.5.6 --accept-dns=false --shields-up --advertise-tags=tag:foo,tag:bar --unattended --advertise-routes=10.0.0.0/16 --netfilter-mode=nodivert --operator=alice", + want: accidentalUpPrefix + " --hostname=newhostname --accept-dns=false --accept-routes --advertise-routes=10.0.0.0/16 --advertise-tags=tag:foo,tag:bar --exit-node=100.64.5.6 --host-routes=false --netfilter-mode=nodivert --operator=alice --shields-up", }, { - name: "loggedout_is_implicit", - flagSet: f("advertise-exit-node"), + name: "loggedout_is_implicit", + flags: []string{"--hostname=foo"}, curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - LoggedOut: true, - }, - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - AdvertiseRoutes: []netaddr.IPPrefix{ - netaddr.MustParseIPPrefix("0.0.0.0/0"), - }, - }, - AdvertiseRoutesSet: true, + ControlURL: ipn.DefaultControlURL, + LoggedOut: true, + AllowSingleHosts: true, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, }, - // not an error. LoggedOut is implicit. - want: "", + want: "", // not an error. LoggedOut is implicit. }, { // Test that a pre-1.8 version of Tailscale with bogus NoSNAT pref // values is able to enable exit nodes without warnings. - name: "make_windows_exit_node", - flagSet: f("advertise-exit-node"), + name: "make_windows_exit_node", + flags: []string{"--advertise-exit-node"}, curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - NoSNAT: true, // assume this no-op accidental pre-1.8 value + ControlURL: ipn.DefaultControlURL, + AllowSingleHosts: true, + CorpDNS: true, + + // And assume this no-op accidental pre-1.8 value: + NoSNAT: true, }, goos: "windows", - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - AdvertiseRoutes: []netaddr.IPPrefix{ - netaddr.MustParseIPPrefix("192.168.0.0/16"), - }, - }, - AdvertiseRoutesSet: true, - }, want: "", // not an error }, { - name: "ignore_netfilter_change_non_linux", - flagSet: f("accept-dns"), + name: "ignore_netfilter_change_non_linux", + flags: []string{"--accept-dns"}, curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, + ControlURL: ipn.DefaultControlURL, + AllowSingleHosts: true, + NetfilterMode: preftype.NetfilterNoDivert, // we never had this bug, but pretend it got set non-zero on Windows somehow }, goos: "windows", - mp: &ipn.MaskedPrefs{ - Prefs: ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - CorpDNS: false, - }, - CorpDNSSet: true, - }, want: "", // not an error }, { @@ -407,7 +343,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { netaddr.MustParseIPPrefix("1.2.0.0/16"), }, }, - want: accidentalUpPrefix + " --operator=expbits --advertise-routes=1.2.0.0/16 --advertise-exit-node", + want: accidentalUpPrefix + " --operator=expbits --advertise-exit-node --advertise-routes=1.2.0.0/16", }, { name: "operator_losing_routes_step2", // https://twitter.com/EXPbits/status/1390418145047887877 @@ -425,6 +361,47 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { }, want: accidentalUpPrefix + " --advertise-routes=1.2.0.0/16 --operator=expbits --advertise-exit-node", }, + { + name: "errors_preserve_explicit_flags", + flags: []string{"--reset", "--force-reauth=false", "--authkey=secretrand"}, + curPrefs: &ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, + WantRunning: false, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, + AllowSingleHosts: true, + + Hostname: "foo", + }, + want: accidentalUpPrefix + " --authkey=secretrand --force-reauth=false --reset --hostname=foo", + }, + { + name: "error_exit_node_omit_with_ip_pref", + flags: []string{"--hostname=foo"}, + curPrefs: &ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, + AllowSingleHosts: true, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, + + ExitNodeIP: netaddr.MustParseIP("100.64.5.4"), + }, + want: accidentalUpPrefix + " --hostname=foo --exit-node=100.64.5.4", + }, + { + name: "error_exit_node_omit_with_id_pref", + flags: []string{"--hostname=foo"}, + curExitNodeIP: netaddr.MustParseIP("100.64.5.7"), + curPrefs: &ipn.Prefs{ + ControlURL: ipn.DefaultControlURL, + AllowSingleHosts: true, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, + + ExitNodeID: "some_stable_id", + }, + want: accidentalUpPrefix + " --hostname=foo --exit-node=100.64.5.7", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -432,23 +409,19 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { if tt.goos != "" { goos = tt.goos } - flagSet := tt.flagSet - mp := tt.mp - if tt.flags != nil { - if tt.flagSet != nil || tt.mp != nil { - t.Fatal("flags and flagSet/mp are mutually exclusive") - } - var upArgs upArgsT - fs := newUpFlagSet(goos, &upArgs) - fs.Parse(tt.flags) - prefs, err := prefsFromUpArgs(upArgs, t.Logf, new(ipnstate.Status), goos) - if err != nil { - t.Fatal(err) - } - flagSet, mp = flagSetAndMaskedPrefs(prefs, fs) + var upArgs upArgsT + flagSet := newUpFlagSet(goos, &upArgs) + flagSet.Parse(tt.flags) + newPrefs, err := prefsFromUpArgs(upArgs, t.Logf, new(ipnstate.Status), goos) + if err != nil { + t.Fatal(err) } + applyImplicitPrefs(newPrefs, tt.curPrefs, tt.curUser) var got string - if err := checkForAccidentalSettingReverts(flagSet, tt.curPrefs, mp, goos, tt.curUser); err != nil { + if err := checkForAccidentalSettingReverts(flagSet, tt.curPrefs, newPrefs, upCheckEnv{ + goos: goos, + curExitNodeIP: tt.curExitNodeIP, + }); err != nil { got = err.Error() } if strings.TrimSpace(got) != tt.want { @@ -458,16 +431,6 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { } } -func defaultPrefsFromUpArgs(t testing.TB, goos string) *ipn.Prefs { - upArgs := upArgsFromOSArgs(goos) - prefs, err := prefsFromUpArgs(upArgs, logger.Discard, new(ipnstate.Status), "linux") - if err != nil { - t.Fatalf("defaultPrefsFromUpArgs: %v", err) - } - prefs.WantRunning = true - return prefs -} - func upArgsFromOSArgs(goos string, flagArgs ...string) (args upArgsT) { fs := newUpFlagSet(goos, &args) fs.Parse(flagArgs) // populates args @@ -663,10 +626,17 @@ func TestPrefsFromUpArgs(t *testing.T) { } func TestPrefFlagMapping(t *testing.T) { + prefHasFlag := map[string]bool{} + for _, pv := range prefsOfFlag { + for _, pref := range pv { + prefHasFlag[pref] = true + } + } + prefType := reflect.TypeOf(ipn.Prefs{}) for i := 0; i < prefType.NumField(); i++ { prefName := prefType.Field(i).Name - if _, ok := flagForPref[prefName]; ok { + if prefHasFlag[prefName] { continue } switch prefName { @@ -684,3 +654,15 @@ func TestPrefFlagMapping(t *testing.T) { t.Errorf("unexpected new ipn.Pref field %q is not handled by up.go (see addPrefFlagMapping and checkForAccidentalSettingReverts)", prefName) } } + +func TestFlagAppliesToOS(t *testing.T) { + for _, goos := range geese { + var upArgs upArgsT + fs := newUpFlagSet(goos, &upArgs) + fs.VisitAll(func(f *flag.Flag) { + if !flagAppliesToOS(f.Name, goos) { + t.Errorf("flagAppliesToOS(%q, %q) = false but found in %s set", f.Name, goos, goos) + } + }) + } +} diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index cca6509de..35b29deb9 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -13,7 +13,6 @@ import ( "reflect" "runtime" "sort" - "strconv" "strings" "sync" @@ -295,10 +294,13 @@ func runUp(ctx context.Context, args []string) error { return err } - flagSet, mp := flagSetAndMaskedPrefs(prefs, upFlagSet) - if !upArgs.reset { - if err := checkForAccidentalSettingReverts(flagSet, curPrefs, mp, runtime.GOOS, os.Getenv("USER")); err != nil { + applyImplicitPrefs(prefs, curPrefs, os.Getenv("USER")) + + if err := checkForAccidentalSettingReverts(upFlagSet, curPrefs, prefs, upCheckEnv{ + goos: runtime.GOOS, + curExitNodeIP: exitNodeIP(prefs, st), + }); err != nil { fatalf("%s", err) } } @@ -310,7 +312,7 @@ func runUp(ctx context.Context, args []string) error { // If we're already running and none of the flags require a // restart, we can just do an EditPrefs call and change the - // prefs at runtime (e.g. changing hostname, changinged + // prefs at runtime (e.g. changing hostname, changing // advertised tags, routes, etc) justEdit := st.BackendState == ipn.Running.String() && !upArgs.forceReauth && @@ -318,6 +320,13 @@ func runUp(ctx context.Context, args []string) error { upArgs.authKey == "" && !controlURLChanged if justEdit { + mp := new(ipn.MaskedPrefs) + mp.WantRunningSet = true + mp.Prefs = *prefs + upFlagSet.Visit(func(f *flag.Flag) { + updateMaskedPrefsFromUpFlag(mp, f.Name) + }) + _, err := tailscale.EditPrefs(ctx, mp) return err } @@ -325,7 +334,7 @@ func runUp(ctx context.Context, args []string) error { // simpleUp is whether we're running a simple "tailscale up" // to transition to running from a previously-logged-in but // down state, without changing any settings. - simpleUp := len(flagSet) == 0 && + simpleUp := upFlagSet.NFlag() == 0 && curPrefs.Persist != nil && curPrefs.Persist.LoginName != "" && st.BackendState != ipn.NeedsLogin.String() @@ -457,14 +466,20 @@ func runUp(ctx context.Context, args []string) error { } var ( - flagForPref = map[string]string{} // "ExitNodeIP" => "exit-node" - prefsOfFlag = map[string][]string{} + prefsOfFlag = map[string][]string{} // "exit-node" => ExitNodeIP, ExitNodeID ) func init() { + // Both these have the same ipn.Pref: + addPrefFlagMapping("advertise-exit-node", "AdvertiseRoutes") + addPrefFlagMapping("advertise-routes", "AdvertiseRoutes") + + // And this flag has two ipn.Prefs: + addPrefFlagMapping("exit-node", "ExitNodeIP", "ExitNodeID") + + // The rest are 1:1: addPrefFlagMapping("accept-dns", "CorpDNS") addPrefFlagMapping("accept-routes", "RouteAll") - addPrefFlagMapping("advertise-routes", "AdvertiseRoutes") addPrefFlagMapping("advertise-tags", "AdvertiseTags") addPrefFlagMapping("host-routes", "AllowSingleHosts") addPrefFlagMapping("hostname", "Hostname") @@ -472,7 +487,6 @@ func init() { addPrefFlagMapping("netfilter-mode", "NetfilterMode") addPrefFlagMapping("shields-up", "ShieldsUp") addPrefFlagMapping("snat-subnet-routes", "NoSNAT") - addPrefFlagMapping("exit-node", "ExitNodeIP", "ExitNodeID") addPrefFlagMapping("exit-node-allow-lan-access", "ExitNodeAllowLANAccess") addPrefFlagMapping("unattended", "ForceDaemon") addPrefFlagMapping("operator", "OperatorUser") @@ -482,8 +496,6 @@ func addPrefFlagMapping(flagName string, prefNames ...string) { prefsOfFlag[flagName] = prefNames prefType := reflect.TypeOf(ipn.Prefs{}) for _, pref := range prefNames { - flagForPref[pref] = flagName - // Crash at runtime if there's a typo in the prefName. if _, ok := prefType.FieldByName(pref); !ok { panic(fmt.Sprintf("invalid ipn.Prefs field %q", pref)) @@ -491,33 +503,27 @@ func addPrefFlagMapping(flagName string, prefNames ...string) { } } -func flagSetAndMaskedPrefs(prefs *ipn.Prefs, fs *flag.FlagSet) (flagSetMap map[string]bool, mp *ipn.MaskedPrefs) { - flagSetMap = map[string]bool{} - mp = new(ipn.MaskedPrefs) - mp.WantRunningSet = true - mp.Prefs = *prefs - fs.Visit(func(f *flag.Flag) { - updateMaskedPrefsFromUpFlag(mp, f.Name) - flagSetMap[f.Name] = true - }) - return flagSetMap, mp +// preflessFlag reports whether flagName is a flag that doesn't +// correspond to an ipn.Pref. +func preflessFlag(flagName string) bool { + switch flagName { + case "authkey", "force-reauth", "reset": + return true + } + return false } func updateMaskedPrefsFromUpFlag(mp *ipn.MaskedPrefs, flagName string) { + if preflessFlag(flagName) { + return + } if prefs, ok := prefsOfFlag[flagName]; ok { for _, pref := range prefs { reflect.ValueOf(mp).Elem().FieldByName(pref + "Set").SetBool(true) } return } - switch flagName { - case "authkey", "force-reauth", "reset": - // Not pref-related flags. - case "advertise-exit-node": - // This pref is a shorthand for advertise-routes. - default: - panic(fmt.Sprintf("internal error: unhandled flag %q", flagName)) - } + panic(fmt.Sprintf("internal error: unhandled flag %q", flagName)) } const accidentalUpPrefix = "Error: changing settings via 'tailscale up' requires mentioning all\n" + @@ -526,9 +532,16 @@ const accidentalUpPrefix = "Error: changing settings via 'tailscale up' requires "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. +// upCheckEnv are extra parameters describing the environment as +// needed by checkForAccidentalSettingReverts and friends. +type upCheckEnv struct { + goos string + curExitNodeIP netaddr.IP +} + +// checkForAccidentalSettingReverts (the "up checker") checks for +// people running "tailscale up" with a subset of the flags they +// originally ran it with. // // For example, in Tailscale 1.6 and prior, a user might've advertised // a tag, but later tried to change just one other setting and forgot @@ -540,173 +553,171 @@ const accidentalUpPrefix = "Error: changing settings via 'tailscale up' requires // // mp is the mask of settings actually set, where mp.Prefs is the new // preferences to set, including any values set from implicit flags. -func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Prefs, mp *ipn.MaskedPrefs, goos, curUser string) error { - if len(flagSet) == 0 { - // A bare "tailscale up" is a special case to just - // mean bringing the network up without any changes. - return nil - } +func checkForAccidentalSettingReverts(flagSet *flag.FlagSet, curPrefs, newPrefs *ipn.Prefs, env upCheckEnv) error { if curPrefs.ControlURL == "" { // Don't validate things on initial "up" before a control URL has been set. return nil } - curWithExplicitEdits := curPrefs.Clone() - curWithExplicitEdits.ApplyEdits(mp) - prefType := reflect.TypeOf(ipn.Prefs{}) + flagIsSet := map[string]bool{} + flagSet.Visit(func(f *flag.Flag) { + flagIsSet[f.Name] = true + }) - // Explicit values (current + explicit edit): - ev := reflect.ValueOf(curWithExplicitEdits).Elem() - // Implicit values (what we'd get if we replaced everything with flag defaults): - iv := reflect.ValueOf(&mp.Prefs).Elem() + if len(flagIsSet) == 0 { + // A bare "tailscale up" is a special case to just + // mean bringing the network up without any changes. + return nil + } + + // flagsCur is what flags we'd need to use to keep the exact + // settings as-is. + flagsCur := prefsToFlags(env, curPrefs) + flagsNew := prefsToFlags(env, newPrefs) 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 - // Persist is a legacy field used for storing keys, which - // probably should never have been part of Prefs. It's - // likely to migrate elsewhere eventually. - if prefName == "Persist" { - continue - } - // LoggedOut is a preference, but running the "up" command - // always implies that the user now prefers LoggedOut->false. - if prefName == "LoggedOut" { + for flagName := range flagsCur { + valCur, valNew := flagsCur[flagName], flagsNew[flagName] + if flagIsSet[flagName] { continue } - flagName, hasFlag := flagForPref[prefName] - - // Special case for advertise-exit-node; which is a - // flag but doesn't have a corresponding pref. The - // flag augments advertise-routes, so we have to infer - // the imaginary pref's current value from the routes. - if prefName == "AdvertiseRoutes" && - hasExitNodeRoutes(curPrefs.AdvertiseRoutes) && - !hasExitNodeRoutes(curWithExplicitEdits.AdvertiseRoutes) && - !flagSet["advertise-exit-node"] { - missing = append(missing, "--advertise-exit-node") - } - - if hasFlag && flagSet[flagName] { - flagExplicitValue[flagName] = ev.Field(i).Interface() + if reflect.DeepEqual(valCur, valNew) { continue } + missing = append(missing, fmtFlagValueArg(flagName, valCur)) + } + if len(missing) == 0 { + return nil + } + sort.Strings(missing) - if prefName == "AdvertiseRoutes" && - (len(curPrefs.AdvertiseRoutes) == 0 || - hasExitNodeRoutes(curPrefs.AdvertiseRoutes) && len(curPrefs.AdvertiseRoutes) == 2) && - hasExitNodeRoutes(mp.Prefs.AdvertiseRoutes) && - len(mp.Prefs.AdvertiseRoutes) == 2 && - flagSet["advertise-exit-node"] { - continue + // Compute the stringification of the explicitly provided args in flagSet + // to prepend to the command to run. + var explicit []string + flagSet.Visit(func(f *flag.Flag) { + type isBool interface { + IsBoolFlag() bool } - - // Get explicit value and implicit value - ex, im := ev.Field(i), iv.Field(i) - switch ex.Kind() { - case reflect.String, reflect.Slice: - if ex.Kind() == reflect.Slice && ex.Len() == 0 && im.Len() == 0 { - // Treat nil and non-nil empty slices as equivalent. - continue + if ib, ok := f.Value.(isBool); ok && ib.IsBoolFlag() { + if f.Value.String() == "false" { + explicit = append(explicit, "--"+f.Name+"=false") + } else { + explicit = append(explicit, "--"+f.Name) } + } else { + explicit = append(explicit, fmtFlagValueArg(f.Name, f.Value.String())) } - exi, imi := ex.Interface(), im.Interface() + }) - if reflect.DeepEqual(exi, imi) { - continue - } - switch flagName { - case "operator": - if imi == "" && exi == curUser { - // Don't require setting operator if the current user matches - // the configured operator. - continue - } - case "snat-subnet-routes", "netfilter-mode": - if goos != "linux" { - // Issue 1833: we used to accidentally set the NoSNAT - // pref for non-Linux nodes. It only affects Linux, so - // ignore it if it changes. Likewise, ignore - // Linux-only netfilter-mode on non-Linux. - continue - } - } - switch flagName { - case "": - 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 prefName == "ExitNodeIP" { - missing = append(missing, fmtFlagValueArg("exit-node", fmtSettingVal(exi))) - } - case "advertise-routes": - hadExitNode := hasExitNodeRoutes(exi.([]netaddr.IPPrefix)) - routes := withoutExitNodes(exi.([]netaddr.IPPrefix)) - missing = append(missing, fmtFlagValueArg("advertise-routes", fmtSettingVal(routes))) - if hadExitNode && !flagSet["advertise-exit-node"] { - missing = append(missing, "--advertise-exit-node") - } - default: - missing = append(missing, fmtFlagValueArg(flagName, fmtSettingVal(exi))) - } - } - 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))) - } else { - fmt.Fprintf(&sb, " --%s", flagName) - } - } - for _, a := range missing { + for _, a := range append(explicit, 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 +// applyImplicitPrefs mutates prefs to add implicit preferences. Currently +// this is just the operator user, which only needs to be set if it doesn't +// match the current user. +// +// curUser is os.Getenv("USER"). It's pulled out for testability. +func applyImplicitPrefs(prefs, oldPrefs *ipn.Prefs, curUser string) { + if prefs.OperatorUser == "" && oldPrefs.OperatorUser == curUser { + prefs.OperatorUser = oldPrefs.OperatorUser } - if val == "" { - return "--" + flagName + "=" +} + +func flagAppliesToOS(flag, goos string) bool { + switch flag { + case "netfilter-mode", "snat-subnet-routes": + return goos == "linux" + case "unattended": + return goos == "windows" } - return fmt.Sprintf("--%s=%v", flagName, shellquote.Join(val)) + return true } -func fmtSettingVal(v interface{}) string { - switch v := v.(type) { - case bool: - return strconv.FormatBool(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(',') +func prefsToFlags(env upCheckEnv, prefs *ipn.Prefs) (flagVal map[string]interface{}) { + ret := make(map[string]interface{}) + + exitNodeIPStr := func() string { + if !prefs.ExitNodeIP.IsZero() { + return prefs.ExitNodeIP.String() + } + if prefs.ExitNodeID.IsZero() || env.curExitNodeIP.IsZero() { + return "" + } + return env.curExitNodeIP.String() + } + + fs := newUpFlagSet(env.goos, new(upArgsT) /* dummy */) + fs.VisitAll(func(f *flag.Flag) { + if preflessFlag(f.Name) { + return + } + set := func(v interface{}) { + if flagAppliesToOS(f.Name, env.goos) { + ret[f.Name] = v + } else { + ret[f.Name] = nil } - sb.WriteString(r.String()) } - return sb.String() + switch f.Name { + default: + panic(fmt.Sprintf("unhandled flag %q", f.Name)) + case "login-server": + set(prefs.ControlURL) + case "accept-routes": + set(prefs.RouteAll) + case "host-routes": + set(prefs.AllowSingleHosts) + case "accept-dns": + set(prefs.CorpDNS) + case "shields-up": + set(prefs.ShieldsUp) + case "exit-node": + set(exitNodeIPStr()) + case "exit-node-allow-lan-access": + set(prefs.ExitNodeAllowLANAccess) + case "advertise-tags": + set(strings.Join(prefs.AdvertiseTags, ",")) + case "hostname": + set(prefs.Hostname) + case "operator": + set(prefs.OperatorUser) + case "advertise-routes": + var sb strings.Builder + for i, r := range withoutExitNodes(prefs.AdvertiseRoutes) { + if i > 0 { + sb.WriteByte(',') + } + sb.WriteString(r.String()) + } + set(sb.String()) + case "advertise-exit-node": + set(hasExitNodeRoutes(prefs.AdvertiseRoutes)) + case "snat-subnet-routes": + set(!prefs.NoSNAT) + case "netfilter-mode": + set(prefs.NetfilterMode.String()) + case "unattended": + set(prefs.ForceDaemon) + } + }) + return ret +} + +func fmtFlagValueArg(flagName string, val interface{}) string { + if val == true { + return "--" + flagName } - return fmt.Sprint(v) + if val == "" { + return "--" + flagName + "=" + } + return fmt.Sprintf("--%s=%v", flagName, shellquote.Join(fmt.Sprint(val))) } func hasExitNodeRoutes(rr []netaddr.IPPrefix) bool { @@ -738,3 +749,27 @@ func withoutExitNodes(rr []netaddr.IPPrefix) []netaddr.IPPrefix { } return out } + +// exitNodeIP returns the exit node IP from p, using st to map +// it from its ID form to an IP address if needed. +func exitNodeIP(p *ipn.Prefs, st *ipnstate.Status) (ip netaddr.IP) { + if p == nil { + return + } + if !p.ExitNodeIP.IsZero() { + return p.ExitNodeIP + } + id := p.ExitNodeID + if id.IsZero() { + return + } + for _, p := range st.Peer { + if p.ID == id { + if len(p.TailscaleIPs) > 0 { + return p.TailscaleIPs[0] + } + break + } + } + return +}