wgengine/router: remove wireguard-go config from settings.

Instead, pass in only exactly the relevant configuration pieces
that the OS network stack cares about.

Signed-off-by: David Anderson <danderson@tailscale.com>

wgengine/router/ifconfig_windows.go

@@ -20,7 +20,6 @@ import (
 	winipcfg "github.com/tailscale/winipcfg-go"
 	"github.com/tailscale/wireguard-go/device"
 	"github.com/tailscale/wireguard-go/tun"
-	"github.com/tailscale/wireguard-go/wgcfg"
 	"golang.org/x/sys/windows"
 	"golang.org/x/sys/windows/registry"
 	"tailscale.com/wgengine/winnet"
@@ -237,7 +236,7 @@ func setFirewall(ifcGUID *windows.GUID) (bool, error) {
 	return false, nil
 }
 
-func configureInterface(m *wgcfg.Config, tun *tun.NativeTun, dns []wgcfg.IP, dnsDomains []string) error {
+func configureInterface(rs Settings, tun *tun.NativeTun) error {
 	const mtu = 0
 	guid := tun.GUID()
 	log.Printf("wintun GUID is %v\n", guid)
@@ -263,13 +262,13 @@ func configureInterface(m *wgcfg.Config, tun *tun.NativeTun, dns []wgcfg.IP, dns
 		}
 	}()
 
-	setDNSDomains(guid, dnsDomains)
+	setDNSDomains(guid, rs.DNSDomains)
 
 	routes := []winipcfg.RouteData{}
 	var firstGateway4 *net.IP
 	var firstGateway6 *net.IP
-	addresses := make([]*net.IPNet, len(m.Addresses))
+	addresses := make([]*net.IPNet, len(rs.LocalAddrs))
-	for i, addr := range m.Addresses {
+	for i, addr := range rs.LocalAddrs {
 		ipnet := addr.IPNet()
 		addresses[i] = ipnet
 		gateway := ipnet.IP
@@ -282,48 +281,46 @@ func configureInterface(m *wgcfg.Config, tun *tun.NativeTun, dns []wgcfg.IP, dns
 
 	foundDefault4 := false
 	foundDefault6 := false
-	for _, peer := range m.Peers {
+	for _, route := range rs.Routes {
-		for _, allowedip := range peer.AllowedIPs {
+		if (route.IP.Is4() && firstGateway4 == nil) || (route.IP.Is6() && firstGateway6 == nil) {
-			if (allowedip.IP.Is4() && firstGateway4 == nil) || (allowedip.IP.Is6() && firstGateway6 == nil) {
+			return errors.New("Due to a Windows limitation, one cannot have interface routes without an interface address")
-				return errors.New("Due to a Windows limitation, one cannot have interface routes without an interface address")
-			}
-
-			ipn := allowedip.IPNet()
-			var gateway net.IP
-			if allowedip.IP.Is4() {
-				gateway = *firstGateway4
-			} else if allowedip.IP.Is6() {
-				gateway = *firstGateway6
-			}
-			r := winipcfg.RouteData{
-				Destination: net.IPNet{
-					IP:   ipn.IP.Mask(ipn.Mask),
-					Mask: ipn.Mask,
-				},
-				NextHop: gateway,
-				Metric:  0,
-			}
-			if bytes.Compare(r.Destination.IP, gateway) == 0 {
-				// no need to add a route for the interface's
-				// own IP. The kernel does that for us.
-				// If we try to replace it, we'll fail to
-				// add the route unless NextHop is set, but
-				// then the interface's IP won't be pingable.
-				continue
-			}
-			if allowedip.IP.Is4() {
-				if allowedip.Mask == 0 {
-					foundDefault4 = true
-				}
-				r.NextHop = *firstGateway4
-			} else if allowedip.IP.Is6() {
-				if allowedip.Mask == 0 {
-					foundDefault6 = true
-				}
-				r.NextHop = *firstGateway6
-			}
-			routes = append(routes, r)
 		}
+
+		ipn := route.IPNet()
+		var gateway net.IP
+		if route.IP.Is4() {
+			gateway = *firstGateway4
+		} else if route.IP.Is6() {
+			gateway = *firstGateway6
+		}
+		r := winipcfg.RouteData{
+			Destination: net.IPNet{
+				IP:   ipn.IP.Mask(ipn.Mask),
+				Mask: ipn.Mask,
+			},
+			NextHop: gateway,
+			Metric:  0,
+		}
+		if bytes.Compare(r.Destination.IP, gateway) == 0 {
+			// no need to add a route for the interface's
+			// own IP. The kernel does that for us.
+			// If we try to replace it, we'll fail to
+			// add the route unless NextHop is set, but
+			// then the interface's IP won't be pingable.
+			continue
+		}
+		if route.IP.Is4() {
+			if route.Mask == 0 {
+				foundDefault4 = true
+			}
+			r.NextHop = *firstGateway4
+		} else if route.IP.Is6() {
+			if route.Mask == 0 {
+				foundDefault6 = true
+			}
+			r.NextHop = *firstGateway6
+		}
+		routes = append(routes, r)
 	}
 
 	err = iface.SyncAddresses(addresses)
@@ -362,7 +359,7 @@ func configureInterface(m *wgcfg.Config, tun *tun.NativeTun, dns []wgcfg.IP, dns
 	}
 
 	var dnsIPs []net.IP
-	for _, ip := range dns {
+	for _, ip := range rs.DNS {
 		dnsIPs = append(dnsIPs, ip.IP())
 	}
 	err = iface.SetDNS(dnsIPs)

wgengine/router/router.go

@@ -7,8 +7,6 @@
 package router
 
 import (
-	"fmt"
-
 	"github.com/tailscale/wireguard-go/device"
 	"github.com/tailscale/wireguard-go/tun"
 	"github.com/tailscale/wireguard-go/wgcfg"
@@ -22,10 +20,10 @@ type Router interface {
 	// Up brings the router up.
 	Up() error
 
-	// SetRoutes is called regularly on network map updates.
+	// Set updates the OS network stack with new settings. It may be
-	// It's how you kernel route table entries are populated for
+	// called multiple times with identical Settings, which the
-	// each peer.
+	// implementation should handle gracefully.
-	SetRoutes(RouteSettings) error
+	Set(Settings) error
 
 	// Close closes the router.
 	Close() error
@@ -37,23 +35,12 @@ func New(logf logger.Logf, wgdev *device.Device, tundev tun.Device) (Router, err
 	return newUserspaceRouter(logf, wgdev, tundev)
 }
 
-// RouteSettings is the full WireGuard config data (set of peers keys,
+// Settings is the subset of Tailscale configuration that is relevant
-// IP, etc in wgcfg.Config) plus the things that WireGuard doesn't do
+// to the OS's network stack.
-// itself, like DNS stuff.
+type Settings struct {
-type RouteSettings struct {
 	LocalAddrs   []wgcfg.CIDR
 	DNS          []wgcfg.IP
 	DNSDomains   []string
+	Routes       []wgcfg.CIDR // routes to point into the Tailscale interface
 	SubnetRoutes []wgcfg.CIDR // subnets being advertised to other Tailscale nodes
-	Cfg          *wgcfg.Config
-}
-
-// OnlyRelevantParts returns a string minimally describing the route settings.
-func (rs *RouteSettings) OnlyRelevantParts() string {
-	var peers [][]wgcfg.CIDR
-	for _, p := range rs.Cfg.Peers {
-		peers = append(peers, p.AllowedIPs)
-	}
-	return fmt.Sprintf("%v %v %v %v %v",
-		rs.LocalAddrs, rs.DNS, rs.DNSDomains, rs.SubnetRoutes, peers)
 }

wgengine/router/router_darwin.go

@@ -26,7 +26,7 @@ func (r *darwinRouter) Up() error {
 	return nil
 }
 
-func (r *darwinRouter) SetRoutes(rs RouteSettings) error {
+func (r *darwinRouter) Set(rs Settings) error {
 	if SetRoutesFunc != nil {
 		return SetRoutesFunc(rs)
 	}

wgengine/router/router_darwin_support.go

@@ -6,7 +6,7 @@
 
 package router
 
-// SetRoutesFunc applies the given route settings to the OS network
+// SetRoutesFunc applies the given router settings to the OS network
 // stack.
 //
 // This is logically part of the router_darwin.go implementation, and
@@ -22,4 +22,4 @@ package router
 // as MacOS, so that we don't have to wait until the Mac CI to
 // discover that we broke it. So this one definition needs to exist in
 // both the darwin and linux builds. Hence this file and build tag.
-var SetRoutesFunc func(rs RouteSettings) error
+var SetRoutesFunc func(rs Settings) error

wgengine/router/router_fake.go

@@ -25,8 +25,8 @@ func (r fakeRouter) Up() error {
 	return nil
 }
 
-func (r fakeRouter) SetRoutes(rs RouteSettings) error {
+func (r fakeRouter) Set(rs Settings) error {
-	r.logf("Warning: fakeRouter.SetRoutes: not implemented.")
+	r.logf("Warning: fakeRouter.Set: not implemented.")
 	return nil
 }
 

wgengine/router/router_freebsd.go

@@ -55,7 +55,7 @@ func (r *freebsdRouter) Up() error {
 	return nil
 }
 
-func (r *freebsdRouter) SetRoutes(rs RouteSettings) error {
+func (r *freebsdRouter) Set(rs Settings) error {
 	if len(rs.LocalAddrs) == 0 {
 		return nil
 	}
@@ -95,10 +95,8 @@ func (r *freebsdRouter) SetRoutes(rs RouteSettings) error {
 	}
 
 	newRoutes := make(map[wgcfg.CIDR]struct{})
-	for _, peer := range rs.Cfg.Peers {
+	for _, route := range rs.Routes {
-		for _, route := range peer.AllowedIPs {
+		newRoutes[route] = struct{}{}
-			newRoutes[route] = struct{}{}
-		}
 	}
 	// Delete any pre-existing routes.
 	for route := range r.routes {

wgengine/router/router_linux.go

@@ -138,90 +138,59 @@ func (r *linuxRouter) Close() error {
 	return ret
 }
 
-func (r *linuxRouter) SetRoutes(rs RouteSettings) error {
+// Set implements the Router interface.
+func (r *linuxRouter) Set(rs Settings) error {
+	// cidrDiff calls add and del as needed to make the set of prefixes in
+	// old and new match. Returns a map version of new, and the first
+	// error encountered while reconfiguring, if any.
+	cidrDiff := func(kind string, old map[wgcfg.CIDR]bool, new []wgcfg.CIDR, add, del func(wgcfg.CIDR) error) (map[wgcfg.CIDR]bool, error) {
+		var (
+			ret  = make(map[wgcfg.CIDR]bool, len(new))
+			errq error
+		)
+
+		for _, cidr := range new {
+			ret[cidr] = true
+		}
+		for cidr := range old {
+			if ret[cidr] {
+				continue
+			}
+			if err := del(cidr); err != nil {
+				r.logf("%s del failed: %v", kind, err)
+				if errq == nil {
+					errq = err
+				}
+			}
+		}
+		for cidr := range ret {
+			if old[cidr] {
+				continue
+			}
+			if err := add(cidr); err != nil {
+				r.logf("%s add failed: %v", kind, err)
+				if errq == nil {
+					errq = err
+				}
+			}
+		}
+
+		return ret, errq
+	}
+
 	var errq error
 
-	newAddrs := make(map[wgcfg.CIDR]bool)
+	newAddrs, err := cidrDiff("addr", r.addrs, rs.LocalAddrs, r.addAddress, r.delAddress)
-	for _, addr := range rs.LocalAddrs {
+	if err != nil && errq == nil {
-		newAddrs[addr] = true
+		errq = err
 	}
-	for addr := range r.addrs {
+	newRoutes, err := cidrDiff("route", r.routes, rs.Routes, r.addRoute, r.delRoute)
-		if newAddrs[addr] {
+	if err != nil && errq == nil {
-			continue
+		errq = err
-		}
-		if err := r.delAddress(addr); err != nil {
-			r.logf("addr del failed: %v", err)
-			if errq == nil {
-				errq = err
-			}
-		}
 	}
-	for addr := range newAddrs {
+	newSubnetRoutes, err := cidrDiff("subnet rule", r.subnetRoutes, rs.SubnetRoutes, r.addSubnetRule, r.delSubnetRule)
-		if r.addrs[addr] {
+	if err != nil && errq == nil {
-			continue
+		errq = err
-		}
-		if err := r.addAddress(addr); err != nil {
-			r.logf("addr add failed: %v", err)
-			if errq == nil {
-				errq = err
-			}
-		}
-	}
-
-	newRoutes := make(map[wgcfg.CIDR]bool)
-	for _, peer := range rs.Cfg.Peers {
-		for _, route := range peer.AllowedIPs {
-			newRoutes[route] = true
-		}
-	}
-	for route := range r.routes {
-		if newRoutes[route] {
-			continue
-		}
-		if err := r.delRoute(route); err != nil {
-			r.logf("route del failed: %v", err)
-			if errq == nil {
-				errq = err
-			}
-		}
-	}
-	for route := range newRoutes {
-		if r.routes[route] {
-			continue
-		}
-		if err := r.addRoute(route); err != nil {
-			r.logf("route add failed: %v", err)
-			if errq == nil {
-				errq = err
-			}
-		}
-	}
-
-	newSubnetRoutes := map[wgcfg.CIDR]bool{}
-	for _, route := range rs.SubnetRoutes {
-		newSubnetRoutes[route] = true
-	}
-	for route := range r.subnetRoutes {
-		if newSubnetRoutes[route] {
-			continue
-		}
-		if err := r.delSubnetRule(route); err != nil {
-			r.logf("subnet rule del failed: %v", err)
-			if errq == nil {
-				errq = err
-			}
-		}
-	}
-	for route := range newSubnetRoutes {
-		if r.subnetRoutes[route] {
-			continue
-		}
-		if err := r.addSubnetRule(route); err != nil {
-			r.logf("subnet rule add failed: %v", err)
-			if errq == nil {
-				errq = err
-			}
-		}
 	}
 
 	r.addrs = newAddrs

wgengine/router/router_openbsd.go

@@ -60,7 +60,7 @@ func (r *openbsdRouter) Up() error {
 	return nil
 }
 
-func (r *openbsdRouter) SetRoutes(rs RouteSettings) error {
+func (r *openbsdRouter) Set(rs Settings) error {
 	// TODO: support configuring multiple local addrs on interface.
 	if len(rs.LocalAddrs) != 1 {
 		return errors.New("freebsd doesn't support setting multiple local addrs yet")
@@ -114,10 +114,8 @@ func (r *openbsdRouter) SetRoutes(rs RouteSettings) error {
 	}
 
 	newRoutes := make(map[wgcfg.CIDR]struct{})
-	for _, peer := range rs.Cfg.Peers {
+	for _, route := range rs.Routes {
-		for _, route := range peer.AllowedIPs {
+		newRoutes[route] = struct{}{}
-			newRoutes[route] = struct{}{}
-		}
 	}
 	for route := range r.routes {
 		if _, keep := newRoutes[route]; !keep {

wgengine/router/router_windows.go

@@ -45,8 +45,8 @@ func (r *winRouter) Up() error {
 	return nil
 }
 
-func (r *winRouter) SetRoutes(rs RouteSettings) error {
+func (r *winRouter) Set(rs Settings) error {
-	err := configureInterface(rs.Cfg, r.nativeTun, rs.DNS, rs.DNSDomains)
+	err := configureInterface(rs, r.nativeTun)
 	if err != nil {
 		r.logf("ConfigureInterface: %v\n", err)
 		return err

wgengine/userspace.go

@@ -58,7 +58,6 @@ type userspaceEngine struct {
 	wgLock       sync.Mutex // serializes all wgdev operations; see lock order comment below
 	lastReconfig string
 	lastCfg      wgcfg.Config
-	lastRoutes   string
 
 	mu             sync.Mutex // guards following; see lock order comment below
 	filt           *filter.Filter
@@ -241,7 +240,7 @@ func newUserspaceEngineAdvanced(logf logger.Logf, tundev tun.Device, routerGen R
 		e.wgdev.Close()
 		return nil, err
 	}
-	if err := e.router.SetRoutes(router.RouteSettings{Cfg: new(wgcfg.Config)}); err != nil {
+	if err := e.router.Set(router.Settings{}); err != nil {
 		e.wgdev.Close()
 		return nil, err
 	}
@@ -324,6 +323,16 @@ func (e *userspaceEngine) pinger(peerKey wgcfg.Key, ips []wgcfg.IP) {
 	}
 }
 
+func configSignature(cfg *wgcfg.Config, dnsDomains []string, localRoutes []wgcfg.CIDR) (string, error) {
+	// TODO(apenwarr): get rid of uapi stuff for in-process comms
+	uapi, err := cfg.ToUAPI()
+	if err != nil {
+		return "", err
+	}
+
+	return fmt.Sprintf("%s %v %v", uapi, dnsDomains, localRoutes), nil
+}
+
 // TODO(apenwarr): dnsDomains really ought to be in wgcfg.Config.
 // However, we don't actually ever provide it to wireguard and it's not in
 // the traditional wireguard config format. On the other hand, wireguard
@@ -346,13 +355,10 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, dnsDomains []string, local
 	}
 	e.mu.Unlock()
 
-	// TODO(apenwarr): get rid of uapi stuff for in-process comms
+	rc, err := configSignature(cfg, dnsDomains, localRoutes)
-	uapi, err := cfg.ToUAPI()
 	if err != nil {
 		return err
 	}
-
-	rc := uapi + "\x00" + strings.Join(dnsDomains, "\x00")
 	if rc == e.lastReconfig {
 		return ErrNoChanges
 	}
@@ -390,30 +396,18 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, dnsDomains []string, local
 		})
 	}
 
-	rs := router.RouteSettings{
+	rs := router.Settings{
 		LocalAddrs:   addrs,
-		Cfg:          cfg,
 		DNS:          cfg.DNS,
 		DNSDomains:   dnsDomains,
 		SubnetRoutes: localRoutes,
 	}
+	for _, peer := range cfg.Peers {
+		rs.Routes = append(rs.Routes, peer.AllowedIPs...)
+	}
 
-	// TODO(apenwarr): all the parts of RouteSettings should be "relevant."
+	if err := e.router.Set(rs); err != nil {
-	// We're checking only the "relevant" parts to see if they have
+		return err
-	// changed, and if not, skipping SetRoutes(). But if SetRoutes()
-	// is getting the non-relevant parts of Cfg, it might act on them,
-	// and this optimization is unsafe. Probably we should not pass
-	// a whole Cfg object as part of RouteSettings; instead, trim it to
-	// just what's absolutely needed (the set of actual routes).
-	rss := rs.OnlyRelevantParts()
-	if rss != e.lastRoutes {
-		e.logf("wgengine: Reconfig: reconfiguring router. la=%v dns=%v dom=%v; new routes: %v",
-			rs.LocalAddrs, rs.DNS, rs.DNSDomains, rss)
-		e.lastRoutes = rss
-		err = e.router.SetRoutes(rs)
-		if err != nil {
-			return err
-		}
 	}
 
 	e.logf("wgengine: Reconfig done")