From bbcd6625324dac24aad3448e67f0b95cc1567455 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Tue, 4 Jul 2023 12:32:14 +0200 Subject: [PATCH] api: Allow ContainerCreate to take several EndpointsConfig for >= 1.44 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 --- .../router/container/container_routes.go | 12 +++++ api/swagger.yaml | 12 ++--- daemon/create.go | 12 +---- docs/api/version-history.md | 2 + integration-cli/docker_api_containers_test.go | 27 ----------- integration/container/create_test.go | 37 +++++++++++++++ integration/internal/container/ops.go | 11 +++++ integration/internal/network/network.go | 7 +++ integration/network/bridge_test.go | 45 +++++++++++++++++++ 9 files changed, 123 insertions(+), 42 deletions(-) create mode 100644 integration/network/bridge_test.go diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index fe9554c8ef..4dd88a1cea 100644 --- a/api/server/router/container/container_routes.go +++ b/api/server/router/container/container_routes.go @@ -8,6 +8,7 @@ import ( "net/http" "runtime" "strconv" + "strings" "github.com/containerd/containerd/log" "github.com/containerd/containerd/platforms" @@ -607,6 +608,17 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo } } + if versions.LessThan(version, "1.44") { + // Creating a container connected to several networks is not supported until v1.44. + if networkingConfig != nil && len(networkingConfig.EndpointsConfig) > 1 { + l := make([]string, 0, len(networkingConfig.EndpointsConfig)) + for k := range networkingConfig.EndpointsConfig { + l = append(l, k) + } + return errdefs.InvalidParameter(errors.Errorf("Container cannot be connected to network endpoints: %s", strings.Join(l, ", "))) + } + } + ccr, err := s.backend.ContainerCreate(ctx, types.ContainerCreateConfig{ Name: name, Config: config, diff --git a/api/swagger.yaml b/api/swagger.yaml index 27da02af2e..952c2a46f4 100644 --- a/api/swagger.yaml +++ b/api/swagger.yaml @@ -1363,16 +1363,16 @@ definitions: EndpointsConfig: description: | A mapping of network name to endpoint configuration for that network. + The endpoint configuration can be left empty to connect to that + network with no particular endpoint configuration. type: "object" additionalProperties: $ref: "#/definitions/EndpointSettings" example: # putting an example here, instead of using the example values from - # /definitions/EndpointSettings, because containers/create currently - # does not support attaching to multiple networks, so the example request - # would be confusing if it showed that multiple networks can be contained - # in the EndpointsConfig. - # TODO remove once we support multiple networks on container create (see https://github.com/moby/moby/blob/07e6b843594e061f82baa5fa23c2ff7d536c2a05/daemon/create.go#L323) + # /definitions/EndpointSettings, because EndpointSettings contains + # operational data returned when inspecting a container that we don't + # accept here. EndpointsConfig: isolated_nw: IPAMConfig: @@ -1387,6 +1387,7 @@ definitions: Aliases: - "server_x" - "server_y" + database_nw: {} NetworkSettings: description: "NetworkSettings exposes the network settings in the API" @@ -6439,6 +6440,7 @@ paths: Aliases: - "server_x" - "server_y" + database_nw: {} required: true responses: diff --git a/daemon/create.go b/daemon/create.go index 2b8f22c72c..81c9a481ca 100644 --- a/daemon/create.go +++ b/daemon/create.go @@ -321,19 +321,11 @@ func (daemon *Daemon) mergeAndVerifyConfig(config *containertypes.Config, img *i return nil } -// Checks if the client set configurations for more than one network while creating a container -// Also checks if the IPAMConfig is valid +// verifyNetworkingConfig validates if the nwConfig is valid. func verifyNetworkingConfig(nwConfig *networktypes.NetworkingConfig) error { - if nwConfig == nil || len(nwConfig.EndpointsConfig) == 0 { + if nwConfig == nil { return nil } - if len(nwConfig.EndpointsConfig) > 1 { - l := make([]string, 0, len(nwConfig.EndpointsConfig)) - for k := range nwConfig.EndpointsConfig { - l = append(l, k) - } - return fmt.Errorf("container cannot be connected to network endpoints: %s", strings.Join(l, ", ")) - } for k, v := range nwConfig.EndpointsConfig { if v == nil { diff --git a/docs/api/version-history.md b/docs/api/version-history.md index 23118d04cc..d2365eb53d 100644 --- a/docs/api/version-history.md +++ b/docs/api/version-history.md @@ -45,6 +45,8 @@ keywords: "API, Docker, rcli, REST, documentation" such, the `CheckDuplicate` field is now deprecated. Note that this change is _unversioned_ and applied to all API versions on daemon that support version 1.44. +* `POST /containers/create` now accepts multiple `EndpointSettings` in + `NetworkingConfig.EndpointSettings`. ## v1.43 API changes diff --git a/integration-cli/docker_api_containers_test.go b/integration-cli/docker_api_containers_test.go index bde7bd328d..c14cf4d8ff 100644 --- a/integration-cli/docker_api_containers_test.go +++ b/integration-cli/docker_api_containers_test.go @@ -556,33 +556,6 @@ func (s *DockerAPISuite) TestContainerAPICreateEmptyConfig(c *testing.T) { assert.ErrorContains(c, err, "no command specified") } -func (s *DockerAPISuite) TestContainerAPICreateMultipleNetworksConfig(c *testing.T) { - // Container creation must fail if client specified configurations for more than one network - config := container.Config{ - Image: "busybox", - } - - networkingConfig := network.NetworkingConfig{ - EndpointsConfig: map[string]*network.EndpointSettings{ - "net1": {}, - "net2": {}, - "net3": {}, - }, - } - - apiClient, err := client.NewClientWithOpts(client.FromEnv) - assert.NilError(c, err) - defer apiClient.Close() - - _, err = apiClient.ContainerCreate(testutil.GetContext(c), &config, &container.HostConfig{}, &networkingConfig, nil, "") - msg := err.Error() - // network name order in error message is not deterministic - assert.Assert(c, strings.Contains(msg, "container cannot be connected to network endpoints")) - assert.Assert(c, strings.Contains(msg, "net1")) - assert.Assert(c, strings.Contains(msg, "net2")) - assert.Assert(c, strings.Contains(msg, "net3")) -} - func (s *DockerAPISuite) TestContainerAPICreateBridgeNetworkMode(c *testing.T) { // Windows does not support bridge testRequires(c, DaemonIsLinux) diff --git a/integration/container/create_test.go b/integration/container/create_test.go index 912a50824f..38483a608e 100644 --- a/integration/container/create_test.go +++ b/integration/container/create_test.go @@ -13,6 +13,7 @@ import ( containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/network" "github.com/docker/docker/api/types/versions" + "github.com/docker/docker/client" "github.com/docker/docker/errdefs" ctr "github.com/docker/docker/integration/internal/container" "github.com/docker/docker/oci" @@ -585,3 +586,39 @@ func TestCreateInvalidHostConfig(t *testing.T) { }) } } + +func TestCreateWithMultipleEndpointSettings(t *testing.T) { + defer setupTest(t)() + ctx := context.Background() + + testcases := []struct { + apiVersion string + expectedErr string + }{ + {apiVersion: "1.44"}, + {apiVersion: "1.43", expectedErr: "Container cannot be connected to network endpoints"}, + } + for _, tc := range testcases { + t.Run("with API v"+tc.apiVersion, func(t *testing.T) { + apiClient, err := client.NewClientWithOpts(client.FromEnv, client.WithVersion(tc.apiVersion)) + assert.NilError(t, err) + + config := container.Config{ + Image: "busybox", + } + networkingConfig := network.NetworkingConfig{ + EndpointsConfig: map[string]*network.EndpointSettings{ + "net1": {}, + "net2": {}, + "net3": {}, + }, + } + _, err = apiClient.ContainerCreate(ctx, &config, &container.HostConfig{}, &networkingConfig, nil, "") + if tc.expectedErr == "" { + assert.NilError(t, err) + } else { + assert.ErrorContains(t, err, tc.expectedErr) + } + }) + } +} diff --git a/integration/internal/container/ops.go b/integration/internal/container/ops.go index 8b9ad9d22e..89f48768c0 100644 --- a/integration/internal/container/ops.go +++ b/integration/internal/container/ops.go @@ -146,6 +146,17 @@ func WithIPv6(networkName, ip string) func(*TestContainerConfig) { } } +func WithEndpointSettings(nw string, config *network.EndpointSettings) func(*TestContainerConfig) { + return func(c *TestContainerConfig) { + if c.NetworkingConfig.EndpointsConfig == nil { + c.NetworkingConfig.EndpointsConfig = map[string]*network.EndpointSettings{} + } + if _, ok := c.NetworkingConfig.EndpointsConfig[nw]; !ok { + c.NetworkingConfig.EndpointsConfig[nw] = config + } + } +} + // WithLogDriver sets the log driver to use for the container func WithLogDriver(driver string) func(*TestContainerConfig) { return func(c *TestContainerConfig) { diff --git a/integration/internal/network/network.go b/integration/internal/network/network.go index 5a682ce84d..3989a4f7d2 100644 --- a/integration/internal/network/network.go +++ b/integration/internal/network/network.go @@ -33,3 +33,10 @@ func CreateNoError(ctx context.Context, t *testing.T, client client.APIClient, n assert.NilError(t, err) return name } + +func RemoveNoError(ctx context.Context, t *testing.T, apiClient client.APIClient, name string) { + t.Helper() + + err := apiClient.NetworkRemove(ctx, name) + assert.NilError(t, err) +} diff --git a/integration/network/bridge_test.go b/integration/network/bridge_test.go new file mode 100644 index 0000000000..b09ff5766c --- /dev/null +++ b/integration/network/bridge_test.go @@ -0,0 +1,45 @@ +package network + +import ( + "context" + "strings" + "testing" + "time" + + networktypes "github.com/docker/docker/api/types/network" + "github.com/docker/docker/api/types/versions" + ctr "github.com/docker/docker/integration/internal/container" + "github.com/docker/docker/integration/internal/network" + "gotest.tools/v3/assert" + "gotest.tools/v3/skip" +) + +func TestCreateWithMultiNetworks(t *testing.T) { + skip.If(t, testEnv.DaemonInfo.OSType == "windows") + skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.44"), "requires API v1.44") + + ctx := setupTest(t) + apiClient := testEnv.APIClient() + + network.CreateNoError(ctx, t, apiClient, "testnet1") + defer network.RemoveNoError(ctx, t, apiClient, "testnet1") + + network.CreateNoError(ctx, t, apiClient, "testnet2") + defer network.RemoveNoError(ctx, t, apiClient, "testnet2") + + attachCtx, cancel := context.WithTimeout(ctx, 1*time.Second) + defer cancel() + res := ctr.RunAttach(attachCtx, t, apiClient, + ctr.WithCmd("ip", "-o", "-4", "addr", "show"), + ctr.WithNetworkMode("testnet1"), + ctr.WithEndpointSettings("testnet1", &networktypes.EndpointSettings{}), + ctr.WithEndpointSettings("testnet2", &networktypes.EndpointSettings{})) + + assert.Equal(t, res.ExitCode, 0) + assert.Equal(t, res.Stderr.String(), "") + + // Only interfaces with an IPv4 address are printed by iproute2 when flag -4 is specified. Here, we should have two + // interfaces for testnet1 and testnet2, plus lo. + ifacesWithAddress := strings.Count(res.Stdout.String(), "\n") + assert.Equal(t, ifacesWithAddress, 3) +}