Merge pull request #129 from mauriciovasquezbernal/fix_ip_leakage_bridge
bridge: release IP in case of error
This commit is contained in:
Dan Williams
GitHub
commit
2b819b5571
@ -19,6 +19,7 @@ import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"net"
|
||||
"os"
|
||||
"runtime"
|
||||
"syscall"
|
||||
|
||||
@ -36,6 +37,9 @@ import (
|
||||
"github.com/vishvananda/netlink"
|
||||
)
|
||||
|
||||
// For testcases to force an error after IPAM has been performed
|
||||
var debugPostIPAMError error
|
||||
|
||||
const defaultBrName = "cni0"
|
||||
|
||||
type NetConf struct {
|
||||
@ -323,6 +327,8 @@ func enableIPForward(family int) error {
|
||||
}
|
||||
|
||||
func cmdAdd(args *skel.CmdArgs) error {
|
||||
var success bool = false
|
||||
|
||||
n, cniVersion, err := loadNetConf(args.StdinData)
|
||||
if err != nil {
|
||||
return err
|
||||
@ -358,6 +364,15 @@ func cmdAdd(args *skel.CmdArgs) error {
|
||||
return err
|
||||
}
|
||||
|
||||
// release IP in case of failure
|
||||
defer func() {
|
||||
if !success {
|
||||
os.Setenv("CNI_COMMAND", "DEL")
|
||||
ipam.ExecDel(n.IPAM.Type, args.StdinData)
|
||||
os.Setenv("CNI_COMMAND", "ADD")
|
||||
}
|
||||
}()
|
||||
|
||||
// Convert whatever the IPAM result was into the current Result type
|
||||
result, err := current.NewResultFromResult(r)
|
||||
if err != nil {
|
||||
@ -454,6 +469,13 @@ func cmdAdd(args *skel.CmdArgs) error {
|
||||
|
||||
result.DNS = n.DNS
|
||||
|
||||
// Return an error requested by testcases, if any
|
||||
if debugPostIPAMError != nil {
|
||||
return debugPostIPAMError
|
||||
}
|
||||
|
||||
success = true
|
||||
|
||||
return types.PrintResult(result, cniVersion)
|
||||
}
|
||||
|
||||
|
||||
@ -16,7 +16,9 @@ package main
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"io/ioutil"
|
||||
"net"
|
||||
"os"
|
||||
"strings"
|
||||
|
||||
"github.com/containernetworking/cni/pkg/skel"
|
||||
@ -83,6 +85,9 @@ const (
|
||||
"ipam": {
|
||||
"type": "host-local"`
|
||||
|
||||
ipamDataDirStr = `,
|
||||
"dataDir": "%s"`
|
||||
|
||||
// Single subnet configuration (legacy)
|
||||
subnetConfStr = `,
|
||||
"subnet": "%s"`
|
||||
@ -110,10 +115,13 @@ const (
|
||||
|
||||
// netConfJSON() generates a JSON network configuration string
|
||||
// for a test case.
|
||||
func (tc testCase) netConfJSON() string {
|
||||
func (tc testCase) netConfJSON(dataDir string) string {
|
||||
conf := fmt.Sprintf(netConfStr, tc.cniVersion, BRNAME)
|
||||
if tc.subnet != "" || tc.ranges != nil {
|
||||
conf += ipamStartStr
|
||||
if dataDir != "" {
|
||||
conf += fmt.Sprintf(ipamDataDirStr, dataDir)
|
||||
}
|
||||
if tc.subnet != "" {
|
||||
conf += tc.subnetConfig()
|
||||
}
|
||||
@ -152,8 +160,8 @@ var counter uint
|
||||
|
||||
// createCmdArgs generates network configuration and creates command
|
||||
// arguments for a test case.
|
||||
func (tc testCase) createCmdArgs(targetNS ns.NetNS) *skel.CmdArgs {
|
||||
conf := tc.netConfJSON()
|
||||
func (tc testCase) createCmdArgs(targetNS ns.NetNS, dataDir string) *skel.CmdArgs {
|
||||
conf := tc.netConfJSON(dataDir)
|
||||
defer func() { counter += 1 }()
|
||||
return &skel.CmdArgs{
|
||||
ContainerID: fmt.Sprintf("dummy-%d", counter),
|
||||
@ -213,9 +221,27 @@ func ipVersion(ip net.IP) string {
|
||||
return "6"
|
||||
}
|
||||
|
||||
func countIPAMIPs(path string) (int, error) {
|
||||
count := 0
|
||||
files, err := ioutil.ReadDir(path)
|
||||
if err != nil {
|
||||
return -1, err
|
||||
}
|
||||
for _, file := range files {
|
||||
if file.IsDir() {
|
||||
continue
|
||||
}
|
||||
|
||||
if net.ParseIP(file.Name()) != nil {
|
||||
count++
|
||||
}
|
||||
}
|
||||
return count, nil
|
||||
}
|
||||
|
||||
type cmdAddDelTester interface {
|
||||
setNS(testNS ns.NetNS, targetNS ns.NetNS)
|
||||
cmdAddTest(tc testCase)
|
||||
cmdAddTest(tc testCase, dataDir string)
|
||||
cmdDelTest(tc testCase)
|
||||
}
|
||||
|
||||
@ -240,9 +266,9 @@ func (tester *testerV03x) setNS(testNS ns.NetNS, targetNS ns.NetNS) {
|
||||
tester.targetNS = targetNS
|
||||
}
|
||||
|
||||
func (tester *testerV03x) cmdAddTest(tc testCase) {
|
||||
func (tester *testerV03x) cmdAddTest(tc testCase, dataDir string) {
|
||||
// Generate network config and command arguments
|
||||
tester.args = tc.createCmdArgs(tester.targetNS)
|
||||
tester.args = tc.createCmdArgs(tester.targetNS, dataDir)
|
||||
|
||||
// Execute cmdADD on the plugin
|
||||
var result *current.Result
|
||||
@ -419,9 +445,9 @@ func (tester *testerV01xOr02x) setNS(testNS ns.NetNS, targetNS ns.NetNS) {
|
||||
tester.targetNS = targetNS
|
||||
}
|
||||
|
||||
func (tester *testerV01xOr02x) cmdAddTest(tc testCase) {
|
||||
func (tester *testerV01xOr02x) cmdAddTest(tc testCase, dataDir string) {
|
||||
// Generate network config and calculate gateway addresses
|
||||
tester.args = tc.createCmdArgs(tester.targetNS)
|
||||
tester.args = tc.createCmdArgs(tester.targetNS, dataDir)
|
||||
|
||||
// Execute cmdADD on the plugin
|
||||
err := tester.testNS.Do(func(ns.NetNS) error {
|
||||
@ -537,7 +563,7 @@ func (tester *testerV01xOr02x) cmdDelTest(tc testCase) {
|
||||
Expect(err).NotTo(HaveOccurred())
|
||||
}
|
||||
|
||||
func cmdAddDelTest(testNS ns.NetNS, tc testCase) {
|
||||
func cmdAddDelTest(testNS ns.NetNS, tc testCase, dataDir string) {
|
||||
// Get a Add/Del tester based on test case version
|
||||
tester := testerByVersion(tc.cniVersion)
|
||||
|
||||
@ -547,7 +573,7 @@ func cmdAddDelTest(testNS ns.NetNS, tc testCase) {
|
||||
tester.setNS(testNS, targetNS)
|
||||
|
||||
// Test IP allocation
|
||||
tester.cmdAddTest(tc)
|
||||
tester.cmdAddTest(tc, dataDir)
|
||||
|
||||
// Test IP Release
|
||||
tester.cmdDelTest(tc)
|
||||
@ -558,15 +584,23 @@ func cmdAddDelTest(testNS ns.NetNS, tc testCase) {
|
||||
|
||||
var _ = Describe("bridge Operations", func() {
|
||||
var originalNS ns.NetNS
|
||||
var dataDir string
|
||||
|
||||
BeforeEach(func() {
|
||||
// Create a new NetNS so we don't modify the host
|
||||
var err error
|
||||
originalNS, err = testutils.NewNS()
|
||||
Expect(err).NotTo(HaveOccurred())
|
||||
|
||||
dataDir, err = ioutil.TempDir("", "bridge_test")
|
||||
Expect(err).NotTo(HaveOccurred())
|
||||
|
||||
// Do not emulate an error, each test will set this if needed
|
||||
debugPostIPAMError = nil
|
||||
})
|
||||
|
||||
AfterEach(func() {
|
||||
Expect(os.RemoveAll(dataDir)).To(Succeed())
|
||||
Expect(originalNS.Close()).To(Succeed())
|
||||
})
|
||||
|
||||
@ -661,7 +695,7 @@ var _ = Describe("bridge Operations", func() {
|
||||
}
|
||||
for _, tc := range testCases {
|
||||
tc.cniVersion = "0.3.0"
|
||||
cmdAddDelTest(originalNS, tc)
|
||||
cmdAddDelTest(originalNS, tc, dataDir)
|
||||
}
|
||||
})
|
||||
|
||||
@ -691,7 +725,7 @@ var _ = Describe("bridge Operations", func() {
|
||||
}
|
||||
for _, tc := range testCases {
|
||||
tc.cniVersion = "0.3.1"
|
||||
cmdAddDelTest(originalNS, tc)
|
||||
cmdAddDelTest(originalNS, tc, dataDir)
|
||||
}
|
||||
})
|
||||
|
||||
@ -707,7 +741,7 @@ var _ = Describe("bridge Operations", func() {
|
||||
Expect(err).NotTo(HaveOccurred())
|
||||
defer targetNS.Close()
|
||||
tester.setNS(originalNS, targetNS)
|
||||
tester.args = tc.createCmdArgs(targetNS)
|
||||
tester.args = tc.createCmdArgs(targetNS, dataDir)
|
||||
|
||||
// Execute cmdDEL on the plugin, expect no errors
|
||||
tester.cmdDelTest(tc)
|
||||
@ -739,7 +773,7 @@ var _ = Describe("bridge Operations", func() {
|
||||
}
|
||||
for _, tc := range testCases {
|
||||
tc.cniVersion = "0.1.0"
|
||||
cmdAddDelTest(originalNS, tc)
|
||||
cmdAddDelTest(originalNS, tc, dataDir)
|
||||
}
|
||||
})
|
||||
|
||||
@ -909,11 +943,44 @@ var _ = Describe("bridge Operations", func() {
|
||||
Expect(err).NotTo(HaveOccurred())
|
||||
origMac := link.Attrs().HardwareAddr
|
||||
|
||||
cmdAddDelTest(originalNS, tc)
|
||||
cmdAddDelTest(originalNS, tc, dataDir)
|
||||
|
||||
link, err = netlink.LinkByName(BRNAME)
|
||||
Expect(err).NotTo(HaveOccurred())
|
||||
Expect(link.Attrs().HardwareAddr).To(Equal(origMac))
|
||||
}
|
||||
})
|
||||
|
||||
It("checks ip release in case of error", func() {
|
||||
err := originalNS.Do(func(ns.NetNS) error {
|
||||
defer GinkgoRecover()
|
||||
tc := testCase{
|
||||
cniVersion: "0.3.1",
|
||||
subnet: "10.1.2.0/24",
|
||||
}
|
||||
|
||||
_, _, err := setupBridge(tc.netConf())
|
||||
Expect(err).NotTo(HaveOccurred())
|
||||
|
||||
args := tc.createCmdArgs(originalNS, dataDir)
|
||||
|
||||
// get number of allocated IPs before asking for a new one
|
||||
before, err := countIPAMIPs(dataDir)
|
||||
Expect(err).NotTo(HaveOccurred())
|
||||
|
||||
debugPostIPAMError = fmt.Errorf("debugPostIPAMError")
|
||||
_, _, err = testutils.CmdAddWithArgs(args, func() error {
|
||||
return cmdAdd(args)
|
||||
})
|
||||
Expect(err).To(MatchError("debugPostIPAMError"))
|
||||
|
||||
// get number of allocated IPs after failure
|
||||
after, err := countIPAMIPs(dataDir)
|
||||
Expect(err).NotTo(HaveOccurred())
|
||||
|
||||
Expect(before).To(Equal(after))
|
||||
return nil
|
||||
})
|
||||
Expect(err).NotTo(HaveOccurred())
|
||||
})
|
||||
})
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user