From c34adae75457f5e6448bb64e2f4e55a8d45f919a Mon Sep 17 00:00:00 2001 From: Brian Botha Date: Fri, 25 Mar 2022 18:20:05 +1100 Subject: [PATCH] refactor: updated implementation of `nodePing` Added `nodePing` command to `NodeConnectionManager` and `NodeManager.nodePing` calls that now. the new `nodePing` command essentially attempts to create a connection to the target node. In this process we authenticate the connection and check that the nodeIds match. If no address is provided it will default to trying to find the node through kademlia. Related #322 --- src/nodes/NodeConnectionManager.ts | 38 ++++- src/nodes/NodeManager.ts | 29 +--- .../NodeConnectionManager.lifecycle.test.ts | 156 +++++++++++++++++- 3 files changed, 197 insertions(+), 26 deletions(-) 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(); + } + }); });