diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index b5a0a353c..03863648a 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -5458,12 +5458,16 @@ func shouldUseOneCGNATRoute(logf logger.Logf, mon *netmon.Monitor, controlKnobs return true } - // Also prefer to do this on the Mac, so that we don't need to constantly - // update the network extension configuration (which is disruptive to - // Chrome, see https://github.com/tailscale/tailscale/issues/3102). Only - // use fine-grained routes if another interfaces is also using the CGNAT + // Prefer a single CGNAT route on platforms where updateing the VPN + // configuration is espensive. On macOS, changing the network extension + // configuration can disrupt existing connections notably Chrome; see + // https://github.com/tailscale/tailscale/issues/3102). On Android, updating + // VpnService.Builder configuration requires establishing a new VPN interface, + // which tears down long lived TCP connections. + // + // Only use fine-grained routes if another interfaces is also using the CGNAT // IP range. - if versionOS == "macOS" { + if versionOS == "macOS" || versionOS == "android" { hasCGNATInterface, err := mon.HasCGNATInterface() if err != nil { logf("shouldUseOneCGNATRoute: Could not determine if any interfaces use CGNAT: %v", err) diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 70cbc8991..21188d784 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -32,6 +32,7 @@ import ( "tailscale.com/appc" "tailscale.com/appc/appctest" "tailscale.com/control/controlclient" + "tailscale.com/control/controlknobs" "tailscale.com/drive" "tailscale.com/drive/driveimpl" "tailscale.com/feature" @@ -8158,3 +8159,76 @@ func TestStartPreservesLoginFlags(t *testing.T) { t.Errorf("cc.Login was never called with LoginEphemeral; got flags=%v", flags) } } + +func TestShouldUseOneCGNATRoute(t *testing.T) { + tests := []struct { + name string + versionOS string + want bool + }{ + {"android", "android", true}, + {"macOS", "macOS", true}, + {"plan9", "plan9", true}, + {"linux", "linux", false}, + {"windows", "windows", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := shouldUseOneCGNATRoute(t.Logf, nil, nil, tt.versionOS) + if got != tt.want { + t.Errorf("shouldUseOneCGNATRoute(%q) = %v; want %v", tt.versionOS, got, tt.want) + } + }) + } + + // Control knob takes precedence over everything. + t.Run("control-knob-override", func(t *testing.T) { + knobs := &controlknobs.Knobs{} + knobs.OneCGNAT.Store(opt.NewBool(false)) + if got := shouldUseOneCGNATRoute(t.Logf, nil, knobs, "android"); got { + t.Error("control knob should override android default; got true, want false") + } + knobs.OneCGNAT.Store(opt.NewBool(true)) + if got := shouldUseOneCGNATRoute(t.Logf, nil, knobs, "linux"); !got { + t.Error("control knob should override linux default; got false, want true") + } + }) +} + +func TestPeerRoutesCGNATCollapse(t *testing.T) { + pp := netip.MustParsePrefix + + // With cgnatThreshold=1 (oneCGNATRoute), adding a peer should not + // change the route list. Both collapse to a single 100.64.0.0/10. + twoPeers := []wgcfg.Peer{ + {AllowedIPs: []netip.Prefix{pp("100.64.0.1/32")}}, + {AllowedIPs: []netip.Prefix{pp("100.64.0.2/32")}}, + } + threePeers := []wgcfg.Peer{ + {AllowedIPs: []netip.Prefix{pp("100.64.0.1/32")}}, + {AllowedIPs: []netip.Prefix{pp("100.64.0.2/32")}}, + {AllowedIPs: []netip.Prefix{pp("100.64.0.3/32")}}, + } + + routesTwo := peerRoutes(t.Logf, twoPeers, 1, true) + routesThree := peerRoutes(t.Logf, threePeers, 1, true) + + wantCGNAT := []netip.Prefix{pp("100.64.0.0/10")} + if !reflect.DeepEqual(routesTwo, wantCGNAT) { + t.Errorf("two peers: got %v; want %v", routesTwo, wantCGNAT) + } + if !reflect.DeepEqual(routesThree, wantCGNAT) { + t.Errorf("three peers: got %v; want %v", routesThree, wantCGNAT) + } + + // Subnet routes must still appear alongside the collapsed CGNAT route. + peersWithSubnet := []wgcfg.Peer{ + {AllowedIPs: []netip.Prefix{pp("100.64.0.1/32")}}, + {AllowedIPs: []netip.Prefix{pp("100.64.0.2/32"), pp("10.0.0.0/24")}}, + } + got := peerRoutes(t.Logf, peersWithSubnet, 1, true) + want := []netip.Prefix{pp("100.64.0.0/10"), pp("10.0.0.0/24")} + if !reflect.DeepEqual(got, want) { + t.Errorf("with subnet: got %v; want %v", got, want) + } +}