From 5abc571b49c5cce4da00054851a1fc0dd685d62e Mon Sep 17 00:00:00 2001 From: baude Date: Tue, 6 Oct 2020 12:24:21 -0500 Subject: [PATCH] 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 Signed-off-by: Matthew Heon --- cmd/podman/networks/create.go | 4 - cmd/podman/networks/list.go | 2 +- {pkg => libpod}/network/config.go | 11 ++ libpod/network/create.go | 195 ++++++++++++++++++++ {pkg => libpod}/network/devices.go | 0 {pkg => libpod}/network/files.go | 0 {pkg => libpod}/network/ip.go | 0 libpod/network/lock.go | 26 +++ {pkg => libpod}/network/netconflist.go | 0 {pkg => libpod}/network/netconflist_test.go | 0 {pkg => libpod}/network/network.go | 10 +- {pkg => libpod}/network/network_test.go | 0 {pkg => libpod}/network/subnet.go | 0 {pkg => libpod}/network/subnet_test.go | 0 pkg/api/handlers/compat/networks.go | 8 +- pkg/domain/infra/abi/network.go | 176 +----------------- test/e2e/network_create_test.go | 2 +- 17 files changed, 249 insertions(+), 185 deletions(-) rename {pkg => libpod}/network/config.go (93%) create mode 100644 libpod/network/create.go rename {pkg => libpod}/network/devices.go (100%) rename {pkg => libpod}/network/files.go (100%) rename {pkg => libpod}/network/ip.go (100%) create mode 100644 libpod/network/lock.go rename {pkg => libpod}/network/netconflist.go (100%) rename {pkg => libpod}/network/netconflist_test.go (100%) rename {pkg => libpod}/network/network.go (96%) rename {pkg => libpod}/network/network_test.go (100%) rename {pkg => libpod}/network/subnet.go (100%) rename {pkg => libpod}/network/subnet_test.go (100%) diff --git a/cmd/podman/networks/create.go b/cmd/podman/networks/create.go index 68a577ae12..17f39bd8bd 100644 --- a/cmd/podman/networks/create.go +++ b/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 diff --git a/cmd/podman/networks/list.go b/cmd/podman/networks/list.go index b6fb2bb80a..c53f50c9ff 100644 --- a/cmd/podman/networks/list.go +++ b/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" ) diff --git a/pkg/network/config.go b/libpod/network/config.go similarity index 93% rename from pkg/network/config.go rename to libpod/network/config.go index 0115433e1a..a08e684d85 100644 --- a/pkg/network/config.go +++ b/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") diff --git a/libpod/network/create.go b/libpod/network/create.go new file mode 100644 index 0000000000..a9ed4c4efa --- /dev/null +++ b/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 +} diff --git a/pkg/network/devices.go b/libpod/network/devices.go similarity index 100% rename from pkg/network/devices.go rename to libpod/network/devices.go diff --git a/pkg/network/files.go b/libpod/network/files.go similarity index 100% rename from pkg/network/files.go rename to libpod/network/files.go diff --git a/pkg/network/ip.go b/libpod/network/ip.go similarity index 100% rename from pkg/network/ip.go rename to libpod/network/ip.go diff --git a/libpod/network/lock.go b/libpod/network/lock.go new file mode 100644 index 0000000000..0395359eb4 --- /dev/null +++ b/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() +} diff --git a/pkg/network/netconflist.go b/libpod/network/netconflist.go similarity index 100% rename from pkg/network/netconflist.go rename to libpod/network/netconflist.go diff --git a/pkg/network/netconflist_test.go b/libpod/network/netconflist_test.go similarity index 100% rename from pkg/network/netconflist_test.go rename to libpod/network/netconflist_test.go diff --git a/pkg/network/network.go b/libpod/network/network.go similarity index 96% rename from pkg/network/network.go rename to libpod/network/network.go index c4c1ff67f5..7327a1a7d1 100644 --- a/pkg/network/network.go +++ b/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 diff --git a/pkg/network/network_test.go b/libpod/network/network_test.go similarity index 100% rename from pkg/network/network_test.go rename to libpod/network/network_test.go diff --git a/pkg/network/subnet.go b/libpod/network/subnet.go similarity index 100% rename from pkg/network/subnet.go rename to libpod/network/subnet.go diff --git a/pkg/network/subnet_test.go b/libpod/network/subnet_test.go similarity index 100% rename from pkg/network/subnet_test.go rename to libpod/network/subnet_test.go diff --git a/pkg/api/handlers/compat/networks.go b/pkg/api/handlers/compat/networks.go index c5387b1e93..a46784a6c0 100644 --- a/pkg/api/handlers/compat/networks.go +++ b/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, "") } diff --git a/pkg/domain/infra/abi/network.go b/pkg/domain/infra/abi/network.go index 807e4b272a..4f7273226b 100644 --- a/pkg/domain/infra/abi/network.go +++ b/pkg/domain/infra/abi/network.go @@ -2,18 +2,12 @@ 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/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" ) @@ -101,173 +95,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 { diff --git a/test/e2e/network_create_test.go b/test/e2e/network_create_test.go index ddb5204c67..3230255378 100644 --- a/test/e2e/network_create_test.go +++ b/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"