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

Switch to forwarding based on URL and add TLS forwarding #108

Merged
merged 3 commits into from
Aug 29, 2023

Conversation

jrobsonchase
Copy link
Collaborator

Switches us from the explicit tcp, http, and pipe forwarding methods to
accepting a URL. This should make it easier for us to update what
protocols we support without having to change the public API.

Eventually, we'll also be able to forward smarter with regards to tls,
proxyproto, etc. like the agent, but that'll come later.

ngrok/src/tunnel_ext.rs Show resolved Hide resolved
@bobzilladev
Copy link
Contributor

bobzilladev commented Aug 29, 2023

I wanted to make sure this would keep working, so I've gone through the upgrade process for the mutable builders and the forwarding changes. I've got everything working except one test with a unix file without a full path specified. Trying to get to the bottom of that before +1'ing here. The WIP branch: ngrok/ngrok-javascript@main...bob/upgrade-ngrok-rust

@jrobsonchase
Copy link
Collaborator Author

Ah, @bobzilladev - I think your unix socket is missing a //. You've got format!("unix:{addr}"), which for addrs like /tmp/some.sock would give you unix:/tmp/some.sock, where you actually want unix:///tmp/some.sock. Though now that I'm reading this article, maybe yours is supposed to be equivalent. I'll fiddle with it on my end a bit to get those to be considered the same.

@bobzilladev
Copy link
Contributor

Ah, @bobzilladev - I think your unix socket is missing a //. You've got format!("unix:{addr}"), which for addrs like /tmp/some.sock would give you unix:/tmp/some.sock, where you actually want unix:///tmp/some.sock. Though now that I'm reading this article, maybe yours is supposed to be equivalent. I'll fiddle with it on my end a bit to get those to be considered the same.

Yeah, I tried that variation but it wasn't happy for the case that is unhappy, which is with a relative path. e.g. tunnel.sock used to work, creating a file in current working directory. Appears unix:tunnel.sock or unix://tunnel.sock seem to not be ok, but I need to play with it more.

@bobzilladev
Copy link
Contributor

Relative is probably just off the table. I'll have to do a little preprocessing to convert to absolute path.

@jrobsonchase
Copy link
Collaborator Author

OK, I think I got it fixed. Here's the playground where I fiddled with various formats.

@bobzilladev
Copy link
Contributor

The changes help a lot, relative unix pathing is good on nodejs again. A couple issues building on windows noted in comments. As you note in the rust playground, you get some odd results if you do pipe with a blank host, or no host and prepending forward slash. I think it's correct but we'll have to be explicit in documentation.

pipe:pipe_ngrok -> \\.\pipe\pipe_ngrok looks good!
pipe://./pipe_ngrok -> \\.\pipe\pipe_ngrok looks good!
pipe:/pipe_ngrok -> \\.\pipe\/pipe_ngrok wacky, but i guess correct
pipe:///pipe_ngrok -> \\.\pipe\/pipe_ngrok wacky, but i guess correct

@jrobsonchase
Copy link
Collaborator Author

OK, got it building. And I think I covered all of the unix socket and pipe variants in the forward doc comment.

Copy link
Contributor

@bobzilladev bobzilladev left a comment

Choose a reason for hiding this comment

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

Builds cleanly on windows and generates expected pipe names. WIP branch ngrok-nodejs branch passes all tests. lgtm.

@jrobsonchase jrobsonchase merged commit d5e36c1 into main Aug 29, 2023
11 checks passed
@jrobsonchase jrobsonchase deleted the josh/forward-urls-tls branch August 29, 2023 17:41
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.

3 participants