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

Support Overlay Network #337

Merged
merged 17 commits into from May 1, 2016

Conversation

Projects
None yet
@baptistedonaux
Contributor

baptistedonaux commented Jan 17, 2016

  • Migrate docker-gen from 0.4.2 to 0.5.0 (add []Network supports)
  • Nginx template use now current IP into network.
  • Documentation updated. Proxy must run it with the option --net=host to access at all differents networks (overlay network support - tested with docker-compose --x-networking up -d)
@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented Jan 17, 2016

If PR merged, missing Circle CI container creation with flag --net=host to deal with network differents of bridge (default value).

@md5

This comment has been minimized.

Contributor

md5 commented Jan 17, 2016

@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented Jan 18, 2016

--net=host flag isn't required while proxy container is in the same network that the container called. So, if all containers called (with VIRTUAL_HOST) are in default network (currently bridge), the flag isn't required.

With the new overlay network and the --x-networking docker-compose flag (currently beta - 1.5), every application are isolated in specific network. Containers into bridge network can't call containers in other network. With --net=host flag, proxy container is on the localhost network. All virtual networks are callables.

@md5

This comment has been minimized.

Contributor

md5 commented Jan 18, 2016

Based on the Getting started with multi-host networking document, it seems like they're recommending --net=my-net where my-net is the name of the overlay network.

I think this should be documented in its own section of the readme instead of adding --net= flags to every docker run in the existing readme examples. That new section should use --net=my-net instead of --net=host and should link to https://docs.docker.com/engine/userguide/networking/get-started-overlay/ or some other Docker documentation.

@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented Jan 18, 2016

Okay, --net=host flag is probably too much and require few explications. host "network" is the ultimate solution for proxy in front-end.

@md5 Do you want propose a README text block to insert ?

@md5

This comment has been minimized.

Contributor

md5 commented Jan 18, 2016

I haven't really played around with overlay enough to write the section myself and I'm not sure when I'll get a chance to do so.

Regarding --net=host, I'm really reluctant to ever use it since it means that the container shares all network namespaces with the host (including Unix domain sockets). I've seen weird, unexpected things happen when --net=host is used (eg moby/moby#5899), so I consider it an absolute last resort.

@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented Jan 18, 2016

I understand. The best solution could be an aggregation of namespace networks. I currently search a best solution.

@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented Jan 18, 2016

Currently, we can associate a container at many networks with docker network connect my_network my_container but only one network at container creation (moby/moby#17796).

Can be a best alternative at --net=host flag.

@md5

This comment has been minimized.

Contributor

md5 commented Jan 18, 2016

I agree the connecting explicitly to all the relevant networks is the best approach. It looks like support for doing this in a docker-compose.yml was added in docker/compose#2564

@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented Jan 20, 2016

@md5 Please feedback for English language and content writing.

@md5

This comment has been minimized.

Contributor

md5 commented Jan 21, 2016

@baptistedonaux I opened baptistedonaux#1 with my suggestions.

One thing that updating the README made me wonder is whether some attempt should be made in nginx.tmpl to filter the containers to only include the ones in the same networks as the nginx-proxy instance itself. That would require the ability to identify the container ID in which docker-gen is currently executing. I think jwilder/docker-gen#146 may be a step in that direction, but I'm not sure that's possible currently.

@md5

This comment has been minimized.

Contributor

md5 commented Jan 21, 2016

Looking closer at jwilder/docker-gen#146, I don't think it actually does anything to help us figure out the container ID of the nginx-proxy container in which docker-gen is currently executing.

@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented Jan 21, 2016

@md5 I was thinking at this feature. I try implement it and I come back to you.

@md5

This comment has been minimized.

Contributor

md5 commented Jan 21, 2016

It looks like docker-gen could determine this from /proc/self/cgroup. Here's an example with awk:

# awk -F/ '/cpu:/ { print $NF }' /proc/self/cgroup 
7f72ffdf3089df475a9cfe459fd0ec3b0d214686e794887ca17bc1b32a6f995b

docker-gen could pretty easily read the /proc/self/cgroup file itself directly, assuming this is consistent across Docker hosts.

Merge pull request #1 from appropriate/overlay-support
Update README wording for overlay networking
@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented Jan 25, 2016

@md5 To retrieve current container name, I propose awk command:

awk -F"-|\." '/1:/ {print $3}' /proc/self/cgroup

Then, get container's networks for current network requires to know current container name. So I propose two solutions:

  • Add into Docker structure a new field with name of current container.
type Docker struct {
   Name            string
   NumContainers   int
   NumImages       int
   Version         string
   ApiVersion      string
   GoVersion       string
   OperatingSystem string
   Architecture    string
+  CurrentContainerName string
}
  • Add new template's method into template.go to retrieve container name (with awk).
func getCurrentContainerName() (string, error);

In two cases, docker-gen requires a new feature implementation (or I need help).

@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented Jan 27, 2016

@md5 What do you think of my propositions ?

@md5

This comment has been minimized.

Contributor

md5 commented Jan 27, 2016

@baptistedonaux I don't think that using awk will be necessary. The Go code should be able to deal with /proc/self/cgroup directly.

I think adding the current container id (or name) to the Docker struct makes sense. I started looking at the code to do so, but the code for populating $.Docker is spread around a bit and I didn't end up writing any code.

@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented Jan 28, 2016

@md5 I try too on a git branch to prepare the next implementation (matches between proxy container and networks).

@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented Jan 28, 2016

I have implemented a parser for /proc/self/cgroup. I'm not satisfied because parse a file with split methods doesn't seem me "conventional". Default to find (to know) a best solution, this is my implementation.

baptistedonaux/docker-gen@dc155e1

I wait your implementation or your feedback.

@md5

This comment has been minimized.

Contributor

md5 commented Jan 28, 2016

I commented on the commit. Perhaps @jwilder can take a look when he has a minute.

@jwilder

This comment has been minimized.

Owner

jwilder commented Jan 28, 2016

Agree w/ all of @md5 comments. Format of my /proc/self/cgroup matches @md5's as well.

@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented Feb 2, 2016

I propose this PR jwilder/docker-gen#158

@thomasleveil

This comment has been minimized.

Contributor

thomasleveil commented Mar 24, 2016

Keep in mind that CircleCI compile their own custom docker engine binary. Make sure that issue isn't specific to CircleCI

@thomasleveil

This comment has been minimized.

Contributor

thomasleveil commented Mar 24, 2016

See thomasleveil@c85839b for switching back to TravisCI (if it helps)

@pitkley

This comment has been minimized.

Contributor

pitkley commented Mar 24, 2016

@thomasleveil Thanks for the input on that! I guess that this is rather specific, because I haven't heard of /proc/self/cgroup not providing the ID. I wouldn't "blame" it on the Circle-CI build without further analysis though, because it might have to do with the LXC setup they use rather than with the build itself.

Anyway, this would probably be a "custom-tailored" solution to get CI happy, with the side-effect of supporting the low percentage of other users who might run a special build of Docker that doesn't expose the ID through the cgroups directly.

While switching to Travis is an option (and a quick test shows that the cgroups are available), I'm not sure if @jwilder would be too happy about switching.

I think that this is @jwilder's call: "monkeypatch" docker-gen such that it tries to find the container ID elsewhere (like /proc/self/mountinfo), or switch CI to e.g. Travis?

@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented Mar 24, 2016

@pitkley True, only @jwilder can switch to Travis CI.

Neverheless, @thomasleveil was right because the last Travis built successful. https://travis-ci.org/thomasleveil/nginx-proxy/builds/95239874

@pitkley

This comment has been minimized.

Contributor

pitkley commented Mar 24, 2016

@baptistedonaux Furthermore, I have tried to build and test this PR, which worked too. So Travis-CI definitely seems like a viable option! 👍

@jwilder

This comment has been minimized.

Owner

jwilder commented Mar 25, 2016

I'm fine switching to Travis CI. This project used to use it as well, but switched to Circle CI when the test suite was added. See: #246 for why.

@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented Mar 25, 2016

@jwilder Great, Travais fail seems solved.

@thomasleveil @pitkley Somebody to propose a PR (switch Circle to Travis) or I add this changement in current PR.

@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented Apr 5, 2016

@jwilder Travis test works

@@ -73,6 +73,7 @@ load test_helpers
-v /var/run/docker.sock:/tmp/docker.sock:ro \
-v $BATS_TEST_DIRNAME/../nginx.tmpl:/etc/docker-gen/templates/nginx.tmpl:ro \
--volumes-from bats-nginx \
--expose 80 \

This comment has been minimized.

@md5

md5 Apr 22, 2016

Contributor

Forgive me if this is covered in the previous comments, but why is this change needed?

This comment has been minimized.

@pitkley

This comment has been minimized.

@baptistedonaux

baptistedonaux Apr 25, 2016

Contributor

@pitkley @md5 Unit test are fixed and Travis executes with success.

@nikashitsa

This comment has been minimized.

nikashitsa commented Apr 22, 2016

@baptistedonaux any updates? Can't wait for it 😊

@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented Apr 25, 2016

I wait a feedback of @jwilder to advance this PR.

/cc @md5 @pitkley @nikashitsa

@jwilder jwilder merged commit 1c98df2 into jwilder:master May 1, 2016

1 of 2 checks passed

ci/circleci Your tests failed on CircleCI
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jwilder

This comment has been minimized.

Owner

jwilder commented May 1, 2016

Thanks @baptistedonaux!

@lessless

This comment has been minimized.

lessless commented May 31, 2016

does that mean that it's possible to use with v2 syntax?

@baptistedonaux

This comment has been minimized.

Contributor

baptistedonaux commented May 31, 2016

It's a feature only available with the v2 syntax. In v1, it's impossible to manage networks.

@thomasleveil

This comment has been minimized.

Contributor

thomasleveil commented on nginx.tmpl in 658e20f Feb 26, 2017

$currentContainer being the container running the docker-gen process, $CurrentContainer is not the container running the nginx proces when you use this template in the Separate containers case.

@thomasleveil

This comment has been minimized.

Contributor

thomasleveil commented on nginx.tmpl in 658e20f Feb 26, 2017

If the container running docker-gen is not on the same networks as the container running nginx, this check makes no sense.

almereyda referenced this pull request in ecobytes/docker-nginx-gen-letsencrypt Sep 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment