Skip to content

Conversation

thmhoag
Copy link
Contributor

@thmhoag thmhoag commented Jul 6, 2017

Remove -only-exposed from the docker-gen statement in order to allow the letsencrypt-companion container to pickup containers that are in internal networks.

Docker itself will not actually expose ports on containers that are in internal-only networks. This creates a problem when attempting to proxy web servers in containers that are on internal docker networks, as they will never be picked up by letsencrypt-companion since they technically do not have an "exposed" port.

Example: I'd like to proxy requests for a container running Portainer. Since Portainer only interacts with the docker service itself, it can sit on an internal docker network for increased security. However, since letsencrypt-companion only picks up containers with exposed ports, and this Portainer container can't expose a port on an internal docker network, it will never get a cert created by letsencrypt-companion.

This might be better as some kind of option, maybe an environment variable that can be set, but since letsencrypt-companion is only looking for containers with the LETSENCRYPT_HOST and LETSENCRYPT_EMAIL environment variables anyway, it seems reasonable to just remove this argument from the docker-gen statement.

@buchdag
Copy link
Member

buchdag commented Jul 8, 2017

Have you tested it ?

I just tested what you suggested (portainer on an internal network) in a variety of docker networks setup, none of them works with your PR, you end up with an empty upstream section in the generated /etc/nginx/conf/default.conf

I'm not sure of what you are trying to accomplish by putting a container supposed to provide an UI on an internal network that is unreachable from the outside.

@thmhoag
Copy link
Contributor Author

thmhoag commented Jul 8, 2017

I have tested it, but this change would only affect the generation of the certs by the letsencrypt-companion container. Did you check to see if your certs had been generated?

To get the default.conf to generate properly, a similar change has to be made to the docker-gen statement that generates the default.conf. The difference is that you can change this without modifying the code for docker-gen since you specify your own entrypoint for the docker-gen container.

Here's an example docker-compose file that works and might make more sense: https://gist.github.com/thmhoag/2e160cee99fb7c9f46db8e4a34c80d15

Of course to run it you'll have to change the image name of the nginx-letsencrypt to whatever you named the build from my pull-request, and also the VIRTUAL_HOST, LETSENCRYPT_HOST, LETSENCRYPT_EMAIL.

The key for the default.conf is that my entrypoint line for the docker-gen container also excludes the -only-exposed argument.

entrypoint: /usr/local/bin/docker-gen -notify-sighup nginx -watch -wait 5s:30s /etc/docker-gen/templates/nginx.tmpl /etc/nginx/conf.d/default.conf

As far as what I'm trying to accomplish, you're right, it's a bit of an edge-case. The idea is simply to keep all containers that don't absolutely require outside internet access on an internal network, especially the containers that have access to the docker.sock. This still leaves the letsencrypt-companion container on an external network, but that is unavoidable since it requires internet access to generate and maintain ssl certs.

@buchdag
Copy link
Member

buchdag commented Jul 8, 2017

The idea is simply to keep all containers that don't absolutely require outside internet access on an internal network

Correct me if I'm wrong, but if you want to proxy traffic to a container and have it accessible via a domain name (and a portainer container you can't reach has pretty much zero purpose), it no longer qualify as a container that don't require outside internet access. The two seems pretty contradictory to me.

edit : oh I think I get it, you mean preventing those containers from reaching the outside world on their own accord, not the other way around ?

@thmhoag
Copy link
Contributor Author

thmhoag commented Jul 8, 2017

Right, exactly. I'm intentionally exposing web traffic via the proxy, but that container doesn't need access to the internet to serve the purpose of providing a web UI for docker.sock.

I think my question here would be, is there any reason that we need the -only-exposed argument for this docker-gen statement? My thought behind the pull request was that the argument is unnecessary since containers will be filtered out by the LETSENCRYPT_HOST and LETSENCRYPT_EMAIL environment variables anyway, so I can't imagine anyone unintentionally generating certs for containers by removing it. However, if there is a specific reason for needing it that I'm missing here, maybe we could also consider making in an option via another environment variable, or providing some kind of override?

@buchdag
Copy link
Member

buchdag commented Jul 8, 2017

My thought behind the pull request was that the argument is unnecessary since containers will be filtered out by the LETSENCRYPT_HOST and LETSENCRYPT_EMAIL environment variables anyway, so I can't imagine anyone unintentionally generating certs for containers by removing it.

True.

I don't see any real necessity behind the argument either (beside the presumption that container without exposed ports won't require proxying anyway and thus don't need to be feeded to docker-gen, which prevents your use case).

Copy link
Member

@buchdag buchdag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--only-exposed has already been removed from the docker-gen process running on nginx-proxy and as @thmhoag pointed out the containers retrieved from Docker's API are filtered by LETSENCRYPT_HOST anyway.

@buchdag buchdag merged commit 43b913e into nginx-proxy:master Nov 20, 2017
buchdag pushed a commit that referenced this pull request Nov 20, 2017
Remove the last remaining -only-exposed on /app/function.sh after #230
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.

2 participants