Skip to content

Commit

Permalink
Merge pull request #7943 from baude/issue7807
Browse files Browse the repository at this point in the history
prevent unpredictable results with network create|remove
  • Loading branch information
openshift-merge-robot authored Oct 7, 2020
2 parents 173e3c2 + fe3faa5 commit 9ae873e
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 9ae873e

Please sign in to comment.