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 <percy@tailscale.com>

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 {
+		HTTPC:               &http.Client{Transport: transport},
-			return must.Get(smallzstd.NewEncoder(nil))
+		CompressLogs:        true,
-		},
-		HTTPC: &http.Client{Transport: transport},
 	}
 	logcfg.FlushDelayFn = func() time.Duration { return 2 * time.Minute }
 

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 (

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.20240404175649-853e3e29a0a6 h1:olrk8pALH8inlmr4+euszd6cDI1dHhkRiFffxQ+fvpU=
-tailscale.com v1.63.0-pre.0.20240327135352-66e4d843c138/go.mod h1:6kGByHNxnFfK1i4gVpdtvpdS1HicHohWXnsfwmXy64I=
+tailscale.com v1.63.0-pre.0.20240404175649-853e3e29a0a6/go.mod h1:6kGByHNxnFfK1i4gVpdtvpdS1HicHohWXnsfwmXy64I=

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,
+		Tun:            b.devices,
-		Router:       cb,
+		Router:         vf,
-		DNS:          cb,
+		DNS:            vf,
-		Dialer:       dialer,
+		ReconfigureVPN: vf.ReconfigureVPN,
-		SetSubsystem: sys.Set,
+		Dialer:         dialer,
-		NetMon:       b.netMon,
+		SetSubsystem:   sys.Set,
+		NetMon:         b.netMon,
 	})
 	if err != nil {
 		return nil, fmt.Errorf("runBackend: NewUserspaceEngine: %v", err)

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 {

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 {
+		HTTPC:               &http.Client{Transport: transport},
-			return must.Get(smallzstd.NewEncoder(nil))
+		CompressLogs:        true,
-		},
-		HTTPC: &http.Client{Transport: transport},
 	}
 	logcfg.FlushDelayFn = func() time.Duration { return 2 * time.Minute }
 

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)
+}