ipn/ipnlocal: re-advertise appc routes on startup, take 2 (#14740)

* Reapply "ipn/ipnlocal: re-advertise appc routes on startup (#14609)"

This reverts commit 51adaec35a3e4d25df88d81e6264584e151bd33d.

Signed-off-by: Andrew Lytvynov <awly@tailscale.com>

* ipn/ipnlocal: fix a deadlock in readvertiseAppConnectorRoutes

Don't hold LocalBackend.mu while calling the methods of
appc.AppConnector. Those methods could call back into LocalBackend and
try to acquire it's mutex.

Fixes https://github.com/tailscale/corp/issues/25965
Fixes #14606

Signed-off-by: Andrew Lytvynov <awly@tailscale.com>

---------

Signed-off-by: Andrew Lytvynov <awly@tailscale.com>

ipn/ipnlocal/local.go

@@ -4418,6 +4418,41 @@ func (b *LocalBackend) reconfigAppConnectorLocked(nm *netmap.NetworkMap, prefs i
 	b.appConnector.UpdateDomainsAndRoutes(domains, routes)
 }
 
+func (b *LocalBackend) readvertiseAppConnectorRoutes() {
+	// Note: we should never call b.appConnector methods while holding b.mu.
+	// This can lead to a deadlock, like
+	// https://github.com/tailscale/corp/issues/25965.
+	//
+	// Grab a copy of the field, since b.mu only guards access to the
+	// b.appConnector field itself.
+	b.mu.Lock()
+	appConnector := b.appConnector
+	b.mu.Unlock()
+
+	if appConnector == nil {
+		return
+	}
+	domainRoutes := appConnector.DomainRoutes()
+	if domainRoutes == nil {
+		return
+	}
+
+	// Re-advertise the stored routes, in case stored state got out of
+	// sync with previously advertised routes in prefs.
+	var prefixes []netip.Prefix
+	for _, ips := range domainRoutes {
+		for _, ip := range ips {
+			prefixes = append(prefixes, netip.PrefixFrom(ip, ip.BitLen()))
+		}
+	}
+	// Note: AdvertiseRoute will trim routes that are already
+	// advertised, so if everything is already being advertised this is
+	// a noop.
+	if err := b.AdvertiseRoute(prefixes...); err != nil {
+		b.logf("error advertising stored app connector routes: %v", err)
+	}
+}
+
 // authReconfig pushes a new configuration into wgengine, if engine
 // updates are not currently blocked, based on the cached netmap and
 // user prefs.
@@ -4496,6 +4531,7 @@ func (b *LocalBackend) authReconfig() {
 	}
 
 	b.initPeerAPIListener()
+	b.readvertiseAppConnectorRoutes()
 }
 
 // shouldUseOneCGNATRoute reports whether we should prefer to make one big
@@ -7261,7 +7297,7 @@ var ErrDisallowedAutoRoute = errors.New("route is not allowed")
 // If the route is disallowed, ErrDisallowedAutoRoute is returned.
 func (b *LocalBackend) AdvertiseRoute(ipps ...netip.Prefix) error {
 	finalRoutes := b.Prefs().AdvertiseRoutes().AsSlice()
-	newRoutes := false
+	var newRoutes []netip.Prefix
 
 	for _, ipp := range ipps {
 		if !allowedAutoRoute(ipp) {
@@ -7277,13 +7313,14 @@ func (b *LocalBackend) AdvertiseRoute(ipps ...netip.Prefix) error {
 		}
 
 		finalRoutes = append(finalRoutes, ipp)
-		newRoutes = true
+		newRoutes = append(newRoutes, ipp)
 	}
 
-	if !newRoutes {
+	if len(newRoutes) == 0 {
 		return nil
 	}
 
+	b.logf("advertising new app connector routes: %v", newRoutes)
 	_, err := b.EditPrefs(&ipn.MaskedPrefs{
 		Prefs: ipn.Prefs{
 			AdvertiseRoutes: finalRoutes,

ipn/ipnlocal/local_test.go

@@ -1501,6 +1501,53 @@ func TestReconfigureAppConnector(t *testing.T) {
 	}
 }
 
+func TestBackfillAppConnectorRoutes(t *testing.T) {
+	// Create backend with an empty app connector.
+	b := newTestBackend(t)
+	if err := b.Start(ipn.Options{}); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := b.EditPrefs(&ipn.MaskedPrefs{
+		Prefs: ipn.Prefs{
+			AppConnector: ipn.AppConnectorPrefs{Advertise: true},
+		},
+		AppConnectorSet: true,
+	}); err != nil {
+		t.Fatal(err)
+	}
+	b.reconfigAppConnectorLocked(b.netMap, b.pm.prefs)
+
+	// Smoke check that AdvertiseRoutes doesn't have the test IP.
+	ip := netip.MustParseAddr("1.2.3.4")
+	routes := b.Prefs().AdvertiseRoutes().AsSlice()
+	if slices.Contains(routes, netip.PrefixFrom(ip, ip.BitLen())) {
+		t.Fatalf("AdvertiseRoutes %v on a fresh backend already contains advertised route for %v", routes, ip)
+	}
+
+	// Store the test IP in profile data, but not in Prefs.AdvertiseRoutes.
+	b.ControlKnobs().AppCStoreRoutes.Store(true)
+	if err := b.storeRouteInfo(&appc.RouteInfo{
+		Domains: map[string][]netip.Addr{
+			"example.com": {ip},
+		},
+	}); err != nil {
+		t.Fatal(err)
+	}
+
+	// Mimic b.authReconfigure for the app connector bits.
+	b.mu.Lock()
+	b.reconfigAppConnectorLocked(b.netMap, b.pm.prefs)
+	b.mu.Unlock()
+	b.readvertiseAppConnectorRoutes()
+
+	// Check that Prefs.AdvertiseRoutes got backfilled with routes stored in
+	// profile data.
+	routes = b.Prefs().AdvertiseRoutes().AsSlice()
+	if !slices.Contains(routes, netip.PrefixFrom(ip, ip.BitLen())) {
+		t.Fatalf("AdvertiseRoutes %v was not backfilled from stored app connector routes with %v", routes, ip)
+	}
+}
+
 func resolversEqual(t *testing.T, a, b []*dnstype.Resolver) bool {
 	if a == nil && b == nil {
 		return true