sclorg / nginx-container

Nginx high-performance HTTP server and reverse proxy container images based on Red Hat Software Collections and intended for OpenShift and general usage. Users can choose between Red Hat Enterprise Linux, Fedora, and CentOS based images.
http://softwarecollections.org
Apache License 2.0
91 stars 196 forks source link

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

Open srang opened 6 years ago

srang commented 6 years ago

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

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.

omron93 commented 6 years ago

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?

srang commented 6 years ago

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

omron93 commented 6 years ago

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

srang commented 6 years ago

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 commented 6 years ago

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.

omron93 commented 6 years ago

Thanks. I hope I finally understand. Sorry.

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

jkroepke commented 6 years ago

/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 /.

srang commented 6 years ago

@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

jkroepke commented 6 years ago

@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.

srang commented 6 years ago

@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

command-tab commented 6 years ago

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! ❤️

lholmquist commented 6 years ago

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

misanche commented 4 years ago

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

InfoSec812 commented 4 years ago

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

jkroepke commented 3 years ago

@jkroepke Any updates?

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