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

Create default EndpointSettings if no --network provided #4493

Merged

Conversation

akerouanton
Copy link
Member

@akerouanton akerouanton commented Aug 10, 2023

Depends on:

- What I did

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, 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-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 make sure these invalid parameters lead to a proper error message on docker container create / docker run.

- How to verify it

# Before:
$ docker container create --link-local-ip 169.254.169.254 --name test busybox
$ docker inspect -f "{{ json .NetworkSettings.Networks.bridge.IPAMConfig }}" test
null

# After:
$ docker container create --link-local-ip 169.254.169.254 --name test busybox
$ docker inspect -f "{{ json .NetworkSettings.Networks.bridge.IPAMConfig }}" test
{"LinkLocalIPs":["169.254.169.254"]}

- Description for the changelog

  • --link-local-ip is now supported by docker container create and docker run when no --network parameter is specified

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

@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2023

Codecov Report

Merging #4493 (f1048e1) into master (d4aca90) will decrease coverage by 0.01%.
The diff coverage is 53.84%.

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     

@thaJeztah thaJeztah added this to the 25.0.0 milestone Aug 18, 2023
@@ -729,6 +729,20 @@ func parseNetworkOpts(copts *containerOptions) (map[string]*networktypes.Endpoin
hasUserDefined, hasNonUserDefined bool
)

if len(copts.netMode.Value()) == 0 {
n := opts.NetworkAttachmentOpts{
Target: "default",
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines 819 to 824
if len(ep.Links) > 0 {
return nil, errors.New("links are only supported for user-defined networks")
}
Copy link
Member

@thaJeztah thaJeztah Aug 18, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

note that the above validation is only performed on the first network in the list (which holds data of the "default" network)

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right:

  • copts.links will be put into HostConfig.Links, which is the way to use legacy links;
  • n.Links is 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]>
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 8671a5e into docker:master Sep 11, 2023
74 checks passed
@akerouanton akerouanton deleted the create-default-endpoint-settings branch September 11, 2023 09:30
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