From 51adaec35a3e4d25df88d81e6264584e151bd33d Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@tailscale.com>
Date: Tue, 21 Jan 2025 08:02:24 -0800
Subject: [PATCH] Revert "ipn/ipnlocal: re-advertise appc routes on startup
 (#14609)"

This reverts commit 1b303ee5baef3ddab40be4d1c2 (#14609).

It caused a deadlock; see tailscale/corp#25965

Updates tailscale/corp#25965
Updates #13680
Updates #14606
---
 ipn/ipnlocal/local.go      | 35 +++-------------------------
 ipn/ipnlocal/local_test.go | 47 --------------------------------------
 2 files changed, 3 insertions(+), 79 deletions(-)

diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go
index 214d3a4e4..bb84012fd 100644
--- a/ipn/ipnlocal/local.go
+++ b/ipn/ipnlocal/local.go
@@ -4356,33 +4356,6 @@ func (b *LocalBackend) reconfigAppConnectorLocked(nm *netmap.NetworkMap, prefs i
 	b.appConnector.UpdateDomainsAndRoutes(domains, routes)
 }
 
-func (b *LocalBackend) readvertiseAppConnectorRoutes() {
-	var domainRoutes map[string][]netip.Addr
-	b.mu.Lock()
-	if b.appConnector != nil {
-		domainRoutes = b.appConnector.DomainRoutes()
-	}
-	b.mu.Unlock()
-	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.
@@ -4461,7 +4434,6 @@ func (b *LocalBackend) authReconfig() {
 	}
 
 	b.initPeerAPIListener()
-	b.readvertiseAppConnectorRoutes()
 }
 
 // shouldUseOneCGNATRoute reports whether we should prefer to make one big
@@ -7204,7 +7176,7 @@ func (b *LocalBackend) ObserveDNSResponse(res []byte) {
 // 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
+	newRoutes := false
 
 	for _, ipp := range ipps {
 		if !allowedAutoRoute(ipp) {
@@ -7220,14 +7192,13 @@ func (b *LocalBackend) AdvertiseRoute(ipps ...netip.Prefix) error {
 		}
 
 		finalRoutes = append(finalRoutes, ipp)
-		newRoutes = append(newRoutes, ipp)
+		newRoutes = true
 	}
 
-	if len(newRoutes) == 0 {
+	if !newRoutes {
 		return nil
 	}
 
-	b.logf("advertising new app connector routes: %v", newRoutes)
 	_, err := b.EditPrefs(&ipn.MaskedPrefs{
 		Prefs: ipn.Prefs{
 			AdvertiseRoutes: finalRoutes,
diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go
index 348bdcab3..415791c60 100644
--- a/ipn/ipnlocal/local_test.go
+++ b/ipn/ipnlocal/local_test.go
@@ -1501,53 +1501,6 @@ 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