Skip to content

Commit

Permalink
prevent unpredictable results with network create|remove
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
baude committed Oct 7, 2020
1 parent defb754 commit fe3faa5
Show file tree
Hide file tree
Showing 17 changed files with 249 additions and 185 deletions.
4 changes: 0 additions & 4 deletions cmd/podman/networks/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion cmd/podman/networks/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down
11 changes: 11 additions & 0 deletions pkg/network/config.go → libpod/network/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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")
Expand Down
195 changes: 195 additions & 0 deletions libpod/network/create.go
Original file line number Diff line number Diff line change
@@ -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
}
File renamed without changes.
File renamed without changes.
File renamed without changes.
26 changes: 26 additions & 0 deletions libpod/network/lock.go
Original file line number Diff line number Diff line change
@@ -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()
}
File renamed without changes.
File renamed without changes.
10 changes: 8 additions & 2 deletions pkg/network/network.go → libpod/network/network.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
}
Expand Down Expand Up @@ -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
Expand Down
File renamed without changes.
File renamed without changes.
File renamed without changes.
8 changes: 5 additions & 3 deletions pkg/api/handlers/compat/networks.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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, "")
}
Loading

0 comments on commit fe3faa5

Please sign in to comment.