Move upnp portmap to separate fn

This isolates the upnp portmapping to another function

Signed-off-by: julianknodt <julianknodt@gmail.com>

cmd/tailscale/depaware.txt

@@ -36,16 +36,16 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
         tailscale.com/net/tlsdial                                    from tailscale.com/derp/derphttp
         tailscale.com/net/tsaddr                                     from tailscale.com/net/interfaces+
      💣 tailscale.com/net/tshttpproxy                                from tailscale.com/derp/derphttp+
-        tailscale.com/net/upnp                                       from tailscale.com/net/upnp/dcps/internetgateway2
-        tailscale.com/net/upnp/dcps/internetgateway2                 from tailscale.com/net/portmapper
-        tailscale.com/net/upnp/httpu                                 from tailscale.com/net/upnp
-        tailscale.com/net/upnp/scpd                                  from tailscale.com/net/upnp
-        tailscale.com/net/upnp/soap                                  from tailscale.com/net/upnp+
-        tailscale.com/net/upnp/ssdp                                  from tailscale.com/net/upnp
         tailscale.com/paths                                          from tailscale.com/cmd/tailscale/cli+
         tailscale.com/safesocket                                     from tailscale.com/cmd/tailscale/cli+
         tailscale.com/syncs                                          from tailscale.com/net/interfaces+
         tailscale.com/tailcfg                                        from tailscale.com/cmd/tailscale/cli+
+        tailscale.com/tempfork/upnp                                  from tailscale.com/tempfork/upnp/dcps/internetgateway2
+        tailscale.com/tempfork/upnp/dcps/internetgateway2            from tailscale.com/net/portmapper
+        tailscale.com/tempfork/upnp/httpu                            from tailscale.com/tempfork/upnp
+        tailscale.com/tempfork/upnp/scpd                             from tailscale.com/tempfork/upnp
+        tailscale.com/tempfork/upnp/soap                             from tailscale.com/tempfork/upnp+
+        tailscale.com/tempfork/upnp/ssdp                             from tailscale.com/tempfork/upnp
    W    tailscale.com/tsconst                                        from tailscale.com/net/interfaces
         tailscale.com/types/empty                                    from tailscale.com/ipn
         tailscale.com/types/ipproto                                  from tailscale.com/net/flowtrack+

cmd/tailscaled/depaware.txt

@@ -112,18 +112,18 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
         tailscale.com/net/tsaddr                                     from tailscale.com/ipn/ipnlocal+
      💣 tailscale.com/net/tshttpproxy                                from tailscale.com/control/controlclient+
         tailscale.com/net/tstun                                      from tailscale.com/cmd/tailscaled+
-        tailscale.com/net/upnp                                       from tailscale.com/net/upnp/dcps/internetgateway2
-        tailscale.com/net/upnp/dcps/internetgateway2                 from tailscale.com/net/portmapper
-        tailscale.com/net/upnp/httpu                                 from tailscale.com/net/upnp
-        tailscale.com/net/upnp/scpd                                  from tailscale.com/net/upnp
-        tailscale.com/net/upnp/soap                                  from tailscale.com/net/upnp+
-        tailscale.com/net/upnp/ssdp                                  from tailscale.com/net/upnp
         tailscale.com/paths                                          from tailscale.com/cmd/tailscaled+
         tailscale.com/portlist                                       from tailscale.com/ipn/ipnlocal
         tailscale.com/safesocket                                     from tailscale.com/ipn/ipnserver
         tailscale.com/smallzstd                                      from tailscale.com/ipn/ipnserver+
         tailscale.com/syncs                                          from tailscale.com/net/interfaces+
         tailscale.com/tailcfg                                        from tailscale.com/control/controlclient+
+        tailscale.com/tempfork/upnp                                  from tailscale.com/tempfork/upnp/dcps/internetgateway2
+        tailscale.com/tempfork/upnp/dcps/internetgateway2            from tailscale.com/net/portmapper
+        tailscale.com/tempfork/upnp/httpu                            from tailscale.com/tempfork/upnp
+        tailscale.com/tempfork/upnp/scpd                             from tailscale.com/tempfork/upnp
+        tailscale.com/tempfork/upnp/soap                             from tailscale.com/tempfork/upnp+
+        tailscale.com/tempfork/upnp/ssdp                             from tailscale.com/tempfork/upnp
    W    tailscale.com/tsconst                                        from tailscale.com/net/interfaces
         tailscale.com/tstime                                         from tailscale.com/wgengine/magicsock
         tailscale.com/types/empty                                    from tailscale.com/control/controlclient+
@@ -238,7 +238,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
         encoding/hex                                                 from crypto/x509+
         encoding/json                                                from expvar+
         encoding/pem                                                 from crypto/tls+
-        encoding/xml                                                 from tailscale.com/net/upnp+
+        encoding/xml                                                 from tailscale.com/tempfork/upnp+
         errors                                                       from bufio+
         expvar                                                       from tailscale.com/derp+
         flag                                                         from tailscale.com/cmd/tailscaled+

net/netcheck/netcheck.go

@@ -693,8 +693,7 @@ func (rs *reportState) probePortMapServices() {
 	rs.setOptBool(&rs.report.PMP, false)
 	rs.setOptBool(&rs.report.PCP, false)
 
-	rs.c.PortMapper.NewProber(context.Background())
+	res, err := rs.c.PortMapper.Probe(context.Background())
-	res, err := rs.c.PortMapper.Prober.StatusBlock()
 	if err != nil {
 		rs.c.logf("probePortMapServices: %v", err)
 		return

net/portmapper/portmapper.go

@@ -58,12 +58,18 @@ type Client struct {
 
 	localPort uint16
 
-	mapping Mapping // non-nil if we have a mapping
+	mapping // non-nil if we have a mapping
 
-	Prober *Prober
+	// Prober is this portmappers stateful mechanism for detecting when portmapping services are
+	// available on the current network. It is exposed so that clients can pause or stop probing.
+	// In order to create a prober, either call `Probe()` or `NewProber()`, which will populate
+	// this field.
+	*Prober
 }
 
-type Mapping interface {
+// Mapping represents a created port-mapping over some protocol.  It specifies a lease duration,
+// how to release the mapping, and whether the map is still valid.
+type mapping interface {
 	isCurrent() bool
 	release()
 	validUntil() time.Time
@@ -252,6 +258,11 @@ var (
 	ErrGatewayNotFound       = errors.New("failed to look up gateway address")
 )
 
+// Probe starts a periodic probe and blocks until the first result of probing.
+func (c *Client) Probe(ctx context.Context) (ProbeResult, error) {
+	return c.NewProber(ctx).StatusBlock()
+}
+
 // CreateOrGetMapping either creates a new mapping or returns a cached
 // valid one.
 //
@@ -265,9 +276,10 @@ func (c *Client) CreateOrGetMapping(ctx context.Context) (external netaddr.IPPor
 
 	c.mu.Lock()
 	localPort := c.localPort
+	internalAddr := netaddr.IPPortFrom(myIP, localPort)
 	m := &pmpMapping{
 		gw:       gw,
-		internal: netaddr.IPPortFrom(myIP, localPort),
+		internal: internalAddr,
 	}
 
 	// prevPort is the port we had most previously, if any. We try
@@ -368,53 +380,7 @@ func (c *Client) CreateOrGetMapping(ctx context.Context) (external netaddr.IPPor
 		}
 	}
 
-	// If did not see UPnP within the past 5 seconds then bail
+	return c.getUPnPPortMapping(ctx, gw, internalAddr, prevPort)
-	haveRecentUPnP := c.sawUPnPRecently()
-	if c.lastProbe.After(now.Add(-5*time.Second)) && !haveRecentUPnP {
-		return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices}
-	}
-	// Otherwise try a uPnP mapping if PMP did not work
-	mpnp := &upnpMapping{
-		gw:       m.gw,
-		internal: m.internal,
-	}
-
-	var client upnpClient
-	c.mu.Lock()
-	oldMapping, ok := c.mapping.(*upnpMapping)
-	c.mu.Unlock()
-	if ok {
-		client = oldMapping.client
-	} else if c.Prober != nil && c.Prober.upnpClient != nil {
-		client = c.Prober.upnpClient
-	} else {
-		client, err = getUPnPClient(ctx)
-		if err != nil {
-			return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices}
-		}
-	}
-	if client == nil {
-		return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices}
-	}
-
-	var newPort uint16
-	newPort, err = AddAnyPortMapping(
-		ctx, client,
-		"", prevPort, "UDP", localPort, m.internal.IP().String(), true,
-		"tailscale-portfwd", pmpMapLifetimeSec,
-	)
-	if err != nil {
-		return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices}
-	}
-	mpnp.external = netaddr.IPPortFrom(gw, newPort)
-	d := time.Duration(pmpMapLifetimeSec) * time.Second / 2
-	mpnp.useUntil = time.Now().Add(d)
-	mpnp.client = client
-	c.mu.Lock()
-	defer c.mu.Unlock()
-	c.mapping = mpnp
-	c.localPort = newPort
-	return mpnp.external, nil
 }
 
 type pmpResultCode uint16

net/portmapper/portmapper_test.go

@@ -48,8 +48,7 @@ func TestClientProbeThenMap(t *testing.T) {
 	}
 	c := NewClient(t.Logf)
 	c.SetLocalPort(1234)
-	c.NewProber(context.Background())
+	res, err := c.Probe(context.Background())
-	res, err := c.Prober.StatusBlock()
 	t.Logf("Probe: %+v, %v", res, err)
 	ext, err := c.CreateOrGetMapping(context.Background())
 	t.Logf("CreateOrGetMapping: %v, %v", ext, err)

net/portmapper/probe.go

@@ -12,25 +12,34 @@ import (
 	"tailscale.com/net/netns"
 )
 
+// Prober periodically pings the network and checks for port-mapping services.
 type Prober struct {
 	// pause signals the probe to either pause temporarily (true), or stop entirely (false)
 	// to restart the probe, send another pause to it.
 	pause chan<- bool
 
-	PMP *ProbeSubResult
+	// Each of the SubResults below is intended to expose whether a specific service is available
-	PCP *ProbeSubResult
+	// for use on a client, and the most recent seen time. Should not be modified externally, and
+	// will be periodically updated.
 
+	// PMP stores the result of probing pmp services and is populated by prober.
+	PMP ProbeSubResult
+	// PCP stores the result of probing pcp services and is populated by prober.
+	PCP ProbeSubResult
+
+	// upnpClient is a reused upnpClient for probing upnp results.
 	upnpClient upnpClient
-	UPnP       *ProbeSubResult
+	// PCP stores the result of probing pcp services and is populated by prober.
+	UPnP ProbeSubResult
 }
 
 // NewProber creates a new prober for a given client.
-func (c *Client) NewProber(ctx context.Context) (p *Prober) {
+func (c *Client) NewProber(ctx context.Context) *Prober {
 	if c.Prober != nil {
 		return c.Prober
 	}
 	pause := make(chan bool)
-	p = &Prober{
+	p := &Prober{
 		pause: pause,
 
 		PMP:  NewProbeSubResult(),
@@ -41,8 +50,8 @@ func (c *Client) NewProber(ctx context.Context) (p *Prober) {
 
 	go func() {
 		for {
-			pmp_ctx, cancel := context.WithTimeout(ctx, portMapServiceTimeout)
+			pmpCtx, cancel := context.WithTimeout(ctx, portMapServiceTimeout)
-			hasPCP, hasPMP, err := c.probePMPAndPCP(pmp_ctx)
+			hasPCP, hasPMP, err := c.probePMPAndPCP(pmpCtx)
 			if err != nil {
 				if ctx.Err() == context.DeadlineExceeded {
 					err = nil
@@ -50,7 +59,7 @@ func (c *Client) NewProber(ctx context.Context) (p *Prober) {
 					cancel()
 					return
 				}
-				if pmp_ctx.Err() == context.DeadlineExceeded {
+				if pmpCtx.Err() == context.DeadlineExceeded {
 					err = nil
 				}
 			}
@@ -92,19 +101,19 @@ func (c *Client) NewProber(ctx context.Context) (p *Prober) {
 		}()
 		// TODO maybe do something fancy/dynamic with more delay (exponential back-off)
 		for {
-			upnp_ctx, cancel := context.WithTimeout(ctx, portMapServiceTimeout*5)
+			upnpCtx, cancel := context.WithTimeout(ctx, portMapServiceTimeout*5)
 			retries := 0
 			hasUPnP := false
 			const num_connect_retries = 5
 			for retries < num_connect_retries {
-				status, _, _, statusErr := p.upnpClient.GetStatusInfo(upnp_ctx)
+				status, _, _, statusErr := p.upnpClient.GetStatusInfo(upnpCtx)
 				if statusErr != nil {
 					err = statusErr
 					break
 				}
 				hasUPnP = hasUPnP || status == "Connected"
 				if status == "Disconnected" {
-					upnpClient.RequestConnection(upnp_ctx)
+					upnpClient.RequestConnection(upnpCtx)
 				}
 				retries += 1
 			}
@@ -115,7 +124,7 @@ func (c *Client) NewProber(ctx context.Context) (p *Prober) {
 				cancel()
 				return
 			}
-			if upnp_ctx.Err() == context.DeadlineExceeded {
+			if upnpCtx.Err() == context.DeadlineExceeded {
 				err = nil
 			}
 			cancel()
@@ -139,18 +148,14 @@ func (c *Client) NewProber(ctx context.Context) (p *Prober) {
 		}
 	}()
 
-	return
+	return p
 }
 
-// Stop gracefully turns the Prober off.
+// Stop gracefully turns the Prober off, completing the current probes before exiting.
-func (p *Prober) Stop() {
+func (p *Prober) Stop() { close(p.pause) }
-	close(p.pause)
-}
 
-// Pauses the prober if currently running, or starts if it was previously paused
+// Pauses the prober if currently running, or starts if it was previously paused.
-func (p *Prober) Toggle() {
+func (p *Prober) Toggle() { p.pause <- true }
-	p.pause <- true
-}
 
 // CurrentStatus returns the current results of the prober, regardless of whether they have
 // completed or not.
@@ -173,6 +178,7 @@ func (p *Prober) CurrentStatus() (res ProbeResult, err error) {
 	return
 }
 
+// Blocks until the current probe gets any result.
 func (p *Prober) StatusBlock() (res ProbeResult, err error) {
 	hasPMP, errPMP := p.PMP.PresentBlock()
 	res.PMP = hasPMP
@@ -192,6 +198,7 @@ func (p *Prober) StatusBlock() (res ProbeResult, err error) {
 	return
 }
 
+// ProbeSubResult is a result for a single probing service.
 type ProbeSubResult struct {
 	cond *sync.Cond
 	// If this probe has finished, regardless of success or failure
@@ -202,12 +209,12 @@ type ProbeSubResult struct {
 	// most recent error
 	err error
 
-	// time we last saw it to be available.
+	// Time we last saw the service to be available.
 	sawTime time.Time
 }
 
-func NewProbeSubResult() *ProbeSubResult {
+func NewProbeSubResult() ProbeSubResult {
-	return &ProbeSubResult{
+	return ProbeSubResult{
 		cond: &sync.Cond{
 			L: &sync.Mutex{},
 		},
@@ -232,6 +239,8 @@ func (psr *ProbeSubResult) PresentCurrent() (bool, error) {
 	return present, psr.err
 }
 
+// Assigns the result of the probe and any error seen, signalling to any items waiting for this
+// result that it is now available.
 func (psr *ProbeSubResult) Set(present bool, err error) {
 	saw := time.Now()
 	psr.cond.L.Lock()

net/portmapper/upnp.go

@@ -43,14 +43,18 @@ type upnpClient interface {
 		newLeaseDuration uint32,
 	) (err error)
 
-	DeletePortMapping(ctx context.Context, NewRemoteHost string, NewExternalPort uint16, NewProtocol string) error
+	DeletePortMapping(ctx context.Context, newRemoteHost string, newExternalPort uint16, newProtocol string) error
 	GetStatusInfo(ctx context.Context) (status string, lastErr string, uptime uint32, err error)
+	GetExternalIPAddress(ctx context.Context) (externalIPAddress string, err error)
 
 	RequestTermination(ctx context.Context) error
 	RequestConnection(ctx context.Context) error
 }
 
-func AddAnyPortMapping(
+// addAnyPortMapping abstracts over different UPnP client connections, calling the available
+// AddAnyPortMapping call if available, otherwise defaulting to the old behavior of calling
+// AddPortMapping with port = 0 to specify a wildcard port.
+func addAnyPortMapping(
 	ctx context.Context,
 	upnp upnpClient,
 	newRemoteHost string,
@@ -94,6 +98,7 @@ func AddAnyPortMapping(
 // Adapted from https://github.com/huin/goupnp/blob/master/GUIDE.md.
 func getUPnPClient(ctx context.Context) (upnpClient, error) {
 	tasks, _ := errgroup.WithContext(ctx)
+	// Attempt to connect over the multiple available connection types.
 	var ip1Clients []*internetgateway2.WANIPConnection1
 	tasks.Go(func() error {
 		var err error
@@ -128,3 +133,70 @@ func getUPnPClient(ctx context.Context) (upnpClient, error) {
 		return nil, err
 	}
 }
+
+// getUPnPPortMapping will attempt to create a port-mapping over the UPnP protocol. On success,
+// it will return the externally exposed IP and port. Otherwise, it will return a zeroed IP and
+// port and an error.
+func (c *Client) getUPnPPortMapping(ctx context.Context, gw netaddr.IP, internal netaddr.IPPort,
+	prevPort uint16) (external netaddr.IPPort, err error) {
+	// If did not see UPnP within the past 5 seconds then bail
+	haveRecentUPnP := c.sawUPnPRecently()
+	now := time.Now()
+	if c.lastProbe.After(now.Add(-5*time.Second)) && !haveRecentUPnP {
+		return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices}
+	}
+	// Otherwise try a uPnP mapping if PMP did not work
+	mpnp := &upnpMapping{
+		gw:       gw,
+		internal: internal,
+	}
+
+	var client upnpClient
+	c.mu.Lock()
+	oldMapping, ok := c.mapping.(*upnpMapping)
+	c.mu.Unlock()
+	if ok && oldMapping != nil {
+		client = oldMapping.client
+	} else if c.Prober != nil && c.Prober.upnpClient != nil {
+		client = c.Prober.upnpClient
+	} else {
+		client, err = getUPnPClient(ctx)
+		if err != nil {
+			return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices}
+		}
+	}
+	if client == nil {
+		return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices}
+	}
+
+	var newPort uint16
+	newPort, err = addAnyPortMapping(
+		ctx, client,
+		"", prevPort, "UDP", internal.Port(), internal.IP().String(), true,
+		// string below is just a name for reporting on device.
+		"tailscale-portmap", pmpMapLifetimeSec,
+	)
+	if err != nil {
+		return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices}
+	}
+	// TODO cache this ip somewhere?
+	extIP, err := client.GetExternalIPAddress(ctx)
+	if err != nil {
+		// TODO this doesn't seem right
+		return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices}
+	}
+	externalIP, err := netaddr.ParseIP(extIP)
+	if err != nil {
+		return netaddr.IPPort{}, NoMappingError{ErrNoPortMappingServices}
+	}
+
+	mpnp.external = netaddr.IPPortFrom(externalIP, newPort)
+	d := time.Duration(pmpMapLifetimeSec) * time.Second / 2
+	mpnp.useUntil = time.Now().Add(d)
+	mpnp.client = client
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	c.mapping = mpnp
+	c.localPort = newPort
+	return mpnp.external, nil
+}