cmd/tailscale/cli: make 'tailscale up' protect --advertise-exit-node removal

The new "tailscale up" checks previously didn't protect against
--advertise-exit-node being omitted in the case that
--advertise-routes was also provided. It wasn't done before because
there is no corresponding pref for "--advertise-exit-node"; it's a
helper flag that augments --advertise-routes. But that's an
implementation detail and we can still help users. We just have to
special case that pref and look whether the current routes include
both the v4 and v6 /0 routes.

Fixes #1767

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

cmd/tailscale/cli/cli_test.go

@@ -181,6 +181,97 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) {
 			},
 			want: "",
 		},
+		{
+			name:    "error_advertised_routes_exit_node_removed",
+			flagSet: f("advertise-routes"),
+			curPrefs: &ipn.Prefs{
+				ControlURL: ipn.DefaultControlURL,
+				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("10.0.42.0/24"),
+					},
+				},
+				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",
+		},
+		{
+			name:    "advertised_routes_exit_node_removed",
+			flagSet: f("advertise-routes", "advertise-exit-node"),
+			curPrefs: &ipn.Prefs{
+				ControlURL: ipn.DefaultControlURL,
+				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("10.0.42.0/24"),
+					},
+				},
+				AdvertiseRoutesSet: true,
+			},
+			want: "",
+		},
+		{
+			name:    "advertised_routes_includes_the_0_routes", // but no --advertise-exit-node
+			flagSet: f("advertise-routes"),
+			curPrefs: &ipn.Prefs{
+				ControlURL: ipn.DefaultControlURL,
+				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: "",
+		},
+		{
+			name:    "advertised_routes_includes_only_one_0_route", // and no --advertise-exit-node
+			flagSet: f("advertise-routes"),
+			curPrefs: &ipn.Prefs{
+				ControlURL: ipn.DefaultControlURL,
+				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"),
+					},
+				},
+				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",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

cmd/tailscale/cli/up.go

@@ -527,6 +527,18 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre
 			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"] {
+			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"))
+		}
+
 		if hasFlag && flagSet[flagName] {
 			continue
 		}
@@ -540,6 +552,7 @@ func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Pre
 			}
 		}
 		exi, imi := ex.Interface(), im.Interface()
+
 		if reflect.DeepEqual(exi, imi) {
 			continue
 		}
@@ -575,3 +588,17 @@ func fmtSettingVal(v interface{}) string {
 	}
 	return fmt.Sprint(v)
 }
+
+func hasExitNodeRoutes(rr []netaddr.IPPrefix) bool {
+	var v4, v6 bool
+	for _, r := range rr {
+		if r.Bits == 0 {
+			if r.IP.Is4() {
+				v4 = true
+			} else if r.IP.Is6() {
+				v6 = true
+			}
+		}
+	}
+	return v4 && v6
+}