portforward: extract manual port forwarder to its own file

The manual port forwarder landed in peer/peer.go via #466 (commit a342889)
to support routers without UPnP. Move it to the portforward package
alongside the UPnP-based Forwarder so every portForwarder implementation
lives in one place.

Net zero functional change, just relocation:

  peer/peer.go
    - manualPortForwarder type + 4 method receivers
    - manualPort() env-parser helper
    - 'strconv' import (no longer needed)
    + NewForwarder closure now calls portforward.NewManualForwarder /
      portforward.ParseManualPort

  peer/peer_test.go
    - TestManualPort + TestManualPortForwarder (moved out of peer pkg)

  portforward/manual.go (new)
    + ManualForwarder + NewManualForwarder
    + ParseManualPort (the env-parser, factored out so callers can decide
      whether to log + fall through or treat as a hard error)
    + MapPort/UnmapPort/StartRenewal/ExternalIP methods
    + 'manual' method tag (was 'manual-env'; dropped the -env suffix
      since this implementation now serves both env and setting paths)

  portforward/manual_test.go (new)
    + TestParseManualPort (9 input cases — boundaries, invalid, empty)
    + TestManualForwarder (full portForwarder contract)

The peer package retains the portForwarder *interface* — that's where
peer expresses what it needs from a forwarder; the concrete implementations
live in portforward.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Author: myleshorton

peer/peer.go

@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"log/slog"
 	"math/rand/v2"
-	"strconv"
 	"sync"
 	"sync/atomic"
 	"time"
@@ -22,41 +21,6 @@ import (
 	"github.com/getlantern/radiance/portforward"
 )
 
-// manualPortForwarder satisfies the portForwarder interface without doing
-// any UPnP work. Used when env.PeerExternalPort is set.
-type manualPortForwarder struct{ port uint16 }
-
-func (m *manualPortForwarder) MapPort(_ context.Context, _ uint16, _ string) (*portforward.Mapping, error) {
-	return &portforward.Mapping{
-		ExternalPort: m.port,
-		InternalPort: m.port,
-		Method:       "manual-env",
-	}, nil
-}
-func (m *manualPortForwarder) UnmapPort(_ context.Context) error { return nil }
-func (m *manualPortForwarder) StartRenewal(_ context.Context)    {}
-func (m *manualPortForwarder) ExternalIP(_ context.Context) (string, error) {
-	// An empty external IP signals the server to use the address it
-	// observed on the inbound request — when the user has supplied a
-	// manual port but no WAN IP, the server's view is the right answer.
-	return "", nil
-}
-
-// manualPort returns the parsed env.PeerExternalPort value, or 0 if unset
-// or invalid.
-func manualPort() uint16 {
-	raw := env.GetString(env.PeerExternalPort)
-	if raw == "" {
-		return 0
-	}
-	p, err := strconv.Atoi(raw)
-	if err != nil || p < 1 || p > 65535 {
-		slog.Warn("ignoring invalid "+env.PeerExternalPort.String(), "value", raw)
-		return 0
-	}
-	return uint16(p)
-}
-
 // StatusEvent fires whenever the Client's session state changes — successful
 // Start, user Stop, or auto-Stop on a 404 heartbeat.
 type StatusEvent struct {
@@ -222,12 +186,18 @@ func NewClient(cfg Config) (*Client, error) {
 			if port := uint16(settings.GetInt(settings.PeerManualPortKey)); port != 0 {
 				slog.Info("peer client using manual port forward",
 					"port", port, "source", "setting")
-				return &manualPortForwarder{port: port}, nil
+				return portforward.NewManualForwarder(port), nil
 			}
-			if p := manualPort(); p != 0 {
-				slog.Info("peer client using manual port forward",
-					"port", p, "source", env.PeerExternalPort.String())
-				return &manualPortForwarder{port: p}, nil
+			if raw := env.GetString(env.PeerExternalPort); raw != "" {
+				port, err := portforward.ParseManualPort(raw)
+				if err != nil {
+					slog.Warn("ignoring invalid "+env.PeerExternalPort.String(),
+						"value", raw, "error", err)
+				} else {
+					slog.Info("peer client using manual port forward",
+						"port", port, "source", env.PeerExternalPort.String())
+					return portforward.NewManualForwarder(port), nil
+				}
 			}
 			// Explicitly return a nil interface on error — `return
 			// portforward.NewForwarder(ctx)` collapses the (*Forwarder, error)

peer/peer_test.go

@@ -627,58 +627,6 @@ func TestPickInternalPort_InRange(t *testing.T) {
 	}
 }
 
-// manualPort parses the RADIANCE_PEER_EXTERNAL_PORT env var. Unset, empty,
-// non-numeric, and out-of-range values all collapse to 0, which the
-// NewClient default factory treats as "no override → use UPnP discovery".
-// Only a 1..65535 value selects the manual path.
-func TestManualPort(t *testing.T) {
-	tests := []struct {
-		name string
-		env  string
-		want uint16
-	}{
-		{"unset", "", 0},
-		{"valid mid-range", "5698", 5698},
-		{"valid low boundary", "1", 1},
-		{"valid high boundary", "65535", 65535},
-		{"non-numeric", "abc", 0},
-		{"zero", "0", 0},
-		{"negative", "-5", 0},
-		{"above uint16", "65536", 0},
-		{"way above uint16", "99999", 0},
-	}
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			t.Setenv("RADIANCE_PEER_EXTERNAL_PORT", tc.env)
-			assert.Equal(t, tc.want, manualPort())
-		})
-	}
-}
-
-// manualPortForwarder must satisfy the portForwarder contract: MapPort
-// returns a Mapping using the configured port for both internal and
-// external (no rewrite — that's the user's responsibility), UnmapPort
-// and StartRenewal are no-ops, and ExternalIP returns "" so the server
-// substitutes the IP it observed on the request.
-func TestManualPortForwarder(t *testing.T) {
-	f := &manualPortForwarder{port: 5698}
-
-	m, err := f.MapPort(context.Background(), 30001, "ignored")
-	require.NoError(t, err)
-	assert.Equal(t, uint16(5698), m.ExternalPort)
-	assert.Equal(t, uint16(5698), m.InternalPort, "external==internal — user mapped them themselves")
-	assert.Equal(t, "manual-env", m.Method)
-
-	require.NoError(t, f.UnmapPort(context.Background()), "UnmapPort is a no-op for manual forwarders")
-
-	// StartRenewal must not panic or block.
-	f.StartRenewal(context.Background())
-
-	ip, err := f.ExternalIP(context.Background())
-	require.NoError(t, err)
-	assert.Empty(t, ip, "empty ip signals server to use observed source address")
-}
-
 func TestAPIError_StringFormat(t *testing.T) {
 	e := &APIError{Status: 422, Body: "could not connect to peer port"}
 	assert.Contains(t, e.Error(), "422")

portforward/manual.go

@@ -0,0 +1,71 @@
+package portforward
+
+import (
+	"context"
+	"fmt"
+	"strconv"
+)
+
+// ManualForwarder satisfies the portForwarder contract without talking
+// to a UPnP gateway. The user is expected to have configured a port
+// forward on their router by hand (single-port 1:1 NAT — every consumer
+// router exposes port forwarding as a single port number) and pointed
+// peer.Client at it via setting or env var.
+//
+// Use case: networks where UPnP is disabled or unavailable (router has
+// UPnP off for security, ISP-provided gateways without IGD, networks
+// behind double-NAT). UPnP-based Forwarder fails in those environments.
+type ManualForwarder struct {
+	port uint16
+}
+
+// NewManualForwarder builds a ManualForwarder for a pre-configured router
+// port forward. port must be a valid TCP port; callers should obtain it
+// from ParseManualPort (env-var path) or from a setting that already
+// constrains the value to uint16.
+func NewManualForwarder(port uint16) *ManualForwarder {
+	return &ManualForwarder{port: port}
+}
+
+// ParseManualPort parses a string into a TCP port number. Values outside
+// 1..65535 return an error so callers can log and fall through to UPnP
+// discovery rather than register a non-listening port with the server.
+func ParseManualPort(s string) (uint16, error) {
+	p, err := strconv.Atoi(s)
+	if err != nil {
+		return 0, fmt.Errorf("parse %q: %w", s, err)
+	}
+	if p < 1 || p > 65535 {
+		return 0, fmt.Errorf("port %d out of range (1..65535)", p)
+	}
+	return uint16(p), nil
+}
+
+// MapPort reports the configured port as both external and internal. The
+// router-side rule is already in place; nothing to do at the protocol
+// layer.
+func (m *ManualForwarder) MapPort(_ context.Context, _ uint16, _ string) (*Mapping, error) {
+	return &Mapping{
+		ExternalPort: m.port,
+		InternalPort: m.port,
+		Method:       "manual",
+	}, nil
+}
+
+// UnmapPort is a no-op: the user owns the router rule and is responsible
+// for removing it.
+func (m *ManualForwarder) UnmapPort(_ context.Context) error { return nil }
+
+// StartRenewal is a no-op: manually-configured rules don't carry a UPnP
+// lease and don't need refreshing.
+func (m *ManualForwarder) StartRenewal(_ context.Context) {}
+
+// ExternalIP returns the empty string deliberately. With a manual port
+// forward we have no UPnP gateway to ask for the WAN address, and
+// probing a public IP service from the client adds a network roundtrip
+// for information lantern-cloud already has — the server observes the
+// peer's source address on the register call and uses that as the
+// canonical external IP when this field is empty.
+func (m *ManualForwarder) ExternalIP(_ context.Context) (string, error) {
+	return "", nil
+}

portforward/manual_test.go

@@ -0,0 +1,66 @@
+package portforward
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// ParseManualPort accepts 1..65535 verbatim and rejects everything else
+// with an error so callers can log + fall through to UPnP discovery
+// rather than register a non-listening port with lantern-cloud.
+func TestParseManualPort(t *testing.T) {
+	tests := []struct {
+		name    string
+		input   string
+		want    uint16
+		wantErr bool
+	}{
+		{"valid mid-range", "5698", 5698, false},
+		{"valid low boundary", "1", 1, false},
+		{"valid high boundary", "65535", 65535, false},
+		{"empty", "", 0, true},
+		{"non-numeric", "abc", 0, true},
+		{"zero", "0", 0, true},
+		{"negative", "-5", 0, true},
+		{"above uint16", "65536", 0, true},
+		{"way above uint16", "99999", 0, true},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := ParseManualPort(tc.input)
+			if tc.wantErr {
+				assert.Error(t, err)
+				assert.Equal(t, uint16(0), got)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}
+
+// ManualForwarder satisfies the portForwarder contract: MapPort returns
+// a Mapping with external==internal port and the "manual" method tag,
+// UnmapPort and StartRenewal are no-ops, ExternalIP returns "" so the
+// server substitutes the IP it observed on the register call.
+func TestManualForwarder(t *testing.T) {
+	f := NewManualForwarder(5698)
+
+	m, err := f.MapPort(context.Background(), 30001, "ignored")
+	require.NoError(t, err)
+	assert.Equal(t, uint16(5698), m.ExternalPort)
+	assert.Equal(t, uint16(5698), m.InternalPort, "external==internal — user mapped them themselves")
+	assert.Equal(t, "manual", m.Method)
+
+	require.NoError(t, f.UnmapPort(context.Background()), "UnmapPort is a no-op for manual forwarders")
+
+	// StartRenewal must not panic or block.
+	f.StartRenewal(context.Background())
+
+	ip, err := f.ExternalIP(context.Background())
+	require.NoError(t, err)
+	assert.Empty(t, ip, "empty IP signals server to use observed source address")
+}