Add option --ipv4 #5599

robmry · 2024-11-06T18:49:07Z

- What I did

Added network create option --ipv4 ... like --ipv6, but it defaults to true.

Updated network create docs to include --ipv4 - and --fixed-cidr-v6, --bip6 ... and tried to make it more obvious that the short options only affect docker0.

- How I did it

Mostly by copying code for --ipv6.

- How to verify it

# docker network create b4
0d4a8723ec72d2d4b661e60c095865fcd77819459ae0e318505fa1feadbae3de
# docker network create --ipv6 b46
b24ceec262cf8c01e0586ebcea3bd440b52fa0f9b1d40c93f8cec189b7674865
# docker network create --ipv6 --ipv4=false b6
cdfdd74fc7f3b5c421894b0c5b33d48ae8517e8edfcaca6fd40a448e801a189d

# docker network ls --format "table {{.ID}}\t{{.Name}}\t{{.Driver}}\t{{.Scope}}\t{{.IPv4}}\t{{.IPv6}}"
NETWORK ID     NAME      DRIVER    SCOPE     IPV4      IPV6
0d4a8723ec72   b4        bridge    local     true      false
cdfdd74fc7f3   b6        bridge    local     false     true
b24ceec262cf   b46       bridge    local     true      true
be4cf667c3a7   bridge    bridge    local     true      false
8f0b062d1967   host      host      local     false     false
08d4868e22d1   none      null      local     false     false

# docker run --rm -ti --network b6 alpine ip a show dev eth0
17: eth0@if18: <BROADCAST,MULTICAST,UP,LOWER_UP,M-DOWN> mtu 1500 qdisc noqueue state UP
    link/ether 86:18:48:8f:f3:3d brd ff:ff:ff:ff:ff:ff
    inet6 fd50:8667:92c::2/64 scope global flags 02
       valid_lft forever preferred_lft forever
    inet6 fe80::8418:48ff:fe8f:f33d/64 scope link tentative
       valid_lft forever preferred_lft forever

- Description for the changelog

- Added `docker network create` option `--ipv4`.
  To disable IPv4 address assignment for a network, use `docker network create --ipv4=false [...]`.

codecov-commenter · 2024-11-06T18:52:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.51%. Comparing base (e554bfe) to head (6a2cde6).
Report is 7 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5599      +/-   ##
==========================================
- Coverage   59.54%   59.51%   -0.03%     
==========================================
  Files         346      346              
  Lines       29376    29379       +3     
==========================================
- Hits        17491    17486       -5     
- Misses      10914    10923       +9     
+ Partials      971      970       -1

Signed-off-by: Rob Murray <[email protected]>

robmry · 2024-12-10T10:19:28Z

Some apparently-unrelated test failures ...

=== Failed
=== FAIL: e2e/container TestProcessTermination (11.00s)
    run_test.go:280: assertion failed:
        Command:  docker run --rm -i registry:5000/alpine:frozen sh -c echo 'starting trap'; trap 'echo got signal; exit 0;' TERM; while true; do sleep 10; done
        ExitCode: 0 (timeout)
        Stdout:   starting trap
        got signal

        Stderr:

        Failures:
        Expected command to finish, but it hit the timeout

#20 62.19 === FAIL: cli-plugins/socket TestConnectAndWait/connect_goroutine_exits_after_EOF (0.00s)
#20 62.19     socket_test.go:190: assertion failed: 7 (int) != 6 (numGoroutines + 1 int)

#20 61.97 === Failed
#20 61.97 === FAIL: cli/command/container TestRunAttachTermination (unknown)
#20 61.97 signal: interrupt

akerouanton · 2024-12-10T13:42:22Z

docs/reference/commandline/network_create.md

+| `com.docker.network.bridge.enable_icc`           | `--icc`                     | Enable or Disable Inter Container Connectivity        |
+| `com.docker.network.bridge.host_binding_ipv4`    | `--ip`                      | Default IP when binding container ports               |
+| `com.docker.network.driver.mtu`                  | `--mtu`                     | Set the containers network MTU                        |
+| `com.docker.network.container_iface_prefix`      | -                           | Set a custom prefix for container interfaces          |

 The following arguments can be passed to `docker network create` for any
 network driver, again with their approximate equivalents to Docker daemon
 flags used for the docker0 bridge:


Suggested change

flags used for the docker0 bridge:

flags used for the `docker0` bridge:

(unrelated to this PR)

akerouanton · 2024-12-10T13:43:51Z

docs/reference/commandline/network_create.md

-| `--gateway`  | -                                 | IPv4 or IPv6 Gateway for the master subnet |
-| `--ip-range` | `--fixed-cidr`, `--fixed-cidr-v6` | Allocate IPs from a range                  |
-| `--internal` | -                                 | Restrict external access to the network    |
-| `--ipv6`     | `--ipv6`                          | Enable or disable IPv6 networking          |


Did you remove this line by mistake, or on purpose? The daemon has an --ipv6 flag that controls whether the default bridge is IPv6-capable, so we should keep this line.

Oh, thank you ... by mistake while juggling commits (it came back in the next commit). Should be sorted now.

akerouanton · 2024-12-10T13:44:32Z

cli/command/network/create.go

@@ -73,7 +77,8 @@ func newCreateCommand(dockerCLI command.Cli) *cobra.Command {
 	flags.VarP(&options.driverOpts, "opt", "o", "Set driver specific options")
 	flags.Var(&options.labels, "label", "Set metadata on a network")
 	flags.BoolVar(&options.internal, "internal", false, "Restrict external access to the network")
-	flags.BoolVar(&ipv6, "ipv6", false, "Enable or disable IPv6 networking")
+	flags.BoolVar(&ipv4, "ipv4", true, "Enable or disable IPv4 address assignment")
+	flags.BoolVar(&ipv6, "ipv6", false, "Enable or disable IPv6 address assignment")


👍 for the revised help text.

Signed-off-by: Rob Murray <[email protected]>

akerouanton

LGTM

vvoland · 2024-12-12T14:54:31Z

cli/command/network/create.go

@@ -73,7 +77,8 @@ func newCreateCommand(dockerCLI command.Cli) *cobra.Command {
 	flags.VarP(&options.driverOpts, "opt", "o", "Set driver specific options")
 	flags.Var(&options.labels, "label", "Set metadata on a network")
 	flags.BoolVar(&options.internal, "internal", false, "Restrict external access to the network")
-	flags.BoolVar(&ipv6, "ipv6", false, "Enable or disable IPv6 networking")
+	flags.BoolVar(&ipv4, "ipv4", true, "Enable or disable IPv4 address assignment")


Suggested change

flags.BoolVar(&ipv4, "ipv4", true, "Enable or disable IPv4 address assignment")

flags.BoolVar(&ipv4, "ipv4", true, "Enable or disable IPv4 address assignment")

Wondering if we should force this to true on the CLI side.
The EnableIPv4 is *bool on the API side, so we could just keep it as missing/null if user haven't passed this option.
For example if in future we'd need to distinguish between "user explicitly doesn't want ipv4" or "user doesn't care" on the daemon side.

Does that make sense?

LOL, I just wrote a blurb below about "defaults".

so we could just keep it as missing/null if user haven't passed this option

Just above, lines 63-65 (copied from the IPv6 setting), the pointer in the API request is only set if the CLI flag is Changed ... so I think it's ok?

Oh right, I missed that!

thaJeztah · 2024-12-12T14:55:11Z

Related to this PR: does the compose-spec need an update to allow setting these? I'm not sure if it currently does, and we probably would need to keep parity between compose and docker CLI creating networks.

Not for this PR, because it's an existing situation: we need to start thinking of a strategy for defaults. We currently try to reflect defaults on the client side by showing true / false as default. Those defaults may be currently true, but could change over time (especially looking at IPv6, which may become a default at some point?). I don't have a clear-cut answer to this; we could fetch defaults from the daemon, or (much larger change) if we want things to be declarative, possibly have some fetch default -> patch -> send approach. Not a worry for now, but we may need to start having some of that in the back of our minds to start thinking about.

thaJeztah

changes in this PR LGTM

robmry · 2024-12-12T15:08:22Z

Related to this PR: does the compose-spec need an update to allow setting these? I'm not sure if it currently does, and we probably would need to keep parity between compose and docker CLI creating networks.

Yes, we should do ... it's work-around-able in Compose by setting the equivalent driver option, not pretty though. We need to add gw-priority anyway, so we can pick up the two together.

robmry added impact/changelog kind/feature area/networking labels Nov 6, 2024

robmry added this to the 28.0.0 milestone Nov 6, 2024

robmry self-assigned this Nov 6, 2024

robmry force-pushed the option_ipv4 branch from 2c4c90f to afbbecd Compare November 6, 2024 19:08

Docs: include --fixed-cidr-v6 and --bip6 docker0 options

a5353e5

Signed-off-by: Rob Murray <[email protected]>

robmry force-pushed the option_ipv4 branch from afbbecd to ebcaea3 Compare December 10, 2024 09:49

robmry marked this pull request as ready for review December 10, 2024 10:19

robmry requested review from thaJeztah and a team as code owners December 10, 2024 10:19

akerouanton reviewed Dec 10, 2024

View reviewed changes

robmry added 2 commits December 10, 2024 15:51

Docs: emphasise that some options that are for docker0

5c896c9

Signed-off-by: Rob Murray <[email protected]>

Add option '--ipv4'

6a2cde6

Signed-off-by: Rob Murray <[email protected]>

robmry force-pushed the option_ipv4 branch from ebcaea3 to 6a2cde6 Compare December 10, 2024 15:53

robmry mentioned this pull request Dec 11, 2024

Updates for moby 28.0 networking docker/docs#21612

Draft

3 tasks

akerouanton approved these changes Dec 12, 2024

View reviewed changes

vvoland reviewed Dec 12, 2024

View reviewed changes

thaJeztah approved these changes Dec 12, 2024

View reviewed changes

vvoland approved these changes Dec 12, 2024

View reviewed changes

robmry merged commit 2080e09 into docker:master Dec 12, 2024
89 checks passed

robmry mentioned this pull request Dec 17, 2024

[epic] iptables and port publishing improvements for moby 28.0 moby/moby#48815

Closed

11 tasks

robmry deleted the option_ipv4 branch January 21, 2025 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option --ipv4 #5599

Add option --ipv4 #5599

robmry commented Nov 6, 2024 •

edited

Loading

codecov-commenter commented Nov 6, 2024 •

edited

Loading

robmry commented Dec 10, 2024

akerouanton Dec 10, 2024

robmry Dec 10, 2024

akerouanton Dec 10, 2024

robmry Dec 10, 2024

akerouanton Dec 10, 2024

akerouanton left a comment

vvoland Dec 12, 2024

thaJeztah Dec 12, 2024

robmry Dec 12, 2024 •

edited

Loading

vvoland Dec 12, 2024

thaJeztah commented Dec 12, 2024

thaJeztah left a comment

robmry commented Dec 12, 2024

	flags used for the docker0 bridge:
	flags used for the `docker0` bridge:

	flags.BoolVar(&ipv4, "ipv4", true, "Enable or disable IPv4 address assignment")
	flags.BoolVar(&ipv4, "ipv4", true, "Enable or disable IPv4 address assignment")

Add option --ipv4 #5599

Add option --ipv4 #5599

Conversation

robmry commented Nov 6, 2024 • edited Loading

codecov-commenter commented Nov 6, 2024 • edited Loading

Codecov Report

robmry commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akerouanton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robmry Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Dec 12, 2024

thaJeztah left a comment

Choose a reason for hiding this comment

robmry commented Dec 12, 2024

robmry commented Nov 6, 2024 •

edited

Loading

codecov-commenter commented Nov 6, 2024 •

edited

Loading

robmry Dec 12, 2024 •

edited

Loading