nginx-buildpack icon indicating copy to clipboard operation
nginx-buildpack copied to clipboard

RFC major rewrite

Open agriffis opened this issue 10 years ago • 3 comments

This pull request is NOT ready to be pulled. It breaks backward compatibility for existing users, and it diverges significantly from how this buildpack has historically worked. This pull request is for the sake of discussion leading to a series of sensible commits.

Major differences:

  • NGINX is built during initial deployment, rather than using a prebuilt binary. The binary is stored in CACHE_DIR between deploys with a version check to force rebuild. Alternatively the user can set NGINX_FORCE_BUILD to ignore the cached binary.
  • NGINX logs are sent directly to stderr rather than to the filesystem. This results in faster log delivery to Heroku logplex, and doesn't gradually fill the filesystem. This is accomplished using a small patch to NGINX since it's not supported upstream. (Upstream assumes you can use /dev/stderr as an output file, but Heroku makes this impossible, see http://stackoverflow.com/questions/22694530)
  • PORT is overridden to refer to the UNIX domain socket, so the application can detect whether it's running under NGINX or directly. This is especially because NGINX can be enabled or disabled at run-time by toggling NGINX_ENABLED.
  • NGINX, PCRE and ngx_headers_more are all updated to the latest versions, and the user can override the versions by setting config variables.
  • Signals are handled gracefully by the wrapper script, allowing both the app and NGINX to stop properly on dyno shutdown.
  • Coordinated dyno start depends on the appearance of the UNIX domain socket rather than a separate file. This can be configured with NGINX_WAIT_ENABLED (default 1) and NGINX_WAIT_FILE (defaults to the value of NGINX_SOCKET which defaults to /tmp/nginx-socket)
  • The scripts are written in a style that allows then to be sourced into the current shell without executing, so that individual functions can be called for development and debugging.

Known breakage:

  • The main breakage is the change in dyno startup coordination. I can't see the point of the extra file.
  • Not sure if anybody cares about the prebuilt vs. building change. I don't like using prebuilt binaries, mainly for security/trust reasons.
  • The readme is written from the perspective of a fork that isn't expecting to be pulled back.
  • I killed the changelog. Need to undo that change, and update it, before pulling.
  • Removed client_body_timeout 5; from the default nginx.conf. Not clear what problem this is intended to solve.

agriffis avatar Mar 31 '14 17:03 agriffis

Our server doesn't support use of domain sockets.

jcf avatar Apr 04 '14 15:04 jcf

+1 customizable nginx config file

amacneil avatar Jun 28 '14 04:06 amacneil

@ryandotsmith If I put some work into rebasing this and resolving the issues I mentioned above, would you be interested in pulling it? IMHO the improvements here are pretty significant. I've been using my fork in production for the past couple years.

agriffis avatar Aug 20 '15 13:08 agriffis