ipv6 addresses with square brackets cannot be used with the --add-host option

[coilysiren]

Description

Using any ipv6 address with --add-host produces an error:

for "--add-host" flag: invalid IP address in add-host: <ipv6 address>

Reproduce

$ docker run --add-host "host.docker.internal:[::1]" nginx

> invalid argument "host.docker.internal:[::1]" for "--add-host" flag: invalid IP address in add-host: "[::1]"
> See 'docker run --help'.

Expected behavior

I am looking to be able to curl -g -6 "http://[::1]:8080/" from inside of the container. This is not possible, as --add-host does not allow specifying [::1] as the host's address.

docker version

docker version
Client:
 Cloud integration: v1.0.35+desktop.5
 Version:           24.0.6
 API version:       1.43
 Go version:        go1.20.7
 Git commit:        ed223bc
 Built:             Mon Sep  4 12:28:49 2023
 OS/Arch:           darwin/arm64
 Context:           desktop-linux

Server: Docker Desktop 4.25.0 (126437)
 Engine:
  Version:          24.0.6
  API version:      1.43 (minimum version 1.12)
  Go version:       go1.20.7
  Git commit:       1a79695
  Built:            Mon Sep  4 12:31:36 2023
  OS/Arch:          linux/arm64
  Experimental:     true
 containerd:
  Version:          1.6.22
  GitCommit:        8165feabfdfe38c65b599c4993d227328c231fca
 runc:
  Version:          1.1.8
  GitCommit:        v1.1.8-0-g82f18fe
 docker-init:
  Version:          0.19.0
  GitCommit:        de40ad0

docker info

docker info
Client:
 Version:    24.0.6
 Context:    desktop-linux
 Debug Mode: false
 Plugins:
  buildx: Docker Buildx (Docker Inc.)
    Version:  v0.11.2-desktop.5
    Path:     /Users/kai/.docker/cli-plugins/docker-buildx
  compose: Docker Compose (Docker Inc.)
    Version:  v2.23.0-desktop.1
    Path:     /Users/kai/.docker/cli-plugins/docker-compose
  dev: Docker Dev Environments (Docker Inc.)
    Version:  v0.1.0
    Path:     /Users/kai/.docker/cli-plugins/docker-dev
  extension: Manages Docker extensions (Docker Inc.)
    Version:  v0.2.20
    Path:     /Users/kai/.docker/cli-plugins/docker-extension
  init: Creates Docker-related starter files for your project (Docker Inc.)
    Version:  v0.1.0-beta.9
    Path:     /Users/kai/.docker/cli-plugins/docker-init
  sbom: View the packaged-based Software Bill Of Materials (SBOM) for an image (Anchore Inc.)
    Version:  0.6.0
    Path:     /Users/kai/.docker/cli-plugins/docker-sbom
  scan: Docker Scan (Docker Inc.)
    Version:  v0.26.0
    Path:     /Users/kai/.docker/cli-plugins/docker-scan
  scout: Docker Scout (Docker Inc.)
    Version:  v1.0.9
    Path:     /Users/kai/.docker/cli-plugins/docker-scout

Server:
 Containers: 33
  Running: 0
  Paused: 0
  Stopped: 33
 Images: 23
 Server Version: 24.0.6
 Storage Driver: overlay2
  Backing Filesystem: extfs
  Supports d_type: true
  Using metacopy: false
  Native Overlay Diff: true
  userxattr: false
 Logging Driver: json-file
 Cgroup Driver: cgroupfs
 Cgroup Version: 2
 Plugins:
  Volume: local
  Network: bridge host ipvlan macvlan null overlay
  Log: awslogs fluentd gcplogs gelf journald json-file local logentries splunk syslog
 Swarm: inactive
 Runtimes: io.containerd.runc.v2 runc
 Default Runtime: runc
 Init Binary: docker-init
 containerd version: 8165feabfdfe38c65b599c4993d227328c231fca
 runc version: v1.1.8-0-g82f18fe
 init version: de40ad0
 Security Options:
  seccomp
   Profile: unconfined
  cgroupns
 Kernel Version: 6.4.16-linuxkit
 Operating System: Docker Desktop
 OSType: linux
 Architecture: aarch64
 CPUs: 10
 Total Memory: 7.661GiB
 Name: linuxkit-a6b776a9963f
 ID: b7de54a1-66ec-4ab2-b3d6-ddea4d05bfdc
 Docker Root Dir: /var/lib/docker
 Debug Mode: false
 HTTP Proxy: http.docker.internal:3128
 HTTPS Proxy: http.docker.internal:3128
 No Proxy: hubproxy.docker.internal
 Experimental: true
 Insecure Registries:
  hubproxy.docker.internal:5555
  127.0.0.0/8
 Live Restore Enabled: false

WARNING: daemon is not using the default seccomp profile

Additional Info

daemon.json file:

{
  "builder": {
    "gc": {
      "defaultKeepStorage": "20GB",
      "enabled": true
    }
  },
  "experimental": true,
  "fixed-cidr-v6": "fdaa::/16",
  "ip6tables": true,
  "ipv6": true
}

[thaJeztah]

Thanks for reporting; can you try specifying the IP-address without square brackets?

IPv6 addresses surrounded by square-brackets are commonly used for URLs, to prevent ambiguity if the IP-address is followed by a port ([::1]:8080), which does not apply here.

I had a quick peek where this validation happens;

This error comes from

cli/opts/opts.go

Lines 228 to 229 in b90a3d4

	func ValidateIPAddress(val string) (string, error) {
	ip := net.ParseIP(strings.TrimSpace(val))

which uses Golang's stdlib net.ParseIP(), which looks to only support bare IPv6 addresses without brackets; https://pkg.go.dev/net#ParseIP

ParseIP parses s as an IP address, returning the result. The string s can be in IPv4 dotted decimal ("192.0.2.1"), IPv6 ("2001:db8::68"), or IPv4-mapped IPv6 ("::ffff:192.0.2.1") form. If s is not a valid textual representation of an IP address, ParseIP returns nil.

We could relax validation on the client side, and strip the square brackets, but not sure if that's a good idea, as we wouldn't be able to support the same over the API as the daemon-side likely would run into the same issue (which may make things more confusing)

/cc @akerouanton (in case you think we should accept this format here)

[thaJeztah]

One thing we should consider though is to use hostname=<ip-address> (using a = instead of : to separate name/value; we did similar changes for other options that used a colon before.

[coilysiren]

Ahah, thank you @thaJeztah. This form does in-fact work:

docker run --add-host "host.docker.internal:::1" nginx

But I strongly agree with your implication, that this form would make more sense:

docker run --add-host "host.docker.internal=::1" nginx

[thaJeztah]

Oh! And I just realized for some reason I completely glanced over your avatar and didn't recognize your handle; did you change it or do I just mis-remember? Either way: hope you're doing well! I don't think we met in person, but have interacted before (at least on urfave/cli).

But I strongly agree with your implication, that this form would make more sense:

Yes, I'm considering opening a pull-request for that.

Using a colon (:) as separator was commonly used in the "early days" of docker (-p 80:80, -v /foo/bar:/bar/baz, etc.), with the naive expectation that colons would be non-ambiguous. That obviously didn't age well (Windows paths, IPv6, ...); such things are taken more into account in newer changes, and we're trying to move away from them where possible (and where we can do so without breaking things). For this one, I think that would be possible (and we can consider "soft-deprecating", but continue supporting both variants).

Supporting the bracketed format ([::1]) may require more discussion. On one hand, I'd be in favor, as I think the format is (more) clear on intent, so supporting it would improve usability. That said, IPv6 still isn't deeply ingrained in my system, and not sure how common it is (or expected) for that format to be supported in non-URL situations. (At least stdlib doesn't expect it as outlined above).

[akerouanton]

I agree the current colon-based format is confusing and supporting square brackets would remove that confusion. We should definitely move away from that format.

For the record, RFC 5952: A Recommendation for IPv6 Address Text Representation, section 6, is the only case where the use of square brackets are recommended. That's why net.ParseIP doesn't support the bracketed format. But this recommendation exist for quite the same reason as what we face here: having an extra colon hanging around makes it harder (or, for the IP + port text represtation, even impossible in some cases) to understand what's the IP address.

We could relax validation on the client side, and strip the square brackets, but not sure if that's a good idea, as we wouldn't be able to support the same over the API as the daemon-side likely would run into the same issue (which may make things more confusing)

If we accept to do that, it's only due to the confusion created by the colon seperator we currently have on the CLI. So, I think this change should not slip into the deamon.

And to keep it easy to move from the old, colon-based format to a new, equal-based format, we should support bracketed IP address in both IMO.

I think that could be a good one for @robmry.

[thaJeztah]

I started looking at this one, and (fun, fun fun!) it looks like this is one of those examples where the UX became part of the API. The API takes the literal input from the CLI, and passes that as a []string in the API request.

Unfortunately, that means that we either need to

• unconditionally convert the host=ip-address format to host:ip-address before sending the request to the daemon
• or consider making that API-version dependent, but also may have to take the reverse into account (i.e., container created with new API version, but inspected with an old API version).

I also recall some discussions in the past on introducing custom DNS entries (the current --add-host implementation patches /etc/hosts), and potentially changing the implementation to always be DNS-based; if we decide to do that, we likely would be introducing a new API, so we could consider keeping the API as-is for now (API only supports host:ip-address using colon as separator), and improve the new API (if that would be implemented).

One thing I DID notice is that ValidateIPAddress (which is used as part of the validation of this option) also returns a normalized version of the IP-address (e.g. 0:0:0:0:0:0:0:1 gets normalized to ::1, and whitespace is removed; see #4657). However it looks like we currently don't use that normalized version, or at least: we validate if we can parse the input, after which we send the value as-is. We should consider sending the normalized version in the API request.

[robmry]

I'll have a bash at this ... handling it in the CLI to avoid the compatibility problems, and permitting bracketed addresses.

For now, to avoid surprising a user who's expecting to see the address they asked for, I won't use the canonical address returned by ValidateIPAddress() - but can easily change that if others disagree.

[thaJeztah]

The smallest / most narrowed change for now would probably be to accept both : and = as separator on the client side, and to normalize it to use the existing : separator for the request.

In the meantime we can already update the daemon side as well to accept both (for future)

[robmry]

The change on my CLI clone (linked above) deals with docker run --add-host. However, there's also docker build --add-host option, for setting up extra hosts in the build container. It'd be weird for it not to accept the same formats, so I've gone down that rabbit hole ...

For non-buildx, the option is handled by the same code as docker run. But, buildx does its own processing of the option in build/utils.go:toBuildkitExtraHosts(), so it needs an equivalent change.

[thaJeztah]

Arf, yes forgot about buildx. One more reason to have more of these in a proper sdk to allow easier re-use, but well get there eventually