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

Vendoring libnetwork #19198

Merged
merged 2 commits into from
Jan 10, 2016
Merged

Vendoring libnetwork #19198

merged 2 commits into from
Jan 10, 2016

Conversation

sanimej
Copy link

@sanimej sanimej commented Jan 8, 2016

Changes include..

Fixes #18814
Fixes #19139
Fixes #18871

Santhosh Manohar added 2 commits January 8, 2016 14:13
- 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 icecrime added this to the 1.10.0 milestone Jan 8, 2016
@icecrime
Copy link
Contributor

icecrime commented Jan 9, 2016

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
Copy link
Contributor

icecrime commented Jan 9, 2016

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
Copy link
Contributor

LK4D4 commented Jan 9, 2016

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

@sanimej
Copy link
Author

sanimej commented Jan 9, 2016

@LK4D4 Corrected the typo.

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

recs = append(recs, etchosts.Record{
Hosts: h,
IP: ip.String(),
IP: ip[0].String(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this ever panic?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😕

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@thaJeztah
Copy link
Member

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

@vdemeester
Copy link
Member

LGTM 🐰

@thaJeztah
Copy link
Member

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
Copy link
Member

@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
Copy link
Contributor

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

icecrime pushed a commit that referenced this pull request Jan 10, 2016
@icecrime icecrime merged commit fe3d1f9 into moby:master Jan 10, 2016
@thaJeztah
Copy link
Member

sgtm! I'll open an issue

@thaJeztah
Copy link
Member

Opened #19221 to track docs changes

@mavenugo
Copy link
Contributor

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
Copy link
Member

@mavenugo clear, thanks!

@tonistiigi
Copy link
Member

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
Copy link
Author

sanimej commented Jan 11, 2016

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.

@sanimej sanimej deleted the vin branch January 11, 2016 23:20
@tonistiigi
Copy link
Member

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

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

@sanimej
Copy link
Author

sanimej commented Jan 12, 2016

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

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

Successfully merging this pull request may close these issues.

9 participants