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

ability to override default location block to support single-page-applications

Open srang opened this issue 7 years ago • 15 comments

Right now the nginx.conf provided by s2i by default includes a block:

 location {
 }

in the default_server block. When serving an SPA at root this causes problems. There are a bunch of examples out there for how to serve single page apps from root

  • https://gist.github.com/dimitardanailov/7a7c4e3be9e03d1b578a
  • https://gkedge.gitbooks.io/react-router-in-the-real/content/nginx.html
  • https://serverfault.com/questions/773705/make-nginx-serve-index-html-if-requested-url-is-not-found-without-using-error-pa

What I'd like to propose (and will submit a PR for) is to allow an environment flag for replacing the default location block with SPA block.

 location / {
    try_files $uri $uri/ /index.html
 }

Or if we want to add the ability to inject a SPA block through a predefined named file.

srang avatar Jun 07 '18 13:06 srang

Right now the nginx.conf provided by s2i by default includes a block:

location { }

@srang I can see empty location block only in test app or examples. What file do you think?

omron93 avatar Jun 08 '18 11:06 omron93

I was gonna put up a PR with an update to assemble that looks for a file called ‘nginx-default-loc.conf’ and if found, sed’s the default location block with an include for that new file

srang avatar Jun 08 '18 12:06 srang

To be honest I'm not sure what you want to do - from PR it'll be more clear I think.

So guessing: following can't be used?

./nginx-default-cfg/*.conf Contains any nginx config snippets to include in the default server block

omron93 avatar Jun 08 '18 13:06 omron93

The issue I ran into when trying to serve an SPA using ‘nginx-default-cfg/‘ was that when I needed to serve the app from root it conflicted with the existing location block. I’ll get up the PR and we can review and figure out if it makes sense (I can also throw together a gist example of what I’m trying to allow for)

srang avatar Jun 08 '18 15:06 srang

Hey sorry it took me so long to get something together. I still have a couple kinks to work out but here is an example of what I'm looking to do. Use Case

This is working for serving the app, but my proxy and spa configs aren't working well together right now. Let me know if this (the flag in the assemble script) is something that is a good fit or should not be included in the upstream.

srang avatar Jun 21 '18 01:06 srang

Thanks. I hope I finally understand. Sorry.

It makes sense, but I'm not nginx expert. @notroj What do you think?

omron93 avatar Jun 22 '18 07:06 omron93

/cc @atamanroman

Any updates here? We are hitting this bug. We want to use this image not for S2I builds just for plain docker build.

COPY nginx.conf /opt/app-root/etc/nginx.default.d/default.conf

is not good enough since we need to use our own

location / {
  try_files $uri $uri/ /index.html;
}

Nginx does not allow to define a location twice and I don't see the reason why there is a empty location /.

jkroepke avatar Jun 25 '18 13:06 jkroepke

@jkroepke in the short term you might be able to pull out the sed snippet from above and then run your copy command into nginx.default.d.

sed snippet for reference:

sed -i '/^\s*location \/ {/ { N; /^\s*location \/ {\n\s*}/d }' "${NGINX_CONF_PATH}"

I did have some permission problems with running the inplace (-i) sed on the base nginx.conf (hence why I didn't actually do inplace) but with a docker build you might be able to get away with it

srang avatar Jun 26 '18 19:06 srang

@srang Yeah Thank for the snippet. We steal the code already from here ;-)

https://github.com/srang/angular-5-example/blob/rhel/nginx/.s2i/bin/assemble#L10

But this shouldn't be the workaround in 2018.

jkroepke avatar Jun 26 '18 21:06 jkroepke

@jkroepke yeah it'd be great if it was a little easier

soooo @omron93 @notroj do we think this is a good feature candidate? If so I can finalize a PR with tests

srang avatar Jun 27 '18 14:06 srang

Yes please! I've run into the same issue while deploying single-page apps. My current solution to avoiding conflicting location / blocks is to instead specify a whole new nginx server block in nginx-cfg/spa.conf (not nginx-default-cfg/!) like so:

server {
    listen 8081;
    server_name app;
    root /opt/app-root/src;
    location / {
        try_files $uri $uri/ /index.html;
    }
}

But then I'm wedded to /opt/app-root/src, which is kind of an implementation detail, but more importantly, I have to run the new server on port 8081 to avoid colliding with the default server over on 8080. And because I'm running on a non-default port, I have to modify my Service, DeploymentConfig, and Route resources accordingly. This works well, but it's a lot to have to remember to set up for each app, and to recall the details of when doing maintenance.

It would be great if I didn't have to inherit so many custom configurations when all I really want is try_files $uri $uri/ /index.html; inside the default location block.

Thanks! ❤️

command-tab avatar Jul 21 '18 04:07 command-tab

This is the generated nginx.conf from my running container, https://gist.github.com/lholmquist/d9c3138b1253f4f6ef72e6eaef2f9a5b

I just modified this where i needed to, then supplied it to the image, so it would use this one. a bit of a hack, but i think it gets around the issue for now

lholmquist avatar Aug 21 '18 19:08 lholmquist

Hi I think that to solve this issue the best way could be adding the following: location / { include <otherPath>; }

misanche avatar Feb 11 '20 11:02 misanche

I JUST hit this same issue and it's still open from 2 years ago. @jkroepke Any updates?

InfoSec812 avatar Jun 30 '20 13:06 InfoSec812

@jkroepke Any updates?

Yes. I use the sed from here (https://github.com/sclorg/nginx-container/issues/58#issuecomment-400435116) in my downstream images.

jkroepke avatar Nov 02 '20 09:11 jkroepke