prevent unpredictable results with network create|remove

due to a lack of "locking" on cni operations, we could get ourselves in trouble when doing rapid creation or removal of networks.  added a simple file lock to deal with the collision and because it is not considered a performent path, use of the file lock should be ok.  if proven otherwise in the future, some generic shared memory lock should be implemented for libpod and also used here.

moved pkog/network to libpod/network because libpod is now being pulled into the package and it has therefore lost its generic nature. this will make it easier to absorb into libpod as we try to make the network closer to core operations.

Fixes: #7807

Signed-off-by: baude <bbaude@redhat.com>

cmd/podman/networks/create.go

@@ -7,7 +7,6 @@ import (
 	"github.com/containers/podman/v2/cmd/podman/registry"
 	"github.com/containers/podman/v2/libpod/define"
 	"github.com/containers/podman/v2/pkg/domain/entities"
-	"github.com/containers/podman/v2/pkg/network"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 )
@@ -56,9 +55,6 @@ func networkCreate(cmd *cobra.Command, args []string) error {
 	var (
 		name string
 	)
-	if err := network.IsSupportedDriver(networkCreateOptions.Driver); err != nil {
-		return err
-	}
 	if len(args) > 0 {
 		if !define.NameRegex.MatchString(args[0]) {
 			return define.RegexError

cmd/podman/networks/list.go

@@ -10,8 +10,8 @@ import (
 
 	"github.com/containers/podman/v2/cmd/podman/registry"
 	"github.com/containers/podman/v2/cmd/podman/validate"
+	"github.com/containers/podman/v2/libpod/network"
 	"github.com/containers/podman/v2/pkg/domain/entities"
-	"github.com/containers/podman/v2/pkg/network"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 )

libpod/network/config.go

@@ -3,6 +3,8 @@ package network
 import (
 	"encoding/json"
 	"net"
+
+	"github.com/containers/storage/pkg/lockfile"
 )
 
 // TODO once the containers.conf file stuff is worked out, this should be modified
@@ -17,8 +19,17 @@ const (
 	// DefaultPodmanDomainName is used for the dnsname plugin to define
 	// a localized domain name for a created network
 	DefaultPodmanDomainName = "dns.podman"
+	// LockFileName is used for obtaining a lock and is appended
+	// to libpod's tmpdir in practice
+	LockFileName = "cni.lock"
 )
 
+// CNILock is for preventing name collision and
+// unpredictable results when doing some CNI operations.
+type CNILock struct {
+	lockfile.Locker
+}
+
 // GetDefaultPodmanNetwork outputs the default network for podman
 func GetDefaultPodmanNetwork() (*net.IPNet, error) {
 	_, n, err := net.ParseCIDR("10.88.1.0/24")

libpod/network/create.go

@@ -0,0 +1,195 @@
+package network
+
+import (
+	"encoding/json"
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+
+	"github.com/containernetworking/cni/pkg/version"
+	"github.com/containers/podman/v2/libpod"
+	"github.com/containers/podman/v2/pkg/domain/entities"
+	"github.com/containers/podman/v2/pkg/util"
+	"github.com/pkg/errors"
+)
+
+func Create(name string, options entities.NetworkCreateOptions, r *libpod.Runtime) (*entities.NetworkCreateReport, error) {
+	var fileName string
+	if err := isSupportedDriver(options.Driver); err != nil {
+		return nil, err
+	}
+	config, err := r.GetConfig()
+	if err != nil {
+		return nil, err
+	}
+	// Acquire a lock for CNI
+	l, err := acquireCNILock(filepath.Join(config.Engine.TmpDir, LockFileName))
+	if err != nil {
+		return nil, err
+	}
+	defer l.releaseCNILock()
+	if len(options.MacVLAN) > 0 {
+		fileName, err = createMacVLAN(r, name, options)
+	} else {
+		fileName, err = createBridge(r, name, options)
+	}
+	if err != nil {
+		return nil, err
+	}
+	return &entities.NetworkCreateReport{Filename: fileName}, nil
+}
+
+// createBridge creates a CNI network
+func createBridge(r *libpod.Runtime, name string, options entities.NetworkCreateOptions) (string, error) {
+	isGateway := true
+	ipMasq := true
+	subnet := &options.Subnet
+	ipRange := options.Range
+	runtimeConfig, err := r.GetConfig()
+	if err != nil {
+		return "", err
+	}
+	// if range is provided, make sure it is "in" network
+	if subnet.IP != nil {
+		// if network is provided, does it conflict with existing CNI or live networks
+		err = ValidateUserNetworkIsAvailable(runtimeConfig, subnet)
+	} else {
+		// if no network is provided, figure out network
+		subnet, err = GetFreeNetwork(runtimeConfig)
+	}
+	if err != nil {
+		return "", err
+	}
+	gateway := options.Gateway
+	if gateway == nil {
+		// if no gateway is provided, provide it as first ip of network
+		gateway = CalcGatewayIP(subnet)
+	}
+	// if network is provided and if gateway is provided, make sure it is "in" network
+	if options.Subnet.IP != nil && options.Gateway != nil {
+		if !subnet.Contains(gateway) {
+			return "", errors.Errorf("gateway %s is not in valid for subnet %s", gateway.String(), subnet.String())
+		}
+	}
+	if options.Internal {
+		isGateway = false
+		ipMasq = false
+	}
+
+	// if a range is given, we need to ensure it is "in" the network range.
+	if options.Range.IP != nil {
+		if options.Subnet.IP == nil {
+			return "", errors.New("you must define a subnet range to define an ip-range")
+		}
+		firstIP, err := FirstIPInSubnet(&options.Range)
+		if err != nil {
+			return "", err
+		}
+		lastIP, err := LastIPInSubnet(&options.Range)
+		if err != nil {
+			return "", err
+		}
+		if !subnet.Contains(firstIP) || !subnet.Contains(lastIP) {
+			return "", errors.Errorf("the ip range %s does not fall within the subnet range %s", options.Range.String(), subnet.String())
+		}
+	}
+	bridgeDeviceName, err := GetFreeDeviceName(runtimeConfig)
+	if err != nil {
+		return "", err
+	}
+
+	if len(name) > 0 {
+		netNames, err := GetNetworkNamesFromFileSystem(runtimeConfig)
+		if err != nil {
+			return "", err
+		}
+		if util.StringInSlice(name, netNames) {
+			return "", errors.Errorf("the network name %s is already used", name)
+		}
+	} else {
+		// If no name is given, we give the name of the bridge device
+		name = bridgeDeviceName
+	}
+
+	ncList := NewNcList(name, version.Current())
+	var plugins []CNIPlugins
+	var routes []IPAMRoute
+
+	defaultRoute, err := NewIPAMDefaultRoute(IsIPv6(subnet.IP))
+	if err != nil {
+		return "", err
+	}
+	routes = append(routes, defaultRoute)
+	ipamConfig, err := NewIPAMHostLocalConf(subnet, routes, ipRange, gateway)
+	if err != nil {
+		return "", err
+	}
+
+	// TODO need to iron out the role of isDefaultGW and IPMasq
+	bridge := NewHostLocalBridge(bridgeDeviceName, isGateway, false, ipMasq, ipamConfig)
+	plugins = append(plugins, bridge)
+	plugins = append(plugins, NewPortMapPlugin())
+	plugins = append(plugins, NewFirewallPlugin())
+	// if we find the dnsname plugin, we add configuration for it
+	if HasDNSNamePlugin(runtimeConfig.Network.CNIPluginDirs) && !options.DisableDNS {
+		// Note: in the future we might like to allow for dynamic domain names
+		plugins = append(plugins, NewDNSNamePlugin(DefaultPodmanDomainName))
+	}
+	ncList["plugins"] = plugins
+	b, err := json.MarshalIndent(ncList, "", "   ")
+	if err != nil {
+		return "", err
+	}
+	if err := os.MkdirAll(GetCNIConfDir(runtimeConfig), 0755); err != nil {
+		return "", err
+	}
+	cniPathName := filepath.Join(GetCNIConfDir(runtimeConfig), fmt.Sprintf("%s.conflist", name))
+	err = ioutil.WriteFile(cniPathName, b, 0644)
+	return cniPathName, err
+}
+
+func createMacVLAN(r *libpod.Runtime, name string, options entities.NetworkCreateOptions) (string, error) {
+	var (
+		plugins []CNIPlugins
+	)
+	liveNetNames, err := GetLiveNetworkNames()
+	if err != nil {
+		return "", err
+	}
+
+	config, err := r.GetConfig()
+	if err != nil {
+		return "", err
+	}
+
+	// Make sure the host-device exists
+	if !util.StringInSlice(options.MacVLAN, liveNetNames) {
+		return "", errors.Errorf("failed to find network interface %q", options.MacVLAN)
+	}
+	if len(name) > 0 {
+		netNames, err := GetNetworkNamesFromFileSystem(config)
+		if err != nil {
+			return "", err
+		}
+		if util.StringInSlice(name, netNames) {
+			return "", errors.Errorf("the network name %s is already used", name)
+		}
+	} else {
+		name, err = GetFreeDeviceName(config)
+		if err != nil {
+			return "", err
+		}
+	}
+	ncList := NewNcList(name, version.Current())
+	macvlan := NewMacVLANPlugin(options.MacVLAN)
+	plugins = append(plugins, macvlan)
+	ncList["plugins"] = plugins
+	b, err := json.MarshalIndent(ncList, "", "   ")
+	if err != nil {
+		return "", err
+	}
+	cniPathName := filepath.Join(GetCNIConfDir(config), fmt.Sprintf("%s.conflist", name))
+	err = ioutil.WriteFile(cniPathName, b, 0644)
+	return cniPathName, err
+}

libpod/network/lock.go

@@ -0,0 +1,26 @@
+package network
+
+import (
+	"github.com/containers/storage"
+)
+
+// acquireCNILock gets a lock that should be used in create and
+// delete cases to avoid unwanted collisions in network names.
+// TODO this uses a file lock and should be converted to shared memory
+// when we have a more general shared memory lock in libpod
+func acquireCNILock(lockPath string) (*CNILock, error) {
+	l, err := storage.GetLockfile(lockPath)
+	if err != nil {
+		return nil, err
+	}
+	l.Lock()
+	cnilock := CNILock{
+		Locker: l,
+	}
+	return &cnilock, nil
+}
+
+// ReleaseCNILock unlocks the previously held lock
+func (l *CNILock) releaseCNILock() {
+	l.Unlock()
+}

libpod/network/network.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"net"
 	"os"
+	"path/filepath"
 
 	"github.com/containernetworking/cni/pkg/types"
 	"github.com/containernetworking/plugins/plugins/ipam/host-local/backend/allocator"
@@ -20,8 +21,8 @@ var DefaultNetworkDriver = "bridge"
 // SupportedNetworkDrivers describes the list of supported drivers
 var SupportedNetworkDrivers = []string{DefaultNetworkDriver}
 
-// IsSupportedDriver checks if the user provided driver is supported
-func IsSupportedDriver(driver string) error {
+// isSupportedDriver checks if the user provided driver is supported
+func isSupportedDriver(driver string) error {
 	if util.StringInSlice(driver, SupportedNetworkDrivers) {
 		return nil
 	}
@@ -168,6 +169,11 @@ func ValidateUserNetworkIsAvailable(config *config.Config, userNet *net.IPNet) e
 // RemoveNetwork removes a given network by name.  If the network has container associated with it, that
 // must be handled outside the context of this.
 func RemoveNetwork(config *config.Config, name string) error {
+	l, err := acquireCNILock(filepath.Join(config.Engine.TmpDir, LockFileName))
+	if err != nil {
+		return err
+	}
+	defer l.releaseCNILock()
 	cniPath, err := GetCNIConfigPathByName(config, name)
 	if err != nil {
 		return err

pkg/api/handlers/compat/networks.go

@@ -12,10 +12,10 @@ import (
 	"github.com/containernetworking/cni/libcni"
 	"github.com/containers/podman/v2/libpod"
 	"github.com/containers/podman/v2/libpod/define"
+	"github.com/containers/podman/v2/libpod/network"
 	"github.com/containers/podman/v2/pkg/api/handlers/utils"
 	"github.com/containers/podman/v2/pkg/domain/entities"
 	"github.com/containers/podman/v2/pkg/domain/infra/abi"
-	"github.com/containers/podman/v2/pkg/network"
 	"github.com/docker/docker/api/types"
 	dockerNetwork "github.com/docker/docker/api/types/network"
 	"github.com/gorilla/schema"
@@ -210,6 +210,7 @@ func ListNetworks(w http.ResponseWriter, r *http.Request) {
 		report, err := getNetworkResourceByName(name, runtime)
 		if err != nil {
 			utils.InternalServerError(w, err)
+			return
 		}
 		reports = append(reports, report)
 	}
@@ -267,9 +268,9 @@ func CreateNetwork(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 	ce := abi.ContainerEngine{Libpod: runtime}
-	_, err := ce.NetworkCreate(r.Context(), name, ncOptions)
-	if err != nil {
+	if _, err := ce.NetworkCreate(r.Context(), name, ncOptions); err != nil {
 		utils.InternalServerError(w, err)
+		return
 	}
 	report := types.NetworkCreate{
 		CheckDuplicate: networkCreate.CheckDuplicate,
@@ -307,6 +308,7 @@ func RemoveNetwork(w http.ResponseWriter, r *http.Request) {
 	}
 	if err := network.RemoveNetwork(config, name); err != nil {
 		utils.InternalServerError(w, err)
+		return
 	}
 	utils.WriteResponse(w, http.StatusNoContent, "")
 }

pkg/domain/infra/abi/network.go

@@ -2,19 +2,13 @@ package abi
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
-	"io/ioutil"
-	"os"
-	"path/filepath"
 	"strings"
 
 	"github.com/containernetworking/cni/libcni"
-	cniversion "github.com/containernetworking/cni/pkg/version"
-	"github.com/containers/podman/v2/libpod"
 	"github.com/containers/podman/v2/libpod/define"
+	"github.com/containers/podman/v2/libpod/network"
 	"github.com/containers/podman/v2/pkg/domain/entities"
-	"github.com/containers/podman/v2/pkg/network"
 	"github.com/containers/podman/v2/pkg/util"
 	"github.com/pkg/errors"
 )
@@ -111,173 +105,7 @@ func (ic *ContainerEngine) NetworkRm(ctx context.Context, namesOrIds []string, o
 }
 
 func (ic *ContainerEngine) NetworkCreate(ctx context.Context, name string, options entities.NetworkCreateOptions) (*entities.NetworkCreateReport, error) {
-	var (
-		err      error
-		fileName string
-	)
-	if len(options.MacVLAN) > 0 {
-		fileName, err = createMacVLAN(ic.Libpod, name, options)
-	} else {
-		fileName, err = createBridge(ic.Libpod, name, options)
-	}
-	if err != nil {
-		return nil, err
-	}
-	return &entities.NetworkCreateReport{Filename: fileName}, nil
-}
-
-// createBridge creates a CNI network
-func createBridge(r *libpod.Runtime, name string, options entities.NetworkCreateOptions) (string, error) {
-	isGateway := true
-	ipMasq := true
-	subnet := &options.Subnet
-	ipRange := options.Range
-	runtimeConfig, err := r.GetConfig()
-	if err != nil {
-		return "", err
-	}
-	// if range is provided, make sure it is "in" network
-	if subnet.IP != nil {
-		// if network is provided, does it conflict with existing CNI or live networks
-		err = network.ValidateUserNetworkIsAvailable(runtimeConfig, subnet)
-	} else {
-		// if no network is provided, figure out network
-		subnet, err = network.GetFreeNetwork(runtimeConfig)
-	}
-	if err != nil {
-		return "", err
-	}
-	gateway := options.Gateway
-	if gateway == nil {
-		// if no gateway is provided, provide it as first ip of network
-		gateway = network.CalcGatewayIP(subnet)
-	}
-	// if network is provided and if gateway is provided, make sure it is "in" network
-	if options.Subnet.IP != nil && options.Gateway != nil {
-		if !subnet.Contains(gateway) {
-			return "", errors.Errorf("gateway %s is not in valid for subnet %s", gateway.String(), subnet.String())
-		}
-	}
-	if options.Internal {
-		isGateway = false
-		ipMasq = false
-	}
-
-	// if a range is given, we need to ensure it is "in" the network range.
-	if options.Range.IP != nil {
-		if options.Subnet.IP == nil {
-			return "", errors.New("you must define a subnet range to define an ip-range")
-		}
-		firstIP, err := network.FirstIPInSubnet(&options.Range)
-		if err != nil {
-			return "", err
-		}
-		lastIP, err := network.LastIPInSubnet(&options.Range)
-		if err != nil {
-			return "", err
-		}
-		if !subnet.Contains(firstIP) || !subnet.Contains(lastIP) {
-			return "", errors.Errorf("the ip range %s does not fall within the subnet range %s", options.Range.String(), subnet.String())
-		}
-	}
-	bridgeDeviceName, err := network.GetFreeDeviceName(runtimeConfig)
-	if err != nil {
-		return "", err
-	}
-
-	if len(name) > 0 {
-		netNames, err := network.GetNetworkNamesFromFileSystem(runtimeConfig)
-		if err != nil {
-			return "", err
-		}
-		if util.StringInSlice(name, netNames) {
-			return "", errors.Errorf("the network name %s is already used", name)
-		}
-	} else {
-		// If no name is given, we give the name of the bridge device
-		name = bridgeDeviceName
-	}
-
-	ncList := network.NewNcList(name, cniversion.Current())
-	var plugins []network.CNIPlugins
-	var routes []network.IPAMRoute
-
-	defaultRoute, err := network.NewIPAMDefaultRoute(network.IsIPv6(subnet.IP))
-	if err != nil {
-		return "", err
-	}
-	routes = append(routes, defaultRoute)
-	ipamConfig, err := network.NewIPAMHostLocalConf(subnet, routes, ipRange, gateway)
-	if err != nil {
-		return "", err
-	}
-
-	// TODO need to iron out the role of isDefaultGW and IPMasq
-	bridge := network.NewHostLocalBridge(bridgeDeviceName, isGateway, false, ipMasq, ipamConfig)
-	plugins = append(plugins, bridge)
-	plugins = append(plugins, network.NewPortMapPlugin())
-	plugins = append(plugins, network.NewFirewallPlugin())
-	// if we find the dnsname plugin, we add configuration for it
-	if network.HasDNSNamePlugin(runtimeConfig.Network.CNIPluginDirs) && !options.DisableDNS {
-		// Note: in the future we might like to allow for dynamic domain names
-		plugins = append(plugins, network.NewDNSNamePlugin(network.DefaultPodmanDomainName))
-	}
-	ncList["plugins"] = plugins
-	b, err := json.MarshalIndent(ncList, "", "   ")
-	if err != nil {
-		return "", err
-	}
-	if err := os.MkdirAll(network.GetCNIConfDir(runtimeConfig), 0755); err != nil {
-		return "", err
-	}
-	cniPathName := filepath.Join(network.GetCNIConfDir(runtimeConfig), fmt.Sprintf("%s.conflist", name))
-	err = ioutil.WriteFile(cniPathName, b, 0644)
-	return cniPathName, err
-}
-
-func createMacVLAN(r *libpod.Runtime, name string, options entities.NetworkCreateOptions) (string, error) {
-	var (
-		plugins []network.CNIPlugins
-	)
-	liveNetNames, err := network.GetLiveNetworkNames()
-	if err != nil {
-		return "", err
-	}
-
-	config, err := r.GetConfig()
-	if err != nil {
-		return "", err
-	}
-
-	// Make sure the host-device exists
-	if !util.StringInSlice(options.MacVLAN, liveNetNames) {
-		return "", errors.Errorf("failed to find network interface %q", options.MacVLAN)
-	}
-	if len(name) > 0 {
-		netNames, err := network.GetNetworkNamesFromFileSystem(config)
-		if err != nil {
-			return "", err
-		}
-		if util.StringInSlice(name, netNames) {
-			return "", errors.Errorf("the network name %s is already used", name)
-		}
-	} else {
-		name, err = network.GetFreeDeviceName(config)
-		if err != nil {
-			return "", err
-		}
-	}
-	ncList := network.NewNcList(name, cniversion.Current())
-	macvlan := network.NewMacVLANPlugin(options.MacVLAN)
-	plugins = append(plugins, macvlan)
-	ncList["plugins"] = plugins
-	b, err := json.MarshalIndent(ncList, "", "   ")
-	if err != nil {
-		return "", err
-	}
-	cniPathName := filepath.Join(network.GetCNIConfDir(config), fmt.Sprintf("%s.conflist", name))
-	err = ioutil.WriteFile(cniPathName, b, 0644)
-	return cniPathName, err
+	return network.Create(name, options, ic.Libpod)
 }
 
 func ifPassesFilterTest(netconf *libcni.NetworkConfigList, filter []string) bool {

test/e2e/network_create_test.go

@@ -8,7 +8,7 @@ import (
 	"strings"
 
 	cniversion "github.com/containernetworking/cni/pkg/version"
-	"github.com/containers/podman/v2/pkg/network"
+	"github.com/containers/podman/v2/libpod/network"
 	. "github.com/containers/podman/v2/test/utils"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"