Skip to content

feature: add tcpsock:bind api#712

Closed
doujiang24 wants to merge 9 commits into
openresty:masterfrom
coderxh:bind
Closed

feature: add tcpsock:bind api#712
doujiang24 wants to merge 9 commits into
openresty:masterfrom
coderxh:bind

Conversation

@doujiang24

Copy link
Copy Markdown
Member

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

xuhui and others added 9 commits March 17, 2016 15:16
@JakSprats

Copy link
Copy Markdown

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)

@agentzh

agentzh commented May 9, 2016

Copy link
Copy Markdown
Member

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

@JakSprats

Copy link
Copy Markdown

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 :)

@alonbg

alonbg commented Aug 22, 2016

Copy link
Copy Markdown

@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!

@agentzh

agentzh commented Aug 22, 2016

Copy link
Copy Markdown
Member

@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

@alonbg

alonbg commented Aug 23, 2016

Copy link
Copy Markdown

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

@iseriser

Copy link
Copy Markdown

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

@kiwina

kiwina commented Nov 24, 2019

Copy link
Copy Markdown

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

@djaara

djaara commented Jan 28, 2020

Copy link
Copy Markdown

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

@mergify

mergify Bot commented Jun 26, 2020

Copy link
Copy Markdown

This pull request is now in conflict :(

@mergify

mergify Bot commented Mar 20, 2023

Copy link
Copy Markdown

This pull request is now in conflict :(

@mergify mergify Bot added the conflict label Mar 20, 2023
@mergify mergify Bot removed the conflict label May 10, 2023
@mergify

mergify Bot commented May 10, 2023

Copy link
Copy Markdown

This pull request is now in conflict :(

@mergify mergify Bot added the conflict label May 10, 2023
@mergify mergify Bot removed the conflict label Sep 23, 2023
@mergify

mergify Bot commented Sep 23, 2023

Copy link
Copy Markdown

This pull request is now in conflict :(

@mergify mergify Bot added the conflict label Sep 23, 2023
@Scott41382

Scott41382 commented Sep 24, 2023 via email

Copy link
Copy Markdown

@alerque

alerque commented Nov 10, 2023

Copy link
Copy Markdown

@agentzh @alonbg Is anything in the linked LuaSockets issue still relevant? I'm one of the new maintainers but for that particular issue I'm a bit lost. I'm happy to facilitate if somebody has a contribution and concrete reason why it is needed, but there isn't enough to go on and move it forward as it stands. Just FYI.

@zhuizhuhaomeng

Copy link
Copy Markdown
Contributor

The feature has been merged from the command line. I forgot to close this PR.

@mergify mergify Bot removed the conflict label Nov 11, 2023
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.

10 participants