diff options
author | baude <bbaude@redhat.com> | 2020-10-06 12:24:21 -0500 |
---|---|---|
committer | baude <bbaude@redhat.com> | 2020-10-07 10:03:21 -0500 |
commit | fe3faa517e1bbc3b2e82afaae32d8712c844fdae (patch) | |
tree | 3b4a74edc98a2861d2e1b6bb1d9769e078b9ba3c | |
parent | defb754945b3f99c1d786dac95d9b17b24f55e59 (diff) | |
download | podman-fe3faa517e1bbc3b2e82afaae32d8712c844fdae.tar.gz podman-fe3faa517e1bbc3b2e82afaae32d8712c844fdae.tar.bz2 podman-fe3faa517e1bbc3b2e82afaae32d8712c844fdae.zip |
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>
-rw-r--r-- | cmd/podman/networks/create.go | 4 | ||||
-rw-r--r-- | cmd/podman/networks/list.go | 2 | ||||
-rw-r--r-- | libpod/network/config.go (renamed from pkg/network/config.go) | 11 | ||||
-rw-r--r-- | libpod/network/create.go | 195 | ||||
-rw-r--r-- | libpod/network/devices.go (renamed from pkg/network/devices.go) | 0 | ||||
-rw-r--r-- | libpod/network/files.go (renamed from pkg/network/files.go) | 0 | ||||
-rw-r--r-- | libpod/network/ip.go (renamed from pkg/network/ip.go) | 0 | ||||
-rw-r--r-- | libpod/network/lock.go | 26 | ||||
-rw-r--r-- | libpod/network/netconflist.go (renamed from pkg/network/netconflist.go) | 0 | ||||
-rw-r--r-- | libpod/network/netconflist_test.go (renamed from pkg/network/netconflist_test.go) | 0 | ||||
-rw-r--r-- | libpod/network/network.go (renamed from pkg/network/network.go) | 10 | ||||
-rw-r--r-- | libpod/network/network_test.go (renamed from pkg/network/network_test.go) | 0 | ||||
-rw-r--r-- | libpod/network/subnet.go (renamed from pkg/network/subnet.go) | 0 | ||||
-rw-r--r-- | libpod/network/subnet_test.go (renamed from pkg/network/subnet_test.go) | 0 | ||||
-rw-r--r-- | pkg/api/handlers/compat/networks.go | 8 | ||||
-rw-r--r-- | pkg/domain/infra/abi/network.go | 176 | ||||
-rw-r--r-- | test/e2e/network_create_test.go | 2 |
17 files changed, 249 insertions, 185 deletions
diff --git a/cmd/podman/networks/create.go b/cmd/podman/networks/create.go index 68a577ae1..17f39bd8b 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 b6fb2bb80..c53f50c9f 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 index 0115433e1..a08e684d8 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 000000000..a9ed4c4ef --- /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 index a5d23fae4..a5d23fae4 100644 --- a/pkg/network/devices.go +++ b/libpod/network/devices.go diff --git a/pkg/network/files.go b/libpod/network/files.go index a2090491f..a2090491f 100644 --- a/pkg/network/files.go +++ b/libpod/network/files.go diff --git a/pkg/network/ip.go b/libpod/network/ip.go index ba93a0d05..ba93a0d05 100644 --- a/pkg/network/ip.go +++ b/libpod/network/ip.go diff --git a/libpod/network/lock.go b/libpod/network/lock.go new file mode 100644 index 000000000..0395359eb --- /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 index 8187fdb39..8187fdb39 100644 --- a/pkg/network/netconflist.go +++ b/libpod/network/netconflist.go diff --git a/pkg/network/netconflist_test.go b/libpod/network/netconflist_test.go index 5893bf985..5893bf985 100644 --- a/pkg/network/netconflist_test.go +++ b/libpod/network/netconflist_test.go diff --git a/pkg/network/network.go b/libpod/network/network.go index c4c1ff67f..7327a1a7d 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 index 1969e792c..1969e792c 100644 --- a/pkg/network/network_test.go +++ b/libpod/network/network_test.go diff --git a/pkg/network/subnet.go b/libpod/network/subnet.go index 90f0cdfce..90f0cdfce 100644 --- a/pkg/network/subnet.go +++ b/libpod/network/subnet.go diff --git a/pkg/network/subnet_test.go b/libpod/network/subnet_test.go index 917c3be88..917c3be88 100644 --- a/pkg/network/subnet_test.go +++ b/libpod/network/subnet_test.go diff --git a/pkg/api/handlers/compat/networks.go b/pkg/api/handlers/compat/networks.go index c5387b1e9..a46784a6c 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 5acfea853..f40df828a 100644 --- a/pkg/domain/infra/abi/network.go +++ b/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 { diff --git a/test/e2e/network_create_test.go b/test/e2e/network_create_test.go index edd76739f..5c5895537 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" |