fix: Wait for the tunnel to close before finishing client close COMPASS-4474 #352

gribnoysup · 2021-02-24T11:01:53Z

Replaces ssh-tunnel implementation with @mongodb-js/ssh-tunnel and safely overrides driver close* to always wait for tunnel to close before resolving the callback. That way we can always be sure that tunnel is completely stopped before allowing for another connection to be open through a tunnel with the same port in e.g., Compass

* - I considered other options how to achieve that (for example, exposing tunnel with the client from the connect callback and then awaiting explicitly on it in data-service), but overriding close on client still feels like a better balance between how much stuff is exposed from the connection-model and "hackiness" of the implementation

…and fail) before server stops trying

mcasimir · 2021-02-26T10:13:56Z

lib/connect.js

+      let maybeTunnelError;
+
+      if (tunnel) {
+        tunnel.on('error', (tunnelError) => {


tunnel.once('error' ?

lib/connect.js

mcasimir · 2021-02-26T10:56:15Z

lib/connect.js

+            tunnel.close().then(
+              () => {
+                debug('ssh tunnel stopped');
+                cb(err);


cb(null)?

Now i get it, sorry .. this code is quite complex though .. i think we should give the promise version / or a more de-tangled approach a chance

mcasimir · 2021-02-26T10:58:20Z

lib/connect.js

@@ -12,7 +12,7 @@ const {
 const { MongoClient } = require('mongodb');
 const { parseConnectionString } = require('mongodb/lib/core');
 const Connection = require('./extended-model');
-const createSSHTunnel = require('./ssh-tunnel');
+const { default: SSHTunnel } = require('@mongodb-js/ssh-tunnel');


mcasimir · 2021-02-26T10:59:05Z

lib/connect.js

+                tunnel.close().then(
+                  () => {
+                    debug('ssh tunnel stopped');
+                    callbackOrPromise(clientError);


why we return an error here? Isn't this what is supposed to happen?

This case is for when client failed to connect. We wait for tunnel to close and only then resolve the callback with the original client error

mcasimir · 2021-02-26T11:06:05Z

lib/connect.js

+          }
+        });
+      }
+
      mongoClient.connect((err, _client) => {


now that the tunnel uses promises, what about having an async function connect() {} that we export with util.callbackify instead than manually converting all of the promises call manually?

This would not be a 1 to 1 replacement, connect method synchronously returns state emitter and additionally allows to pass a callback that will evaluate when connection happened, can't really replace this behavior with promises

mcasimir

I really think we should avoid changing the client, let's investigate first some alternative with client.once('close') for example

lib/connect.js

gribnoysup · 2021-02-26T13:21:04Z

After discussing F2F we decided to go with the exposing the tunnel from the connect method option

…rough connect instead of overriding client.close method

gribnoysup

Re-implemented as discussed, want to give it another look?

gribnoysup · 2021-02-26T17:36:08Z

lib/connect.js

+        const _client = await Promise.race([
+          mongoClient.connect(),
+          waitForTunnelError
+        ]);


Not really happy with this bit here as we are not really waiting for MongoClient.connect to resolve if tunnel threw (this should not affect anything though, so I think it's okay as is), but that's more or less the only way to do it without doing weird callback stuff as it was in the previous implementation

Good point!

Are user actionable errors about the tunnel thrown by .listen?

The one thing I was assuming here is this: all the ssh related errors are thrown on tunnel.listen ie if a cert or password is invalid .... and the only issues catched by on('error') are not caused by user mistakes (ie. they picked a wrong file) but by programming bug or network conditions that are not actionable beyond a retry: exception when the tcp connection is opened by MongoClient.

If that is true i think we can even ignore the race with waitForTunnelError (just use it to make sure tunnel closes), since MongoClient will throw a connection error and we could just give a nice error in the UI: The tunnel was open correctly but the connection to mongodb failed: ${mongoError}.

For sure we could give more info but is super fine, we are making it connect when it should, which now does not, we already improve the user life immensely.

If however instead errors like wrong password can only be catched by waitForTunnelError than is a different story and probably we should wait for that as you did, or better make sure those errors are thrown by listen.

all the ssh related errors are thrown on tunnel.listen ie if a cert or password is invalid

This doesn't happen until first connection happens, tunnel is established for every incoming connection to ssh tunnel server, so things like forward out are just not happening before that. If we want them to be thrown when listen is called, some kind of ping request needs to be issued, which is doable. What do you think, should we add something like this here?

So in current implementation both errors emitted by tunnel or provided in callback can be helpful. But as mentioned, if tunnel emitted one, it probably will have a higher priority in terms of being useful to the user.

forward out are just not happening before that

When it gets to forward the connection to ssh should already be established, isn't it?

tunnel is established for every incoming connection to ssh

why? can't we just both create a new tunnel and 'forwarding tcp server' on listen instead?

why? can't we just both create a new tunnel and 'forwarding tcp server' on listen instead?

ssh2 docs don't recommend reusing one client for multiple connections, so @mongodb-js/ssh-tunnel creates new client for every incoming connection, which also means that client is not created until incoming connection is received

mcasimir

Looks great! For what i remember race may be returning an array, you may probably want to check that.

mcasimir · 2021-03-01T15:10:17Z

lib/connect.js

+        const _client = await Promise.race([
+          mongoClient.connect(),
+          waitForTunnelError
+        ]);


Good point!

Are user actionable errors about the tunnel thrown by .listen?

The one thing I was assuming here is this: all the ssh related errors are thrown on tunnel.listen ie if a cert or password is invalid .... and the only issues catched by on('error') are not caused by user mistakes (ie. they picked a wrong file) but by programming bug or network conditions that are not actionable beyond a retry: exception when the tcp connection is opened by MongoClient.

If that is true i think we can even ignore the race with waitForTunnelError (just use it to make sure tunnel closes), since MongoClient will throw a connection error and we could just give a nice error in the UI: The tunnel was open correctly but the connection to mongodb failed: ${mongoError}.

For sure we could give more info but is super fine, we are making it connect when it should, which now does not, we already improve the user life immensely.

If however instead errors like wrong password can only be catched by waitForTunnelError than is a different story and probably we should wait for that as you did, or better make sure those errors are thrown by listen.

mcasimir · 2021-03-01T15:34:49Z

lib/connect.js

-        cb(null, { url: model.driverUrlWithSsh, options: validOptions });
-      });
+      try {
+        const _client = await Promise.race([


isn't this: const [_client]? is there a test that verifies if we return a client and a tunnel?

Promise.race returns only one value (it races a bunch of them against each other and only one "wins", right?), so it's not an array.

We have a whole suite of tests for connect method and they are still passing, so client is returned correctly, this implementation doesn't change current behavior. I haven't added any tests for returning tunnel because I initially thought that this will require setting up a proper tunnel server to test the whole thing and it seemed like too much for a trivial test like that, but I can mock a bunch of things and write one for returning tunnel, yeah

Nice! makes a lot of sense :), could have checked myself.

lib/connect.js

addaleax

👍

lib/connect.js

Co-authored-by: Anna Henningsen <anna@addaleax.net>

fix: Wait for the tunnel to close before finishing client close

51f0a80

gribnoysup force-pushed the compass-4474-refactor-ssh-tunnel branch 2 times, most recently from ce94374 to 905c181 Compare February 24, 2021 16:24

fix: Update ssh-tunnel mock to match new implementation

e4c4576

gribnoysup force-pushed the compass-4474-refactor-ssh-tunnel branch 2 times, most recently from e97ccc6 to e4c4576 Compare February 26, 2021 08:46

gribnoysup added 2 commits February 26, 2021 09:50

fix: Only pick up errors that originated in ssh-client

61b0283

fix: Increase the timeout to make sure tunnel has a chance to start (…

5beed8c

…and fail) before server stops trying

mcasimir reviewed Feb 26, 2021

View reviewed changes

mcasimir suggested changes Feb 26, 2021

View reviewed changes

mcasimir reviewed Feb 26, 2021

View reviewed changes

lib/connect.js Outdated Show resolved Hide resolved

gribnoysup added the wip label Feb 26, 2021

refactor: Async-ify create tunnel and connect tasks; Expose tunnel th…

7e1ac00

…rough connect instead of overriding client.close method

gribnoysup removed the wip label Feb 26, 2021

gribnoysup requested a review from mcasimir February 26, 2021 17:32

gribnoysup commented Feb 26, 2021

View reviewed changes

mcasimir approved these changes Mar 1, 2021

View reviewed changes

refactor: Pass tunnel as function parameter

851db7e

addaleax approved these changes Mar 2, 2021

View reviewed changes

lib/connect.js Outdated Show resolved Hide resolved

gribnoysup and others added 3 commits March 4, 2021 16:41

fix: Bump ssh-tunnel version once more

97ebcd3

refactor: Asyncify awaiting on tunnel error

6158306

Co-authored-by: Anna Henningsen <anna@addaleax.net>

fix: Change spied close method

6a4c2b9

gribnoysup merged commit 299b632 into master Mar 4, 2021

gribnoysup deleted the compass-4474-refactor-ssh-tunnel branch March 4, 2021 16:25

gribnoysup mentioned this pull request Mar 4, 2021

fix: Use latest connection-model and keep reference to tunnel to be able to close it when disconnecting native client COMPASS-4474 mongodb-js/data-service#308

Merged

gribnoysup mentioned this pull request Jun 17, 2021

feat: bump mongosh APIs to latest VSCODE-240 mongodb-js/vscode#301

Merged

10 tasks

fix: Wait for the tunnel to close before finishing client close COMPASS-4474 #352

fix: Wait for the tunnel to close before finishing client close COMPASS-4474 #352

Uh oh!

Conversation

gribnoysup commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcasimir Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcasimir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gribnoysup commented Feb 26, 2021

Uh oh!

gribnoysup left a comment

Choose a reason for hiding this comment

Uh oh!

gribnoysup Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcasimir Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcasimir left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcasimir Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcasimir Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gribnoysup commented Feb 24, 2021 •

edited

Loading

mcasimir Feb 26, 2021 •

edited

Loading

gribnoysup Feb 26, 2021 •

edited

Loading

mcasimir Mar 1, 2021 •

edited

Loading

mcasimir left a comment •

edited

Loading

mcasimir Mar 1, 2021 •

edited

Loading

gribnoysup Mar 1, 2021 •

edited

Loading

mcasimir Mar 2, 2021 •

edited

Loading