Create default EndpointSettings if no --network provided#4493
Create default EndpointSettings if no --network provided#4493thaJeztah merged 1 commit intodocker:masterfrom
Conversation
akerouanton
force-pushed
the
create-default-endpoint-settings
branch
from
6c765b3 to
e7bf31e
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #4493 +/- ##
==========================================
- Coverage 59.66% 59.66% -0.01%
==========================================
Files 288 288
Lines 24785 24797 +12
==========================================
+ Hits 14789 14795 +6
- Misses 9111 9115 +4
- Partials 885 887 +2 |
|
|
||
| if len(copts.netMode.Value()) == 0 { | ||
| n := opts.NetworkAttachmentOpts{ | ||
| Target: "default", |
There was a problem hiding this comment.
I had to double-check if this was only a thing on Windows, but I think we use the magic default on both. It reminded me of;
And we should look at having a const for this (makes it easier to discover where its used), but that's a change to make in the daemon code, so not for this PR.
There was a problem hiding this comment.
| if len(ep.Links) > 0 { | ||
| return nil, errors.New("links are only supported for user-defined networks") | ||
| } |
There was a problem hiding this comment.
I dug up my original PR / commit that changed this code; 5bc0963
A couple of things there that I was curious about (but it's a long time); on this line, there's an error that treats container.links and ep.links as separate things; 5bc0963#diff-e546b85d27f9dbea4fd41e83a58e36bb540bcda3e304355d56c92a119cd5aa2aR721
if len(n.Links) > 0 && copts.links.Len() > 0 {
return errdefs.InvalidParameter(errors.New("conflicting options: cannot specify both --link and per-network links"))
}It's valid to use --link on the default ("bridge") network, but for that network (currently) only with the legacy link implementation (hard-wired links).
Do you know if the daemon-side code makes this distinction as well when using the network-config make / slice? (i.e. does it look "if its the default (bridge) network, then use legacy links, and otherwise the DNS-based links?) or would the daemon only look at container.links to create legacy links?
There was a problem hiding this comment.
note that the above validation is only performed on the first network in the list (which holds data of the "default" network)
There was a problem hiding this comment.
You're right:
copts.linkswill be put intoHostConfig.Links, which is the way to use legacy links;n.Linksis used for DNS-based links.
I removed the first commit (ie. the one removing this check) and instead I changed the condition under which copts.links is copied into n.Links (ie. to not do that on "default" network).
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 <[email protected]>
akerouanton
force-pushed
the
create-default-endpoint-settings
branch
from
e7bf31e to
f1048e1
Compare
3 participants
Depends on:
- What I did
Following flags are silently ignored when they're passed with no
--networkspecified (ie. when the default network is used):--network-alias--ip--ip6--link-local-ipThis 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, the container-wide MacAddress parameter will be deprecated and dismissed. Because of that, with docker/cli#4419, it's currently not possible to use the
--mac-addressflag with no default network specified.Morever,
docker network connect --link-local-ip ...works properly, so it should also work ondocker 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 make sure these invalid parameters lead to a proper error message on
docker container create/docker run.- How to verify it
- Description for the changelog
--link-local-ipis now supported bydocker container createanddocker runwhen no--networkparameter is specified- A picture of a cute animal (not mandatory but encouraged)