diff --git a/api/swagger.yaml b/api/swagger.yaml index 960ab1f951..27da02af2e 100644 --- a/api/swagger.yaml +++ b/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." diff --git a/api/types/types.go b/api/types/types.go index a213f49192..84a85df786 100644 --- a/api/types/types.go +++ b/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 diff --git a/client/network_create.go b/client/network_create.go index 278d9383a8..d77c1c24aa 100644 --- a/client/network_create.go +++ b/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) diff --git a/client/network_create_test.go b/client/network_create_test.go index 064a59665b..10abc16e5d 100644 --- a/client/network_create_test.go +++ b/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", }, diff --git a/daemon/cluster/executor/container/container.go b/daemon/cluster/executor/container/container.go index 107b84b412..29d308906f 100644 --- a/daemon/cluster/executor/container/container.go +++ b/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() != "" { diff --git a/daemon/cluster/executor/container/executor.go b/daemon/cluster/executor/container/executor.go index e8325c3ef8..db0a1b6260 100644 --- a/daemon/cluster/executor/container/executor.go +++ b/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 { diff --git a/daemon/network.go b/daemon/network.go index 6e3917e0ff..c5dae5c20c 100644 --- a/daemon/network.go +++ b/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 } diff --git a/docs/api/version-history.md b/docs/api/version-history.md index 082f1c1d8a..23118d04cc 100644 --- a/docs/api/version-history.md +++ b/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 diff --git a/hack/make/test-docker-py b/hack/make/test-docker-py index 0c1f41254b..66b51ddd61 100644 --- a/hack/make/test-docker-py +++ b/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 diff --git a/integration-cli/docker_api_network_test.go b/integration-cli/docker_api_network_test.go index a485ba2aae..ae8da51851 100644 --- a/integration-cli/docker_api_network_test.go +++ b/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 diff --git a/integration-cli/docker_api_swarm_test.go b/integration-cli/docker_api_swarm_test.go index c7be7dd4da..7cae8441c2 100644 --- a/integration-cli/docker_api_swarm_test.go +++ b/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. diff --git a/integration-cli/docker_cli_swarm_test.go b/integration-cli/docker_cli_swarm_test.go index 856ef76c03..1daf4a52ee 100644 --- a/integration-cli/docker_cli_swarm_test.go +++ b/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) diff --git a/integration/internal/network/ops.go b/integration/internal/network/ops.go index 33bd3d05f3..dc0fce06ef 100644 --- a/integration/internal/network/ops.go +++ b/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) { diff --git a/integration/network/delete_test.go b/integration/network/delete_test.go index 0fcabb919b..b5dd58309f 100644 --- a/integration/network/delete_test.go +++ b/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 diff --git a/integration/network/inspect_test.go b/integration/network/inspect_test.go index 136eec508f..06eda3a89f 100644 --- a/integration/network/inspect_test.go +++ b/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 diff --git a/integration/network/network_test.go b/integration/network/network_test.go index 50ff58132e..4e6687d4df 100644 --- a/integration/network/network_test.go +++ b/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") +} diff --git a/integration/network/service_test.go b/integration/network/service_test.go index 5231ecf16e..54060ffae9 100644 --- a/integration/network/service_test.go +++ b/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 diff --git a/integration/service/create_test.go b/integration/service/create_test.go index 4d70361e3b..877dbb0284 100644 --- a/integration/service/create_test.go +++ b/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) diff --git a/libnetwork/controller.go b/libnetwork/controller.go index e4727b3f0f..80b349ee5d 100644 --- a/libnetwork/controller.go +++ b/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() }