Populate internal IP using NetworkSettings.Networks where available

[jonuwz]

Docker api 1.21 deprecates NetworkSettings.IPAddress and its value is only set when using the default docker bridge. To support overlay networks in docker 1.9, we need to inspect NetworkSettings.Networks.

go-dockerclient has been extended to support this : https://github.com/fsouza/go-dockerclient/commit/c3e8735e510cf8bc6d439300ff2fc247b1f2867c

[mgood]

Thanks for looking into this. Can you update this to use tabs for indentation instead of spaces? Or I'd recommend running it through the gofmt tool to reformat it based on the Go coding conventions.

[osterman]

Thanks for contributing @jonuwz . Was banging my head against the wall for a few hours on this one...

[angeldimitrov]

@jonuwz this patch works, however it does not cover all the cases. In case you have one container attached to multiple networks or you attach the container to a overlay network after running it, the registrator won't communicate the correct values to the KV.

[jonuwz]

@angeldimitrov how would you see registrator handle multiple IPs ? At the moment I think it only deals with 1 network, the external or internal depending on the argument passed at start time. Does registrator handle updating the datastore if you attach a non-networked container to the bridge network currently ?

Side note - I can't find tests - are there any ?

[angeldimitrov]

@jonuwz - i already use your patch. However my thought is that this is not a solution of the more global problem with the overlay networks and registrator. Probably all the interfaces should be registered. Otherwise all the containers must use the same overlay network which is not cool. Here an example use-case:

                           FE-BE Overlay
                               ^
                               |
+----------------------------+ | +--------------------------+
|                            | | |                          |
| +----------+ +---------------+-------------+ +----------+ |
| | BE NODE 1| |             |   |           | | FE NODE 1| |
| +----------+ | +-------+   |   | +------+  | +----------+ |
| +----------+ | | LB BE |   |   | | FE LB|  | +----------+ |
| | BE NODE #| | +-------+   |   | +------+  | | FE NODE #| |
| +----------+ +-----------------------------+ +----------+ |
|                            |   |                          |
+-------------+--------------+   +-------------+------------+
              |                                |
              |                                |
              v                                v
         BE Overlay                       FE Overlay

[jonuwz]

Wouldn't you just use the external interfaces if you wanted to make the services available on multiple overlay networks ?

[angeldimitrov]

@jonuwz - yes i can do that, but this is agains the network isolation, especially if you like to have multiple environments running on the same cluster. The reference i have in mind is the security groups from AWS. In my opinion a good solution shoud support the system discovery within the overlay networks.

[progrium]

Can we simplify this by making Registrator handle one network at a time. If you have multiple networks, you run multiple Registrators...

[jonuwz]

@progrium I would agree with this approach. It would be non trivial to have one instance of registrator respond with service definitions appropriate to the origin of the requestor.

[progrium]

How does this PR look? https://github.com/gliderlabs/registrator/pull/316

[jonuwz]

@progrium specifying the desired network is an awesome idea. But it looks like it would break on docker < 1.9 ( I cant see a legacy method to retrieve the internal ip)

[chaseajen]

@jonuwz that's true. Since Docker is deprecating the previous property, however, we may want to create a separate branch for 1.9+. It seems like it might get kind of ugly to have the same codebase working with both versions of the Docker API, particularly since the old property will go away at some point.

Alternatively, we could inspect the version and switch based on that, but that feels messy to me.

[jonuwz]

@chaseajen I think it's as simple as 'if network = bridge, and Networks[bridge].IPAddress is empty, see if you can get the value from container.NetworkSettings.IPAddress'

This only applies if network=bridge, since c.NS.IPAddress will we empty for overlay networks.

I don't think it requires a separate branch.

(The commits in #293 work with bridge in 1.8 and 1.9, and overlay + bridge in 1.9)

[chaseajen]

@jonuwz we can do that. Keep in mind that NetworkSettings.IPAddress will be empty for more than just overlay networks — it's empty for any network other than the default bridge network in Docker 1.9, and the documentation states it will be removed in a future release. That's why I was thinking it was a bit ugly to keep one version to rule them all. As long as the upstream go-dockerclient keeps the property, we could do the 'if network' thing.

[progrium]

I'm all for compatibility and all for a single release/branch. If we have some branching logic to deal with backwards compatibility for as long as go-dockerclient allow us, we can always remove it later.

[chaseajen]

Ok, PR #316 should have the branching logic now.

[jonuwz]

Yeah. I'm aware of the deprecation. They removed NS.IP in one of the 1.9 betas, but put it back in because stuff broke.

You can use reflection to see if NS.IP exists before using it to stop it blowing up. This mitigates the risk of go-dockerclient changes breaking the build.

I had to rewrite some go-dockerclient tests to support 1.8 + 1.9 using something like this...

s := reflect.Indirect(reflect.ValueOf(container.NetworkSettings) IP := s.FieldByName("IPAddress") if IP.IsValid() { # field exists, get the value }

[etoews]

FWIW, there's an image kidibox/registrator out there with this PR in it.

$ docker run --detach \
  --name registrator-1 \
  --volume /var/run/docker.sock:/tmp/docker.sock \
  --net mynetwork \
  --env constraint:node==*n1 \
  kidibox/registrator \
  -internal consul://consul:8500

$ docker run -d \
  --name world \
  --net mynetwork \
  --expose 8080 \
  --env SERVICE_NAME=world \
  --env constraint:node==*n1 \
  google/python-hello

$ curl -s http://consul:8500/v1/catalog/service/world | jq .
[
  {
    "Node": "b6f97bc42226",
    "Address": "10.0.2.2",
    "ServiceID": "ff6d5895b3d7:world:8080",
    "ServiceName": "world",
    "ServiceTags": [],
    "ServiceAddress": "10.0.2.6",
    "ServicePort": 8080,
    "ServiceEnableTagOverride": false,
    "CreateIndex": 881,
    "ModifyIndex": 881
  }
]

Now ServiceAddress and ServicePort are correct.

[ledgr]

I have just tested this in my environment ant it works as expected. I'm using kidibox/registrator image with docker-compose.

:+1:

[bfelaco]

+1 from me on this fix. Using the kidibox/registrator image worked for me, using docker 1.10.3 with docker-compose version 1.6.2 and a new named network (which is the default behavior when using the version 2 format).

But I can see a lot of RFEs coming in to deal with various network topologies. People might want to only register containers/service using the address on 1 network or another, and even possibly use different back-end registries for different networks. I think the simplest solution to address that is to stick with pairing 1 registrator instance with a single network, and have it filter out containers which aren't joined to that network. So I also vote for PR #316.