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

api: Allow ContainerCreate to take several EndpointsConfig for >= API 1.44 #45906

Merged
merged 3 commits into from
Sep 15, 2023

Conversation

akerouanton
Copy link
Member

@akerouanton akerouanton commented Jul 7, 2023

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:

$ docker network create testnet1
$ docker network create testnet2
$ docker run --rm --network=testnet1 --network=testnet2 busybox ip -o link show

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

@akerouanton akerouanton added area/api status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. area/networking impact/api area/ux labels Jul 7, 2023
@akerouanton akerouanton force-pushed the create-with-several-networks branch 3 times, most recently from 67dcd7e to 2a364df Compare July 8, 2023 15:07
@akerouanton akerouanton marked this pull request as ready for review July 8, 2023 15:07
Comment on lines +1373 to +1375
# /definitions/EndpointSettings, because EndpointSettings contains
# operational data returned when inspecting a container that we don't
# accept here.
Copy link
Member

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

moby/api/swagger.yaml

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"

Copy link
Member Author

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

moby/api/swagger.yaml

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.

Copy link
Member

@thaJeztah thaJeztah Jul 11, 2023

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"
Screenshot 2023-07-11 at 13 39 25

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"
Screenshot 2023-07-11 at 13 45 06

Copy link
Member Author

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:

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

Copy link
Member Author

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.

api/swagger.yaml Show resolved Hide resolved
daemon/create.go Outdated Show resolved Hide resolved
docs/api/version-history.md Outdated Show resolved Hide resolved
daemon/create.go Outdated Show resolved Hide resolved
integration-cli/docker_api_containers_test.go Show resolved Hide resolved
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>
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
@akerouanton
Copy link
Member Author

Last force-push was only fixing a bad ctx in an integration test, so I'm gonna merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api area/networking area/ux impact/api impact/changelog kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"docker run" does not connect to more than one network
5 participants