Vendoring libnetwork #19198

sanimej · 2016-01-08T23:40:00Z

Changes include..

vendoring libnetwork and its dependencies (miekg/dns, vishvananda/netlink)
replace /etc/hosts based name resolution with embedded DNS for user defined networks
overlay veth cleanup: Failed to start container in overlay network since "br0" exchange full #18814
check before programming ipv6 in bridge: Can't start ipv6 daemon twice #19139
disable IPv6 Duplicate Address Detection: IPv6 addresses not available for a brief period after container start #18871

Fixes #18814
Fixes #19139
Fixes #18871

- replace /etc/hosts based name resolution with embedded DNS for user defined networks - overlay veth cleanup: moby#18814 - check before programming ipv6 in bridge: moby#19139 - diable DAD: moby#18871 Signed-off-by: Santhosh Manohar <santhosh@docker.com>

Signed-off-by: Santhosh Manohar <santhosh@docker.com>

icecrime · 2016-01-09T00:07:06Z

Just pasting the line diff here as GitHub can't show it:

$ git log -2 --shortstat --pretty=oneline
64a6dc355815261ac438b12a262e3cda7c9181df Docker changes for libnetwork vendoring..
 7 files changed, 22 insertions(+), 56 deletions(-)
8ccf5cffa7cd3cfd8e83fdaebc60a9f3d2473bcc Vendoring libnetwork and its dependencies..
 79 files changed, 15590 insertions(+), 215 deletions(-)

Even among the dependencies, the bulk comes from github.com/miekg/dns, changes to libnetwork itself are pretty small:

 vendor/src/github.com/docker/libnetwork/Dockerfile.build                  |    5 +
 vendor/src/github.com/docker/libnetwork/Makefile                          |   18 +-
 vendor/src/github.com/docker/libnetwork/controller.go                     |    4 +-
 vendor/src/github.com/docker/libnetwork/drivers/bridge/bridge.go          |    5 +-
 vendor/src/github.com/docker/libnetwork/drivers/bridge/interface.go       |   16 +
 vendor/src/github.com/docker/libnetwork/drivers/bridge/setup_ip_tables.go |   12 +
 vendor/src/github.com/docker/libnetwork/drivers/bridge/setup_ipv6.go      |   29 +-
 vendor/src/github.com/docker/libnetwork/drivers/bridge/setup_verify.go    |   15 +-
 vendor/src/github.com/docker/libnetwork/drivers/overlay/joinleave.go      |   11 +
 vendor/src/github.com/docker/libnetwork/drivers/overlay/ov_endpoint.go    |    7 +-
 vendor/src/github.com/docker/libnetwork/endpoint.go                       |   87 ++++-
 vendor/src/github.com/docker/libnetwork/iptables/iptables.go              |    8 +
 vendor/src/github.com/docker/libnetwork/netutils/utils.go                 |   60 +++
 vendor/src/github.com/docker/libnetwork/network.go                        |  127 +++---
 vendor/src/github.com/docker/libnetwork/osl/interface_linux.go            |    3 +-
 vendor/src/github.com/docker/libnetwork/resolver.go                       |  205 ++++++++++
 vendor/src/github.com/docker/libnetwork/sandbox.go                        |  180 ++++++++-

icecrime · 2016-01-09T00:13:37Z

As far as I can tell the only important Engine side impact to have in mind is this one.

I haven't tested yet, but code LGTM.

LK4D4 · 2016-01-09T00:22:44Z

@sanimej There is misspell in commit message - "diable DAD"

sanimej · 2016-01-09T00:33:39Z

@LK4D4 Corrected the typo.

cpuguy83 · 2016-01-09T02:51:13Z

vendor/src/github.com/docker/libnetwork/network.go

 		if ep != nil && strings.Split(h, ".")[0] == ep.Name() {
 			continue
 		}

 		recs = append(recs, etchosts.Record{
 			Hosts: h,
-			IP:    ip.String(),
+			IP:    ip[0].String(),


Will this ever panic?

no. there wont be a case when ip[0] would be nil.
having said that, there are some paths in the code that requires the svcMap should be locked during certain operations. We are tracking that to be fixed.

infact, this function is currently of no use (since we moved away from /etc/hosts) and we can even remove that,
@sanimej is that correct ?

@mavenugo Yes, the call to delete the service records from /etc/hosts is not necessary. What has to be removed is the self entry for IP of the endpoint that is being disconnected. This is a known issue. Will push a PR to address these.

Off topic, but we should really make sure that "no longer updating /etc/hosts" is explicitly mentioned in the changelog/release notes. I already had someone reporting that they currently have a filewatcher on /etc/hosts to automate things 😕

@sanimej thanks. Just to make sure we are clear, this PR isn't dependent on that change and it can go in independently.

@thaJeztah yes. this is a good candidate for release-notes. Added the changelog tag to reflect this.

Yes. This PR has no dependency on the change referred to in this comment.

thaJeztah · 2016-01-10T16:30:05Z

ping @jfrazelle @calavera @tonistiigi can you please have a look at this one; it's important for libnetwork to move forward

vdemeester · 2016-01-10T19:26:14Z

LGTM 🐰

thaJeztah · 2016-01-10T19:39:07Z

For documentation changes; does this also affect the way classic links work (I.e. are the aliases no longer written to /etc/hosts?) Asking, because there are some references to /etc/hosts being updated in the documentation; https://github.com/docker/docker/search?l=markdown&q=%22%2Fetc%2Fhosts%22&utf8=✓

vdemeester · 2016-01-10T19:45:08Z

@thaJeztah Tried the PR out and yes, /etc/hosts is not written anymore 😻.
I think we could make a documentation PR after this one (to make it reach master quick), wdyt ?

icecrime · 2016-01-10T19:46:25Z

Yes, let's do it this way to unblock Madhu. Thanks @vdemeester!

Vendoring libnetwork

thaJeztah · 2016-01-10T19:51:21Z

sgtm! I'll open an issue

thaJeztah · 2016-01-10T19:54:13Z

Opened #19221 to track docs changes

mavenugo · 2016-01-10T20:08:49Z

thanks @icecrime @thaJeztah @vdemeester

For documentation changes; does this also affect the way classic links work (I.e. are the aliases no longer written to /etc/hosts?)

There is no change to the classic links (in bridge network).
But when it comes to links aka locally-scoped alias for user-defined network, it will use the DNS method.

We certainly have to document the newly added link functionality for user-defined networks.
But existing link for bridge network stays as is (including /etc/hosts update).

thaJeztah · 2016-01-10T20:11:31Z

@mavenugo clear, thanks!

tonistiigi · 2016-01-11T18:07:54Z

Couple of issues/questions with the dns discovery:

Starting a container without user defined network and connecting to it later does not make other containers discoverable anymore.
Because we only handle A and PTR requests it means that usually we would require access to the external DNS server for local discovery(otherwise it hangs). It also means we leak all our internal server names and get a performance penalty. Maybe it would make more sense to handle the other requests as well(at least AAAA) and return empty response.
Why does PTR record only return name.network record and not name?
Why is TTL fixed to 1800?

sanimej · 2016-01-11T23:19:26Z

Starting a container without user defined network and connecting to it later does not make other containers discoverable anymore.

This is because of an incorrect check during network correct. I will push a PR shortly to fix this.

Because we only handle A and PTR requests it means that usually we would require access to the external DNS server for local discovery(otherwise it hangs). It also means we leak all our internal server names and get a performance penalty. Maybe it would make more sense to handle the other requests as well(at least AAAA) and return empty response.

So far (with /etc/hosts) we didn't have service discovery for IPv6. Currently the same behavior is maintained. But it should be possible to support AAAA queries with minimal changes. I am working on that..

Why does PTR record only return name.network record and not name?

PTR record typically gives the complete name. Also, in cases where service name exists on more than one network the container is connected to this can be used as a quick way to see in which network the name is getting resolved.

Why is TTL fixed to 1800?

Embedded DNS server directly interfaces with the host resolvers which doesn't cache the replies (apps might do; but they don't see the TTL value). So the TTL from embedded server is not of much significance. It might be relevant if someone runs a caching DNS server inside the container and have that talk to the embedded server. In that case also 30 mins seems to be a reasonable number.

tonistiigi · 2016-01-12T00:40:50Z

(apps might do; but they don't see the TTL value).

What do mean by "apps don't see TTL value"?

sanimej · 2016-01-12T00:45:47Z

apps use a resolver library and won't see the DNS reply directly.

Santhosh Manohar added 2 commits January 8, 2016 14:13

Docker changes for libnetwork vendoring..

64a6dc3

Signed-off-by: Santhosh Manohar <santhosh@docker.com>

GordonTheTurtle added the status/0-triage label Jan 8, 2016

icecrime added this to the 1.10.0 milestone Jan 8, 2016

icecrime added status/2-code-review and removed status/0-triage labels Jan 9, 2016

cpuguy83 reviewed Jan 9, 2016
View reviewed changes

mavenugo added the impact/changelog label Jan 9, 2016

This was referenced Jan 9, 2016

Adding global alias config fields in endpointsettings docker/engine-api#22

Merged

Adding link (local alias) config in endpointsettings docker/engine-api#20

Merged

icecrime pushed a commit that referenced this pull request Jan 10, 2016

Merge pull request #19198 from sanimej/vin

fe3d1f9

Vendoring libnetwork

icecrime merged commit fe3d1f9 into moby:master Jan 10, 2016

thaJeztah mentioned this pull request Jan 10, 2016

Update docs as /etc/hosts is no longer used for links/discovery #19221

Closed

sanimej deleted the vin branch January 11, 2016 23:20

This was referenced Jan 17, 2016

Container starts before network iface was upped #19238

Closed

IPV6 does not work during docker build command #16913

Closed

Networking stack not ready when building on Windows #19314

Closed

lanwen mentioned this pull request Jan 18, 2016

/etc/hosts does not contain IPv6 addresses (of linked containers) #13228

Closed

mountkin mentioned this pull request Feb 1, 2016

2 veth are created when **deleting** a container with overlay network #19884

Closed

davidgiesberg mentioned this pull request Feb 9, 2016

Default advertised hostname resolution broken in Docker 1.10.0 ches/docker-kafka#13

Closed

thaJeztah mentioned this pull request Apr 25, 2016

Proposal: deprecate "old" /etc/hosts based linking and always use embedded DNS #22295

Open

thaJeztah mentioned this pull request May 31, 2016

Linking containers doesn't work on host machine, but works anywhere else #23096

Closed

thaJeztah mentioned this pull request Mar 15, 2017

/etc/hosts not getting updated in user-defined network. #31835

Closed

thaJeztah mentioned this pull request Feb 22, 2024

Add a deprecation warning about --link on default nw docker/cli#4892

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vendoring libnetwork #19198

Vendoring libnetwork #19198

sanimej commented Jan 8, 2016

icecrime commented Jan 9, 2016

icecrime commented Jan 9, 2016

LK4D4 commented Jan 9, 2016

sanimej commented Jan 9, 2016

cpuguy83 Jan 9, 2016

mavenugo Jan 9, 2016

mavenugo Jan 9, 2016

sanimej Jan 9, 2016

thaJeztah Jan 9, 2016

mavenugo Jan 9, 2016

mavenugo Jan 9, 2016

sanimej Jan 9, 2016

thaJeztah commented Jan 10, 2016

vdemeester commented Jan 10, 2016

thaJeztah commented Jan 10, 2016

vdemeester commented Jan 10, 2016

icecrime commented Jan 10, 2016

thaJeztah commented Jan 10, 2016

thaJeztah commented Jan 10, 2016

mavenugo commented Jan 10, 2016

thaJeztah commented Jan 10, 2016

tonistiigi commented Jan 11, 2016

sanimej commented Jan 11, 2016

tonistiigi commented Jan 12, 2016

sanimej commented Jan 12, 2016

Vendoring libnetwork #19198

Vendoring libnetwork #19198

Conversation

sanimej commented Jan 8, 2016

icecrime commented Jan 9, 2016

icecrime commented Jan 9, 2016

LK4D4 commented Jan 9, 2016

sanimej commented Jan 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Jan 10, 2016

vdemeester commented Jan 10, 2016

thaJeztah commented Jan 10, 2016

vdemeester commented Jan 10, 2016

icecrime commented Jan 10, 2016

thaJeztah commented Jan 10, 2016

thaJeztah commented Jan 10, 2016

mavenugo commented Jan 10, 2016

thaJeztah commented Jan 10, 2016

tonistiigi commented Jan 11, 2016

sanimej commented Jan 11, 2016

tonistiigi commented Jan 12, 2016

sanimej commented Jan 12, 2016