lua-nginx-module icon indicating copy to clipboard operation
lua-nginx-module copied to clipboard

feature: add tcpsock:bind api

Open doujiang24 opened this issue 9 years ago • 10 comments
trafficstars

Just like the standard proxy_bind directive, this api makes the outgoing connection to a upstream server originate from the specified local IP address.

doujiang24 avatar Mar 18 '16 06:03 doujiang24

I complied the patch and used some simple scripts to test them, everything looks good.

bind_patch.zip

(Not included in files) Additionally, I tested binding to a bad address, test failed (correct behavior)

JakSprats avatar May 08 '16 21:05 JakSprats

I'll look into this as soon as I can manage :)

agentzh avatar May 09 '16 20:05 agentzh

I do have one comment, but it is not directly related to sock:bind() but I found it while testing sock:bind(), so I am posting it here. This applies to all sock:* calls and is related to the luaL_error() call.

There is a different behavior for different types of errors.

For instance if I try to bind to the ip "A.B.C.D" I get a error as the 2nd return value of the call e.g. local ok, err = sock:bind("A.B.C.D");

Whereas if I put some garbage arguments on the end of the sock:bind() call e.g. local ok, err = sock:bind("127.0.0.1", 8888, 1, 2, 3); The error propagates directly to the logs/error.log level and the script is killed logs/error.log: 2016/05/11 13:24:26 [error] 3023#0: *1 lua entry thread aborted: runtime error: nginx/./locations/test.lua:23: ngx.socket bind: expecting at least 2 arguments (including the object) but seen 6 stack traceback: coroutine 0: [C]: in function 'bind'

It just seems like there should be only one error path

Anyways, not a big deal, but it's a problem I ran into, and this is my (lazy) way of documenting it :)

JakSprats avatar May 11 '16 20:05 JakSprats

@agentzh @JakSprats I'd like to add the IP_TRANSPARENT socket option. Per my understanding I just need to set pc->transparent and nginx will take of the rest

where would be a better location to set this flag? adding a parameter to udp.setpeername() / tcp.connect() or to this new bind() api ?

@JakSprats, just for my understanding, why did you choose to implement a separate bind() and not do it all in one go with additional args to 'connect()` ?

Thanks!

alonbg avatar Aug 22 '16 16:08 alonbg

@JakSprats We distinguish between bad argument errors due to API misuse and other kinds of errors. The former usually results in Lua exceptions so that the user cannot (easily) ignore it.

@alonbg I think we'd better add support for the transparent argument of the setoption method, similar to LuaSocket's same-name method:

http://w3.impa.br/~diego/software/luasocket/tcp.html#setoption

agentzh avatar Aug 22 '16 16:08 agentzh

@agentzh Thanks! btw, for your reference, some time ago, I opened an issue@LuaSockets for this.

alonbg avatar Aug 23 '16 11:08 alonbg

@agentzh Please, review and add this code into main branch. Almost two years have passed.

iseriser avatar Jul 18 '18 10:07 iseriser

love to see the bind option merged as well, it is really usefull

kiwina avatar Nov 24 '19 13:11 kiwina

What is the plan with this pull request? We would like to leverage bind() to ensure correct source IP for our requests.

djaara avatar Jan 28 '20 10:01 djaara

This pull request is now in conflict :(

mergify[bot] avatar Jun 26 '20 00:06 mergify[bot]