Merge pull request #412 from tomdee/safe-del

plugins/*: Don't error if the device doesn't exist

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

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

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"

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() {

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"

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() {

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

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() {

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

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() {

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