-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
api: Allow ContainerCreate to take several EndpointsConfig for >= API 1.44 #45906
api: Allow ContainerCreate to take several EndpointsConfig for >= API 1.44 #45906
Conversation
67dcd7e
to
2a364df
Compare
# /definitions/EndpointSettings, because EndpointSettings contains | ||
# operational data returned when inspecting a container that we don't | ||
# accept here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what data we have in "#/definitions/EndpointSettings"
that doesn't apply here?
Funny thing is that I don't think this example is actually used, because the container-create endpoint defines a "full" example for creating containers (including NetworkSettings);
Lines 6409 to 6423 in 462d6ef
NetworkingConfig: | |
EndpointsConfig: | |
isolated_nw: | |
IPAMConfig: | |
IPv4Address: "172.20.30.33" | |
IPv6Address: "2001:db8:abcd::3033" | |
LinkLocalIPs: | |
- "169.254.34.68" | |
- "fe80::3468" | |
Links: | |
- "container_1" | |
- "container_2" | |
Aliases: | |
- "server_x" | |
- "server_y" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what data we have in "#/definitions/EndpointSettings" that doesn't apply here?
Assuming this example was really used for documenting docker create
and docker network connect
(which is not the case, as you pointed out):
Lines 2464 to 2521 in 462d6ef
# Operational data | |
NetworkID: | |
description: | | |
Unique ID of the network. | |
type: "string" | |
example: "08754567f1f40222263eab4102e1c733ae697e8e354aa9cd6e18d7402835292a" | |
EndpointID: | |
description: | | |
Unique ID for the service endpoint in a Sandbox. | |
type: "string" | |
example: "b88f5b905aabf2893f3cbc4ee42d1ea7980bbc0a92e2c8922b1e1795298afb0b" | |
Gateway: | |
description: | | |
Gateway address for this network. | |
type: "string" | |
example: "172.17.0.1" | |
IPAddress: | |
description: | | |
IPv4 address. | |
type: "string" | |
example: "172.17.0.4" | |
IPPrefixLen: | |
description: | | |
Mask length of the IPv4 address. | |
type: "integer" | |
example: 16 | |
IPv6Gateway: | |
description: | | |
IPv6 gateway address. | |
type: "string" | |
example: "2001:db8:2::100" | |
GlobalIPv6Address: | |
description: | | |
Global IPv6 address. | |
type: "string" | |
example: "2001:db8::5689" | |
GlobalIPv6PrefixLen: | |
description: | | |
Mask length of the global IPv6 address. | |
type: "integer" | |
format: "int64" | |
example: 64 | |
MacAddress: | |
description: | | |
MAC address for the endpoint on this network. | |
type: "string" | |
example: "02:42:ac:11:00:04" | |
DriverOpts: | |
description: | | |
DriverOpts is a mapping of driver options and values. These options | |
are passed directly to the driver and are driver specific. | |
type: "object" | |
x-nullable: true | |
additionalProperties: | |
type: "string" | |
example: | |
com.example.some-label: "some-value" | |
com.example.some-other-label: "some-other-value" |
I started looking at whether we could move these operational data from EndpointSettings
into a dedicated EndpointState
, but this is going to be high-effort/low-impact so I'd prefer to focus on more pressing issues first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there's still a lot of cleaning up to do.
I've been slowly chipping away some of that; my attempts there are mostly to use per field example
values. Reason for that is that the swagger docs present example responses as-is, which also means that they don't have to match the actual type returned ... at all (!).
That has lead to examples not matching reality on various occasions, e.g. the following just happily renders a completely bogus response;
swagger: "2.0"
info:
title: "Foobar API"
version: "1.0.0"
definitions:
MyType:
type: "object"
properties:
Hello:
type: "string"
example: "hello"
World:
type: "string"
example: "world"
paths:
/containers/json:
get:
responses:
200:
description: "no error"
schema:
$ref: "#/definitions/MyType"
examples:
application/json:
- This: "this"
Does: "does"
Not:
Have: "have"
To: "match"
The downside on the other hand, is that all fields in the definition need to have some sample value (doable), but due to how we use the same structs for different purposes, and some fields being "optional", the examples could sometimes contain fields set that should not be set (e.g. container create would not contain an ID, whereas container inspect would contain it (as it was generated by the daemon).
So we'd have to split definitions for those, or find some other way to reflect that.
Taking the same swagger as above, but removing the examples:
response, it would take the example values of the definition;
swagger: "2.0"
info:
title: "Foobar API"
version: "1.0.0"
definitions:
MyType:
type: "object"
properties:
Hello:
type: "string"
example: "hello"
World:
type: "string"
example: "world"
paths:
/containers/json:
get:
responses:
200:
description: "no error"
schema:
$ref: "#/definitions/MyType"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some fields being "optional", the examples could sometimes contain fields set that should not be set (e.g. container create would not contain an ID, whereas container inspect would contain it (as it was generated by the daemon).
EndpointSettings
is actually one of these. All these fields here should not be sent on ContainerCreate
:
moby/api/types/network/network.go
Lines 54 to 64 in b2bde4a
// Operational data | |
NetworkID string | |
EndpointID string | |
Gateway string | |
IPAddress string | |
IPPrefixLen int | |
IPv6Gateway string | |
GlobalIPv6Address string | |
GlobalIPv6PrefixLen int | |
MacAddress string | |
DriverOpts map[string]string |
As suggested in this comment #46059 (comment), I would really like to see this struct split in two: one struct for the user-provided settings, and the other tracking the endpoint state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we'd have to split definitions for those, or find some other way to reflect that.
I think that's out of scope for this PR, so I'll keep it as is if you don't mind.
2a364df
to
41901ff
Compare
41901ff
to
b956536
Compare
b956536
to
a149c18
Compare
9e68772
to
9a4d630
Compare
The API endpoint `/containers/create` accepts several EndpointsConfig since v1.22 but the daemon would error out in such case. This check is moved from the daemon to the api and is now applied only for API < 1.44, effectively allowing the daemon to create containers connected to several networks. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
9a4d630
to
ccab37f
Compare
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
ccab37f
to
7ec9f30
Compare
Last force-push was only fixing a bad |
Related to:
- What I did
Prior to API v1.44, multiple entries could be specified in EndpointSettings but the daemon was artificially disallowing it by returning an HTTP error in such case. This restriction is now lifted, effectively allowing users to pass multiple EndpointSettings to connect the container to multiple networks when it starts, without needing to submit extra
/networks/<nid>/connect
requests.- How to verify it
With:
- Description for the changelog
A container can now be created with multiple EndpointSettings specified, allowing to start a container connected to multiple networks without the need to submit extra
/networks/<nid>/connect
requests.- A picture of a cute animal (not mandatory but encouraged)