cmd/tailscale: make pref-revert checks ignore OS-irrelevant prefs

This fixes #1833 in two ways:

* stop setting NoSNAT on non-Linux. It only matters on Linux and the flag
  is hidden on non-Linux, but the code was still setting it. Because of
  that, the new pref-reverting safety checks were failing when it was
  changing.

* Ignore the two Linux-only prefs changing on non-Linux.

Fixes #1833

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

cmd/tailscale/cli/cli_test.go

@@ -41,6 +41,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 		flagSet  map[string]bool
 		curPrefs *ipn.Prefs
 		curUser  string // os.Getenv("USER") on the client side
+		goos     string // empty means "linux"
 		mp       *ipn.MaskedPrefs
 		want     string
 	}{
@@ -348,11 +349,72 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 			},
 			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",
 		},
+		{
+			name:    "loggedout_is_implicit",
+			flagSet: f("advertise-exit-node"),
+			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,
+			},
+			// not an error. LoggedOut is implicit.
+			want: "",
+		},
+		{
+			// 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"),
+			curPrefs: &ipn.Prefs{
+				ControlURL: ipn.DefaultControlURL,
+				NoSNAT:     true, // assume this no-op accidental pre-1.8 value
+			},
+			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"),
+			curPrefs: &ipn.Prefs{
+				ControlURL:    ipn.DefaultControlURL,
+				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
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			goos := "linux"
+			if tt.goos != "" {
+				goos = tt.goos
+			}
 			var got string
-			if err := checkForAccidentalSettingReverts(tt.flagSet, tt.curPrefs, tt.mp, tt.curUser); err != nil {
+			if err := checkForAccidentalSettingReverts(tt.flagSet, tt.curPrefs, tt.mp, goos, tt.curUser); err != nil {
 				got = err.Error()
 			}
 			if strings.TrimSpace(got) != tt.want {
@@ -411,7 +473,6 @@ func TestPrefsFromUpArgs(t *testing.T) {
 				CorpDNS:          true,
 				AllowSingleHosts: true,
 				NetfilterMode:    preftype.NetfilterOn,
-				NoSNAT:           true,
 			},
 		},
 		{

cmd/tailscale/cli/up.go

@@ -216,12 +216,13 @@ func prefsFromUpArgs(upArgs upArgsT, warnf logger.Logf, st *ipnstate.Status, goo
 	prefs.ShieldsUp = upArgs.shieldsUp
 	prefs.AdvertiseRoutes = routes
 	prefs.AdvertiseTags = tags
-	prefs.NoSNAT = !upArgs.snat
 	prefs.Hostname = upArgs.hostname
 	prefs.ForceDaemon = upArgs.forceDaemon
 	prefs.OperatorUser = upArgs.opUser
 
 	if goos == "linux" {
+		prefs.NoSNAT = !upArgs.snat
+
 		switch upArgs.netfilterMode {
 		case "on":
 			prefs.NetfilterMode = preftype.NetfilterOn
@@ -304,7 +305,7 @@ func runUp(ctx context.Context, args []string) error {
 	})
 
 	if !upArgs.reset {
-		if err := checkForAccidentalSettingReverts(flagSet, curPrefs, mp, os.Getenv("USER")); err != nil {
+		if err := checkForAccidentalSettingReverts(flagSet, curPrefs, mp, runtime.GOOS, os.Getenv("USER")); err != nil {
 			fatalf("%s", err)
 		}
 	}
@@ -530,7 +531,7 @@ func updateMaskedPrefsFromUpFlag(mp *ipn.MaskedPrefs, flagName string) {
 //
 // 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, curUser string) error {
+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.
@@ -588,11 +589,22 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre
 		if reflect.DeepEqual(exi, imi) {
 			continue
 		}
-		if flagName == "operator" && imi == "" && exi == curUser {
+		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)