diff --git a/src/nodes/NodeConnectionManager.ts b/src/nodes/NodeConnectionManager.ts index 234cde2d2e..11135cec16 100644 --- a/src/nodes/NodeConnectionManager.ts +++ b/src/nodes/NodeConnectionManager.ts @@ -515,7 +515,7 @@ class NodeConnectionManager { // Check to see if any of these are the target node. At the same time, add // them to the shortlist for (const [nodeId, nodeData] of foundClosest) { - // Ignore any nodes that have been contacted + // Ignore a`ny nodes that have been contacted if (contacted[nodeId]) { continue; } @@ -688,6 +688,42 @@ class NodeConnectionManager { ); return nodeIds; } + + /** + * Checks if a connection can be made to the target. Returns true if the + * connection can be authenticated, it's certificate matches the nodeId and + * the addresses match if provided. Otherwise returns false. + * @param nodeId - NodeId of the target + * @param address - Optional address of the target + * @param connConnectTime - Optional timeout for making the connection. + */ + @ready(new nodesErrors.ErrorNodeConnectionManagerNotRunning()) + public async pingNode(nodeId: NodeId, address?: NodeAddress, connConnectTime?: number): Promise { + // If we can create a connection then we have punched though the NAT, + // authenticated and confimed the nodeId matches + let connAndLock: ConnectionAndLock; + try { + connAndLock = await this.createConnection(nodeId, address, connConnectTime); + } catch (e) { + if ( + e instanceof nodesErrors.ErrorNodeConnectionDestroyed || + e instanceof nodesErrors.ErrorNodeConnectionTimeout || + e instanceof grpcErrors.ErrorGRPC || + e instanceof agentErrors.ErrorAgentClientDestroyed + ) { + // failed to connect, returning false + return false; + } + throw e; + } + const remoteHost = connAndLock.connection?.host; + const remotePort = connAndLock.connection?.port; + // if address wasn't set then nothing to check + if (address == null) return true; + // check if the address information match in case there was an + // existing connection + return address.host === remoteHost && address.port === remotePort; + } } export default NodeConnectionManager; diff --git a/src/nodes/NodeManager.ts b/src/nodes/NodeManager.ts index d8f7617b2e..f24b707212 100644 --- a/src/nodes/NodeManager.ts +++ b/src/nodes/NodeManager.ts @@ -54,31 +54,12 @@ class NodeManager { /** * Determines whether a node in the Polykey network is online. * @return true if online, false if offline + * @param nodeId - NodeId of the node we're pinging + * @param address - Optional Host and Port we want to ping + * @param timeout - Optional timeout */ - // FIXME: We shouldn't be trying to find the node just to ping it - // since we are usually pinging it during the find procedure anyway. - // I think we should be providing the address of what we're trying to ping, - // possibly make it an optional parameter? - public async pingNode(targetNodeId: NodeId): Promise { - const targetAddress: NodeAddress = - await this.nodeConnectionManager.findNode(targetNodeId); - try { - // Attempt to open a connection via the forward proxy - // i.e. no NodeConnection object created (no need for GRPCClient) - await this.nodeConnectionManager.holePunchForward( - targetNodeId, - await networkUtils.resolveHost(targetAddress.host), - targetAddress.port, - ); - } catch (e) { - // If the connection request times out, then return false - if (e instanceof networkErrors.ErrorConnectionStart) { - return false; - } - // Throw any other error back up the callstack - throw e; - } - return true; + public async pingNode(nodeId: NodeId, address?: NodeAddress, timeout?: number): Promise { + return this.nodeConnectionManager.pingNode(nodeId, address, timeout); } /** diff --git a/tests/nodes/NodeConnectionManager.lifecycle.test.ts b/tests/nodes/NodeConnectionManager.lifecycle.test.ts index 867fe77b6a..8ec36dc66c 100644 --- a/tests/nodes/NodeConnectionManager.lifecycle.test.ts +++ b/tests/nodes/NodeConnectionManager.lifecycle.test.ts @@ -1,4 +1,4 @@ -import type { NodeId, NodeIdString, SeedNodes } from '@/nodes/types'; +import type { NodeAddress, NodeId, NodeIdString, SeedNodes } from '@/nodes/types'; import type { Host, Port } from '@/network/types'; import fs from 'fs'; import path from 'path'; @@ -97,12 +97,18 @@ describe(`${NodeConnectionManager.name} lifecycle test`, () => { remoteNode1 = await PolykeyAgent.createPolykeyAgent({ password, nodePath: path.join(dataDir2, 'remoteNode1'), + networkConfig: { + proxyHost: serverHost + }, logger: logger.getChild('remoteNode1'), }); remoteNodeId1 = remoteNode1.keyManager.getNodeId(); remoteNode2 = await PolykeyAgent.createPolykeyAgent({ password, nodePath: path.join(dataDir2, 'remoteNode2'), + networkConfig: { + proxyHost: serverHost + }, logger: logger.getChild('remoteNode2'), }); remoteNodeId2 = remoteNode2.keyManager.getNodeId(); @@ -521,4 +527,152 @@ describe(`${NodeConnectionManager.name} lifecycle test`, () => { await nodeConnectionManager?.stop(); } }); + + // new ping tests + test('should ping node', async () => { + // NodeConnectionManager under test + let nodeConnectionManager: NodeConnectionManager | undefined; + try { + nodeConnectionManager = new NodeConnectionManager({ + keyManager, + nodeGraph, + proxy, + logger: nodeConnectionManagerLogger, + }); + await nodeConnectionManager.start(); + // @ts-ignore: kidnap connections + const connections = nodeConnectionManager.connections; + await expect(nodeConnectionManager.pingNode(remoteNodeId1)).resolves.toBe(true); + const finalConnLock = connections.get( + remoteNodeId1.toString() as NodeIdString, + ); + // Check entry is in map and lock is released + expect(finalConnLock).toBeDefined(); + expect(finalConnLock?.lock.isLocked()).toBeFalsy(); + } finally { + await nodeConnectionManager?.stop(); + } + }); + test('should ping node with address', async () => { + // NodeConnectionManager under test + let nodeConnectionManager: NodeConnectionManager | undefined; + try { + nodeConnectionManager = new NodeConnectionManager({ + keyManager, + nodeGraph, + proxy, + logger: nodeConnectionManagerLogger, + }); + await nodeConnectionManager.start(); + const remoteNodeAddress1: NodeAddress = { + host: remoteNode1.proxy.getProxyHost(), + port: remoteNode1.proxy.getProxyPort(), + } + await nodeConnectionManager.pingNode(remoteNodeId1, remoteNodeAddress1); + } finally { + await nodeConnectionManager?.stop(); + } + }); + test('should ping node with address when connection exists', async () => { + // NodeConnectionManager under test + let nodeConnectionManager: NodeConnectionManager | undefined; + try { + nodeConnectionManager = new NodeConnectionManager({ + keyManager, + nodeGraph, + proxy, + logger: nodeConnectionManagerLogger, + }); + await nodeConnectionManager.start(); + const remoteNodeAddress1: NodeAddress = { + host: remoteNode1.proxy.getProxyHost(), + port: remoteNode1.proxy.getProxyPort(), + } + await nodeConnectionManager.withConnF(remoteNodeId1, nop); + await nodeConnectionManager.pingNode(remoteNodeId1, remoteNodeAddress1); + } finally { + await nodeConnectionManager?.stop(); + } + }); + test('should fail to ping non existent node', async () => { + // NodeConnectionManager under test + let nodeConnectionManager: NodeConnectionManager | undefined; + try { + nodeConnectionManager = new NodeConnectionManager({ + keyManager, + nodeGraph, + proxy, + logger: nodeConnectionManagerLogger, + }); + await nodeConnectionManager.start(); + + // Pinging node + expect(await nodeConnectionManager.pingNode( + remoteNodeId1, + {host: '127.1.2.3' as Host, port: 55555 as Port}, + 1000, + )).toEqual(false); + + } finally { + await nodeConnectionManager?.stop(); + } + }); + test('should fail to ping node with wrong address when connection exists', async () => { + // NodeConnectionManager under test + let nodeConnectionManager: NodeConnectionManager | undefined; + try { + nodeConnectionManager = new NodeConnectionManager({ + keyManager, + nodeGraph, + proxy, + logger: nodeConnectionManagerLogger, + }); + await nodeConnectionManager.start(); + await nodeConnectionManager.withConnF(remoteNodeId1, nop); + expect(await nodeConnectionManager.pingNode( + remoteNodeId1, + {host: '127.1.2.3' as Host, port: 55555 as Port}, + 1000, + )).toEqual(false); + + } finally { + await nodeConnectionManager?.stop(); + } + }); + test('should fail to ping node if NodeId does not match', async () => { + // NodeConnectionManager under test + let nodeConnectionManager: NodeConnectionManager | undefined; + try { + nodeConnectionManager = new NodeConnectionManager({ + keyManager, + nodeGraph, + proxy, + logger: nodeConnectionManagerLogger, + }); + await nodeConnectionManager.start(); + const remoteNodeAddress1: NodeAddress = { + host: remoteNode1.proxy.getProxyHost(), + port: remoteNode1.proxy.getProxyPort(), + } + const remoteNodeAddress2: NodeAddress = { + host: remoteNode2.proxy.getProxyHost(), + port: remoteNode2.proxy.getProxyPort(), + } + + expect(await nodeConnectionManager.pingNode( + remoteNodeId1, + remoteNodeAddress2, + 1000, + )).toEqual(false); + + expect(await nodeConnectionManager.pingNode( + remoteNodeId2, + remoteNodeAddress1, + 1000, + )).toEqual(false); + + } finally { + await nodeConnectionManager?.stop(); + } + }); });