feature/portmapper: make the portmapper & its debugging tools modular

Starting at a minimal binary and adding one feature back...
    tailscaled tailscale combined (linux/amd64)
     30073135  17451704  31543692 omitting everything
    +  480302 +   10258 +  493896 .. add debugportmapper
    +  475317 +  151943 +  467660 .. add portmapper
    +  500086 +  162873 +  510511 .. add portmapper+debugportmapper

Fixes #17148

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

net/portmapper/igd_test.go

@@ -14,7 +14,6 @@ import (
 	"sync/atomic"
 	"testing"
 
-	"tailscale.com/control/controlknobs"
 	"tailscale.com/net/netaddr"
 	"tailscale.com/net/netmon"
 	"tailscale.com/syncs"
@@ -273,10 +272,9 @@ func newTestClient(t *testing.T, igd *TestIGD, bus *eventbus.Bus) *Client {
 	}
 	var c *Client
 	c = NewClient(Config{
-		Logf:         tstest.WhileTestRunningLogger(t),
-		NetMon:       netmon.NewStatic(),
-		ControlKnobs: new(controlknobs.Knobs),
-		EventBus:     bus,
+		Logf:     tstest.WhileTestRunningLogger(t),
+		NetMon:   netmon.NewStatic(),
+		EventBus: bus,
 		OnChange: func() { // TODO(creachadair): Remove.
 			t.Logf("port map changed")
 			t.Logf("have mapping: %v", c.HaveMapping())

net/portmapper/portmapper.go

@@ -8,7 +8,6 @@ package portmapper
 import (
 	"context"
 	"encoding/binary"
-	"errors"
 	"fmt"
 	"io"
 	"net"
@@ -20,12 +19,12 @@ import (
 	"time"
 
 	"go4.org/mem"
-	"tailscale.com/control/controlknobs"
 	"tailscale.com/envknob"
 	"tailscale.com/net/netaddr"
 	"tailscale.com/net/neterror"
 	"tailscale.com/net/netmon"
 	"tailscale.com/net/netns"
+	"tailscale.com/net/portmapper/portmappertype"
 	"tailscale.com/net/sockstats"
 	"tailscale.com/syncs"
 	"tailscale.com/types/logger"
@@ -34,6 +33,13 @@ import (
 	"tailscale.com/util/eventbus"
 )
 
+var (
+	ErrNoPortMappingServices = portmappertype.ErrNoPortMappingServices
+	ErrGatewayRange          = portmappertype.ErrGatewayRange
+	ErrGatewayIPv6           = portmappertype.ErrGatewayIPv6
+	ErrPortMappingDisabled   = portmappertype.ErrPortMappingDisabled
+)
+
 var disablePortMapperEnv = envknob.RegisterBool("TS_DISABLE_PORTMAPPER")
 
 // DebugKnobs contains debug configuration that can be provided when creating a
@@ -49,15 +55,33 @@ type DebugKnobs struct {
 	LogHTTP bool
 
 	// Disable* disables a specific service from mapping.
-	DisableUPnP bool
-	DisablePMP  bool
-	DisablePCP  bool
+	// If the funcs are nil or return false, the service is not disabled.
+	// Use the corresponding accessor methods without the "Func" suffix
+	// to check whether a service is disabled.
+	DisableUPnPFunc func() bool
+	DisablePMPFunc  func() bool
+	DisablePCPFunc  func() bool
 
 	// DisableAll, if non-nil, is a func that reports whether all port
 	// mapping attempts should be disabled.
 	DisableAll func() bool
 }
 
+// DisableUPnP reports whether UPnP is disabled.
+func (k *DebugKnobs) DisableUPnP() bool {
+	return k != nil && k.DisableUPnPFunc != nil && k.DisableUPnPFunc()
+}
+
+// DisablePMP reports whether NAT-PMP is disabled.
+func (k *DebugKnobs) DisablePMP() bool {
+	return k != nil && k.DisablePMPFunc != nil && k.DisablePMPFunc()
+}
+
+// DisablePCP reports whether PCP is disabled.
+func (k *DebugKnobs) DisablePCP() bool {
+	return k != nil && k.DisablePCPFunc != nil && k.DisablePCPFunc()
+}
+
 func (k *DebugKnobs) disableAll() bool {
 	if disablePortMapperEnv() {
 		return true
@@ -88,11 +112,10 @@ type Client struct {
 	// The following two fields must both be non-nil.
 	// Both are immutable after construction.
 	pubClient *eventbus.Client
-	updates   *eventbus.Publisher[Mapping]
+	updates   *eventbus.Publisher[portmappertype.Mapping]
 
 	logf         logger.Logf
 	netMon       *netmon.Monitor // optional; nil means interfaces will be looked up on-demand
-	controlKnobs *controlknobs.Knobs
 	ipAndGateway func() (gw, ip netip.Addr, ok bool)
 	onChange     func() // or nil
 	debug        DebugKnobs
@@ -130,6 +153,8 @@ type Client struct {
 	mapping mapping // non-nil if we have a mapping
 }
 
+var _ portmappertype.Client = (*Client)(nil)
+
 func (c *Client) vlogf(format string, args ...any) {
 	if c.debug.VerboseLogs {
 		c.logf(format, args...)
@@ -159,7 +184,6 @@ type mapping interface {
 	MappingDebug() string
 }
 
-// HaveMapping reports whether we have a current valid mapping.
 func (c *Client) HaveMapping() bool {
 	c.mu.Lock()
 	defer c.mu.Unlock()
@@ -223,10 +247,6 @@ type Config struct {
 	// debugging.  If nil, a sensible set of defaults will be used.
 	DebugKnobs *DebugKnobs
 
-	// ControlKnobs, if non-nil, specifies knobs from the control plane that
-	// might disable port mapping.
-	ControlKnobs *controlknobs.Knobs
-
 	// OnChange is called to run in a new goroutine whenever the port mapping
 	// status has changed. If nil, no callback is issued.
 	OnChange func()
@@ -246,10 +266,9 @@ func NewClient(c Config) *Client {
 		netMon:       c.NetMon,
 		ipAndGateway: netmon.LikelyHomeRouterIP, // TODO(bradfitz): move this to method on netMon
 		onChange:     c.OnChange,
-		controlKnobs: c.ControlKnobs,
 	}
 	ret.pubClient = c.EventBus.Client("portmapper")
-	ret.updates = eventbus.Publish[Mapping](ret.pubClient)
+	ret.updates = eventbus.Publish[portmappertype.Mapping](ret.pubClient)
 	if ret.logf == nil {
 		ret.logf = logger.Discard
 	}
@@ -448,13 +467,6 @@ func IsNoMappingError(err error) bool {
 	return ok
 }
 
-var (
-	ErrNoPortMappingServices = errors.New("no port mapping services were found")
-	ErrGatewayRange          = errors.New("skipping portmap; gateway range likely lacks support")
-	ErrGatewayIPv6           = errors.New("skipping portmap; no IPv6 support for portmapping")
-	ErrPortMappingDisabled   = errors.New("port mapping is disabled")
-)
-
 // GetCachedMappingOrStartCreatingOne quickly returns with our current cached portmapping, if any.
 // If there's not one, it starts up a background goroutine to create one.
 // If the background goroutine ends up creating one, the onChange hook registered with the
@@ -512,7 +524,7 @@ func (c *Client) createMapping() {
 		// the control flow to eliminate that possibility. Meanwhile, this
 		// mitigates a panic downstream, cf. #16662.
 	}
-	c.updates.Publish(Mapping{
+	c.updates.Publish(portmappertype.Mapping{
 		External:  mapping.External(),
 		Type:      mapping.MappingType(),
 		GoodUntil: mapping.GoodUntil(),
@@ -524,15 +536,6 @@ func (c *Client) createMapping() {
 	}
 }
 
-// Mapping is an event recording the allocation of a port mapping.
-type Mapping struct {
-	External  netip.AddrPort
-	Type      string
-	GoodUntil time.Time
-
-	// TODO(creachadair): Record whether we reused an existing mapping?
-}
-
 // wildcardIP is used when the previous external IP is not known for PCP port mapping.
 var wildcardIP = netip.MustParseAddr("0.0.0.0")
 
@@ -545,7 +548,7 @@ func (c *Client) createOrGetMapping(ctx context.Context) (mapping mapping, exter
 	if c.debug.disableAll() {
 		return nil, netip.AddrPort{}, NoMappingError{ErrPortMappingDisabled}
 	}
-	if c.debug.DisableUPnP && c.debug.DisablePCP && c.debug.DisablePMP {
+	if c.debug.DisableUPnP() && c.debug.DisablePCP() && c.debug.DisablePMP() {
 		return nil, netip.AddrPort{}, NoMappingError{ErrNoPortMappingServices}
 	}
 	gw, myIP, ok := c.gatewayAndSelfIP()
@@ -624,7 +627,7 @@ func (c *Client) createOrGetMapping(ctx context.Context) (mapping mapping, exter
 		prevPort = m.External().Port()
 	}
 
-	if c.debug.DisablePCP && c.debug.DisablePMP {
+	if c.debug.DisablePCP() && c.debug.DisablePMP() {
 		c.mu.Unlock()
 		if external, ok := c.getUPnPPortMapping(ctx, gw, internalAddr, prevPort); ok {
 			return nil, external, nil
@@ -675,7 +678,7 @@ func (c *Client) createOrGetMapping(ctx context.Context) (mapping mapping, exter
 
 	pxpAddr := netip.AddrPortFrom(gw, c.pxpPort())
 
-	preferPCP := !c.debug.DisablePCP && (c.debug.DisablePMP || (!haveRecentPMP && haveRecentPCP))
+	preferPCP := !c.debug.DisablePCP() && (c.debug.DisablePMP() || (!haveRecentPMP && haveRecentPCP))
 
 	// Create a mapping, defaulting to PMP unless only PCP was seen recently.
 	if preferPCP {
@@ -860,19 +863,13 @@ func parsePMPResponse(pkt []byte) (res pmpResponse, ok bool) {
 	return res, true
 }
 
-type ProbeResult struct {
-	PCP  bool
-	PMP  bool
-	UPnP bool
-}
-
 // Probe returns a summary of which port mapping services are
 // available on the network.
 //
 // If a probe has run recently and there haven't been any network changes since,
 // the returned result might be server from the Client's cache, without
 // sending any network traffic.
-func (c *Client) Probe(ctx context.Context) (res ProbeResult, err error) {
+func (c *Client) Probe(ctx context.Context) (res portmappertype.ProbeResult, err error) {
 	if c.debug.disableAll() {
 		return res, ErrPortMappingDisabled
 	}
@@ -907,19 +904,19 @@ func (c *Client) Probe(ctx context.Context) (res ProbeResult, err error) {
 	// https://github.com/tailscale/tailscale/issues/1001
 	if c.sawPMPRecently() {
 		res.PMP = true
-	} else if !c.debug.DisablePMP {
+	} else if !c.debug.DisablePMP() {
 		metricPMPSent.Add(1)
 		uc.WriteToUDPAddrPort(pmpReqExternalAddrPacket, pxpAddr)
 	}
 	if c.sawPCPRecently() {
 		res.PCP = true
-	} else if !c.debug.DisablePCP {
+	} else if !c.debug.DisablePCP() {
 		metricPCPSent.Add(1)
 		uc.WriteToUDPAddrPort(pcpAnnounceRequest(myIP), pxpAddr)
 	}
 	if c.sawUPnPRecently() {
 		res.UPnP = true
-	} else if !c.debug.DisableUPnP {
+	} else if !c.debug.DisableUPnP() {
 		// Strictly speaking, you discover UPnP services by sending an
 		// SSDP query (which uPnPPacket is) to udp/1900 on the SSDP
 		// multicast address, and then get a flood of responses back

net/portmapper/portmapper_test.go

@@ -11,7 +11,7 @@ import (
 	"testing"
 	"time"
 
-	"tailscale.com/control/controlknobs"
+	"tailscale.com/net/portmapper/portmappertype"
 	"tailscale.com/util/eventbus/eventbustest"
 )
 
@@ -19,7 +19,7 @@ func TestCreateOrGetMapping(t *testing.T) {
 	if v, _ := strconv.ParseBool(os.Getenv("HIT_NETWORK")); !v {
 		t.Skip("skipping test without HIT_NETWORK=1")
 	}
-	c := NewClient(Config{Logf: t.Logf, ControlKnobs: new(controlknobs.Knobs)})
+	c := NewClient(Config{Logf: t.Logf})
 	defer c.Close()
 	c.SetLocalPort(1234)
 	for i := range 2 {
@@ -35,7 +35,7 @@ func TestClientProbe(t *testing.T) {
 	if v, _ := strconv.ParseBool(os.Getenv("HIT_NETWORK")); !v {
 		t.Skip("skipping test without HIT_NETWORK=1")
 	}
-	c := NewClient(Config{Logf: t.Logf, ControlKnobs: new(controlknobs.Knobs)})
+	c := NewClient(Config{Logf: t.Logf})
 	defer c.Close()
 	for i := range 3 {
 		if i > 0 {
@@ -50,7 +50,7 @@ func TestClientProbeThenMap(t *testing.T) {
 	if v, _ := strconv.ParseBool(os.Getenv("HIT_NETWORK")); !v {
 		t.Skip("skipping test without HIT_NETWORK=1")
 	}
-	c := NewClient(Config{Logf: t.Logf, ControlKnobs: new(controlknobs.Knobs)})
+	c := NewClient(Config{Logf: t.Logf})
 	defer c.Close()
 	c.debug.VerboseLogs = true
 	c.SetLocalPort(1234)
@@ -150,7 +150,7 @@ func TestUpdateEvent(t *testing.T) {
 		t.Fatalf("Probe failed: %v", err)
 	}
 	c.GetCachedMappingOrStartCreatingOne()
-	if err := eventbustest.Expect(tw, eventbustest.Type[Mapping]()); err != nil {
+	if err := eventbustest.Expect(tw, eventbustest.Type[portmappertype.Mapping]()); err != nil {
 		t.Error(err.Error())
 	}
 }

net/portmapper/portmappertype/portmappertype.go

@@ -0,0 +1,88 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+// Package portmappertype defines the net/portmapper interface, which may or may not be
+// linked into the binary.
+package portmappertype
+
+import (
+	"context"
+	"errors"
+	"net/netip"
+	"time"
+
+	"tailscale.com/feature"
+	"tailscale.com/net/netmon"
+	"tailscale.com/types/logger"
+	"tailscale.com/util/eventbus"
+)
+
+// HookNewPortMapper is a hook to install the portmapper creation function.
+// It must be set by an init function when buildfeatures.HasPortmapper is true.
+var HookNewPortMapper feature.Hook[func(logf logger.Logf,
+	bus *eventbus.Bus,
+	netMon *netmon.Monitor,
+	disableUPnPOrNil,
+	onlyTCP443OrNil func() bool) Client]
+
+var (
+	ErrNoPortMappingServices = errors.New("no port mapping services were found")
+	ErrGatewayRange          = errors.New("skipping portmap; gateway range likely lacks support")
+	ErrGatewayIPv6           = errors.New("skipping portmap; no IPv6 support for portmapping")
+	ErrPortMappingDisabled   = errors.New("port mapping is disabled")
+)
+
+// ProbeResult is the result of a portmapper probe, saying
+// which port mapping protocols were discovered.
+type ProbeResult struct {
+	PCP  bool
+	PMP  bool
+	UPnP bool
+}
+
+// Client is the interface implemented by a portmapper client.
+type Client interface {
+	// Probe returns a summary of which port mapping services are available on
+	// the network.
+	//
+	// If a probe has run recently and there haven't been any network changes
+	// since, the returned result might be server from the Client's cache,
+	// without sending any network traffic.
+	Probe(context.Context) (ProbeResult, error)
+
+	// HaveMapping reports whether we have a current valid mapping.
+	HaveMapping() bool
+
+	// SetGatewayLookupFunc set the func that returns the machine's default
+	// gateway IP, and the primary IP address for that gateway. It must be
+	// called before the client is used. If not called,
+	// interfaces.LikelyHomeRouterIP is used.
+	SetGatewayLookupFunc(f func() (gw, myIP netip.Addr, ok bool))
+
+	// NoteNetworkDown should be called when the network has transitioned to a down state.
+	// It's too late to release port mappings at this point (the user might've just turned off
+	// their wifi), but we can make sure we invalidate mappings for later when the network
+	// comes back.
+	NoteNetworkDown()
+
+	// GetCachedMappingOrStartCreatingOne quickly returns with our current cached portmapping, if any.
+	// If there's not one, it starts up a background goroutine to create one.
+	// If the background goroutine ends up creating one, the onChange hook registered with the
+	// NewClient constructor (if any) will fire.
+	GetCachedMappingOrStartCreatingOne() (external netip.AddrPort, ok bool)
+
+	// SetLocalPort updates the local port number to which we want to port
+	// map UDP traffic
+	SetLocalPort(localPort uint16)
+
+	Close() error
+}
+
+// Mapping is an event recording the allocation of a port mapping.
+type Mapping struct {
+	External  netip.AddrPort
+	Type      string
+	GoodUntil time.Time
+
+	// TODO(creachadair): Record whether we reused an existing mapping?
+}

net/portmapper/upnp.go

@@ -209,7 +209,7 @@ func addAnyPortMapping(
 // The meta is the most recently parsed UDP discovery packet response
 // from the Internet Gateway Device.
 func getUPnPRootDevice(ctx context.Context, logf logger.Logf, debug DebugKnobs, gw netip.Addr, meta uPnPDiscoResponse) (rootDev *goupnp.RootDevice, loc *url.URL, err error) {
-	if debug.DisableUPnP {
+	if debug.DisableUPnP() {
 		return nil, nil, nil
 	}
 
@@ -434,7 +434,7 @@ func (c *Client) getUPnPPortMapping(
 	internal netip.AddrPort,
 	prevPort uint16,
 ) (external netip.AddrPort, ok bool) {
-	if disableUPnpEnv() || c.debug.DisableUPnP || (c.controlKnobs != nil && c.controlKnobs.DisableUPnP.Load()) {
+	if disableUPnpEnv() || c.debug.DisableUPnP() {
 		return netip.AddrPort{}, false
 	}
 

net/portmapper/upnp_test.go

@@ -18,6 +18,7 @@ import (
 	"sync/atomic"
 	"testing"
 
+	"tailscale.com/net/portmapper/portmappertype"
 	"tailscale.com/tstest"
 )
 
@@ -1039,7 +1040,7 @@ func (u *upnpServer) handleControl(w http.ResponseWriter, r *http.Request, handl
 	}
 }
 
-func mustProbeUPnP(tb testing.TB, ctx context.Context, c *Client) ProbeResult {
+func mustProbeUPnP(tb testing.TB, ctx context.Context, c *Client) portmappertype.ProbeResult {
 	tb.Helper()
 	res, err := c.Probe(ctx)
 	if err != nil {