Merge pull request kubevirt#129 from mauriciovasquezbernal/fix_ip_lea…

…kage_bridge

bridge: release IP in case of error

plugins/main/bridge/bridge.go

@@ -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)
 }
 

plugins/main/bridge/bridge_test.go

@@ -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())
+	})
 })