From 9ccbcda612e9e77a30a73855cc3f3c202ba43bce Mon Sep 17 00:00:00 2001
From: David Anderson <danderson@tailscale.com>
Date: Tue, 12 May 2020 07:08:52 +0000
Subject: [PATCH] wgengine/router: rename config.Settings to config.Config,
 make pointer.

Signed-off-by: David Anderson <danderson@tailscale.com>
---
 ipn/local.go                             | 12 ++++++------
 wgengine/router/ifconfig_windows.go      | 12 ++++++------
 wgengine/router/router.go                | 21 +++++++++++++++------
 wgengine/router/router_darwin.go         | 11 +++++++----
 wgengine/router/router_darwin_support.go |  4 ++--
 wgengine/router/router_fake.go           |  2 +-
 wgengine/router/router_freebsd.go        | 15 +++++++++------
 wgengine/router/router_linux.go          | 20 ++++++++++++--------
 wgengine/router/router_openbsd.go        | 14 +++++++++-----
 wgengine/router/router_windows.go        |  8 ++++++--
 wgengine/userspace.go                    |  6 +++---
 wgengine/watchdog.go                     |  2 +-
 wgengine/wgengine.go                     |  5 +----
 13 files changed, 78 insertions(+), 54 deletions(-)

diff --git a/ipn/local.go b/ipn/local.go
index 8745b799f..c280c78e8 100644
--- a/ipn/local.go
+++ b/ipn/local.go
@@ -711,16 +711,16 @@ func (b *LocalBackend) authReconfig() {
 		log.Fatalf("WGCfg: %v", err)
 	}
 
-	err = b.e.Reconfig(cfg, routerSettings(cfg, uc, dom))
+	err = b.e.Reconfig(cfg, routerConfig(cfg, uc, dom))
 	if err == wgengine.ErrNoChanges {
 		return
 	}
 	b.logf("authReconfig: ra=%v dns=%v 0x%02x: %v", uc.RouteAll, uc.CorpDNS, uflags, err)
 }
 
-// routerSettings produces a router.Settings from a wireguard config,
+// routerConfig produces a router.Config from a wireguard config,
 // IPN prefs, and the dnsDomains pulled from control's network map.
-func routerSettings(cfg *wgcfg.Config, prefs *Prefs, dnsDomains []string) router.Settings {
+func routerConfig(cfg *wgcfg.Config, prefs *Prefs, dnsDomains []string) *router.Config {
 	var addrs []wgcfg.CIDR
 	for _, addr := range cfg.Addresses {
 		addrs = append(addrs, wgcfg.CIDR{
@@ -733,7 +733,7 @@ func routerSettings(cfg *wgcfg.Config, prefs *Prefs, dnsDomains []string) router
 		})
 	}
 
-	rs := router.Settings{
+	rs := &router.Config{
 		LocalAddrs:   wgCIDRToNetaddr(addrs),
 		DNS:          wgIPToNetaddr(cfg.DNS),
 		DNSDomains:   dnsDomains,
@@ -793,7 +793,7 @@ func (b *LocalBackend) enterState(newState State) {
 		b.blockEngineUpdates(true)
 		fallthrough
 	case Stopped:
-		err := b.e.Reconfig(&wgcfg.Config{}, router.Settings{})
+		err := b.e.Reconfig(&wgcfg.Config{}, nil)
 		if err != nil {
 			b.logf("Reconfig(down): %v", err)
 		}
@@ -869,7 +869,7 @@ func (b *LocalBackend) stateMachine() {
 
 func (b *LocalBackend) stopEngineAndWait() {
 	b.logf("stopEngineAndWait...")
-	b.e.Reconfig(&wgcfg.Config{}, router.Settings{})
+	b.e.Reconfig(&wgcfg.Config{}, nil)
 	b.requestEngineStatusAndWait()
 	b.logf("stopEngineAndWait: done.")
 }
diff --git a/wgengine/router/ifconfig_windows.go b/wgengine/router/ifconfig_windows.go
index 6ff21c6b0..588381f1a 100644
--- a/wgengine/router/ifconfig_windows.go
+++ b/wgengine/router/ifconfig_windows.go
@@ -236,7 +236,7 @@ func setFirewall(ifcGUID *windows.GUID) (bool, error) {
 	return false, nil
 }
 
-func configureInterface(rs Settings, tun *tun.NativeTun) error {
+func configureInterface(cfg *Config, tun *tun.NativeTun) error {
 	const mtu = 0
 	guid := tun.GUID()
 	log.Printf("wintun GUID is %v\n", guid)
@@ -262,13 +262,13 @@ func configureInterface(rs Settings, tun *tun.NativeTun) error {
 		}
 	}()
 
-	setDNSDomains(guid, rs.DNSDomains)
+	setDNSDomains(guid, cfg.DNSDomains)
 
 	routes := []winipcfg.RouteData{}
 	var firstGateway4 *net.IP
 	var firstGateway6 *net.IP
-	addresses := make([]*net.IPNet, len(rs.LocalAddrs))
-	for i, addr := range rs.LocalAddrs {
+	addresses := make([]*net.IPNet, len(cfg.LocalAddrs))
+	for i, addr := range cfg.LocalAddrs {
 		ipnet := addr.IPNet()
 		addresses[i] = ipnet
 		gateway := ipnet.IP
@@ -281,7 +281,7 @@ func configureInterface(rs Settings, tun *tun.NativeTun) error {
 
 	foundDefault4 := false
 	foundDefault6 := false
-	for _, route := range rs.Routes {
+	for _, route := range cfg.Routes {
 		if (route.IP.Is4() && firstGateway4 == nil) || (route.IP.Is6() && firstGateway6 == nil) {
 			return errors.New("Due to a Windows limitation, one cannot have interface routes without an interface address")
 		}
@@ -359,7 +359,7 @@ func configureInterface(rs Settings, tun *tun.NativeTun) error {
 	}
 
 	var dnsIPs []net.IP
-	for _, ip := range rs.DNS {
+	for _, ip := range cfg.DNS {
 		dnsIPs = append(dnsIPs, ip.IPAddr().IP)
 	}
 	err = iface.SetDNS(dnsIPs)
diff --git a/wgengine/router/router.go b/wgengine/router/router.go
index 8393482ec..ed989fe08 100644
--- a/wgengine/router/router.go
+++ b/wgengine/router/router.go
@@ -20,10 +20,10 @@ type Router interface {
 	// Up brings the router up.
 	Up() error
 
-	// Set updates the OS network stack with new settings. It may be
-	// called multiple times with identical Settings, which the
+	// Set updates the OS network stack with a new Config. It may be
+	// called multiple times with identical Configs, which the
 	// implementation should handle gracefully.
-	Set(Settings) error
+	Set(*Config) error
 
 	// Close closes the router.
 	Close() error
@@ -35,9 +35,9 @@ func New(logf logger.Logf, wgdev *device.Device, tundev tun.Device) (Router, err
 	return newUserspaceRouter(logf, wgdev, tundev)
 }
 
-// Settings is the subset of Tailscale configuration that is relevant
-// to the OS's network stack.
-type Settings struct {
+// Config is the subset of Tailscale configuration that is relevant to
+// the OS's network stack.
+type Config struct {
 	LocalAddrs   []netaddr.IPPrefix
 	DNS          []netaddr.IP
 	DNSDomains   []string
@@ -45,3 +45,12 @@ type Settings struct {
 	SubnetRoutes []netaddr.IPPrefix // subnets being advertised to other Tailscale nodes
 	NoSNAT       bool               // don't SNAT traffic to local subnets
 }
+
+// shutdownConfig is a routing configuration that removes all router
+// state from the OS. It's the config used when callers pass in a nil
+// Config.
+var shutdownConfig = Config{
+	// TODO(danderson): set more things in here to disable all
+	// firewall rules and routing overrides when nil.
+	NoSNAT: true,
+}
diff --git a/wgengine/router/router_darwin.go b/wgengine/router/router_darwin.go
index c28adb6f0..ac937ad7f 100644
--- a/wgengine/router/router_darwin.go
+++ b/wgengine/router/router_darwin.go
@@ -26,11 +26,14 @@ func (r *darwinRouter) Up() error {
 	return nil
 }
 
-func (r *darwinRouter) Set(rs Settings) error {
-	if SetRoutesFunc != nil {
-		return SetRoutesFunc(rs)
+func (r *darwinRouter) Set(cfg *Config) error {
+	if SetRoutesFunc == nil {
+		return nil
 	}
-	return nil
+	if cfg == nil {
+		cfg = &shutdownConfig
+	}
+	return SetRoutesFunc(cfg)
 }
 
 func (r *darwinRouter) Close() error {
diff --git a/wgengine/router/router_darwin_support.go b/wgengine/router/router_darwin_support.go
index 364f66650..9a3e0c864 100644
--- a/wgengine/router/router_darwin_support.go
+++ b/wgengine/router/router_darwin_support.go
@@ -7,7 +7,7 @@
 package router
 
 // SetRoutesFunc applies the given router settings to the OS network
-// stack.
+// stack. cfg is guaranteed to be non-nil.
 //
 // This is logically part of the router_darwin.go implementation, and
 // should not be used on other platforms.
@@ -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 Settings) error
+var SetRoutesFunc func(cfg *Config) error
diff --git a/wgengine/router/router_fake.go b/wgengine/router/router_fake.go
index b60cc9428..9c1543d6b 100644
--- a/wgengine/router/router_fake.go
+++ b/wgengine/router/router_fake.go
@@ -25,7 +25,7 @@ func (r fakeRouter) Up() error {
 	return nil
 }
 
-func (r fakeRouter) Set(rs Settings) error {
+func (r fakeRouter) Set(cfg *Config) error {
 	r.logf("Warning: fakeRouter.Set: not implemented.")
 	return nil
 }
diff --git a/wgengine/router/router_freebsd.go b/wgengine/router/router_freebsd.go
index e16c9f082..86fd04a42 100644
--- a/wgengine/router/router_freebsd.go
+++ b/wgengine/router/router_freebsd.go
@@ -55,15 +55,18 @@ func (r *freebsdRouter) Up() error {
 	return nil
 }
 
-func (r *freebsdRouter) Set(rs Settings) error {
-	if len(rs.LocalAddrs) == 0 {
+func (r *freebsdRouter) Set(cfg *Config) error {
+	if cfg == nil {
+		cfg = &shutdownConfig
+	}
+	if len(cfg.LocalAddrs) == 0 {
 		return nil
 	}
 	// TODO: support configuring multiple local addrs on interface.
-	if len(rs.LocalAddrs) != 1 {
+	if len(cfg.LocalAddrs) != 1 {
 		return errors.New("freebsd doesn't support setting multiple local addrs yet")
 	}
-	localAddr := rs.LocalAddrs[0]
+	localAddr := cfg.LocalAddrs[0]
 
 	var errq error
 
@@ -95,7 +98,7 @@ func (r *freebsdRouter) Set(rs Settings) error {
 	}
 
 	newRoutes := make(map[netaddr.IPPrefix]struct{})
-	for _, route := range rs.Routes {
+	for _, route := range cfg.Routes {
 		newRoutes[route] = struct{}{}
 	}
 	// Delete any pre-existing routes.
@@ -139,7 +142,7 @@ func (r *freebsdRouter) Set(rs Settings) error {
 	r.local = localAddr
 	r.routes = newRoutes
 
-	if err := r.replaceResolvConf(rs.DNS, rs.DNSDomains); err != nil {
+	if err := r.replaceResolvConf(cfg.DNS, cfg.DNSDomains); err != nil {
 		errq = fmt.Errorf("replacing resolv.conf failed: %v", err)
 	}
 
diff --git a/wgengine/router/router_linux.go b/wgengine/router/router_linux.go
index 1b07118fc..99b5bd934 100644
--- a/wgengine/router/router_linux.go
+++ b/wgengine/router/router_linux.go
@@ -141,7 +141,11 @@ func (r *linuxRouter) Close() error {
 }
 
 // Set implements the Router interface.
-func (r *linuxRouter) Set(rs Settings) error {
+func (r *linuxRouter) Set(cfg *Config) error {
+	if cfg == nil {
+		cfg = &shutdownConfig
+	}
+
 	// 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.
@@ -182,23 +186,23 @@ func (r *linuxRouter) Set(rs Settings) error {
 
 	var errq error
 
-	newAddrs, err := cidrDiff("addr", r.addrs, rs.LocalAddrs, r.addAddress, r.delAddress)
+	newAddrs, err := cidrDiff("addr", r.addrs, cfg.LocalAddrs, r.addAddress, r.delAddress)
 	if err != nil && errq == nil {
 		errq = err
 	}
-	newRoutes, err := cidrDiff("route", r.routes, rs.Routes, r.addRoute, r.delRoute)
+	newRoutes, err := cidrDiff("route", r.routes, cfg.Routes, r.addRoute, r.delRoute)
 	if err != nil && errq == nil {
 		errq = err
 	}
-	newSubnetRoutes, err := cidrDiff("subnet rule", r.subnetRoutes, rs.SubnetRoutes, r.addSubnetRule, r.delSubnetRule)
+	newSubnetRoutes, err := cidrDiff("subnet rule", r.subnetRoutes, cfg.SubnetRoutes, r.addSubnetRule, r.delSubnetRule)
 	if err != nil && errq == nil {
 		errq = err
 	}
 
 	switch {
-	case rs.NoSNAT == r.noSNAT:
+	case cfg.NoSNAT == r.noSNAT:
 		// state already correct, nothing to do.
-	case rs.NoSNAT:
+	case cfg.NoSNAT:
 		if err := r.delSNATRule(); err != nil && errq == nil {
 			errq = err
 		}
@@ -211,11 +215,11 @@ func (r *linuxRouter) Set(rs Settings) error {
 	r.addrs = newAddrs
 	r.routes = newRoutes
 	r.subnetRoutes = newSubnetRoutes
-	r.noSNAT = rs.NoSNAT
+	r.noSNAT = cfg.NoSNAT
 
 	// TODO: this:
 	if false {
-		if err := r.replaceResolvConf(rs.DNS, rs.DNSDomains); err != nil {
+		if err := r.replaceResolvConf(cfg.DNS, cfg.DNSDomains); err != nil {
 			errq = fmt.Errorf("replacing resolv.conf failed: %v", err)
 		}
 	}
diff --git a/wgengine/router/router_openbsd.go b/wgengine/router/router_openbsd.go
index 41e414b8d..dab71a5a8 100644
--- a/wgengine/router/router_openbsd.go
+++ b/wgengine/router/router_openbsd.go
@@ -60,12 +60,16 @@ func (r *openbsdRouter) Up() error {
 	return nil
 }
 
-func (r *openbsdRouter) Set(rs Settings) error {
+func (r *openbsdRouter) Set(cfg *Config) error {
+	if cfg == nil {
+		cfg = &shutdownConfig
+	}
+
 	// TODO: support configuring multiple local addrs on interface.
-	if len(rs.LocalAddrs) != 1 {
+	if len(cfg.LocalAddrs) != 1 {
 		return errors.New("freebsd doesn't support setting multiple local addrs yet")
 	}
-	localAddr := rs.LocalAddrs[0]
+	localAddr := cfg.LocalAddrs[0]
 
 	var errq error
 
@@ -114,7 +118,7 @@ func (r *openbsdRouter) Set(rs Settings) error {
 	}
 
 	newRoutes := make(map[netaddr.IPPrefix]struct{})
-	for _, route := range rs.Routes {
+	for _, route := range cfg.Routes {
 		newRoutes[route] = struct{}{}
 	}
 	for route := range r.routes {
@@ -155,7 +159,7 @@ func (r *openbsdRouter) Set(rs Settings) error {
 	r.local = localAddr
 	r.routes = newRoutes
 
-	if err := r.replaceResolvConf(rs.DNS, rs.DNSDomains); err != nil {
+	if err := r.replaceResolvConf(cfg.DNS, cfg.DNSDomains); err != nil {
 		errq = fmt.Errorf("replacing resolv.conf failed: %v", err)
 	}
 
diff --git a/wgengine/router/router_windows.go b/wgengine/router/router_windows.go
index 6e9d9c853..a0e48cd9c 100644
--- a/wgengine/router/router_windows.go
+++ b/wgengine/router/router_windows.go
@@ -45,8 +45,12 @@ func (r *winRouter) Up() error {
 	return nil
 }
 
-func (r *winRouter) Set(rs Settings) error {
-	err := configureInterface(rs, r.nativeTun)
+func (r *winRouter) Set(cfg *Config) error {
+	if cfg == nil {
+		cfg = &shutdownConfig
+	}
+
+	err := configureInterface(cfg, r.nativeTun)
 	if err != nil {
 		r.logf("ConfigureInterface: %v\n", err)
 		return err
diff --git a/wgengine/userspace.go b/wgengine/userspace.go
index ce7090ba3..d803f5a8b 100644
--- a/wgengine/userspace.go
+++ b/wgengine/userspace.go
@@ -243,7 +243,7 @@ func newUserspaceEngineAdvanced(logf logger.Logf, tundev tun.Device, routerGen R
 	}
 	// TODO(danderson): we should delete this. It's pointless to apply
 	// a no-op settings here.
-	if err := e.router.Set(router.Settings{}); err != nil {
+	if err := e.router.Set(nil); err != nil {
 		e.wgdev.Close()
 		return nil, err
 	}
@@ -326,7 +326,7 @@ func (e *userspaceEngine) pinger(peerKey wgcfg.Key, ips []wgcfg.IP) {
 	}
 }
 
-func configSignature(cfg *wgcfg.Config, routerCfg router.Settings) (string, error) {
+func configSignature(cfg *wgcfg.Config, routerCfg *router.Config) (string, error) {
 	// TODO(apenwarr): get rid of uapi stuff for in-process comms
 	uapi, err := cfg.ToUAPI()
 	if err != nil {
@@ -336,7 +336,7 @@ func configSignature(cfg *wgcfg.Config, routerCfg router.Settings) (string, erro
 	return fmt.Sprintf("%s %v", uapi, routerCfg), nil
 }
 
-func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg router.Settings) error {
+func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config) error {
 	e.wgLock.Lock()
 	defer e.wgLock.Unlock()
 
diff --git a/wgengine/watchdog.go b/wgengine/watchdog.go
index 2c60839ab..9d409ece7 100644
--- a/wgengine/watchdog.go
+++ b/wgengine/watchdog.go
@@ -62,7 +62,7 @@ func (e *watchdogEngine) watchdog(name string, fn func()) {
 	})
 }
 
-func (e *watchdogEngine) Reconfig(cfg *wgcfg.Config, routerCfg router.Settings) error {
+func (e *watchdogEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config) error {
 	return e.watchdogErr("Reconfig", func() error { return e.wrap.Reconfig(cfg, routerCfg) })
 }
 func (e *watchdogEngine) GetFilter() *filter.Filter {
diff --git a/wgengine/wgengine.go b/wgengine/wgengine.go
index a6d194fad..3a229c713 100644
--- a/wgengine/wgengine.go
+++ b/wgengine/wgengine.go
@@ -53,14 +53,11 @@ type Engine interface {
 	// Reconfig reconfigures WireGuard and makes sure it's running.
 	// This also handles setting up any kernel routes.
 	//
-	// The provided DNS domains are not part of wgcfg.Config, as
-	// WireGuard itself doesn't care about such things.
-	//
 	// This is called whenever the tailcontrol (control plane)
 	// sends an updated network map.
 	//
 	// The returned error is ErrNoChanges if no changes were made.
-	Reconfig(cfg *wgcfg.Config, routerCfg router.Settings) error
+	Reconfig(*wgcfg.Config, *router.Config) error
 
 	// GetFilter returns the current packet filter, if any.
 	GetFilter() *filter.Filter