diff --git a/library/storage_thread_requests/requests/node_debug_request.nim b/library/storage_thread_requests/requests/node_debug_request.nim index fe9bd389..8c7dec7d 100644 --- a/library/storage_thread_requests/requests/node_debug_request.nim +++ b/library/storage_thread_requests/requests/node_debug_request.nim @@ -9,7 +9,6 @@ import std/[options] import chronos import chronicles import codexdht/discv5/spr -import pkg/libp2p/protocols/connectivity/autonat/service import ../../alloc import ../../../storage/conf import ../../../storage/rest/json @@ -60,7 +59,13 @@ proc getDebug( if node.discovery.dhtRecord.isSome: node.discovery.dhtRecord.get.toURI else: "", "announceAddresses": node.discovery.announceAddrs, "table": table, - "nat": {"reachability": $storage[].autonatService.networkReachability}, + "nat": { + "reachability": + if storage[].autonatService.isSome: + $storage[].autonatService.get.networkReachability + else: + "unknown" + }, } return ok($json) diff --git a/storage/discovery.nim b/storage/discovery.nim index 36c50b10..d9faf162 100644 --- a/storage/discovery.nim +++ b/storage/discovery.nim @@ -200,6 +200,8 @@ proc updateRecords*( d.protocol.updateRecord(d.dhtRecord).expect("Should update SPR") proc updateAnnounceRecord*(d: Discovery, addrs: openArray[MultiAddress]) = + # Updates announce addresses only, not the DHT routing record. + # Relay addresses should not pollute DHT routing. d.announceAddrs = @addrs info "Updating announce record", addrs = d.announceAddrs d.providerRecord = SignedPeerRecord @@ -254,7 +256,8 @@ proc new*( key: PrivateKey, bindIp = IPv4_any(), bindPort = 0.Port, - announceAddrs: openArray[MultiAddress], + announceAddrs: openArray[MultiAddress] = [], + discoveryPort = 0.Port, bootstrapNodes: openArray[SignedPeerRecord] = [], store: Datastore = SQLiteDatastore.new(Memory).expect("Should not fail!"), ): Discovery = @@ -265,8 +268,7 @@ proc new*( key: key, peerId: PeerId.init(key).expect("Should construct PeerId"), store: store ) - # Update with empty values to get a valid SPR - self.updateRecords(@[], Port(0)) + self.updateRecords(announceAddrs, discoveryPort) # -------------------------------------------------------------------------- # FIXME disable IP limits temporarily so we can run our workshop. Re-enable diff --git a/storage/storage.nim b/storage/storage.nim index 2176210f..26aa205d 100644 --- a/storage/storage.nim +++ b/storage/storage.nim @@ -85,6 +85,9 @@ proc start*(s: StorageServer) {.async.} = if s.config.nat.hasExtIp: # extip means that we assume the IP is reachable # So we just take the first peer addr and remap it with extip to keep the port only + if s.storageNode.switch.peerInfo.addrs.len == 0: + raise + newException(StorageError, "extip is set but switch has no listen addresses") @[ s.storageNode.switch.peerInfo.addrs[0].remapAddr( ip = some(s.config.nat.extIp), port = none(Port) @@ -94,7 +97,7 @@ proc start*(s: StorageServer) {.async.} = # If extip is not set, we have 2 choices: # 1- Announce the peer addrs contains detected addresses on the machine. # 2- Wait for AutoNat - # The probleme with 1 is that you will certainly announce private addresses + # The problem with 1 is that you will certainly announce private addresses # and if you advertise a CID, you will advertise these private addresses. # TODO: DHT client mode #s.storageNode.switch.peerInfo.addrs @@ -124,7 +127,8 @@ proc stop*(s: StorageServer) {.async.} = notice "Stopping Storage node" - s.natMapper.close() + if s.natMapper != nil: + s.natMapper.close() var futures = @[ s.storageNode.switch.stop(), @@ -273,8 +277,9 @@ proc new*( discovery = Discovery.new( switch.peerInfo.privateKey, - announceAddrs = @[listenMultiAddr], + announceAddrs = @[], bindPort = config.discoveryPort, + discoveryPort = config.discoveryPort, bootstrapNodes = config.bootstrapNodes, store = discoveryStore, ) diff --git a/tests/storage/testnat.nim b/tests/storage/testnat.nim index 78cf07f1..f843acf2 100644 --- a/tests/storage/testnat.nim +++ b/tests/storage/testnat.nim @@ -1,9 +1,8 @@ -import std/[net, importutils] +import std/[net, importutils, envvars] import pkg/chronos import ../../storage/utils/natutils import pkg/libp2p/[multiaddress, multihash, multicodec] -import pkg/libp2p/protocols/connectivity/autonatv2/service except setup -import pkg/libp2p/protocols/connectivity/autonatv2/types +import pkg/libp2p/protocols/connectivity/autonat/types import pkg/libp2p/protocols/connectivity/relay/client as relayClientModule import pkg/libp2p/services/autorelayservice except setup @@ -15,7 +14,6 @@ import ../../storage/nat import ../../storage/discovery import ../../storage/rng import ../../storage/utils -import ../../storage/utils/addrutils privateAccess(NatMapper) @@ -42,7 +40,7 @@ method mapNatPorts*( ): Future[Option[(Port, Port)]] {.async: (raises: [CancelledError]), gcsafe.} = m.mappedPorts -suite "NatMapper.close": +suite "NAT - NatMapper.close": test "does nothing when no upnp mapping": let mapper = MockNatMapper( natConfig: NatConfig(hasExtIp: false, nat: NatAuto), @@ -65,30 +63,15 @@ suite "NatMapper.close": check device.deletedPorts == @[(Port(8080), NatIpProtocol.Tcp), (Port(8090), NatIpProtocol.Udp)] -suite "remapAddr": - test "replaces protocol tcp with udp": - let ma = MultiAddress.init("/ip4/1.2.3.4/tcp/5000").expect("valid") - let remapped = ma.remapAddr(protocol = some("udp"), port = some(Port(9000))) - check remapped == MultiAddress.init("/ip4/1.2.3.4/udp/9000").expect("valid") - - test "replaces only port, keeping protocol": - let ma = MultiAddress.init("/ip4/1.2.3.4/tcp/5000").expect("valid") - let remapped = ma.remapAddr(port = some(Port(9000))) - check remapped == MultiAddress.init("/ip4/1.2.3.4/tcp/9000").expect("valid") - - test "replaces only ip, keeping protocol and port": - let ma = MultiAddress.init("/ip4/1.2.3.4/tcp/5000").expect("valid") - let remapped = ma.remapAddr(ip = some(parseIpAddress("8.8.8.8"))) - check remapped == MultiAddress.init("/ip4/8.8.8.8/tcp/5000").expect("valid") - -asyncchecksuite "handleNatStatus": +asyncchecksuite "NAT - handleNatStatus": var sw: Switch var key: PrivateKey var disc: Discovery - let autoRelay = - AutoRelayService.new(1, relayClientModule.RelayClient.new(), nil, Rng.instance()) + var autoRelay: AutoRelayService setup: + autoRelay = + AutoRelayService.new(1, relayClientModule.RelayClient.new(), nil, Rng.instance()) key = PrivateKey.random(Rng.instance[]).get() disc = Discovery.new(key, announceAddrs = @[]) sw = newStandardSwitch() @@ -155,3 +138,22 @@ asyncchecksuite "handleNatStatus": check not autoRelay.isRunning check disc.announceAddrs == @[dialBack] + +suite "NAT - UPnP port mapping (requires NAT_TEST_UPNP=1)": + test "mapPorts and cleanup": + if getEnv("NAT_TEST_UPNP") != "1": + skip() + + let res = UpnpDevice.init() + check res.isOk + + let device = res.value + let ports = device.mapPorts(Port(8101), Port(8090)) + check ports.isSome + + let (tcp, udp) = ports.get() + check tcp == Port(8101) + check udp == Port(8090) + + check device.deletePortMapping(Port(8101), NatIpProtocol.Tcp).isOk + check device.deletePortMapping(Port(8090), NatIpProtocol.Udp).isOk diff --git a/tests/storage/testnatutils.nim b/tests/storage/testnatutils.nim index 9eea951a..10de4d18 100644 --- a/tests/storage/testnatutils.nim +++ b/tests/storage/testnatutils.nim @@ -46,7 +46,7 @@ method addPortMapping*( else: err("mapping failed") -suite "UpnpDevice.init": +suite "NAT - UpnpDevice.init": test "returns err when discover fails": check MockUpnpDev(discoverOk: false).init().isErr @@ -65,7 +65,7 @@ suite "UpnpDevice.init": test "returns ok when IP not routable": check MockUpnpDev(discoverOk: true, igdResult: IGDIpNotRoutable).init().isOk -suite "UpnpDevice.mapPorts": +suite "NAT - UpnpDevice.mapPorts": test "returns none when addPortMapping fails": check MockUpnpDev(addPortMappingOk: false).mapPorts(Port(8080), Port(8090)).isNone @@ -82,7 +82,7 @@ suite "UpnpDevice.mapPorts": let d = MockUpnpDev(addPortMappingOk: true, failOnProto: some(NatIpProtocol.Udp)) check d.mapPorts(Port(8080), Port(8090)).isNone -suite "PmpDevice.mapPorts": +suite "NAT - PmpDevice.mapPorts": test "returns none when mapping fails": check MockPmpDev(addPortMappingOk: false).mapPorts(Port(8080), Port(8090)).isNone