I think I just spotted a bug in our logic (unrelated to this PR) here;

In the above, we copy the hosts's /etc/hosts (sb.config.originHostsPath) for the container, but ONLY if there's no extra-hosts. If extra hosts ARE provided, we fall through, and create a fresh /etc/hosts for the container, which means that we're not preserving entries from /etc/hosts in that case?

☝️ I think we need to always do the copyFile, but we can return early if we don't have any extraHosts to add 🤔

i.e., something like;

if sb.config.useDefaultSandBox {
	// We are working under the assumption that the origin file option had been properly expressed by the upper layer
	// if not here we are going to error out
	if err := copyFile(sb.config.originHostsPath, sb.config.hostsPath); err != nil && !os.IsNotExist(err) {
		return types.InternalErrorf("could not copy source hosts file %s to %s: %v", sb.config.originHostsPath, sb.config.hostsPath, err)
	}
	if len(sb.config.extraHosts) == 0 {
		// nothing to add; we're done.
		return nil
	}
}

⚠️ ☝️ maybe some additional changes are needed to make sure we append the extraHosts (not replace the content)

I think I just spotted a bug in our logic (unrelated to this PR) here;

I'm not convinced that's an unintentional bug. This was introduced in:

• Proper handling sandbox options libnetwork#2232

This PR doesn't justify why this condition was tighten, but it references this issue:

• Ubuntu 18.04 + Docker 17.12.1-ce break DNS resolution libnetwork#2187

Which has no mentions of /etc/hosts except this comment: moby/libnetwork#2187 (comment).

Documentation ref:
https://docs.docker.com/config/containers/container-networking/#dns-services
"By default, a container inherits the DNS settings of the Docker daemon, including the /etc/hosts and /etc/resolv.conf. You can override these settings on a per-container basis"

Currently the docs says (see here):

Your container will have lines in /etc/hosts which define the hostname of the container itself, as well as localhost and a few other common things. Custom hosts, defined in /etc/hosts on the host machine, aren't inherited by containers.

Now let's imagine the following situation:

1. You have foo.bar pointing to 10.0.0.10 in your host's /etc/hosts ;
2. And you want to change that in your --net=host container to point to 192.168.0.10 ;

With your proposal, the container would end up with two entries, one for each IP address. libresolv would then pick up either one randomly. So, there would be no way to override what's set in the host's /etc/hosts (at least not without an entrypoint mangling /etc/hosts). I'm wrong actually, libresolv will only take the first entry into consideration. BUT Go returns both addresses:

❯ /bin/cat main.go
package main

import (
	"fmt"
	"net"
)

func main() {
	addrs, err := net.LookupHost("foo.bar")
	if err != nil {
		panic(err)
	}
	fmt.Printf("%+v\n", addrs)
}

❯ go run main.go
[10.0.0.10 192.168.0.10]

I'm afraid that if we implement what you're suggesting, we're going to break someone, somewhere.

That's way to late, but I think the correct UX would have been to disallow --add-host on --net=host containers -- /etc/hosts being part of the mount namespace instead of the netns is mostly an implementation details that should have been properly hidden from users.

EDIT: I think we should add a proper comment stating why it's done that way. I always find this condition confusing and need a bit of time to find out a good reason to keep it that way.

Thank you @thaJeztah ... I cherry-picked your commits to this branch (wasn't quite sure what github would do if I merged your PR) - they all look good, I didn't see any reason to squash or modify anything.

While we're in the area, it's tempting to deal with this FIXME, but there's no need to mix a functional change with the refactoring in this PR so I'll try to look at it separately ...

That's way to late, but I think the correct UX would have been to disallow --add-host on --net=host containers -- /etc/hosts being part of the mount namespace instead of the netns is mostly an implementation details that should have been properly hidden from users.

Yes, that was my other thinking as well; when running with --net=host, the container is basicaly a "guess" of the host's networking namespace, but (likely?) should have no control over it (i.e., joining the host's network should make the container act as close as possible to not being containerised from a networking perspective).

Currently we're doing something in between and we either use /etc/hosts from the host, OR "extra-hosts" defined by the user (and drop the ones from the host). The only thing I'm wondering is if the host.docker.internal case is something that would require an exception, but maybe not.

EDIT: I think we should add a proper comment stating why it's done that way. I always find this condition confusing and need a bit of time to find out a good reason to keep it that way.

At least 💯 on that. I also think we should still consider the "produce an error" idea; ISTR we did something for that when trying to use -p (port mapping) when using --net=host, but don't recall if we chose to use an error or print a warning ("mapping discarded")

wasn't quite sure what github would do if I merged your PR

Merging works as well (but you'd get a "merge" commit in your branch, so usually a "rebase" after to get rid of those). It's pretty neat actually, as it's an easy way to bring in suggestions.

Perhaps (but not a "strong" blocker sqash the "rename var that shadowed type" commit with your second commit; that would make git-blame slightly easier to find back the actually relevant changes (instead of seeing "rename var")

Perhaps (but not a "strong" blocker sqash the "rename var that shadowed type" commit with your second commit; that would make git-blame slightly easier to find back the actually relevant changes (instead of seeing "rename var")

Done.

Merging works as well (but you'd get a "merge" commit in your branch, so usually a "rebase" after to get rid of those). It's pretty neat actually, as it's an easy way to bring in suggestions.

Ah, thank you - it was the merge commit I wanted to avoid. Will do that next time.

EDIT: I think we should add a proper comment stating why it's done that way. I always find this condition confusing and need a bit of time to find out a good reason to keep it that way.

I've been around that loop before too - so, added a comment pointing at @akerouanton's analysis above.