Looking at ipv6Enabled(), the only case where ok == false is when sb.osSbox == nil. This means, if we enter this branch because ok == false, the next line will panic.

Ok, yes - the function would have returned early if osSbox == nil, so the check is just unnecessary (until some change happens in the implementation ipv6Enabled that means it returns !ok for some other reason).

That code goes away again in a later commit, so I've changed it anyway.

While the CLI prevents you from attaching to the default network and custom networks at the same time, there's no such check in the daemon. That means, this condition was previously defining a specific order of operations:

1. The default bridge network, if it's specified;
2. Then, any custom networks.

But since the order of network attachment defines the default gateway, removing that code means the default bridge might not be the default gateway now. Although the legacy --link justification doesn't hold anymore, we need to keep that old code / behavior for the time being (until we add a way to specify which attachment should be the default gateway).

After chatting more about this, I think you're right: the default gateway is changed whenever an endpoint that sorts first (in lexicographic order) is attached to the sandbox. We should be good to go with that change.

Marking as "unresolved" to keep these comments visible (making it easier to discover in future)

Just to note then ... the endpoints are sorted by gateway-ness, then by name. (So, the "best" choices of gateway endpoints are preferred, but equally good endpoints are just ordered lexicographically.)

For a follow-up; now that this code is in a platform-specific file, we should inline this function (makes it clearer what it does);

And remove the windows stub;

That's now #48821.

It's existing code, but this is confusing; i.e., we overwrite cEndpointID in each iteration, and cEndpointID is used elsewhere.

Oh! I see Cory commented about that as well; 68a4761#r1526578113

That link doesn't work - but I think it's #47406 (comment)

In any case, I think it's something for follow-up as it's not changed by this PR - which is already big enough!

Slightly tempted to keep this unwrapped; yes, it's a long line, but most of it can be skimped over, knowing that it's just the func's signature.

Done. Is the rule "no line-wrapping in function declarations"?

(To me, it's just slightly harder to read and see the return types. And diffs won't pick out future changes as clearly.)

Most of the codebase is written that way, but I admit I'm not a big fan of that. To @robmry's point, it's harder to read IMHO.

Tempted to say; if the list of arguments for a function is becoming so long that it needs wrapping, that's a very good indicator that the signature may be in need of an overhaul 😄

Two params and a return value plus context and error, seems ok? So we need shorter names?

nit (but there may be some linters complaining);

return daemon.updateNetworkSettings(ctr, n, endpointConfig)

Ok.

Curious (no need to change); did this panic somewhere? (in that case; are we missing a check somewhere to prevent that)?

Yes ...

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x48 pc=0x17aaf44]

goroutine 27 [running]:
testing.tRunner.func1.2({0x1aa5ce0, 0x3171920})
	/usr/local/go/src/testing/testing.go:1631 +0x1c4
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1634 +0x33c
panic({0x1aa5ce0?, 0x3171920?})
	/usr/local/go/src/runtime/panic.go:770 +0x124
github.com/docker/docker/daemon.(*Daemon).updateNetworkSettings(0x400051dd00, 0x400051dab8, 0x40000504e0, 0x40001a5b20)
	/go/src/github.com/docker/docker/daemon/container_operations.go:170 +0x224
github.com/docker/docker/daemon.(*Daemon).updateNetworkConfig(0x400051dd00, 0x400051dab8, 0x40000504e0, 0x40001a5b20)
	/go/src/github.com/docker/docker/daemon/container_operations.go:631 +0x2ac
github.com/docker/docker/daemon.TestDNSNamesOrder(0x4000563d40)
	/go/src/github.com/docker/docker/daemon/container_operations_test.go:35 +0x264
testing.tRunner(0x4000563d40, 0x1ee8270)
	/usr/local/go/src/testing/testing.go:1689 +0xec
created by testing.(*T).Run in goroutine 1
	/usr/local/go/src/testing/testing.go:1742 +0x318

Which comes from:

Looks like it's normally initialised in daemon.newContainer - and there's lots of code that assumes it exists:

Not entirely new (I think), but it caught my eye now;

This logic;

• Appends extrahosts
• Calls "rebuild" for the hosts file

But looking at the "rebuild";

1. It generates a new host-file (to replace the existing one?) with the new config (which includes the new "extra hosts")
2. After that, it updates the file for each endpoint

That last part is done in a loop, and for each iteration it adds an endpoint, and (from a quick look), locks the hosts-file and writes it to disk

I'm wondering;

• Could we could we skip the "updating" part if we would update the config before we call sb.buildHostsFile()?
• (or have sb.buildHostsFile() accept the list of "additional hosts" to add)
• ☝️ so that it has all the data it needs, and just has to construct (and write) once

func (sb *Sandbox) rebuildHostsFile(ctx context.Context) error {
	if err := sb.buildHostsFile(); err != nil {
		return errdefs.System(err)
	}
	for _, ep := range sb.Endpoints() {
		if err := sb.updateHostsFile(ctx, ep.getEtcHostsAddrs()); err != nil {
			return errdefs.System(err)
		}
	}
	return nil
}

If that's not an option, I noticed that sb.updateHostsFile() accepts a slice of addresses; so instead of updating it one at a time, would that mean we could just collect the endpoints, and pass it? Something like;

eps := sb.Endpoints()
addrs :=  make([]string, 0, len(eps))
for _, ep := range eps {
    addrs = append(addrs, ep.getEtcHostsAddrs()...)
    if err := sb.updateHostsFile(ctx, ep.getEtcHostsAddrs()); err != nil {
        return errdefs.System(err)
    }
}
if err := sb.updateHostsFile(ctx, addrs...); err != nil {
	// NOTE: should sb.updateHostsFile already return a proper errdefs type (errdefs.System)?
    return errdefs.System(err)
}
return nil

Sandbox.AddHostsEntry is only used for legacy --link options for containers on the default bridge network, before they're started - and those containers are only likely to be connected to the default bridge (only one Endpoint) ... so I don't think there's much to gain by optimising it.

Sandbox.rebuildHostsFile is called from SetKey for a newly created container once the its support for IPv6 is known. That container could be connected to multiple endpoints - so maybe there are some msecs to be gained by accumulating addresses from all of the endpoints. But, optimising the whole etchosts thing so that it doesn't keep writing/reading/merging the contents (before a container starts and the user's had a chance to modify it), is probably the way to go if that's an issue.

These /etc/hosts entries are pretty questionable anyway for user-defined networks that have an internal DNS resolver. If we figure out a fix for buildkit, so that we can use the internal resolver for the default network too, even more so.

I don't really want to muck about with it in this PR, maybe as a follow-up if needed?

This is now #48823

Mostly curious; is there any order in the results? In this case, should "last" or "first" take precedence? (currently the first one is used, and the last one is discarded; wondering if we should loop in reverse).

This PR only changes whitespace here ... but no, I don't think the order can matter - the results come from a map in the first place, and DNS resolvers are free to rotate addresses in responses anyway to balance load.

Not relevant for this PR, but this could uses a map[string]struct{} to avoid allocations.