DanielDent / docker-nginx-ssl-proxy

SSL Front-End Proxy With Automatic Free Certificate Management
https://hub.docker.com/r/danieldent/nginx-ssl-proxy/
Other
204 stars 68 forks source link

include extra (optional) configuration folder in dockerfile #14

Closed Groostav closed 6 years ago

Groostav commented 6 years ago

first and foremost: I am not a docker expert, so apologies if this request is mislaid.

Looking at jetbrain's products, most suggest that you can (but do not have to) create volumes for logs and configuration, such that you can optionally supply additional configuration or persist logs on the host. I found this very convenient since it effectively makes their docker images parametric on some configuration files and the images "output" logs.

I want your image to do the same:

I'm looking at doing some configuration of the nginx proxy for a teamcity front-end, as described here, so that I can have some large files going up into the service proxy'd by your container.

The problem I'm having as a non-docker-expert is getting my configuration to stick.

TL;DR: I would really appreciate it if you would update the dockerfile to contain something to the effect of

echo "include /etc/nginx/extra-conf/*.conf" >> /etc/nginx/nginx.conf

in your docker file, along with a suggestion in the configuration section that you

add a volume /etc/nginx/extra-conf and place relevant nginx *.conf files there to have them loaded.

DanielDent commented 6 years ago

Hi @Groostav, thanks for the detailed message.

A lot of people who customize the configuration use a Dockerfile along the following lines:

FROM danieldent/nginx-ssl-proxy
COPY config-file.conf /etc/nginx/conf.d

With the appropriate docker-compose.yml, it can be quite simple to rebuild the container on the host itself whenever a configuration change is needed.

But maybe there is something about your workflow which make it that you really want volumes. In which case I would ask: where would your proposed include directive be located? Inside the HTTP block below the current include directive?

WIth regards to your specific customizations: The default config already sends the X-Forwarded headers out of the box. If your only other customizations relate to maximum upload size, that might be a configuration I'd be willing to support with an additional environment variable -- it's a situation I've had to deal with multiple times in my own usage of this proxy.

Groostav commented 6 years ago

it can be quite simple to rebuild the container on the host itself whenever a configuration change is needed ... But maybe there is something about your workflow which make it that you really want volumes.

At this point I've been getting by with pure docker-compose changes. I sort've thought of changing a pulled dockerfile as akin to changing a published binary (read: a really bad idea), but clearly this is an erroneous line of thinking. I'll start playing around with manual dockerfile modifications myself.

where would your proposed [nginx config file] include directive be located?

This is a good question, I thought maybe nginx config files were 'mergable' in the sense that two server {} blocks would get merged together, but from the way you ask this question it sounds like this is not the case, in which case making the locations of the extra include directives would be very important and difficult.

But as you suggest, simply adding a line, perhaps with a well named file would make this clear...

effectively:

server {
  http {
    include more-conf/additional-http-config.conf
  }
  include more-conf/additional-server-config.conf
}
include more-conf/additional-root-config.conf

But, as I'm sure my notes have made clear, I really don't know what I'm doing...

your only other customizations relate to maximum upload size, that might be a configuration I'd be willing to support with an additional environment variable -- it's a situation I've had to deal with multiple times in my own usage of this proxy.

An environment variable would put this problem to bed for me quite nicely! I'd really appreciate it.

DanielDent commented 6 years ago

I'd never considered the possibility of nginx merging config files: I'm pretty sure it doesn't, and I think the config format makes it a significant implementation challenge (it would require a mechanism to distinguish between "add this as an additional http block" vs "merge this into this specific existing http block").

For the moment, I think I'll avoid adding support for arbitrary includes: I think the general strategy you outline above (a number of different includes in different locations) is probably sensible, but in the meantime there are also a number of ways that people can achieve the result with the image as-is. This can be revisited when appropriate.

If you have a chance, please try out the :dev branch/tag. It contains the extra environment variables I mentioned above. Please report back any problems - I am currently so pressed for time that I haven't even built this container, let alone had a chance to try running it and seeing if it works.