From 8250582fe6758b05018efb1c6ad74062b741271a Mon Sep 17 00:00:00 2001 From: James Tucker Date: Thu, 18 Jan 2024 10:18:25 -0800 Subject: [PATCH] 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 --- appc/appconnector.go | 39 ++++++++++++++++++++++++++++++------ appc/appconnector_test.go | 22 ++++++++++++-------- cmd/tailscaled/depaware.txt | 2 +- ipn/ipnlocal/local.go | 3 +-- ipn/ipnlocal/local_test.go | 3 +++ ipn/ipnlocal/peerapi_test.go | 3 +++ 6 files changed, 55 insertions(+), 17 deletions(-) diff --git a/appc/appconnector.go b/appc/appconnector.go index 11ca41c3d..8935b7909 100644 --- a/appc/appconnector.go +++ b/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 -// 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. +// UpdateDomainsAndRoutes starts an asynchronous update of the configuration +// given the new domains and routes. +func (e *AppConnector) UpdateDomainsAndRoutes(domains []string, routes []netip.Prefix) { + 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() diff --git a/appc/appconnector_test.go b/appc/appconnector_test.go index cb42dee6f..2e999a589 100644 --- a/appc/appconnector_test.go +++ b/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) } diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index e363278d5..454c3dc66 100644 --- a/cmd/tailscaled/depaware.txt +++ b/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 diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index f5ea9e5c3..85c0eb73f 100644 --- a/ipn/ipnlocal/local.go +++ b/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.UpdateDomains(domains) + b.appConnector.UpdateDomainsAndRoutes(domains, routes) } // authReconfig pushes a new configuration into wgengine, if engine diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index f1e8aa3b2..ef4c2f450 100644 --- a/ipn/ipnlocal/local_test.go +++ b/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) { diff --git a/ipn/ipnlocal/peerapi_test.go b/ipn/ipnlocal/peerapi_test.go index 074d11482..ab182bb28 100644 --- a/ipn/ipnlocal/peerapi_test.go +++ b/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) {