diff --git a/build.nims b/build.nims index 1eb7acbb..f1831020 100644 --- a/build.nims +++ b/build.nims @@ -78,20 +78,6 @@ task testIntegration, "Run integration tests": # test "testIntegration", params = "-d:chronicles_sinks=textlines[notimestamps,stdout],textlines[dynamic] " & # "-d:chronicles_enabled_topics:integration:TRACE" -task testNatPortMapping, "Run UPnP NAT integration test (requires miniupnpd container)": - buildBinary "storage", - outName = "storage", - params = "-d:chronicles_runtime_filtering -d:chronicles_log_level=TRACE" - putEnv("STORAGE_INTEGRATION_TEST_INCLUDES", "nat/testnatupnp.nim") - test "testIntegration", outName = "testIntegrationNat" - -task testNatPcpMapping, "Run PCP NAT integration test (requires miniupnpd container)": - buildBinary "storage", - outName = "storage", - params = "-d:chronicles_runtime_filtering -d:chronicles_log_level=TRACE" - putEnv("STORAGE_INTEGRATION_TEST_INCLUDES", "nat/testnatpcp.nim") - test "testIntegration", outName = "testIntegrationNatPcp" - task testNatNotReachable, "Run NAT not-reachable scenario (needs the image + podman-compose)": test "integration/nat/not-reachable/testnotreachable", outName = "testNatNotReachable" diff --git a/storage/nat.nim b/storage/nat.nim index cbda208e..da65e8b5 100644 --- a/storage/nat.nim +++ b/storage/nat.nim @@ -128,7 +128,9 @@ proc stop*(m: NatPortMapper) = proc isPortMapped*(m: NatPortMapper, port: Port): bool = m.activeTcpPort.isSome and m.activeTcpPort.get == port -method hasActiveMapping*(m: NatPortMapper): bool {.base, gcsafe.} = +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 proc announcePeerInfoAddrs*(discovery: Discovery, peerInfo: PeerInfo, udpPort: Port) = @@ -195,11 +197,11 @@ method handleNatStatus*( if dialBackAddr.isNone: warn "Got empty dialback address in AutoNat when node is NotReachable" - if m.hasActiveMapping(): + if m.hasMappingIds(): m.close() discovery.announceDirectAddrs(@[], udpPort = discoveryPort) - elif m.hasActiveMapping(): + elif m.hasMappingIds(): warn "Not Reachable with active port mapping. The port mapping will be deleted and relay will start." # The mapping was created the the node is still not reachable. diff --git a/storage/storage.nim b/storage/storage.nim index a7c1b2c6..dfe0fb12 100644 --- a/storage/storage.nim +++ b/storage/storage.nim @@ -462,6 +462,10 @@ proc new*( # response can also carry loopback/private addresses: # they are never dialable by a remote peer, so drop them. let publicAddrs = addresses.filterIt(it.hasPublicRelayTransport()) + if publicAddrs.len == 0: + warn "Relay reservation has no publicly dialable address, keeping previous announce", + addresses + return info "Relay reservation updated", addresses = publicAddrs # relay addresses are for download traffic only, not DHT routing discovery.announceRelayAddrs(publicAddrs), diff --git a/tests/integration/nathelper.nim b/tests/integration/nathelper.nim deleted file mode 100644 index 8436cd7d..00000000 --- a/tests/integration/nathelper.nim +++ /dev/null @@ -1,65 +0,0 @@ -import std/algorithm -import std/json -import std/sequtils -import pkg/chronos -import pkg/questionable/results - -import ./multinodes -import ./storageclient -import ./storageconfig - -const - RelayTimeout* = 30_000 - PollInterval* = 1_000 - NatScheduleInterval* = 5.seconds - -proc checkNatStatus*( - client: StorageClient, reachability: string, relayRunning: bool, clientMode: bool -) {.async.} = - check eventuallySafe( - block: - let info = (await client.info()).get - let nat = info["nat"] - let addrs = info["addrs"].getElems.mapIt(it.getStr) - let r = nat["reachability"].getStr() - let cm = nat["clientMode"].getBool() - let rr = nat["relayRunning"].getBool() - let ha = addrs.anyIt("p2p-circuit" in it) - let pm = nat["portMapping"].getStr() - let aa = info["announceAddresses"].getElems.mapIt(it.getStr) - - # Reachable nodes must announce their dialable (non-circuit) addrs to - # the DHT (peerInfo observer); relayed nodes must announce their - # circuit addrs (onReservation). - let announceOk = - if reachability == "Reachable": - aa.len > 0 and aa.sorted == addrs.filterIt("p2p-circuit" notin it).sorted - elif relayRunning: - aa.len > 0 and aa.allIt("p2p-circuit" in it) - else: - true - - # It is important to check all the conditions together to avoid race - # (new autonat iteration) - # So we add a checkoint for better debug in case of failures - checkpoint( - "reachability=" & r & " (want " & reachability & ")" & " clientMode=" & $cm & - " (want " & $clientMode & ")" & " relayRunning=" & $rr & " (want " & - $relayRunning & ")" & " p2p-circuit=" & $ha & " (want " & $relayRunning & ")" & - " portMapping=" & pm & " announceAddresses=" & $aa - ) - r == reachability and cm == clientMode and rr == relayRunning and - ha == relayRunning and announceOk, - timeout = RelayTimeout, - pollInterval = PollInterval, - ) - -proc checkReachable*(client: StorageClient) {.async.} = - await client.checkNatStatus("Reachable", relayRunning = false, clientMode = false) - -# Relay might be false when the mapping has been created for UPnP / TCP but -# Autonat didn't detect yet Reachable -proc checkNotReachable*(client: StorageClient, relayRunning = true) {.async.} = - await client.checkNatStatus( - "NotReachable", relayRunning = relayRunning, clientMode = true - ) diff --git a/tests/storage/testaddrutils.nim b/tests/storage/testaddrutils.nim index 94c3db76..d9a4e91e 100644 --- a/tests/storage/testaddrutils.nim +++ b/tests/storage/testaddrutils.nim @@ -3,6 +3,13 @@ import pkg/libp2p/multiaddress import ../asynctest import ../../storage/utils/addrutils +const relayId = "16Uiu2HAkyRvHo1AyyQY1xiHC8QbYjXCHkZbneVC8dBtJjp1SZcGD" + +proc circuitAddr(relayIp: string): MultiAddress = + MultiAddress + .init("/ip4/" & relayIp & "/tcp/8070/p2p/" & relayId & "/p2p-circuit") + .expect("valid") + suite "addrutils - getTcpPort": test "extracts port from ipv4 tcp address": let ma = MultiAddress.init("/ip4/1.2.3.4/tcp/5000").expect("valid") @@ -37,13 +44,6 @@ suite "addrutils - remapAddr": check remapped == MultiAddress.init("/ip4/8.8.8.8/tcp/5000").expect("valid") suite "addrutils - hasPublicRelayTransport": - const relayId = "16Uiu2HAkyRvHo1AyyQY1xiHC8QbYjXCHkZbneVC8dBtJjp1SZcGD" - - proc circuitAddr(relayIp: string): MultiAddress = - MultiAddress - .init("/ip4/" & relayIp & "/tcp/8070/p2p/" & relayId & "/p2p-circuit") - .expect("valid") - test "true when the relay has a public ip": check circuitAddr("204.168.234.45").hasPublicRelayTransport() @@ -54,13 +54,6 @@ suite "addrutils - hasPublicRelayTransport": check not circuitAddr("172.17.0.1").hasPublicRelayTransport() suite "addrutils - dialableAddressPolicy": - const relayId = "16Uiu2HAkyRvHo1AyyQY1xiHC8QbYjXCHkZbneVC8dBtJjp1SZcGD" - - proc circuitAddr(relayIp: string): MultiAddress = - MultiAddress - .init("/ip4/" & relayIp & "/tcp/8070/p2p/" & relayId & "/p2p-circuit") - .expect("valid") - test "keeps a public direct address": check MultiAddress .init("/ip4/204.168.234.45/tcp/8070") diff --git a/tests/storage/testnatreaction.nim b/tests/storage/testnatreaction.nim index 1f0d17ef..258f8606 100644 --- a/tests/storage/testnatreaction.nim +++ b/tests/storage/testnatreaction.nim @@ -26,7 +26,7 @@ method mapNatPorts*( .} = m.mappedPorts -method hasActiveMapping*(m: MockNatPortMapper): bool = +method hasMappingIds*(m: MockNatPortMapper): bool = m.activeMapping asyncchecksuite "NAT reaction - port mapping":