diff --git a/VERSION.txt b/VERSION.txt index 79f82f6b8e0ce..7af7cf49339fb 100644 --- a/VERSION.txt +++ b/VERSION.txt @@ -1 +1 @@ -1.58.0 +1.58.2 diff --git a/appc/appconnector.go b/appc/appconnector.go index 11ca41c3d896b..a08fbb0b85148 100644 --- a/appc/appconnector.go +++ b/appc/appconnector.go @@ -10,6 +10,7 @@ package appc import ( + "context" "net/netip" "slices" "strings" @@ -20,17 +21,18 @@ 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 // newly discovered routes that need to be served through the AppConnector. type RouteAdvertiser interface { - // AdvertiseRoute adds a new route advertisement if the route is not already - // being advertised. - AdvertiseRoute(netip.Prefix) error + // AdvertiseRoute adds one or more route advertisements skipping any that + // are already advertised. + AdvertiseRoute(...netip.Prefix) error - // UnadvertiseRoute removes a route advertisement. - UnadvertiseRoute(netip.Prefix) error + // UnadvertiseRoute removes any matching route advertisements. + UnadvertiseRoute(...netip.Prefix) error } // AppConnector is an implementation of an AppConnector that performs @@ -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() @@ -117,26 +144,30 @@ func (e *AppConnector) UpdateRoutes(routes []netip.Prefix) { return } + if err := e.routeAdvertiser.AdvertiseRoute(routes...); err != nil { + e.logf("failed to advertise routes: %v: %v", routes, err) + return + } + + var toRemove []netip.Prefix + nextRoute: for _, r := range routes { - if err := e.routeAdvertiser.AdvertiseRoute(r); err != nil { - e.logf("failed to advertise route: %v: %v", r, err) - continue - } - for _, addr := range e.domains { for _, a := range addr { - if r.Contains(a) { + if r.Contains(a) && netip.PrefixFrom(a, a.BitLen()) != r { pfx := netip.PrefixFrom(a, a.BitLen()) - if err := e.routeAdvertiser.UnadvertiseRoute(pfx); err != nil { - e.logf("failed to unadvertise route: %v: %v", pfx, err) - } + toRemove = append(toRemove, pfx) continue nextRoute } } } } + if err := e.routeAdvertiser.UnadvertiseRoute(toRemove...); err != nil { + e.logf("failed to unadvertise routes: %v: %v", toRemove, err) + } + e.controlRoutes = routes } diff --git a/appc/appconnector_test.go b/appc/appconnector_test.go index cb42dee6f11c0..510ee83617043 100644 --- a/appc/appconnector_test.go +++ b/appc/appconnector_test.go @@ -4,6 +4,7 @@ package appc import ( + "context" "net/netip" "reflect" "slices" @@ -11,13 +12,17 @@ import ( xmaps "golang.org/x/exp/maps" "golang.org/x/net/dns/dnsmessage" + "tailscale.com/appc/appctest" "tailscale.com/util/mak" "tailscale.com/util/must" ) 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 +30,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,39 +38,66 @@ 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) } } func TestUpdateRoutes(t *testing.T) { - rc := &routeCollector{} + ctx := context.Background() + rc := &appctest.RouteCollector{} a := NewAppConnector(t.Logf, rc) - routes := []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24")} - a.UpdateRoutes(routes) + a.updateDomains([]string{"*.example.com"}) + + // This route should be collapsed into the range + a.ObserveDNSResponse(dnsResponse("a.example.com.", "192.0.2.1")) + a.Wait(ctx) + + if !slices.Equal(rc.Routes(), []netip.Prefix{netip.MustParsePrefix("192.0.2.1/32")}) { + t.Fatalf("got %v, want %v", rc.Routes(), []netip.Prefix{netip.MustParsePrefix("192.0.2.1/32")}) + } + + // This route should not be collapsed or removed + a.ObserveDNSResponse(dnsResponse("b.example.com.", "192.0.0.1")) + a.Wait(ctx) + + routes := []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24"), netip.MustParsePrefix("192.0.0.1/32")} + a.updateRoutes(routes) + + slices.SortFunc(rc.Routes(), prefixCompare) + rc.SetRoutes(slices.Compact(rc.Routes())) + slices.SortFunc(routes, prefixCompare) + + // Ensure that the non-matching /32 is preserved, even though it's in the domains table. + if !slices.EqualFunc(routes, rc.Routes(), prefixEqual) { + t.Errorf("added routes: got %v, want %v", rc.Routes(), routes) + } - if !slices.EqualFunc(routes, rc.routes, prefixEqual) { - t.Fatalf("got %v, want %v", rc.routes, routes) + // Ensure that the contained /32 is removed, replaced by the /24. + wantRemoved := []netip.Prefix{netip.MustParsePrefix("192.0.2.1/32")} + if !slices.EqualFunc(rc.RemovedRoutes(), wantRemoved, prefixEqual) { + t.Fatalf("unexpected removed routes: %v", rc.RemovedRoutes()) } } func TestUpdateRoutesUnadvertisesContainedRoutes(t *testing.T) { - rc := &routeCollector{} + rc := &appctest.RouteCollector{} a := NewAppConnector(t.Logf, rc) 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")} + rc.SetRoutes([]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) + if !slices.EqualFunc(routes, rc.Routes(), prefixEqual) { + t.Fatalf("got %v, want %v", rc.Routes(), routes) } } func TestDomainRoutes(t *testing.T) { - rc := &routeCollector{} + rc := &appctest.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{ @@ -77,43 +110,43 @@ func TestDomainRoutes(t *testing.T) { } func TestObserveDNSResponse(t *testing.T) { - rc := &routeCollector{} + rc := &appctest.RouteCollector{} a := NewAppConnector(t.Logf, rc) // a has no domains configured, so it should not advertise any routes a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8")) - if got, want := rc.routes, ([]netip.Prefix)(nil); !slices.Equal(got, want) { + if got, want := rc.Routes(), ([]netip.Prefix)(nil); !slices.Equal(got, want) { t.Errorf("got %v; want %v", got, want) } 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) { + if got, want := rc.Routes(), wantRoutes; !slices.Equal(got, want) { t.Errorf("got %v; want %v", got, want) } wantRoutes = append(wantRoutes, netip.MustParsePrefix("2001:db8::1/128")) a.ObserveDNSResponse(dnsResponse("example.com.", "2001:db8::1")) - if got, want := rc.routes, wantRoutes; !slices.Equal(got, want) { + if got, want := rc.Routes(), wantRoutes; !slices.Equal(got, want) { t.Errorf("got %v; want %v", got, want) } // don't re-advertise routes that have already been advertised a.ObserveDNSResponse(dnsResponse("example.com.", "2001:db8::1")) - if !slices.Equal(rc.routes, wantRoutes) { - t.Errorf("rc.routes: got %v; want %v", rc.routes, wantRoutes) + if !slices.Equal(rc.Routes(), wantRoutes) { + t.Errorf("rc.Routes(): got %v; want %v", rc.Routes(), wantRoutes) } // 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) { - t.Errorf("rc.routes: got %v; want %v", rc.routes, wantRoutes) + if !slices.Equal(rc.Routes(), wantRoutes) { + t.Errorf("rc.Routes(): got %v; want %v", rc.Routes(), wantRoutes) } if !slices.Contains(a.domains["example.com"], netip.MustParseAddr("192.0.2.1")) { t.Errorf("missing %v from %v", "192.0.2.1", a.domains["exmaple.com"]) @@ -121,19 +154,19 @@ func TestObserveDNSResponse(t *testing.T) { } func TestWildcardDomains(t *testing.T) { - rc := &routeCollector{} + rc := &appctest.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) { + 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) } if got, want := a.wildcards, []string{"example.com"}; !slices.Equal(got, want) { 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 +175,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) } @@ -185,30 +218,13 @@ 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 -} - -// routeCollector implements RouteAdvertiser -var _ RouteAdvertiser = (*routeCollector)(nil) - -func (rc *routeCollector) AdvertiseRoute(pfx netip.Prefix) error { - rc.routes = append(rc.routes, pfx) - return nil +func prefixEqual(a, b netip.Prefix) bool { + return a == b } -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) - } +func prefixCompare(a, b netip.Prefix) int { + if a.Addr().Compare(b.Addr()) == 0 { + return a.Bits() - b.Bits() } - return nil -} - -func prefixEqual(a, b netip.Prefix) bool { - return a.Addr().Compare(b.Addr()) == 0 && a.Bits() == b.Bits() + return a.Addr().Compare(b.Addr()) } diff --git a/appc/appctest/appctest.go b/appc/appctest/appctest.go new file mode 100644 index 0000000000000..d62c0e233c03f --- /dev/null +++ b/appc/appctest/appctest.go @@ -0,0 +1,49 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package appctest + +import ( + "net/netip" + "slices" +) + +// RouteCollector is a test helper that collects the list of routes advertised +type RouteCollector struct { + routes []netip.Prefix + removedRoutes []netip.Prefix +} + +func (rc *RouteCollector) AdvertiseRoute(pfx ...netip.Prefix) error { + rc.routes = append(rc.routes, pfx...) + return nil +} + +func (rc *RouteCollector) UnadvertiseRoute(toRemove ...netip.Prefix) error { + routes := rc.routes + rc.routes = rc.routes[:0] + for _, r := range routes { + if !slices.Contains(toRemove, r) { + rc.routes = append(rc.routes, r) + } else { + rc.removedRoutes = append(rc.removedRoutes, r) + } + } + return nil +} + +// RemovedRoutes returns the list of routes that were removed. +func (rc *RouteCollector) RemovedRoutes() []netip.Prefix { + return rc.removedRoutes +} + +// Routes returns the ordered list of routes that were added, including +// possible duplicates. +func (rc *RouteCollector) Routes() []netip.Prefix { + return rc.routes +} + +func (rc *RouteCollector) SetRoutes(routes []netip.Prefix) error { + rc.routes = routes + return nil +} diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index eeb57a3a8545b..454c3dc66d5ad 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -348,6 +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/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/control/controlclient/auto.go b/control/controlclient/auto.go index 86b03efa58a1d..d0551cdab5782 100644 --- a/control/controlclient/auto.go +++ b/control/controlclient/auto.go @@ -22,6 +22,7 @@ import ( "tailscale.com/types/netmap" "tailscale.com/types/persist" "tailscale.com/types/structs" + "tailscale.com/util/execqueue" ) type LoginGoal struct { @@ -118,7 +119,7 @@ type Auto struct { closed bool updateCh chan struct{} // readable when we should inform the server of a change observer Observer // called to update Client status; always non-nil - observerQueue execQueue + observerQueue execqueue.ExecQueue unregisterHealthWatch func() @@ -675,7 +676,7 @@ func (c *Auto) Shutdown() { direct := c.direct if !closed { c.closed = true - c.observerQueue.shutdown() + c.observerQueue.Shutdown() c.cancelAuthCtxLocked() c.cancelMapCtxLocked() for _, w := range c.unpauseWaiters { @@ -696,7 +697,7 @@ func (c *Auto) Shutdown() { } ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - c.observerQueue.wait(ctx) + c.observerQueue.Wait(ctx) c.logf("Client.Shutdown done.") } } @@ -737,95 +738,3 @@ func (c *Auto) DoNoiseRequest(req *http.Request) (*http.Response, error) { func (c *Auto) GetSingleUseNoiseRoundTripper(ctx context.Context) (http.RoundTripper, *tailcfg.EarlyNoise, error) { return c.direct.GetSingleUseNoiseRoundTripper(ctx) } - -type execQueue struct { - mu sync.Mutex - closed bool - inFlight bool // whether a goroutine is running q.run - doneWaiter chan struct{} // non-nil if waiter is waiting, then closed - queue []func() -} - -func (q *execQueue) Add(f func()) { - q.mu.Lock() - defer q.mu.Unlock() - if q.closed { - return - } - if q.inFlight { - q.queue = append(q.queue, f) - } else { - q.inFlight = true - go q.run(f) - } -} - -// RunSync waits for the queue to be drained and then synchronously runs f. -// It returns an error if the queue is closed before f is run or ctx expires. -func (q *execQueue) RunSync(ctx context.Context, f func()) error { - for { - if err := q.wait(ctx); err != nil { - return err - } - q.mu.Lock() - if q.inFlight { - q.mu.Unlock() - continue - } - defer q.mu.Unlock() - if q.closed { - return errors.New("closed") - } - f() - return nil - } -} - -func (q *execQueue) run(f func()) { - f() - - q.mu.Lock() - for len(q.queue) > 0 && !q.closed { - f := q.queue[0] - q.queue[0] = nil - q.queue = q.queue[1:] - q.mu.Unlock() - f() - q.mu.Lock() - } - q.inFlight = false - q.queue = nil - if q.doneWaiter != nil { - close(q.doneWaiter) - q.doneWaiter = nil - } - q.mu.Unlock() -} - -func (q *execQueue) shutdown() { - q.mu.Lock() - defer q.mu.Unlock() - q.closed = true -} - -// wait waits for the queue to be empty. -func (q *execQueue) wait(ctx context.Context) error { - q.mu.Lock() - waitCh := q.doneWaiter - if q.inFlight && waitCh == nil { - waitCh = make(chan struct{}) - q.doneWaiter = waitCh - } - q.mu.Unlock() - - if waitCh == nil { - return nil - } - - select { - case <-waitCh: - return nil - case <-ctx.Done(): - return ctx.Err() - } -} diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 7c06e94bf1914..7edde9621e028 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 @@ -5791,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 - } - 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 - }) { +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 + } + + 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, }) diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index f1e8aa3b25fc2..9ff967b2ebbb2 100644 --- a/ipn/ipnlocal/local_test.go +++ b/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,14 +1205,52 @@ 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()) 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) + } } } @@ -1250,6 +1289,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) { @@ -1349,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 diff --git a/ipn/ipnlocal/peerapi_test.go b/ipn/ipnlocal/peerapi_test.go index 074d114829951..a5f057bcad54d 100644 --- a/ipn/ipnlocal/peerapi_test.go +++ b/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" @@ -685,10 +686,11 @@ 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") - rc := &routeCollector{} + rc := &appctest.RouteCollector{} eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0) pm := must.Get(newProfileManager(new(mem.Store), t.Logf)) h.ps = &peerAPIServer{ @@ -700,6 +702,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,10 +720,11 @@ 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) { - t.Errorf("got %v; want %v", rc.routes, wantRoutes) + if !slices.Equal(rc.Routes(), wantRoutes) { + t.Errorf("got %v; want %v", rc.Routes(), wantRoutes) } } diff --git a/net/portmapper/upnp.go b/net/portmapper/upnp.go index 67d9cbbab2296..0f44463e61d9e 100644 --- a/net/portmapper/upnp.go +++ b/net/portmapper/upnp.go @@ -252,7 +252,8 @@ func getUPnPRootDevice(ctx context.Context, logf logger.Logf, debug DebugKnobs, } // selectBestService picks the "best" service from the given UPnP root device -// to use to create a port mapping. +// to use to create a port mapping. It may return (nil, nil) if no supported +// service was found in the provided *goupnp.RootDevice. // // loc is the parsed location that was used to fetch the given RootDevice. // @@ -559,6 +560,20 @@ func (c *Client) tryUPnPPortmapWithDevice( return netip.AddrPort{}, nil, err } + // If we have no client, we cannot continue; this can happen if we get + // a valid UPnP response that does not contain any of the service types + // that we know how to use. + if client == nil { + // For debugging, print all available services that we aren't + // using because they're not supported; use c.vlogf so we don't + // spam the logs unless verbose debugging is turned on. + rootDev.Device.VisitServices(func(s *goupnp.Service) { + c.vlogf("unsupported UPnP service: Type=%q ID=%q ControlURL=%q", s.ServiceType, s.ServiceId, s.ControlURL.Str) + }) + + return netip.AddrPort{}, nil, fmt.Errorf("no supported UPnP clients") + } + // Start by trying to make a temporary lease with a duration. var newPort uint16 newPort, err = addAnyPortMapping( diff --git a/net/portmapper/upnp_test.go b/net/portmapper/upnp_test.go index 8748cf42719be..4ca332ff1dc75 100644 --- a/net/portmapper/upnp_test.go +++ b/net/portmapper/upnp_test.go @@ -165,6 +165,172 @@ const ( http://10.0.0.1:2828 +` + + // Huawei, https://github.com/tailscale/tailscale/issues/10911 + huaweiRootDescXML = ` + + + 1 + 0 + + + urn:dslforum-org:device:InternetGatewayDevice:1 + HG531 V1 + Huawei Technologies Co., Ltd. + http://www.huawei.com + Huawei Home Gateway + HG531 V1 + Huawei Model + http://www.huawei.com + G6J8W15326003974 + uuid:00e0fc37-2626-2828-2600-587f668bdd9a + 000000000001 + + + urn:www-huawei-com:service:DeviceConfig:1 + urn:www-huawei-com:serviceId:DeviceConfig1 + /desc/DevCfg.xml + /ctrlt/DeviceConfig_1 + /evt/DeviceConfig_1 + + + urn:dslforum-org:service:LANConfigSecurity:1 + urn:dslforum-org:serviceId:LANConfigSecurity1 + /desc/LANSec.xml + /ctrlt/LANConfigSecurity_1 + /evt/LANConfigSecurity_1 + + + urn:dslforum-org:service:Layer3Forwarding:1 + urn:dslforum-org:serviceId:Layer3Forwarding1 + /desc/L3Fwd.xml + /ctrlt/Layer3Forwarding_1 + /evt/Layer3Forwarding_1 + + + + + urn:dslforum-org:device:WANDevice:1 + WANDevice + Huawei Technologies Co., Ltd. + http://www.huawei.com + Huawei Home Gateway + HG531 V1 + Huawei Model + http://www.huawei.com + G6J8W15326003974 + uuid:00e0fc37-2626-2828-2601-587f668bdd9a + 000000000001 + + + urn:dslforum-org:service:WANDSLInterfaceConfig:1 + urn:dslforum-org:serviceId:WANDSLInterfaceConfig1 + /desc/WanDslIfCfg.xml + /ctrlt/WANDSLInterfaceConfig_1 + /evt/WANDSLInterfaceConfig_1 + + + urn:dslforum-org:service:WANCommonInterfaceConfig:1 + urn:dslforum-org:serviceId:WANCommonInterfaceConfig1 + /desc/WanCommonIfc1.xml + /ctrlt/WANCommonInterfaceConfig_1 + /evt/WANCommonInterfaceConfig_1 + + + + + urn:dslforum-org:device:WANConnectionDevice:1 + WANConnectionDevice + Huawei Technologies Co., Ltd. + http://www.huawei.com + Huawei Home Gateway + HG531 V1 + Huawei Model + http://www.huawei.com + G6J8W15326003974 + uuid:00e0fc37-2626-2828-2603-587f668bdd9a + 000000000001 + + + urn:dslforum-org:service:WANPPPConnection:1 + urn:dslforum-org:serviceId:WANPPPConnection1 + /desc/WanPppConn.xml + /ctrlt/WANPPPConnection_1 + /evt/WANPPPConnection_1 + + + urn:dslforum-org:service:WANEthernetConnectionManagement:1 + urn:dslforum-org:serviceId:WANEthernetConnectionManagement1 + /desc/WanEthConnMgt.xml + /ctrlt/WANEthernetConnectionManagement_1 + /evt/WANEthernetConnectionManagement_1 + + + urn:dslforum-org:service:WANDSLLinkConfig:1 + urn:dslforum-org:serviceId:WANDSLLinkConfig1 + /desc/WanDslLink.xml + /ctrlt/WANDSLLinkConfig_1 + /evt/WANDSLLinkConfig_1 + + + + + + + urn:dslforum-org:device:LANDevice:1 + LANDevice + Huawei Technologies Co., Ltd. + http://www.huawei.com + Huawei Home Gateway + HG531 V1 + Huawei Model + http://www.huawei.com + G6J8W15326003974 + uuid:00e0fc37-2626-2828-2602-587f668bdd9a + 000000000001 + + + urn:dslforum-org:service:WLANConfiguration:1 + urn:dslforum-org:serviceId:WLANConfiguration4 + /desc/WLANCfg.xml + /ctrlt/WLANConfiguration_4 + /evt/WLANConfiguration_4 + + + urn:dslforum-org:service:WLANConfiguration:1 + urn:dslforum-org:serviceId:WLANConfiguration3 + /desc/WLANCfg.xml + /ctrlt/WLANConfiguration_3 + /evt/WLANConfiguration_3 + + + urn:dslforum-org:service:WLANConfiguration:1 + urn:dslforum-org:serviceId:WLANConfiguration2 + /desc/WLANCfg.xml + /ctrlt/WLANConfiguration_2 + /evt/WLANConfiguration_2 + + + urn:dslforum-org:service:WLANConfiguration:1 + urn:dslforum-org:serviceId:WLANConfiguration1 + /desc/WLANCfg.xml + /ctrlt/WLANConfiguration_1 + /evt/WLANConfiguration_1 + + + urn:dslforum-org:service:LANHostConfigManagement:1 + urn:dslforum-org:serviceId:LANHostConfigManagement1 + /desc/LanHostCfgMgmt.xml + /ctrlt/LANHostConfigManagement_1 + /evt/LANHostConfigManagement_1 + + + + + http://127.0.0.1 + + ` ) @@ -233,6 +399,14 @@ func TestGetUPnPClient(t *testing.T) { "*internetgateway2.WANIPConnection1", "saw UPnP type WANIPConnection1 at http://127.0.0.1:NNN/rootDesc.xml; MikroTik Router (MikroTik), method=none\n", }, + { + "huawei", + huaweiRootDescXML, + // services not supported and thus returns nil, but shouldn't crash + "", + "", + }, + // TODO(bradfitz): find a PPP one in the wild } for _, tt := range tests { @@ -375,6 +549,48 @@ func TestGetUPnPPortMapping(t *testing.T) { } } +// TestGetUPnPPortMapping_NoValidServices tests that getUPnPPortMapping doesn't +// crash when a valid UPnP response with no supported services is discovered +// and parsed. +// +// See https://github.com/tailscale/tailscale/issues/10911 +func TestGetUPnPPortMapping_NoValidServices(t *testing.T) { + igd, err := NewTestIGD(t.Logf, TestIGDOptions{UPnP: true}) + if err != nil { + t.Fatal(err) + } + defer igd.Close() + + igd.SetUPnPHandler(&upnpServer{ + t: t, + Desc: huaweiRootDescXML, + }) + + c := newTestClient(t, igd) + defer c.Close() + c.debug.VerboseLogs = true + + ctx := context.Background() + res, err := c.Probe(ctx) + if err != nil { + t.Fatalf("Probe: %v", err) + } + if !res.UPnP { + t.Errorf("didn't detect UPnP") + } + + gw, myIP, ok := c.gatewayAndSelfIP() + if !ok { + t.Fatalf("could not get gateway and self IP") + } + + // This shouldn't panic + _, ok = c.getUPnPPortMapping(ctx, gw, netip.AddrPortFrom(myIP, 12345), 0) + if ok { + t.Fatal("did not expect to get UPnP port mapping") + } +} + func TestGetUPnPPortMappingNoResponses(t *testing.T) { igd, err := NewTestIGD(t.Logf, TestIGDOptions{UPnP: true}) if err != nil { diff --git a/util/execqueue/execqueue.go b/util/execqueue/execqueue.go new file mode 100644 index 0000000000000..889cea2555806 --- /dev/null +++ b/util/execqueue/execqueue.go @@ -0,0 +1,104 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +// Package execqueue implements an ordered asynchronous queue for executing functions. +package execqueue + +import ( + "context" + "errors" + "sync" +) + +type ExecQueue struct { + mu sync.Mutex + closed bool + inFlight bool // whether a goroutine is running q.run + doneWaiter chan struct{} // non-nil if waiter is waiting, then closed + queue []func() +} + +func (q *ExecQueue) Add(f func()) { + q.mu.Lock() + defer q.mu.Unlock() + if q.closed { + return + } + if q.inFlight { + q.queue = append(q.queue, f) + } else { + q.inFlight = true + go q.run(f) + } +} + +// RunSync waits for the queue to be drained and then synchronously runs f. +// It returns an error if the queue is closed before f is run or ctx expires. +func (q *ExecQueue) RunSync(ctx context.Context, f func()) error { + for { + if err := q.Wait(ctx); err != nil { + return err + } + q.mu.Lock() + if q.inFlight { + q.mu.Unlock() + continue + } + defer q.mu.Unlock() + if q.closed { + return errors.New("closed") + } + f() + return nil + } +} + +func (q *ExecQueue) run(f func()) { + f() + + q.mu.Lock() + for len(q.queue) > 0 && !q.closed { + f := q.queue[0] + q.queue[0] = nil + q.queue = q.queue[1:] + q.mu.Unlock() + f() + q.mu.Lock() + } + q.inFlight = false + q.queue = nil + if q.doneWaiter != nil { + close(q.doneWaiter) + q.doneWaiter = nil + } + q.mu.Unlock() +} + +// Shutdown asynchronously signals the queue to stop. +func (q *ExecQueue) Shutdown() { + q.mu.Lock() + defer q.mu.Unlock() + q.closed = true +} + +// Wait waits for the queue to be empty. +func (q *ExecQueue) Wait(ctx context.Context) error { + q.mu.Lock() + waitCh := q.doneWaiter + if q.inFlight && waitCh == nil { + waitCh = make(chan struct{}) + q.doneWaiter = waitCh + } + q.mu.Unlock() + + if waitCh == nil { + return nil + } + + select { + case <-waitCh: + return nil + case <-ctx.Done(): + return ctx.Err() + } +} diff --git a/util/execqueue/execqueue_test.go b/util/execqueue/execqueue_test.go new file mode 100644 index 0000000000000..d10b741f72f8f --- /dev/null +++ b/util/execqueue/execqueue_test.go @@ -0,0 +1,22 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package execqueue + +import ( + "context" + "sync/atomic" + "testing" +) + +func TestExecQueue(t *testing.T) { + ctx := context.Background() + var n atomic.Int32 + q := &ExecQueue{} + defer q.Shutdown() + q.Add(func() { n.Add(1) }) + q.Wait(ctx) + if got := n.Load(); got != 1 { + t.Errorf("n=%d; want 1", got) + } +}