wgengine: unify NewUserspaceEngine, NewUserspaceEngineAdvanced

Also rename EngineConfig to Config to avoid wgengine.EngineConfig
stutter.

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

Makefile

@@ -12,7 +12,10 @@ depaware:
 	go run github.com/tailscale/depaware --check tailscale.com/cmd/tailscaled
 	go run github.com/tailscale/depaware --check tailscale.com/cmd/tailscale
 
-check: staticcheck vet depaware
+buildwindows:
+	GOOS=windows GOARCH=amd64 go install tailscale.com/cmd/tailscale tailscale.com/cmd/tailscaled
+
+check: staticcheck vet depaware buildwindows
 
 staticcheck:
 	go run honnef.co/go/tools/cmd/staticcheck -- $$(go list ./... | grep -v tempfork)

cmd/tailscaled/tailscaled.go

@@ -198,7 +198,10 @@ func run() error {
 		}
 		e, err = wgengine.NewFakeUserspaceEngine(logf, 0, impl)
 	} else {
-		e, err = wgengine.NewUserspaceEngine(logf, args.tunname, args.port)
+		e, err = wgengine.NewUserspaceEngine(logf, wgengine.Config{
+			TUNName:    args.tunname,
+			ListenPort: args.port,
+		})
 	}
 	if err != nil {
 		logf("wgengine.New: %v", err)

cmd/tailscaled/tailscaled_windows.go

@@ -114,7 +114,10 @@ func startIPNServer(ctx context.Context, logid string) error {
 	var err error
 
 	getEngine := func() (wgengine.Engine, error) {
-		eng, err := wgengine.NewUserspaceEngine(logf, "Tailscale", 41641)
+		eng, err := wgengine.NewUserspaceEngine(logf, wgengine.Config{
+			TUNName:    "Tailscale",
+			ListenPort: 41641,
+		})
 		if err != nil {
 			return nil, err
 		}

wgengine/userspace.go

@@ -96,6 +96,7 @@ type userspaceEngine struct {
 	resolver          *tsdns.Resolver
 	magicConn         *magicsock.Conn
 	linkMon           *monitor.Mon
+	linkMonUnregister func() // unsubscribes from changes; used regardless of linkMonOwned
 
 	testMaybeReconfigHook func() // for tests; if non-nil, fires if maybeReconfigWireguardLocked called
 
@@ -133,15 +134,24 @@ type userspaceEngine struct {
 // router.Router.
 type RouterGen func(logf logger.Logf, wgdev *device.Device, tundev tun.Device) (router.Router, error)
 
-type EngineConfig struct {
-	// Logf is the logging function used by the engine.
-	Logf logger.Logf
-	// TUN is the tun device used by the engine.
+// Config is the engine configuration.
+type Config struct {
+	// TUN is the TUN device used by the engine.
+	// Exactly one of either TUN or TUNName must be specified.
 	TUN tun.Device
+
+	// TUNName is the TUN device to create.
+	// Exactly one of either TUN or TUNName must be specified.
+	TUNName string
+
 	// RouterGen is the function used to instantiate the router.
+	// If nil, wgengine/router.New is used.
 	RouterGen RouterGen
+
 	// ListenPort is the port on which the engine will listen.
+	// If zero, a port is automatically selected.
 	ListenPort uint16
+
 	// Fake determines whether this engine is running in fake mode,
 	// which disables such features as DNS configuration and unrestricted ICMP Echo responses.
 	Fake bool
@@ -156,7 +166,7 @@ type EngineConfig struct {
 }
 
 // FakeImpl is a fake or alternate version of Engine that can be started. See
-// EngineConfig.FakeImplFactory for details.
+// Config.FakeImplFactory for details.
 type FakeImpl interface {
 	Start() error
 }
@@ -166,27 +176,29 @@ type FakeImplFactory func(logger.Logf, *tstun.TUN, Engine, *magicsock.Conn) (Fak
 
 func NewFakeUserspaceEngine(logf logger.Logf, listenPort uint16, impl FakeImplFactory) (Engine, error) {
 	logf("Starting userspace wireguard engine (with fake TUN device)")
-	conf := EngineConfig{
-		Logf:            logf,
+	return NewUserspaceEngine(logf, Config{
 		TUN:             tstun.NewFakeTUN(),
 		RouterGen:       router.NewFake,
 		ListenPort:      listenPort,
 		Fake:            true,
 		FakeImplFactory: impl,
-	}
-	return NewUserspaceEngineAdvanced(conf)
+	})
 }
 
 // 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) {
-	if tunName == "" {
-		return nil, fmt.Errorf("--tun name must not be blank")
+func NewUserspaceEngine(logf logger.Logf, conf Config) (Engine, error) {
+	if conf.TUN != nil && conf.TUNName != "" {
+		return nil, errors.New("TUN and TUNName are mutually exclusive")
 	}
-
+	if conf.TUN == nil && conf.TUNName == "" {
+		return nil, errors.New("either TUN or TUNName are required")
+	}
+	tunDev := conf.TUN
+	var err error
+	if tunName := conf.TUNName; tunName != "" {
 		logf("Starting userspace wireguard engine with tun device %q", tunName)
-
-	tun, err := tun.CreateTUN(tunName, minimalMTU)
+		tunDev, err = tun.CreateTUN(tunName, minimalMTU)
 		if err != nil {
 			diagnoseTUNFailure(tunName, logf)
 			logf("CreateTUN: %v", err)
@@ -194,50 +206,24 @@ func NewUserspaceEngine(logf logger.Logf, tunName string, listenPort uint16) (En
 		}
 		logf("CreateTUN ok.")
 
-	if err := waitInterfaceUp(tun, 90*time.Second, logf); err != nil {
+		if err := waitInterfaceUp(tunDev, 90*time.Second, logf); err != nil {
 			return nil, err
 		}
-
-	conf := EngineConfig{
-		Logf:       logf,
-		TUN:        tun,
-		RouterGen:  router.New,
-		ListenPort: listenPort,
 	}
 
-	e, err := NewUserspaceEngineAdvanced(conf)
-	if err != nil {
-		return nil, err
-	}
-	return e, err
+	if conf.RouterGen == nil {
+		conf.RouterGen = router.New
 	}
 
-type closeOnErrorPool []func()
-
-func (p *closeOnErrorPool) add(c io.Closer)   { *p = append(*p, func() { c.Close() }) }
-func (p *closeOnErrorPool) addFunc(fn func()) { *p = append(*p, fn) }
-func (p closeOnErrorPool) closeAllIfError(errp *error) {
-	if *errp != nil {
-		for _, closeFn := range p {
-			closeFn()
-		}
-	}
-}
-
-// NewUserspaceEngineAdvanced is like NewUserspaceEngine
-// but provides control over all config fields.
-func NewUserspaceEngineAdvanced(conf EngineConfig) (Engine, error) {
-	return newUserspaceEngineAdvanced(conf)
+	return newUserspaceEngine(logf, tunDev, conf)
 }
 
-func newUserspaceEngineAdvanced(conf EngineConfig) (_ Engine, reterr error) {
-	logf := conf.Logf
-
+func newUserspaceEngine(logf logger.Logf, rawTUNDev tun.Device, conf Config) (_ Engine, reterr error) {
 	var closePool closeOnErrorPool
 	defer closePool.closeAllIfError(&reterr)
 
-	tunDev := tstun.WrapTUN(logf, conf.TUN)
-	closePool.add(tunDev)
+	tsTUNDev := tstun.WrapTUN(logf, rawTUNDev)
+	closePool.add(tsTUNDev)
 
 	rconf := tsdns.ResolverConfig{
 		Logf:    logf,
@@ -248,7 +234,7 @@ func newUserspaceEngineAdvanced(conf EngineConfig) (_ Engine, reterr error) {
 		logf:     logf,
 		reqCh:    make(chan struct{}, 1),
 		waitCh:   make(chan struct{}),
-		tundev:   tunDev,
+		tundev:   tsTUNDev,
 		resolver: tsdns.NewResolver(rconf),
 		pingers:  make(map[wgkey.Key]*pinger),
 	}
@@ -261,12 +247,13 @@ func newUserspaceEngineAdvanced(conf EngineConfig) (_ Engine, reterr error) {
 		return nil, err
 	}
 	closePool.add(mon)
-	unregisterMonWatch := mon.RegisterChangeCallback(func() {
+	e.linkMon = mon
+	unregisterMonWatch := e.linkMon.RegisterChangeCallback(func() {
 		e.LinkChange(false)
 		tshttpproxy.InvalidateCache()
 	})
 	closePool.addFunc(unregisterMonWatch)
-	e.linkMon = mon
+	e.linkMonUnregister = unregisterMonWatch
 
 	endpointsFn := func(endpoints []string) {
 		e.mu.Lock()
@@ -1249,6 +1236,7 @@ func (e *userspaceEngine) Close() {
 	e.wgdev.IpcSetOperation(r)
 	e.resolver.Close()
 	e.magicConn.Close()
+	e.linkMonUnregister()
 	e.linkMon.Close()
 	e.router.Close()
 	e.wgdev.Close()
@@ -1475,3 +1463,15 @@ func diagnoseLinuxTUNFailure(tunName string, logf logger.Logf) {
 		}
 	}
 }
+
+type closeOnErrorPool []func()
+
+func (p *closeOnErrorPool) add(c io.Closer)   { *p = append(*p, func() { c.Close() }) }
+func (p *closeOnErrorPool) addFunc(fn func()) { *p = append(*p, fn) }
+func (p closeOnErrorPool) closeAllIfError(errp *error) {
+	if *errp != nil {
+		for _, closeFn := range p {
+			closeFn()
+		}
+	}
+}