From 4b296ba330efd0dcaa45fd975450eadc7755bcf6 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Wed, 25 Jul 2018 12:19:17 -0500 Subject: [PATCH 1/3] bridge: add random datadir to all testcases --- plugins/main/bridge/bridge_test.go | 43 +++++++++++++++++++----------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/plugins/main/bridge/bridge_test.go b/plugins/main/bridge/bridge_test.go index bbfa3a68..f2bb9d0d 100644 --- a/plugins/main/bridge/bridge_test.go +++ b/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), @@ -215,7 +223,7 @@ func ipVersion(ip net.IP) string { type cmdAddDelTester interface { setNS(testNS ns.NetNS, targetNS ns.NetNS) - cmdAddTest(tc testCase) + cmdAddTest(tc testCase, dataDir string) cmdDelTest(tc testCase) } @@ -240,9 +248,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 +427,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 +545,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 +555,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 +566,20 @@ 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()) }) AfterEach(func() { + Expect(os.RemoveAll(dataDir)).To(Succeed()) Expect(originalNS.Close()).To(Succeed()) }) @@ -661,7 +674,7 @@ var _ = Describe("bridge Operations", func() { } for _, tc := range testCases { tc.cniVersion = "0.3.0" - cmdAddDelTest(originalNS, tc) + cmdAddDelTest(originalNS, tc, dataDir) } }) @@ -691,7 +704,7 @@ var _ = Describe("bridge Operations", func() { } for _, tc := range testCases { tc.cniVersion = "0.3.1" - cmdAddDelTest(originalNS, tc) + cmdAddDelTest(originalNS, tc, dataDir) } }) @@ -707,7 +720,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 +752,7 @@ var _ = Describe("bridge Operations", func() { } for _, tc := range testCases { tc.cniVersion = "0.1.0" - cmdAddDelTest(originalNS, tc) + cmdAddDelTest(originalNS, tc, dataDir) } }) @@ -909,7 +922,7 @@ 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()) From 3a7ee332be6a470720a20d596d1989f22091e04a Mon Sep 17 00:00:00 2001 From: Mauricio Vasquez B Date: Fri, 9 Mar 2018 22:29:41 -0500 Subject: [PATCH 2/3] bridge: release IP in case of error If there is an error after an IP has been allocated it is necesary to release it. Signed-off-by: Mauricio Vasquez B --- plugins/main/bridge/bridge.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/plugins/main/bridge/bridge.go b/plugins/main/bridge/bridge.go index e6b0f118..7941bfad 100644 --- a/plugins/main/bridge/bridge.go +++ b/plugins/main/bridge/bridge.go @@ -19,6 +19,7 @@ import ( "errors" "fmt" "net" + "os" "runtime" "syscall" @@ -323,6 +324,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 +361,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 +466,8 @@ func cmdAdd(args *skel.CmdArgs) error { result.DNS = n.DNS + success = true + return types.PrintResult(result, cniVersion) } From 316489903bfaa665a8d902057990c10f4f62c135 Mon Sep 17 00:00:00 2001 From: Mauricio Vasquez B Date: Thu, 26 Jul 2018 21:15:10 -0500 Subject: [PATCH 3/3] bridge: add test case for release IP on error Signed-off-by: Mauricio Vasquez B --- plugins/main/bridge/bridge.go | 8 +++++ plugins/main/bridge/bridge_test.go | 54 ++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/plugins/main/bridge/bridge.go b/plugins/main/bridge/bridge.go index 7941bfad..2f14e603 100644 --- a/plugins/main/bridge/bridge.go +++ b/plugins/main/bridge/bridge.go @@ -37,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 { @@ -466,6 +469,11 @@ 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) diff --git a/plugins/main/bridge/bridge_test.go b/plugins/main/bridge/bridge_test.go index f2bb9d0d..6ed07a5e 100644 --- a/plugins/main/bridge/bridge_test.go +++ b/plugins/main/bridge/bridge_test.go @@ -221,6 +221,24 @@ 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, dataDir string) @@ -576,6 +594,9 @@ var _ = Describe("bridge Operations", func() { 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() { @@ -929,4 +950,37 @@ var _ = Describe("bridge Operations", func() { 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()) + }) })