Skip to content

Commit 78479b1

Browse files
akerouantonakerouanton
committed
libnet: Make sure network names are unique
Fixes #18864, #20648, #33561, #40901. [This GH comment][1] makes clear network name uniqueness has never been enforced due to the eventually consistent nature of Classic Swarm datastores: > there is no guaranteed way to check for duplicates across a cluster of > docker hosts. And this is further confirmed by other comments made by @mrjana in that same issue, eg. [this one][2]: > we want to adopt a schema which can pave the way in the future for a > completely decentralized cluster of docker hosts (if scalability is > needed). This decentralized model is what Classic Swarm was trying to be. It's been superseded since then by Docker Swarm, which has a centralized control plane. To circumvent this drawback, the `NetworkCreate` endpoint accepts a `CheckDuplicate` flag. However it's not perfectly reliable as it won't catch concurrent requests. Due to this design decision, API clients like Compose have to implement workarounds to make sure names are really unique (eg. docker/compose#9585). And the daemon itself has seen a string of issues due to that decision, including some that aren't fixed to this day (for instance #40901): > The problem is, that if you specify a network for a container using > the ID, it will add that network to the container but it will then > change it to reference the network by using the name. To summarize, this "feature" is broken, has no practical use and is a source of pain for Docker users and API consumers. So let's just remove it for _all_ API versions. [1]: #18864 (comment) [2]: #18864 (comment) Signed-off-by: Albin Kerouanton <albinker@gmail.com>
1 parent 4f28802 commit 78479b1

File tree

19 files changed

+58
-258
lines changed

19 files changed

+58
-258
lines changed

‎api/swagger.yaml‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9962,13 +9962,7 @@ paths:
99629962
type: "string"
99639963
CheckDuplicate:
99649964
description: |
9965-
Check for networks with duplicate names. Since Network is
9966-
primarily keyed based on a random ID and not on the name, and
9967-
network name is strictly a user-friendly alias to the network
9968-
which is uniquely identified using ID, there is no guaranteed
9969-
way to check for duplicates. CheckDuplicate is there to provide
9970-
a best effort checking of any networks which has the same name
9971-
but it is not guaranteed to catch all name collisions.
9965+
Deprecated: CheckDuplicate is now always enabled.
99729966
type: "boolean"
99739967
Driver:
99749968
description: "Name of the network driver plugin to use."

‎api/types/types.go‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -443,14 +443,9 @@ type EndpointResource struct {
443443

444444
// NetworkCreate is the expected body of the "create network" http request message
445445
type NetworkCreate struct {
446-
// Check for networks with duplicate names.
447-
// Network is primarily keyed based on a random ID and not on the name.
448-
// Network name is strictly a user-friendly alias to the network
449-
// which is uniquely identified using ID.
450-
// And there is no guaranteed way to check for duplicates.
451-
// Option CheckDuplicate is there to provide a best effort checking of any networks
452-
// which has the same name but it is not guaranteed to catch all name collisions.
453-
CheckDuplicate bool
446+
// Deprecated: CheckDuplicate is deprecated since API v1.44, but it defaults to true when sent by the client
447+
// package to older daemons.
448+
CheckDuplicate bool `json:",omitempty"`
454449
Driver string
455450
Scope string
456451
EnableIPv6 bool

‎client/network_create.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"encoding/json"
66

77
"github.com/docker/docker/api/types"
8+
"github.com/docker/docker/api/types/versions"
89
)
910

1011
// NetworkCreate creates a new network in the docker host.
@@ -13,6 +14,10 @@ func (cli *Client) NetworkCreate(ctx context.Context, name string, options types
1314
NetworkCreate: options,
1415
Name: name,
1516
}
17+
if versions.LessThan(cli.version, "1.44") {
18+
networkCreateRequest.CheckDuplicate = true //nolint:staticcheck // ignore SA1019: CheckDuplicate is deprecated since API v1.44.
19+
}
20+
1621
var response types.NetworkCreateResponse
1722
serverResp, err := cli.post(ctx, "/networks/create", nil, networkCreateRequest, nil)
1823
defer ensureReaderClosed(serverResp)

‎client/network_create_test.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,9 @@ func TestNetworkCreate(t *testing.T) {
5353
}
5454

5555
networkResponse, err := client.NetworkCreate(context.Background(), "mynetwork", types.NetworkCreate{
56-
CheckDuplicate: true,
57-
Driver: "mydriver",
58-
EnableIPv6: true,
59-
Internal: true,
56+
Driver: "mydriver",
57+
EnableIPv6: true,
58+
Internal: true,
6059
Options: map[string]string{
6160
"opt-key": "opt-value",
6261
},

‎daemon/cluster/executor/container/container.go‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -639,13 +639,12 @@ func (c *containerConfig) networkCreateRequest(name string) (clustertypes.Networ
639639

640640
options := types.NetworkCreate{
641641
// ID: na.Network.ID,
642-
Labels: na.Network.Spec.Annotations.Labels,
643-
Internal: na.Network.Spec.Internal,
644-
Attachable: na.Network.Spec.Attachable,
645-
Ingress: convert.IsIngressNetwork(na.Network),
646-
EnableIPv6: na.Network.Spec.Ipv6Enabled,
647-
CheckDuplicate: true,
648-
Scope: scope.Swarm,
642+
Labels: na.Network.Spec.Annotations.Labels,
643+
Internal: na.Network.Spec.Internal,
644+
Attachable: na.Network.Spec.Attachable,
645+
Ingress: convert.IsIngressNetwork(na.Network),
646+
EnableIPv6: na.Network.Spec.Ipv6Enabled,
647+
Scope: scope.Swarm,
649648
}
650649

651650
if na.Network.Spec.GetNetwork() != "" {

‎daemon/cluster/executor/container/executor.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -219,9 +219,8 @@ func (e *executor) Configure(ctx context.Context, node *api.Node) error {
219219
IPAM: &network.IPAM{
220220
Driver: ingressNA.Network.IPAM.Driver.Name,
221221
},
222-
Options: ingressNA.Network.DriverState.Options,
223-
Ingress: true,
224-
CheckDuplicate: true,
222+
Options: ingressNA.Network.DriverState.Options,
223+
Ingress: true,
225224
}
226225

227226
for _, ic := range ingressNA.Network.IPAM.Configs {

‎daemon/network.go‎

Lines changed: 1 addition & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -307,24 +307,6 @@ func (daemon *Daemon) createNetwork(cfg *config.Config, create types.NetworkCrea
307307
create.EnableIPv6 = true
308308
}
309309

310-
var warning string
311-
nw, err := daemon.GetNetworkByName(create.Name)
312-
if err != nil {
313-
if _, ok := err.(libnetwork.ErrNoSuchNetwork); !ok {
314-
return nil, err
315-
}
316-
}
317-
if nw != nil {
318-
// check if user defined CheckDuplicate, if set true, return err
319-
// otherwise prepare a warning message
320-
if create.CheckDuplicate {
321-
if !agent || nw.Dynamic() {
322-
return nil, libnetwork.NetworkNameError(create.Name)
323-
}
324-
}
325-
warning = fmt.Sprintf("Network with name %s (id : %s) already exists", nw.Name(), nw.ID())
326-
}
327-
328310
networkOptions := make(map[string]string)
329311
for k, v := range create.Options {
330312
networkOptions[k] = v
@@ -397,8 +379,7 @@ func (daemon *Daemon) createNetwork(cfg *config.Config, create types.NetworkCrea
397379
daemon.LogNetworkEvent(n, events.ActionCreate)
398380

399381
return &types.NetworkCreateResponse{
400-
ID: n.ID(),
401-
Warning: warning,
382+
ID: n.ID(),
402383
}, nil
403384
}
404385

‎docs/api/version-history.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,10 @@ keywords: "API, Docker, rcli, REST, documentation"
4141
* `POST /networks/create` now returns a 400 if the `IPAMConfig` has invalid
4242
values. Note that this change is _unversioned_ and applied to all API
4343
versions on daemon that support version 1.44.
44+
* `POST /networks/create` with a duplicated name now fails systematically. As
45+
such, the `CheckDuplicate` field is now deprecated. Note that this change is
46+
_unversioned_ and applied to all API versions on daemon that support version
47+
1.44.
4448

4549
## v1.43 API changes
4650

‎hack/make/test-docker-py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,8 @@ source hack/make/.integration-test-helpers
2424
# TODO re-enable test_connect_with_ipv6_address after we updated to a version of docker-py with https://github.com/docker/docker-py/pull/3169
2525
# TODO re-enable test_create_network_ipv6_enabled after we updated to a version of docker-py with https://github.com/docker/docker-py/pull/3169
2626
# TODO re-enable test_create_with_ipv6_address after we updated to a version of docker-py with https://github.com/docker/docker-py/pull/3169
27-
: "${PY_TEST_OPTIONS:=--junitxml=${DEST}/junit-report.xml --deselect=tests/integration/api_container_test.py::AttachContainerTest::test_attach_no_stream --deselect=tests/integration/api_container_test.py::AttachContainerTest::test_run_container_reading_socket_ws --deselect=tests/integration/api_container_test.py::CreateContainerTest::test_create_with_restart_policy --deselect=tests/integration/errors_test.py::ErrorsTest::test_api_error_parses_json --deselect=tests/integration/api_network_test.py::TestNetworks::test_connect_with_ipv6_address --deselect=tests/integration/api_network_test.py::TestNetworks::test_create_network_ipv6_enabled --deselect=tests/integration/api_network_test.py::TestNetworks::test_create_with_ipv6_address}"
27+
# TODO re-enable test_create_check_duplicate after we updated to a version of docker-py with https://github.com/docker/docker-py/pull/3170
28+
: "${PY_TEST_OPTIONS:=--junitxml=${DEST}/junit-report.xml --deselect=tests/integration/api_container_test.py::AttachContainerTest::test_attach_no_stream --deselect=tests/integration/api_container_test.py::AttachContainerTest::test_run_container_reading_socket_ws --deselect=tests/integration/api_container_test.py::CreateContainerTest::test_create_with_restart_policy --deselect=tests/integration/errors_test.py::ErrorsTest::test_api_error_parses_json --deselect=tests/integration/api_network_test.py::TestNetworks::test_connect_with_ipv6_address --deselect=tests/integration/api_network_test.py::TestNetworks::test_create_network_ipv6_enabled --deselect=tests/integration/api_network_test.py::TestNetworks::test_create_with_ipv6_address --deselect=tests/integration/api_network_test.py::TestNetworks::test_create_check_duplicate}"
2829
(
2930
bundle .integration-daemon-start
3031

‎integration-cli/docker_api_network_test.go‎

Lines changed: 1 addition & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -27,44 +27,6 @@ func (s *DockerAPISuite) TestAPINetworkGetDefaults(c *testing.T) {
2727
}
2828
}
2929

30-
func (s *DockerAPISuite) TestAPINetworkCreateCheckDuplicate(c *testing.T) {
31-
testRequires(c, DaemonIsLinux)
32-
name := "testcheckduplicate"
33-
configOnCheck := types.NetworkCreateRequest{
34-
Name: name,
35-
NetworkCreate: types.NetworkCreate{
36-
CheckDuplicate: true,
37-
},
38-
}
39-
configNotCheck := types.NetworkCreateRequest{
40-
Name: name,
41-
NetworkCreate: types.NetworkCreate{
42-
CheckDuplicate: false,
43-
},
44-
}
45-
46-
// Creating a new network first
47-
createNetwork(c, configOnCheck, http.StatusCreated)
48-
assert.Assert(c, isNetworkAvailable(c, name))
49-
50-
// Creating another network with same name and CheckDuplicate must fail
51-
isOlderAPI := versions.LessThan(testEnv.DaemonAPIVersion(), "1.34")
52-
expectedStatus := http.StatusConflict
53-
if isOlderAPI {
54-
// In the early test code it uses bool value to represent
55-
// whether createNetwork() is expected to fail or not.
56-
// Therefore, we use negation to handle the same logic after
57-
// the code was changed in https://github.com/moby/moby/pull/35030
58-
// -http.StatusCreated will also be checked as NOT equal to
59-
// http.StatusCreated in createNetwork() function.
60-
expectedStatus = -http.StatusCreated
61-
}
62-
createNetwork(c, configOnCheck, expectedStatus)
63-
64-
// Creating another network with same name and not CheckDuplicate must succeed
65-
createNetwork(c, configNotCheck, http.StatusCreated)
66-
}
67-
6830
func (s *DockerAPISuite) TestAPINetworkFilter(c *testing.T) {
6931
testRequires(c, DaemonIsLinux)
7032
nr := getNetworkResource(c, getNetworkIDByName(c, "bridge"))
@@ -248,12 +210,7 @@ func (s *DockerAPISuite) TestAPICreateDeletePredefinedNetworks(c *testing.T) {
248210

249211
func createDeletePredefinedNetwork(c *testing.T, name string) {
250212
// Create pre-defined network
251-
config := types.NetworkCreateRequest{
252-
Name: name,
253-
NetworkCreate: types.NetworkCreate{
254-
CheckDuplicate: true,
255-
},
256-
}
213+
config := types.NetworkCreateRequest{Name: name}
257214
expectedStatus := http.StatusForbidden
258215
if versions.LessThan(testEnv.DaemonAPIVersion(), "1.34") {
259216
// In the early test code it uses bool value to represent

0 commit comments

Comments
 (0)