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 moby/moby#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]: https://github.com/moby/moby/issues/18864#issuecomment-167201414
[2]: https://github.com/moby/moby/issues/18864#issuecomment-167202589

Signed-off-by: Albin Kerouanton <albinker@gmail.com>

api/swagger.yaml

@@ -9962,13 +9962,7 @@ paths:
                 type: "string"
               CheckDuplicate:
                 description: |
-                  Check for networks with duplicate names. Since Network is
-                  primarily keyed based on a random ID and not on the name, and
-                  network name is strictly a user-friendly alias to the network
-                  which is uniquely identified using ID, there is no guaranteed
-                  way to check for duplicates. CheckDuplicate is there to provide
-                  a best effort checking of any networks which has the same name
-                  but it is not guaranteed to catch all name collisions.
+                  Deprecated: CheckDuplicate is now always enabled.
                 type: "boolean"
               Driver:
                 description: "Name of the network driver plugin to use."

api/types/types.go

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

client/network_create.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 
 	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/api/types/versions"
 )
 
 // NetworkCreate creates a new network in the docker host.
@@ -13,6 +14,10 @@ func (cli *Client) NetworkCreate(ctx context.Context, name string, options types
 		NetworkCreate: options,
 		Name:          name,
 	}
+	if versions.LessThan(cli.version, "1.44") {
+		networkCreateRequest.CheckDuplicate = true //nolint:staticcheck // ignore SA1019: CheckDuplicate is deprecated since API v1.44.
+	}
+
 	var response types.NetworkCreateResponse
 	serverResp, err := cli.post(ctx, "/networks/create", nil, networkCreateRequest, nil)
 	defer ensureReaderClosed(serverResp)

client/network_create_test.go

@@ -53,7 +53,6 @@ func TestNetworkCreate(t *testing.T) {
 	}
 
 	networkResponse, err := client.NetworkCreate(context.Background(), "mynetwork", types.NetworkCreate{
-		CheckDuplicate: true,
 		Driver:     "mydriver",
 		EnableIPv6: true,
 		Internal:   true,

daemon/cluster/executor/container/container.go

@@ -644,7 +644,6 @@ func (c *containerConfig) networkCreateRequest(name string) (clustertypes.Networ
 		Attachable: na.Network.Spec.Attachable,
 		Ingress:    convert.IsIngressNetwork(na.Network),
 		EnableIPv6: na.Network.Spec.Ipv6Enabled,
-		CheckDuplicate: true,
 		Scope:      scope.Swarm,
 	}
 

daemon/cluster/executor/container/executor.go

@@ -221,7 +221,6 @@ func (e *executor) Configure(ctx context.Context, node *api.Node) error {
 		},
 		Options: ingressNA.Network.DriverState.Options,
 		Ingress: true,
-		CheckDuplicate: true,
 	}
 
 	for _, ic := range ingressNA.Network.IPAM.Configs {

daemon/network.go

@@ -307,24 +307,6 @@ func (daemon *Daemon) createNetwork(cfg *config.Config, create types.NetworkCrea
 		create.EnableIPv6 = true
 	}
 
-	var warning string
-	nw, err := daemon.GetNetworkByName(create.Name)
-	if err != nil {
-		if _, ok := err.(libnetwork.ErrNoSuchNetwork); !ok {
-			return nil, err
-		}
-	}
-	if nw != nil {
-		// check if user defined CheckDuplicate, if set true, return err
-		// otherwise prepare a warning message
-		if create.CheckDuplicate {
-			if !agent || nw.Dynamic() {
-				return nil, libnetwork.NetworkNameError(create.Name)
-			}
-		}
-		warning = fmt.Sprintf("Network with name %s (id : %s) already exists", nw.Name(), nw.ID())
-	}
-
 	networkOptions := make(map[string]string)
 	for k, v := range create.Options {
 		networkOptions[k] = v
@@ -398,7 +380,6 @@ func (daemon *Daemon) createNetwork(cfg *config.Config, create types.NetworkCrea
 
 	return &types.NetworkCreateResponse{
 		ID: n.ID(),
-		Warning: warning,
 	}, nil
 }
 

docs/api/version-history.md

@@ -41,6 +41,10 @@ keywords: "API, Docker, rcli, REST, documentation"
 * `POST /networks/create` now returns a 400 if the `IPAMConfig` has invalid
   values. Note that this change is _unversioned_ and applied to all API
   versions on daemon that support version 1.44.
+* `POST /networks/create` with a duplicated name now fails systematically. As
+  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.
 
 ## v1.43 API changes
 

hack/make/test-docker-py

@@ -24,7 +24,8 @@ source hack/make/.integration-test-helpers
 # 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
 # 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
 # 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
-: "${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}"
+# 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
+: "${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}"
 (
 	bundle .integration-daemon-start
 

integration-cli/docker_api_network_test.go

@@ -27,44 +27,6 @@ func (s *DockerAPISuite) TestAPINetworkGetDefaults(c *testing.T) {
 	}
 }
 
-func (s *DockerAPISuite) TestAPINetworkCreateCheckDuplicate(c *testing.T) {
-	testRequires(c, DaemonIsLinux)
-	name := "testcheckduplicate"
-	configOnCheck := types.NetworkCreateRequest{
-		Name: name,
-		NetworkCreate: types.NetworkCreate{
-			CheckDuplicate: true,
-		},
-	}
-	configNotCheck := types.NetworkCreateRequest{
-		Name: name,
-		NetworkCreate: types.NetworkCreate{
-			CheckDuplicate: false,
-		},
-	}
-
-	// Creating a new network first
-	createNetwork(c, configOnCheck, http.StatusCreated)
-	assert.Assert(c, isNetworkAvailable(c, name))
-
-	// Creating another network with same name and CheckDuplicate must fail
-	isOlderAPI := versions.LessThan(testEnv.DaemonAPIVersion(), "1.34")
-	expectedStatus := http.StatusConflict
-	if isOlderAPI {
-		// In the early test code it uses bool value to represent
-		// whether createNetwork() is expected to fail or not.
-		// Therefore, we use negation to handle the same logic after
-		// the code was changed in https://github.com/moby/moby/pull/35030
-		// -http.StatusCreated will also be checked as NOT equal to
-		// http.StatusCreated in createNetwork() function.
-		expectedStatus = -http.StatusCreated
-	}
-	createNetwork(c, configOnCheck, expectedStatus)
-
-	// Creating another network with same name and not CheckDuplicate must succeed
-	createNetwork(c, configNotCheck, http.StatusCreated)
-}
-
 func (s *DockerAPISuite) TestAPINetworkFilter(c *testing.T) {
 	testRequires(c, DaemonIsLinux)
 	nr := getNetworkResource(c, getNetworkIDByName(c, "bridge"))
@@ -248,12 +210,7 @@ func (s *DockerAPISuite) TestAPICreateDeletePredefinedNetworks(c *testing.T) {
 
 func createDeletePredefinedNetwork(c *testing.T, name string) {
 	// Create pre-defined network
-	config := types.NetworkCreateRequest{
-		Name: name,
-		NetworkCreate: types.NetworkCreate{
-			CheckDuplicate: true,
-		},
-	}
+	config := types.NetworkCreateRequest{Name: name}
 	expectedStatus := http.StatusForbidden
 	if versions.LessThan(testEnv.DaemonAPIVersion(), "1.34") {
 		// In the early test code it uses bool value to represent

integration-cli/docker_api_swarm_test.go

@@ -917,39 +917,6 @@ func (s *DockerSwarmSuite) TestAPISwarmErrorHandling(c *testing.T) {
 	assert.ErrorContains(c, err, "address already in use")
 }
 
-// Test case for 30242, where duplicate networks, with different drivers `bridge` and `overlay`,
-// caused both scopes to be `swarm` for `docker network inspect` and `docker network ls`.
-// This test makes sure the fixes correctly output scopes instead.
-func (s *DockerSwarmSuite) TestAPIDuplicateNetworks(c *testing.T) {
-	ctx := testutil.GetContext(c)
-	d := s.AddDaemon(ctx, c, true, true)
-	cli := d.NewClientT(c)
-	defer cli.Close()
-
-	name := "foo"
-	networkCreate := types.NetworkCreate{
-		CheckDuplicate: false,
-	}
-
-	networkCreate.Driver = "bridge"
-
-	n1, err := cli.NetworkCreate(testutil.GetContext(c), name, networkCreate)
-	assert.NilError(c, err)
-
-	networkCreate.Driver = "overlay"
-
-	n2, err := cli.NetworkCreate(testutil.GetContext(c), name, networkCreate)
-	assert.NilError(c, err)
-
-	r1, err := cli.NetworkInspect(testutil.GetContext(c), n1.ID, types.NetworkInspectOptions{})
-	assert.NilError(c, err)
-	assert.Equal(c, r1.Scope, "local")
-
-	r2, err := cli.NetworkInspect(testutil.GetContext(c), n2.ID, types.NetworkInspectOptions{})
-	assert.NilError(c, err)
-	assert.Equal(c, r2.Scope, "swarm")
-}
-
 // Test case for 30178
 func (s *DockerSwarmSuite) TestAPISwarmHealthcheckNone(c *testing.T) {
 	// Issue #36386 can be a independent one, which is worth further investigation.

integration-cli/docker_cli_swarm_test.go

@@ -18,7 +18,6 @@ import (
 	"time"
 
 	"github.com/cloudflare/cfssl/helpers"
-	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/swarm"
 	"github.com/docker/docker/integration-cli/checker"
 	"github.com/docker/docker/integration-cli/cli"
@@ -1656,71 +1655,6 @@ func (s *DockerSwarmSuite) TestSwarmReadonlyRootfs(c *testing.T) {
 	assert.Equal(c, strings.TrimSpace(out), "true")
 }
 
-func (s *DockerSwarmSuite) TestNetworkInspectWithDuplicateNames(c *testing.T) {
-	ctx := testutil.GetContext(c)
-	d := s.AddDaemon(ctx, c, true, true)
-
-	name := "foo"
-	options := types.NetworkCreate{
-		CheckDuplicate: false,
-		Driver:         "bridge",
-	}
-
-	cli := d.NewClientT(c)
-	defer cli.Close()
-
-	n1, err := cli.NetworkCreate(testutil.GetContext(c), name, options)
-	assert.NilError(c, err)
-
-	// Full ID always works
-	out, err := d.Cmd("network", "inspect", "--format", "{{.ID}}", n1.ID)
-	assert.NilError(c, err, out)
-	assert.Equal(c, strings.TrimSpace(out), n1.ID)
-
-	// Name works if it is unique
-	out, err = d.Cmd("network", "inspect", "--format", "{{.ID}}", name)
-	assert.NilError(c, err, out)
-	assert.Equal(c, strings.TrimSpace(out), n1.ID)
-
-	n2, err := cli.NetworkCreate(testutil.GetContext(c), name, options)
-	assert.NilError(c, err)
-	// Full ID always works
-	out, err = d.Cmd("network", "inspect", "--format", "{{.ID}}", n1.ID)
-	assert.NilError(c, err, out)
-	assert.Equal(c, strings.TrimSpace(out), n1.ID)
-
-	out, err = d.Cmd("network", "inspect", "--format", "{{.ID}}", n2.ID)
-	assert.NilError(c, err, out)
-	assert.Equal(c, strings.TrimSpace(out), n2.ID)
-
-	// Name with duplicates
-	out, err = d.Cmd("network", "inspect", "--format", "{{.ID}}", name)
-	assert.ErrorContains(c, err, "", out)
-	assert.Assert(c, strings.Contains(out, "2 matches found based on name"), out)
-	out, err = d.Cmd("network", "rm", n2.ID)
-	assert.NilError(c, err, out)
-
-	// Duplicates with name but with different driver
-	options.Driver = "overlay"
-
-	n2, err = cli.NetworkCreate(testutil.GetContext(c), name, options)
-	assert.NilError(c, err)
-
-	// Full ID always works
-	out, err = d.Cmd("network", "inspect", "--format", "{{.ID}}", n1.ID)
-	assert.NilError(c, err, out)
-	assert.Equal(c, strings.TrimSpace(out), n1.ID)
-
-	out, err = d.Cmd("network", "inspect", "--format", "{{.ID}}", n2.ID)
-	assert.NilError(c, err, out)
-	assert.Equal(c, strings.TrimSpace(out), n2.ID)
-
-	// Name with duplicates
-	out, err = d.Cmd("network", "inspect", "--format", "{{.ID}}", name)
-	assert.ErrorContains(c, err, "", out)
-	assert.Assert(c, strings.Contains(out, "2 matches found based on name"), out)
-}
-
 func (s *DockerSwarmSuite) TestSwarmStopSignal(c *testing.T) {
 	ctx := testutil.GetContext(c)
 	testRequires(c, DaemonIsLinux, UserNamespaceROMount)

integration/internal/network/ops.go

@@ -19,13 +19,6 @@ func WithIPv6() func(*types.NetworkCreate) {
 	}
 }
 
-// WithCheckDuplicate sets the CheckDuplicate field on create network request
-func WithCheckDuplicate() func(*types.NetworkCreate) {
-	return func(n *types.NetworkCreate) {
-		n.CheckDuplicate = true
-	}
-}
-
 // WithInternal enables Internal flag on the create network request
 func WithInternal() func(*types.NetworkCreate) {
 	return func(n *types.NetworkCreate) {

integration/network/delete_test.go

@@ -48,9 +48,7 @@ func TestNetworkCreateDelete(t *testing.T) {
 	client := testEnv.APIClient()
 
 	netName := "testnetwork_" + t.Name()
-	network.CreateNoError(ctx, t, client, netName,
-		network.WithCheckDuplicate(),
-	)
+	network.CreateNoError(ctx, t, client, netName)
 	assert.Check(t, IsNetworkAvailable(ctx, client, netName))
 
 	// delete the network and make sure it is deleted

integration/network/inspect_test.go

@@ -25,7 +25,6 @@ func TestInspectNetwork(t *testing.T) {
 	networkName := "Overlay" + t.Name()
 	overlayID := network.CreateNoError(ctx, t, c, networkName,
 		network.WithDriver("overlay"),
-		network.WithCheckDuplicate(),
 	)
 
 	var instances uint64 = 2

integration/network/network_test.go

@@ -258,3 +258,21 @@ func TestDefaultNetworkOpts(t *testing.T) {
 		})
 	}
 }
+
+func TestForbidDuplicateNetworkNames(t *testing.T) {
+	skip.If(t, testEnv.DaemonInfo.OSType == "windows")
+
+	ctx := testutil.StartSpan(baseContext, t)
+
+	d := daemon.New(t)
+	d.StartWithBusybox(ctx, t)
+	defer d.Stop(t)
+
+	c := d.NewClientT(t)
+	defer c.Close()
+
+	network.CreateNoError(ctx, t, c, "testnet")
+
+	_, err := c.NetworkCreate(ctx, "testnet", types.NetworkCreate{})
+	assert.Error(t, err, "Error response from daemon: network with name testnet already exists", "2nd NetworkCreate call should have failed")
+}

integration/network/service_test.go

@@ -427,7 +427,6 @@ func TestServiceWithDefaultAddressPoolInit(t *testing.T) {
 	name := "sthira" + t.Name()
 	overlayID := network.CreateNoError(ctx, t, cli, name,
 		network.WithDriver("overlay"),
-		network.WithCheckDuplicate(),
 	)
 
 	var instances uint64 = 1

integration/service/create_test.go

@@ -88,7 +88,6 @@ func TestCreateServiceMultipleTimes(t *testing.T) {
 
 	overlayName := "overlay1_" + t.Name()
 	overlayID := network.CreateNoError(ctx, t, client, overlayName,
-		network.WithCheckDuplicate(),
 		network.WithDriver("overlay"),
 	)
 
@@ -193,58 +192,6 @@ func TestCreateServiceMaxReplicas(t *testing.T) {
 	assert.NilError(t, err)
 }
 
-func TestCreateWithDuplicateNetworkNames(t *testing.T) {
-	skip.If(t, testEnv.DaemonInfo.OSType == "windows")
-	ctx := setupTest(t)
-	d := swarm.NewSwarm(ctx, t, testEnv)
-	defer d.Stop(t)
-	client := d.NewClientT(t)
-	defer client.Close()
-
-	name := "foo_" + t.Name()
-	n1 := network.CreateNoError(ctx, t, client, name, network.WithDriver("bridge"))
-	n2 := network.CreateNoError(ctx, t, client, name, network.WithDriver("bridge"))
-
-	// Duplicates with name but with different driver
-	n3 := network.CreateNoError(ctx, t, client, name, network.WithDriver("overlay"))
-
-	// Create Service with the same name
-	var instances uint64 = 1
-
-	serviceName := "top_" + t.Name()
-	serviceID := swarm.CreateService(ctx, t, d,
-		swarm.ServiceWithReplicas(instances),
-		swarm.ServiceWithName(serviceName),
-		swarm.ServiceWithNetwork(name),
-	)
-
-	poll.WaitOn(t, swarm.RunningTasksCount(ctx, client, serviceID, instances), swarm.ServicePoll)
-
-	resp, _, err := client.ServiceInspectWithRaw(ctx, serviceID, types.ServiceInspectOptions{})
-	assert.NilError(t, err)
-	assert.Check(t, is.Equal(n3, resp.Spec.TaskTemplate.Networks[0].Target))
-
-	// Remove Service, and wait for its tasks to be removed
-	err = client.ServiceRemove(ctx, serviceID)
-	assert.NilError(t, err)
-	poll.WaitOn(t, swarm.NoTasksForService(ctx, client, serviceID), swarm.ServicePoll)
-
-	// Remove networks
-	err = client.NetworkRemove(ctx, n3)
-	assert.NilError(t, err)
-
-	err = client.NetworkRemove(ctx, n2)
-	assert.NilError(t, err)
-
-	err = client.NetworkRemove(ctx, n1)
-	assert.NilError(t, err)
-
-	// Make sure networks have been destroyed.
-	poll.WaitOn(t, network.IsRemoved(ctx, client, n3), poll.WithTimeout(1*time.Minute), poll.WithDelay(10*time.Second))
-	poll.WaitOn(t, network.IsRemoved(ctx, client, n2), poll.WithTimeout(1*time.Minute), poll.WithDelay(10*time.Second))
-	poll.WaitOn(t, network.IsRemoved(ctx, client, n1), poll.WithTimeout(1*time.Minute), poll.WithDelay(10*time.Second))
-}
-
 func TestCreateServiceSecretFileMode(t *testing.T) {
 	skip.If(t, testEnv.DaemonInfo.OSType == "windows")
 	ctx := setupTest(t)

libnetwork/controller.go

@@ -475,6 +475,17 @@ func (c *Controller) NewNetwork(networkType, name string, id string, options ...
 		return nil, ErrInvalidName(name)
 	}
 
+	// Make sure two concurrent calls to this method won't create conflicting
+	// networks, otherwise libnetwork will end up in an invalid state.
+	if name != "" {
+		c.networkLocker.Lock(name)
+		defer c.networkLocker.Unlock(name)
+
+		if _, err := c.NetworkByName(name); err == nil {
+			return nil, NetworkNameError(name)
+		}
+	}
+
 	if id == "" {
 		id = stringid.GenerateRandomID()
 	}