net/dns/resolver: remove Start method, fully spin up in New instead.

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

net/dns/resolver/tsdns.go

@@ -84,7 +84,7 @@ type Resolver struct {
 
 // New returns a new resolver.
 // linkMon optionally specifies a link monitor to use for socket rebinding.
-func New(logf logger.Logf, linkMon *monitor.Mon) *Resolver {
+func New(logf logger.Logf, linkMon *monitor.Mon) (*Resolver, error) {
 	r := &Resolver{
 		logf:      logger.WithPrefix(logf, "dns: "),
 		linkMon:   linkMon,
@@ -98,20 +98,14 @@ func New(logf logger.Logf, linkMon *monitor.Mon) *Resolver {
 		r.unregLinkMon = r.linkMon.RegisterChangeCallback(r.onLinkMonitorChange)
 	}
 
-	return r
-}
-
-func (r *Resolver) Start() error {
-	if r.forwarder != nil {
 	if err := r.forwarder.Start(); err != nil {
-			return err
-		}
+		return nil, err
 	}
 
 	r.wg.Add(1)
 	go r.poll()
 
-	return nil
+	return r, nil
 }
 
 // Close shuts down the resolver and ensures poll goroutines have exited.

net/dns/resolver/tsdns_test.go

@@ -194,14 +194,14 @@ func TestRDNSNameToIPv6(t *testing.T) {
 }
 
 func TestResolve(t *testing.T) {
-	r := New(t.Logf, nil)
-	r.SetMap(dnsMap)
-
-	if err := r.Start(); err != nil {
+	r, err := New(t.Logf, nil)
+	if err != nil {
 		t.Fatalf("start: %v", err)
 	}
 	defer r.Close()
 
+	r.SetMap(dnsMap)
+
 	tests := []struct {
 		name  string
 		qname string
@@ -240,14 +240,14 @@ func TestResolve(t *testing.T) {
 }
 
 func TestResolveReverse(t *testing.T) {
-	r := New(t.Logf, nil)
-	r.SetMap(dnsMap)
-
-	if err := r.Start(); err != nil {
+	r, err := New(t.Logf, nil)
+	if err != nil {
 		t.Fatalf("start: %v", err)
 	}
 	defer r.Close()
 
+	r.SetMap(dnsMap)
+
 	tests := []struct {
 		name string
 		ip   netaddr.IP
@@ -318,18 +318,18 @@ func TestDelegate(t *testing.T) {
 		return
 	}
 
-	r := New(t.Logf, nil)
+	r, err := New(t.Logf, nil)
+	if err != nil {
+		t.Fatalf("start: %v", err)
+	}
+	defer r.Close()
+
 	r.SetMap(dnsMap)
 	r.SetUpstreams([]net.Addr{
 		v4server.PacketConn.LocalAddr(),
 		v6server.PacketConn.LocalAddr(),
 	})
 
-	if err := r.Start(); err != nil {
-		t.Fatalf("start: %v", err)
-	}
-	defer r.Close()
-
 	tests := []struct {
 		title    string
 		query    []byte
@@ -397,15 +397,15 @@ func TestDelegateCollision(t *testing.T) {
 	}
 	defer server.Shutdown()
 
-	r := New(t.Logf, nil)
-	r.SetMap(dnsMap)
-	r.SetUpstreams([]net.Addr{server.PacketConn.LocalAddr()})
-
-	if err := r.Start(); err != nil {
+	r, err := New(t.Logf, nil)
+	if err != nil {
 		t.Fatalf("start: %v", err)
 	}
 	defer r.Close()
 
+	r.SetMap(dnsMap)
+	r.SetUpstreams([]net.Addr{server.PacketConn.LocalAddr()})
+
 	packets := []struct {
 		qname string
 		qtype dns.Type
@@ -463,9 +463,8 @@ func TestDelegateCollision(t *testing.T) {
 }
 
 func TestConcurrentSetMap(t *testing.T) {
-	r := New(t.Logf, nil)
-
-	if err := r.Start(); err != nil {
+	r, err := New(t.Logf, nil)
+	if err != nil {
 		t.Fatalf("start: %v", err)
 	}
 	defer r.Close()
@@ -499,14 +498,14 @@ func TestConcurrentSetUpstreams(t *testing.T) {
 	}
 	defer server.Shutdown()
 
-	r := New(t.Logf, nil)
-	r.SetMap(dnsMap)
-
-	if err := r.Start(); err != nil {
+	r, err := New(t.Logf, nil)
+	if err != nil {
 		t.Fatalf("start: %v", err)
 	}
 	defer r.Close()
 
+	r.SetMap(dnsMap)
+
 	packet := dnspacket("test.site.", dns.TypeA)
 	// This is purely to ensure that delegation does not race with SetUpstreams.
 	var wg sync.WaitGroup
@@ -670,14 +669,14 @@ var emptyResponse = []byte{
 }
 
 func TestFull(t *testing.T) {
-	r := New(t.Logf, nil)
-	r.SetMap(dnsMap)
-
-	if err := r.Start(); err != nil {
+	r, err := New(t.Logf, nil)
+	if err != nil {
 		t.Fatalf("start: %v", err)
 	}
 	defer r.Close()
 
+	r.SetMap(dnsMap)
+
 	// One full packet and one error packet
 	tests := []struct {
 		name     string
@@ -709,13 +708,12 @@ func TestFull(t *testing.T) {
 }
 
 func TestAllocs(t *testing.T) {
-	r := New(t.Logf, nil)
-	r.SetMap(dnsMap)
-
-	if err := r.Start(); err != nil {
+	r, err := New(t.Logf, nil)
+	if err != nil {
 		t.Fatalf("start: %v", err)
 	}
 	defer r.Close()
+	r.SetMap(dnsMap)
 
 	// It is seemingly pointless to test allocs in the delegate path,
 	// as dialer.Dial -> Read -> Write alone comprise 12 allocs.
@@ -778,15 +776,15 @@ func BenchmarkFull(b *testing.B) {
 	}
 	defer server.Shutdown()
 
-	r := New(b.Logf, nil)
-	r.SetMap(dnsMap)
-	r.SetUpstreams([]net.Addr{server.PacketConn.LocalAddr()})
-
-	if err := r.Start(); err != nil {
+	r, err := New(b.Logf, nil)
+	if err != nil {
 		b.Fatalf("start: %v", err)
 	}
 	defer r.Close()
 
+	r.SetMap(dnsMap)
+	r.SetUpstreams([]net.Addr{server.PacketConn.LocalAddr()})
+
 	tests := []struct {
 		name    string
 		request []byte

wgengine/userspace.go

@@ -219,7 +219,11 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error)
 		e.linkMonOwned = true
 	}
 
-	e.resolver = resolver.New(logf, e.linkMon)
+	var err error
+	e.resolver, err = resolver.New(logf, e.linkMon)
+	if err != nil {
+		return nil, err
+	}
 
 	logf("link state: %+v", e.linkMon.InterfaceState())
 
@@ -246,7 +250,7 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error)
 		NoteRecvActivity: e.noteReceiveActivity,
 		LinkMonitor:      e.linkMon,
 	}
-	var err error
+
 	e.magicConn, err = magicsock.NewConn(magicsockOpts)
 	if err != nil {
 		return nil, fmt.Errorf("wgengine: %v", err)
@@ -374,8 +378,6 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error)
 	e.logf("Starting magicsock...")
 	e.magicConn.Start()
 
-	e.logf("Starting resolver...")
-	e.resolver.Start()
 	go e.pollResolver()
 
 	e.logf("Engine created.")