search

GUI / nginx-upstream-dynamic-servers

An nginx module to resolve domain names inside upstreams and keep them up to date.
MIT License
311 stars 74 forks source link

Solve segmentation faults and nginx reload issues #8

Closed wandenberg closed 8 years ago

GUI commented 8 years ago

Thanks so much, this looks great! :star2:

It looks like you pinpointed the segfault issues, which is really fantastic. Out of curiosity, do you know if there was a reproducible way to trigger those segfaults? Back when I was working on this, I did eventually hit some sporadic segfault issues, but I was never able to figure out how to reproduce them in a controlled way. Ideally, it would be nice to add some tests to the test suite for whatever situation triggers the segfaults to prevent this from regressing again. But if it's not easy to reproduce, then I'll take your word for it being fixed.

Also, it sounds like you've tested this against nginx 1.7.10. Have you by any chance tested against any other newer versions of nginx? I'm just wondering if your fixes may have also magically added compatibility with 1.8 or 1.9. But if you don't know off-hand, don't worry about it. I'll try to get the test suite updated tomorrow and give those other versions of nignx a spin.

Thanks again for your fixes and contributions!

wandenberg commented 8 years ago

@GUI I developed the fixes on Nginx 1.8.0 and also run on 1.7.10. It's on my plans to make it compatible with 1.9.x also. To reproduce the segfaults I reload nginx configuration two or three times. I guess the same result can be achieved "changing the ip associated with a name". I'm not familiarized with the nginx tests in perl, because of that I do not automated this tests. I can try to do.

In time, what do you think in change the module directives to work in the same way nginx plus does? Using "server" instead of "dynamic_server" and setting a "resolve" attribute to give user control of which domain it want to be resolved. (this would be one of my next pull requests)

GUI commented 8 years ago

That's great to hear about 1.8 compatibility. I'm also happy to lend a hand getting the test suite updated, if that would help (I know Test::Nginx can have a bit of a learning curve). I can probably find some time in the next week to try and get it updated to test the reload handling and also test against multiple versions of nginx.

Regarding changing the API to use server, I think I had originally opted for dynamic_server to avoid any potential conflicts with nginx's default server implementation (since I didn't want this plugin to possibly impact any upstreams that were not interested in using the dynamic DNS handling). However, if you think it's possible to implement this plugin in such a way that it won't conflict with the normal server implementation and mimics the nginx plus API, I'm certainly open to that.