Adding support for internal ip/port usage.

[cnf]

This patch adds support for using -internal to register docker0
internal ip and port settings, instead of using the docker host
mapped ones.

[progrium]

can be collapsed into one line. var hp, hip string

[andrew-d]

@cnf Any update on this?

[sheldonh]

I would love to register container internal addresses, because I use routable (inside the cluster) --bip networks.

[cnf]

For consul, we are waiting for a new release that would change the way this works. @progrium would you accept this PR if I took out the consul part for now, and applied only the etcd part?

[progrium]

Yes. :)

[sheldonh]

I found the implementation of the -internal flag hard to read. I think it might be easier to do if creation of a registrator Service from a Docker PortBinding is turned into a strategy. Then we can have two separate code paths that are both easy to read for both use cases.

However, this would be simpler if we could rename PublishedPort to PortBinding, so that the code can clearly distinguish between exposed and published ports (since those are really the two registration strategies we're talking about here).

Would you be comfortable with that intrusive a change?

[cnf]

Pulled out the consul changed for now.

I'll make a new PR when the new consul changes come out.

[sheldonh]

My patch didn't work out any cleaner than @cnf's. The interplay between the registrator's hostname and the container's HostPort is nuggety.

Except for one thing. @cnf, I think you can keep -internal logic encapsulated in NewService() like this.

[sheldonh]

FYI, the merge conflict is just from stage/registrator.

[sheldonh]

I hope we're not in one of those situations where everyone is waiting for everyone else. ;-)

I'm waiting for this PR to go in. Is that on the cards?

[cnf]

Anything you need to accept this, @progrium ?

[progrium]

I'm going to check it out this week.

On Sun, Oct 5, 2014 at 7:30 AM, Frank Rosquin notifications@github.com
wrote:

Anything you need to accept this, @progrium https://github.com/progrium
?

—
Reply to this email directly or view it on GitHub
https://github.com/progrium/registrator/pull/18#issuecomment-57934574.

Jeff Lindsay
http://progrium.com

[progrium]

HostName is unnecessary now, right? Otherwise ... ready to merge.

[patatepartie]

@cnf could you expand on your comment about waiting on a change in Consul, please ? Do you have a pointer on what will that change be ?

[progrium]

Also needs a rebase. Sorry.

[sheldonh]

Hooray! Now for releases and tests. Busy skilling up on golang's testing library. Do you have a library preference for testing and/or assertions?

[progrium]

Usually the builtin one. Backends will need mocks. :\

[andyshinn]

@cnf was there a reason you added the check for internal at https://github.com/progrium/registrator/blob/master/bridge.go#L173? This causes all exposed ports (instead of just published ones) to be registered, which isn't consistent behavior. I'm proposing to remove that check in a new pull request if you aren't explicitly depending on it for some reason.

[cnf]

@andyshinn this was the entire point of the pull request. Register internal ports. you need to supply the --internal flag for this to be applied.

[andyshinn]

I meant more specifically about registered all the exposed internal ports instead of just the ones specified with -p. For example, if I have a container that exposes 4000, 5000, and 6000, and I only want to register 5000, I would currently set -p 5000. The mapped host port will be registered.

If I use the -internal switch while specifying -p 5000, it will register the internal IP, but all the exposed ports (not just 5000). Any specific reason for this?

[cnf]

-p publishes, which isn't what internal is about.