nginxinc / nginx-ldap-auth

Example of LDAP authentication using ngx_http_auth_request_module
BSD 2-Clause "Simplified" License
681 stars 202 forks source link

Fix a flaky shebang handling issue #30

Closed Kegeruneku closed 6 years ago

Kegeruneku commented 6 years ago

Running nginx-ldap-auth daemon as-is yields the following results:

12:12 kegeruneku@arkhangelsk ~/repositories/other/nginx-ldap-auth % ./nginx-ldap-auth-daemon.py                
./nginx-ldap-auth-daemon.py: 2: ./nginx-ldap-auth-daemon.py: cannot create : Directory nonexistent
./nginx-ldap-auth-daemon.py: 3: ./nginx-ldap-auth-daemon.py: cannot create : Directory nonexistent
Traceback (most recent call last):
  File "./nginx-ldap-auth-daemon.py", line 287, in <module>
    server = AuthHTTPServer(Listen, LDAPAuthHandler)
  File "/usr/lib/python2.7/SocketServer.py", line 417, in __init__
    self.server_bind()
  File "/usr/lib/python2.7/BaseHTTPServer.py", line 108, in server_bind
    SocketServer.TCPServer.server_bind(self)
  File "/usr/lib/python2.7/SocketServer.py", line 431, in server_bind
    self.socket.bind(self.server_address)
  File "/usr/lib/python2.7/socket.py", line 228, in meth
    return getattr(self._sock,name)(*args)
socket.error: [Errno 98] Address already in use

I think it is a better idea to use env to select the python version to use, to prevent issues like this.

Moreover, as this project is advertised to support python 2 only, https://www.python.org/dev/peps/pep-0394/ recommends specifying 'python2' as interpreter

vl-homutov commented 6 years ago

Thanks for reporting.

The issue with running script as is was fixed, now it writes a log to stdout.

I prefer to leave shebang as is, it was made on purpose: there is a total mess in distributions, 'python' can point both to 2 or 3rd version, so we first try explicitly 'python2', and if then simply 'python' in hope it is not v3.

Also we expect python2 or python to be in PATH, and test using 'which', rather than /usr/bin/env, which is known to happen in different places (/bin vs /usr/bin) as well on some systems. It is hard to say what is more robust 'which' or 'env' without exhaustive testing of systems, but practically this works and I wouldn't change until some real evidence of current implementaion is broken.