Permit '=' separator and '[ipv6]' in --add-host by robmry · Pull Request #4663 · docker/cli

robmry · 2023-11-16T10:36:55Z

- What I did

Make it easier to specify IPv6 addresses in the '--add-host' option by permitting 'host=ip' in addition to 'host:ip', and allowing square brackets around the address.

For example:

--add-host=hostname:127.0.0.1
--add-host=hostname:::1
--add-host=hostname=::1
--add-host=hostname:[::1]

- How I did it

To avoid compatibility problems, the CLI will replace an '=' separator with ':', and strip brackets, before sending the request to the API.

- How to verify it

New unit tests pass.

The docker run command sets up a /etc/hosts entry as expected when using '=' and brackets ...

# /usr/local/cli/cli/build/docker run -ti --add-host thishost=[::1] alpine sh
/ # cat /etc/hosts
127.0.0.1       localhost
::1     localhost ip6-localhost ip6-loopback
fe00::0 ip6-localnet
ff00::0 ip6-mcastprefix
ff02::1 ip6-allnodes
ff02::2 ip6-allrouters
::1     thishost
172.18.0.2      43c134b9fad0

But docker inspect still shows the option with a ':' separator and no brackets ...

# /usr/local/cli/cli/build/docker inspect 43 -f '{{ .HostConfig.ExtraHosts }}'
[thishost:::1]

The same formats work for legacy docker build, buildx needs a separate change ...

root@6b6f43dcb70f:/go/src/github.com/docker/docker/a# cat Dockerfile
FROM alpine:latest
RUN ping -c1 something
RUN ping -c1 somethingv6

root@6b6f43dcb70f:/go/src/github.com/docker/docker/a# DOCKER_BUILDKIT=0 /usr/local/cli/cli/build/docker build --no-cache --add-host=something=8.8.8.8 --add-host somethingv6=[::1] .
DEPRECATED: The legacy builder is deprecated and will be removed in a future release.
            BuildKit is currently disabled; enable it by removing the DOCKER_BUILDKIT=0
            environment-variable.

Sending build context to Docker daemon  3.072kB
Step 1/3 : FROM alpine:latest
 ---> 3cc203321400
Step 2/3 : RUN ping -c1 something
 ---> Running in e4bbbe9f65d4
PING something (8.8.8.8): 56 data bytes
64 bytes from 8.8.8.8: seq=0 ttl=62 time=16.070 ms

--- something ping statistics ---
1 packets transmitted, 1 packets received, 0% packet loss
round-trip min/avg/max = 16.070/16.070/16.070 ms
 ---> Removed intermediate container e4bbbe9f65d4
 ---> 13397ec9bb56
Step 3/3 : RUN ping -c1 somethingv6
 ---> Running in 39453415243e
PING somethingv6 (::1): 56 data bytes
64 bytes from ::1: seq=0 ttl=64 time=0.024 ms

--- somethingv6 ping statistics ---
1 packets transmitted, 1 packets received, 0% packet loss
round-trip min/avg/max = 0.024/0.024/0.024 ms
 ---> Removed intermediate container 39453415243e
 ---> e968e3d10d73
Successfully built e968e3d10d73

- Description for the changelog

Permit '=' separator and '[ipv6]' in --add-host

thaJeztah

just some quick comments from my comments phone ☺️

thaJeztah · 2023-11-16T12:50:26Z


 	// Network and port publishing flag
-	flags.Var(&copts.extraHosts, "add-host", "Add a custom host-to-IP mapping (host:ip)")
+	flags.Var(&copts.extraHosts, "add-host", "Add a custom host-to-IP mapping (host:ip, or host=ip)")


I'm slightly leaning towards "soft deprecating" the old format, and not advertising it too prominent.

In the docs, we could still make a mention of the old format (for users who are running an older version of the cli)

Ah, yes - good idea! Will do, thank you.

thaJeztah · 2023-11-16T12:51:57Z

 $ docker build --add-host docker:10.180.0.1 .
 ```

+For IPv6 addresses, the equivalent `host=ip` form may be clearer, for example:


Related to my other comment; we can use the new format as default for the example, and make a mention of the old format.

(also /cc @dvdksn for suggestions)

Ok, will do.

dvdksn

looks good, a couple suggestions

codecov-commenter · 2023-11-17T09:51:00Z

Codecov Report

Merging #4663 (a682b8e) into master (af23916) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4663      +/-   ##
==========================================
+ Coverage   59.75%   59.77%   +0.01%     
==========================================
  Files         287      287              
  Lines       24821    24830       +9     
==========================================
+ Hits        14832    14841       +9     
  Misses       9103     9103              
  Partials      886      886

dvdksn

docs lgtm 👍🏻

thaJeztah

Thanks! I left some comments; non-critical, but some would probably be good to address before merging (happy to hear your thoughts!)

thaJeztah · 2023-11-21T11:13:40Z


 **--add-host** []
-   Add a custom host-to-IP mapping (host:ip)
+   Add a custom host-to-IP mapping (host:ip, or host=ip)


Micro-nit; perhaps swap the order to put the preference on =;

Add a custom host-to-IP mapping (host=ip, or host:ip)

Yes, will do. (Also makes it consistent with other text that I'd already fixed.)

thaJeztah · 2023-11-21T11:13:51Z


 **--add-host**=[]
-   Add a custom host-to-IP mapping (host:ip)
+   Add a custom host-to-IP mapping (host:ip, or host=ip)


thaJeztah · 2023-11-21T11:43:22Z

+The `--add-host` flag also accepts a `:` separator, for example:
+
+```console
+$ docker run --add-host=docker:93.184.216.34 --rm -it alpine


For examples, I try to use names that help identify they're "custom" values (and for illustrative purpose). Using docker (or hostname as was used elsewhere) could be confused for "docker" being some special value.

Something like my-hostname could perhaps work;

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

I'm curious where the 93.184.216.34 address came from; it

It's also good to make an example "do" something. In this case, we want to illustrate that adding the hostname allows the hostname to be resolved inside the container. I'm curious where the 93.184.216.34 IP-address came from, but if we want to be sure ping works, perhaps we should use a well-known IP-address, e.g. 8.8.8.8;

$ docker run --add-host=my-hostname:8.8.8.8 --rm alpine ping -c1 my-hostname PING my-hostname (8.8.8.8): 56 data bytes 64 bytes from 8.8.8.8: seq=0 ttl=63 time=14.995 ms --- my-hostname ping statistics --- 1 packets transmitted, 1 packets received, 0% packet loss round-trip min/avg/max = 14.995/14.995/14.995 ms

The advantage of ping of course is that (most) users would understand what we're doing. Alternatively (if we don't want to depend on networking, and only illustrate the actual effect), we could show that the entry was added to /etc/hosts inside the container (but not sure if that's to be considered too much an implementation details);

$ docker run --add-host=my-hostname:8.8.8.8 --rm alpine sh -c 'cat /etc/hosts | grep my-hostname' 8.8.8.8 my-hostname

I considered nslookup as alternative, but 🙈 of course that doesn't work with /etc/hosts, so we'd need something like getent, which probably isn't ideal (not as widely used; at least I had to quickly lookup the right way to use it 😂)

$ docker run --add-host=my-hostname:8.8.8.8 --rm alpine getent ahosts my-hostname 8.8.8.8 STREAM my-hostname 8.8.8.8 DGRAM my-hostname

Oh! Before I forget (and probably not needed for these examples); if we use example domains in our docs; always make sure to use one of the designated domains for that purpose, such as *.example.com, *.example.org (https://www.rfc-editor.org/rfc/rfc6761.html).

(We've had cases where we used some (assumed) made-up domain, which... was in active use 😬)

docker:93.184.216.34 was already used in the ping example (the address was introduced in 47ba76a ) ... so I kept it, but something like my-hostname:8.8.8.8 looks better, I'll change it.

"ping" seems like a nice simple/clear example, and with 8.8.8.8 it should normally work, so I'll keep it.

thaJeztah · 2023-11-21T11:46:52Z

+//	hostname:127.0.0.1
+//	hostname:::1
+//	hostname=::1
+//	hostname:[::1]


See my other comment; perhaps use my-hostname or something similar, to prevent hostname being confused for a special keyword.

Makes sense - will do!

thaJeztah · 2023-11-21T11:50:12Z

+		// ValidateIPAddress returns the address in canonical form (for example,
+		// 0:0:0:0:0:0:0:1 -> ::1). But, stick with the original form, to avoid
+		// surprising a user who's expecting to see the address they supplied in the
+		// output of 'docker inspect' or '/etc/hosts'.
 		if _, err := ValidateIPAddress(v); err != nil {


Not for this PR, but still considering using the normalised variant; that would potentially avoid duplicates;

docker run --add-host=my-hostname:0:0:0:0:0:0:0:1 --add-host=my-hostname:::1 --rm alpine sh -c 'cat /etc/hosts | grep my-hostname' 0:0:0:0:0:0:0:1 my-hostname ::1 my-hostname

Ah, yes, I hadn't thought about duplicates - was more worried about second-guessing the user. I think only the last entry will be used if there are duplicates. I suppose we could use the canonical form to check for duplicates internally, and preserve the user's address in the file.

thaJeztah · 2023-11-21T11:51:21Z

+or more `--add-host` flags. This example adds static addresses for hosts named
+`docker` and `dockerv6`:

 ```console
-$ docker build --add-host docker:10.180.0.1 .
+$ docker build --add-host docker=10.180.0.1 --add-host dockerv6=2001:db8::33 .


While we're updating these, perhaps also update the example hostnames (per my other comment)

thaJeztah · 2023-11-21T11:54:53Z

+		},
+		{
+			doc:         "Missing address, eq sep",
+			input:       `missing.address.eq=`,


For tests, I also tend to use the special domain-names (example.com, example.org, and/or the .invalid TLDs) where possible of course.

Unlikely to happen for a unit-test, but I've had some test-cases in other repositories happily sending requests to some random domains that actually existed 🙈

Ah, yes - that'd be better, will do.

thaJeztah · 2023-11-23T22:04:06Z

Looks like this needs a rebase 🙈

thaJeztah · 2023-12-07T16:44:42Z


 	// Network and port publishing flag
-	flags.Var(&copts.extraHosts, "add-host", "Add a custom host-to-IP mapping (host:ip)")
+	flags.Var(&copts.extraHosts, "add-host", "Add a custom host-to-IP mapping (host=ip)")


@robmry as we're still debating the "race condition" between buildx and the cli; perhaps we should consider changing the flag description back to host:ip, and update it to the new format for v26 (release after v25) WDYT?

Yes - sure, I'll do that now ...

Fixes docker#4648 Make it easier to specify IPv6 addresses in the '--add-host' option by permitting 'host=ip' in addition to 'host:ip', and allowing square brackets around the address. For example: --add-host=my-hostname:127.0.0.1 --add-host=my-hostname:::1 --add-host=my-hostname=::1 --add-host=my-hostname:[::1] To avoid compatibility problems, the CLI will replace an '=' separator with ':', and strip brackets, before sending the request to the API. Signed-off-by: Rob Murray <[email protected]>

thaJeztah

LGTM

let me do a quick check if consensus was reached on including the new format in buildx 0.12.x

thaJeztah · 2023-12-11T15:40:53Z

Let's bring this one in; looks like buildx will get this format in v0.13.0, so there will be some period where they don't match, but don't think there's a good way around that (without docker/buildx#2149)

robmry requested a review from thaJeztah as a code owner November 16, 2023 10:36

robmry force-pushed the 4648-clearer_ipv6_in_add-host branch from 249cc14 to 0cecb05 Compare November 16, 2023 12:35

thaJeztah reviewed Nov 16, 2023

View reviewed changes

robmry force-pushed the 4648-clearer_ipv6_in_add-host branch from 0cecb05 to cf631cd Compare November 16, 2023 16:24

dvdksn reviewed Nov 17, 2023

View reviewed changes

Comment thread docs/reference/commandline/build.md Outdated

Comment thread docs/reference/commandline/run.md Outdated

Comment thread docs/reference/commandline/run.md Outdated

Comment thread man/docker-build.1.md Outdated

Comment thread man/docker-run.1.md Outdated

robmry force-pushed the 4648-clearer_ipv6_in_add-host branch from cf631cd to 59df3e9 Compare November 17, 2023 09:50

dvdksn approved these changes Nov 17, 2023

View reviewed changes

thaJeztah mentioned this pull request Nov 17, 2023

Permit '=' separator and '[ipv6]' in --add-host docker/buildx#2121

Merged

thaJeztah added this to the 25.0.0 milestone Nov 17, 2023

thaJeztah added impact/changelog kind/enhancement status/2-code-review area/ux labels Nov 17, 2023

thaJeztah requested changes Nov 21, 2023

View reviewed changes

robmry force-pushed the 4648-clearer_ipv6_in_add-host branch from 59df3e9 to 974b5d4 Compare November 21, 2023 14:32

thaJeztah assigned robmry Nov 23, 2023

robmry force-pushed the 4648-clearer_ipv6_in_add-host branch from 974b5d4 to 3068630 Compare November 23, 2023 22:18

thaJeztah mentioned this pull request Dec 4, 2023

[v0.12 backport] Permit '=' separator and '[ipv6]' in --add-host docker/buildx#2149

Closed

thaJeztah reviewed Dec 7, 2023

View reviewed changes

robmry force-pushed the 4648-clearer_ipv6_in_add-host branch from 3068630 to a682b8e Compare December 7, 2023 18:30

thaJeztah approved these changes Dec 8, 2023

View reviewed changes

thaJeztah merged commit 4dd5c23 into docker:master Dec 11, 2023

robmry deleted the 4648-clearer_ipv6_in_add-host branch December 11, 2023 15:47

This was referenced Dec 18, 2023

cli: use the new add-host separator docker/docs#18950

Closed

cli: use the new add-host separator docker/docs#18955

Merged

dvdksn mentioned this pull request Feb 7, 2024

Example for host-gateway doesn't work docker/docs#19335

Closed

1 task

dvdksn mentioned this pull request Jul 1, 2024

invalid argument "host.docker.internal=host-gateway" for "--add-host" flag docker/docs#20297

Closed

1 task

Conversation

robmry commented Nov 16, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvdksn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dvdksn left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Dec 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

codecov-commenter commented Nov 17, 2023 •

edited

Loading