diff --git a/plugins/main/bridge/bridge.go b/plugins/main/bridge/bridge.go index e6b0f118..2f14e603 100644 --- a/plugins/main/bridge/bridge.go +++ b/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) } diff --git a/plugins/main/bridge/bridge_test.go b/plugins/main/bridge/bridge_test.go index bbfa3a68..6ed07a5e 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), @@ -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()) + }) }) diff --git a/plugins/meta/tuning/tuning.go b/plugins/meta/tuning/tuning.go index 9397bc31..beea39a2 100644 --- a/plugins/meta/tuning/tuning.go +++ b/plugins/meta/tuning/tuning.go @@ -50,7 +50,7 @@ type MACEnvArgs struct { } func parseConf(data []byte, envArgs string) (*TuningConf, error) { - conf := TuningConf{Promisc: false, Mtu: -1} + conf := TuningConf{Promisc: false} if err := json.Unmarshal(data, &conf); err != nil { return nil, fmt.Errorf("failed to load netconf: %v", err) } @@ -110,13 +110,12 @@ func changeMacAddr(ifName string, newMacAddr string) error { return netlink.LinkSetUp(link) } -func updateResultsMacAddr(config TuningConf, ifName string, newMacAddr string) error { +func updateResultsMacAddr(config TuningConf, ifName string, newMacAddr string) { for _, i := range config.PrevResult.Interfaces { if i.Name == ifName { i.Mac = newMacAddr } } - return nil } func changePromisc(ifName string, val bool) error { @@ -169,7 +168,7 @@ func cmdAdd(args *skel.CmdArgs) error { if tuningConf.Mac != "" { if err = changeMacAddr(args.IfName, tuningConf.Mac); err == nil { - err = updateResultsMacAddr(*tuningConf, args.IfName, tuningConf.Mac) + updateResultsMacAddr(*tuningConf, args.IfName, tuningConf.Mac) } } @@ -179,7 +178,7 @@ func cmdAdd(args *skel.CmdArgs) error { } } - if tuningConf.Mtu != -1 { + if tuningConf.Mtu != 0 { if err = changeMtu(args.IfName, tuningConf.Mtu); err != nil { return err }