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

Companion for the separated networks #571

Open
SilverFire opened this issue Aug 19, 2019 · 11 comments · Fixed by #612 · May be fixed by #957
Open

Companion for the separated networks #571

SilverFire opened this issue Aug 19, 2019 · 11 comments · Fixed by #612 · May be fixed by #957
Labels
kind/feature-request Issue requesting a new feature

Comments

@SilverFire
Copy link
Contributor

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.

@buchdag buchdag added the kind/feature-request Issue requesting a new feature label Aug 19, 2019
@buchdag
Copy link
Member

buchdag commented Aug 19, 2019

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.

@SilverFire
Copy link
Contributor Author

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

@buchdag
Copy link
Member

buchdag commented Aug 19, 2019

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.

@SilverFire
Copy link
Contributor Author

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

@buchdag
Copy link
Member

buchdag commented Aug 19, 2019

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.

@SilverFire
Copy link
Contributor Author

SilverFire commented Aug 19, 2019

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

So do you want me to create a PR?

@buchdag
Copy link
Member

buchdag commented Sep 5, 2019

@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.

@SilverFire
Copy link
Contributor Author

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

@buchdag buchdag linked a pull request Jun 14, 2020 that will close this issue
@buchdag
Copy link
Member

buchdag commented Oct 10, 2020

Hi @SilverFire

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

@SilverFire
Copy link
Contributor Author

Yes, works as expected. Created a PR #703 to fix merge conflicts and prepare dev breach to be merged in master

@SilverFire SilverFire linked a pull request Apr 28, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature-request Issue requesting a new feature
Projects
None yet
2 participants