appc: unadvertise routes when reconfiguring app connector

If the controlknob to persist app connector routes is enabled, when
reconfiguring an app connector unadvertise routes that are no longer
relevant.

Updates #11008
Signed-off-by: Fran Bull <fran@tailscale.com>

appc/appconnector.go

@@ -23,6 +23,7 @@
 	"tailscale.com/util/dnsname"
 	"tailscale.com/util/execqueue"
 	"tailscale.com/util/mak"
+	"tailscale.com/util/slicesx"
 )
 
 // RouteAdvertiser is an interface that allows the AppConnector to advertise
@@ -166,10 +167,26 @@ func (e *AppConnector) updateDomains(domains []string) {
 		for _, wc := range e.wildcards {
 			if dnsname.HasSuffix(d, wc) {
 				e.domains[d] = addrs
+				delete(oldDomains, d)
 				break
 			}
 		}
 	}
+
+	// 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() {
+		toRemove := []netip.Prefix{}
+		for _, addrs := range oldDomains {
+			for _, a := range addrs {
+				toRemove = append(toRemove, netip.PrefixFrom(a, a.BitLen()))
+			}
+		}
+		if err := e.routeAdvertiser.UnadvertiseRoute(toRemove...); err != nil {
+			e.logf("failed to unadvertise routes on domain removal: %v: %v: %v", xmaps.Keys(oldDomains), toRemove, err)
+		}
+	}
+
 	e.logf("handling domains: %v and wildcards: %v", xmaps.Keys(e.domains), e.wildcards)
 }
 
@@ -193,6 +210,14 @@ 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() {
+		toRemove = routesWithout(e.controlRoutes, routes)
+	}
+
 nextRoute:
 	for _, r := range routes {
 		for _, addr := range e.domains {
@@ -447,3 +472,15 @@ func (e *AppConnector) addDomainAddrLocked(domain string, addr netip.Addr) {
 func compareAddr(l, r netip.Addr) int {
 	return l.Compare(r)
 }
+
+// routesWithout returns a without b where a and b
+// are unsorted slices of netip.Prefix
+func routesWithout(a, b []netip.Prefix) []netip.Prefix {
+	m := make(map[netip.Prefix]bool, len(b))
+	for _, p := range b {
+		m[p] = true
+	}
+	return slicesx.Filter(make([]netip.Prefix, 0, len(a)), a, func(p netip.Prefix) bool {
+		return !m[p]
+	})
+}

appc/appconnector_test.go

@@ -24,9 +24,9 @@ func TestUpdateDomains(t *testing.T) {
 		ctx := context.Background()
 		var a *AppConnector
 		if shouldStore {
-			a = NewAppConnector(t.Logf, nil, &RouteInfo{}, fakeStoreRoutes)
+			a = NewAppConnector(t.Logf, &appctest.RouteCollector{}, &RouteInfo{}, fakeStoreRoutes)
 		} else {
-			a = NewAppConnector(t.Logf, nil, nil, nil)
+			a = NewAppConnector(t.Logf, &appctest.RouteCollector{}, nil, nil)
 		}
 		a.UpdateDomains([]string{"example.com"})
 
@@ -354,3 +354,169 @@ func prefixCompare(a, b netip.Prefix) int {
 	}
 	return a.Addr().Compare(b.Addr())
 }
+
+func prefixes(in ...string) []netip.Prefix {
+	toRet := make([]netip.Prefix, len(in))
+	for i, s := range in {
+		toRet[i] = netip.MustParsePrefix(s)
+	}
+	return toRet
+}
+
+func TestUpdateRouteRouteRemoval(t *testing.T) {
+	for _, shouldStore := range []bool{false, true} {
+		ctx := context.Background()
+		rc := &appctest.RouteCollector{}
+
+		assertRoutes := func(prefix string, routes, removedRoutes []netip.Prefix) {
+			if !slices.Equal(routes, rc.Routes()) {
+				t.Fatalf("%s: (shouldStore=%t) routes want %v, got %v", prefix, shouldStore, routes, rc.Routes())
+			}
+			if !slices.Equal(removedRoutes, rc.RemovedRoutes()) {
+				t.Fatalf("%s: (shouldStore=%t) removedRoutes want %v, got %v", prefix, shouldStore, removedRoutes, rc.RemovedRoutes())
+			}
+		}
+
+		var a *AppConnector
+		if shouldStore {
+			a = NewAppConnector(t.Logf, rc, &RouteInfo{}, fakeStoreRoutes)
+		} else {
+			a = NewAppConnector(t.Logf, rc, nil, nil)
+		}
+		// nothing has yet been advertised
+		assertRoutes("appc init", []netip.Prefix{}, []netip.Prefix{})
+
+		a.UpdateDomainsAndRoutes([]string{}, prefixes("1.2.3.1/32", "1.2.3.2/32"))
+		a.Wait(ctx)
+		// the routes passed to UpdateDomainsAndRoutes have been advertised
+		assertRoutes("simple update", prefixes("1.2.3.1/32", "1.2.3.2/32"), []netip.Prefix{})
+
+		// one route the same, one different
+		a.UpdateDomainsAndRoutes([]string{}, prefixes("1.2.3.1/32", "1.2.3.3/32"))
+		a.Wait(ctx)
+		// old behavior: routes are not removed, resulting routes are both old and new
+		// (we have dupe 1.2.3.1 routes because the test RouteAdvertiser doesn't have the deduplication
+		// the real one does)
+		wantRoutes := prefixes("1.2.3.1/32", "1.2.3.2/32", "1.2.3.1/32", "1.2.3.3/32")
+		wantRemovedRoutes := []netip.Prefix{}
+		if shouldStore {
+			// new behavior: routes are removed, resulting routes are new only
+			wantRoutes = prefixes("1.2.3.1/32", "1.2.3.1/32", "1.2.3.3/32")
+			wantRemovedRoutes = prefixes("1.2.3.2/32")
+		}
+		assertRoutes("removal", wantRoutes, wantRemovedRoutes)
+	}
+}
+
+func TestUpdateDomainRouteRemoval(t *testing.T) {
+	for _, shouldStore := range []bool{false, true} {
+		ctx := context.Background()
+		rc := &appctest.RouteCollector{}
+
+		assertRoutes := func(prefix string, routes, removedRoutes []netip.Prefix) {
+			if !slices.Equal(routes, rc.Routes()) {
+				t.Fatalf("%s: (shouldStore=%t) routes want %v, got %v", prefix, shouldStore, routes, rc.Routes())
+			}
+			if !slices.Equal(removedRoutes, rc.RemovedRoutes()) {
+				t.Fatalf("%s: (shouldStore=%t) removedRoutes want %v, got %v", prefix, shouldStore, removedRoutes, rc.RemovedRoutes())
+			}
+		}
+
+		var a *AppConnector
+		if shouldStore {
+			a = NewAppConnector(t.Logf, rc, &RouteInfo{}, fakeStoreRoutes)
+		} else {
+			a = NewAppConnector(t.Logf, rc, nil, nil)
+		}
+		assertRoutes("appc init", []netip.Prefix{}, []netip.Prefix{})
+
+		a.UpdateDomainsAndRoutes([]string{"a.example.com", "b.example.com"}, []netip.Prefix{})
+		a.Wait(ctx)
+		// adding domains doesn't immediately cause any routes to be advertised
+		assertRoutes("update domains", []netip.Prefix{}, []netip.Prefix{})
+
+		a.ObserveDNSResponse(dnsResponse("a.example.com.", "1.2.3.1"))
+		a.ObserveDNSResponse(dnsResponse("a.example.com.", "1.2.3.2"))
+		a.ObserveDNSResponse(dnsResponse("b.example.com.", "1.2.3.3"))
+		a.ObserveDNSResponse(dnsResponse("b.example.com.", "1.2.3.4"))
+		a.Wait(ctx)
+		// observing dns responses causes routes to be advertised
+		assertRoutes("observed dns", prefixes("1.2.3.1/32", "1.2.3.2/32", "1.2.3.3/32", "1.2.3.4/32"), []netip.Prefix{})
+
+		a.UpdateDomainsAndRoutes([]string{"a.example.com"}, []netip.Prefix{})
+		a.Wait(ctx)
+		// old behavior, routes are not removed
+		wantRoutes := prefixes("1.2.3.1/32", "1.2.3.2/32", "1.2.3.3/32", "1.2.3.4/32")
+		wantRemovedRoutes := []netip.Prefix{}
+		if shouldStore {
+			// new behavior, routes are removed for b.example.com
+			wantRoutes = prefixes("1.2.3.1/32", "1.2.3.2/32")
+			wantRemovedRoutes = prefixes("1.2.3.3/32", "1.2.3.4/32")
+		}
+		assertRoutes("removal", wantRoutes, wantRemovedRoutes)
+	}
+}
+
+func TestUpdateWildcardRouteRemoval(t *testing.T) {
+	for _, shouldStore := range []bool{false, true} {
+		ctx := context.Background()
+		rc := &appctest.RouteCollector{}
+
+		assertRoutes := func(prefix string, routes, removedRoutes []netip.Prefix) {
+			if !slices.Equal(routes, rc.Routes()) {
+				t.Fatalf("%s: (shouldStore=%t) routes want %v, got %v", prefix, shouldStore, routes, rc.Routes())
+			}
+			if !slices.Equal(removedRoutes, rc.RemovedRoutes()) {
+				t.Fatalf("%s: (shouldStore=%t) removedRoutes want %v, got %v", prefix, shouldStore, removedRoutes, rc.RemovedRoutes())
+			}
+		}
+
+		var a *AppConnector
+		if shouldStore {
+			a = NewAppConnector(t.Logf, rc, &RouteInfo{}, fakeStoreRoutes)
+		} else {
+			a = NewAppConnector(t.Logf, rc, nil, nil)
+		}
+		assertRoutes("appc init", []netip.Prefix{}, []netip.Prefix{})
+
+		a.UpdateDomainsAndRoutes([]string{"a.example.com", "*.b.example.com"}, []netip.Prefix{})
+		a.Wait(ctx)
+		// adding domains doesn't immediately cause any routes to be advertised
+		assertRoutes("update domains", []netip.Prefix{}, []netip.Prefix{})
+
+		a.ObserveDNSResponse(dnsResponse("a.example.com.", "1.2.3.1"))
+		a.ObserveDNSResponse(dnsResponse("a.example.com.", "1.2.3.2"))
+		a.ObserveDNSResponse(dnsResponse("1.b.example.com.", "1.2.3.3"))
+		a.ObserveDNSResponse(dnsResponse("2.b.example.com.", "1.2.3.4"))
+		a.Wait(ctx)
+		// observing dns responses causes routes to be advertised
+		assertRoutes("observed dns", prefixes("1.2.3.1/32", "1.2.3.2/32", "1.2.3.3/32", "1.2.3.4/32"), []netip.Prefix{})
+
+		a.UpdateDomainsAndRoutes([]string{"a.example.com"}, []netip.Prefix{})
+		a.Wait(ctx)
+		// old behavior, routes are not removed
+		wantRoutes := prefixes("1.2.3.1/32", "1.2.3.2/32", "1.2.3.3/32", "1.2.3.4/32")
+		wantRemovedRoutes := []netip.Prefix{}
+		if shouldStore {
+			// new behavior, routes are removed for *.b.example.com
+			wantRoutes = prefixes("1.2.3.1/32", "1.2.3.2/32")
+			wantRemovedRoutes = prefixes("1.2.3.3/32", "1.2.3.4/32")
+		}
+		assertRoutes("removal", wantRoutes, wantRemovedRoutes)
+	}
+}
+
+func TestRoutesWithout(t *testing.T) {
+	assert := func(msg string, got, want []netip.Prefix) {
+		if !slices.Equal(want, got) {
+			t.Errorf("%s: want %v, got %v", msg, want, got)
+		}
+	}
+
+	assert("empty routes", routesWithout([]netip.Prefix{}, []netip.Prefix{}), []netip.Prefix{})
+	assert("a empty", routesWithout([]netip.Prefix{}, prefixes("1.1.1.1/32", "1.1.1.2/32")), []netip.Prefix{})
+	assert("b empty", routesWithout(prefixes("1.1.1.1/32", "1.1.1.2/32"), []netip.Prefix{}), prefixes("1.1.1.1/32", "1.1.1.2/32"))
+	assert("no overlap", routesWithout(prefixes("1.1.1.1/32", "1.1.1.2/32"), prefixes("1.1.1.3/32", "1.1.1.4/32")), prefixes("1.1.1.1/32", "1.1.1.2/32"))
+	assert("a has fewer", routesWithout(prefixes("1.1.1.1/32", "1.1.1.2/32"), prefixes("1.1.1.1/32", "1.1.1.2/32", "1.1.1.3/32", "1.1.1.4/32")), []netip.Prefix{})
+	assert("a has more", routesWithout(prefixes("1.1.1.1/32", "1.1.1.2/32", "1.1.1.3/32", "1.1.1.4/32"), prefixes("1.1.1.1/32", "1.1.1.3/32")), prefixes("1.1.1.2/32", "1.1.1.4/32"))
+}