appc,ipn/ipnlocal: optimize preference adjustments when routes update

This change allows us to perform batch modification for new route
advertisements and route removals. Additionally, we now handle the case
where newly added routes are covered by existing ranges.

This change also introduces a new appctest package that contains some
shared functions used for testing.

Updates tailscale/corp#16833

Signed-off-by: Charlotte Brandhorst-Satzkorn <charlotte@tailscale.com>

ipn/ipnlocal/local.go

@@ -5790,45 +5790,73 @@ 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.
-func (b *LocalBackend) AdvertiseRoute(ipp netip.Prefix) error {
-	if !allowedAutoRoute(ipp) {
-		return ErrDisallowedAutoRoute
+func (b *LocalBackend) AdvertiseRoute(ipps ...netip.Prefix) error {
+	finalRoutes := b.Prefs().AdvertiseRoutes().AsSlice()
+	newRoutes := false
+
+	for _, ipp := range ipps {
+		if !allowedAutoRoute(ipp) {
+			continue
+		}
+		if slices.Contains(finalRoutes, ipp) {
+			continue
+		}
+
+		// If the new prefix is already contained by existing routes, skip it.
+		if coveredRouteRange(finalRoutes, ipp) {
+			continue
+		}
+
+		finalRoutes = append(finalRoutes, ipp)
+		newRoutes = true
 	}
-	currentRoutes := b.Prefs().AdvertiseRoutes()
-	if currentRoutes.ContainsFunc(func(r netip.Prefix) bool {
-		// TODO(raggi): add support for subset checks and avoid subset route creations.
-		return ipp.IsSingleIP() && r.Contains(ipp.Addr()) || r == ipp
-	}) {
+
+	if !newRoutes {
 		return nil
 	}
-	routes := append(currentRoutes.AsSlice(), ipp)
+
 	_, err := b.EditPrefs(&ipn.MaskedPrefs{
 		Prefs: ipn.Prefs{
-			AdvertiseRoutes: routes,
+			AdvertiseRoutes: finalRoutes,
 		},
 		AdvertiseRoutesSet: true,
 	})
 	return err
 }
 
+// coveredRouteRange checks if a route is already included in a slice of
+// prefixes.
+func coveredRouteRange(finalRoutes []netip.Prefix, ipp netip.Prefix) bool {
+	for _, r := range finalRoutes {
+		if ipp.IsSingleIP() {
+			if r.Contains(ipp.Addr()) {
+				return true
+			}
+		} else {
+			if r.Contains(ipp.Addr()) && r.Contains(netipx.PrefixLastIP(ipp)) {
+				return true
+			}
+		}
+	}
+	return false
+}
+
 // UnadvertiseRoute implements the appc.RouteAdvertiser interface. It removes
 // a route advertisement if one is present in the existing routes.
-func (b *LocalBackend) UnadvertiseRoute(ipp netip.Prefix) error {
+func (b *LocalBackend) UnadvertiseRoute(toRemove ...netip.Prefix) error {
 	currentRoutes := b.Prefs().AdvertiseRoutes().AsSlice()
-	if !slices.Contains(currentRoutes, ipp) {
-		return nil
-	}
+	finalRoutes := currentRoutes[:0]
 
-	newRoutes := currentRoutes[:0]
-	for _, r := range currentRoutes {
-		if r != ipp {
-			newRoutes = append(newRoutes, r)
+	for _, ipp := range currentRoutes {
+		if slices.Contains(toRemove, ipp) {
+			continue
 		}
+		finalRoutes = append(finalRoutes, ipp)
 	}
 
 	_, err := b.EditPrefs(&ipn.MaskedPrefs{
 		Prefs: ipn.Prefs{
-			AdvertiseRoutes: newRoutes,
+			AdvertiseRoutes: finalRoutes,
 		},
 		AdvertiseRoutesSet: true,
 	})

ipn/ipnlocal/local_test.go

@@ -18,6 +18,7 @@ import (
 	"go4.org/netipx"
 	"golang.org/x/net/dns/dnsmessage"
 	"tailscale.com/appc"
+	"tailscale.com/appc/appctest"
 	"tailscale.com/control/controlclient"
 	"tailscale.com/ipn"
 	"tailscale.com/ipn/store/mem"
@@ -1204,7 +1205,7 @@ func TestObserveDNSResponse(t *testing.T) {
 	// ensure no error when no app connector is configured
 	b.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8"))
 
-	rc := &routeCollector{}
+	rc := &appctest.RouteCollector{}
 	b.appConnector = appc.NewAppConnector(t.Logf, rc)
 	b.appConnector.UpdateDomains([]string{"example.com"})
 	b.appConnector.Wait(context.Background())
@@ -1212,8 +1213,44 @@ func TestObserveDNSResponse(t *testing.T) {
 	b.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8"))
 	b.appConnector.Wait(context.Background())
 	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 !slices.Equal(rc.Routes(), wantRoutes) {
+		t.Fatalf("got routes %v, want %v", rc.Routes(), wantRoutes)
+	}
+}
+
+func TestCoveredRouteRange(t *testing.T) {
+	tests := []struct {
+		existingRoute netip.Prefix
+		newRoute      netip.Prefix
+		want          bool
+	}{
+		{
+			existingRoute: netip.MustParsePrefix("192.0.0.1/32"),
+			newRoute:      netip.MustParsePrefix("192.0.0.1/32"),
+			want:          true,
+		},
+		{
+			existingRoute: netip.MustParsePrefix("192.0.0.1/32"),
+			newRoute:      netip.MustParsePrefix("192.0.0.2/32"),
+			want:          false,
+		},
+		{
+			existingRoute: netip.MustParsePrefix("192.0.0.0/24"),
+			newRoute:      netip.MustParsePrefix("192.0.0.1/32"),
+			want:          true,
+		},
+		{
+			existingRoute: netip.MustParsePrefix("192.0.0.0/16"),
+			newRoute:      netip.MustParsePrefix("192.0.0.0/24"),
+			want:          true,
+		},
+	}
+
+	for _, tt := range tests {
+		got := coveredRouteRange([]netip.Prefix{tt.existingRoute}, tt.newRoute)
+		if got != tt.want {
+			t.Errorf("coveredRouteRange(%v, %v) = %v, want %v", tt.existingRoute, tt.newRoute, got, tt.want)
+		}
 	}
 }
 
@@ -1352,27 +1389,6 @@ func dnsResponse(domain, address string) []byte {
 	return must.Get(b.Finish())
 }
 
-// routeCollector is a test helper that collects the list of routes advertised
-type routeCollector struct {
-	routes []netip.Prefix
-}
-
-func (rc *routeCollector) AdvertiseRoute(pfx netip.Prefix) error {
-	rc.routes = append(rc.routes, pfx)
-	return nil
-}
-
-func (rc *routeCollector) UnadvertiseRoute(pfx netip.Prefix) error {
-	routes := rc.routes
-	rc.routes = rc.routes[:0]
-	for _, r := range routes {
-		if r != pfx {
-			rc.routes = append(rc.routes, r)
-		}
-	}
-	return nil
-}
-
 type errorSyspolicyHandler struct {
 	t         *testing.T
 	err       error

ipn/ipnlocal/peerapi_test.go

@@ -23,6 +23,7 @@ import (
 	"go4.org/netipx"
 	"golang.org/x/net/dns/dnsmessage"
 	"tailscale.com/appc"
+	"tailscale.com/appc/appctest"
 	"tailscale.com/client/tailscale/apitype"
 	"tailscale.com/ipn"
 	"tailscale.com/ipn/store/mem"
@@ -689,7 +690,7 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) {
 	var h peerAPIHandler
 	h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345")
 
-	rc := &routeCollector{}
+	rc := &appctest.RouteCollector{}
 	eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0)
 	pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
 	h.ps = &peerAPIServer{
@@ -722,8 +723,8 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) {
 	h.ps.b.appConnector.Wait(ctx)
 
 	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 !slices.Equal(rc.Routes(), wantRoutes) {
+		t.Errorf("got %v; want %v", rc.Routes(), wantRoutes)
 	}
 }