Merge pull request #46251 from akerouanton/libnet-forbid-duplicated-network-names

libnet: Make sure network names are unique

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,10 +53,9 @@ func TestNetworkCreate(t *testing.T) {
 	}
 
 	networkResponse, err := client.NetworkCreate(context.Background(), "mynetwork", types.NetworkCreate{
-		CheckDuplicate: true,
-		Driver:         "mydriver",
-		EnableIPv6:     true,
-		Internal:       true,
+		Driver:     "mydriver",
+		EnableIPv6: true,
+		Internal:   true,
 		Options: map[string]string{
 			"opt-key": "opt-value",
 		},

daemon/cluster/executor/container/container.go

@@ -639,13 +639,12 @@ func (c *containerConfig) networkCreateRequest(name string) (clustertypes.Networ
 
 	options := types.NetworkCreate{
 		// ID:     na.Network.ID,
-		Labels:         na.Network.Spec.Annotations.Labels,
-		Internal:       na.Network.Spec.Internal,
-		Attachable:     na.Network.Spec.Attachable,
-		Ingress:        convert.IsIngressNetwork(na.Network),
-		EnableIPv6:     na.Network.Spec.Ipv6Enabled,
-		CheckDuplicate: true,
-		Scope:          scope.Swarm,
+		Labels:     na.Network.Spec.Annotations.Labels,
+		Internal:   na.Network.Spec.Internal,
+		Attachable: na.Network.Spec.Attachable,
+		Ingress:    convert.IsIngressNetwork(na.Network),
+		EnableIPv6: na.Network.Spec.Ipv6Enabled,
+		Scope:      scope.Swarm,
 	}
 
 	if na.Network.Spec.GetNetwork() != "" {

daemon/cluster/executor/container/executor.go

@@ -219,9 +219,8 @@ func (e *executor) Configure(ctx context.Context, node *api.Node) error {
 		IPAM: &network.IPAM{
 			Driver: ingressNA.Network.IPAM.Driver.Name,
 		},
-		Options:        ingressNA.Network.DriverState.Options,
-		Ingress:        true,
-		CheckDuplicate: true,
+		Options: ingressNA.Network.DriverState.Options,
+		Ingress: 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
@@ -397,8 +379,7 @@ func (daemon *Daemon) createNetwork(cfg *config.Config, create types.NetworkCrea
 	daemon.LogNetworkEvent(n, events.ActionCreate)
 
 	return &types.NetworkCreateResponse{
-		ID:      n.ID(),
-		Warning: warning,
+		ID: n.ID(),
 	}, 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()
 	}