From dfc71fc6451a385363e3fac013c9f6543961cdb7 Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Wed, 19 Mar 2025 15:50:00 -0700 Subject: [PATCH] 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 --- cmd/tailscale/cli/netcheck.go | 5 ++- ipn/localapi/localapi.go | 28 +++++++++------- net/portmapper/igd_test.go | 11 +++++-- net/portmapper/portmapper.go | 55 +++++++++++++++++++------------ net/portmapper/portmapper_test.go | 6 ++-- wgengine/magicsock/magicsock.go | 8 ++++- 6 files changed, 73 insertions(+), 40 deletions(-) diff --git a/cmd/tailscale/cli/netcheck.go b/cmd/tailscale/cli/netcheck.go index 312475ece..14e337b89 100644 --- a/cmd/tailscale/cli/netcheck.go +++ b/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{ diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go index d1f07ea4e..5901855e3 100644 --- a/ipn/localapi/localapi.go +++ b/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() diff --git a/net/portmapper/igd_test.go b/net/portmapper/igd_test.go index 5c24d03aa..67d873c35 100644 --- a/net/portmapper/igd_test.go +++ b/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() diff --git a/net/portmapper/portmapper.go b/net/portmapper/portmapper.go index 71b55b8a7..b49a8f7bb 100644 --- a/net/portmapper/portmapper.go +++ b/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 } diff --git a/net/portmapper/portmapper_test.go b/net/portmapper/portmapper_test.go index d321b720a..c815f21d1 100644 --- a/net/portmapper/portmapper_test.go +++ b/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) diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index e8e966582..62658cac4 100644 --- a/wgengine/magicsock/magicsock.go +++ b/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