From db31550854afbc337826674a6dd8abe8f30e0693 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 12 Mar 2020 20:10:11 -0700 Subject: [PATCH] wgengine: don't Reconfig on boring link changes Signed-off-by: Brad Fitzpatrick --- wgengine/magicsock/magicsock.go | 42 ++++++-------------------------- wgengine/userspace.go | 43 +++++++++++++++++++++++++++++++-- wgengine/wgengine.go | 6 +++++ 3 files changed, 54 insertions(+), 37 deletions(-) diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index db9ad5d6c..4855da2c9 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -64,9 +64,6 @@ type Conn struct { connCtx context.Context // closed on Conn.Close connCtxCancel func() // closes connCtx - linkChangeMu sync.Mutex - linkState *interfaces.State - // addrsByUDP is a map of every remote ip:port to a priority // list of endpoint addresses for a peer. // The priority list is provided by wgengine configuration. @@ -205,7 +202,6 @@ func Listen(opts Options) (*Conn, error) { derpTLSConfig: opts.derpTLSConfig, derps: opts.DERPs, } - c.linkState, _ = getLinkState() if c.derps == nil { c.derps = derpmap.Prod() } @@ -218,7 +214,7 @@ func Listen(opts Options) (*Conn, error) { c.ignoreSTUNPackets() c.pconn.Reset(packetConn.(*net.UDPConn)) - c.reSTUN("initial") + c.ReSTUN("initial") c.goroutines.Add(1) go func() { @@ -1110,42 +1106,18 @@ func (c *Conn) Close() error { return err } -func (c *Conn) reSTUN(why string) { +// ReSTUN triggers an address discovery. +// The provided why string is for debug logging only. +func (c *Conn) ReSTUN(why string) { select { case c.startEpUpdate <- why: case <-c.donec(): } } -// LinkChange should be called whenever something changed with the -// network, no matter how minor. The LinkChange method then looks -// at the state of the network and decides whether the change from -// before is interesting enough to warrant taking action on. -func (c *Conn) LinkChange() { - defer c.reSTUN("link-change") - - c.linkChangeMu.Lock() - defer c.linkChangeMu.Unlock() - - cur, err := getLinkState() - if err != nil { - return - } - if c.linkState != nil && !cur.Equal(c.linkState) { - c.linkState = cur - c.rebind() - } -} - -func getLinkState() (*interfaces.State, error) { - s, err := interfaces.GetState() - if s != nil { - s.RemoveTailscaleInterfaces() - } - return s, err -} - -func (c *Conn) rebind() { +// Rebind closes and re-binds the UDP sockets. +// It should be followed by a call to ReSTUN. +func (c *Conn) Rebind() { if c.pconnPort != 0 { c.pconn.mu.Lock() if err := c.pconn.pconn.Close(); err != nil { diff --git a/wgengine/userspace.go b/wgengine/userspace.go index b2f06e21a..3985a4333 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -18,6 +18,7 @@ import ( "github.com/tailscale/wireguard-go/device" "github.com/tailscale/wireguard-go/tun" "github.com/tailscale/wireguard-go/wgcfg" + "tailscale.com/net/interfaces" "tailscale.com/tailcfg" "tailscale.com/types/logger" "tailscale.com/wgengine/filter" @@ -46,6 +47,7 @@ type userspaceEngine struct { peerSequence []wgcfg.Key endpoints []string pingers map[wgcfg.Key]context.CancelFunc // mu must be held to call CancelFunc + linkState *interfaces.State } type Loggify struct { @@ -103,6 +105,7 @@ func newUserspaceEngineAdvanced(logf logger.Logf, tundev tun.Device, routerGen R tundev: tundev, pingers: make(map[wgcfg.Key]context.CancelFunc), } + e.linkState, _ = getLinkState() mon, err := monitor.New(logf, func() { e.LinkChange(false) }) if err != nil { @@ -585,13 +588,41 @@ func (e *userspaceEngine) Wait() { <-e.waitCh } +func (e *userspaceEngine) setLinkState(st *interfaces.State) (changed bool) { + if st == nil { + return false + } + e.mu.Lock() + defer e.mu.Unlock() + changed = e.linkState == nil || !st.Equal(e.linkState) + e.linkState = st + return changed +} + func (e *userspaceEngine) LinkChange(isExpensive bool) { - e.logf("LinkChange(isExpensive=%v): rebinding socket", isExpensive) + cur, err := getLinkState() + if err != nil { + e.logf("LinkChange: interfaces.GetState: %v", err) + return + } + needRebind := e.setLinkState(cur) + + e.logf("LinkChange(isExpensive=%v); needsRebind=%v", isExpensive, needRebind) + + why := "link-change-minor" + if needRebind { + why = "link-change-major" + e.magicConn.Rebind() + } + e.magicConn.ReSTUN(why) + if !needRebind { + return + } + e.wgLock.Lock() defer e.wgLock.Unlock() // TODO(crawshaw): use isExpensive=true to switch into "client mode" on macOS? - e.magicConn.LinkChange() // TODO(crawshaw): when we have an incremental notion of reconfig, // be gentler here. No need to smash in-progress connections, @@ -606,6 +637,14 @@ func (e *userspaceEngine) LinkChange(isExpensive bool) { } } +func getLinkState() (*interfaces.State, error) { + s, err := interfaces.GetState() + if s != nil { + s.RemoveTailscaleInterfaces() + } + return s, err +} + func (e *userspaceEngine) SetNetInfoCallback(cb NetInfoCallback) { e.magicConn.SetNetInfoCallback(cb) } diff --git a/wgengine/wgengine.go b/wgengine/wgengine.go index 6d54bb790..baf9f35a8 100644 --- a/wgengine/wgengine.go +++ b/wgengine/wgengine.go @@ -125,6 +125,12 @@ type Engine interface { // link has changed. The isExpensive parameter is set on links // where sending packets uses substantial power or money, // such as mobile data on a phone. + // + // LinkChange should be called whenever something changed with + // the network, no matter how minor. The implementation should + // look at the state of the network and decide whether the + // change from before is interesting enough to warrant taking + // action on. LinkChange(isExpensive bool) // SetDERPEnabled controls whether DERP is enabled.