Don't drop host aliases from swarm extra hosts - #53338

Plain containers already work like this, docker run --add-host "host1 host2:1.1.1.1"

🤔 I actually wonder if that's intentional, or just a missing validation / handling for the spaces; <hostname>:<ip-address> using a colon as separator was the legacy format (but still handled), and now replaced with <hostname>=<ip-address>.

At a quick glance, at least passing multiple hostnames there isn't documented, so if it works, that's very well possible undocumented behavior, and not intentional; https://docs.docker.com/reference/cli/docker/container/run/#add-host

You can add other hosts into a container's /etc/hosts file by using one or more --add-host flags. This example adds a static address for a host named my-hostname:

$ docker run --add-host=my-hostname=8.8.8.8 --rm -it alpine

....
You can wrap an IPv6 address in square brackets:

$ docker run --add-host my-hostname=[2001:db8::33] --rm -it alpine

if it works, that's very well possible undocumented behavior, and not intentional

I went through the history a bit. You are right that the multi name form is not documented anywhere, the docs and the API reference only show a single hostname, and no test covers it. But it does not look like an accident to me either. The original --add-host PR (#8019) described the flag as "appends lines to /etc/hosts", the validation never restricted the hostname part in twelve years, including the recent = separator rework, and the hosts file writer supports multi name records on purpose.

Since the behavior has been there and stable for that long, I would rather make it official than treat it as an accident. I can document it (the ExtraHosts description in the API, and the --add-host docs) and add tests to pin it down, for ValidateExtraHost and for the resulting hosts file content, either in this PR or as a follow-up. Would that work for you?

Is adding multiple --host options a solution for your use-case?

docker service create --name myservice --host host1:1.1.1.1 --host host2:1.1.1.1 nginx:alpine

docker ps -n1
CONTAINER ID   IMAGE          COMMAND                  CREATED              STATUS              PORTS     NAMES
919895b58df9   nginx:alpine   "/docker-entrypoint.…"   About a minute ago   Up About a minute   80/tcp    myservice.1.tl0ocyp0vkpln9wzr8qi0cp9p

docker exec 919895b58df9 cat /etc/hosts
127.0.0.1	localhost
::1	localhost ip6-localhost ip6-loopback
fe00::	ip6-localnet
ff00::	ip6-mcastprefix
ff02::1	ip6-allnodes
ff02::2	ip6-allrouters
1.1.1.1	host1
1.1.1.1	host2
172.17.0.2	919895b58df9

The docker run case clearly looks to be missing validation on the host part; it takes anything before the first colon or equal as "hostname";

docker run --rm \
    --add-host "host1 host2=1.1.1.1" \
    --add-host "host2=1.1.1.1" \
    --add-host "host2 host1=1.1.1.1" \
    --add-host "host2=2.2.2.2" \
    --add-host "host1 # I can haz comments=2.2.2.2" \
    --add-host "host3,host4,host5=2.2.2.2" \
    --add-host "$(printf 'host6\n# comment line\nhost7 host8 1.1.1.1=2.2.2.2')" \
    alpine cat /etc/hosts

127.0.0.1	localhost
::1	localhost ip6-localhost ip6-loopback
fe00::	ip6-localnet
ff00::	ip6-mcastprefix
ff02::1	ip6-allnodes
ff02::2	ip6-allrouters
1.1.1.1	host1 host2
1.1.1.1	host2
1.1.1.1	host2 host1
2.2.2.2	host2
2.2.2.2	host1 # I can haz comments
2.2.2.2	host3,host4,host5
2.2.2.2	host6
# comment line
host7 host8 1.1.1.1
172.17.0.3	848ddcee0411

We should fix that, because it's clearly not intentional; for swarm services, this would likely also be problematic for docker service update, which allows adding/removing prior options (--host-add / --host-rm), which I think (need to double-check) also allows adding/removing "all entries of host".

If this is an important use-case, we should probably look at an alternative syntax, similar to what we did for --volume / --mount, for example;

# short form (legacy)
--add-host host1:1.1.1.1

# short form
--add-host host1=1.1.1.1

# advanced syntax
--add-host name=host1,alias=host2,alias=host3,ip=1.1.1.1

And, likely send it in more structured format over the API; something like;

type HostEntry struct {
	IP      netip.Addr
	Name    string
	Aliases []string
}

Is adding multiple --host options a solution for your use-case?

For plain name to IP resolution, yes, multiple --host flags work.

The two layouts are not the same for the resolver though. On a single line the first name is the canonical hostname and the rest are aliases, on separate lines every name is its own canonical name:

$ # one line: "1.1.1.1  host1 host2"
$ docker run --rm --add-host 'host1 host2:1.1.1.1' debian:stable-slim getent hosts host2
1.1.1.1         host1 host2

$ # separate lines: "1.1.1.1  host1" and "1.1.1.1  host2"
$ docker run --rm --add-host host1:1.1.1.1 --add-host host2:1.1.1.1 debian:stable-slim getent hosts host2
1.1.1.1         host2

gethostbyname() and getaddrinfo() with AI_CANONNAME report host1 as the canonical name for host2 in the first case, and host2 itself in the second. Anything that canonicalizes hostnames (Kerberos principal resolution for example) behaves differently between the two.

The main problem this PR wants to fix is not the exact file layout. The Hosts field in ContainerSpec documents the IP_address canonical_hostname [aliases...] format, the API accepts such entries today, and service update --host-rm handles multi name entries per name (docker/cli#1054). The executor is the only place that silently drops the aliases, so an entry that goes in through the documented API comes out of the task incomplete. That is the data loss I want to fix here, independent of what happens to the --add-host syntax on the client side.

for swarm services, this would likely also be problematic for docker service update, which allows adding/removing prior options (--host-add / --host-rm), which I think (need to double-check) also allows adding/removing "all entries of host".

that part already deals with multi name entries correctly. Removal is per name, not per entry, that was made granular in docker/cli#1054:

$ docker service create --name rmtest --host 'host1 host2:1.1.1.1' busybox sleep 600
$ docker service inspect rmtest --format '{{json .Spec.TaskTemplate.ContainerSpec.Hosts}}'
["1.1.1.1 host1 host2"]

$ docker service update --host-rm host2 rmtest
$ docker service inspect rmtest --format '{{json .Spec.TaskTemplate.ContainerSpec.Hosts}}'
["1.1.1.1 host1"]

The two layouts are not the same for the resolver though. On a single line the first name is the canonical hostname and the rest are aliases, on separate lines every name is its own canonical name:

Thanks! Yes, I'm aware they're not identical; it was me being curious if your use-case was specifically for this purpose, or if the existing "multiple host entries" would work as a solution (at least for now).

The Hosts field in ContainerSpec documents the IP_address canonical_hostname [aliases...] format, the API accepts such entries today, and service update --host-rm handles multi name entries per name (docker/cli#1054)

You're right; I was mostly looking at the "container" equivalent, which at the code-level is pretty under-defined;

For the (follow-up) work on the non-swarm (container) bit; just writing it down here, but probably could use a separate ticket;

Perhaps instead of a structured format in the API, we should follow the same approach as Swarm does; the format used in /etc/hosts is pretty straightforward, and should probably work ok for this (perhaps some normalization could be useful if we don't have this, to avoid unwanted updates due to whitespace changes).

For the container part, besides fixing the CLI-side, I think we should do a transition to a new field;

• Add Hosts []string, using the same format as used for swarm.ContainerSpec
• Improve validation on the daemon side; we could (temporarily) allow the undocumented format, but should reject clearly malformed ones.
• For new API versions, use Hosts (client)
• If both Hosts and ExtraHosts are set, then prefer Hosts (alternatively; produce an error if both are set and/or if both are set with conflicting values).
• For persisting state, we can use the new Hosts field, and migrate pre-existing state, and back-fil the legacy field for older API versions.