portmapper: update NewClient to use a Config argument

In preparation for adding more parameters (and later, moving some away), rework
the portmapper constructor to accept its arguments on a Config struct rather
than positionally.

This is a breaking change to the function signature, but one that is very easy
to update, and a search of GitHub reveals only six instances of usage outside
clones and forks of Tailscale itself, that are not direct copies of the code
fixed up here.

While we could stub in another constructor, I think it is safe to let those
folks do the update in-place, since their usage is already affected by other
changes we can't test for anyway.

Updates #15160

Change-Id: I9f8a5e12b38885074c98894b7376039261b43f43
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>

cmd/tailscale/cli/netcheck.go

@@ -55,7 +55,10 @@ func runNetcheck(ctx context.Context, args []string) error {
 
 	// Ensure that we close the portmapper after running a netcheck; this
 	// will release any port mappings created.
-	pm := portmapper.NewClient(logf, netMon, nil, nil, nil)
+	pm := portmapper.NewClient(portmapper.Config{
+		Logf:   logf,
+		NetMon: netMon,
+	})
 	defer pm.Close()
 
 	c := &netcheck.Client{

ipn/localapi/localapi.go

@@ -818,19 +818,25 @@ func (h *Handler) serveDebugPortmap(w http.ResponseWriter, r *http.Request) {
 	done := make(chan bool, 1)
 
 	var c *portmapper.Client
-	c = portmapper.NewClient(logger.WithPrefix(logf, "portmapper: "), h.b.NetMon(), debugKnobs, h.b.ControlKnobs(), func() {
-		logf("portmapping changed.")
-		logf("have mapping: %v", c.HaveMapping())
+	c = portmapper.NewClient(portmapper.Config{
+		Logf:         logger.WithPrefix(logf, "portmapper: "),
+		NetMon:       h.b.NetMon(),
+		DebugKnobs:   debugKnobs,
+		ControlKnobs: h.b.ControlKnobs(),
+		OnChange: func() {
+			logf("portmapping changed.")
+			logf("have mapping: %v", c.HaveMapping())
 
-		if ext, ok := c.GetCachedMappingOrStartCreatingOne(); ok {
-			logf("cb: mapping: %v", ext)
-			select {
-			case done <- true:
-			default:
+			if ext, ok := c.GetCachedMappingOrStartCreatingOne(); ok {
+				logf("cb: mapping: %v", ext)
+				select {
+				case done <- true:
+				default:
+				}
+				return
 			}
-			return
-		}
-		logf("cb: no mapping")
+			logf("cb: no mapping")
+		},
 	})
 	defer c.Close()
 

net/portmapper/igd_test.go

@@ -260,9 +260,14 @@ func (d *TestIGD) handlePCPQuery(pkt []byte, src netip.AddrPort) {
 
 func newTestClient(t *testing.T, igd *TestIGD) *Client {
 	var c *Client
-	c = NewClient(t.Logf, netmon.NewStatic(), nil, new(controlknobs.Knobs), func() {
-		t.Logf("port map changed")
-		t.Logf("have mapping: %v", c.HaveMapping())
+	c = NewClient(Config{
+		Logf:         t.Logf,
+		NetMon:       netmon.NewStatic(),
+		ControlKnobs: new(controlknobs.Knobs),
+		OnChange: func() {
+			t.Logf("port map changed")
+			t.Logf("have mapping: %v", c.HaveMapping())
+		},
 	})
 	c.testPxPPort = igd.TestPxPPort()
 	c.testUPnPPort = igd.TestUPnPPort()

net/portmapper/portmapper.go

@@ -201,32 +201,45 @@ func (m *pmpMapping) Release(ctx context.Context) {
 	uc.WriteToUDPAddrPort(pkt, m.gw)
 }
 
-// NewClient returns a new portmapping client.
-//
-// The netMon parameter is required.
-//
-// The debug argument allows configuring the behaviour of the portmapper for
-// debugging; if nil, a sensible set of defaults will be used.
-//
-// The controlKnobs, if non-nil, specifies the control knobs from the control
-// plane that might disable portmapping.
-//
-// The optional onChange argument specifies a func to run in a new goroutine
-// whenever the port mapping status has changed. If nil, it doesn't make a
-// callback.
-func NewClient(logf logger.Logf, netMon *netmon.Monitor, debug *DebugKnobs, controlKnobs *controlknobs.Knobs, onChange func()) *Client {
-	if netMon == nil {
+// Config carries the settings for a [Client].
+type Config struct {
+	// Logf is called to generate text logs for the client. If nil, logger.Discard is used.
+	Logf logger.Logf
+
+	// NetMon is the network monitor used by the client. It must be non-nil.
+	NetMon *netmon.Monitor
+
+	// DebugKnobs, if non-nil, configure the behaviour of the portmapper for
+	// 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()
+}
+
+// NewClient constructs a new portmapping [Client] from c. It will panic if any
+// required parameters are omitted.
+func NewClient(c Config) *Client {
+	if c.NetMon == nil {
 		panic("nil netMon")
 	}
 	ret := &Client{
-		logf:         logf,
-		netMon:       netMon,
+		logf:         c.Logf,
+		netMon:       c.NetMon,
 		ipAndGateway: netmon.LikelyHomeRouterIP, // TODO(bradfitz): move this to method on netMon
-		onChange:     onChange,
-		controlKnobs: controlKnobs,
+		onChange:     c.OnChange,
+		controlKnobs: c.ControlKnobs,
 	}
-	if debug != nil {
-		ret.debug = *debug
+	if ret.logf == nil {
+		ret.logf = logger.Discard
+	}
+	if c.DebugKnobs != nil {
+		ret.debug = *c.DebugKnobs
 	}
 	return ret
 }

net/portmapper/portmapper_test.go

@@ -18,7 +18,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(t.Logf, nil, nil, new(controlknobs.Knobs), nil)
+	c := NewClient(Config{Logf: t.Logf, ControlKnobs: new(controlknobs.Knobs)})
 	defer c.Close()
 	c.SetLocalPort(1234)
 	for i := range 2 {
@@ -34,7 +34,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(t.Logf, nil, nil, new(controlknobs.Knobs), nil)
+	c := NewClient(Config{Logf: t.Logf, ControlKnobs: new(controlknobs.Knobs)})
 	defer c.Close()
 	for i := range 3 {
 		if i > 0 {
@@ -49,7 +49,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(t.Logf, nil, nil, new(controlknobs.Knobs), nil)
+	c := NewClient(Config{Logf: t.Logf, ControlKnobs: new(controlknobs.Knobs)})
 	defer c.Close()
 	c.debug.VerboseLogs = true
 	c.SetLocalPort(1234)

wgengine/magicsock/magicsock.go

@@ -544,7 +544,13 @@ func NewConn(opts Options) (*Conn, error) {
 	portMapOpts := &portmapper.DebugKnobs{
 		DisableAll: func() bool { return opts.DisablePortMapper || c.onlyTCP443.Load() },
 	}
-	c.portMapper = portmapper.NewClient(portmapperLogf, opts.NetMon, portMapOpts, opts.ControlKnobs, c.onPortMapChanged)
+	c.portMapper = portmapper.NewClient(portmapper.Config{
+		Logf:         portmapperLogf,
+		NetMon:       opts.NetMon,
+		DebugKnobs:   portMapOpts,
+		ControlKnobs: opts.ControlKnobs,
+		OnChange:     c.onPortMapChanged,
+	})
 	c.portMapper.SetGatewayLookupFunc(opts.NetMon.GatewayAndSelfIP)
 	c.netMon = opts.NetMon
 	c.health = opts.HealthTracker