heroku / heroku-buildpack-static

[DEPRECATED] Heroku buildpack for handling static sites and single page web apps
MIT License
673 stars 427 forks source link

Using a hostname rather than a URL in 'proxies' results in "[emerg] invalid number of arguments in "set" directive" #189

Closed Hc747 closed 2 years ago

Hc747 commented 3 years ago

NGINX is unable to start on heroku-20 with the following buildpack configurations.

Receiving the following log output. 2020-12-03T02:57:13.000000+00:00 app[api]: Build succeeded 2020-12-03T02:57:14.467859+00:00 app[web.1]: Starting log redirection... 2020-12-03T02:57:14.468156+00:00 app[web.1]: Starting nginx... 2020-12-03T02:57:14.757957+00:00 app[web.1]: nginx: [emerg] invalid number of arguments in "set" directive in ./config/nginx.conf:94 2020-12-03T02:57:14.759577+00:00 app[web.1]: Process exited unexpectedly: nginx 2020-12-03T02:57:14.759757+00:00 app[web.1]: Going down, terminating child processes... 2020-12-03T02:57:14.815580+00:00 heroku[web.1]: Process exited with status 1 2020-12-03T02:57:14.853246+00:00 heroku[web.1]: State changed from starting to crashed

Happy to provide more context or specific details if it'll assist in resolving this issue. Haven't experienced this issue on heroku-18.

Hc747 commented 3 years ago

182

Hc747 commented 3 years ago

This was in the staging environment btw - seems to work as expected in production.

edmorley commented 3 years ago

@Hc747 Hi! Thank you for opening this issue. On Heroku-20 this buildpack uses a newer version of nginx + ngx_mruby, so there's a possible incompatibility that I'd like to track down.

Could you paste in a code block your static.json?

Also, are there any differences in the environment variables set on the staging vs prod apps that might explain why the latter works?

Many thanks :-)

edmorley commented 3 years ago

Also if you could heroku run bash against the non-working staging app, followed by:

~ $ bin/boot &
~ $ cat /app/config/nginx.conf

...and then add the file to a gist and link from here - that would be very helpful :-)

Hc747 commented 3 years ago

Hey @edmorley, thanks for getting back to me! No difference in environment variables. Was initially configured as:

{
  "root": "build",
  "encoding": "UTF-8",
  "https_only": false,
  "clean_urls": true,
  "routes": {
    "/static/**": "/static/",
    "/**": "index.html"
  },
  "proxies": {
    "/api/": {
      "origin": "www.example.com"
    }
  }
} 

Current configuration (which works in production but failed in staging):

{
  "root": "build",
  "encoding": "UTF-8",
  "https_only": true,
  "clean_urls": true,
  "routes": {
    "/static/**": "/static/",
    "/**": "index.html"
  }
}
Hc747 commented 3 years ago

Will try and replicate the staging issue; managed to reconfigure in production and get it up and running! :)

Hc747 commented 3 years ago

@edmorley Was able to replicate the issue (in staging - haven't tried production). Here's the gist.

edmorley commented 3 years ago

@Hc747 Thank you for those.

Looking at the generated nginx config, line 94 corresponds to: set $upstream_endpoint_0 ;

This is generated here: https://github.com/heroku/heroku-buildpack-static/blob/8a4baf25142246d1f46d972f388d6398df27a167/scripts/config/templates/nginx.conf.erb#L107

Using the values set here: https://github.com/heroku/heroku-buildpack-static/blob/8bea3b3d31ddd9a9672c84b567af13e3d7427138/scripts/config/lib/nginx_config.rb#L34-L49

This would suggest that staging still has proxies set in its static.json. Is it possible different code is deployed to staging than expected? Does heroku run cat static.json return the expected content?

Hc747 commented 3 years ago

Can confirm that static.json is returning the expected configuration (same as what was provided in the gist). Resolved by removing proxies.

edmorley commented 3 years ago

Ah ok. I'd interpreted this to mean the second config (without proxy) was working in production but not staging:

Current configuration (which works in production but failed in staging):

...but it sounds now like the issue is not staging vs production, but using proxies vs not?

In which case the reason this error is occurring, is that the proxies config is not a valid URL (it needs to be a URL rather than a hostname). This is the case under both Heroku-18 and Heroku-20 - ie: it's not Heroku-20 specific.

Repro on Heroku-18:

$ mkdir testapp-static && cd $_ && git init && h create --stack heroku-18
$ h buildpacks:add https://github.com/heroku/heroku-buildpack-static
$ cat > static.json <<EOF
{
  "proxies": {
    "/api/": {
      "origin": "www.example.com"
    }
  }
}
EOF
$ git add -A; git commit -m '.' && git push heroku main
...
$ h open
$ h logs
...
2020-12-04T00:15:24.975088+00:00 heroku[web.1]: Starting process with command `bin/boot`
2020-12-04T00:15:28.130848+00:00 app[web.1]: Starting log redirection...
2020-12-04T00:15:28.131116+00:00 app[web.1]: Starting nginx...
2020-12-04T00:15:28.147135+00:00 app[web.1]: nginx: [emerg] invalid number of arguments in "set" directive in ./config/nginx.conf:72

The buildpack really should handle this case more gracefully (either by supporting URLs missing a scheme, or giving an explicit error message) and the docs could probably make this clearer (though the example uses a full URL) - though this limitation has been present for some time, so not a regression from #182.

Hc747 commented 3 years ago

Great catch. Sorry for the misrepresentation of the issue. Really appreciate you digging into it!

edmorley commented 3 years ago

You're welcome! Thank you for providing the extra information needed to get to the bottom of this. I'm relieved it's something that doesn't need to be fixed before we can roll out the new nginx to all stacks (and also doesn't block usage of Heroku-20 in the meantime) :-)

edmorley commented 3 years ago

I've adjusted the title and will leave this open for improving the UX here if such a config is used.

edmorley commented 2 years ago

Hi

This buildpack is now deprecated and we are recommending people move the more actively maintained heroku-buildpack-nginx. For migration advice see here.

As such, I'm closing this issue out since we won't be making further changes to this buildpack.