From 71f03cf0d214db0bf2d43ff020189e2c8842c35a Mon Sep 17 00:00:00 2001 From: Percy Wegmann Date: Thu, 4 Apr 2024 07:02:05 -0500 Subject: [PATCH] android: only reconfigure VPN when ready This avoids reconfiguring the VPN both when routes changed and then again when DNS changed. Updates tailscale/corp#18928 Signed-off-by: Percy Wegmann --- cmd/tailscale/backend.go | 8 +-- go.mod | 2 +- go.sum | 4 +- libtailscale/backend.go | 16 +++--- libtailscale/net.go | 15 ++++++ libtailscale/tailscale.go | 8 +-- libtailscale/vpnfacade.go | 105 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 135 insertions(+), 23 deletions(-) create mode 100644 libtailscale/vpnfacade.go diff --git a/cmd/tailscale/backend.go b/cmd/tailscale/backend.go index b8f75c6..1bb424a 100644 --- a/cmd/tailscale/backend.go +++ b/cmd/tailscale/backend.go @@ -26,13 +26,11 @@ import ( "tailscale.com/net/dns" "tailscale.com/net/netmon" "tailscale.com/net/tsdial" - "tailscale.com/smallzstd" "tailscale.com/tsd" "tailscale.com/types/logger" "tailscale.com/types/logid" "tailscale.com/util/clientmetric" "tailscale.com/util/dnsname" - "tailscale.com/util/must" "tailscale.com/wgengine" "tailscale.com/wgengine/netstack" "tailscale.com/wgengine/router" @@ -356,10 +354,8 @@ func (b *backend) SetupLogs(logDir string, logID logid.PrivateID, logf logger.Lo MetricsDelta: clientmetric.EncodeLogTailMetricsDelta, IncludeProcID: true, IncludeProcSequence: true, - NewZstdEncoder: func() logtail.Encoder { - return must.Get(smallzstd.NewEncoder(nil)) - }, - HTTPC: &http.Client{Transport: transport}, + HTTPC: &http.Client{Transport: transport}, + CompressLogs: true, } logcfg.FlushDelayFn = func() time.Duration { return 2 * time.Minute } diff --git a/go.mod b/go.mod index 8b2730a..a774c8a 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( golang.org/x/mobile v0.0.0-20240319015410-c58ccf4b0c87 golang.org/x/sys v0.18.0 inet.af/netaddr v0.0.0-20220617031823-097006376321 - tailscale.com v1.63.0-pre.0.20240327135352-66e4d843c138 + tailscale.com v1.63.0-pre.0.20240404175649-853e3e29a0a6 ) require ( diff --git a/go.sum b/go.sum index df2790d..b24582e 100644 --- a/go.sum +++ b/go.sum @@ -670,5 +670,5 @@ sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o= software.sslmate.com/src/go-pkcs12 v0.4.0 h1:H2g08FrTvSFKUj+D309j1DPfk5APnIdAQAB8aEykJ5k= software.sslmate.com/src/go-pkcs12 v0.4.0/go.mod h1:Qiz0EyvDRJjjxGyUQa2cCNZn/wMyzrRJ/qcDXOQazLI= sourcegraph.com/sourcegraph/appdash v0.0.0-20190731080439-ebfcffb1b5c0/go.mod h1:hI742Nqp5OhwiqlzhgfbWU4mW4yO10fP+LoT9WOswdU= -tailscale.com v1.63.0-pre.0.20240327135352-66e4d843c138 h1:CUxCS0sUOwasW/zpNGt63oLCV7QZL7dnrwYMik4OGwQ= -tailscale.com v1.63.0-pre.0.20240327135352-66e4d843c138/go.mod h1:6kGByHNxnFfK1i4gVpdtvpdS1HicHohWXnsfwmXy64I= +tailscale.com v1.63.0-pre.0.20240404175649-853e3e29a0a6 h1:olrk8pALH8inlmr4+euszd6cDI1dHhkRiFffxQ+fvpU= +tailscale.com v1.63.0-pre.0.20240404175649-853e3e29a0a6/go.mod h1:6kGByHNxnFfK1i4gVpdtvpdS1HicHohWXnsfwmXy64I= diff --git a/libtailscale/backend.go b/libtailscale/backend.go index 3def430..04a4d3c 100644 --- a/libtailscale/backend.go +++ b/libtailscale/backend.go @@ -266,18 +266,18 @@ func newBackend(dataDir, directFileRoot string, appCtx AppContext, store *stateS b.netMon = netMon b.setupLogs(dataDir, logID, logf) dialer := new(tsdial.Dialer) - cb := &router.CallbackRouter{ + vf := &VPNFacade{ SetBoth: b.setCfg, - SplitDNS: false, GetBaseConfigFunc: b.getDNSBaseConfig, } engine, err := wgengine.NewUserspaceEngine(logf, wgengine.Config{ - Tun: b.devices, - Router: cb, - DNS: cb, - Dialer: dialer, - SetSubsystem: sys.Set, - NetMon: b.netMon, + Tun: b.devices, + Router: vf, + DNS: vf, + ReconfigureVPN: vf.ReconfigureVPN, + Dialer: dialer, + SetSubsystem: sys.Set, + NetMon: b.netMon, }) if err != nil { return nil, fmt.Errorf("runBackend: NewUserspaceEngine: %v", err) diff --git a/libtailscale/net.go b/libtailscale/net.go index 9186f27..49ced68 100644 --- a/libtailscale/net.go +++ b/libtailscale/net.go @@ -118,25 +118,33 @@ var googleDNSServers = []netip.Addr{ func (b *backend) updateTUN(service IPNService, rcfg *router.Config, dcfg *dns.OSConfig) error { if reflect.DeepEqual(rcfg, b.lastCfg) && reflect.DeepEqual(dcfg, b.lastDNSCfg) { + b.logger.Logf("updateTUN: no change to Routes or DNS, ignore") return nil } + b.logger.Logf("updateTUN: changed") + defer b.logger.Logf("updateTUN: finished") + // Close previous tunnel(s). // This is necessary for ChromeOS, native Android devices // seem to handle seamless handover between tunnels correctly. // // TODO(eliasnaur): If seamless handover becomes a desirable feature, skip // the closing on ChromeOS. + b.logger.Logf("updateTUN: closing old TUNs") b.CloseTUNs() + b.logger.Logf("updateTUN: closed old TUNs") if len(rcfg.LocalAddrs) == 0 { return nil } builder := service.NewBuilder() + b.logger.Logf("updateTUN: got new builder") if err := builder.SetMTU(defaultMTU); err != nil { return err } + b.logger.Logf("updateTUN: set MTU") if dcfg != nil { nameservers := dcfg.Nameservers if b.avoidEmptyDNS && len(nameservers) == 0 { @@ -152,6 +160,7 @@ func (b *backend) updateTUN(service IPNService, rcfg *router.Config, dcfg *dns.O return err } } + b.logger.Logf("updateTUN: set nameservers") } for _, route := range rcfg.Routes { @@ -161,12 +170,14 @@ func (b *backend) updateTUN(service IPNService, rcfg *router.Config, dcfg *dns.O return err } } + b.logger.Logf("updateTUN: added %d routes", len(rcfg.Routes)) for _, addr := range rcfg.LocalAddrs { if err := builder.AddAddress(addr.Addr().String(), int32(addr.Bits())); err != nil { return err } } + b.logger.Logf("updateTUN: added %d local addrs", len(rcfg.LocalAddrs)) parcelFD, err := builder.Establish() if err != nil { @@ -175,6 +186,7 @@ func (b *backend) updateTUN(service IPNService, rcfg *router.Config, dcfg *dns.O } return fmt.Errorf("VpnService.Builder.establish: %v", err) } + b.logger.Logf("updateTUN: established VPN") if parcelFD == nil { return errVPNNotPrepared @@ -185,6 +197,7 @@ func (b *backend) updateTUN(service IPNService, rcfg *router.Config, dcfg *dns.O if err != nil { return fmt.Errorf("detachFd: %v", err) } + b.logger.Logf("updateTUN: detached FD") // Create TUN device. tunDev, _, err := tun.CreateUnmonitoredTUNFromFD(int(tunFD)) @@ -192,8 +205,10 @@ func (b *backend) updateTUN(service IPNService, rcfg *router.Config, dcfg *dns.O unix.Close(int(tunFD)) return err } + b.logger.Logf("updateTUN: created TUN device") b.devices.add(tunDev) + b.logger.Logf("updateTUN: added TUN device") // TODO(oxtoacart): figure out what to do with this // if err != nil { diff --git a/libtailscale/tailscale.go b/libtailscale/tailscale.go index d0e4bac..93d7a58 100644 --- a/libtailscale/tailscale.go +++ b/libtailscale/tailscale.go @@ -15,11 +15,9 @@ import ( "tailscale.com/logtail" "tailscale.com/logtail/filch" "tailscale.com/net/interfaces" - "tailscale.com/smallzstd" "tailscale.com/types/logger" "tailscale.com/types/logid" "tailscale.com/util/clientmetric" - "tailscale.com/util/must" ) const defaultMTU = 1280 // minimalMTU from wgengine/userspace.go @@ -104,10 +102,8 @@ func (b *backend) setupLogs(logDir string, logID logid.PrivateID, logf logger.Lo MetricsDelta: clientmetric.EncodeLogTailMetricsDelta, IncludeProcID: true, IncludeProcSequence: true, - NewZstdEncoder: func() logtail.Encoder { - return must.Get(smallzstd.NewEncoder(nil)) - }, - HTTPC: &http.Client{Transport: transport}, + HTTPC: &http.Client{Transport: transport}, + CompressLogs: true, } logcfg.FlushDelayFn = func() time.Duration { return 2 * time.Minute } diff --git a/libtailscale/vpnfacade.go b/libtailscale/vpnfacade.go new file mode 100644 index 0000000..cc3963a --- /dev/null +++ b/libtailscale/vpnfacade.go @@ -0,0 +1,105 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package libtailscale + +import ( + "sync" + + "tailscale.com/net/dns" + "tailscale.com/wgengine/router" +) + +var ( + _ router.Router = (*VPNFacade)(nil) + _ dns.OSConfigurator = (*VPNFacade)(nil) +) + +// VPNFacade is an implementation of both wgengine.Router and +// dns.OSConfigurator. When ReconfigureVPN is called by the backend, SetBoth +// gets called. +type VPNFacade struct { + SetBoth func(rcfg *router.Config, dcfg *dns.OSConfig) error + + // GetBaseConfigFunc optionally specifies a function to return the current DNS + // config in response to GetBaseConfig. + // + // If nil, reading the current config isn't supported and GetBaseConfig() + // will return ErrGetBaseConfigNotSupported. + GetBaseConfigFunc func() (dns.OSConfig, error) + + // InitialMTU is the MTU the tun should be initialized with. + // Zero means don't change the MTU from the default. This MTU + // is applied only once, shortly after the TUN is created, and + // ignored thereaftef. + InitialMTU uint32 + + mu sync.Mutex // protects all the following + didSetMTU bool // if we set the MTU already + rcfg *router.Config // last applied router config + dcfg *dns.OSConfig // last applied DNS config +} + +// Up implements wgengine.router. +func (vf *VPNFacade) Up() error { + return nil // TODO: check that all callers have no need for initialization +} + +// Set implements wgengine.router. +func (vf *VPNFacade) Set(rcfg *router.Config) error { + vf.mu.Lock() + defer vf.mu.Unlock() + if vf.rcfg.Equal(rcfg) { + return nil + } + if vf.didSetMTU == false { + vf.didSetMTU = true + rcfg.NewMTU = int(vf.InitialMTU) + } + vf.rcfg = rcfg + return nil +} + +// UpdateMagicsockPort implements wgengine.Router. This implementation +// does nothing and returns nil because this router does not currently need +// to know what the magicsock UDP port is. +func (vf *VPNFacade) UpdateMagicsockPort(_ uint16, _ string) error { + return nil +} + +// SetDNS implements dns.OSConfigurator. +func (vf *VPNFacade) SetDNS(dcfg dns.OSConfig) error { + vf.mu.Lock() + defer vf.mu.Unlock() + if vf.dcfg != nil && vf.dcfg.Equal(dcfg) { + return nil + } + vf.dcfg = &dcfg + return nil +} + +// Implements dns.OSConfigurator. +func (vf *VPNFacade) SupportsSplitDNS() bool { + return false +} + +// Implements dns.OSConfigurator. +func (vf *VPNFacade) GetBaseConfig() (dns.OSConfig, error) { + if vf.GetBaseConfigFunc == nil { + return dns.OSConfig{}, dns.ErrGetBaseConfigNotSupported + } + return vf.GetBaseConfigFunc() +} + +// Implements wgengine.router and dns.OSConfigurator. +func (vf *VPNFacade) Close() error { + return vf.SetBoth(nil, nil) // TODO: check if makes sense +} + +// ReconfigureVPN is the method value passed to wgengine.Config.ReconfigureVPN. +func (vf *VPNFacade) ReconfigureVPN() error { + vf.mu.Lock() + defer vf.mu.Unlock() + + return vf.SetBoth(vf.rcfg, vf.dcfg) +}