Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
srang opened this issue Jun 7, 2018 · 15 comments

Comments

@srang
Copy link

srang commented Jun 7, 2018

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
Copy link

omron93 commented Jun 8, 2018

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
Copy link
Author

srang commented Jun 8, 2018

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
Copy link

omron93 commented Jun 8, 2018

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
Copy link
Author

srang commented Jun 8, 2018

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
Copy link
Author

srang commented Jun 21, 2018

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
Copy link

omron93 commented Jun 22, 2018

Thanks. I hope I finally understand. Sorry.

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

@jkroepke
Copy link
Contributor

/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
Copy link
Author

srang commented Jun 26, 2018

@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
Copy link
Contributor

@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
Copy link
Author

srang commented Jun 27, 2018

@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
Copy link

command-tab commented Jul 21, 2018

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
Copy link
Member

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
Copy link

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

@InfoSec812
Copy link

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

@jkroepke
Copy link
Contributor

jkroepke commented Nov 2, 2020

@jkroepke Any updates?

Yes. I use the sed from here (#58 (comment)) in my downstream images.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants