further complete test and shouldStore check for Local routes

ipn/ipnlocal/local.go

@@ -3147,11 +3147,11 @@ func (b *LocalBackend) checkFunnelEnabledLocked(p *ipn.Prefs) error {
 func (b *LocalBackend) PatchPrefsHandler(mp *ipn.MaskedPrefs) (ipn.PrefsView, error) {
 	// we believe that for the purpose of figuring out advertisedRoutes setPrefsLockedOnEntry is _only_ called when
 	// up or set is used on the tailscale cli _not_ when we calculate the new advertisedRoutes field.
-	if b.appConnector.ShouldStoreRoutes {
+	if b.appConnector != nil && b.appConnector.ShouldStoreRoutes {
 		routeInfo := b.appConnector.RouteInfo()
 		curRoutes := routeInfo.Routes(false, true, true)
 
-		if mp.AdvertiseRoutesSet && b.appConnector.ShouldStoreRoutes {
+		if mp.AdvertiseRoutesSet {
 			routeInfo.Local = mp.AdvertiseRoutes
 			b.StoreRouteInfo(routeInfo)
 			curRoutes := append(curRoutes, mp.AdvertiseRoutes...)

ipn/ipnlocal/local_test.go

@@ -2560,18 +2560,30 @@ func TestReadWriteRouteInfo(t *testing.T) {
 	}
 }
 
-func TestPatchPrefsHandler(t *testing.T) {
+func TestPatchPrefsHandlerWithPresistStore(t *testing.T) {
 	// test can read what's written
 	prefix1 := netip.MustParsePrefix("1.2.3.4/32")
 	prefix2 := netip.MustParsePrefix("1.2.3.5/32")
 	prefix3 := netip.MustParsePrefix("1.2.3.6/32")
-	rc := &appctest.RouteCollector{}
+	// rc := &appctest.RouteCollector{}
-	testAppConnector := appc.NewAppConnector(t.Logf, rc, true)
 	mp := new(ipn.MaskedPrefs)
 	mp.AdvertiseRoutesSet = true
 	mp.AdvertiseRoutes = []netip.Prefix{prefix1}
-	mp.AppConnectorSet = true
+
-	mp.AppConnector.Advertise = true
+	b := newTestBackend(t)
+	testAppConnector := appc.NewAppConnector(t.Logf, b, true)
+	b.appConnector = testAppConnector
+	b.ControlKnobs().AppCStoreRoutes.Store(true)
+
+	b.EditPrefs(&ipn.MaskedPrefs{
+		Prefs: ipn.Prefs{
+			AppConnector: ipn.AppConnectorPrefs{
+				Advertise: true,
+			},
+		},
+		AppConnectorSet: true,
+	})
+
 	now := time.Now()
 	ri := routeinfo.NewRouteInfo()
 	ri.Control = []netip.Prefix{prefix2}
@@ -2583,13 +2595,12 @@ func TestPatchPrefsHandler(t *testing.T) {
 		Routes:      routes,
 	}
 	ri.Discovered = discovered
-	rc.StoreRouteInfo(ri)
 
-	testAppConnector.ShouldStoreRoutes = true
+	b.StoreRouteInfo(ri)
+	b.AdvertiseRoute([]netip.Prefix{prefix2, prefix3}...)
+
 	testAppConnector.RouteInfo()
-	b := newTestBackend(t)
+
-	b.appConnector = testAppConnector
-	b.ControlKnobs().AppCStoreRoutes.Store(true)
 	prefView, err := b.PatchPrefsHandler(mp)
 	if err != nil {
 		t.Fatalf(err.Error())
@@ -2608,7 +2619,7 @@ func TestPatchPrefsHandler(t *testing.T) {
 	}
 
 	//Check if route is stored in Appc/Appc.routeAdvertiser
-	storedRouteInfo, _ := rc.ReadRouteInfo()
+	storedRouteInfo, _ := b.ReadRouteInfo()
 	if len(storedRouteInfo.Local) != 1 {
 		t.Fatalf("wanted %d, got %d", 1, len(storedRouteInfo.Local))
 	}
@@ -2643,4 +2654,80 @@ func TestPatchPrefsHandler(t *testing.T) {
 	if !slices.Contains(prefView2.AdvertiseRoutes().AsSlice(), prefix2) || !slices.Contains(prefView.AdvertiseRoutes().AsSlice(), prefix3) {
 		t.Fatalf("Old prefixes are no longer advertised.")
 	}
+
+}
+
+func TestPatchPrefsHandlerWithoutPresistStore(t *testing.T) {
+	// test can read what's written
+	prefix1 := netip.MustParsePrefix("1.2.3.4/32")
+	prefix2 := netip.MustParsePrefix("1.2.3.5/32")
+	prefix3 := netip.MustParsePrefix("1.2.3.6/32")
+	// rc := &appctest.RouteCollector{}
+	mp := new(ipn.MaskedPrefs)
+	mp.AdvertiseRoutesSet = true
+	mp.AdvertiseRoutes = []netip.Prefix{prefix1}
+
+	b := newTestBackend(t)
+	testAppConnector := appc.NewAppConnector(t.Logf, b, false)
+	b.appConnector = testAppConnector
+	b.ControlKnobs().AppCStoreRoutes.Store(false)
+
+	b.EditPrefs(&ipn.MaskedPrefs{
+		Prefs: ipn.Prefs{
+			AppConnector: ipn.AppConnectorPrefs{
+				Advertise: true,
+			},
+		},
+		AppConnectorSet: true,
+	})
+
+	b.AdvertiseRoute([]netip.Prefix{prefix2, prefix3}...)
+
+	testAppConnector.RouteInfo()
+
+	prefView, err := b.PatchPrefsHandler(mp)
+	if err != nil {
+		t.Fatalf(err.Error())
+	}
+
+	if prefView.AdvertiseRoutes().Len() != 1 {
+		t.Fatalf("wanted %d, got %d", 1, prefView.AdvertiseRoutes().Len())
+	}
+
+	if !slices.Contains(prefView.AdvertiseRoutes().AsSlice(), prefix1) {
+		t.Fatalf("New prefix was not advertised")
+	}
+
+	if slices.Contains(prefView.AdvertiseRoutes().AsSlice(), prefix2) || slices.Contains(prefView.AdvertiseRoutes().AsSlice(), prefix3) {
+		t.Fatalf("Old prefixes are still advertised.")
+	}
+
+	//Check if route is stored in Appc/Appc.routeAdvertiser
+	storedRouteInfo, _ := b.ReadRouteInfo()
+	if len(storedRouteInfo.Local) != 0 {
+		t.Fatalf("wanted %d, got %d", 0, len(storedRouteInfo.Local))
+	}
+	if slices.Contains(storedRouteInfo.Local, prefix1) {
+		t.Fatalf("New local route not stored.")
+	}
+
+	//Patch again with no route, see if prefix1 is removed/ prefix2, prefix3 presists.
+	mp.AdvertiseRoutes = []netip.Prefix{}
+	prefView2, err := b.PatchPrefsHandler(mp)
+	if err != nil {
+		t.Fatalf(err.Error())
+	}
+
+	if prefView2.AdvertiseRoutes().Len() != 0 {
+		t.Fatalf("wanted %d, got %d", 0, prefView.AdvertiseRoutes().Len())
+	}
+
+	if slices.Contains(prefView2.AdvertiseRoutes().AsSlice(), prefix1) {
+		t.Fatalf("Local route was not removed")
+	}
+
+	if slices.Contains(prefView2.AdvertiseRoutes().AsSlice(), prefix2) || slices.Contains(prefView.AdvertiseRoutes().AsSlice(), prefix3) {
+		t.Fatalf("Old prefixes are still advertised.")
+	}
+
 }