-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Automatic SSH tunneling with Deploy credentials file #99
Conversation
Awesome, working for me 👍 |
yusef, let's use ssh keys -- using passwords is a very bad idea. |
const Multihash = require('multihashes') | ||
const { JQ_PATH } = require('../../metadata/jqStream') | ||
const childProcess = require('child_process') | ||
const sshTunnel = require('tunnel-ssh') | ||
const yaml = require('js-yaml') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how did we end up with yaml, just the deploy default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that was mostly just laziness on my part. I'd be happy to change the deploy side to JSON, and possibly change the format so that it matches the options for tunnel-ssh more closely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to avoid config format proliferation this early in the process, even if yaml and JSON can technically the equivalent
? deployCredsToTunnelConfig(deployCredentialsFile) | ||
: null | ||
|
||
let sshTunnelPromise = Promise.resolve() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would put this in an else
block on 119 to make it clearer
sshTunnelPromise | ||
.then(() => handler(subcommandOptions)) | ||
.then(closeTunnel) | ||
.catch(err => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was going to say that you can just write this as .catch(err => ...).then(closeTunnel)
but I guess not if you want to rethrow... I wonder what byzantine reasons exist for finally
not making it into the spec
my only nitpick here is the use of Promise.resolve()
in what is clearly an error context (e.g. https://github.com/mediachain/aleph/pull/99/files#diff-d697977bdb0d2a7923a62aaffcc24d13R28)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I wasn't happy about that either. I think the right thing to do is have the command handlers just throw on error conditions and have the subcommand
helper be responsible for printing errors to the console, cleaning up and exiting the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
90c412a
to
75f8bfd
Compare
75f8bfd
to
6cd9062
Compare
This adds a
--deployCredentialsFile
option that you can point at the file downloaded at the end of the Deploy process. If given, it will create an SSH tunnel for you to the remote machine, using the ip address, username and password from the file. It uses a javascript SSH implementation, so it should work on Windows.To get this to work, I made a
subcommand
helper function that wraps each subcommand handler. If an SSH config is given, it creates the tunnel, runs the handler, then closes the tunnel afterwards. The subcommand handler functions had to be tweaked slightly to return aPromise
, which the wrapper waits on before closing the tunnel.I also made the
subcommand
helper create theRestClient
instance, so subcommands can just pull theclient
member out of their options object, instead of grabbingapiUrl
and creating aRestClient
. That may come in handy if we ever add more options to theRestClient
constructor, since we can just update it in one place.example:
For this to be truly great, I'd like to add persistent configuration to
mcclient
, so we can store the credentials in e.g.~/.mediachain/mcclient/config.json
. Then we wouldn't need to require the--deployCredentialsFile
option for every command. That seems like it belongs in its own PR though.Closes #92