Browse code

Fix network name masking network ID on delete

If a network is created with a name that matches another
network's ID, the network with that name was masking the
other network's ID.

As a result, it was not possible to remove the network
with a given ID.

This patch changes the order in which networks are
matched to be what we use for other cases;

1. Match on full ID
2. Match on full Name
3. Match on Partial ID

Before this patch:

$ docker network create foo
336717eac9eaa3da6557042a04efc803f7e8862ce6cf96f6b9565265ba5c618b

$ docker network create 336717eac9eaa3da6557042a04efc803f7e8862ce6cf96f6b9565265ba5c618b
4a698333f1197f20224583abce14876d7f25fdfe416a8545927006c315915a2a

$ docker network ls
NETWORK ID NAME DRIVER SCOPE
4a698333f119 336717eac9eaa3da6557042a04efc803f7e8862ce6cf96f6b9565265ba5c618b bridge local
d1e40d43a2c0 bridge bridge local
336717eac9ea foo bridge local
13cf280a1bbf host host local
d9e4c03728a0 none null local

$ docker network rm 336717eac9eaa3da6557042a04efc803f7e8862ce6cf96f6b9565265ba5c618b
4a698333f1197f20224583abce14876d7f25fdfe416a8545927006c315915a2a

$ docker network ls
NETWORK ID NAME DRIVER SCOPE
d1e40d43a2c0 bridge bridge local
336717eac9ea foo bridge local
13cf280a1bbf host host local
d9e4c03728a0 none null local

After this patch:

$ docker network create foo
2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835

$ docker network create 2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835
6cbc749a529cd2d9d3b10566c84e56c4203dd88b67417437b5fc7a6e955dd48f

$ docker network ls
NETWORK ID NAME DRIVER SCOPE
6cbc749a529c 2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835 bridge local
166c943dbeb5 bridge bridge local
2d1791a7def4 foo bridge local
6c45b8aa6d8e host host local
b11c96b51ea7 none null local

$ docker network rm 2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835
2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835

$ docker network ls
NETWORK ID NAME DRIVER SCOPE
6cbc749a529c 2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835 bridge local
166c943dbeb5 bridge bridge local
6c45b8aa6d8e host host local
b11c96b51ea7 none null local

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Sebastiaan van Stijn authored on 2017/08/15 05:23:57
Showing 4 changed files
... ...
@@ -31,18 +31,27 @@ func (daemon *Daemon) NetworkControllerEnabled() bool {
31 31
 
32 32
 // FindNetwork function finds a network for a given string that can represent network name or id
33 33
 func (daemon *Daemon) FindNetwork(idName string) (libnetwork.Network, error) {
34
-	// Find by Name
35
-	n, err := daemon.GetNetworkByName(idName)
36
-	if err != nil && !isNoSuchNetworkError(err) {
37
-		return nil, err
34
+	// 1. match by full ID.
35
+	n, err := daemon.GetNetworkByID(idName)
36
+	if err == nil || !isNoSuchNetworkError(err) {
37
+		return n, err
38 38
 	}
39 39
 
40
-	if n != nil {
41
-		return n, nil
40
+	// 2. match by full name
41
+	n, err = daemon.GetNetworkByName(idName)
42
+	if err == nil || !isNoSuchNetworkError(err) {
43
+		return n, err
42 44
 	}
43 45
 
44
-	// Find by id
45
-	return daemon.GetNetworkByID(idName)
46
+	// 3. match by ID prefix
47
+	list := daemon.GetNetworksByIDPrefix(idName)
48
+	if len(list) == 0 {
49
+		return nil, errors.WithStack(networkNotFound(idName))
50
+	}
51
+	if len(list) > 1 {
52
+		return nil, errors.WithStack(invalidIdentifier(idName))
53
+	}
54
+	return list[0], nil
46 55
 }
47 56
 
48 57
 func isNoSuchNetworkError(err error) bool {
... ...
@@ -50,18 +59,14 @@ func isNoSuchNetworkError(err error) bool {
50 50
 	return ok
51 51
 }
52 52
 
53
-// GetNetworkByID function returns a network whose ID begins with the given prefix.
54
-// It fails with an error if no matching, or more than one matching, networks are found.
55
-func (daemon *Daemon) GetNetworkByID(partialID string) (libnetwork.Network, error) {
56
-	list := daemon.GetNetworksByID(partialID)
57
-
58
-	if len(list) == 0 {
59
-		return nil, errors.WithStack(networkNotFound(partialID))
60
-	}
61
-	if len(list) > 1 {
62
-		return nil, errors.WithStack(invalidIdentifier(partialID))
53
+// GetNetworkByID function returns a network whose ID matches the given ID.
54
+// It fails with an error if no matching network is found.
55
+func (daemon *Daemon) GetNetworkByID(id string) (libnetwork.Network, error) {
56
+	c := daemon.netController
57
+	if c == nil {
58
+		return nil, libnetwork.ErrNoSuchNetwork(id)
63 59
 	}
64
-	return list[0], nil
60
+	return c.NetworkByID(id)
65 61
 }
66 62
 
67 63
 // GetNetworkByName function returns a network for a given network name.
... ...
@@ -77,8 +82,8 @@ func (daemon *Daemon) GetNetworkByName(name string) (libnetwork.Network, error)
77 77
 	return c.NetworkByName(name)
78 78
 }
79 79
 
80
-// GetNetworksByID returns a list of networks whose ID partially matches zero or more networks
81
-func (daemon *Daemon) GetNetworksByID(partialID string) []libnetwork.Network {
80
+// GetNetworksByIDPrefix returns a list of networks whose ID partially matches zero or more networks
81
+func (daemon *Daemon) GetNetworksByIDPrefix(partialID string) []libnetwork.Network {
82 82
 	c := daemon.netController
83 83
 	if c == nil {
84 84
 		return nil
... ...
@@ -31,6 +31,11 @@ keywords: "API, Docker, rcli, REST, documentation"
31 31
 * `POST /containers/create` now accepts additional values for the
32 32
   `HostConfig.IpcMode` property. New values are `private`, `shareable`,
33 33
   and `none`.
34
+* `DELETE /networks/{id or name}` fixed issue where a `name` equal to another
35
+  network's name was able to mask that `id`. If both a network with the given
36
+  _name_ exists, and a network with the given _id_, the network with the given
37
+  _id_ is now deleted. This change is not versioned, and affects all API versions
38
+  if the daemon has this patch.
34 39
 
35 40
 ## v1.31 API changes
36 41
 
37 42
new file mode 100644
... ...
@@ -0,0 +1,72 @@
0
+package network
1
+
2
+import (
3
+	"context"
4
+	"testing"
5
+
6
+	"github.com/docker/docker/api/types"
7
+	"github.com/docker/docker/integration/util/request"
8
+	"github.com/stretchr/testify/assert"
9
+	"github.com/stretchr/testify/require"
10
+)
11
+
12
+func containsNetwork(nws []types.NetworkResource, nw types.NetworkCreateResponse) bool {
13
+	for _, n := range nws {
14
+		if n.ID == nw.ID {
15
+			return true
16
+		}
17
+	}
18
+	return false
19
+}
20
+
21
+// createAmbiguousNetworks creates three networks, of which the second network
22
+// uses a prefix of the first network's ID as name. The third network uses the
23
+// first network's ID as name.
24
+//
25
+// After successful creation, properties of all three networks is returned
26
+func createAmbiguousNetworks(t *testing.T) (types.NetworkCreateResponse, types.NetworkCreateResponse, types.NetworkCreateResponse) {
27
+	client := request.NewAPIClient(t)
28
+	ctx := context.Background()
29
+
30
+	testNet, err := client.NetworkCreate(ctx, "testNet", types.NetworkCreate{})
31
+	require.NoError(t, err)
32
+	idPrefixNet, err := client.NetworkCreate(ctx, testNet.ID[:12], types.NetworkCreate{})
33
+	require.NoError(t, err)
34
+	fullIDNet, err := client.NetworkCreate(ctx, testNet.ID, types.NetworkCreate{})
35
+	require.NoError(t, err)
36
+
37
+	nws, err := client.NetworkList(ctx, types.NetworkListOptions{})
38
+	require.NoError(t, err)
39
+
40
+	assert.Equal(t, true, containsNetwork(nws, testNet), "failed to create network testNet")
41
+	assert.Equal(t, true, containsNetwork(nws, idPrefixNet), "failed to create network idPrefixNet")
42
+	assert.Equal(t, true, containsNetwork(nws, fullIDNet), "failed to create network fullIDNet")
43
+	return testNet, idPrefixNet, fullIDNet
44
+}
45
+
46
+// TestDockerNetworkDeletePreferID tests that if a network with a name
47
+// equal to another network's ID exists, the Network with the given
48
+// ID is removed, and not the network with the given name.
49
+func TestDockerNetworkDeletePreferID(t *testing.T) {
50
+	defer setupTest(t)()
51
+	client := request.NewAPIClient(t)
52
+	ctx := context.Background()
53
+	testNet, idPrefixNet, fullIDNet := createAmbiguousNetworks(t)
54
+
55
+	// Delete the network using a prefix of the first network's ID as name.
56
+	// This should the network name with the id-prefix, not the original network.
57
+	err := client.NetworkRemove(ctx, testNet.ID[:12])
58
+	require.NoError(t, err)
59
+
60
+	// Delete the network using networkID. This should remove the original
61
+	// network, not the network with the name equal to the networkID
62
+	err = client.NetworkRemove(ctx, testNet.ID)
63
+	require.NoError(t, err)
64
+
65
+	// networks "testNet" and "idPrefixNet" should be removed, but "fullIDNet" should still exist
66
+	nws, err := client.NetworkList(ctx, types.NetworkListOptions{})
67
+	require.NoError(t, err)
68
+	assert.Equal(t, false, containsNetwork(nws, testNet), "Network testNet not removed")
69
+	assert.Equal(t, false, containsNetwork(nws, idPrefixNet), "Network idPrefixNet not removed")
70
+	assert.Equal(t, true, containsNetwork(nws, fullIDNet), "Network fullIDNet not found")
71
+}
0 72
new file mode 100644
... ...
@@ -0,0 +1,28 @@
0
+package network
1
+
2
+import (
3
+	"fmt"
4
+	"os"
5
+	"testing"
6
+
7
+	"github.com/docker/docker/internal/test/environment"
8
+)
9
+
10
+var testEnv *environment.Execution
11
+
12
+func TestMain(m *testing.M) {
13
+	var err error
14
+	testEnv, err = environment.New()
15
+	if err != nil {
16
+		fmt.Println(err)
17
+		os.Exit(1)
18
+	}
19
+
20
+	testEnv.Print()
21
+	os.Exit(m.Run())
22
+}
23
+
24
+func setupTest(t *testing.T) func() {
25
+	environment.ProtectAll(t, testEnv)
26
+	return func() { testEnv.Clean(t) }
27
+}