darwin: Put a hand-deleted unsafe_route back in line once another interface takes its prefix

examples/config.yml

@@ -338,7 +338,8 @@ tun:
   # `metric`: will default to 0 if this option is not specified
   # `install`: will default to true, controls whether this route is installed in the systems routing table.
   # On macOS, a route whose destination another interface already routes, like the local network, is installed once
-  # that route is removed. One deleted by hand stays deleted until nebula reloads or restarts.
+  # that route is removed. One deleted by hand stays deleted until nebula reloads or restarts, or until another
+  # interface routes its destination and lets it go.
   # This setting is reloadable.
   unsafe_routes:
     #- route: 172.16.1.0/24

overlay/tun_darwin_route_test.go

@@ -207,6 +207,13 @@ func routeVia(t *testing.T, cidr string) int {
 	return routeInterfaceIn(msgs, netip.MustParsePrefix(cidr))
 }
 
+// routeStateOf is what tn made of cidr when it last looked
+func routeStateOf(tn *tun, cidr string) routeState {
+	tn.routesMu.Lock()
+	defer tn.routesMu.Unlock()
+	return tn.routeStates[netip.MustParsePrefix(cidr)]
+}
+
 func routeArgs(action, cidr string, extra ...string) []string {
 	args := []string{"-q", "-n", action}
 	if netip.MustParsePrefix(cidr).Addr().Is6() {
@@ -297,6 +304,44 @@ func TestTunLeavesRouteDeletedByHand(t *testing.T) {
 	assert.Equal(t, ours, routeVia(t, cidr))
 }
 
+// configd deleting ours and adding the lan's more than routeRetryDelay apart, or a sweep landing in between, looks like
+// a hand delete. Seeing the lan hold the prefix puts ours back in line, it goes back once theirs is removed
+func TestTunYieldsAfterLeavingARouteDeletedByHand(t *testing.T) {
+	for name, tc := range map[string]struct {
+		cidr    string
+		sweep   time.Duration
+		misfile func(t *testing.T, tn *tun, cidr string)
+	}{
+		// The table went quiet with nothing in its place
+		"settled": {"10.251.6.0/24", 2 * routeRetryDelay, func(t *testing.T, tn *tun, cidr string) {
+			require.Eventually(t, func() bool { return routeStateOf(tn, cidr).kind == routeRemoved }, 5*time.Second, 50*time.Millisecond)
+		}},
+		// The sweep that caps a table that never settles
+		"swept": {"10.251.7.0/24", 200 * time.Millisecond, func(t *testing.T, tn *tun, cidr string) {
+			tn.reconcileRoutes()
+			require.Equal(t, routeRemoved, routeStateOf(tn, cidr).kind)
+		}},
+	} {
+		t.Run(name, func(t *testing.T) {
+			tn, _, _ := newRouteTestTun(t, tc.sweep, tc.cidr)
+			ours := tn.linkAddr.Load().Index
+			require.Equal(t, ours, routeVia(t, tc.cidr))
+
+			routeCmd(t, "delete", tc.cidr)
+			tc.misfile(t, tn, tc.cidr)
+
+			lo0 := routeThroughLo0(t, tc.cidr)
+			require.Eventually(t, func() bool {
+				s := routeStateOf(tn, tc.cidr)
+				return s.kind == routeYielded && s.holder == lo0
+			}, 5*time.Second, 50*time.Millisecond)
+
+			routeCmd(t, "delete", tc.cidr)
+			assert.Eventually(t, func() bool { return routeVia(t, tc.cidr) == ours }, 5*time.Second, 50*time.Millisecond)
+		})
+	}
+}
+
 // Detaching an interface deletes its routes without a routing socket message for each, like unplugging the dock whose
 // ethernet configd routed the lan through
 func TestTunAddsRouteWhenItsInterfaceDetaches(t *testing.T) {

overlay/tun_routes_darwin.go

@@ -21,7 +21,8 @@ import (
 
 // Darwin has no route metrics, the routing table holds one unscoped route per prefix. When another interface already
 // routes an unsafe_route's prefix, like the lan we started on, theirs stays and ours is added once theirs is removed.
-// Our own route removed by someone else, with nothing in its place, was removed on purpose and stays gone until a reload.
+// Our own route removed by someone else, with nothing in its place, was removed on purpose and stays gone until a reload
+// or until another interface takes the prefix, after which ours follows theirs like any other.
 
 const (
 	// routeRetryDelay is how long the system routing table must be quiet before we judge who holds a prefix. configd
@@ -45,7 +46,8 @@ const (
 	routeInstalled
 	// routeYielded is another interface holding the prefix, ours is added once it lets go
 	routeYielded
-	// routeRemoved is our route deleted by someone else with nothing in its place, it is left alone until a reload
+	// routeRemoved is our route deleted by someone else with nothing in its place, it is left alone until a reload or
+	// another interface takes the prefix
 	routeRemoved
 	// routeFailed is adding our route failing for some other reason, it is retried
 	routeFailed
@@ -214,10 +216,6 @@ func (t *tun) reconcileRoutes() {
 	for _, r := range installRoutes(*t.Routes.Load()) {
 		p := r.Cidr.Masked()
 		prev := t.routeStates[p]
-		if prev.kind == routeRemoved {
-			states[p] = prev
-			continue
-		}
 
 		holder := routeInterfaceIn(msgs, p)
 		switch {
@@ -225,15 +223,19 @@ func (t *tun) reconcileRoutes() {
 			states[p] = routeState{kind: routeInstalled}
 
 		case holder != 0:
+			// Another interface taking a prefix we left alone puts ours back in line behind theirs
 			if prev.kind != routeYielded || prev.holder != holder {
 				t.l.Info("unsafe_route is routed through another interface, it will be added back when that route is removed", "route", r.Cidr, "interface", interfaceName(holder))
 			}
 			states[p] = routeState{kind: routeYielded, holder: holder}
 
 		case prev.kind == routeInstalled:
-			t.l.Warn("unsafe_route was removed outside nebula, leaving it until reload", "route", r.Cidr)
+			t.l.Warn("unsafe_route was removed outside nebula, leaving it until a reload or another interface takes its destination", "route", r.Cidr)
 			states[p] = routeState{kind: routeRemoved}
 
+		case prev.kind == routeRemoved:
+			states[p] = prev
+
 		default:
 			err := addRoute(p, linkAddr)
 			switch {