Fix netwalker issues

Current implementation of netwalker (henceforth network inspect and remove) suffers from many
issues, rooting from mixing names and identifiers in the same namespace, faulty erroring logic,
and overly complicated design making debugging and reasonning harder.

This commit removes the walker and replaces it with a simpler approach.
Next commit will add tests.

Signed-off-by: apostasie <[email protected]>

Author: apostasie

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"} {

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
 }

pkg/containerutil/container_network_manager.go

@@ -667,17 +667,14 @@ func writeEtcHostnameForContainer(globalOptions types.GlobalCommandOptions, host
 // from the networkSlice is of a type within supportedTypes.
 // nolint:unused
 func verifyNetworkTypes(env *netutil.CNIEnv, networkSlice []string, supportedTypes []string) (map[string]*netutil.NetworkConfig, error) {
-	netMap, err := env.NetworkMap()
-	if err != nil {
-		return nil, err
-	}
-
 	res := make(map[string]*netutil.NetworkConfig, len(networkSlice))
+	var netConfig *netutil.NetworkConfig
+	var err error
 	for _, netstr := range networkSlice {
-		netConfig, ok := netMap[netstr]
-		if !ok {
-			return nil, fmt.Errorf("network %s not found", netstr)
+		if netConfig, err = env.NetworkByNameOrID(netstr); err != nil {
+			return nil, err
 		}
+
 		netType := netConfig.Plugins[0].Network.Type
 		if supportedTypes != nil && !strutil.InStringSlice(supportedTypes, netType) {
 			return nil, fmt.Errorf("network type %q is not supported for network mapping %q, must be one of: %v", netType, netstr, supportedTypes)