diff --git a/appc/appconnector.go b/appc/appconnector.go index 291884065..e7b5032f0 100644 --- a/appc/appconnector.go +++ b/appc/appconnector.go @@ -134,8 +134,9 @@ type AppConnector struct { updatePub *eventbus.Publisher[appctype.RouteUpdate] storePub *eventbus.Publisher[appctype.RouteInfo] - // storeRoutesFunc will be called to persist routes if it is not nil. - storeRoutesFunc func(*appctype.RouteInfo) error + // hasStoredRoutes records whether the connector was initialized with + // persisted route information. + hasStoredRoutes bool // mu guards the fields that follow mu sync.Mutex @@ -168,16 +169,14 @@ type Config struct { EventBus *eventbus.Bus // RouteAdvertiser allows the connector to update the set of advertised routes. - // It must be non-nil. RouteAdvertiser RouteAdvertiser // RouteInfo, if non-nil, use used as the initial set of routes for the // connector. If nil, the connector starts empty. RouteInfo *appctype.RouteInfo - // StoreRoutesFunc, if non-nil, is called when the connector's routes - // change, to allow the routes to be persisted. - StoreRoutesFunc func(*appctype.RouteInfo) error + // HasStoredRoutes indicates that the connector should assume stored routes. + HasStoredRoutes bool } // NewAppConnector creates a new AppConnector. @@ -187,8 +186,6 @@ func NewAppConnector(c Config) *AppConnector { panic("missing logger") case c.EventBus == nil: panic("missing event bus") - case c.RouteAdvertiser == nil: - panic("missing route advertiser") } ec := c.EventBus.Client("appc.AppConnector") @@ -199,7 +196,7 @@ func NewAppConnector(c Config) *AppConnector { updatePub: eventbus.Publish[appctype.RouteUpdate](ec), storePub: eventbus.Publish[appctype.RouteInfo](ec), routeAdvertiser: c.RouteAdvertiser, - storeRoutesFunc: c.StoreRoutesFunc, + hasStoredRoutes: c.HasStoredRoutes, } if c.RouteInfo != nil { ac.domains = c.RouteInfo.Domains @@ -218,13 +215,19 @@ func NewAppConnector(c Config) *AppConnector { // ShouldStoreRoutes returns true if the appconnector was created with the controlknob on // and is storing its discovered routes persistently. -func (e *AppConnector) ShouldStoreRoutes() bool { - return e.storeRoutesFunc != nil -} +func (e *AppConnector) ShouldStoreRoutes() bool { return e.hasStoredRoutes } // storeRoutesLocked takes the current state of the AppConnector and persists it -func (e *AppConnector) storeRoutesLocked() error { +func (e *AppConnector) storeRoutesLocked() { if e.storePub.ShouldPublish() { + // log write rate and write size + numRoutes := int64(len(e.controlRoutes)) + for _, rs := range e.domains { + numRoutes += int64(len(rs)) + } + e.writeRateMinute.update(numRoutes) + e.writeRateDay.update(numRoutes) + e.storePub.Publish(appctype.RouteInfo{ // Clone here, as the subscriber will handle these outside our lock. Control: slices.Clone(e.controlRoutes), @@ -232,24 +235,6 @@ func (e *AppConnector) storeRoutesLocked() error { Wildcards: slices.Clone(e.wildcards), }) } - if !e.ShouldStoreRoutes() { - return nil - } - - // log write rate and write size - numRoutes := int64(len(e.controlRoutes)) - for _, rs := range e.domains { - numRoutes += int64(len(rs)) - } - e.writeRateMinute.update(numRoutes) - e.writeRateDay.update(numRoutes) - - // TODO(creachdair): Remove this once it's delivered over the event bus. - return e.storeRoutesFunc(&appctype.RouteInfo{ - Control: e.controlRoutes, - Domains: e.domains, - Wildcards: e.wildcards, - }) } // ClearRoutes removes all route state from the AppConnector. @@ -259,7 +244,8 @@ func (e *AppConnector) ClearRoutes() error { e.controlRoutes = nil e.domains = nil e.wildcards = nil - return e.storeRoutesLocked() + e.storeRoutesLocked() + return nil } // UpdateDomainsAndRoutes starts an asynchronous update of the configuration @@ -331,9 +317,9 @@ func (e *AppConnector) updateDomains(domains []string) { } } - // Everything left in oldDomains is a domain we're no longer tracking - // and if we are storing route info we can unadvertise the routes - if e.ShouldStoreRoutes() { + // Everything left in oldDomains is a domain we're no longer tracking and we + // can unadvertise the routes. + if e.hasStoredRoutes { toRemove := []netip.Prefix{} for _, addrs := range oldDomains { for _, a := range addrs { @@ -342,11 +328,13 @@ func (e *AppConnector) updateDomains(domains []string) { } if len(toRemove) != 0 { - e.queue.Add(func() { - if err := e.routeAdvertiser.UnadvertiseRoute(toRemove...); err != nil { - e.logf("failed to unadvertise routes on domain removal: %v: %v: %v", slicesx.MapKeys(oldDomains), toRemove, err) - } - }) + if ra := e.routeAdvertiser; ra != nil { + e.queue.Add(func() { + if err := e.routeAdvertiser.UnadvertiseRoute(toRemove...); err != nil { + e.logf("failed to unadvertise routes on domain removal: %v: %v: %v", slicesx.MapKeys(oldDomains), toRemove, err) + } + }) + } e.updatePub.Publish(appctype.RouteUpdate{Unadvertise: toRemove}) } } @@ -369,11 +357,10 @@ func (e *AppConnector) updateRoutes(routes []netip.Prefix) { var toRemove []netip.Prefix - // If we're storing routes and know e.controlRoutes is a good - // representation of what should be in AdvertisedRoutes we can stop - // advertising routes that used to be in e.controlRoutes but are not - // in routes. - if e.ShouldStoreRoutes() { + // If we know e.controlRoutes is a good representation of what should be in + // AdvertisedRoutes we can stop advertising routes that used to be in + // e.controlRoutes but are not in routes. + if e.hasStoredRoutes { toRemove = routesWithout(e.controlRoutes, routes) } @@ -390,23 +377,23 @@ nextRoute: } } - e.queue.Add(func() { - if err := e.routeAdvertiser.AdvertiseRoute(routes...); err != nil { - e.logf("failed to advertise routes: %v: %v", routes, err) - } - if err := e.routeAdvertiser.UnadvertiseRoute(toRemove...); err != nil { - e.logf("failed to unadvertise routes: %v: %v", toRemove, err) - } - }) + if e.routeAdvertiser != nil { + e.queue.Add(func() { + if err := e.routeAdvertiser.AdvertiseRoute(routes...); err != nil { + e.logf("failed to advertise routes: %v: %v", routes, err) + } + if err := e.routeAdvertiser.UnadvertiseRoute(toRemove...); err != nil { + e.logf("failed to unadvertise routes: %v: %v", toRemove, err) + } + }) + } e.updatePub.Publish(appctype.RouteUpdate{ Advertise: routes, Unadvertise: toRemove, }) e.controlRoutes = routes - if err := e.storeRoutesLocked(); err != nil { - e.logf("failed to store route info: %v", err) - } + e.storeRoutesLocked() } // Domains returns the currently configured domain list. @@ -485,9 +472,11 @@ func (e *AppConnector) isAddrKnownLocked(domain string, addr netip.Addr) bool { // associated with the given domain. func (e *AppConnector) scheduleAdvertisement(domain string, routes ...netip.Prefix) { e.queue.Add(func() { - if err := e.routeAdvertiser.AdvertiseRoute(routes...); err != nil { - e.logf("failed to advertise routes for %s: %v: %v", domain, routes, err) - return + if e.routeAdvertiser != nil { + if err := e.routeAdvertiser.AdvertiseRoute(routes...); err != nil { + e.logf("failed to advertise routes for %s: %v: %v", domain, routes, err) + return + } } e.updatePub.Publish(appctype.RouteUpdate{Advertise: routes}) e.mu.Lock() @@ -503,9 +492,7 @@ func (e *AppConnector) scheduleAdvertisement(domain string, routes ...netip.Pref e.logf("[v2] advertised route for %v: %v", domain, addr) } } - if err := e.storeRoutesLocked(); err != nil { - e.logf("failed to store route info: %v", err) - } + e.storeRoutesLocked() }) } diff --git a/appc/appconnector_test.go b/appc/appconnector_test.go index 91f0185d0..5c362d6fd 100644 --- a/appc/appconnector_test.go +++ b/appc/appconnector_test.go @@ -26,24 +26,15 @@ import ( "tailscale.com/util/slicesx" ) -func fakeStoreRoutes(*appctype.RouteInfo) error { return nil } - func TestUpdateDomains(t *testing.T) { ctx := t.Context() bus := eventbustest.NewBus(t) for _, shouldStore := range []bool{false, true} { - var a *AppConnector - if shouldStore { - a = NewAppConnector(Config{ - Logf: t.Logf, - EventBus: bus, - RouteAdvertiser: &appctest.RouteCollector{}, - RouteInfo: &appctype.RouteInfo{}, - StoreRoutesFunc: fakeStoreRoutes, - }) - } else { - a = NewAppConnector(Config{Logf: t.Logf, EventBus: bus, RouteAdvertiser: &appctest.RouteCollector{}}) - } + a := NewAppConnector(Config{ + Logf: t.Logf, + EventBus: bus, + HasStoredRoutes: shouldStore, + }) t.Cleanup(a.Close) a.UpdateDomains([]string{"example.com"}) @@ -76,18 +67,12 @@ func TestUpdateRoutes(t *testing.T) { for _, shouldStore := range []bool{false, true} { w := eventbustest.NewWatcher(t, bus) rc := &appctest.RouteCollector{} - var a *AppConnector - if shouldStore { - a = NewAppConnector(Config{ - Logf: t.Logf, - EventBus: bus, - RouteAdvertiser: rc, - RouteInfo: &appctype.RouteInfo{}, - StoreRoutesFunc: fakeStoreRoutes, - }) - } else { - a = NewAppConnector(Config{Logf: t.Logf, EventBus: bus, RouteAdvertiser: rc}) - } + a := NewAppConnector(Config{ + Logf: t.Logf, + EventBus: bus, + RouteAdvertiser: rc, + HasStoredRoutes: shouldStore, + }) t.Cleanup(a.Close) a.updateDomains([]string{"*.example.com"}) @@ -149,18 +134,12 @@ func TestUpdateRoutesUnadvertisesContainedRoutes(t *testing.T) { for _, shouldStore := range []bool{false, true} { w := eventbustest.NewWatcher(t, bus) rc := &appctest.RouteCollector{} - var a *AppConnector - if shouldStore { - a = NewAppConnector(Config{ - Logf: t.Logf, - EventBus: bus, - RouteAdvertiser: rc, - RouteInfo: &appctype.RouteInfo{}, - StoreRoutesFunc: fakeStoreRoutes, - }) - } else { - a = NewAppConnector(Config{Logf: t.Logf, EventBus: bus, RouteAdvertiser: rc}) - } + a := NewAppConnector(Config{ + Logf: t.Logf, + EventBus: bus, + RouteAdvertiser: rc, + HasStoredRoutes: shouldStore, + }) t.Cleanup(a.Close) mak.Set(&a.domains, "example.com", []netip.Addr{netip.MustParseAddr("192.0.2.1")}) @@ -190,18 +169,12 @@ func TestDomainRoutes(t *testing.T) { for _, shouldStore := range []bool{false, true} { w := eventbustest.NewWatcher(t, bus) rc := &appctest.RouteCollector{} - var a *AppConnector - if shouldStore { - a = NewAppConnector(Config{ - Logf: t.Logf, - EventBus: bus, - RouteAdvertiser: rc, - RouteInfo: &appctype.RouteInfo{}, - StoreRoutesFunc: fakeStoreRoutes, - }) - } else { - a = NewAppConnector(Config{Logf: t.Logf, EventBus: bus, RouteAdvertiser: rc}) - } + a := NewAppConnector(Config{ + Logf: t.Logf, + EventBus: bus, + RouteAdvertiser: rc, + HasStoredRoutes: shouldStore, + }) t.Cleanup(a.Close) a.updateDomains([]string{"example.com"}) if err := a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8")); err != nil { @@ -232,18 +205,12 @@ func TestObserveDNSResponse(t *testing.T) { for _, shouldStore := range []bool{false, true} { w := eventbustest.NewWatcher(t, bus) rc := &appctest.RouteCollector{} - var a *AppConnector - if shouldStore { - a = NewAppConnector(Config{ - Logf: t.Logf, - EventBus: bus, - RouteAdvertiser: rc, - RouteInfo: &appctype.RouteInfo{}, - StoreRoutesFunc: fakeStoreRoutes, - }) - } else { - a = NewAppConnector(Config{Logf: t.Logf, EventBus: bus, RouteAdvertiser: rc}) - } + a := NewAppConnector(Config{ + Logf: t.Logf, + EventBus: bus, + RouteAdvertiser: rc, + HasStoredRoutes: shouldStore, + }) t.Cleanup(a.Close) // a has no domains configured, so it should not advertise any routes @@ -346,18 +313,12 @@ func TestWildcardDomains(t *testing.T) { for _, shouldStore := range []bool{false, true} { w := eventbustest.NewWatcher(t, bus) rc := &appctest.RouteCollector{} - var a *AppConnector - if shouldStore { - a = NewAppConnector(Config{ - Logf: t.Logf, - EventBus: bus, - RouteAdvertiser: rc, - RouteInfo: &appctype.RouteInfo{}, - StoreRoutesFunc: fakeStoreRoutes, - }) - } else { - a = NewAppConnector(Config{Logf: t.Logf, EventBus: bus, RouteAdvertiser: rc}) - } + a := NewAppConnector(Config{ + Logf: t.Logf, + EventBus: bus, + RouteAdvertiser: rc, + HasStoredRoutes: shouldStore, + }) t.Cleanup(a.Close) a.updateDomains([]string{"*.example.com"}) @@ -522,18 +483,12 @@ func TestUpdateRouteRouteRemoval(t *testing.T) { } } - var a *AppConnector - if shouldStore { - a = NewAppConnector(Config{ - Logf: t.Logf, - EventBus: bus, - RouteAdvertiser: rc, - RouteInfo: &appctype.RouteInfo{}, - StoreRoutesFunc: fakeStoreRoutes, - }) - } else { - a = NewAppConnector(Config{Logf: t.Logf, EventBus: bus, RouteAdvertiser: rc}) - } + a := NewAppConnector(Config{ + Logf: t.Logf, + EventBus: bus, + RouteAdvertiser: rc, + HasStoredRoutes: shouldStore, + }) t.Cleanup(a.Close) // nothing has yet been advertised @@ -584,18 +539,12 @@ func TestUpdateDomainRouteRemoval(t *testing.T) { } } - var a *AppConnector - if shouldStore { - a = NewAppConnector(Config{ - Logf: t.Logf, - EventBus: bus, - RouteAdvertiser: rc, - RouteInfo: &appctype.RouteInfo{}, - StoreRoutesFunc: fakeStoreRoutes, - }) - } else { - a = NewAppConnector(Config{Logf: t.Logf, EventBus: bus, RouteAdvertiser: rc}) - } + a := NewAppConnector(Config{ + Logf: t.Logf, + EventBus: bus, + RouteAdvertiser: rc, + HasStoredRoutes: shouldStore, + }) t.Cleanup(a.Close) assertRoutes("appc init", []netip.Prefix{}, []netip.Prefix{}) @@ -665,18 +614,12 @@ func TestUpdateWildcardRouteRemoval(t *testing.T) { } } - var a *AppConnector - if shouldStore { - a = NewAppConnector(Config{ - Logf: t.Logf, - EventBus: bus, - RouteAdvertiser: rc, - RouteInfo: &appctype.RouteInfo{}, - StoreRoutesFunc: fakeStoreRoutes, - }) - } else { - a = NewAppConnector(Config{Logf: t.Logf, EventBus: bus, RouteAdvertiser: rc}) - } + a := NewAppConnector(Config{ + Logf: t.Logf, + EventBus: bus, + RouteAdvertiser: rc, + HasStoredRoutes: shouldStore, + }) t.Cleanup(a.Close) assertRoutes("appc init", []netip.Prefix{}, []netip.Prefix{}) @@ -842,8 +785,7 @@ func TestUpdateRoutesDeadlock(t *testing.T) { Logf: t.Logf, EventBus: bus, RouteAdvertiser: rc, - RouteInfo: &appctype.RouteInfo{}, - StoreRoutesFunc: fakeStoreRoutes, + HasStoredRoutes: true, }) t.Cleanup(a.Close) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index c560fdae1..bf6fab8ce 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -592,6 +592,8 @@ func (b *LocalBackend) consumeEventbusTopics(ec *eventbus.Client) func(*eventbus healthChange = healthChangeSub.Events() } changeDeltaSub := eventbus.Subscribe[netmon.ChangeDelta](ec) + routeUpdateSub := eventbus.Subscribe[appctype.RouteUpdate](ec) + storeRoutesSub := eventbus.Subscribe[appctype.RouteInfo](ec) var portlist <-chan PortlistServices if buildfeatures.HasPortList { @@ -612,10 +614,31 @@ func (b *LocalBackend) consumeEventbusTopics(ec *eventbus.Client) func(*eventbus b.onHealthChange(change) case changeDelta := <-changeDeltaSub.Events(): b.linkChange(&changeDelta) + case pl := <-portlist: if buildfeatures.HasPortList { // redundant, but explicit for linker deadcode and humans b.setPortlistServices(pl) } + case ru := <-routeUpdateSub.Events(): + // TODO(creachadair, 2025-10-02): It is currently possible for updates produced under + // one profile to arrive and be applied after a switch to another profile. + // We need to find a way to ensure that changes to the backend state are applied + // consistently in the presnce of profile changes, which currently may not happen in + // a single atomic step. See: https://github.com/tailscale/tailscale/issues/17414 + if err := b.AdvertiseRoute(ru.Advertise...); err != nil { + b.logf("appc: failed to advertise routes: %v: %v", ru.Advertise, err) + } + if err := b.UnadvertiseRoute(ru.Unadvertise...); err != nil { + b.logf("appc: failed to unadvertise routes: %v: %v", ru.Unadvertise, err) + } + case ri := <-storeRoutesSub.Events(): + // Whether or not routes should be stored can change over time. + shouldStoreRoutes := b.ControlKnobs().AppCStoreRoutes.Load() + if shouldStoreRoutes { + if err := b.storeRouteInfo(ri); err != nil { + b.logf("appc: failed to store route info: %v", err) + } + } } } } @@ -4836,35 +4859,27 @@ func (b *LocalBackend) reconfigAppConnectorLocked(nm *netmap.NetworkMap, prefs i } }() + // App connectors have been disabled. if !prefs.AppConnector().Advertise { b.appConnector.Close() // clean up a previous connector (safe on nil) b.appConnector = nil return } - shouldAppCStoreRoutes := b.ControlKnobs().AppCStoreRoutes.Load() - if b.appConnector == nil || b.appConnector.ShouldStoreRoutes() != shouldAppCStoreRoutes { - var ri *appctype.RouteInfo - var storeFunc func(*appctype.RouteInfo) error - if shouldAppCStoreRoutes { - var err error - ri, err = b.readRouteInfoLocked() - if err != nil { - ri = &appctype.RouteInfo{} - if err != ipn.ErrStateNotExist { - b.logf("Unsuccessful Read RouteInfo: ", err) - } - } - storeFunc = b.storeRouteInfo + // We don't (yet) have an app connector configured, or the configured + // connector has a different route persistence setting. + shouldStoreRoutes := b.ControlKnobs().AppCStoreRoutes.Load() + if b.appConnector == nil || (shouldStoreRoutes != b.appConnector.ShouldStoreRoutes()) { + ri, err := b.readRouteInfoLocked() + if err != nil && err != ipn.ErrStateNotExist { + b.logf("Unsuccessful Read RouteInfo: %v", err) } - b.appConnector.Close() // clean up a previous connector (safe on nil) b.appConnector = appc.NewAppConnector(appc.Config{ Logf: b.logf, EventBus: b.sys.Bus.Get(), - RouteAdvertiser: b, RouteInfo: ri, - StoreRoutesFunc: storeFunc, + HasStoredRoutes: shouldStoreRoutes, }) } if nm == nil { @@ -7008,9 +7023,9 @@ func (b *LocalBackend) ObserveDNSResponse(res []byte) error { // ErrDisallowedAutoRoute is returned by AdvertiseRoute when a route that is not allowed is requested. var ErrDisallowedAutoRoute = errors.New("route is not allowed") -// AdvertiseRoute implements the appc.RouteAdvertiser interface. It sets a new -// route advertisement if one is not already present in the existing routes. -// If the route is disallowed, ErrDisallowedAutoRoute is returned. +// AdvertiseRoute implements the appctype.RouteAdvertiser interface. It sets a +// new route advertisement if one is not already present in the existing +// routes. If the route is disallowed, ErrDisallowedAutoRoute is returned. func (b *LocalBackend) AdvertiseRoute(ipps ...netip.Prefix) error { finalRoutes := b.Prefs().AdvertiseRoutes().AsSlice() var newRoutes []netip.Prefix @@ -7066,8 +7081,8 @@ func coveredRouteRangeNoDefault(finalRoutes []netip.Prefix, ipp netip.Prefix) bo return false } -// UnadvertiseRoute implements the appc.RouteAdvertiser interface. It removes -// a route advertisement if one is present in the existing routes. +// UnadvertiseRoute implements the appctype.RouteAdvertiser interface. It +// removes a route advertisement if one is present in the existing routes. func (b *LocalBackend) UnadvertiseRoute(toRemove ...netip.Prefix) error { currentRoutes := b.Prefs().AdvertiseRoutes().AsSlice() finalRoutes := currentRoutes[:0] @@ -7095,7 +7110,7 @@ func namespaceKeyForCurrentProfile(pm *profileManager, key ipn.StateKey) ipn.Sta const routeInfoStateStoreKey ipn.StateKey = "_routeInfo" -func (b *LocalBackend) storeRouteInfo(ri *appctype.RouteInfo) error { +func (b *LocalBackend) storeRouteInfo(ri appctype.RouteInfo) error { if !buildfeatures.HasAppConnectors { return feature.ErrUnavailable } diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index bc8bd2a67..168f76268 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -75,8 +75,6 @@ import ( "tailscale.com/wgengine/wgcfg" ) -func fakeStoreRoutes(*appctype.RouteInfo) error { return nil } - func inRemove(ip netip.Addr) bool { for _, pfx := range removeFromDefaultRoute { if pfx.Contains(ip) { @@ -2321,14 +2319,9 @@ func TestOfferingAppConnector(t *testing.T) { if b.OfferingAppConnector() { t.Fatal("unexpected offering app connector") } - rc := &appctest.RouteCollector{} - if shouldStore { - b.appConnector = appc.NewAppConnector(appc.Config{ - Logf: t.Logf, EventBus: bus, RouteAdvertiser: rc, RouteInfo: &appctype.RouteInfo{}, StoreRoutesFunc: fakeStoreRoutes, - }) - } else { - b.appConnector = appc.NewAppConnector(appc.Config{Logf: t.Logf, EventBus: bus, RouteAdvertiser: rc}) - } + b.appConnector = appc.NewAppConnector(appc.Config{ + Logf: t.Logf, EventBus: bus, HasStoredRoutes: shouldStore, + }) if !b.OfferingAppConnector() { t.Fatal("unexpected not offering app connector") } @@ -2379,6 +2372,7 @@ func TestObserveDNSResponse(t *testing.T) { for _, shouldStore := range []bool{false, true} { b := newTestBackend(t) bus := b.sys.Bus.Get() + w := eventbustest.NewWatcher(t, bus) // ensure no error when no app connector is configured if err := b.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8")); err != nil { @@ -2386,28 +2380,30 @@ func TestObserveDNSResponse(t *testing.T) { } rc := &appctest.RouteCollector{} - if shouldStore { - b.appConnector = appc.NewAppConnector(appc.Config{ - Logf: t.Logf, - EventBus: bus, - RouteAdvertiser: rc, - RouteInfo: &appctype.RouteInfo{}, - StoreRoutesFunc: fakeStoreRoutes, - }) - } else { - b.appConnector = appc.NewAppConnector(appc.Config{Logf: t.Logf, EventBus: bus, RouteAdvertiser: rc}) - } - b.appConnector.UpdateDomains([]string{"example.com"}) - b.appConnector.Wait(context.Background()) + a := appc.NewAppConnector(appc.Config{ + Logf: t.Logf, + EventBus: bus, + RouteAdvertiser: rc, + HasStoredRoutes: shouldStore, + }) + a.UpdateDomains([]string{"example.com"}) + a.Wait(t.Context()) + b.appConnector = a if err := b.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8")); err != nil { t.Errorf("ObserveDNSResponse: %v", err) } - b.appConnector.Wait(context.Background()) + a.Wait(t.Context()) wantRoutes := []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")} if !slices.Equal(rc.Routes(), wantRoutes) { t.Fatalf("got routes %v, want %v", rc.Routes(), wantRoutes) } + + if err := eventbustest.Expect(w, + eqUpdate(appctype.RouteUpdate{Advertise: mustPrefix("192.0.0.8/32")}), + ); err != nil { + t.Error(err) + } } } @@ -2558,7 +2554,7 @@ func TestBackfillAppConnectorRoutes(t *testing.T) { // Store the test IP in profile data, but not in Prefs.AdvertiseRoutes. b.ControlKnobs().AppCStoreRoutes.Store(true) - if err := b.storeRouteInfo(&appctype.RouteInfo{ + if err := b.storeRouteInfo(appctype.RouteInfo{ Domains: map[string][]netip.Addr{ "example.com": {ip}, }, @@ -5511,10 +5507,10 @@ func TestReadWriteRouteInfo(t *testing.T) { b.pm.currentProfile = prof1.View() // set up routeInfo - ri1 := &appctype.RouteInfo{} + ri1 := appctype.RouteInfo{} ri1.Wildcards = []string{"1"} - ri2 := &appctype.RouteInfo{} + ri2 := appctype.RouteInfo{} ri2.Wildcards = []string{"2"} // read before write @@ -7066,3 +7062,41 @@ func toStrings[T ~string](in []T) []string { } return out } + +type textUpdate struct { + Advertise []string + Unadvertise []string +} + +func routeUpdateToText(u appctype.RouteUpdate) textUpdate { + var out textUpdate + for _, p := range u.Advertise { + out.Advertise = append(out.Advertise, p.String()) + } + for _, p := range u.Unadvertise { + out.Unadvertise = append(out.Unadvertise, p.String()) + } + return out +} + +func mustPrefix(ss ...string) (out []netip.Prefix) { + for _, s := range ss { + out = append(out, netip.MustParsePrefix(s)) + } + return +} + +// eqUpdate generates an eventbus test filter that matches an appctype.RouteUpdate +// message equal to want, or reports an error giving a human-readable diff. +// +// TODO(creachadair): This is copied from the appc test package, but we can't +// put it into the appctest package because the appc tests depend on it and +// that makes a cycle. Clean up those tests and put this somewhere common. +func eqUpdate(want appctype.RouteUpdate) func(appctype.RouteUpdate) error { + return func(got appctype.RouteUpdate) error { + if diff := cmp.Diff(routeUpdateToText(got), routeUpdateToText(want)); diff != "" { + return fmt.Errorf("wrong update (-got, +want):\n%s", diff) + } + return nil + } +} diff --git a/ipn/ipnlocal/peerapi_test.go b/ipn/ipnlocal/peerapi_test.go index a16d55b8c..7c2e677a4 100644 --- a/ipn/ipnlocal/peerapi_test.go +++ b/ipn/ipnlocal/peerapi_test.go @@ -256,22 +256,12 @@ func TestPeerAPIPrettyReplyCNAME(t *testing.T) { reg := new(usermetric.Registry) eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0, ht, reg, sys.Bus.Get(), sys.Set) pm := must.Get(newProfileManager(new(mem.Store), t.Logf, ht)) - var a *appc.AppConnector - if shouldStore { - a = appc.NewAppConnector(appc.Config{ - Logf: t.Logf, - EventBus: sys.Bus.Get(), - RouteAdvertiser: &appctest.RouteCollector{}, - RouteInfo: &appctype.RouteInfo{}, - StoreRoutesFunc: fakeStoreRoutes, - }) - } else { - a = appc.NewAppConnector(appc.Config{ - Logf: t.Logf, - EventBus: sys.Bus.Get(), - RouteAdvertiser: &appctest.RouteCollector{}, - }) - } + a := appc.NewAppConnector(appc.Config{ + Logf: t.Logf, + EventBus: sys.Bus.Get(), + HasStoredRoutes: shouldStore, + }) + t.Cleanup(a.Close) sys.Set(pm.Store()) sys.Set(eng) @@ -329,11 +319,11 @@ func TestPeerAPIPrettyReplyCNAME(t *testing.T) { func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) { for _, shouldStore := range []bool{false, true} { - ctx := context.Background() var h peerAPIHandler h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345") sys := tsd.NewSystemWithBus(eventbustest.NewBus(t)) + bw := eventbustest.NewWatcher(t, sys.Bus.Get()) rc := &appctest.RouteCollector{} ht := health.NewTracker(sys.Bus.Get()) @@ -341,18 +331,13 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) { reg := new(usermetric.Registry) eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0, ht, reg, sys.Bus.Get(), sys.Set) - var a *appc.AppConnector - if shouldStore { - a = appc.NewAppConnector(appc.Config{ - Logf: t.Logf, - EventBus: sys.Bus.Get(), - RouteAdvertiser: rc, - RouteInfo: &appctype.RouteInfo{}, - StoreRoutesFunc: fakeStoreRoutes, - }) - } else { - a = appc.NewAppConnector(appc.Config{Logf: t.Logf, EventBus: sys.Bus.Get(), RouteAdvertiser: rc}) - } + a := appc.NewAppConnector(appc.Config{ + Logf: t.Logf, + EventBus: sys.Bus.Get(), + RouteAdvertiser: rc, + HasStoredRoutes: shouldStore, + }) + t.Cleanup(a.Close) sys.Set(pm.Store()) sys.Set(eng) @@ -362,7 +347,7 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) { h.ps = &peerAPIServer{b: b} h.ps.b.appConnector.UpdateDomains([]string{"example.com"}) - h.ps.b.appConnector.Wait(ctx) + a.Wait(t.Context()) h.ps.resolver = &fakeResolver{build: func(b *dnsmessage.Builder) { b.AResource( @@ -392,12 +377,18 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) { if w.Code != http.StatusOK { t.Errorf("unexpected status code: %v", w.Code) } - h.ps.b.appConnector.Wait(ctx) + a.Wait(t.Context()) wantRoutes := []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")} if !slices.Equal(rc.Routes(), wantRoutes) { t.Errorf("got %v; want %v", rc.Routes(), wantRoutes) } + + if err := eventbustest.Expect(bw, + eqUpdate(appctype.RouteUpdate{Advertise: mustPrefix("192.0.0.8/32")}), + ); err != nil { + t.Error(err) + } } } @@ -408,24 +399,20 @@ func TestPeerAPIReplyToDNSQueriesAreObservedWithCNAMEFlattening(t *testing.T) { h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345") sys := tsd.NewSystemWithBus(eventbustest.NewBus(t)) + bw := eventbustest.NewWatcher(t, sys.Bus.Get()) ht := health.NewTracker(sys.Bus.Get()) reg := new(usermetric.Registry) rc := &appctest.RouteCollector{} eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0, ht, reg, sys.Bus.Get(), sys.Set) pm := must.Get(newProfileManager(new(mem.Store), t.Logf, ht)) - var a *appc.AppConnector - if shouldStore { - a = appc.NewAppConnector(appc.Config{ - Logf: t.Logf, - EventBus: sys.Bus.Get(), - RouteAdvertiser: rc, - RouteInfo: &appctype.RouteInfo{}, - StoreRoutesFunc: fakeStoreRoutes, - }) - } else { - a = appc.NewAppConnector(appc.Config{Logf: t.Logf, EventBus: sys.Bus.Get(), RouteAdvertiser: rc}) - } + a := appc.NewAppConnector(appc.Config{ + Logf: t.Logf, + EventBus: sys.Bus.Get(), + RouteAdvertiser: rc, + HasStoredRoutes: shouldStore, + }) + t.Cleanup(a.Close) sys.Set(pm.Store()) sys.Set(eng) @@ -482,6 +469,12 @@ func TestPeerAPIReplyToDNSQueriesAreObservedWithCNAMEFlattening(t *testing.T) { if !slices.Equal(rc.Routes(), wantRoutes) { t.Errorf("got %v; want %v", rc.Routes(), wantRoutes) } + + if err := eventbustest.Expect(bw, + eqUpdate(appctype.RouteUpdate{Advertise: mustPrefix("192.0.0.8/32")}), + ); err != nil { + t.Error(err) + } } }