Merge pull request #34509 from thaJeztah/fix-network-delete

Fix network name masking network ID on delete

daemon/network.go

@@ -31,18 +31,27 @@ func (daemon *Daemon) NetworkControllerEnabled() bool {
 
 // FindNetwork function finds a network for a given string that can represent network name or id
 func (daemon *Daemon) FindNetwork(idName string) (libnetwork.Network, error) {
-	// Find by Name
-	n, err := daemon.GetNetworkByName(idName)
-	if err != nil && !isNoSuchNetworkError(err) {
-		return nil, err
+	// 1. match by full ID.
+	n, err := daemon.GetNetworkByID(idName)
+	if err == nil || !isNoSuchNetworkError(err) {
+		return n, err
 	}
 
-	if n != nil {
-		return n, nil
+	// 2. match by full name
+	n, err = daemon.GetNetworkByName(idName)
+	if err == nil || !isNoSuchNetworkError(err) {
+		return n, err
 	}
 
-	// Find by id
-	return daemon.GetNetworkByID(idName)
+	// 3. match by ID prefix
+	list := daemon.GetNetworksByIDPrefix(idName)
+	if len(list) == 0 {
+		return nil, errors.WithStack(networkNotFound(idName))
+	}
+	if len(list) > 1 {
+		return nil, errors.WithStack(invalidIdentifier(idName))
+	}
+	return list[0], nil
 }
 
 func isNoSuchNetworkError(err error) bool {
@@ -50,18 +59,14 @@ func isNoSuchNetworkError(err error) bool {
 	return ok
 }
 
-// GetNetworkByID function returns a network whose ID begins with the given prefix.
-// It fails with an error if no matching, or more than one matching, networks are found.
-func (daemon *Daemon) GetNetworkByID(partialID string) (libnetwork.Network, error) {
-	list := daemon.GetNetworksByID(partialID)
-
-	if len(list) == 0 {
-		return nil, errors.WithStack(networkNotFound(partialID))
+// GetNetworkByID function returns a network whose ID matches the given ID.
+// It fails with an error if no matching network is found.
+func (daemon *Daemon) GetNetworkByID(id string) (libnetwork.Network, error) {
+	c := daemon.netController
+	if c == nil {
+		return nil, libnetwork.ErrNoSuchNetwork(id)
 	}
-	if len(list) > 1 {
-		return nil, errors.WithStack(invalidIdentifier(partialID))
-	}
-	return list[0], nil
+	return c.NetworkByID(id)
 }
 
 // GetNetworkByName function returns a network for a given network name.
@@ -77,8 +82,8 @@ func (daemon *Daemon) GetNetworkByName(name string) (libnetwork.Network, error)
 	return c.NetworkByName(name)
 }
 
-// GetNetworksByID returns a list of networks whose ID partially matches zero or more networks
-func (daemon *Daemon) GetNetworksByID(partialID string) []libnetwork.Network {
+// GetNetworksByIDPrefix returns a list of networks whose ID partially matches zero or more networks
+func (daemon *Daemon) GetNetworksByIDPrefix(partialID string) []libnetwork.Network {
 	c := daemon.netController
 	if c == nil {
 		return nil

docs/api/version-history.md

@@ -31,6 +31,11 @@ keywords: "API, Docker, rcli, REST, documentation"
 * `POST /containers/create` now accepts additional values for the
   `HostConfig.IpcMode` property. New values are `private`, `shareable`,
   and `none`.
+* `DELETE /networks/{id or name}` fixed issue where a `name` equal to another
+  network's name was able to mask that `id`. If both a network with the given
+  _name_ exists, and a network with the given _id_, the network with the given
+  _id_ is now deleted. This change is not versioned, and affects all API versions
+  if the daemon has this patch.
 
 ## v1.31 API changes
 

integration/network/delete_test.go

@@ -0,0 +1,72 @@
+package network
+
+import (
+	"context"
+	"testing"
+
+	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/integration/util/request"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func containsNetwork(nws []types.NetworkResource, nw types.NetworkCreateResponse) bool {
+	for _, n := range nws {
+		if n.ID == nw.ID {
+			return true
+		}
+	}
+	return false
+}
+
+// createAmbiguousNetworks creates three networks, of which the second network
+// uses a prefix of the first network's ID as name. The third network uses the
+// first network's ID as name.
+//
+// After successful creation, properties of all three networks is returned
+func createAmbiguousNetworks(t *testing.T) (types.NetworkCreateResponse, types.NetworkCreateResponse, types.NetworkCreateResponse) {
+	client := request.NewAPIClient(t)
+	ctx := context.Background()
+
+	testNet, err := client.NetworkCreate(ctx, "testNet", types.NetworkCreate{})
+	require.NoError(t, err)
+	idPrefixNet, err := client.NetworkCreate(ctx, testNet.ID[:12], types.NetworkCreate{})
+	require.NoError(t, err)
+	fullIDNet, err := client.NetworkCreate(ctx, testNet.ID, types.NetworkCreate{})
+	require.NoError(t, err)
+
+	nws, err := client.NetworkList(ctx, types.NetworkListOptions{})
+	require.NoError(t, err)
+
+	assert.Equal(t, true, containsNetwork(nws, testNet), "failed to create network testNet")
+	assert.Equal(t, true, containsNetwork(nws, idPrefixNet), "failed to create network idPrefixNet")
+	assert.Equal(t, true, containsNetwork(nws, fullIDNet), "failed to create network fullIDNet")
+	return testNet, idPrefixNet, fullIDNet
+}
+
+// TestDockerNetworkDeletePreferID tests that if a network with a name
+// equal to another network's ID exists, the Network with the given
+// ID is removed, and not the network with the given name.
+func TestDockerNetworkDeletePreferID(t *testing.T) {
+	defer setupTest(t)()
+	client := request.NewAPIClient(t)
+	ctx := context.Background()
+	testNet, idPrefixNet, fullIDNet := createAmbiguousNetworks(t)
+
+	// Delete the network using a prefix of the first network's ID as name.
+	// This should the network name with the id-prefix, not the original network.
+	err := client.NetworkRemove(ctx, testNet.ID[:12])
+	require.NoError(t, err)
+
+	// Delete the network using networkID. This should remove the original
+	// network, not the network with the name equal to the networkID
+	err = client.NetworkRemove(ctx, testNet.ID)
+	require.NoError(t, err)
+
+	// networks "testNet" and "idPrefixNet" should be removed, but "fullIDNet" should still exist
+	nws, err := client.NetworkList(ctx, types.NetworkListOptions{})
+	require.NoError(t, err)
+	assert.Equal(t, false, containsNetwork(nws, testNet), "Network testNet not removed")
+	assert.Equal(t, false, containsNetwork(nws, idPrefixNet), "Network idPrefixNet not removed")
+	assert.Equal(t, true, containsNetwork(nws, fullIDNet), "Network fullIDNet not found")
+}

integration/network/main_test.go

@@ -0,0 +1,28 @@
+package network
+
+import (
+	"fmt"
+	"os"
+	"testing"
+
+	"github.com/docker/docker/internal/test/environment"
+)
+
+var testEnv *environment.Execution
+
+func TestMain(m *testing.M) {
+	var err error
+	testEnv, err = environment.New()
+	if err != nil {
+		fmt.Println(err)
+		os.Exit(1)
+	}
+
+	testEnv.Print()
+	os.Exit(m.Run())
+}
+
+func setupTest(t *testing.T) func() {
+	environment.ProtectAll(t, testEnv)
+	return func() { testEnv.Clean(t) }
+}