Add missing opts to --network advanced syntax #4419

akerouanton · 2023-07-13T12:19:01Z

- What I did

Related to:

api: Add a field MacAddress to EndpointSettings moby/moby#45905

The new advanced --network syntax introduced in #1767 is lacking support for link-local-ip and mac-address fields. This PR adds both.

- How to verify it

With a daemon built out of moby/moby#45905:

$ docker run --rm --network=name=testnet1,mac-address=02:32:1c:23:00:04 busybox ip -o link show

- Description for the changelog

The advanced syntax for --network now supports link-local-ip and mac-address fields.

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

codecov-commenter · 2023-07-13T12:22:18Z

Codecov Report

Merging #4419 (9e1b42e) into master (d4aca90) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4419      +/-   ##
==========================================
+ Coverage   59.66%   59.71%   +0.04%     
==========================================
  Files         288      288              
  Lines       24785    24805      +20     
==========================================
+ Hits        14789    14812      +23     
+ Misses       9111     9109       -2     
+ Partials      885      884       -1

thaJeztah · 2023-07-15T17:13:36Z

cli/command/container/opts.go

@@ -679,6 +679,14 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con
 		return nil, err
 	}

+	if hostConfig.NetworkMode.IsBridge() || hostConfig.NetworkMode.IsUserDefined() {


Before I forget to comment; we need to check if this does what we expect it to.

I recall I wanted to use some of these functions on the client-side, but the code was written "platform specific", so IsUserDefined() on a Linux client potentially wouldn't detect a "user-defined" network if the daemon is Windows (or vice-versa).

Instead of making this check work when CLI OS and daemon OS don't match, I'll just remove it. It should not be a problem since the daemon checks whether a MAC address is specified along with an incompatible network mode here:

if (hc.NetworkMode.IsContainer() || hc.NetworkMode.IsHost()) && c.MacAddress != "" { return ErrConflictContainerNetworkAndMac }

thaJeztah

OHMAN this one is complicated I tried to make changes what I (think) is needed while reviewing, and opened a PR;

review suggestions akerouanton/docker#3

However, it may need checking, and there's some other comments I left (including "migrating containers during restore"), which I didn't look into yet.

Following flags are silently ignored when they're passed with no `--network` specified (ie. when the default network is used): - `--network-alias` - `--ip` - `--ip6` - `--link-local-ip` This is not really an issue right now since the first 3 parameters are not allowed on the default bridge network. However, with [moby/moby#45905][1], the container-wide MacAddress parameter will be deprecated and dismissed. Because of that, with [docker#4419][2], it's currently not possible to use the `--mac-address` flag for the default network. Morever, `docker network connect --link-local-ip ...` works properly, so it should also work on `docker container create`. This also lay the ground for making the default bridge network just a "normal" network. [1]: moby/moby#45905 [2]: docker#4419 Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Following flags are silently ignored when they're passed with no `--network` specified (ie. when the default network is used): - `--network-alias` - `--ip` - `--ip6` - `--link-local-ip` This is not really an issue right now since the first 3 parameters are not allowed on the default bridge network. However, with [moby/moby#45905][1], the container-wide MacAddress parameter will be deprecated and dismissed. Because of that, with [docker#4419][2], it's currently not possible to use the `--mac-address` flag with no default network specified. Morever, `docker network connect --link-local-ip ...` works properly, so it should also work on `docker container create`. This also lay the ground for making the default bridge network just a "normal" network. [1]: moby/moby#45905 [2]: docker#4419 Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Following flags are silently ignored when they're passed with no `--network` specified (ie. when the default network is used): - `--network-alias` - `--ip` - `--ip6` - `--link-local-ip` This is not really an issue right now since the first 3 parameters are not allowed on the default bridge network. However, with [moby/moby#45905][1], the container-wide MacAddress parameter will be deprecated and dismissed. Because of that, with [docker#4419][2], it's currently not possible to use the `--mac-address` flag with no default network specified. Morever, `docker network connect --link-local-ip ...` works properly, so it should also work on `docker container create`. This also lay the ground for making the default bridge network just a "normal" network. Since the 3 parameters in the list above aren't ignored anymore, if users provide them, moby's ContainerStart endpoint will complain about those. To provide better UX, [moby/moby#46183][3] make sure these invalid parameters lead to a proper error message on `docker container create` / `docker run`. [1]: moby/moby#45905 [2]: docker#4419 [3]: moby/moby#46183 Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Signed-off-by: Albin Kerouanton <albinker@gmail.com>

The new advanced --network syntax introduced in docker#1767 is lacking support for `link-local-ip` and `mac-address` fields. This commit adds both. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

thaJeztah · 2023-09-11T09:45:07Z

cli/command/container/opts.go

+	if n.MacAddress != "" && copts.macAddress != "" {
+		return errdefs.InvalidParameter(errors.New("conflicting options: cannot specify both --mac-address and per-network MAC address"))
+	}


Haven't checked out the code locally yet, but I wonder now if these errors are always correct; this error is legit if I would do the following;

This is a conflict, because I'm setting the mac-address TWICE for the default (bridge) network;

docker run \ --mac-address=foo \ --network name=bridge,mac-address=bar \ myimage

This is NOT a conflict, because I'm setting the mac-address twice, but for different networks; for bridge, I set it to foo and for mynetwork, I set it to bar;

docker run \ --mac-address=foo \ --network bridge \ --network name=mynetwork,mac-address=bar \ myimage

This one is more tricky though, as this one could be seen in 2 different ways;

an implicit --network=bridge with a mac-address specified for the default (bridge) network

a conflict (because I specified the network to be mynetwork, NOT bridge, and specified the mac-address twice for that network);

docker run \ --mac-address=foo \ --network name=mynetwork,mac-address=bar \ myimage

(Taking mac-address as example, but the same applies to other options that also have a separate --flag)

I think the original idea was to make sure no potentially ambiguous notations could be used. I think it will avoid some mistakes, so it's probably better to stick with that UX.

As it's mostly unrelated to my PR, could you open an issue if you think it's worth revisiting?

thaJeztah

LGTM

This is a follow-up of docker#4419. That PR leveraged the fact that EndpointSettings.MacAddress is already available, although not used by the CreateNetwork endpoint. TestParseWithMacAddress was testing whether the container-wide MacAddress field is set, and we still need to test that to ensure backward compatibility. But we now also need to test whether the endpoint-specific MacAddress is set. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

akerouanton requested review from rumpl, thaJeztah and vvoland July 13, 2023 12:19

akerouanton force-pushed the missing-nw-advanced-options branch 2 times, most recently from 795787a to c19599b Compare July 15, 2023 11:17

thaJeztah reviewed Jul 15, 2023

View reviewed changes

thaJeztah mentioned this pull request Jul 29, 2023

review suggestions akerouanton/docker#3

Closed

thaJeztah requested changes Jul 29, 2023

View reviewed changes

akerouanton mentioned this pull request Aug 1, 2023

api: Add a field MacAddress to EndpointSettings moby/moby#45905

Merged

This was referenced Aug 10, 2023

Create default EndpointSettings if no --network provided #4493

Merged

daemon: Improve NetworkingConfig & EndpointSettings validation moby/moby#46183

Merged

akerouanton force-pushed the missing-nw-advanced-options branch from c19599b to efbfb17 Compare September 10, 2023 16:56

akerouanton added 2 commits September 10, 2023 18:57

Rename expectedCfg into expectedHostCfg

58f4260

Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Add missing opts to --network advanced syntax

9e1b42e

The new advanced --network syntax introduced in docker#1767 is lacking support for `link-local-ip` and `mac-address` fields. This commit adds both. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

akerouanton force-pushed the missing-nw-advanced-options branch from efbfb17 to 9e1b42e Compare September 10, 2023 16:57

akerouanton requested a review from thaJeztah September 10, 2023 16:57

thaJeztah reviewed Sep 11, 2023

View reviewed changes

thaJeztah added impact/changelog status/2-code-review area/networking labels Sep 13, 2023

thaJeztah added this to the 25.0.0 milestone Sep 13, 2023

thaJeztah approved these changes Sep 13, 2023

View reviewed changes

thaJeztah merged commit cd6467b into docker:master Sep 13, 2023
74 checks passed

akerouanton deleted the missing-nw-advanced-options branch September 13, 2023 22:34

akerouanton mentioned this pull request Nov 8, 2023

TestParseWithMacAddress: check endpoint-scoped MacAddress field #4646

Merged

akerouanton mentioned this pull request Mar 5, 2024

network: multiple nets with --network docker/docs#19566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing opts to --network advanced syntax #4419

Add missing opts to --network advanced syntax #4419

akerouanton commented Jul 13, 2023 •

edited

Loading

codecov-commenter commented Jul 13, 2023 •

edited

Loading

thaJeztah Jul 15, 2023

akerouanton Sep 10, 2023

thaJeztah left a comment

thaJeztah Sep 11, 2023

akerouanton Sep 11, 2023

thaJeztah left a comment

Add missing opts to --network advanced syntax #4419

Add missing opts to --network advanced syntax #4419

Conversation

akerouanton commented Jul 13, 2023 • edited Loading

codecov-commenter commented Jul 13, 2023 • edited Loading

Codecov Report

thaJeztah Jul 15, 2023

Choose a reason for hiding this comment

akerouanton Sep 10, 2023

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Sep 11, 2023

Choose a reason for hiding this comment

akerouanton Sep 11, 2023

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

akerouanton commented Jul 13, 2023 •

edited

Loading

codecov-commenter commented Jul 13, 2023 •

edited

Loading