Refactor PortMapping object

This commit is contained in:
Arnaud 2026-06-17 20:27:58 +04:00
parent c300282437
commit 4bcabb5ec5
No known key found for this signature in database
GPG Key ID: A6C7C781817146FA
5 changed files with 116 additions and 89 deletions

View File

@ -34,6 +34,13 @@ type NatConfig* = object
of true: extIp*: IpAddress
of false: nat*: NatStrategy
type PortMapping* = object
tcpMappingId: cint
udpMappingId: cint
activeMappingProtocol*: MappingProtocol
activeTcpPort*: Port
activeUdpPort*: Port
type NatPortMapper* = ref object of RootObj
natConfig*: NatConfig
tcpPort*: Port
@ -41,11 +48,7 @@ type NatPortMapper* = ref object of RootObj
discoverTimeout*: int
mappingTimeout*: int
recheckPeriod*: int
tcpMappingId: Option[cint]
udpMappingId: Option[cint]
activeMappingProtocol*: Option[MappingProtocol]
activeTcpPort*: Option[Port]
activeUdpPort*: Option[Port]
portMapping*: Option[PortMapping]
plumInitialized: bool
closed: bool
@ -71,21 +74,21 @@ method createMappingFor*(
method destroyMappingFor*(m: NatPortMapper, id: cint) {.base, gcsafe.} =
destroyMapping(id)
method hasLiveMapping*(m: NatPortMapper, id: cint): bool {.base, gcsafe.} =
hasMapping(id)
method hasLivePortMapping*(m: NatPortMapper): bool {.base, gcsafe.} =
## True only when a mapping was created AND both the TCP and UDP mappings are
## still live in the router.
if m.portMapping.isNone:
return false
let pm = m.portMapping.get
hasMapping(pm.tcpMappingId) and hasMapping(pm.udpMappingId)
proc resetMappings(m: NatPortMapper) =
if m.tcpMappingId.isSome:
m.destroyMappingFor(m.tcpMappingId.get)
m.tcpMappingId = none(cint)
if m.udpMappingId.isSome:
m.destroyMappingFor(m.udpMappingId.get)
m.udpMappingId = none(cint)
m.activeMappingProtocol = none(MappingProtocol)
m.activeTcpPort = none(Port)
m.activeUdpPort = none(Port)
if m.portMapping.isSome:
let pm = m.portMapping.get
m.destroyMappingFor(pm.tcpMappingId)
m.destroyMappingFor(pm.udpMappingId)
m.portMapping = none(PortMapping)
method mapNatPorts*(
m: NatPortMapper
@ -95,11 +98,10 @@ method mapNatPorts*(
if m.closed or m.natConfig.hasExtIp:
return none((Port, Port, MappingProtocol))
# If both mappings are still active, return the stored ports without recreating.
if m.activeTcpPort.isSome and m.activeUdpPort.isSome and m.activeMappingProtocol.isSome and
m.tcpMappingId.isSome and m.hasLiveMapping(m.tcpMappingId.get) and
m.udpMappingId.isSome and m.hasLiveMapping(m.udpMappingId.get):
return some((m.activeTcpPort.get, m.activeUdpPort.get, m.activeMappingProtocol.get))
# If both mappings are still live, return the stored ports without recreating.
if m.hasLivePortMapping():
let pm = m.portMapping.get
return some((pm.activeTcpPort, pm.activeUdpPort, pm.activeMappingProtocol))
if not m.plumInitialized:
let res = m.initPlum()
@ -123,13 +125,18 @@ method mapNatPorts*(
m.destroyMappingFor(tcpRes.value.id)
return none((Port, Port, MappingProtocol))
m.tcpMappingId = some(tcpRes.value.id)
m.udpMappingId = some(udpRes.value.id)
m.activeMappingProtocol = some(tcpRes.value.mapping.mappingProtocol)
m.activeTcpPort = some(Port(tcpRes.value.mapping.externalPort))
m.activeUdpPort = some(Port(udpRes.value.mapping.externalPort))
m.portMapping = some(
PortMapping(
tcpMappingId: tcpRes.value.id,
udpMappingId: udpRes.value.id,
activeMappingProtocol: tcpRes.value.mapping.mappingProtocol,
activeTcpPort: Port(tcpRes.value.mapping.externalPort),
activeUdpPort: Port(udpRes.value.mapping.externalPort),
)
)
some((m.activeTcpPort.get, m.activeUdpPort.get, m.activeMappingProtocol.get))
let pm = m.portMapping.get
some((pm.activeTcpPort, pm.activeUdpPort, pm.activeMappingProtocol))
proc close*(m: NatPortMapper) =
m.resetMappings()
@ -143,14 +150,6 @@ proc stop*(m: NatPortMapper) =
m.closed = true
m.close()
proc isPortMapped*(m: NatPortMapper, port: Port): bool =
m.activeTcpPort.isSome and m.activeTcpPort.get == port
method hasMappingIds*(m: NatPortMapper): bool {.base, gcsafe.} =
# Only checks that mappings were created, not that they are still live
# (use hasMapping() for liveness check).
m.tcpMappingId.isSome and m.udpMappingId.isSome
method handleNatStatus*(
m: NatPortMapper,
networkReachability: NetworkReachability,
@ -174,13 +173,14 @@ method handleNatStatus*(
discovery.protocol.clientMode = false
discovery.announceDirectAddrs(
@[dialBackAddr.get], udpPort = m.activeUdpPort.get(discoveryPort)
)
# Here we don't rely on the port mapping because we consider
# that port mapped is the same as the discovery port.
# This can be wrong for PCP but it is an accepted limitation
discovery.announceDirectAddrs(@[dialBackAddr.get], udpPort = discoveryPort)
else:
warn "Empty dialback address in AutoNat when node is Reachable"
of NotReachable:
var hasPortMapping = false
var mappingCreated = false
discovery.protocol.clientMode = true
@ -189,9 +189,10 @@ method handleNatStatus*(
# If the relay is running, the addresses will be updated on reservation.
discovery.announceDirectAddrs(@[], udpPort = discoveryPort)
if m.hasMappingIds():
# The mapping was created but the node is still not reachable.
debug "Not Reachable with active port mapping, keeping it and starting relay if not started"
if m.hasLivePortMapping():
# The mapping is still live but the node is not reachable: keep it and let
# the relay take over. A dead mapping falls through to be recreated.
debug "Not Reachable with live port mapping, keeping it and starting relay if not started"
else:
debug "Node is not reachable trying port mapping now"
@ -202,16 +203,15 @@ method handleNatStatus*(
info "Port mapping created successfully", tcpPort, udpPort, protocol
# The address mapper uses the mapped port to build the candidate address
# for AutoNAT; the announce happens once AutoNAT confirms Reachable.
# The announce happens once AutoNAT confirms Reachable.
hasPortMapping = true
mappingCreated = true
else:
# In case of failure, close the port mapping in order to rerun discover
# on the next iteration
m.close()
if not hasPortMapping and not autoRelayService.isRunning:
if not mappingCreated and not autoRelayService.isRunning:
debug "No port mapping found let's start autorelay"
await autoRelayService.start(switch)
@ -224,9 +224,9 @@ proc reachabilityStr*(autonat: Option[AutonatV2Service]): string =
"Unknown"
proc portMappingStr*(natMapper: Option[NatPortMapper]): string =
if natMapper.isNone or natMapper.get.activeMappingProtocol.isNone:
if natMapper.isNone or natMapper.get.portMapping.isNone:
return "none"
case natMapper.get.activeMappingProtocol.get
case natMapper.get.portMapping.get.activeMappingProtocol
of MappingProtocol.UPnP: "upnp"
of MappingProtocol.NatPmp: "pmp"
of MappingProtocol.PCP: "pcp"

View File

@ -70,7 +70,8 @@ proc allowInbound(r: NatRouter, remote: TransportAddress, localPort: Port): bool
else:
discard
if r.natMapper.isSome and r.natMapper.get.isPortMapped(localPort):
if r.natMapper.isSome and r.natMapper.get.portMapping.isSome and
r.natMapper.get.portMapping.get.activeTcpPort == localPort:
return true
case r.filtering

View File

@ -50,9 +50,13 @@ method mapNatPorts*(
): Future[Option[(Port, Port, MappingProtocol)]] {.
async: (raises: [CancelledError]), gcsafe
.} =
m.activeTcpPort = some(mockMappedTcpPort)
m.activeUdpPort = some(mockMappedUdpPort)
m.activeMappingProtocol = some(MappingProtocol.PCP)
m.portMapping = some(
PortMapping(
activeMappingProtocol: MappingProtocol.PCP,
activeTcpPort: mockMappedTcpPort,
activeUdpPort: mockMappedUdpPort,
)
)
some((mockMappedTcpPort, mockMappedUdpPort, MappingProtocol.PCP))
# Captures the candidate addresses the service sends and answers Reachable, so

View File

@ -4,7 +4,6 @@ import pkg/libp2p/[multiaddress, multihash, multicodec]
import pkg/libp2p/protocols/connectivity/autonat/types
import pkg/libp2p/protocols/connectivity/relay/client as relayClientModule
import pkg/libp2p/services/autorelayservice except setup
import pkg/libp2p/observedaddrmanager
import pkg/results
import ./helpers
@ -38,8 +37,8 @@ type MockMapNatPortMapper = ref object of NatPortMapper
method initPlum(m: MockMapNatPortMapper): Result[void, string] {.gcsafe.} =
ok()
method hasLiveMapping(m: MockMapNatPortMapper, id: cint): bool {.gcsafe.} =
m.live
method hasLivePortMapping(m: MockMapNatPortMapper): bool {.gcsafe.} =
m.portMapping.isSome and m.live
method createMappingFor(
m: MockMapNatPortMapper, protocol: PlumProtocol, port: uint16
@ -116,12 +115,19 @@ asyncchecksuite "NAT reaction - port mapping":
check disc.announceAddrs == newSeq[MultiAddress]()
check disc.protocol.clientMode
test "handleNatStatus starts relay when NotReachable with an active mapping":
privateAccess(NatPortMapper)
test "handleNatStatus keeps a live mapping and starts relay when NotReachable":
privateAccess(PortMapping)
let dialBack = MultiAddress.init("/ip4/1.2.3.4/tcp/8080").expect("valid")
let mapper = MockNatPortMapper()
mapper.tcpMappingId = some(cint(1))
mapper.udpMappingId = some(cint(2))
let mapper = MockMapNatPortMapper(live: true)
mapper.portMapping = some(
PortMapping(
tcpMappingId: cint(1),
udpMappingId: cint(2),
activeMappingProtocol: MappingProtocol.UPnP,
activeTcpPort: Port(9000),
activeUdpPort: Port(9001),
)
)
autorelayservice.setup(autoRelay, sw)
await mapper.handleNatStatus(
@ -131,7 +137,35 @@ asyncchecksuite "NAT reaction - port mapping":
check autoRelay.isRunning
check disc.announceAddrs == newSeq[MultiAddress]()
check disc.protocol.clientMode
check mapper.hasMappingIds() # the active mapping is kept
check mapper.portMapping.isSome # the live mapping is kept
check mapper.destroyed.len == 0 # never torn down
test "handleNatStatus recreates a dead mapping instead of pinning it":
privateAccess(PortMapping)
let dialBack = MultiAddress.init("/ip4/1.2.3.4/tcp/8080").expect("valid")
let mapper = MockMapNatPortMapper(
live: false,
tcpResult: mappingOk(cint(10), 9000),
udpResult: mappingOk(cint(20), 9001),
)
mapper.portMapping = some(
PortMapping(
tcpMappingId: cint(1),
udpMappingId: cint(2),
activeMappingProtocol: MappingProtocol.UPnP,
activeTcpPort: Port(9000),
activeUdpPort: Port(9001),
)
)
autorelayservice.setup(autoRelay, sw)
await mapper.handleNatStatus(
NotReachable, Opt.some(dialBack), discoveryPort, disc, sw, autoRelay
)
check mapper.destroyed == @[cint(1), cint(2)] # the dead mapping is torn down
check mapper.portMapping.isSome # replaced by a fresh one
check not autoRelay.isRunning # direct path kept, no relay
test "handleNatStatus stops relay and exits client mode when mapping is created and node is Reachable":
let dialBack = MultiAddress.init("/ip4/1.2.3.4/tcp/8080").expect("valid")
@ -193,20 +227,6 @@ asyncchecksuite "NAT reaction - address announcing":
check disc.announceAddrs == @[dialBack]
test "handleNatStatus announces the mapped external UDP port when a mapping is active":
let dialBack = MultiAddress.init("/ip4/1.2.3.4/tcp/9000").expect("valid")
let mapper =
NatPortMapper(discoveryPort: discoveryPort, activeUdpPort: some(Port(40001)))
await mapper.handleNatStatus(
Reachable, Opt.some(dialBack), discoveryPort, disc, sw, autoRelay
)
let sprAddrs = disc.getSpr().data.addresses.mapIt(it.address)
check MultiAddress.init("/ip4/1.2.3.4/udp/40001").expect("valid") in sprAddrs
check MultiAddress.init("/ip4/1.2.3.4/udp/" & $discoveryPort).expect("valid") notin
sprAddrs
test "handleNatStatus does not announce when Reachable without a dial-back address":
let mapper = NatPortMapper(discoveryPort: discoveryPort)
await mapper.handleNatStatus(
@ -246,7 +266,7 @@ asyncchecksuite "NAT reaction - address announcing":
check disc.announceAddrs.len == 0
proc mapperWith(protocol: MappingProtocol): Option[NatPortMapper] =
some(NatPortMapper(activeMappingProtocol: some(protocol)))
some(NatPortMapper(portMapping: some(PortMapping(activeMappingProtocol: protocol))))
asyncchecksuite "NAT - portMappingStr":
test "no mapper is none":
@ -308,15 +328,17 @@ asyncchecksuite "NAT - mapNatPorts":
check mapper.createAttempts.len == 0 # short-circuits before any mapping
test "reuses the existing mapping when both are still live":
privateAccess(NatPortMapper)
let mapper = MockMapNatPortMapper(
live: true,
activeTcpPort: some(Port(9000)),
activeUdpPort: some(Port(9001)),
activeMappingProtocol: some(MappingProtocol.UPnP),
privateAccess(PortMapping)
let mapper = MockMapNatPortMapper(live: true)
mapper.portMapping = some(
PortMapping(
tcpMappingId: cint(1),
udpMappingId: cint(2),
activeMappingProtocol: MappingProtocol.UPnP,
activeTcpPort: Port(9000),
activeUdpPort: Port(9001),
)
)
mapper.tcpMappingId = some(cint(1))
mapper.udpMappingId = some(cint(2))
check (await mapper.mapNatPorts()) ==
some((Port(9000), Port(9001), MappingProtocol.UPnP))

View File

@ -143,7 +143,7 @@ asyncchecksuite "Nat transport - Double NAT":
test "bootstrap cannot connect to nat node regardless of port mapping":
let actualPort = initTAddress(natNode.peerInfo.addrs[0]).get().port
let natMapper = NatPortMapper()
natMapper.activeTcpPort = some(actualPort)
natMapper.portMapping = some(PortMapping(activeTcpPort: actualPort))
router.natMapper = some(natMapper)
check await cannotConnect(bootstrap, natNode)
@ -166,7 +166,7 @@ asyncchecksuite "Nat transport - Port Mapping":
test "bootstrap can connect to nat node when port mapping matches listen port":
let actualPort = initTAddress(natNode.peerInfo.addrs[0]).get().port
let natMapper = NatPortMapper()
natMapper.activeTcpPort = some(actualPort)
natMapper.portMapping = some(PortMapping(activeTcpPort: actualPort))
router.natMapper = some(natMapper)
await bootstrap.connect(natNode.peerInfo.peerId, natNode.peerInfo.addrs)
@ -174,7 +174,7 @@ asyncchecksuite "Nat transport - Port Mapping":
test "bootstrap cannot connect to nat node when port mapping does not match":
let natMapper = NatPortMapper()
natMapper.activeTcpPort = some(Port(1))
natMapper.portMapping = some(PortMapping(activeTcpPort: Port(1)))
router.natMapper = some(natMapper)
check await cannotConnect(bootstrap, natNode)