Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing opts to --network advanced syntax #4419

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

akerouanton
Copy link
Member

@akerouanton akerouanton commented Jul 13, 2023

- What I did

Related to:

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
Copy link

codecov-commenter commented Jul 13, 2023

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     

@akerouanton akerouanton force-pushed the missing-nw-advanced-options branch 2 times, most recently from 795787a to c19599b Compare July 15, 2023 11:17
@@ -679,6 +679,14 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con
return nil, err
}

if hostConfig.NetworkMode.IsBridge() || hostConfig.NetworkMode.IsUserDefined() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
    }

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

akerouanton added a commit to akerouanton/cli that referenced this pull request Aug 10, 2023
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>
akerouanton added a commit to akerouanton/cli that referenced this pull request Aug 10, 2023
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>
akerouanton added a commit to akerouanton/cli that referenced this pull request Aug 10, 2023
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>
akerouanton added a commit to akerouanton/cli that referenced this pull request Aug 18, 2023
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>
akerouanton added a commit to akerouanton/cli that referenced this pull request Sep 10, 2023
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>
Comment on lines +796 to +798
if n.MacAddress != "" && copts.macAddress != "" {
return errdefs.InvalidParameter(errors.New("conflicting options: cannot specify both --mac-address and per-network MAC address"))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah thaJeztah merged commit cd6467b into docker:master Sep 13, 2023
74 checks passed
@akerouanton akerouanton deleted the missing-nw-advanced-options branch September 13, 2023 22:34
akerouanton added a commit to akerouanton/cli that referenced this pull request Nov 8, 2023
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 added a commit to akerouanton/cli that referenced this pull request Nov 8, 2023
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 added a commit to akerouanton/cli that referenced this pull request Nov 8, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants