diff --git a/appc/appconnector.go b/appc/appconnector.go index e04248815..184b3145c 100644 --- a/appc/appconnector.go +++ b/appc/appconnector.go @@ -87,7 +87,7 @@ func NewAppConnector(logf logger.Logf, routeAdvertiser RouteAdvertiser, shouldSt logf: logger.WithPrefix(logf, "appc: "), routeAdvertiser: routeAdvertiser, ShouldStoreRoutes: shouldStoreRoutes, - routeInfo: routeinfo.NewRouteInfo(), + routeInfo: nil, } } diff --git a/appc/appconnector_test.go b/appc/appconnector_test.go index 86508e36b..f04fbd673 100644 --- a/appc/appconnector_test.go +++ b/appc/appconnector_test.go @@ -69,12 +69,15 @@ func TestUpdateRoutes(t *testing.T) { routes := []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24"), netip.MustParsePrefix("192.0.0.1/32")} a.updateRoutes(routes) - wantRouteInfoControlRoutes := []netip.Prefix{} if shouldStore { - wantRouteInfoControlRoutes = routes - } - if !slices.Equal(a.routeInfo.Control, wantRouteInfoControlRoutes) { - t.Fatalf("got %v, want %v (shouldStore=%t)", a.routeInfo.Control, wantRouteInfoControlRoutes, shouldStore) + wantRouteInfoControlRoutes := routes + if !slices.Equal(a.routeInfo.Control, wantRouteInfoControlRoutes) { + t.Fatalf("got %v, want %v (shouldStore=%t)", a.routeInfo.Control, wantRouteInfoControlRoutes, shouldStore) + } + } else { + if a.routeInfo != nil { + t.Fatalf("got %v, want %v (shouldStore=%t)", a.routeInfo.Control, nil, shouldStore) + } } slices.SortFunc(rc.Routes(), prefixCompare) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 41ce466b9..56e024f06 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -3192,14 +3192,16 @@ func (b *LocalBackend) SetUseExitNodeEnabled(v bool) (ipn.PrefsView, 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. - routeInfo := b.appConnector.RouteInfo() - curRoutes := routeInfo.Routes(false, true, true) + if b.appConnector.ShouldStoreRoutes { + routeInfo := b.appConnector.RouteInfo() + curRoutes := routeInfo.Routes(false, true, true) - if mp.AdvertiseRoutesSet && b.appConnector.ShouldStoreRoutes { - routeInfo.Local = mp.AdvertiseRoutes - b.StoreRouteInfo(routeInfo) - curRoutes := append(curRoutes, mp.AdvertiseRoutes...) - mp.AdvertiseRoutes = curRoutes + if mp.AdvertiseRoutesSet && b.appConnector.ShouldStoreRoutes { + routeInfo.Local = mp.AdvertiseRoutes + b.StoreRouteInfo(routeInfo) + curRoutes := append(curRoutes, mp.AdvertiseRoutes...) + mp.AdvertiseRoutes = curRoutes + } } return b.EditPrefs(mp) } diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 94dd1f9db..22935f35f 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -2696,6 +2696,8 @@ func TestPatchPrefsHandler(t *testing.T) { mp := new(ipn.MaskedPrefs) mp.AdvertiseRoutesSet = true mp.AdvertiseRoutes = []netip.Prefix{prefix1} + mp.AppConnectorSet = true + mp.AppConnector.Advertise = true now := time.Now() ri := routeinfo.NewRouteInfo() ri.Control = []netip.Prefix{prefix2} @@ -2707,13 +2709,13 @@ func TestPatchPrefsHandler(t *testing.T) { Routes: routes, } ri.Discovered = discovered - b := newTestBackend(t) - b.reconfigAppConnectorLocked(b.netMap, b.pm.prefs) - if b.appConnector != nil { - t.Fatal("unexpected app connector") - } - b.appConnector = testAppConnector + rc.StoreRouteInfo(ri) + testAppConnector.ShouldStoreRoutes = true + 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()) @@ -2731,8 +2733,40 @@ func TestPatchPrefsHandler(t *testing.T) { t.Fatalf("Old prefixes are no longer advertised.") } - //TODO: test if route if stored in Appc/Appc.routeAdvertiser + //Check if route is stored in Appc/Appc.routeAdvertiser + storedRouteInfo, _ := rc.ReadRouteInfo() + if len(storedRouteInfo.Local) != 1 { + t.Fatalf("wanted %d, got %d", 1, len(storedRouteInfo.Local)) + } + if !slices.Contains(storedRouteInfo.Local, prefix1) { + t.Fatalf("New local route not stored.") + } - //TODO: patch again with no route, see if prefix1 is removed/ prefix2, prefix3 presists. + //Check if the routes in control and discovered are presisted. + routesShouldPresist := storedRouteInfo.Routes(false, true, true) + if len(routesShouldPresist) != 2 { + t.Fatalf("wanted %d, got %d", 1, len(routesShouldPresist)) + } + if !slices.Contains(routesShouldPresist, prefix2) || !slices.Contains(routesShouldPresist, prefix3) { + t.Fatalf("Pre-existed routes not presisted.") + } + //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() != 2 { + t.Fatalf("wanted %d, got %d", 2, 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 no longer advertised.") + } }