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

Implemented segregation by network #612

Merged
merged 2 commits into from
Jun 16, 2020

Conversation

SilverFire
Copy link
Contributor

Hi, @buchdag!

This pull-request implements the discussion we've started in #571

I've introduced a new env variable for LE container MUST_BE_CONNECTED_WITH_NETWORK, that limits the LE scope to containers, included only in the specified network. The tests are included.

Could you review this pull request, please? If it seems acceptable to you, I will add the necessary documentation. Thank you!

@buchdag
Copy link
Member

buchdag commented Jan 16, 2020

Hi @SilverFire

Just to let you know that I saw this but I'm really, really lacking time those days.

I'll take a look as soon as possible.

@SilverFire
Copy link
Contributor Author

No worries, take your time 🙌

I have already combined this PR together with #610 and deployed them on my production.

@SilverFire
Copy link
Contributor Author

@buchdag did you have a chance to check this PR?

@buchdag
Copy link
Member

buchdag commented May 7, 2020

@SilverFire not yet :/

@buchdag
Copy link
Member

buchdag commented May 11, 2020

@SilverFire this will have to be rebased against #647 since it has been merged to master 😃

@SilverFire
Copy link
Contributor Author

Yepp, definitely a good idea. Rebased and force-pushed)

image

@buchdag buchdag added the kind/feature-request Issue requesting a new feature label May 18, 2020
@buchdag
Copy link
Member

buchdag commented Jun 10, 2020

Ok sorry for the delay, I just took a look and it's mostly good, here are my remarks:

  1. maybe we could have something shorter than MUST_BE_CONNECTED_WITH_NETWORK ? What about RESTRICT_TO_NETWORK, WATCH_NETWORK or NETWORK_SCOPE ?
  2. so you choose to be able to specify the target network by name, have you weighted it against the same way nginx-proxy works (only containers on the same network as the nginx-proxy or docker-gen container) ? Any pro and cons to both methods from your point of view ?
  3. I'm still deciphering some of that docker-gen template syntax. 😅

As this is a pretty substantial feature, would you mind if I merged it to dev first ?

@SilverFire
Copy link
Contributor Author

  1. I'd stick for NETWORK_SCOPE if it's OK for you.

  2. Meh, I don't remember how it went this way. Either there was a problem, or I just overengineered :)

  3. It's was my first docker-gen experience and I remember the first time I've touched it I thought I should never do it again :)

As this is a pretty substantial feature, would you mind if I merged it to dev first ?

As you wish

@buchdag
Copy link
Member

buchdag commented Jun 12, 2020

Ok so about the docker-gen template:

  1. we're getting the env from the letsencrypt companion container
  2. we're iterating a first time over containers that have a LETSENCRYPT_HOST variable set
  3. if the MUST_BE_CONNECTED_WITH_NETWORK (NETWORK_SCOPE) env var is set on the letsencrypt companion container we check that the container we're iterating over is connected to the same network and append its ID to a string if true.
  4. if the MUST_BE_CONNECTED_WITH_NETWORK env var isn't set, we append the container ID to the string no matter what.
  5. we create a slice from this string
  6. we iterate a second time over containers that have a LETSENCRYPT_HOST variable set (the original range loop) and now only process containers whose ID intersect with the slice created at 5.

Got that right ?

Does the feature work as intended if the proxied container is connected to more than one network ?

@SilverFire
Copy link
Contributor Author

That's absolutely correct!

@SilverFire
Copy link
Contributor Author

SilverFire commented Jun 13, 2020

Does the feature work as intended if the proxied container is connected to more than one network ?

Yes, it does:

{{ range $containerNetwork := $container.Networks }}
    {{ if eq $CurrentContainer.Env.MUST_BE_CONNECTED_WITH_NETWORK $containerNetwork.Name }}
[ ... ]

We iterate over all networks of the container

@buchdag
Copy link
Member

buchdag commented Jun 14, 2020

LGTM for a merge to dev, do you want to change the variable name and add the doc on this PR or later on ?

@buchdag buchdag changed the base branch from master to dev June 14, 2020 15:00
@buchdag buchdag linked an issue Jun 14, 2020 that may be closed by this pull request
@SilverFire
Copy link
Contributor Author

Renamed. Will add docs lated

@buchdag buchdag merged commit 0b10bb4 into nginx-proxy:dev Jun 16, 2020
@SilverFire SilverFire deleted the networks_segregation branch April 27, 2022 11:08
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
Development

Successfully merging this pull request may close these issues.

Companion for the separated networks
2 participants