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