ipn/ipnlocal: make app connector configuration concurrent

If there are routes changes as a side effect of an app connector
configuration update, the connector configuration may want to reenter a
lock, so must be started asynchronously.

Updates tailscale/corp#16833
Signed-off-by: James Tucker <james@tailscale.com>

appc/appconnector.go

@@ -10,6 +10,7 @@
 package appc
 
 import (
+	"context"
 	"net/netip"
 	"slices"
 	"strings"
@@ -20,6 +21,7 @@ import (
 	"tailscale.com/types/logger"
 	"tailscale.com/types/views"
 	"tailscale.com/util/dnsname"
+	"tailscale.com/util/execqueue"
 )
 
 // RouteAdvertiser is an interface that allows the AppConnector to advertise
@@ -58,6 +60,9 @@ type AppConnector struct {
 
 	// wildcards is the list of domain strings that match subdomains.
 	wildcards []string
+
+	// queue provides ordering for update operations
+	queue execqueue.ExecQueue
 }
 
 // NewAppConnector creates a new AppConnector.
@@ -68,11 +73,33 @@ func NewAppConnector(logf logger.Logf, routeAdvertiser RouteAdvertiser) *AppConn
 	}
 }
 
-// UpdateDomains replaces the current set of configured domains with the
+// UpdateDomainsAndRoutes starts an asynchronous update of the configuration
-// supplied set of domains. Domains must not contain a trailing dot, and should
+// given the new domains and routes.
-// be lower case. If the domain contains a leading '*' label it matches all
+func (e *AppConnector) UpdateDomainsAndRoutes(domains []string, routes []netip.Prefix) {
-// subdomains of a domain.
+	e.queue.Add(func() {
+		// Add the new routes first.
+		e.updateRoutes(routes)
+		e.updateDomains(domains)
+	})
+}
+
+// UpdateDomains asynchronously replaces the current set of configured domains
+// with the supplied set of domains. Domains must not contain a trailing dot,
+// and should be lower case. If the domain contains a leading '*' label it
+// matches all subdomains of a domain.
 func (e *AppConnector) UpdateDomains(domains []string) {
+	e.queue.Add(func() {
+		e.updateDomains(domains)
+	})
+}
+
+// Wait waits for the currently scheduled asynchronous configuration changes to
+// complete.
+func (e *AppConnector) Wait(ctx context.Context) {
+	e.queue.Wait(ctx)
+}
+
+func (e *AppConnector) updateDomains(domains []string) {
 	e.mu.Lock()
 	defer e.mu.Unlock()
 
@@ -104,11 +131,11 @@ func (e *AppConnector) UpdateDomains(domains []string) {
 	e.logf("handling domains: %v and wildcards: %v", xmaps.Keys(e.domains), e.wildcards)
 }
 
-// UpdateRoutes merges the supplied routes into the currently configured routes. The routes supplied
+// updateRoutes merges the supplied routes into the currently configured routes. The routes supplied
 // by control for UpdateRoutes are supplemental to the routes discovered by DNS resolution, but are
 // also more often whole ranges. UpdateRoutes will remove any single address routes that are now
 // covered by new ranges.
-func (e *AppConnector) UpdateRoutes(routes []netip.Prefix) {
+func (e *AppConnector) updateRoutes(routes []netip.Prefix) {
 	e.mu.Lock()
 	defer e.mu.Unlock()
 

appc/appconnector_test.go

@@ -4,6 +4,7 @@
 package appc
 
 import (
+	"context"
 	"net/netip"
 	"reflect"
 	"slices"
@@ -16,8 +17,11 @@ import (
 )
 
 func TestUpdateDomains(t *testing.T) {
+	ctx := context.Background()
 	a := NewAppConnector(t.Logf, nil)
 	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)
 	}
@@ -25,6 +29,7 @@ func TestUpdateDomains(t *testing.T) {
 	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)
@@ -32,6 +37,7 @@ func TestUpdateDomains(t *testing.T) {
 
 	// 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)
 	}
@@ -41,7 +47,7 @@ func TestUpdateRoutes(t *testing.T) {
 	rc := &routeCollector{}
 	a := NewAppConnector(t.Logf, rc)
 	routes := []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24")}
-	a.UpdateRoutes(routes)
+	a.updateRoutes(routes)
 
 	if !slices.EqualFunc(routes, rc.routes, prefixEqual) {
 		t.Fatalf("got %v, want %v", rc.routes, routes)
@@ -54,7 +60,7 @@ func TestUpdateRoutesUnadvertisesContainedRoutes(t *testing.T) {
 	mak.Set(&a.domains, "example.com", []netip.Addr{netip.MustParseAddr("192.0.2.1")})
 	rc.routes = []netip.Prefix{netip.MustParsePrefix("192.0.2.1/32")}
 	routes := []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24")}
-	a.UpdateRoutes(routes)
+	a.updateRoutes(routes)
 
 	if !slices.EqualFunc(routes, rc.routes, prefixEqual) {
 		t.Fatalf("got %v, want %v", rc.routes, routes)
@@ -64,7 +70,7 @@ func TestUpdateRoutesUnadvertisesContainedRoutes(t *testing.T) {
 func TestDomainRoutes(t *testing.T) {
 	rc := &routeCollector{}
 	a := NewAppConnector(t.Logf, rc)
-	a.UpdateDomains([]string{"example.com"})
+	a.updateDomains([]string{"example.com"})
 	a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8"))
 
 	want := map[string][]netip.Addr{
@@ -88,7 +94,7 @@ func TestObserveDNSResponse(t *testing.T) {
 
 	wantRoutes := []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")}
 
-	a.UpdateDomains([]string{"example.com"})
+	a.updateDomains([]string{"example.com"})
 	a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8"))
 	if got, want := rc.routes, wantRoutes; !slices.Equal(got, want) {
 		t.Errorf("got %v; want %v", got, want)
@@ -109,7 +115,7 @@ func TestObserveDNSResponse(t *testing.T) {
 
 	// don't advertise addresses that are already in a control provided route
 	pfx := netip.MustParsePrefix("192.0.2.0/24")
-	a.UpdateRoutes([]netip.Prefix{pfx})
+	a.updateRoutes([]netip.Prefix{pfx})
 	wantRoutes = append(wantRoutes, pfx)
 	a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.2.1"))
 	if !slices.Equal(rc.routes, wantRoutes) {
@@ -124,7 +130,7 @@ func TestWildcardDomains(t *testing.T) {
 	rc := &routeCollector{}
 	a := NewAppConnector(t.Logf, rc)
 
-	a.UpdateDomains([]string{"*.example.com"})
+	a.updateDomains([]string{"*.example.com"})
 	a.ObserveDNSResponse(dnsResponse("foo.example.com.", "192.0.0.8"))
 	if got, want := rc.routes, []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")}; !slices.Equal(got, want) {
 		t.Errorf("routes: got %v; want %v", got, want)
@@ -133,7 +139,7 @@ func TestWildcardDomains(t *testing.T) {
 		t.Errorf("wildcards: got %v; want %v", got, want)
 	}
 
-	a.UpdateDomains([]string{"*.example.com", "example.com"})
+	a.updateDomains([]string{"*.example.com", "example.com"})
 	if _, ok := a.domains["foo.example.com"]; !ok {
 		t.Errorf("expected foo.example.com to be preserved in domains due to wildcard")
 	}
@@ -142,7 +148,7 @@ func TestWildcardDomains(t *testing.T) {
 	}
 
 	// There was an early regression where the wildcard domain was added repeatedly, this guards against that.
-	a.UpdateDomains([]string{"*.example.com", "example.com"})
+	a.updateDomains([]string{"*.example.com", "example.com"})
 	if len(a.wildcards) != 1 {
 		t.Errorf("expected only one wildcard domain, got %v", a.wildcards)
 	}

cmd/tailscaled/depaware.txt

@@ -348,7 +348,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
      💣 tailscale.com/util/deephash                                  from tailscale.com/ipn/ipnlocal+
    L 💣 tailscale.com/util/dirwalk                                   from tailscale.com/metrics+
         tailscale.com/util/dnsname                                   from tailscale.com/hostinfo+
-        tailscale.com/util/execqueue                                 from tailscale.com/control/controlclient
+        tailscale.com/util/execqueue                                 from tailscale.com/control/controlclient+
         tailscale.com/util/goroutines                                from tailscale.com/ipn/ipnlocal
         tailscale.com/util/groupmember                               from tailscale.com/ipn/ipnauth+
      💣 tailscale.com/util/hashx                                     from tailscale.com/util/deephash

ipn/ipnlocal/local.go

@@ -3460,8 +3460,7 @@ func (b *LocalBackend) reconfigAppConnectorLocked(nm *netmap.NetworkMap, prefs i
 	slices.SortFunc(routes, func(i, j netip.Prefix) int { return i.Addr().Compare(j.Addr()) })
 	domains = slices.Compact(domains)
 	routes = slices.Compact(routes)
-	b.appConnector.UpdateRoutes(routes)
+	b.appConnector.UpdateDomainsAndRoutes(domains, routes)
-	b.appConnector.UpdateDomains(domains)
 }
 
 // authReconfig pushes a new configuration into wgengine, if engine

ipn/ipnlocal/local_test.go

@@ -1207,8 +1207,10 @@ func TestObserveDNSResponse(t *testing.T) {
 	rc := &routeCollector{}
 	b.appConnector = appc.NewAppConnector(t.Logf, rc)
 	b.appConnector.UpdateDomains([]string{"example.com"})
+	b.appConnector.Wait(context.Background())
 
 	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)
@@ -1250,6 +1252,7 @@ func TestReconfigureAppConnector(t *testing.T) {
 	}).View()
 
 	b.reconfigAppConnectorLocked(b.netMap, b.pm.prefs)
+	b.appConnector.Wait(context.Background())
 
 	want := []string{"example.com"}
 	if !slices.Equal(b.appConnector.Domains().AsSlice(), want) {

ipn/ipnlocal/peerapi_test.go

@@ -685,6 +685,7 @@ func TestPeerAPIReplyToDNSQueries(t *testing.T) {
 }
 
 func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) {
+	ctx := context.Background()
 	var h peerAPIHandler
 	h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345")
 
@@ -700,6 +701,7 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) {
 		},
 	}
 	h.ps.b.appConnector.UpdateDomains([]string{"example.com"})
+	h.ps.b.appConnector.Wait(ctx)
 
 	h.ps.resolver = &fakeResolver{}
 	f := filter.NewAllowAllForTest(logger.Discard)
@@ -717,6 +719,7 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) {
 	if w.Code != http.StatusOK {
 		t.Errorf("unexpected status code: %v", w.Code)
 	}
+	h.ps.b.appConnector.Wait(ctx)
 
 	wantRoutes := []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")}
 	if !slices.Equal(rc.routes, wantRoutes) {