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

Per VIRTUAL_HOST configuration files #106

Merged
merged 4 commits into from Feb 23, 2015

Conversation

md5
Copy link
Contributor

@md5 md5 commented Feb 22, 2015

This PR adds the ability to configuration Nginx settings on a per VIRTUAL_HOST basis by adding a configuration file under /etc/nginx/vhost.d. As in the case of SSL configuration and basic authentication, the files in this directory are expected to be named after the VIRTUAL_HOST value verbatim (without .conf).

I also added some documentation around providing custom Nginx configuration on a proxy-wide basis. The whole README.md is starting to feel pretty messy to me, so it may make sense at some point to do an editing pass on it as a whole or to move large parts of it into wiki pages.

Fixes #29, #36, #39, #70, #76, #87
Related #71

@thaJeztah
Copy link
Contributor

Wondering, if I defined multiple domains (e.g. VIRTUAL_HOST=www.foo.com,foo.com), will I have to define a configuration file for each separately?

@md5
Copy link
Contributor Author

md5 commented Feb 22, 2015 via email

@md5
Copy link
Contributor Author

md5 commented Feb 22, 2015

Docs updated.

@md5
Copy link
Contributor Author

md5 commented Feb 22, 2015

I'm not sure what to recommend for people using regex-based or other more complex VIRTUAL_HOST configurations, but at least this can handle the common cases.

@thaJeztah
Copy link
Contributor

Ah, I thought it would follow the same rules, wasn't entirely sure.

Thanks a lot for this contribution, I think it will be very useful feature and I like this a lot more than having special "flags" or "environment-vars" for each feature!

@md5
Copy link
Contributor Author

md5 commented Feb 22, 2015

While I was working on this PR, I started having ideas about somehow using docker cp to grab the custom config from /etc/nginx-proxy or some such inside the backing containers themselves. This would run into issues if multiple containers with the same VIRTUAL_HOST had different conf files, but it would make it really easy to plug in backends with custom configuration without having to have a separate step to put a config file into /etc/nginx/proxy.d for the nginx-proxy container.

@jwilder
Copy link
Collaborator

jwilder commented Feb 22, 2015

This looks good. Thanks for updating the docs too. I agree, the README is getting a little messy.

For the custom config files, two questions:

  1. What do you think about having the custom config file actually be a template that is rendered instead of a static file that is included? We'd probably need a custom template func to compile and render the template and pass along the current context.
  2. Instead of just including/rendering a config file within the server block, we could let you generate the whole server block for that virtual host via the template. The template that is used would have to have the location and proxy_pass directives and we'd have to document what the bare minimum template needs to include. I think that would let you configure just about anything (e.g. custom paths, SSL criteria, etc..)

Thoughts?

@md5
Copy link
Contributor Author

md5 commented Feb 22, 2015

I actually started looking into implementing the necessary function for docker-gen a few months ago, but never progressed much with it: https://github.com/md5/docker-gen/compare/include-tmpl

Stepping back a bit, I think that both of your proposed changes sound like the sort of things that could lead to an increased support burden without substantial benefits.

In the case of the first change, users of nginx-proxy will be exposed directly to writing Go templates themselves, something they aren't expected to do currently. The benefit to be gained is that users will be able to do things like client_max_body_size {{ Env.MAX_UPLOAD_SIZE }}; in their own config files instead of having to generate or otherwise maintain the config files directly.

In the case of the second change, users will gain total control over the server block, but I think with the per-VIRTUAL_HOST config files it will be possible to override anything being done in nginx.tmpl anyways. I think at a certain point, users need to roll their own Nginx proxy image if they want to have complete control of everything.

@jwilder
Copy link
Collaborator

jwilder commented Feb 22, 2015

Good points.

Not everything is overideable with this approach I think. I don't think you could override the location / {} block with the include. I believe nginx will complain about about duplicate entries. I know there have been some requests for virtual path support so that still needs some other hooks to resolve.

The include is for both SSL and non-SSL. I'm not sure if it matters but adding SSL options would get included in the non-SSL block. Maybe that can be handled w/ having a .ssl suffix to the vhost file and nginx.tmpl would look for that and fallback to the main one if it doesn't exist? Or perhaps add two separate includes?

There's been other requests to not redirect http->https. Not sure if an include could resolve that issue.

I still think being able to generate the server block is useful. It could actually be a separate feature independent of this. The majority of the cases could be handled w/ this PR and anything not covered could be handled w/ a custom vhost template. Maybe a naming conventions like: example.com.tmpl. If that exists, use that for the server block. This would be the advanced/you need to understand go templates option but should negate the need to fork and maintain a custom image.

@md5
Copy link
Contributor Author

md5 commented Feb 23, 2015

I should have been clearer when I was talking about overriding. I was only referring to the other things that are currently set in the server {} block other than the location / {} part.

Implementing virtual path support requires grouping the backends that serve a particular path for a particular host into their own upstream and connecting each upstream to the appropriate location /path {} block. I started on some code for that at one point, but I haven't kept it up-to-date with other changes to nginx.tmpl: https://github.com/md5/nginx-proxy/compare/multiple-paths

Regarding the SSL thing, I did consider the fact that users might want to use a different config file for the HTTP and HTTPS blocks, but I wasn't sure what the right approach would be to support that. I'd probably lean toward supporting a /etc/nginx/vhost.ssl.d directory instead of a suffix approach, but I'm happy to update the patch either way. Perhaps @thaJeztah or someone else might have an opinion on the issue.

I like the idea of having the custom Go template thing be able to replace the entire server {} block. That way, users that take that approach will be 100% responsible for setting it up the way they want if they decide to go the "power user" route (with some reasonable docs as a starting point).

@jwilder
Copy link
Collaborator

jwilder commented Feb 23, 2015

@md5 I think we can punt on separate SSL configs for now and address it later if there is an issue.

@md5
Copy link
Contributor Author

md5 commented Feb 23, 2015

👍 to dealing with the SSL thing if anyone complains about needing separate configs

@curtiszimmerman
Copy link

Awesome!

@md5 md5 mentioned this pull request May 12, 2015
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

Successfully merging this pull request may close these issues.

Allow to set the value for client_max_body_size
4 participants