From 53a97d712f31af22c167f2ed11d87b5c91f185cc Mon Sep 17 00:00:00 2001 From: Bryant Ly Date: Tue, 21 Jul 2026 13:06:55 -0700 Subject: [PATCH] plugins/vrf: make DEL idempotent when the device is already gone During concurrent pod-netns teardown the VRF can be removed between findVRF and LinkDel, so LinkDel(vrf) returns ENODEV. cmdDel propagated that error and (under Multus/libcni) aborted the conflist DEL before the chained macvlan DEL ran, orphaning the interface and leaking its netns. resetMaster had the same problem when the enslaved interface was already gone, and additionally masked the underlying error by not wrapping it. Per the CNI SPEC, DEL is best-effort and must not fail when a resource has already been removed. Treat "link not found" as success on both LinkDel and resetMaster, matching by errno (ENODEV/ENOENT) as well as the typed LinkNotFoundError -- LinkDel returns a bare errno, so a type-only check misses it. Also switch the existing findVRF guard to the shared helper and wrap resetMaster errors with %w. Signed-off-by: Bryant Ly --- plugins/meta/vrf/main.go | 10 +++++--- plugins/meta/vrf/vrf.go | 26 ++++++++++++++++--- plugins/meta/vrf/vrf_test.go | 48 ++++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 8 deletions(-) diff --git a/plugins/meta/vrf/main.go b/plugins/meta/vrf/main.go index fc1f61203..93d536820 100644 --- a/plugins/meta/vrf/main.go +++ b/plugins/meta/vrf/main.go @@ -99,10 +99,9 @@ func cmdDel(args *skel.CmdArgs) error { } err = ns.WithNetNSPath(args.Netns, func(_ ns.NetNS) error { vrf, err := findVRF(conf.VRFName) - if _, ok := err.(netlink.LinkNotFoundError); ok { + if linkNotFound(err) { return nil } - if err != nil { return err } @@ -119,8 +118,11 @@ func cmdDel(args *skel.CmdArgs) error { // Meaning, we are deleting the last interface assigned to the VRF if len(interfaces) == 0 { - err = netlink.LinkDel(vrf) - if err != nil { + // CNI DEL must be idempotent: a concurrent netns teardown can remove + // the VRF between findVRF and here, so LinkDel returns ENODEV. Treat + // "already gone" as success (LinkDel returns a bare errno, not the + // typed netlink.LinkNotFoundError). + if err = netlink.LinkDel(vrf); err != nil && !linkNotFound(err) { return err } } diff --git a/plugins/meta/vrf/vrf.go b/plugins/meta/vrf/vrf.go index 5e45f5988..793580e6d 100644 --- a/plugins/meta/vrf/vrf.go +++ b/plugins/meta/vrf/vrf.go @@ -15,9 +15,11 @@ package main import ( + "errors" "fmt" "math" "net" + "syscall" "time" "github.com/vishvananda/netlink" @@ -214,16 +216,32 @@ func findFreeRoutingTableID(links []netlink.Link) (uint32, error) { func resetMaster(interfaceName string) error { intf, err := netlinksafe.LinkByName(interfaceName) - if err != nil { - return fmt.Errorf("resetMaster: could not get link by name %s", interfaceName) + if linkNotFound(err) { + // Interface already gone (e.g. concurrent netns teardown). DEL is + // best-effort, so there is nothing left to reset. + return nil } - err = netlink.LinkSetNoMaster(intf) if err != nil { - return fmt.Errorf("resetMaster: could reset master to %s", interfaceName) + return fmt.Errorf("resetMaster: could not get link by name %s: %w", interfaceName, err) + } + if err := netlink.LinkSetNoMaster(intf); err != nil { + return fmt.Errorf("resetMaster: could not reset master of %s: %w", interfaceName, err) } return nil } +// linkNotFound reports whether err indicates the link is already gone. netlink +// returns a typed LinkNotFoundError from LinkByName, but a bare errno (ENODEV) +// from LinkDel, so both must be matched. Per the CNI spec, DEL is best-effort +// and must not fail when a resource has already been removed. +func linkNotFound(err error) bool { + if err == nil { + return false + } + var lnf netlink.LinkNotFoundError + return errors.As(err, &lnf) || errors.Is(err, syscall.ENODEV) || errors.Is(err, syscall.ENOENT) +} + // getGlobalAddresses returns the global addresses of the given interface func getGlobalAddresses(link netlink.Link, family int) ([]netlink.Addr, error) { addresses, err := netlinksafe.AddrList(link, family) diff --git a/plugins/meta/vrf/vrf_test.go b/plugins/meta/vrf/vrf_test.go index 4e1212297..f771fcfc8 100644 --- a/plugins/meta/vrf/vrf_test.go +++ b/plugins/meta/vrf/vrf_test.go @@ -821,6 +821,54 @@ var _ = Describe("vrf plugin", func() { }) }) + It("returns success on DEL when the enslaved interface is already gone (idempotent)", func() { + conf0 := configFor("test", IF0Name, VRF0Name, "10.0.0.2/24") + + By("Adding the interface to the VRF", func() { + err := originalNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + args := &skel.CmdArgs{ + ContainerID: "dummy", + Netns: targetNS.Path(), + IfName: IF0Name, + StdinData: conf0, + } + _, _, err := testutils.CmdAddWithArgs(args, func() error { + return cmdAdd(args) + }) + Expect(err).NotTo(HaveOccurred()) + return nil + }) + Expect(err).NotTo(HaveOccurred()) + }) + + By("Removing the enslaved interface out-of-band (simulating a teardown race)", func() { + err := targetNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + link, err := netlinksafe.LinkByName(IF0Name) + Expect(err).NotTo(HaveOccurred()) + return netlink.LinkDel(link) + }) + Expect(err).NotTo(HaveOccurred()) + }) + + By("DEL succeeding even though the interface is gone", func() { + err := originalNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + args := &skel.CmdArgs{ + ContainerID: "dummy", + Netns: targetNS.Path(), + IfName: IF0Name, + StdinData: conf0, + } + return testutils.CmdDelWithArgs(args, func() error { + return cmdDel(args) + }) + }) + Expect(err).NotTo(HaveOccurred()) + }) + }) + It("configures and deconfigures VRF with CNI 0.4.0 ADD/DEL", func() { conf := []byte(fmt.Sprintf(`{ "name": "test",