Merge pull request containerd#3508 from apostasie/chore-clean-network-walker

Fix raft of issues related to network listing

Author: AkihiroSuda

cmd/nerdctl/completion/completion.go

@@ -124,9 +124,13 @@ func NetworkNames(cmd *cobra.Command, exclude []string) ([]string, cobra.ShellCo
 	if err != nil {
 		return nil, cobra.ShellCompDirectiveError
 	}
-	for netName := range netConfigs {
+	for netName, network := range netConfigs {
 		if _, ok := excludeMap[netName]; !ok {
 			candidates = append(candidates, netName)
+			if network.NerdctlID != nil {
+				candidates = append(candidates, *network.NerdctlID)
+				candidates = append(candidates, (*network.NerdctlID)[0:12])
+			}
 		}
 	}
 	for _, s := range []string{"host", "none"} {

cmd/nerdctl/network/network_inspect_test.go

@@ -19,6 +19,7 @@ package network
 import (
 	"encoding/json"
 	"errors"
+	"strings"
 	"testing"
 
 	"gotest.tools/v3/assert"
@@ -38,6 +39,128 @@ func TestNetworkInspect(t *testing.T) {
 	)
 
 	testGroup := &test.Group{
+		{
+			Description: "non existent network",
+			Command:     test.RunCommand("network", "inspect", "nonexistent"),
+			// FIXME: where is this error even comin from?
+			Expected: test.Expects(1, []error{errors.New("no network found matching")}, nil),
+		},
+		{
+			Description: "invalid name network",
+			Command:     test.RunCommand("network", "inspect", "∞"),
+			// FIXME: this is not even a valid identifier
+			Expected: test.Expects(1, []error{errors.New("no network found matching")}, nil),
+		},
+		{
+			Description: "none",
+			Require:     nerdtest.NerdctlNeedsFixing,
+			Command:     test.RunCommand("network", "inspect", "none"),
+			Expected: test.Expects(0, nil, func(stdout string, info string, t *testing.T) {
+				var dc []dockercompat.Network
+				err := json.Unmarshal([]byte(stdout), &dc)
+				assert.NilError(t, err, "Unable to unmarshal output\n"+info)
+				assert.Equal(t, 1, len(dc), "Unexpectedly got multiple results\n"+info)
+				assert.Equal(t, dc[0].Name, "none")
+			}),
+		},
+		{
+			Description: "host",
+			Require:     nerdtest.NerdctlNeedsFixing,
+			Command:     test.RunCommand("network", "inspect", "host"),
+			Expected: test.Expects(0, nil, func(stdout string, info string, t *testing.T) {
+				var dc []dockercompat.Network
+				err := json.Unmarshal([]byte(stdout), &dc)
+				assert.NilError(t, err, "Unable to unmarshal output\n"+info)
+				assert.Equal(t, 1, len(dc), "Unexpectedly got multiple results\n"+info)
+				assert.Equal(t, dc[0].Name, "host")
+			}),
+		},
+		{
+			Description: "bridge",
+			Require:     test.Not(test.Windows),
+			Command:     test.RunCommand("network", "inspect", "bridge"),
+			Expected: test.Expects(0, nil, func(stdout string, info string, t *testing.T) {
+				var dc []dockercompat.Network
+				err := json.Unmarshal([]byte(stdout), &dc)
+				assert.NilError(t, err, "Unable to unmarshal output\n"+info)
+				assert.Equal(t, 1, len(dc), "Unexpectedly got multiple results\n"+info)
+				assert.Equal(t, dc[0].Name, "bridge")
+			}),
+		},
+		{
+			Description: "custom",
+			Setup: func(data test.Data, helpers test.Helpers) {
+				helpers.Ensure("network", "create", "custom")
+			},
+			Cleanup: func(data test.Data, helpers test.Helpers) {
+				helpers.Anyhow("network", "remove", "custom")
+			},
+			Command: test.RunCommand("network", "inspect", "custom"),
+			Expected: test.Expects(0, nil, func(stdout string, info string, t *testing.T) {
+				var dc []dockercompat.Network
+				err := json.Unmarshal([]byte(stdout), &dc)
+				assert.NilError(t, err, "Unable to unmarshal output\n"+info)
+				assert.Equal(t, 1, len(dc), "Unexpectedly got multiple results\n"+info)
+				assert.Equal(t, dc[0].Name, "custom")
+			}),
+		},
+		{
+			Description: "match exact id",
+			Require:     test.Not(test.Windows),
+			Command: func(data test.Data, helpers test.Helpers) test.Command {
+				id := strings.TrimSpace(helpers.Capture("network", "inspect", "bridge", "--format", "{{ .Id }}"))
+				return helpers.Command("network", "inspect", id)
+			},
+			Expected: test.Expects(0, nil, func(stdout string, info string, t *testing.T) {
+				var dc []dockercompat.Network
+				err := json.Unmarshal([]byte(stdout), &dc)
+				assert.NilError(t, err, "Unable to unmarshal output\n"+info)
+				assert.Equal(t, 1, len(dc), "Unexpectedly got multiple results\n"+info)
+				assert.Equal(t, dc[0].Name, "bridge")
+			}),
+		},
+		{
+			Description: "match part of id",
+			Require:     test.Not(test.Windows),
+			Command: func(data test.Data, helpers test.Helpers) test.Command {
+				id := strings.TrimSpace(helpers.Capture("network", "inspect", "bridge", "--format", "{{ .Id }}"))
+				return helpers.Command("network", "inspect", id[0:25])
+			},
+			Expected: test.Expects(0, nil, func(stdout string, info string, t *testing.T) {
+				var dc []dockercompat.Network
+				err := json.Unmarshal([]byte(stdout), &dc)
+				assert.NilError(t, err, "Unable to unmarshal output\n"+info)
+				assert.Equal(t, 1, len(dc), "Unexpectedly got multiple results\n"+info)
+				assert.Equal(t, dc[0].Name, "bridge")
+			}),
+		},
+		{
+			Description: "using another net short id",
+			Require:     test.Not(test.Windows),
+			Setup: func(data test.Data, helpers test.Helpers) {
+				id := strings.TrimSpace(helpers.Capture("network", "inspect", "bridge", "--format", "{{ .Id }}"))
+				helpers.Ensure("network", "create", id[0:12])
+				data.Set("netname", id[0:12])
+			},
+			Cleanup: func(data test.Data, helpers test.Helpers) {
+				id := strings.TrimSpace(helpers.Capture("network", "inspect", "bridge", "--format", "{{ .Id }}"))
+				helpers.Anyhow("network", "remove", id[0:12])
+			},
+			Command: func(data test.Data, helpers test.Helpers) test.Command {
+				return helpers.Command("network", "inspect", data.Get("netname"))
+			},
+			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
+				return &test.Expected{
+					Output: func(stdout string, info string, t *testing.T) {
+						var dc []dockercompat.Network
+						err := json.Unmarshal([]byte(stdout), &dc)
+						assert.NilError(t, err, "Unable to unmarshal output\n"+info)
+						assert.Equal(t, 1, len(dc), "Unexpectedly got multiple results\n"+info)
+						assert.Equal(t, dc[0].Name, data.Get("netname"))
+					},
+				}
+			},
+		},
 		{
 			Description: "Test network inspect",
 			// IPAMConfig is not implemented on Windows yet
@@ -88,16 +211,16 @@ func TestNetworkInspect(t *testing.T) {
 				return &test.Expected{
 					ExitCode: 0,
 					Output: func(stdout string, info string, t *testing.T) {
-						cmd := helpers.Command().Clear().WithBinary("nerdctl").WithArgs("--namespace", data.Identifier())
+						cmd := helpers.CustomCommand("nerdctl", "--namespace", data.Identifier())
 
 						cmd.Clone().WithArgs("network", "inspect", data.Identifier()).Run(&test.Expected{
 							ExitCode: 1,
-							Errors:   []error{errors.New("no such network")},
+							Errors:   []error{errors.New("no network found")},
 						})
 
 						cmd.Clone().WithArgs("network", "remove", data.Identifier()).Run(&test.Expected{
 							ExitCode: 1,
-							Errors:   []error{errors.New("no such network")},
+							Errors:   []error{errors.New("no network found")},
 						})
 
 						cmd.Clone().WithArgs("network", "ls").Run(&test.Expected{

pkg/cmd/container/kill.go

@@ -161,16 +161,12 @@ func cleanupNetwork(ctx context.Context, container containerd.Container, globalO
 			cniOpts := []gocni.Opt{
 				gocni.WithPluginDir([]string{globalOpts.CNIPath}),
 			}
-			netMap, err := e.NetworkMap()
-			if err != nil {
-				return err
-			}
+			var netw *netutil.NetworkConfig
 			for _, netstr := range networks {
-				net, ok := netMap[netstr]
-				if !ok {
-					return fmt.Errorf("no such network: %q", netstr)
+				if netw, err = e.NetworkByNameOrID(netstr); err != nil {
+					return err
 				}
-				cniOpts = append(cniOpts, gocni.WithConfListBytes(net.Bytes))
+				cniOpts = append(cniOpts, gocni.WithConfListBytes(netw.Bytes))
 			}
 			cni, err := gocni.New(cniOpts...)
 			if err != nil {

pkg/cmd/network/inspect.go

@@ -19,62 +19,71 @@ package network
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 
 	"github.com/containerd/log"
 
 	"github.com/containerd/nerdctl/v2/pkg/api/types"
 	"github.com/containerd/nerdctl/v2/pkg/formatter"
-	"github.com/containerd/nerdctl/v2/pkg/idutil/netwalker"
 	"github.com/containerd/nerdctl/v2/pkg/inspecttypes/dockercompat"
 	"github.com/containerd/nerdctl/v2/pkg/inspecttypes/native"
 	"github.com/containerd/nerdctl/v2/pkg/netutil"
 )
 
 func Inspect(ctx context.Context, options types.NetworkInspectOptions) error {
-	globalOptions := options.GOptions
-	e, err := netutil.NewCNIEnv(globalOptions.CNIPath, globalOptions.CNINetConfPath, netutil.WithNamespace(options.GOptions.Namespace))
+	if options.Mode != "native" && options.Mode != "dockercompat" {
+		return fmt.Errorf("unknown mode %q", options.Mode)
+	}
 
+	cniEnv, err := netutil.NewCNIEnv(options.GOptions.CNIPath, options.GOptions.CNINetConfPath, netutil.WithNamespace(options.GOptions.Namespace))
 	if err != nil {
 		return err
 	}
-	if options.Mode != "native" && options.Mode != "dockercompat" {
-		return fmt.Errorf("unknown mode %q", options.Mode)
-	}
 
 	var result []interface{}
-	walker := netwalker.NetworkWalker{
-		Client: e,
-		OnFound: func(ctx context.Context, found netwalker.Found) error {
-			if found.MatchCount > 1 {
-				return fmt.Errorf("multiple IDs found with provided prefix: %s", found.Req)
-			}
-			r := &native.Network{
-				CNI:           json.RawMessage(found.Network.Bytes),
-				NerdctlID:     found.Network.NerdctlID,
-				NerdctlLabels: found.Network.NerdctlLabels,
-				File:          found.Network.File,
-			}
-			switch options.Mode {
-			case "native":
-				result = append(result, r)
-			case "dockercompat":
-				compat, err := dockercompat.NetworkFromNative(r)
-				if err != nil {
-					return err
-				}
-				result = append(result, compat)
+	netLists, errs := cniEnv.ListNetworksMatch(options.Networks, true)
+
+	for req, netList := range netLists {
+		if len(netList) > 1 {
+			errs = append(errs, fmt.Errorf("multiple IDs found with provided prefix: %s", req))
+			continue
+		}
+		if len(netList) == 0 {
+			errs = append(errs, fmt.Errorf("no network found matching: %s", req))
+			continue
+		}
+		network := netList[0]
+		r := &native.Network{
+			CNI:           json.RawMessage(network.Bytes),
+			NerdctlID:     network.NerdctlID,
+			NerdctlLabels: network.NerdctlLabels,
+			File:          network.File,
+		}
+		switch options.Mode {
+		case "native":
+			result = append(result, r)
+		case "dockercompat":
+			compat, err := dockercompat.NetworkFromNative(r)
+			if err != nil {
+				return err
 			}
-			return nil
-		},
+			result = append(result, compat)
+		}
 	}
 
-	// `network inspect` doesn't support pseudo network.
-	err = walker.WalkAll(ctx, options.Networks, true, false)
 	if len(result) > 0 {
 		if formatErr := formatter.FormatSlice(options.Format, options.Stdout, result); formatErr != nil {
 			log.G(ctx).Error(formatErr)
 		}
+		err = nil
+	} else {
+		err = errors.New("unable to find any network matching the provided request")
+	}
+
+	for _, unErr := range errs {
+		log.G(ctx).Error(unErr)
 	}
+
 	return err
 }

pkg/cmd/network/remove.go

@@ -18,17 +18,18 @@ package network
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	containerd "github.com/containerd/containerd/v2/client"
+	"github.com/containerd/log"
 
 	"github.com/containerd/nerdctl/v2/pkg/api/types"
-	"github.com/containerd/nerdctl/v2/pkg/idutil/netwalker"
 	"github.com/containerd/nerdctl/v2/pkg/netutil"
 )
 
 func Remove(ctx context.Context, client *containerd.Client, options types.NetworkRemoveOptions) error {
-	e, err := netutil.NewCNIEnv(options.GOptions.CNIPath, options.GOptions.CNINetConfPath, netutil.WithNamespace(options.GOptions.Namespace))
+	cniEnv, err := netutil.NewCNIEnv(options.GOptions.CNIPath, options.GOptions.CNINetConfPath, netutil.WithNamespace(options.GOptions.Namespace))
 	if err != nil {
 		return err
 	}
@@ -38,28 +39,52 @@ func Remove(ctx context.Context, client *containerd.Client, options types.Networ
 		return err
 	}
 
-	walker := netwalker.NetworkWalker{
-		Client: e,
-		OnFound: func(ctx context.Context, found netwalker.Found) error {
-			if found.MatchCount > 1 {
-				return fmt.Errorf("multiple IDs found with provided prefix: %s", found.Req)
-			}
-			if value, ok := usedNetworkInfo[found.Network.Name]; ok {
-				return fmt.Errorf("network %q is in use by container %q", found.Req, value)
-			}
-			if found.Network.NerdctlID == nil {
-				return fmt.Errorf("%s is managed outside nerdctl and cannot be removed", found.Req)
-			}
-			if found.Network.File == "" {
-				return fmt.Errorf("%s is a pre-defined network and cannot be removed", found.Req)
-			}
-			if err := e.RemoveNetwork(found.Network); err != nil {
-				return err
-			}
-			fmt.Fprintln(options.Stdout, found.Req)
-			return nil
-		},
+	var result []string
+	netLists, errs := cniEnv.ListNetworksMatch(options.Networks, false)
+
+	for req, netList := range netLists {
+		if len(netList) > 1 {
+			errs = append(errs, fmt.Errorf("multiple IDs found with provided prefix: %s", req))
+			continue
+		}
+		if len(netList) == 0 {
+			errs = append(errs, fmt.Errorf("no network found matching: %s", req))
+			continue
+		}
+		network := netList[0]
+		if value, ok := usedNetworkInfo[network.Name]; ok {
+			errs = append(errs, fmt.Errorf("network %q is in use by container %q", req, value))
+			continue
+		}
+		if network.Name == "bridge" {
+			errs = append(errs, errors.New("cannot remove pre-defined network bridge"))
+			continue
+		}
+		if network.File == "" {
+			errs = append(errs, fmt.Errorf("%s is a pre-defined network and cannot be removed", req))
+			continue
+		}
+		if network.NerdctlID == nil {
+			errs = append(errs, fmt.Errorf("%s is managed outside nerdctl and cannot be removed", req))
+			continue
+		}
+		if err := cniEnv.RemoveNetwork(network); err != nil {
+			errs = append(errs, err)
+		} else {
+			result = append(result, req)
+		}
+	}
+	for _, unErr := range errs {
+		log.G(ctx).Error(unErr)
+	}
+	if len(result) > 0 {
+		for _, id := range result {
+			fmt.Fprintln(options.Stdout, id)
+		}
+		err = nil
+	} else {
+		err = errors.New("no network could be removed")
 	}
 
-	return walker.WalkAll(ctx, options.Networks, true, false)
+	return err
 }