When deleting a domain, only remove a route if the route is no longer used in routeInfo.fran/appc-store-routes-by-source

3 files changed, 104 insertions, 28 deletions

diff --git a/appc/appconnector.go b/appc/appconnector.go
index 34e9845ff..cd45f7c86 100644
--- a/appc/appconnector.go
+++ b/appc/appconnector.go
@@ -223,15 +223,19 @@ func (e *AppConnector) updateDomains(domains []string) {
 
 	if shouldStoreRoutes {
 		e.UpdateRouteInfo(routeInfo)
-		// everything left in oldDiscovered a) won't be in e.domains and b) can be unadvertised if it's not in local or control
+		// every domain left in oldDiscovered won't be in e.domains
+		// routes can be unadvertised if it's not in local, control, or new discovered
+		currentRoutes := routeInfo.Routes(true, true, true)
+		slices.SortFunc(currentRoutes, comparePrefix)
+		currentRoutes = slices.Compact(currentRoutes)
 		for domainName, domainsRoutes := range oldDiscovered {
 			if domainsRoutes != nil {
 				toRemove := []netip.Prefix{}
 				for _, route := range domainsRoutes.RoutesSlice() {
-					if !slices.Contains(routeInfo.Local, route) {
+					_, ok := slices.BinarySearchFunc(currentRoutes, route, comparePrefix)
+					if !ok {
 						toRemove = append(toRemove, route)
 					}
-					// TODO check control also
 				}
 				e.logf("unadvertising %d routes for domain: %s", len(toRemove), domainName)
 				e.scheduleUnadvertisement(domainName, toRemove...)
@@ -564,3 +568,7 @@ func (e *AppConnector) addDomainAddrLocked(domain string, addr netip.Addr) {
 func compareAddr(l, r netip.Addr) int {
 	return l.Compare(r)
 }
+
+func comparePrefix(i, j netip.Prefix) int {
+	return i.Addr().Compare(j.Addr())
+}
diff --git a/appc/appconnector_test.go b/appc/appconnector_test.go
index aa57f6232..9ed14cd46 100644
--- a/appc/appconnector_test.go
+++ b/appc/appconnector_test.go
@@ -9,6 +9,7 @@ import (
 	"reflect"
 	"slices"
 	"testing"
+	"time"
 
 	xmaps "golang.org/x/exp/maps"
 	"golang.org/x/net/dns/dnsmessage"
@@ -18,34 +19,99 @@ import (
 	"tailscale.com/util/must"
 )
 
-func TestUpdateDomains(t *testing.T) {
-	for _, shouldStore := range []bool{true, false} {
-		ctx := context.Background()
-		a := NewAppConnector(t.Logf, nil, shouldStore)
-		a.routeAdvertiser = &appctest.RouteCollector{}
-		a.UpdateRouteInfo(routeinfo.NewRouteInfo())
-		a.UpdateDomains([]string{"example.com"})
+func TestUpdateDomainsWithPresistStore(t *testing.T) {
 
-		a.Wait(ctx)
-		if got, want := a.Domains().AsSlice(), []string{"example.com"}; !slices.Equal(got, want) {
-			t.Errorf("got %v; want %v", got, want)
-		}
+	ctx := context.Background()
+	a := NewAppConnector(t.Logf, nil, true)
+	TestRouteAdvertiser := &appctest.RouteCollector{}
+	a.routeAdvertiser = TestRouteAdvertiser
+	prefixControl := netip.MustParsePrefix("192.0.0.8/32")
+	prefixLocal := netip.MustParsePrefix("192.0.0.9/32")
+	prefixDiscovered := netip.MustParsePrefix("192.0.0.10/32")
+	prefixShouldUnadvertise := netip.MustParsePrefix("192.0.0.11/32")
+	ri := routeinfo.NewRouteInfo()
+	ri.Control = []netip.Prefix{prefixControl}
+	ri.Local = []netip.Prefix{prefixLocal}
+	a.UpdateRouteInfo(ri)
+	a.UpdateDomains([]string{"example.com", "test.com"})
 
-		addr := netip.MustParseAddr("192.0.0.8")
-		a.domains["example.com"] = append(a.domains["example.com"], addr)
-		a.UpdateDomains([]string{"example.com"})
-		a.Wait(ctx)
+	a.Wait(ctx)
+	if got, want := a.Domains().AsSlice(), []string{"example.com", "test.com"}; !slices.Equal(got, want) {
+		t.Errorf("got %v; want %v", got, want)
+	}
+	if _, hasKey := a.routeInfo.Discovered["example.com"]; !hasKey {
+		t.Errorf("Discovered did not record the test domain.")
+	}
 
-		if got, want := a.domains["example.com"], []netip.Addr{addr}; !slices.Equal(got, want) {
-			t.Errorf("got %v; want %v", got, want)
-		}
+	now := time.Now()
+	testRoutes := make(map[netip.Prefix]time.Time)
+	testRoutes[prefixDiscovered] = now
+	exampleRoutes := make(map[netip.Prefix]time.Time)
+	exampleRoutes[prefixShouldUnadvertise] = now
+	exampleRoutes[prefixControl] = now
+	exampleRoutes[prefixLocal] = now
+	exampleRoutes[prefixDiscovered] = now
+	ri.Discovered["test.com"] = &routeinfo.DatedRoutes{Routes: testRoutes, LastCleanup: now}
+	ri.Discovered["example.com"] = &routeinfo.DatedRoutes{Routes: exampleRoutes, LastCleanup: now}
+	a.UpdateRouteInfo(ri)
+	TestRouteAdvertiser.SetRoutes(ri.Discovered["example.com"].RoutesSlice())
 
-		// domains are explicitly downcased on set.
-		a.UpdateDomains([]string{"UP.EXAMPLE.COM"})
-		a.Wait(ctx)
-		if got, want := xmaps.Keys(a.domains), []string{"up.example.com"}; !slices.Equal(got, want) {
-			t.Errorf("got %v; want %v", got, want)
-		}
+	addrControl := netip.MustParseAddr("192.0.0.8")
+	addrLocal := netip.MustParseAddr("192.0.0.9")
+	addrDiscovered := netip.MustParseAddr("192.0.0.10")
+	addrShouldUnadvertise := netip.MustParseAddr("192.0.0.11")
+	a.domains["example.com"] = append(a.domains["example.com"], []netip.Addr{addrControl, addrLocal, addrDiscovered, addrShouldUnadvertise}...)
+	a.domains["test.com"] = append(a.domains["test.com"], addrDiscovered)
+	a.UpdateDomains([]string{"example.com", "test.com"})
+	a.Wait(ctx)
+
+	if got, want := a.domains["example.com"], []netip.Addr{addrControl, addrLocal, addrDiscovered, addrShouldUnadvertise}; !slices.Equal(got, want) {
+		t.Errorf("got %v; want %v", got, want)
+	}
+
+	// domains are explicitly downcased on set.
+	a.UpdateDomains([]string{"UP.EXAMPLE.COM", "test.com"})
+	a.Wait(ctx)
+	if got, want := xmaps.Keys(a.domains), []string{"up.example.com", "test.com"}; !slices.Equal(got, want) {
+		t.Errorf("got %v; want %v", got, want)
+	}
+
+	if got, want := TestRouteAdvertiser.RemovedRoutes(), []netip.Prefix{prefixShouldUnadvertise}; !slices.Equal(got, want) {
+		t.Errorf("got %v; want %v", got, want)
+	}
+
+	if got, want := TestRouteAdvertiser.Routes(), []netip.Prefix{prefixControl, prefixLocal, prefixDiscovered}; !slices.Equal(got, want) {
+		t.Errorf("got %v; want %v", got, want)
+	}
+
+}
+
+func TestUpdateDomainsWithoutPresistStore(t *testing.T) {
+	ctx := context.Background()
+	a := NewAppConnector(t.Logf, nil, false)
+	a.routeAdvertiser = &appctest.RouteCollector{}
+	a.UpdateRouteInfo(routeinfo.NewRouteInfo())
+	a.UpdateDomains([]string{"example.com"})
+
+	a.Wait(ctx)
+	if got, want := a.Domains().AsSlice(), []string{"example.com"}; !slices.Equal(got, want) {
+		t.Errorf("got %v; want %v", got, want)
+	}
+
+	addr := netip.MustParseAddr("192.0.0.8")
+	a.domains["example.com"] = append(a.domains["example.com"], addr)
+	a.UpdateDomains([]string{"example.com"})
+	a.Wait(ctx)
+
+	if got, want := a.domains["example.com"], []netip.Addr{addr}; !slices.Equal(got, want) {
+		t.Errorf("got %v; want %v", got, want)
+	}
+
+	// domains are explicitly downcased on set.
+	a.UpdateDomains([]string{"UP.EXAMPLE.COM"})
+	a.Wait(ctx)
+	if got, want := xmaps.Keys(a.domains), []string{"up.example.com"}; !slices.Equal(got, want) {
+		t.Errorf("got %v; want %v", got, want)
 	}
 }
 
diff --git a/appc/routeinfo/routeinfo.go b/appc/routeinfo/routeinfo.go
index 1203f90aa..d0a5641de 100644
--- a/appc/routeinfo/routeinfo.go
+++ b/appc/routeinfo/routeinfo.go
@@ -40,7 +40,9 @@ func (ri *RouteInfo) Routes(local, control, discovered bool) []netip.Prefix {
 
 	if discovered {
 		for _, dr := range ri.Discovered {
-			ret = append(ret, dr.RoutesSlice()...)
+			if dr != nil {
+				ret = append(ret, dr.RoutesSlice()...)
+			}
 		}
 	}
 	return ret