Skip to content
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

feat(twilio-run:start): options for ngrok config and named tunnel #177

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

philnash
Copy link
Contributor

This allows a user to provide a named tunnel and/or a custom ngrok config when starting twilio-run with the --ngrok flag.

These options are complementary but also work individually, like so:

--ngrok-config=path/to/config

This attempts to load and parse the file at the path supplied. If it does load, then the settings from the config, most importantly the authtoken are used in the tunnel config that is passed to ngrok.connect.

If the config file cannot be found at the path it will log a warning message and fallback to trying to find the config file in the default location (~/.ngrok2/ngrok.yml) and if that doesn't exist the command will continue as if there is no custom config at all.

--ngrok-name=tunnelName

Once the config file loading above has happened, including this flag will attempt to find a tunnel config in the config with the supplied name.

If the tunnel config is found, then the addr and proto settings are over-written (there's no point in tunneling over a different protocol or to a different port number). If a subdomain is passed in then that is over-written too.

If the tunnel config is not found then we log a warning and fallback to the default config.


Let me know if you think the fallbacks make sense, or if a tunnel or config file can't be found it should error out? I'd also like feedback on the flag names.

Before merging the flags should be added to plugin-serverless too.

Fixes #158.

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

Copy link
Member

@dkundel dkundel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just that I understand the difference between --ngrok=dk and --ngrok-name=dk both will be valid with this change but --ngrok=dk will create a tunnel under dk.ngrok.io while --ngrok-name=dk will be based on a name that I have in my ngrok tunnel? Could we add some links to the ngrok docs for named tunnels to avoid confusion?
Also what happens if a named tunnel has a fixed port number? shouldn't we override that? That otherwise the tunnel would be established but it wouldn't actually forward to this

packages/twilio-run/src/config/start.ts Outdated Show resolved Hide resolved
packages/twilio-run/src/config/start.ts Outdated Show resolved Hide resolved
@philnash
Copy link
Contributor Author

So just that I understand the difference between --ngrok=dk and --ngrok-name=dk both will be valid with this change but --ngrok=dk will create a tunnel under dk.ngrok.io while --ngrok-name=dk will be based on a name that I have in my ngrok tunnel? Could we add some links to the ngrok docs for named tunnels to avoid confusion?

I can definitely add links to the ngrok docs, but this is correct.

Also what happens if a named tunnel has a fixed port number? shouldn't we override that? That otherwise the tunnel would be established but it wouldn't actually forward to this

Currently this code over-rides port number and protocol type from the config (line 102). That's why I left a bunch of comments throughout this, but this does need to be documented for end users better too.

@philnash philnash requested a review from dkundel August 31, 2020 01:12
const ngrok = require('ngrok');
try {
// Try to open the ngrok tunnel.
url = await ngrok.connect(tunnelConfig);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not just use configPath option passed into ngrok.connect and avoid trying to manually parse out params for ngrok? We could definitely do a fs.exist(...) on the file to ensure presence, but we should not be required to fallback and load the default ngrok file stored in the home location (that's free by default).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want the ngrok tunnel to connect to the http server we're starting on the port we're starting it on. The config options could be trying to setup a tcp tunnel to a different port. So, I chose to parse the config and turn it into something that will work within the context of a twilio-run powered application.

Note, even if you create a named tunnel that points to port 3000, if we start twilio-run and something else is already running on port 3000 we offer a new random port number to start on instead. Parsing and manipulating the config in this way will result in a correct tunnel in that case, which would be the expected behaviour.

My idea here is that the tunnel we create will always point to the server we are running, even if it overrides the actual config, because that is what the user would expect to happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow ngrok configuration to be passed in
4 participants