From 825b6a97ebd3e500337940da67e49b61d6c327b0 Mon Sep 17 00:00:00 2001 From: Le Roux Bodenstein Date: Mon, 16 May 2022 13:14:52 +0100 Subject: [PATCH 01/15] reconnect the ssh tunnel when it gets disconnected --- packages/ssh-tunnel/src/index.ts | 175 +++++++++++++++++++------------ 1 file changed, 109 insertions(+), 66 deletions(-) diff --git a/packages/ssh-tunnel/src/index.ts b/packages/ssh-tunnel/src/index.ts index 419eacf87fc..44abdba9f9e 100644 --- a/packages/ssh-tunnel/src/index.ts +++ b/packages/ssh-tunnel/src/index.ts @@ -50,6 +50,9 @@ function getSshTunnelConfig(config: Partial): SshTunnelConfig { } export class SshTunnel extends EventEmitter { + private connected = false; + private stayConnected = false; + private connectingPromise?: Promise private connections: Set = new Set(); private server: any; private rawConfig: SshTunnelConfig; @@ -70,57 +73,16 @@ export class SshTunnel extends EventEmitter { this.sshClient = new SshClient(); + this.sshClient.on('close', () => { + debug('sshClient closed'); + this.connected = false; + }); + this.forwardOut = promisify(this.sshClient.forwardOut.bind(this.sshClient)); - // eslint-disable-next-line @typescript-eslint/no-misused-promises - this.server = socks5Server.createServer( - async ( - info: any, - accept: (intercept: true) => Socket, - deny: () => void - ) => { - debug('receiving socks5 forwarding request', info); - let socket: Socket | null = null; - - try { - const channel = await this.forwardOut( - info.srcAddr, - info.srcPort, - info.dstAddr, - info.dstPort - ); - debug('channel opened, accepting socks5 request', info); - - socket = accept(true); - this.connections.add(socket); - - socket.on('error', (err: ErrorWithOrigin) => { - debug('error on socksv5 socket', info, err); - err.origin = err.origin ?? 'connection'; - this.server.emit('error', err); - }); - - socket.once('close', () => { - debug('socksv5 socket closed, removing from set'); - this.connections.delete(socket as Socket); - }); - - socket.pipe(channel).pipe(socket); - } catch (err) { - debug('caught error, rejecting socks5 request', info, err); - deny(); - if (socket) { - (err as any).origin = 'ssh-client'; - socket.destroy(err as any); - } - } - } - ); + this.server = socks5Server.createServer(this.socks5Request.bind(this)); - if (!this.rawConfig.socks5Username) { - debug('skipping auth setup for this server'); - this.server.useAuth(socks5AuthNone()); - } else { + if (this.rawConfig.socks5Username) { this.server.useAuth( socks5AuthUserPassword( (user: string, pass: string, cb: (success: boolean) => void) => { @@ -133,6 +95,10 @@ export class SshTunnel extends EventEmitter { ) ); } + else { + debug('skipping auth setup for this server'); + this.server.useAuth(socks5AuthNone()); + } this.serverListen = promisify(this.server.listen.bind(this.server)); this.serverClose = promisify(this.server.close.bind(this.server)); @@ -159,24 +125,7 @@ export class SshTunnel extends EventEmitter { debug('starting to listen', { localAddr, localPort }); await this.serverListen(localPort, localAddr); - try { - debug('creating SSH connection'); - await Promise.race([ - once(this.sshClient, 'error').then(([err]) => { - throw err; - }), - (() => { - const waitForReady = once(this.sshClient, 'ready') as Promise<[void]>; - this.sshClient.connect(getConnectConfig(this.rawConfig)); - return waitForReady; - })(), - ]); - debug('created SSH connection'); - } catch (err) { - debug('failed to establish SSH connection', err); - await this.serverClose(); - throw err; - } + await this.connectSsh(true); } async close(): Promise { @@ -187,6 +136,7 @@ export class SshTunnel extends EventEmitter { // close error this.serverClose().catch((e) => e), this.closeSshClient(), + // TODO: shouldn't we close the open connections first since they depend on the others? this.closeOpenConnections(), ]); @@ -195,7 +145,55 @@ export class SshTunnel extends EventEmitter { } } + private async connectSsh(stayConnected = false): Promise { + this.stayConnected = stayConnected || this.stayConnected; + + if (this.connected) { + debug('already connected'); + return; + } + + if (this.connectingPromise) { + debug('reusing connectingPromise'); + return this.connectingPromise; + } + + if (!stayConnected) { + // A socks5 request could come in after we deliberately closed the connection. Don't reconnect in that case. + throw new Error('Disconnected.'); + } + + debug('creating SSH connection'); + const ac = new AbortController(); + + this.connectingPromise = Promise.race([ + once(this.sshClient, 'error', { signal: ac.signal }).then(([err]) => { + throw err; + }), + (() => { + const waitForReady = once(this.sshClient, 'ready', { signal: ac.signal }).then(() => { return; }); + this.sshClient.connect(getConnectConfig(this.rawConfig)); + return waitForReady; + })(), + ]); + + try { + await this.connectingPromise; + } catch (err) { + ac.abort(); // stop listening for 'ready' + debug('failed to establish SSH connection', err); + await this.serverClose(); + throw err; + } + + delete this.connectingPromise; + this.connected = true; + ac.abort(); // stop listening for 'error' + debug('created SSH connection'); + } + private async closeSshClient() { + this.stayConnected = false; // stop reconnecting once we close try { return once(this.sshClient, 'close'); } finally { @@ -212,6 +210,51 @@ export class SshTunnel extends EventEmitter { await Promise.all(waitForClose); this.connections.clear(); } + + private async socks5Request( + info: any, + accept: (intercept: true) => Socket, + deny: () => void + ): Promise { + debug('receiving socks5 forwarding request', info); + let socket: Socket | null = null; + + try { + await this.connectSsh(); + + const channel = await this.forwardOut( + info.srcAddr, + info.srcPort, + info.dstAddr, + info.dstPort + ); + debug('channel opened, accepting socks5 request', info); + + socket = accept(true); + this.connections.add(socket); + + socket.on('error', (err: ErrorWithOrigin) => { + debug('error on socksv5 socket', info, err); + err.origin = err.origin ?? 'connection'; + this.server.emit('error', err); + this.connections.delete(socket as Socket); + }); + + socket.once('close', () => { + debug('socksv5 socket closed, removing from set'); + this.connections.delete(socket as Socket); + }); + + socket.pipe(channel).pipe(socket); + } catch (err) { + debug('caught error, rejecting socks5 request', info, err); + deny(); + if (socket) { + (err as any).origin = 'ssh-client'; + socket.destroy(err as any); + } + } + } } export default SshTunnel; From 67e5419635155b81305673b284c4c184773b7d09 Mon Sep 17 00:00:00 2001 From: Le Roux Bodenstein Date: Mon, 16 May 2022 14:40:43 +0100 Subject: [PATCH 02/15] closed --- packages/ssh-tunnel/src/index.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/ssh-tunnel/src/index.ts b/packages/ssh-tunnel/src/index.ts index 44abdba9f9e..c4b3ca16112 100644 --- a/packages/ssh-tunnel/src/index.ts +++ b/packages/ssh-tunnel/src/index.ts @@ -51,7 +51,7 @@ function getSshTunnelConfig(config: Partial): SshTunnelConfig { export class SshTunnel extends EventEmitter { private connected = false; - private stayConnected = false; + private closed = false; private connectingPromise?: Promise private connections: Set = new Set(); private server: any; @@ -145,9 +145,7 @@ export class SshTunnel extends EventEmitter { } } - private async connectSsh(stayConnected = false): Promise { - this.stayConnected = stayConnected || this.stayConnected; - + private async connectSsh(): Promise { if (this.connected) { debug('already connected'); return; @@ -158,7 +156,7 @@ export class SshTunnel extends EventEmitter { return this.connectingPromise; } - if (!stayConnected) { + if (this.closed) { // A socks5 request could come in after we deliberately closed the connection. Don't reconnect in that case. throw new Error('Disconnected.'); } @@ -193,7 +191,7 @@ export class SshTunnel extends EventEmitter { } private async closeSshClient() { - this.stayConnected = false; // stop reconnecting once we close + this.closed = true; try { return once(this.sshClient, 'close'); } finally { From 98cdc7e052a56621d443b5eab7100a7c455f8485 Mon Sep 17 00:00:00 2001 From: Le Roux Bodenstein Date: Mon, 16 May 2022 14:42:22 +0100 Subject: [PATCH 03/15] rm comment --- packages/ssh-tunnel/src/index.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/ssh-tunnel/src/index.ts b/packages/ssh-tunnel/src/index.ts index c4b3ca16112..51bd7d3d7f9 100644 --- a/packages/ssh-tunnel/src/index.ts +++ b/packages/ssh-tunnel/src/index.ts @@ -136,7 +136,6 @@ export class SshTunnel extends EventEmitter { // close error this.serverClose().catch((e) => e), this.closeSshClient(), - // TODO: shouldn't we close the open connections first since they depend on the others? this.closeOpenConnections(), ]); From ccdd67c849b4a7547b8543908d4ffa514172b9e3 Mon Sep 17 00:00:00 2001 From: Le Roux Bodenstein Date: Mon, 16 May 2022 14:52:55 +0100 Subject: [PATCH 04/15] close event called whenever error event called --- packages/ssh-tunnel/src/index.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/ssh-tunnel/src/index.ts b/packages/ssh-tunnel/src/index.ts index 51bd7d3d7f9..ee131a5c51e 100644 --- a/packages/ssh-tunnel/src/index.ts +++ b/packages/ssh-tunnel/src/index.ts @@ -234,7 +234,6 @@ export class SshTunnel extends EventEmitter { debug('error on socksv5 socket', info, err); err.origin = err.origin ?? 'connection'; this.server.emit('error', err); - this.connections.delete(socket as Socket); }); socket.once('close', () => { From 422463d1dff5701c97ad7b84e902c964a8eb47a6 Mon Sep 17 00:00:00 2001 From: Le Roux Bodenstein Date: Mon, 16 May 2022 15:42:29 +0100 Subject: [PATCH 05/15] unused --- packages/ssh-tunnel/src/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ssh-tunnel/src/index.ts b/packages/ssh-tunnel/src/index.ts index ee131a5c51e..3e94195e58a 100644 --- a/packages/ssh-tunnel/src/index.ts +++ b/packages/ssh-tunnel/src/index.ts @@ -125,7 +125,7 @@ export class SshTunnel extends EventEmitter { debug('starting to listen', { localAddr, localPort }); await this.serverListen(localPort, localAddr); - await this.connectSsh(true); + await this.connectSsh(); } async close(): Promise { From f8dfb7fb42379313904b8b0329d30f170dc6f1c6 Mon Sep 17 00:00:00 2001 From: Le Roux Bodenstein Date: Tue, 17 May 2022 11:31:16 +0100 Subject: [PATCH 06/15] reconnect tests --- package-lock.json | 4 + packages/ssh-tunnel/.mocharc.js | 2 +- packages/ssh-tunnel/package.json | 2 + packages/ssh-tunnel/src/index.spec.ts | 110 +++++++++++++++++++++++++- packages/ssh-tunnel/src/index.ts | 16 +++- 5 files changed, 127 insertions(+), 7 deletions(-) diff --git a/package-lock.json b/package-lock.json index af3ffa4249e..46a86f08976 100644 --- a/package-lock.json +++ b/package-lock.json @@ -104088,12 +104088,14 @@ "@mongodb-js/prettier-config-compass": "^0.5.0", "@mongodb-js/tsconfig-compass": "^0.6.0", "@types/chai": "^4.2.21", + "@types/chai-as-promised": "^7.1.4", "@types/debug": "^4.1.7", "@types/mocha": "^9.0.0", "@types/node-fetch": "^2.5.8", "@types/sinon-chai": "^3.2.5", "@types/ssh2": "^0.5.46", "chai": "^4.3.4", + "chai-as-promised": "*", "depcheck": "^1.4.1", "eslint": "^7.25.0", "gen-esm-wrapper": "^1.1.0", @@ -137355,12 +137357,14 @@ "@mongodb-js/prettier-config-compass": "^0.5.0", "@mongodb-js/tsconfig-compass": "^0.6.0", "@types/chai": "^4.2.21", + "@types/chai-as-promised": "^7.1.4", "@types/debug": "^4.1.7", "@types/mocha": "^9.0.0", "@types/node-fetch": "^2.5.8", "@types/sinon-chai": "^3.2.5", "@types/ssh2": "^0.5.46", "chai": "^4.3.4", + "chai-as-promised": "*", "debug": "4.3.0", "depcheck": "^1.4.1", "eslint": "^7.25.0", diff --git a/packages/ssh-tunnel/.mocharc.js b/packages/ssh-tunnel/.mocharc.js index 7e473d17b76..30aecfb78c3 100644 --- a/packages/ssh-tunnel/.mocharc.js +++ b/packages/ssh-tunnel/.mocharc.js @@ -1 +1 @@ -module.exports = require('@mongodb-js/mocha-config-compass'); +module.exports = require('@mongodb-js/mocha-config-compass/react'); diff --git a/packages/ssh-tunnel/package.json b/packages/ssh-tunnel/package.json index 73ef28c6ff3..baf0667c16b 100644 --- a/packages/ssh-tunnel/package.json +++ b/packages/ssh-tunnel/package.json @@ -54,12 +54,14 @@ "@mongodb-js/prettier-config-compass": "^0.5.0", "@mongodb-js/tsconfig-compass": "^0.6.0", "@types/chai": "^4.2.21", + "@types/chai-as-promised": "^7.1.4", "@types/debug": "^4.1.7", "@types/mocha": "^9.0.0", "@types/node-fetch": "^2.5.8", "@types/sinon-chai": "^3.2.5", "@types/ssh2": "^0.5.46", "chai": "^4.3.4", + "chai-as-promised": "*", "depcheck": "^1.4.1", "eslint": "^7.25.0", "gen-esm-wrapper": "^1.1.0", diff --git a/packages/ssh-tunnel/src/index.spec.ts b/packages/ssh-tunnel/src/index.spec.ts index 9276e6e2cc6..16e3bf85442 100644 --- a/packages/ssh-tunnel/src/index.spec.ts +++ b/packages/ssh-tunnel/src/index.spec.ts @@ -1,4 +1,5 @@ /* eslint-disable @typescript-eslint/restrict-template-expressions */ +import { once } from 'events'; import type { ServerConfig } from 'ssh2'; import { Server as SSHServer } from 'ssh2'; import type { Server as HttpServer } from 'http'; @@ -7,12 +8,16 @@ import { promisify } from 'util'; import { readFileSync } from 'fs'; import path from 'path'; import { Socket } from 'net'; -import fetch from 'node-fetch'; +import fetch, { FetchError } from 'node-fetch'; import { expect } from 'chai'; import { SocksClient } from 'socks'; - +import chai from 'chai'; +import chaiAsPromised from 'chai-as-promised'; import type { SshTunnelConfig } from './index'; import SSHTunnel from './index'; +import sinon from 'sinon'; + +chai.use(chaiAsPromised); function sleep(ms: number) { return new Promise((resolve) => setTimeout(resolve, ms)); @@ -94,6 +99,7 @@ async function createTestSshTunnel(config: Partial = {}) { localPort: 0, ...config, }); + sinon.spy(sshTunnel.sshClient, 'connect'); await sshTunnel.listen(); } @@ -106,6 +112,14 @@ async function stopTestSshTunnel() { } } +async function breakSshTunnelConnection() { + try { + return once(sshTunnel.sshClient, 'close'); + } finally { + sshTunnel.sshClient.end(); + } +} + interface Socks5ProxyOptions { proxyHost: string; proxyPort: number; @@ -163,9 +177,13 @@ describe('SSHTunnel', function () { }); afterEach(async function () { + console.log('stopping ssh tunnel'); await stopTestSshTunnel(); + console.log('stopping ssh server'); await stopTestSshServer(); + console.log('stopping http server'); await stopTestHttpServer(); + console.log('done'); }); it('should be main export', function () { @@ -273,4 +291,92 @@ describe('SSHTunnel', function () { expect(sshTunnel.server.address()).to.equal(null); } }); + + it('does not reconnect if the tunnel is already connected', async function () { + await createTestSshTunnel(); + + const address = `http://localhost:${httpServer.address().port}/`; + const options = { + proxyHost: sshTunnel.config.localAddr, + proxyPort: sshTunnel.config.localPort, + }; + const expected = 'Hello from http server'; + + const res1 = await httpFetchWithSocks5(address, options); + expect(await res1.text()).to.equal(expected); + + const res2 = await httpFetchWithSocks5(address, options); + expect(await res2.text()).to.equal(expected); + + expect(sshTunnel.sshClient.connect.callCount).to.equal(1); + }); + + it('reconnects tunnel if it got accidentally disconnected', async function () { + await createTestSshTunnel(); + + await breakSshTunnelConnection(); + + const address = `http://localhost:${httpServer.address().port}/`; + const options = { + proxyHost: sshTunnel.config.localAddr, + proxyPort: sshTunnel.config.localPort, + }; + const expected = 'Hello from http server'; + + const res1 = await httpFetchWithSocks5(address, options); + expect(await res1.text()).to.equal(expected); + + const res2 = await httpFetchWithSocks5(address, options); + expect(await res2.text()).to.equal(expected); + + expect(sshTunnel.sshClient.connect.callCount).to.equal(2); + }); + + it('reuses the connection promise if a request comes in before the tunnel connects', async function () { + await createTestSshTunnel(); + + await breakSshTunnelConnection(); + + const address = `http://localhost:${httpServer.address().port}/`; + const options = { + proxyHost: sshTunnel.config.localAddr, + proxyPort: sshTunnel.config.localPort, + }; + const expected = 'Hello from http server'; + + const [res1, res2] = await Promise.all([ + httpFetchWithSocks5(address, options), + httpFetchWithSocks5(address, options), + ]); + expect(await res1.text()).to.equal(expected); + expect(await res2.text()).to.equal(expected); + + expect(sshTunnel.sshClient.connect.callCount).to.equal(2); + }); + + it('does not reconnect the tunnel after it was deliberately closed', async function () { + await createTestSshTunnel(); + + // NOTE: normally you'd call close(), but that also closes the server so + // you'd get a different error first. Trying to trigger the race condition + // where the request made it to the socks5 server in time. + await sshTunnel.closeSshClient(); + + const remotePort = httpServer.address().port; + const address = `http://localhost:${remotePort}/`; + + const options = { + proxyHost: sshTunnel.config.localAddr, + proxyPort: sshTunnel.config.localPort, + }; + + const promise = httpFetchWithSocks5(address, options); + + await expect(promise).to.be.rejectedWith( + FetchError, + `request to http://localhost:${remotePort}/ failed, reason: Socket closed` + ); + + expect(sshTunnel.sshClient.connect.callCount).to.equal(1); + }); }); diff --git a/packages/ssh-tunnel/src/index.ts b/packages/ssh-tunnel/src/index.ts index 3e94195e58a..3c5dd671294 100644 --- a/packages/ssh-tunnel/src/index.ts +++ b/packages/ssh-tunnel/src/index.ts @@ -52,7 +52,7 @@ function getSshTunnelConfig(config: Partial): SshTunnelConfig { export class SshTunnel extends EventEmitter { private connected = false; private closed = false; - private connectingPromise?: Promise + private connectingPromise?: Promise; private connections: Set = new Set(); private server: any; private rawConfig: SshTunnelConfig; @@ -94,8 +94,7 @@ export class SshTunnel extends EventEmitter { } ) ); - } - else { + } else { debug('skipping auth setup for this server'); this.server.useAuth(socks5AuthNone()); } @@ -168,7 +167,10 @@ export class SshTunnel extends EventEmitter { throw err; }), (() => { - const waitForReady = once(this.sshClient, 'ready', { signal: ac.signal }).then(() => { return; }); + // eslint-disable-next-line @typescript-eslint/no-empty-function + const waitForReady = once(this.sshClient, 'ready', { + signal: ac.signal, + }).then(() => {}); this.sshClient.connect(getConnectConfig(this.rawConfig)); return waitForReady; })(), @@ -190,7 +192,13 @@ export class SshTunnel extends EventEmitter { } private async closeSshClient() { + if (!this.connected) { + return; + } + + // don't automatically reconnect if another request comes in this.closed = true; + try { return once(this.sshClient, 'close'); } finally { From 6255cf433a02fd0022d057644e46b9a5ab1facce Mon Sep 17 00:00:00 2001 From: Le Roux Bodenstein Date: Tue, 17 May 2022 11:48:34 +0100 Subject: [PATCH 07/15] :lipstick: --- packages/ssh-tunnel/src/index.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/ssh-tunnel/src/index.ts b/packages/ssh-tunnel/src/index.ts index 3c5dd671294..72115b76a61 100644 --- a/packages/ssh-tunnel/src/index.ts +++ b/packages/ssh-tunnel/src/index.ts @@ -167,10 +167,9 @@ export class SshTunnel extends EventEmitter { throw err; }), (() => { - // eslint-disable-next-line @typescript-eslint/no-empty-function const waitForReady = once(this.sshClient, 'ready', { signal: ac.signal, - }).then(() => {}); + }).then(() => {}); // eslint-disable-line @typescript-eslint/no-empty-function this.sshClient.connect(getConnectConfig(this.rawConfig)); return waitForReady; })(), From c0f4202cccdf83c4fa681784bea098e07d0a686f Mon Sep 17 00:00:00 2001 From: Le Roux Bodenstein Date: Tue, 17 May 2022 12:38:49 +0100 Subject: [PATCH 08/15] remove AbortController. Too much effort for now. --- packages/ssh-tunnel/.mocharc.js | 2 +- packages/ssh-tunnel/src/index.spec.ts | 41 ++++++++++++++++++++++++--- packages/ssh-tunnel/src/index.ts | 40 +++++++++++++++++--------- 3 files changed, 65 insertions(+), 18 deletions(-) diff --git a/packages/ssh-tunnel/.mocharc.js b/packages/ssh-tunnel/.mocharc.js index 30aecfb78c3..7e473d17b76 100644 --- a/packages/ssh-tunnel/.mocharc.js +++ b/packages/ssh-tunnel/.mocharc.js @@ -1 +1 @@ -module.exports = require('@mongodb-js/mocha-config-compass/react'); +module.exports = require('@mongodb-js/mocha-config-compass'); diff --git a/packages/ssh-tunnel/src/index.spec.ts b/packages/ssh-tunnel/src/index.spec.ts index 16e3bf85442..ec7463559a0 100644 --- a/packages/ssh-tunnel/src/index.spec.ts +++ b/packages/ssh-tunnel/src/index.spec.ts @@ -177,13 +177,9 @@ describe('SSHTunnel', function () { }); afterEach(async function () { - console.log('stopping ssh tunnel'); await stopTestSshTunnel(); - console.log('stopping ssh server'); await stopTestSshServer(); - console.log('stopping http server'); await stopTestHttpServer(); - console.log('done'); }); it('should be main export', function () { @@ -379,4 +375,41 @@ describe('SSHTunnel', function () { expect(sshTunnel.sshClient.connect.callCount).to.equal(1); }); + + it('reconnects if the ssh connection times out while we try and open the channel', async function () { + await createTestSshTunnel(); + + const forwardOut = sshTunnel.forwardOut; + sinon + .stub(sshTunnel, 'forwardOut') + .callsFake(async function ( + srcAddr: string, + srcPort: number, + dstAddr: string, + dstPort: number + ) { + await breakSshTunnelConnection(); + const promise = forwardOut.call( + this, + srcAddr, + srcPort, + dstAddr, + dstPort + ); + sshTunnel.forwardOut.restore(); + return promise; + }); + + const address = `http://localhost:${httpServer.address().port}/`; + const options = { + proxyHost: sshTunnel.config.localAddr, + proxyPort: sshTunnel.config.localPort, + }; + const expected = 'Hello from http server'; + + const res = await httpFetchWithSocks5(address, options); + expect(await res.text()).to.equal(expected); + + expect(sshTunnel.sshClient.connect.callCount).to.equal(2); + }); }); diff --git a/packages/ssh-tunnel/src/index.ts b/packages/ssh-tunnel/src/index.ts index 72115b76a61..50e8af9a62c 100644 --- a/packages/ssh-tunnel/src/index.ts +++ b/packages/ssh-tunnel/src/index.ts @@ -160,16 +160,13 @@ export class SshTunnel extends EventEmitter { } debug('creating SSH connection'); - const ac = new AbortController(); this.connectingPromise = Promise.race([ - once(this.sshClient, 'error', { signal: ac.signal }).then(([err]) => { + once(this.sshClient, 'error').then(([err]) => { throw err; }), (() => { - const waitForReady = once(this.sshClient, 'ready', { - signal: ac.signal, - }).then(() => {}); // eslint-disable-line @typescript-eslint/no-empty-function + const waitForReady = once(this.sshClient, 'ready').then(() => {}); // eslint-disable-line @typescript-eslint/no-empty-function this.sshClient.connect(getConnectConfig(this.rawConfig)); return waitForReady; })(), @@ -178,7 +175,6 @@ export class SshTunnel extends EventEmitter { try { await this.connectingPromise; } catch (err) { - ac.abort(); // stop listening for 'ready' debug('failed to establish SSH connection', err); await this.serverClose(); throw err; @@ -186,7 +182,6 @@ export class SshTunnel extends EventEmitter { delete this.connectingPromise; this.connected = true; - ac.abort(); // stop listening for 'error' debug('created SSH connection'); } @@ -226,12 +221,31 @@ export class SshTunnel extends EventEmitter { try { await this.connectSsh(); - const channel = await this.forwardOut( - info.srcAddr, - info.srcPort, - info.dstAddr, - info.dstPort - ); + let channel; + try { + channel = await this.forwardOut( + info.srcAddr, + info.srcPort, + info.dstAddr, + info.dstPort + ); + } catch (err) { + // Assume that either we were already disconnected or we were in the process of + // disconnecting. Either way try and reconnect once then try and open + // the channel again. There are multiple different errors that can occur + // and rather than try and match exact messages it is probably safer to + // asume that any error is a worthy of a retry. + this.connected = false; + debug('error forwarding. retrying..', info, err); + await this.connectSsh(); + channel = await this.forwardOut( + info.srcAddr, + info.srcPort, + info.dstAddr, + info.dstPort + ); + } + debug('channel opened, accepting socks5 request', info); socket = accept(true); From f5bce6eb6643f35ba43677581e41331e8ec5b549 Mon Sep 17 00:00:00 2001 From: Le Roux Bodenstein Date: Tue, 17 May 2022 12:52:12 +0100 Subject: [PATCH 09/15] no need for try/finally --- packages/ssh-tunnel/src/index.spec.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/ssh-tunnel/src/index.spec.ts b/packages/ssh-tunnel/src/index.spec.ts index ec7463559a0..71fb2490ca8 100644 --- a/packages/ssh-tunnel/src/index.spec.ts +++ b/packages/ssh-tunnel/src/index.spec.ts @@ -112,12 +112,10 @@ async function stopTestSshTunnel() { } } -async function breakSshTunnelConnection() { - try { - return once(sshTunnel.sshClient, 'close'); - } finally { - sshTunnel.sshClient.end(); - } +function breakSshTunnelConnection() { + const promise = once(sshTunnel.sshClient, 'close'); + sshTunnel.sshClient.end(); + return promise; } interface Socks5ProxyOptions { From e996f3e7c17a7525ca3fb6110920c87213ccaa6f Mon Sep 17 00:00:00 2001 From: Le Roux Bodenstein Date: Tue, 17 May 2022 12:53:13 +0100 Subject: [PATCH 10/15] another unnecessary try/finally --- packages/ssh-tunnel/src/index.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/ssh-tunnel/src/index.ts b/packages/ssh-tunnel/src/index.ts index 50e8af9a62c..a64404b374d 100644 --- a/packages/ssh-tunnel/src/index.ts +++ b/packages/ssh-tunnel/src/index.ts @@ -193,11 +193,9 @@ export class SshTunnel extends EventEmitter { // don't automatically reconnect if another request comes in this.closed = true; - try { - return once(this.sshClient, 'close'); - } finally { - this.sshClient.end(); - } + const promise = once(this.sshClient, 'close'); + this.sshClient.end(); + return promise; } private async closeOpenConnections() { From f87b3ea163e3d45bddda7e18a9f3c2fddb61e286 Mon Sep 17 00:00:00 2001 From: Le Roux Bodenstein Date: Tue, 17 May 2022 12:55:12 +0100 Subject: [PATCH 11/15] :lipstick: --- packages/ssh-tunnel/src/index.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/ssh-tunnel/src/index.ts b/packages/ssh-tunnel/src/index.ts index a64404b374d..5eec3bfe6ba 100644 --- a/packages/ssh-tunnel/src/index.ts +++ b/packages/ssh-tunnel/src/index.ts @@ -228,11 +228,11 @@ export class SshTunnel extends EventEmitter { info.dstPort ); } catch (err) { - // Assume that either we were already disconnected or we were in the process of - // disconnecting. Either way try and reconnect once then try and open - // the channel again. There are multiple different errors that can occur - // and rather than try and match exact messages it is probably safer to - // asume that any error is a worthy of a retry. + // Assume that either we were already disconnected or we were in the + // process of disconnecting. Either way try and reconnect once then try + // and open the channel again. There are multiple different errors that + // can occur and rather than try and match exact messages it is probably + // safer to asume that any error is a worthy of a retry. this.connected = false; debug('error forwarding. retrying..', info, err); await this.connectSsh(); From 2e59137d3739c90fed670dd0cf48715925905534 Mon Sep 17 00:00:00 2001 From: Le Roux Bodenstein Date: Tue, 17 May 2022 12:56:01 +0100 Subject: [PATCH 12/15] :lipstick: --- packages/ssh-tunnel/src/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ssh-tunnel/src/index.ts b/packages/ssh-tunnel/src/index.ts index 5eec3bfe6ba..40d1568a269 100644 --- a/packages/ssh-tunnel/src/index.ts +++ b/packages/ssh-tunnel/src/index.ts @@ -232,7 +232,7 @@ export class SshTunnel extends EventEmitter { // process of disconnecting. Either way try and reconnect once then try // and open the channel again. There are multiple different errors that // can occur and rather than try and match exact messages it is probably - // safer to asume that any error is a worthy of a retry. + // safer to asume that any error is worthy of a retry. this.connected = false; debug('error forwarding. retrying..', info, err); await this.connectSsh(); From 3ccbd51af0bcebb7c7304b4c824c73803d3aa460 Mon Sep 17 00:00:00 2001 From: Le Roux Bodenstein Date: Tue, 17 May 2022 14:07:19 +0100 Subject: [PATCH 13/15] pass DEBUG --- .evergreen/connectivity-tests/run.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/.evergreen/connectivity-tests/run.sh b/.evergreen/connectivity-tests/run.sh index 213bbed0b2d..04895b027c2 100644 --- a/.evergreen/connectivity-tests/run.sh +++ b/.evergreen/connectivity-tests/run.sh @@ -14,6 +14,7 @@ echo running connectivity tests image docker run \ --rm \ + -e DEBUG="${DEBUG}" \ -e E2E_TESTS_ATLAS_HOST="${E2E_TESTS_ATLAS_HOST}" \ -e E2E_TESTS_DATA_LAKE_HOST="${E2E_TESTS_DATA_LAKE_HOST}" \ -e E2E_TESTS_ANALYTICS_NODE_HOST="${E2E_TESTS_ANALYTICS_NODE_HOST}" \ From 979b01bc4c4f6ac84331a4366d2b61f723b761c8 Mon Sep 17 00:00:00 2001 From: Le Roux Bodenstein Date: Tue, 17 May 2022 14:40:00 +0100 Subject: [PATCH 14/15] clear the cached promise even if we throw --- packages/ssh-tunnel/src/index.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/ssh-tunnel/src/index.ts b/packages/ssh-tunnel/src/index.ts index 40d1568a269..4fb2a8afe59 100644 --- a/packages/ssh-tunnel/src/index.ts +++ b/packages/ssh-tunnel/src/index.ts @@ -176,6 +176,7 @@ export class SshTunnel extends EventEmitter { await this.connectingPromise; } catch (err) { debug('failed to establish SSH connection', err); + delete this.connectingPromise; await this.serverClose(); throw err; } From dd2392c26f4f0c915d3b34a6b02e82c063c6130e Mon Sep 17 00:00:00 2001 From: Le Roux Bodenstein Date: Tue, 17 May 2022 14:44:04 +0100 Subject: [PATCH 15/15] leave other errors alone --- packages/ssh-tunnel/src/index.ts | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/packages/ssh-tunnel/src/index.ts b/packages/ssh-tunnel/src/index.ts index 4fb2a8afe59..e3e4c3573eb 100644 --- a/packages/ssh-tunnel/src/index.ts +++ b/packages/ssh-tunnel/src/index.ts @@ -229,20 +229,19 @@ export class SshTunnel extends EventEmitter { info.dstPort ); } catch (err) { - // Assume that either we were already disconnected or we were in the - // process of disconnecting. Either way try and reconnect once then try - // and open the channel again. There are multiple different errors that - // can occur and rather than try and match exact messages it is probably - // safer to asume that any error is worthy of a retry. - this.connected = false; - debug('error forwarding. retrying..', info, err); - await this.connectSsh(); - channel = await this.forwardOut( - info.srcAddr, - info.srcPort, - info.dstAddr, - info.dstPort - ); + if ((err as Error).message === 'Not connected') { + this.connected = false; + debug('error forwarding. retrying..', info, err); + await this.connectSsh(); + channel = await this.forwardOut( + info.srcAddr, + info.srcPort, + info.dstAddr, + info.dstPort + ); + } else { + throw err; + } } debug('channel opened, accepting socks5 request', info);