docker / cli

The Docker CLI
Apache License 2.0
4.87k stars 1.91k forks source link

network create: make --ipv6 optional #5126

Closed akerouanton closed 3 months ago

akerouanton commented 3 months ago

- What I did

The API field EnableIPv6 was marked as optional in our Swagger docs, and its default value in the Go client came from that field being a bool, thus defaulting to its zero value. That's not the case anymore.

This field is now a *bool as to let daemon's config define the default value. IPv6 can still be enabled / disabled by explicitly specifying the --ipv6 flag when doing docker network create.

- How to verify it

Start a daemon from moby/moby master branch:

$ ./build/docker-darwin-arm64 network create testnet
$ docker network inspect --format='{{ (index .IPAM.Config) }}' testnet
[{172.20.0.0/16  172.20.0.1 map[]}]

$ ./build/docker-darwin-arm64 network create --ipv6 testnetv6
$ docker network inspect --format='{{ (index .IPAM.Config) }}' testnetv6
[{172.19.0.0/16  172.19.0.1 map[]} {fdbc:8ec0:bb64::/64  fdbc:8ec0:bb64::1/64 map[]}]

Then restart the daemon with --default-network-opt=bridge=com.docker.network.enable_ipv6=true:

$ ./build/docker-darwin-arm64 network create testnetv6
$ docker network inspect --format='{{ (index .IPAM.Config) }}' testnetv6
[{172.20.0.0/16  172.20.0.1 map[]} {fd8c:5815:be1a::/64  fd8c:5815:be1a::1/64 map[]}]

$ ./build/docker-darwin-arm64 network create --ipv6=false testnet
$ docker network inspect --format='{{ (index .IPAM.Config) }}' testnet
[{172.19.0.0/16  172.19.0.1 map[]}]

- A picture of a cute animal (not mandatory but encouraged)

codecov-commenter commented 3 months ago

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 61.34%. Comparing base (9b61bbb) to head (db2672e).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #5126 +/- ## ========================================== - Coverage 61.37% 61.34% -0.03% ========================================== Files 298 295 -3 Lines 20717 20714 -3 ========================================== - Hits 12715 12707 -8 - Misses 7102 7104 +2 - Partials 900 903 +3 ```
thaJeztah commented 3 months ago

The only thing I'm wondering is if we need to add more details about what the default is, but we can do in a follow-up.

akerouanton commented 3 months ago

@thaJeztah Sorry for the 1st version of this patch, it was crap. I should stop multi-tasking sometimes 😓

The only thing I'm wondering is if we need to add more details about what the default is, but we can do in a follow-up.

I think we need to update the documentation to explain how to set the default daemon-wide, how to disable ipv6 for a specific network, etc... but that's pretty much it. Do you have anything else in mind?

FWIW, we discussed docs updates a bit with @robmry and we decided to wait for at least the rc1 to start working on that -- we'd like to get every PR ready first.