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

Also setting destination values #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sadasant
Copy link

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.

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
Copy link
Author

@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
Copy link
Owner

daguej commented Aug 20, 2013

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:

  • socket.remoteAddress = PROXY "source"
  • socket.localAddress = the node server instance's IP address
  • The PROXY "destination" (in most cases, the load balancer's IP) is ignored

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
Copy link
Author

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?

@daguej daguej mentioned this pull request Mar 12, 2014
@daguej daguej added this to the v0.3.0 milestone Mar 12, 2014
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.

2 participants