daguej / node-proxywrap

Wrap node's Server interfaces to be compatible with the PROXY protocol
48 stars 36 forks source link

Also setting destination values #2

Open sadasant opened 11 years ago

sadasant commented 11 years ago

On the changes, besides the mentioned above, I changed remoteAddress and remotePort to sourceAddress and sourcePort respectively, which seemed to be more related to the original spec. Then I added destinationAddres and destinationPort.

sadasant commented 11 years ago

@daguej I think this is a lot cleaner, please review. There are two end of lines that I don't know how to put away from the commits :/

Any recommendation is appreciated!

daguej commented 11 years ago

Thanks, this is much clearer. :)

remoteAddress was so named because that's node's name for the property containing the client IP address.

The original goal of this module was to be a drop-in wrapper of node's native net module (and its subclasses, like http), preserving semantics.

In other words, let's assume an application is written to run on a single server. So to get the end-user IP address, it simply gets the address from socket.remoteAddress. A while later, the author finds his application is wildly successful and needs to scale beyond a single server, so it is moved behind an AWS ELB. Of course, the end-user IP is now lost, because the net module will only see the IP address of the ELB connecting to the server.

Ideally, an author needs only wrap a net module with this module, and everything else "just works," no code changes required. That is, code written expecting the real client IP in socket.remoteAddress continues behaving as before, when an end-user connected directly to the node server.

I like this approach since it is very low friction (requiring changing only two lines of code in an existing app), and I don't think anyone would need the 'physical' remoteAddress (that of the ELB).

So currently:

To be logically consistent, it seems like it might be most appropriate to replace socket.localAddress with the PROXY destination. (That is, if we're keeping up the illusion that the client is connecting directly to the node server.)

However, I'm torn between that and creating a new, third property (socket.publicAddress? originalAddress?) that contains the PROXY destination IP.

sadasant commented 11 years ago

Thanks a lot for the reply.

I like the way you make it as a direct drop-in replacement, however, the destination variables are indeed useful, in whichever form you'd like to include them. One way would be to provide a PROXY variable which could have all the PROXY-specific values in the right order, for example? That way, even if it's a clean replacement, it could provide the original information almost as received?