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

Unix socket support #104

Closed
jimisaacs opened this issue Sep 8, 2011 · 22 comments
Closed

Unix socket support #104

jimisaacs opened this issue Sep 8, 2011 · 22 comments

Comments

@jimisaacs
Copy link

So I wanted to use node-http-proxy to serve content from multiple apps running locally on unix sockets.
I found out quick that this was not going to work:

var proxyServer = httpProxy.createServer({
    router: {
        'localhost': '/tmp/myapp.sock'
    }
});

I spent the next 5 hours or so trying to figure out why.
It turns out the problem was in this method:

ProxyTable.prototype.getProxyLocation

That method is always looking for a host and a port, because it does a split on the string. It could easily be modified to use a unix socket path, which is the first argument for the net.Server.listen method, the same as a port. The core lib simply checks if this value is a number or a string.

So anyway, rather than modifying the code because I didn't want to monkey around in there just yet. The simple solution after 5 hours of trying to make this reverse proxy unix sockets was this:

var proxyServer = httpProxy.createServer({
    router: {
        'localhost': ':/tmp/myapp.sock'
    }
});

That my friends, is one magical colon.

Thanks again for this great and extremely useful module. Let me know what you think about all this, I can try to contribute.

Jim

@Floby
Copy link

Floby commented Sep 8, 2011

I had the same problem. I was expecting this to work since it works seamlessly in node core. Anyway, @jimisaacs solution is working right now.

@jimisaacs
Copy link
Author

Here is a suggestion for a possible change:

var proxyServer = httpProxy.createServer({
    router: {
        'app1.local': [8080, localhost]
      , 'app2.local': [null, 'localhost'] // null defaults to 80
      , 'app3.local': [8181] // port as number treated normally
      , 'app4.local': ['/tmp/myapp.sock'] // port as a string is treated as unix socket
    }
});

Whether this is actually proxying other node servers or not, this setup still simply looks like arguments for other servers' listen methods.

@tj
Copy link
Contributor

tj commented Sep 8, 2011

@jimisaacs +1, just ports too, defaulting the host would be nice & a very common use-case for people just wanting cheap vhost-like behaviour

httpProxy.createServer({
  router: {
     'foo.bar': 3001
   , 'bar.baz': 3002
  }
})

etc

@Floby
Copy link

Floby commented Sep 8, 2011

I gave this solution a try and started a pull request thread

@Floby
Copy link

Floby commented Sep 8, 2011

By the way I +1 @visionmedia proposal too.

@indexzero
Copy link
Contributor

@Floby How does your pull request support UNIX sockets?

@indexzero
Copy link
Contributor

This is a duplicate of #79. I'm closing #79 because I think your description is more astute.

@Floby
Copy link

Floby commented Sep 8, 2011

errr, I inverted host and port. I updated the pull request.
You should be able to use unix sockets by doing things like this:

httpProxy.createServer({
  router: {
    'unix-socket.com': ['/tmp/app.sock'],
    'usual.com':       [8094, '127.0.0.1']
  }
})

String targets still work. I haven't found a simple way to add the proper testing though since it seems that you can't use UNIX sockets with Mickeal's request module.

@jimisaacs
Copy link
Author

+1 Floby's pull request.

@jimisaacs jimisaacs reopened this Sep 8, 2011
@jimisaacs
Copy link
Author

Sorry, I think you can tell I don't actually comment very much on github.
I'm not a noob I swear ;)

@Floby
Copy link

Floby commented Sep 8, 2011

added testing. Some testing helpers were acting funny but it works now.

@indexzero
Copy link
Contributor

@Floby I want to accept your pull request, but there is a big set of changes coming down the pipeline for v0.7.x which optimizes the hot-paths by ~10%. I will most likely do sometime similar once that refactor is done.

I will leave this open as a reminder to do so.

@Floby
Copy link

Floby commented Sep 9, 2011

no problem. The magic colon is working for me for now.

@reekoheek
Copy link

Error while using magic colon for

  • node 0.6.6
  • node-http-proxy 0.8.0

I think there is new checking for host and port exists

/home/jafar/node_modules/http-proxy/lib/node-http-proxy/routing-proxy.js:243
    throw new Error('options.host and options.port or options.target are requi
          ^
Error: options.host and options.port or options.target are required.
    at [object Object]._getKey (/home/jafar/node_modules/http-proxy/lib/node-http-proxy/routing-proxy.js:243:11)
    at [object Object].proxyRequest (/home/jafar/node_modules/http-proxy/lib/node-http-proxy/routing-proxy.js:183:18)
    at Array.0 (/home/jafar/node_modules/http-proxy/lib/node-http-proxy.js:159:15)
    at Server.<anonymous> (/home/jafar/node_modules/http-proxy/lib/node-http-proxy.js:174:39)
    at Server.emit (events.js:70:17)
    at HTTPParser.onIncoming (http.js:1478:12)
    at HTTPParser.onHeadersComplete (http.js:102:31)
    at Socket.ondata (http.js:1374:22)
    at TCP.onread (net.js:348:27)

@esamattis
Copy link

Any update on this?

@brianloveswords
Copy link

I would also like this.

@indexzero
Copy link
Contributor

@brianloveswords I would like you to implement it ;)

@brianloveswords
Copy link

@indexzero I'll see if I can start throwing together a patch today!

@buschtoens
Copy link

@indexzero @brianloveswords Hey, guys. I don't want to be a party-crasher but this neat bug still isn't fixed yet.If you don't want to or can't fix it could you please point me to the lines of codes, that probably cause the problem? I'd then try to fix it myself and make a pull request.

Thanks!

@buschtoens
Copy link

Fixed it myself.
If you can't wait you can use this script which fixes the module: https://gist.github.com/2828473

Pull Request: #254
Thread on Stackoverflow: http://stackoverflow.com/a/10800727/420747

@indexzero
Copy link
Contributor

Fixed in #294

@buschtoens
Copy link

You can't imagine, how much I love you.

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

Successfully merging a pull request may close this issue.

8 participants