From c8e912896e564e6a30b9efa0e5549fc7b4fc8498 Mon Sep 17 00:00:00 2001 From: Percy Wegmann Date: Tue, 23 Apr 2024 10:56:36 -0500 Subject: [PATCH] wgengine/router: consolidate routes before reconfiguring router for mobile clients This helps reduce memory pressure on tailnets with large numbers of routes. Updates tailscale/corp#19332 Signed-off-by: Percy Wegmann --- wgengine/router/consolidating_router.go | 59 +++++++++++++++++ wgengine/router/consolidating_router_test.go | 68 ++++++++++++++++++++ wgengine/userspace.go | 20 +++++- 3 files changed, 146 insertions(+), 1 deletion(-) create mode 100644 wgengine/router/consolidating_router.go create mode 100644 wgengine/router/consolidating_router_test.go diff --git a/wgengine/router/consolidating_router.go b/wgengine/router/consolidating_router.go new file mode 100644 index 000000000..10c4825d8 --- /dev/null +++ b/wgengine/router/consolidating_router.go @@ -0,0 +1,59 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package router + +import ( + "go4.org/netipx" + "tailscale.com/types/logger" +) + +// ConsolidatingRoutes wraps a Router with logic that consolidates Routes +// whenever Set is called. It attempts to consolidate cfg.Routes into the +// smallest possible set. +func ConsolidatingRoutes(logf logger.Logf, router Router) Router { + return &consolidatingRouter{Router: router, logf: logger.WithPrefix(logf, "router: ")} +} + +type consolidatingRouter struct { + Router + logf logger.Logf +} + +// Set implements Router and attempts to consolidate cfg.Routes into the +// smallest possible set. +func (cr *consolidatingRouter) Set(cfg *Config) error { + return cr.Router.Set(cr.consolidateRoutes(cfg)) +} + +func (cr *consolidatingRouter) consolidateRoutes(cfg *Config) *Config { + if cfg == nil { + return nil + } + if len(cfg.Routes) < 2 { + return cfg + } + if len(cfg.Routes) == 2 && cfg.Routes[0].Addr().Is4() != cfg.Routes[1].Addr().Is4() { + return cfg + } + var builder netipx.IPSetBuilder + for _, route := range cfg.Routes { + builder.AddPrefix(route) + } + set, err := builder.IPSet() + if err != nil { + cr.logf("consolidateRoutes failed, keeping existing routes: %s", err) + return cfg + } + newRoutes := set.Prefixes() + oldLength := len(cfg.Routes) + newLength := len(newRoutes) + if oldLength == newLength { + // Nothing consolidated, return as-is. + return cfg + } + cr.logf("consolidated %d routes down to %d", oldLength, newLength) + newCfg := *cfg + newCfg.Routes = newRoutes + return &newCfg +} diff --git a/wgengine/router/consolidating_router_test.go b/wgengine/router/consolidating_router_test.go new file mode 100644 index 000000000..871682d13 --- /dev/null +++ b/wgengine/router/consolidating_router_test.go @@ -0,0 +1,68 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package router + +import ( + "log" + "net/netip" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestConsolidateRoutes(t *testing.T) { + parseRoutes := func(routes ...string) []netip.Prefix { + parsed := make([]netip.Prefix, 0, len(routes)) + for _, routeString := range routes { + route, err := netip.ParsePrefix(routeString) + if err != nil { + t.Fatal(err) + } + parsed = append(parsed, route) + } + return parsed + } + + tests := []struct { + name string + cfg *Config + want *Config + }{ + { + "nil cfg", + nil, + nil, + }, + { + "single route", + &Config{Routes: parseRoutes("10.0.0.0/32")}, + &Config{Routes: parseRoutes("10.0.0.0/32")}, + }, + { + "two routes from different families", + &Config{Routes: parseRoutes("10.0.0.0/32", "2603:1030:c02::/47")}, + &Config{Routes: parseRoutes("10.0.0.0/32", "2603:1030:c02::/47")}, + }, + { + "two disjoint routes", + &Config{Routes: parseRoutes("10.0.0.0/32", "10.0.2.0/32")}, + &Config{Routes: parseRoutes("10.0.0.0/32", "10.0.2.0/32")}, + }, + { + "two overlapping routes", + &Config{Routes: parseRoutes("10.0.0.0/32", "10.0.0.0/31")}, + &Config{Routes: parseRoutes("10.0.0.0/31")}, + }, + } + + cr := &consolidatingRouter{logf: log.Printf} + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := cr.consolidateRoutes(test.cfg) + if diff := cmp.Diff(got, test.want); diff != "" { + t.Errorf("wrong result; (-got+want):%v", diff) + } + }) + } +} diff --git a/wgengine/userspace.go b/wgengine/userspace.go index 73de66393..f7df59f9f 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -47,6 +47,7 @@ import ( "tailscale.com/util/deephash" "tailscale.com/util/mak" "tailscale.com/util/set" + "tailscale.com/version" "tailscale.com/wgengine/capture" "tailscale.com/wgengine/filter" "tailscale.com/wgengine/magicsock" @@ -281,13 +282,30 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error) } closePool.add(tsTUNDev) + rtr := conf.Router + if version.IsMobile() { + // Android and iOS don't handle large numbers of routes well, so we + // wrap the Router with one that consolidates routes down to the + // smallest number possible. + // + // On Android, too many routes at VPN configuration time result in an + // android.os.TransactionTooLargeException because Android's VPNBuilder + // tries to send the entire set of routes to the VPNService as a single + // Bundle, which is typically limited to 1 MB. The number of routes + // that's too much seems to be very roughly around 4000. + // + // On iOS, the VPNExtension is limited to only 50 MB of memory, so + // keeping the number of routes down helps with memory consumption. + rtr = router.ConsolidatingRoutes(logf, rtr) + } + e := &userspaceEngine{ timeNow: mono.Now, logf: logf, reqCh: make(chan struct{}, 1), waitCh: make(chan struct{}), tundev: tsTUNDev, - router: conf.Router, + router: rtr, confListenPort: conf.ListenPort, birdClient: conf.BIRDClient, controlKnobs: conf.ControlKnobs,