From 13824487c6f97a8ffde7b4ef10330c75ddc34826 Mon Sep 17 00:00:00 2001 From: Tom Denham Date: Mon, 20 Mar 2017 15:49:35 -0700 Subject: [PATCH] plugins/*: Don't error if the device doesn't exist I wasn't able to test or update the dhcp plugin but from a code read it should be fine. All the other plugins are tested and fixed --- pkg/ip/link.go | 8 ++++ pkg/ip/link_test.go | 14 +++++++ plugins/ipam/host-local/host_local_test.go | 34 +++++++++++++++++ plugins/main/bridge/bridge.go | 15 +++++--- plugins/main/bridge/bridge_test.go | 43 ++++++++++++++++++++++ plugins/main/ipvlan/ipvlan.go | 13 ++++++- plugins/main/ipvlan/ipvlan_test.go | 37 +++++++++++++++++++ plugins/main/macvlan/macvlan.go | 13 ++++++- plugins/main/macvlan/macvlan_test.go | 38 +++++++++++++++++++ plugins/main/ptp/ptp.go | 15 +++++--- plugins/main/ptp/ptp_test.go | 38 +++++++++++++++++++ 11 files changed, 254 insertions(+), 14 deletions(-) diff --git a/pkg/ip/link.go b/pkg/ip/link.go index ff0bdb29..a9842627 100644 --- a/pkg/ip/link.go +++ b/pkg/ip/link.go @@ -16,6 +16,7 @@ package ip import ( "crypto/rand" + "errors" "fmt" "net" "os" @@ -25,6 +26,10 @@ import ( "github.com/vishvananda/netlink" ) +var ( + ErrLinkNotFound = errors.New("link not found") +) + func makeVethPair(name, peer string, mtu int) (netlink.Link, error) { veth := &netlink.Veth{ LinkAttrs: netlink.LinkAttrs{ @@ -168,6 +173,9 @@ func DelLinkByName(ifName string) error { func DelLinkByNameAddr(ifName string, family int) (*net.IPNet, error) { iface, err := netlink.LinkByName(ifName) if err != nil { + if err != nil && err.Error() == "Link not found" { + return nil, ErrLinkNotFound + } return nil, fmt.Errorf("failed to lookup %q: %v", ifName, err) } diff --git a/pkg/ip/link_test.go b/pkg/ip/link_test.go index 85d8b944..23182a54 100644 --- a/pkg/ip/link_test.go +++ b/pkg/ip/link_test.go @@ -127,6 +127,20 @@ var _ = Describe("Link", func() { }) }) + Context("deleting an non-existent device", func() { + It("returns known error", func() { + _ = containerNetNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + + // This string should match the expected error codes in the cmdDel functions of some of the plugins + _, err := ip.DelLinkByNameAddr("THIS_DONT_EXIST", netlink.FAMILY_V4) + Expect(err).To(Equal(ip.ErrLinkNotFound)) + + return nil + }) + }) + }) + Context("when there is no name available for the host-side", func() { BeforeEach(func() { //adding different interface to container ns diff --git a/plugins/ipam/host-local/host_local_test.go b/plugins/ipam/host-local/host_local_test.go index 87aa141e..90e1260e 100644 --- a/plugins/ipam/host-local/host_local_test.go +++ b/plugins/ipam/host-local/host_local_test.go @@ -101,6 +101,40 @@ var _ = Describe("host-local Operations", func() { Expect(err).To(HaveOccurred()) }) + It("doesn't error when passed an unknown ID on DEL", func() { + const ifname string = "eth0" + const nspath string = "/some/where" + + tmpDir, err := ioutil.TempDir("", "host_local_artifacts") + Expect(err).NotTo(HaveOccurred()) + defer os.RemoveAll(tmpDir) + + conf := fmt.Sprintf(`{ + "cniVersion": "0.3.0", + "name": "mynet", + "type": "ipvlan", + "master": "foo0", + "ipam": { + "type": "host-local", + "subnet": "10.1.2.0/24", + "dataDir": "%s" + } +}`, tmpDir) + + args := &skel.CmdArgs{ + ContainerID: "dummy", + Netns: nspath, + IfName: ifname, + StdinData: []byte(conf), + } + + // Release the IP + err = testutils.CmdDelWithResult(nspath, ifname, func() error { + return cmdDel(args) + }) + Expect(err).NotTo(HaveOccurred()) + }) + It("allocates and releases an address with ADD/DEL and 0.1.0 config", func() { const ifname string = "eth0" const nspath string = "/some/where" diff --git a/plugins/main/bridge/bridge.go b/plugins/main/bridge/bridge.go index 82334918..876cb5bb 100644 --- a/plugins/main/bridge/bridge.go +++ b/plugins/main/bridge/bridge.go @@ -386,25 +386,30 @@ func cmdDel(args *skel.CmdArgs) error { return nil } + // There is a netns so try to clean up. Delete can be called multiple times + // so don't return an error if the device is already removed. + // If the device isn't there then don't try to clean up IP masq either. var ipn *net.IPNet err = ns.WithNetNSPath(args.Netns, func(_ ns.NetNS) error { var err error ipn, err = ip.DelLinkByNameAddr(args.IfName, netlink.FAMILY_V4) + if err != nil && err == ip.ErrLinkNotFound { + return nil + } return err }) + if err != nil { return err } - if n.IPMasq { + if ipn != nil && n.IPMasq { chain := utils.FormatChainName(n.Name, args.ContainerID) comment := utils.FormatComment(n.Name, args.ContainerID) - if err = ip.TeardownIPMasq(ipn, chain, comment); err != nil { - return err - } + err = ip.TeardownIPMasq(ipn, chain, comment) } - return nil + return err } func main() { diff --git a/plugins/main/bridge/bridge_test.go b/plugins/main/bridge/bridge_test.go index 5a8fd5a0..5b5578e2 100644 --- a/plugins/main/bridge/bridge_test.go +++ b/plugins/main/bridge/bridge_test.go @@ -273,6 +273,49 @@ var _ = Describe("bridge Operations", func() { }) }) + It("deconfigures an unconfigured bridge with DEL", func() { + const BRNAME = "cni0" + const IFNAME = "eth0" + + _, subnet, err := net.ParseCIDR("10.1.2.1/24") + Expect(err).NotTo(HaveOccurred()) + + conf := fmt.Sprintf(`{ + "cniVersion": "0.3.0", + "name": "mynet", + "type": "bridge", + "bridge": "%s", + "isDefaultGateway": true, + "ipMasq": false, + "ipam": { + "type": "host-local", + "subnet": "%s" + } +}`, BRNAME, subnet.String()) + + targetNs, err := ns.NewNS() + Expect(err).NotTo(HaveOccurred()) + defer targetNs.Close() + + args := &skel.CmdArgs{ + ContainerID: "dummy", + Netns: targetNs.Path(), + IfName: IFNAME, + StdinData: []byte(conf), + } + + err = originalNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + + err := testutils.CmdDelWithResult(targetNs.Path(), IFNAME, func() error { + return cmdDel(args) + }) + Expect(err).NotTo(HaveOccurred()) + return nil + }) + Expect(err).NotTo(HaveOccurred()) + }) + It("configures and deconfigures a bridge and veth with default route with ADD/DEL for 0.1.0 config", func() { const BRNAME = "cni0" const IFNAME = "eth0" diff --git a/plugins/main/ipvlan/ipvlan.go b/plugins/main/ipvlan/ipvlan.go index 3afced35..4fba4327 100644 --- a/plugins/main/ipvlan/ipvlan.go +++ b/plugins/main/ipvlan/ipvlan.go @@ -191,9 +191,18 @@ func cmdDel(args *skel.CmdArgs) error { return nil } - return ns.WithNetNSPath(args.Netns, func(_ ns.NetNS) error { - return ip.DelLinkByName(args.IfName) + // There is a netns so try to clean up. Delete can be called multiple times + // so don't return an error if the device is already removed. + err = ns.WithNetNSPath(args.Netns, func(_ ns.NetNS) error { + if _, err := ip.DelLinkByNameAddr(args.IfName, netlink.FAMILY_V4); err != nil { + if err != ip.ErrLinkNotFound { + return err + } + } + return nil }) + + return err } func main() { diff --git a/plugins/main/ipvlan/ipvlan_test.go b/plugins/main/ipvlan/ipvlan_test.go index 7f2d64ed..f1585fa4 100644 --- a/plugins/main/ipvlan/ipvlan_test.go +++ b/plugins/main/ipvlan/ipvlan_test.go @@ -187,4 +187,41 @@ var _ = Describe("ipvlan Operations", func() { }) Expect(err).NotTo(HaveOccurred()) }) + + It("deconfigures an unconfigured ipvlan link with DEL", func() { + const IFNAME = "ipvl0" + + conf := fmt.Sprintf(`{ + "cniVersion": "0.3.0", + "name": "mynet", + "type": "ipvlan", + "master": "%s", + "ipam": { + "type": "host-local", + "subnet": "10.1.2.0/24" + } +}`, MASTER_NAME) + + targetNs, err := ns.NewNS() + Expect(err).NotTo(HaveOccurred()) + defer targetNs.Close() + + args := &skel.CmdArgs{ + ContainerID: "dummy", + Netns: targetNs.Path(), + IfName: IFNAME, + StdinData: []byte(conf), + } + + err = originalNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + + err = testutils.CmdDelWithResult(targetNs.Path(), IFNAME, func() error { + return cmdDel(args) + }) + Expect(err).NotTo(HaveOccurred()) + return nil + }) + Expect(err).NotTo(HaveOccurred()) + }) }) diff --git a/plugins/main/macvlan/macvlan.go b/plugins/main/macvlan/macvlan.go index c4502f1f..0d34a4c7 100644 --- a/plugins/main/macvlan/macvlan.go +++ b/plugins/main/macvlan/macvlan.go @@ -232,9 +232,18 @@ func cmdDel(args *skel.CmdArgs) error { return nil } - return ns.WithNetNSPath(args.Netns, func(_ ns.NetNS) error { - return ip.DelLinkByName(args.IfName) + // There is a netns so try to clean up. Delete can be called multiple times + // so don't return an error if the device is already removed. + err = ns.WithNetNSPath(args.Netns, func(_ ns.NetNS) error { + if _, err := ip.DelLinkByNameAddr(args.IfName, netlink.FAMILY_V4); err != nil { + if err != ip.ErrLinkNotFound { + return err + } + } + return nil }) + + return err } func main() { diff --git a/plugins/main/macvlan/macvlan_test.go b/plugins/main/macvlan/macvlan_test.go index dfb4e8c6..e986e7a7 100644 --- a/plugins/main/macvlan/macvlan_test.go +++ b/plugins/main/macvlan/macvlan_test.go @@ -189,4 +189,42 @@ var _ = Describe("macvlan Operations", func() { }) Expect(err).NotTo(HaveOccurred()) }) + + It("deconfigures an unconfigured macvlan link with DEL", func() { + const IFNAME = "macvl0" + + conf := fmt.Sprintf(`{ + "cniVersion": "0.3.0", + "name": "mynet", + "type": "macvlan", + "master": "%s", + "ipam": { + "type": "host-local", + "subnet": "10.1.2.0/24" + } +}`, MASTER_NAME) + + targetNs, err := ns.NewNS() + Expect(err).NotTo(HaveOccurred()) + defer targetNs.Close() + + args := &skel.CmdArgs{ + ContainerID: "dummy", + Netns: targetNs.Path(), + IfName: IFNAME, + StdinData: []byte(conf), + } + + err = originalNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + + err := testutils.CmdDelWithResult(targetNs.Path(), IFNAME, func() error { + return cmdDel(args) + }) + Expect(err).NotTo(HaveOccurred()) + return nil + }) + Expect(err).NotTo(HaveOccurred()) + + }) }) diff --git a/plugins/main/ptp/ptp.go b/plugins/main/ptp/ptp.go index 00109154..f90bafbc 100644 --- a/plugins/main/ptp/ptp.go +++ b/plugins/main/ptp/ptp.go @@ -268,25 +268,30 @@ func cmdDel(args *skel.CmdArgs) error { return nil } + // There is a netns so try to clean up. Delete can be called multiple times + // so don't return an error if the device is already removed. + // If the device isn't there then don't try to clean up IP masq either. var ipn *net.IPNet err := ns.WithNetNSPath(args.Netns, func(_ ns.NetNS) error { var err error ipn, err = ip.DelLinkByNameAddr(args.IfName, netlink.FAMILY_V4) + if err != nil && err == ip.ErrLinkNotFound { + return nil + } return err }) + if err != nil { return err } - if conf.IPMasq { + if ipn != nil && conf.IPMasq { chain := utils.FormatChainName(conf.Name, args.ContainerID) comment := utils.FormatComment(conf.Name, args.ContainerID) - if err = ip.TeardownIPMasq(ipn, chain, comment); err != nil { - return err - } + err = ip.TeardownIPMasq(ipn, chain, comment) } - return nil + return err } func main() { diff --git a/plugins/main/ptp/ptp_test.go b/plugins/main/ptp/ptp_test.go index 0330a2c6..d01f05f9 100644 --- a/plugins/main/ptp/ptp_test.go +++ b/plugins/main/ptp/ptp_test.go @@ -111,4 +111,42 @@ var _ = Describe("ptp Operations", func() { }) Expect(err).NotTo(HaveOccurred()) }) + It("deconfigures an unconfigured ptp link with DEL", func() { + const IFNAME = "ptp0" + + conf := `{ + "cniVersion": "0.3.0", + "name": "mynet", + "type": "ptp", + "ipMasq": true, + "mtu": 5000, + "ipam": { + "type": "host-local", + "subnet": "10.1.2.0/24" + } +}` + + targetNs, err := ns.NewNS() + Expect(err).NotTo(HaveOccurred()) + defer targetNs.Close() + + args := &skel.CmdArgs{ + ContainerID: "dummy", + Netns: targetNs.Path(), + IfName: IFNAME, + StdinData: []byte(conf), + } + + // Call the plugins with the DEL command. It should not error even though the veth doesn't exist. + err = originalNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + + err := testutils.CmdDelWithResult(targetNs.Path(), IFNAME, func() error { + return cmdDel(args) + }) + Expect(err).NotTo(HaveOccurred()) + return nil + }) + Expect(err).NotTo(HaveOccurred()) + }) })