wgengine: simplify, change some signatures

* make RouterGen return an error, not take both tunname and tundev
* also remove RouteGen taking a wireguard/device.Device; currently unused
* remove derp parameter (it'll work differently)
* unexport NewUserspaceRouter in per-OS impls, add documented wrapper

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

cmd/relaynode/relaynode.go

@@ -53,7 +53,6 @@ func main() {
 	droutes := getopt.BoolLong("default-routes", 'D', "allow default route on remote node")
 	routes := getopt.StringLong("routes", 0, "", "list of IP ranges this node can relay")
 	aclfile := getopt.StringLong("acl-file", 0, "", "restrict traffic relaying according to json ACL file")
-	derp := getopt.BoolLong("derp", 0, "enable bypass via Detour Encrypted Routing Protocol (DERP)", "false")
 	debug := getopt.StringLong("debug", 0, "", "Address of debug server")
 	getopt.Parse()
 	if len(getopt.Args()) > 0 {
@@ -75,9 +74,9 @@ func main() {
 	// controlclient, and runs the actual tunnels and packets.
 	var e wgengine.Engine
 	if *fake {
-		e, err = wgengine.NewFakeUserspaceEngine(logf, *listenport, *derp)
+		e, err = wgengine.NewFakeUserspaceEngine(logf, *listenport)
 	} else {
-		e, err = wgengine.NewUserspaceEngine(logf, *tunname, *listenport, *derp)
+		e, err = wgengine.NewUserspaceEngine(logf, *tunname, *listenport)
 	}
 	if err != nil {
 		log.Fatalf("Error starting wireguard engine: %v\n", err)

cmd/tailscaled/tailscaled.go

@@ -49,9 +49,9 @@ func main() {
 
 	var e wgengine.Engine
 	if *fake {
-		e, err = wgengine.NewFakeUserspaceEngine(logf, 0, false)
+		e, err = wgengine.NewFakeUserspaceEngine(logf, 0)
 	} else {
-		e, err = wgengine.NewUserspaceEngine(logf, *tunname, *listenport, false)
+		e, err = wgengine.NewUserspaceEngine(logf, *tunname, *listenport)
 	}
 	if err != nil {
 		log.Fatalf("wgengine.New: %v\n", err)

wgengine/monitor/monitor.go

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
+// +build linux freebsd
+
 // Package monitor provides facilities for monitoring network
 // interface changes.
 package monitor

wgengine/router_bsd.go

@@ -34,12 +34,15 @@ type bsdRouter struct {
 	routes  map[wgcfg.CIDR]struct{}
 }
 
-func NewUserspaceRouter(logf logger.Logf, tunname string, _ *device.Device, tuntap tun.Device, _ func()) Router {
+func newUserspaceRouter(logf logger.Logf, _ *device.Device, tundev tun.Device, _ func()) (Router, error) {
-	r := bsdRouter{
+	tunname, err := tundev.Name()
+	if err != nil {
+		return nil, err
+	}
+	return &bsdRouter{
 		logf:    logf,
 		tunname: tunname,
-	}
+	}, nil
-	return &r
 }
 
 // TODO(mbaillie): extract as identical to linux version

wgengine/router_darwin.go

@@ -14,11 +14,12 @@ type darwinRouter struct {
 	tunname string
 }
 
-func NewUserspaceRouter(logf logger.Logf, tunname string, dev *device.Device, tuntap tun.Device, netChanged func()) Router {
+func newUserspaceRouter(logf logger.Logf, _ *device.Device, tundev tun.Device, netChanged func()) (Router, error) {
-	r := darwinRouter{
+	tunname, err := tundev.Name()
-		tunname: tunname,
+	if err != nil {
+		return nil, err
 	}
-	return &r
+	return &darwinRouter{tunname: tunname}, nil
 }
 
 func (r *darwinRouter) Up() error {

wgengine/router_default.go

@@ -12,6 +12,6 @@ import (
 	"tailscale.com/types/logger"
 )
 
-func NewUserspaceRouter(logf logger.Logf, tunname string, dev *device.Device, tuntap tun.Device, netChanged func()) Router {
+func newUserspaceRouter(logf logger.Logf, tunname string, dev *device.Device, tuntap tun.Device, netChanged func()) Router {
 	return NewFakeRouter(logf, tunname, dev, tuntap, netChanged)
 }

wgengine/router_fake.go

@@ -10,29 +10,27 @@ import (
 	"tailscale.com/types/logger"
 )
 
-type fakeRouter struct {
+// NewFakeRouter returns a new fake Router implementation whose
-	tunname string
+// implementation does nothing and always returns nil errors.
-	logf    logger.Logf
+func NewFakeRouter(logf logger.Logf, _ *device.Device, _ tun.Device, netChanged func()) (Router, error) {
+	return fakeRouter{logf: logf}, nil
 }
 
-func NewFakeRouter(logf logger.Logf, tunname string, dev *device.Device, tuntap tun.Device, netChanged func()) Router {
+type fakeRouter struct {
-	return &fakeRouter{
+	logf logger.Logf
-		logf:    logf,
-		tunname: tunname,
-	}
 }
 
-func (r *fakeRouter) Up() error {
+func (r fakeRouter) Up() error {
 	r.logf("Warning: fakeRouter.Up: not implemented.\n")
 	return nil
 }
 
-func (r *fakeRouter) SetRoutes(rs RouteSettings) error {
+func (r fakeRouter) SetRoutes(rs RouteSettings) error {
 	r.logf("Warning: fakeRouter.SetRoutes: not implemented.\n")
 	return nil
 }
 
-func (r *fakeRouter) Close() error {
+func (r fakeRouter) Close() error {
 	r.logf("Warning: fakeRouter.Close: not implemented.\n")
 	return nil
 }

wgengine/router_linux.go

@@ -32,19 +32,24 @@ type linuxRouter struct {
 	routes     map[wgcfg.CIDR]struct{}
 }
 
-func NewUserspaceRouter(logf logger.Logf, tunname string, dev *device.Device, tuntap tun.Device, netChanged func()) Router {
+func newUserspaceRouter(logf logger.Logf, _ *device.Device, tunDev tun.Device, netChanged func()) (Router, error) {
+	// TODO: move monitor out of Router, make it created/owned by Engine
 	mon, err := monitor.New(logf, netChanged)
 	if err != nil {
-		log.Fatalf("rtnlmon.New() failed: %v", err)
+		return nil, err
 	}
 
-	r := linuxRouter{
+	tunname, err := tunDev.Name()
+	if err != nil {
+		return nil, err
+	}
+
+	return &linuxRouter{
 		logf:       logf,
 		tunname:    tunname,
 		mon:        mon,
 		netChanged: netChanged,
-	}
+	}, nil
-	return &r
 }
 
 func cmd(args ...string) *exec.Cmd {

wgengine/router_windows.go

@@ -16,26 +16,29 @@ import (
 type winRouter struct {
 	logf                func(fmt string, args ...interface{})
 	tunname             string
-	dev                 *device.Device
 	nativeTun           *tun.NativeTun
+	wgdev               *device.Device
 	routeChangeCallback *winipcfg.RouteChangeCallback
 }
 
-func NewUserspaceRouter(logf logger.Logf, tunname string, dev *device.Device, tuntap tun.Device, netChanged func()) Router {
+func newUserspaceRouter(logf logger.Logf, wgdev *device.Device, tundev tun.Device, netChanged func()) (Router, error) {
-	r := winRouter{
+	tunname, err := tundev.Name()
+	if err != nil {
+		return nil, err
+	}
+	return &winRouter{
 		logf:      logf,
+		wgdev:     wgdev,
 		tunname:   tunname,
-		dev:       dev,
+		nativeTun: tundev.(*tun.NativeTun),
-		nativeTun: tuntap.(*tun.NativeTun),
+	}, nil
-	}
-	return &r
 }
 
 func (r *winRouter) Up() error {
 	// MonitorDefaultRoutes handles making sure our wireguard UDP
 	// traffic goes through the old route, not recursively through the VPN.
 	var err error
-	r.routeChangeCallback, err = MonitorDefaultRoutes(r.dev, true, r.nativeTun)
+	r.routeChangeCallback, err = MonitorDefaultRoutes(r.wgdev, true, r.nativeTun)
 	if err != nil {
 		log.Fatalf("MonitorDefaultRoutes: %v\n", err)
 	}

wgengine/userspace.go

@@ -28,7 +28,7 @@ type userspaceEngine struct {
 	statusCallback StatusCallback
 	reqCh          chan struct{}
 	waitCh         chan struct{}
-	tuntap         tun.Device
+	tundev         tun.Device
 	wgdev          *device.Device
 	router         Router
 	magicConn      *magicsock.Conn
@@ -51,13 +51,15 @@ func (l *Loggify) Write(b []byte) (int, error) {
 	return len(b), nil
 }
 
-func NewFakeUserspaceEngine(logf logger.Logf, listenPort uint16, derp bool) (Engine, error) {
+func NewFakeUserspaceEngine(logf logger.Logf, listenPort uint16) (Engine, error) {
 	logf("Starting userspace wireguard engine (FAKE tuntap device).")
 	tun := NewFakeTun()
-	return NewUserspaceEngineAdvanced(logf, tun, NewFakeRouter, listenPort, derp)
+	return NewUserspaceEngineAdvanced(logf, tun, NewFakeRouter, listenPort)
 }
 
-func NewUserspaceEngine(logf logger.Logf, tunname string, listenPort uint16, derp bool) (Engine, error) {
+// NewUserspaceEngine creates the named tun device and returns a Tailscale Engine
+// running on it.
+func NewUserspaceEngine(logf logger.Logf, tunname string, listenPort uint16) (Engine, error) {
 	logf("Starting userspace wireguard engine.")
 	logf("external packet routing via --tun=%s enabled", tunname)
 
@@ -65,34 +67,34 @@ func NewUserspaceEngine(logf logger.Logf, tunname string, listenPort uint16, der
 		return nil, fmt.Errorf("--tun name must not be blank")
 	}
 
-	tuntap, err := tun.CreateTUN(tunname, device.DefaultMTU)
+	tundev, err := tun.CreateTUN(tunname, device.DefaultMTU)
 	if err != nil {
 		logf("CreateTUN: %v\n", err)
 		return nil, err
 	}
 	logf("CreateTUN ok.\n")
 
-	e, err := NewUserspaceEngineAdvanced(logf, tuntap, NewUserspaceRouter, listenPort, derp)
+	e, err := NewUserspaceEngineAdvanced(logf, tundev, newUserspaceRouter, listenPort)
 	if err != nil {
 		logf("NewUserspaceEngineAdv: %v\n", err)
+		tundev.Close()
 		return nil, err
 	}
 	return e, err
 }
 
-type RouterGen func(logf logger.Logf, tunname string, dev *device.Device, tuntap tun.Device, netStateChanged func()) Router
+// NewUserspaceEngineAdvanced is like NewUserspaceEngine but takes a pre-created TUN device and allows specifing
+// a custom router constructor and listening port.
+func NewUserspaceEngineAdvanced(logf logger.Logf, tundev tun.Device, routerGen RouterGen, listenPort uint16) (Engine, error) {
+	return newUserspaceEngineAdvanced(logf, tundev, routerGen, listenPort)
+}
 
-func NewUserspaceEngineAdvanced(logf logger.Logf, tuntap tun.Device, routerGen RouterGen, listenPort uint16, derp bool) (Engine, error) {
+func newUserspaceEngineAdvanced(logf logger.Logf, tundev tun.Device, routerGen RouterGen, listenPort uint16) (_ Engine, reterr error) {
 	e := &userspaceEngine{
 		logf:   logf,
 		reqCh:  make(chan struct{}, 1),
 		waitCh: make(chan struct{}),
-		tuntap: tuntap,
+		tundev: tundev,
-	}
-
-	tunname, err := tuntap.Name()
-	if err != nil {
-		return nil, err
 	}
 
 	endpointsFn := func(endpoints []string) {
@@ -111,9 +113,7 @@ func NewUserspaceEngineAdvanced(logf logger.Logf, tuntap tun.Device, routerGen R
 		// TODO(crawshaw): DERP: magicsock.DefaultDERP,
 		EndpointsFunc: endpointsFn,
 	}
-	if derp {
+	var err error
-		magicsockOpts.DERP = magicsock.DefaultDERP
-	}
 	e.magicConn, err = magicsock.Listen(magicsockOpts)
 	if err != nil {
 		return nil, fmt.Errorf("wgengine: %v", err)
@@ -155,13 +155,23 @@ func NewUserspaceEngineAdvanced(logf logger.Logf, tuntap tun.Device, routerGen R
 		SkipBindUpdate: true,
 	}
 
-	e.wgdev = device.NewDevice(e.tuntap, opts)
+	e.wgdev = device.NewDevice(e.tundev, opts)
+	defer func() {
+		if reterr != nil {
+			e.wgdev.Close()
+		}
+	}()
+
+	e.router, err = routerGen(logf, e.wgdev, e.tundev, func() { e.LinkChange(false) })
+	if err != nil {
+		return nil, err
+	}
 
 	go func() {
 		up := false
-		for event := range e.tuntap.Events() {
+		for event := range e.tundev.Events() {
 			if event&tun.EventMTUUpdate != 0 {
-				mtu, err := e.tuntap.MTU()
+				mtu, err := e.tundev.MTU()
 				e.logf("external route MTU: %d (%v)", mtu, err)
 			}
 			if event&tun.EventUp != 0 && !up {
@@ -177,7 +187,6 @@ func NewUserspaceEngineAdvanced(logf logger.Logf, tuntap tun.Device, routerGen R
 		}
 	}()
 
-	e.router = routerGen(logf, tunname, e.wgdev, e.tuntap, func() { e.LinkChange(false) })
 	e.wgdev.Up()
 	if err := e.router.Up(); err != nil {
 		e.wgdev.Close()
@@ -270,7 +279,7 @@ func (e *userspaceEngine) SetFilter(filt *filter.Filter) {
 	if filt == nil {
 		e.logf("wgengine: nil filter provided; no access restrictions.\n")
 	} else {
-		ft, ft_ok := e.tuntap.(*fakeTun)
+		ft, ft_ok := e.tundev.(*fakeTun)
 		filtin = func(b []byte) device.FilterResult {
 			runf := filter.LogDrops
 			//runf |= filter.HexdumpDrops

wgengine/watchdog_test.go

@@ -18,7 +18,7 @@ func TestWatchdog(t *testing.T) {
 	t.Run("default watchdog does not fire", func(t *testing.T) {
 		t.Parallel()
 		tun := NewFakeTun()
-		e, err := NewUserspaceEngineAdvanced(t.Logf, tun, NewFakeRouter, 0, false)
+		e, err := NewUserspaceEngineAdvanced(t.Logf, tun, NewFakeRouter, 0)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -35,7 +35,7 @@ func TestWatchdog(t *testing.T) {
 	t.Run("watchdog fires on blocked getStatus", func(t *testing.T) {
 		t.Parallel()
 		tun := NewFakeTun()
-		e, err := NewUserspaceEngineAdvanced(t.Logf, tun, NewFakeRouter, 0, false)
+		e, err := NewUserspaceEngineAdvanced(t.Logf, tun, NewFakeRouter, 0)
 		if err != nil {
 			t.Fatal(err)
 		}

wgengine/wgengine.go

@@ -9,8 +9,11 @@ import (
 	"net"
 	"time"
 
+	"github.com/tailscale/wireguard-go/device"
+	"github.com/tailscale/wireguard-go/tun"
 	"github.com/tailscale/wireguard-go/wgcfg"
 	"tailscale.com/tailcfg"
+	"tailscale.com/types/logger"
 	"tailscale.com/wgengine/filter"
 )
 
@@ -58,6 +61,15 @@ func (rs *RouteSettings) OnlyRelevantParts() string {
 		rs.LocalAddr, rs.DNS, rs.DNSDomains, peers)
 }
 
+// NewUserspaceRouter returns a new Router for the current platform, using the provided tun device.
+func NewUserspaceRouter(logf logger.Logf, wgdev *device.Device, tundev tun.Device, netChanged func()) (Router, error) {
+	return newUserspaceRouter(logf, wgdev, tundev, netChanged)
+}
+
+// RouterGen is the signature for the two funcs that create Router implementations:
+// NewUserspaceRouter (which varies by operating system) and NewFakeRouter.
+type RouterGen func(logf logger.Logf, wgdev *device.Device, tundev tun.Device, netStateChanged func()) (Router, error)
+
 // Router is responsible for managing the system route table.
 //
 // There's only one instance, and one per-OS implementation.