acme-companion icon indicating copy to clipboard operation
acme-companion copied to clipboard

Companion for the separated networks

Open SilverFire opened this issue 6 years ago • 11 comments

Hi, team! Thank you for your efforts in making and supporting this repository!

I've seen a request to make docker-letsencrypt-nginx-proxy-companion respect the connected networks in order to have multiple separate nginx-proxy installations running on the same host. It is useful for cases when you have multiple external IP addresses on the host and you need to distribute running services between them.

Currently, jwilder/nginx-proxy generates vhosts.conf only out of containers that are in the same network with it, but docker-letsencrypt-nginx-proxy-companion tries to generate certificates for all running containers.

Having a single docker-letsencrypt-nginx-proxy-companion for all nginx-proxy containers might be a solution, but companion can manage only a single nginx proxy service, but going this way makes the proxy service not complementary: I have to run either multiple nginx-proxies and deploy letsencrypt-companion somewhere else.

I suggest to limit letsencrypt_service_data.tmpl to nginx containters that share the same network only. I've added this thing a few years ago and it works in production for a long time without any issues.

If you think this change is acceptable, I can create a pull request.

SilverFire avatar Aug 19 '19 09:08 SilverFire

Hi

Thanks for your input.

Having a single docker-letsencrypt-nginx-proxy-companion for all nginx-proxy containers might be a solution, but companion can manage only a single nginx proxy service

I started to work on multiple nginx-proxy/nginx and docker-gen containers support a while ago but never fully completed it as there was no real demand at the time.

but going this way makes the proxy service not complementary: I have to run either multiple nginx-proxies and deploy letsencrypt-companion somewhere else.

I'm not sure I understand what you mean. If the letsencrypt-companion was able to handle multiple nginx-proxy containers, why would it need to be deployed elsewhere ?

This being said, my implementation of multi nginx-proxy/nginx/docker-gen containers support was rather simplistic, with no awareness of Docker networks. Every nginx-proxy/nginx/docker-gen containers where told to reload their config each time a creation or renewal happened, wether it was for a container in the same network or not.

Your suggested modification is interesting, and might be complementary to nginx-proxy/nginx/docker-gen containers support, but should be made optional to avoid breaking setups that relied on the behaviour that you aim to modify.

buchdag avatar Aug 19 '19 11:08 buchdag

I'm not sure I understand what you mean. If the letsencrypt-companion was able to handle multiple nginx-proxy containers, why would it need to be deployed elsewhere ?

I deploy nginx-proxy for each IP address separately, so I have multiple directories named nginx-proxy-${IP} (e.g. nginx-proxy-192.168.1.14, nginx-proxy-192.168.1.15), each directory contains a docker-compose.yml with nginx-proxy & letsencrypt-companion services, connected to a nginx-proxy-${IP}} network, together with the final nginx servers. With this schema, whenever I want to deploy a new nginx-proxy, I clone my template repository, change the IP address in .env, run script to create a network and a systemd service and I'm ready to go.

If letsencrypt-companion can handle multiple nginx-proxy services, then I have to deploy letsencrypt-companion once, share a volume with SSL certificates between different services and make sure to configure letsencrypt-companion whenever I add a new proxy. It looks less straightforward for me than keeping proxy&companion(&docker-gen) in a single docker-compose.yml and sharing a local directory instead of a volume.

Every nginx-proxy/nginx/docker-gen containers where told to reload their config each time a creation or renewal happened, wether it was for a container in the same network or not.

With the approach I've suggested, we still have to handle every single up/down event, but if the event does not belong to a container that shares the same network with letsencrypt-companion, nothing will happen.

Your suggested modification is interesting, and might be complementary to nginx-proxy/nginx/docker-gen containers support, but should be made optional to avoid breaking setups that relied on the behaviour that you aim to modify.

Yes, we can either make it optional, but considering nginx-proxy default behavior, I'd bump a major release with BC breaking change in order to make it consistent. Anyway, it's up to you

SilverFire avatar Aug 19 '19 12:08 SilverFire

If letsencrypt-companion can handle multiple nginx-proxy services, then I have to deploy letsencrypt-companion once, share a volume with SSL certificates between different services and make sure to configure letsencrypt-companion whenever I add a new proxy. It looks less straightforward for me than keeping proxy&companion(&docker-gen) in a single docker-compose.yml and sharing a local directory instead of a volume.

[In what I was aiming for] reconfiguring the letsencrypt-companion each time you add a new proxy won't be necessary if you use labels on the nginx-proxy containers rather than the env var method. New proxy containers will be picked up automatically.

But your point about having everything clean and separated in different compose files is valid nonetheless.

My only real concern about running a separate letsencrypt-companion for each proxy network would be the increased ressource consumption of the docker-gen process running inside it. That's by far the biggest ressource hog of the whole nginx-proxy stack.

With the approach I've suggested, we still have to handle every single up/down event, but if the event does not belong to a container that shares the same network with letsencrypt-companion, nothing will happen.

My take on this is that multi container support is rather a feature that also happen to "solve" this issue in a way, rather than a different approach to the same problem. Both might be useful.

Yes, we can either make it optional, but considering nginx-proxy default behavior, I'd bump a major release with BC breaking change in order to make it consistent. Anyway, it's up to you.

You point is entirely valid, but experience tells me that most people appear to be using master instead of tagged releases and not really reading release notes (well most people that open issues, as that's the only metric I have). Being the only person actively working on this project, I tend to be cautious with backward compatibility to avoid generating more time consuming issues.

And I don't want to bump major release number until we switch to acme.sh, which will break BC. The behaviour you are proposing will probably be made default then.

buchdag avatar Aug 19 '19 12:08 buchdag

That's by far the biggest ressource hog of the whole nginx-proxy stack.

It's a surprise for me. Are we talking about the consumption of significant resources?

most people appear to be using master instead of tagged releases and not really reading release notes

My experience fully supports your opinion :)

Ok, do you want me to prepare a pull request with the suggested approach and introduce an option ONLY_NETWORK_CONNECTED_CONTAINERS (defaults to false)? // Suggest a better name if you have an idea

SilverFire avatar Aug 19 '19 12:08 SilverFire

It's a surprise for me. Are we talking about the consumption of significant resources?

In htop, using the TIME+ column docker-gen processes are usually the next behind big CPU hitters like database processes, even with almost no container creation / deletion happening in the long run. But my use case for nginx-proxy stacks are limited to rather small workloads, other processes might go way above docker-gen on a more busy host.

docker-gen consume almost no memory though.

I don't really have more substantial metrics.

buchdag avatar Aug 19 '19 14:08 buchdag

Well, consuming 5h or CPU for 3-5 months of uptime (see START column) sounds reasonable, I would not name docker-gen a greedy guy)

Screen Shot 2019-08-19 at 18 58 21

SilverFire avatar Aug 19 '19 16:08 SilverFire

So do you want me to create a PR?

SilverFire avatar Aug 20 '19 14:08 SilverFire

@SilverFire sorry, I totally forgot this issue, yes a PR would be welcome. We'll discuss the env var exact name in the PR if it's ok with you.

buchdag avatar Sep 05 '19 09:09 buchdag

No problem. I'm on vacation right now but will do a PR in a few weeks.

SilverFire avatar Sep 05 '19 20:09 SilverFire

Hi @SilverFire

Have you been testing this since it's been merged to dev ?

buchdag avatar Oct 10 '20 01:10 buchdag

Yes, works as expected. Created a PR https://github.com/nginx-proxy/docker-letsencrypt-nginx-proxy-companion/pull/703 to fix merge conflicts and prepare dev breach to be merged in master

SilverFire avatar Oct 20 '20 10:10 SilverFire