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

diff --git a/pkg/ip/link.go b/pkg/ip/link.go
index ff0bdb2..a984262 100644 (file)
--- 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 85d8b94..23182a5 100644 (file)
--- 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 87aa141..90e1260 100644 (file)
--- 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 8233491..876cb5b 100644 (file)
--- 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 5a8fd5a..5b5578e 100644 (file)
--- 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 3afced3..4fba432 100644 (file)
--- 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 7f2d64e..f1585fa 100644 (file)
--- 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 c4502f1..0d34a4c 100644 (file)
--- 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 dfb4e8c..e986e7a 100644 (file)
--- 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 0010915..f90bafb 100644 (file)
--- 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 0330a2c..d01f05f 100644 (file)
--- 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())
+       })
 })