cmd/tailscale: fix tailscale up --advertise-exit-node validation

Fixes #1859

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

cmd/tailscale/cli/cli_test.go

@@ -274,6 +274,71 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 			},
 			want: accidentalUpPrefix + " --advertise-routes=11.1.43.0/24,0.0.0.0/0 --advertise-exit-node",
 		},
+		{
+			name:    "advertise_exit_node", // Issue 1859
+			flagSet: f("advertise-exit-node"),
+			curPrefs: &ipn.Prefs{
+				ControlURL: ipn.DefaultControlURL,
+			},
+			mp: &ipn.MaskedPrefs{
+				Prefs: ipn.Prefs{
+					ControlURL: ipn.DefaultControlURL,
+					AdvertiseRoutes: []netaddr.IPPrefix{
+						netaddr.MustParseIPPrefix("0.0.0.0/0"),
+						netaddr.MustParseIPPrefix("::/0"),
+					},
+				},
+				// Note: without setting "AdvertiseRoutesSet", as
+				// updateMaskedPrefsFromUpFlag doesn't set that.
+			},
+			want: "",
+		},
+		{
+			name:    "advertise_exit_node_over_existing_routes",
+			flagSet: f("advertise-exit-node"),
+			curPrefs: &ipn.Prefs{
+				ControlURL: ipn.DefaultControlURL,
+				AdvertiseRoutes: []netaddr.IPPrefix{
+					netaddr.MustParseIPPrefix("1.2.0.0/16"),
+				},
+			},
+			mp: &ipn.MaskedPrefs{
+				Prefs: ipn.Prefs{
+					ControlURL: ipn.DefaultControlURL,
+					AdvertiseRoutes: []netaddr.IPPrefix{
+						netaddr.MustParseIPPrefix("0.0.0.0/0"),
+						netaddr.MustParseIPPrefix("::/0"),
+					},
+				},
+				// Note: without setting "AdvertiseRoutesSet", as
+				// updateMaskedPrefsFromUpFlag doesn't set that.
+			},
+			want: accidentalUpPrefix + " --advertise-exit-node --advertise-routes=1.2.0.0/16",
+		},
+		{
+			name:    "advertise_exit_node_over_existing_routes_and_exit_node",
+			flagSet: f("advertise-exit-node"),
+			curPrefs: &ipn.Prefs{
+				ControlURL: ipn.DefaultControlURL,
+				AdvertiseRoutes: []netaddr.IPPrefix{
+					netaddr.MustParseIPPrefix("0.0.0.0/0"),
+					netaddr.MustParseIPPrefix("::/0"),
+					netaddr.MustParseIPPrefix("1.2.0.0/16"),
+				},
+			},
+			mp: &ipn.MaskedPrefs{
+				Prefs: ipn.Prefs{
+					ControlURL: ipn.DefaultControlURL,
+					AdvertiseRoutes: []netaddr.IPPrefix{
+						netaddr.MustParseIPPrefix("0.0.0.0/0"),
+						netaddr.MustParseIPPrefix("::/0"),
+					},
+				},
+				// Note: without setting "AdvertiseRoutesSet", as
+				// updateMaskedPrefsFromUpFlag doesn't set that.
+			},
+			want: accidentalUpPrefix + " --advertise-exit-node --advertise-routes=1.2.0.0/16",
+		},
 		{
 			name:    "exit_node_clearing", // Issue 1777
 			flagSet: f("exit-node"),
@@ -317,7 +382,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 					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",
+			want: accidentalUpPrefix + " --force-reauth --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",
@@ -426,7 +491,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 }
 
 func defaultPrefsFromUpArgs(t testing.TB, goos string) *ipn.Prefs {
-	upArgs := defaultUpArgsByGOOS(goos)
+	upArgs := upArgsFromOSArgs(goos)
 	prefs, err := prefsFromUpArgs(upArgs, logger.Discard, new(ipnstate.Status), "linux")
 	if err != nil {
 		t.Fatalf("defaultPrefsFromUpArgs: %v", err)
@@ -435,9 +500,9 @@ func defaultPrefsFromUpArgs(t testing.TB, goos string) *ipn.Prefs {
 	return prefs
 }
 
-func defaultUpArgsByGOOS(goos string) (args upArgsT) {
+func upArgsFromOSArgs(goos string, flagArgs ...string) (args upArgsT) {
 	fs := newUpFlagSet(goos, &args)
-	fs.Parse(nil) // populates args
+	fs.Parse(flagArgs) // populates args
 	return
 }
 
@@ -454,7 +519,7 @@ func TestPrefsFromUpArgs(t *testing.T) {
 		{
 			name: "default_linux",
 			goos: "linux",
-			args: defaultUpArgsByGOOS("linux"),
+			args: upArgsFromOSArgs("linux"),
 			want: &ipn.Prefs{
 				ControlURL:       ipn.DefaultControlURL,
 				WantRunning:      true,
@@ -467,7 +532,7 @@ func TestPrefsFromUpArgs(t *testing.T) {
 		{
 			name: "default_windows",
 			goos: "windows",
-			args: defaultUpArgsByGOOS("windows"),
+			args: upArgsFromOSArgs("windows"),
 			want: &ipn.Prefs{
 				ControlURL:       ipn.DefaultControlURL,
 				WantRunning:      true,
@@ -476,6 +541,21 @@ func TestPrefsFromUpArgs(t *testing.T) {
 				NetfilterMode:    preftype.NetfilterOn,
 			},
 		},
+		{
+			name: "advertise_default_route",
+			args: upArgsFromOSArgs("linux", "--advertise-exit-node"),
+			want: &ipn.Prefs{
+				ControlURL:       ipn.DefaultControlURL,
+				WantRunning:      true,
+				AllowSingleHosts: true,
+				CorpDNS:          true,
+				AdvertiseRoutes: []netaddr.IPPrefix{
+					netaddr.MustParseIPPrefix("0.0.0.0/0"),
+					netaddr.MustParseIPPrefix("::/0"),
+				},
+				NetfilterMode: preftype.NetfilterOn,
+			},
+		},
 		{
 			name: "error_advertise_route_invalid_ip",
 			args: upArgsT{

cmd/tailscale/cli/up.go

@@ -587,6 +587,16 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre
 			flagExplicitValue[flagName] = ev.Field(i).Interface()
 			continue
 		}
+
+		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
+		}
+
 		// Get explicit value and implicit value
 		ex, im := ev.Field(i), iv.Field(i)
 		switch ex.Kind() {
@@ -624,6 +634,9 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre
 			if prefName == "ExitNodeIP" {
 				missing = append(missing, fmtFlagValueArg("exit-node", fmtSettingVal(exi)))
 			}
+		case "advertise-routes":
+			routes := withoutExitNodes(exi.([]netaddr.IPPrefix))
+			missing = append(missing, fmtFlagValueArg("advertise-routes", fmtSettingVal(routes)))
 		default:
 			missing = append(missing, fmtFlagValueArg(flagName, fmtSettingVal(exi)))
 		}
@@ -642,6 +655,8 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre
 	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 {
@@ -698,3 +713,19 @@ func hasExitNodeRoutes(rr []netaddr.IPPrefix) bool {
 	}
 	return v4 && v6
 }
+
+// withoutExitNodes returns rr unchanged if it has only 1 or 0 /0
+// routes. If it has both IPv4 and IPv6 /0 routes, then it returns
+// a copy with all /0 routes removed.
+func withoutExitNodes(rr []netaddr.IPPrefix) []netaddr.IPPrefix {
+	if !hasExitNodeRoutes(rr) {
+		return rr
+	}
+	var out []netaddr.IPPrefix
+	for _, r := range rr {
+		if r.Bits > 0 {
+			out = append(out, r)
+		}
+	}
+	return out
+}