diff --git a/plugins/main/host-device/host-device.go b/plugins/main/host-device/host-device.go index d4ffec1e5..3555214c9 100644 --- a/plugins/main/host-device/host-device.go +++ b/plugins/main/host-device/host-device.go @@ -199,34 +199,112 @@ func cmdDel(args *skel.CmdArgs) error { return nil } +// setTempName sets a temporary name for netdevice, returns updated Link object or error +// if occurred. +func setTempName(dev netlink.Link) (netlink.Link, error) { + tempName := fmt.Sprintf("%s%d", "temp_", dev.Attrs().Index) + + // rename to tempName + if err := netlink.LinkSetName(dev, tempName); err != nil { + return nil, fmt.Errorf("failed to rename device %q to %q: %v", dev.Attrs().Name, tempName, err) + } + + // Get updated Link obj + tempDev, err := netlink.LinkByName(tempName) + if err != nil { + return nil, fmt.Errorf("failed to find %q after rename to %q: %v", dev.Attrs().Name, tempName, err) + } + + return tempDev, nil +} + func moveLinkIn(hostDev netlink.Link, containerNs ns.NetNS, ifName string) (netlink.Link, error) { - if err := netlink.LinkSetNsFd(hostDev, int(containerNs.Fd())); err != nil { - return nil, err + origLinkFlags := hostDev.Attrs().Flags + hostDevName := hostDev.Attrs().Name + defaultNs, err := ns.GetCurrentNS() + if err != nil { + return nil, fmt.Errorf("failed to get host namespace: %v", err) + } + + // Devices can be renamed only when down + if err = netlink.LinkSetDown(hostDev); err != nil { + return nil, fmt.Errorf("failed to set %q down: %v", hostDev.Attrs().Name, err) + } + + // restore original link state in case of error + defer func() { + if err != nil { + if origLinkFlags&net.FlagUp == net.FlagUp && hostDev != nil { + _ = netlink.LinkSetUp(hostDev) + } + } + }() + + hostDev, err = setTempName(hostDev) + if err != nil { + return nil, fmt.Errorf("failed to rename device %q to temporary name: %v", hostDevName, err) + } + + // restore original netdev name in case of error + defer func() { + if err != nil && hostDev != nil { + _ = netlink.LinkSetName(hostDev, hostDevName) + } + }() + + if err = netlink.LinkSetNsFd(hostDev, int(containerNs.Fd())); err != nil { + return nil, fmt.Errorf("failed to move %q to container ns: %v", hostDev.Attrs().Name, err) } var contDev netlink.Link - if err := containerNs.Do(func(_ ns.NetNS) error { + tempDevName := hostDev.Attrs().Name + if err = containerNs.Do(func(_ ns.NetNS) error { var err error - contDev, err = netlink.LinkByName(hostDev.Attrs().Name) + contDev, err = netlink.LinkByName(tempDevName) if err != nil { - return fmt.Errorf("failed to find %q: %v", hostDev.Attrs().Name, err) - } - // Devices can be renamed only when down - if err = netlink.LinkSetDown(contDev); err != nil { - return fmt.Errorf("failed to set %q down: %v", hostDev.Attrs().Name, err) + return fmt.Errorf("failed to find %q: %v", tempDevName, err) } + + // move netdev back to host namespace in case of error + defer func() { + if err != nil { + _ = netlink.LinkSetNsFd(contDev, int(defaultNs.Fd())) + // we need to get updated link object as link was moved back to host namepsace + _ = defaultNs.Do(func(_ ns.NetNS) error { + hostDev, _ = netlink.LinkByName(tempDevName) + return nil + }) + } + }() + // Save host device name into the container device's alias property - if err := netlink.LinkSetAlias(contDev, hostDev.Attrs().Name); err != nil { - return fmt.Errorf("failed to set alias to %q: %v", hostDev.Attrs().Name, err) + if err = netlink.LinkSetAlias(contDev, hostDevName); err != nil { + return fmt.Errorf("failed to set alias to %q: %v", tempDevName, err) } // Rename container device to respect args.IfName - if err := netlink.LinkSetName(contDev, ifName); err != nil { - return fmt.Errorf("failed to rename device %q to %q: %v", hostDev.Attrs().Name, ifName, err) + if err = netlink.LinkSetName(contDev, ifName); err != nil { + return fmt.Errorf("failed to rename device %q to %q: %v", tempDevName, ifName, err) } + + // restore tempDevName in case of error + defer func() { + if err != nil { + _ = netlink.LinkSetName(contDev, tempDevName) + } + }() + // Bring container device up if err = netlink.LinkSetUp(contDev); err != nil { return fmt.Errorf("failed to set %q up: %v", ifName, err) } + + // bring device down in case of error + defer func() { + if err != nil { + _ = netlink.LinkSetDown(contDev) + } + }() + // Retrieve link again to get up-to-date name and attributes contDev, err = netlink.LinkByName(ifName) if err != nil { @@ -247,11 +325,14 @@ func moveLinkOut(containerNs ns.NetNS, ifName string) error { } defer defaultNs.Close() - return containerNs.Do(func(_ ns.NetNS) error { + var tempName string + var origDev netlink.Link + err = containerNs.Do(func(_ ns.NetNS) error { dev, err := netlink.LinkByName(ifName) if err != nil { return fmt.Errorf("failed to find %q: %v", ifName, err) } + origDev = dev // Devices can be renamed only when down if err = netlink.LinkSetDown(dev); err != nil { @@ -269,16 +350,49 @@ func moveLinkOut(containerNs ns.NetNS, ifName string) error { } }() - // Rename the device to its original name from the host namespace - if err = netlink.LinkSetName(dev, dev.Attrs().Alias); err != nil { - return fmt.Errorf("failed to restore %q to original name %q: %v", ifName, dev.Attrs().Alias, err) + newLink, err := setTempName(dev) + if err != nil { + return fmt.Errorf("failed to rename device %q to temporary name: %v", ifName, err) } + dev = newLink + tempName = dev.Attrs().Name if err = netlink.LinkSetNsFd(dev, int(defaultNs.Fd())); err != nil { - return fmt.Errorf("failed to move %q to host netns: %v", dev.Attrs().Alias, err) + return fmt.Errorf("failed to move %q to host netns: %v", tempName, err) } return nil }) + + if err != nil { + return err + } + + // Rename the device to its original name from the host namespace + tempDev, err := netlink.LinkByName(tempName) + if err != nil { + return fmt.Errorf("failed to find %q in host namespace: %v", tempName, err) + } + + if err = netlink.LinkSetName(tempDev, tempDev.Attrs().Alias); err != nil { + // move device back to container ns so it may be retired + defer func() { + _ = netlink.LinkSetNsFd(tempDev, int(containerNs.Fd())) + _ = containerNs.Do(func(_ ns.NetNS) error { + lnk, err := netlink.LinkByName(tempName) + if err != nil { + return err + } + _ = netlink.LinkSetName(lnk, ifName) + if origDev.Attrs().Flags&net.FlagUp == net.FlagUp { + _ = netlink.LinkSetUp(lnk) + } + return nil + }) + }() + return fmt.Errorf("failed to restore %q to original name %q: %v", tempName, tempDev.Attrs().Alias, err) + } + + return nil } func hasDpdkDriver(pciaddr string) (bool, error) { diff --git a/plugins/main/host-device/host-device_test.go b/plugins/main/host-device/host-device_test.go index 4cf8ba598..5f1341c4b 100644 --- a/plugins/main/host-device/host-device_test.go +++ b/plugins/main/host-device/host-device_test.go @@ -1149,6 +1149,114 @@ var _ = Describe("base functionality", func() { return nil }) }) + + It(fmt.Sprintf("Test CmdAdd/Del when additioinal interface alreay exists in container ns with same name. %s config", ver), func() { + var ( + origLink netlink.Link + containerLink netlink.Link + ) + + hostIfname := "eth0" + containerAdditionalIfname := "eth0" + + // prepare host device in original namespace + _ = originalNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + err := netlink.LinkAdd(&netlink.Dummy{ + LinkAttrs: netlink.LinkAttrs{ + Name: hostIfname, + }, + }) + Expect(err).NotTo(HaveOccurred()) + origLink, err = netlink.LinkByName(hostIfname) + Expect(err).NotTo(HaveOccurred()) + err = netlink.LinkSetUp(origLink) + Expect(err).NotTo(HaveOccurred()) + return nil + }) + + // prepare device in container namespace with same name as host device + _ = targetNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + err := netlink.LinkAdd(&netlink.Dummy{ + LinkAttrs: netlink.LinkAttrs{ + Name: containerAdditionalIfname, + }, + }) + Expect(err).NotTo(HaveOccurred()) + containerLink, err = netlink.LinkByName(containerAdditionalIfname) + Expect(err).NotTo(HaveOccurred()) + err = netlink.LinkSetUp(containerLink) + Expect(err).NotTo(HaveOccurred()) + return nil + }) + + // call CmdAdd + cniName := "net1" + conf := fmt.Sprintf(`{ + "cniVersion": "%s", + "name": "cni-plugin-host-device-test", + "type": "host-device", + "device": %q + }`, ver, hostIfname) + args := &skel.CmdArgs{ + ContainerID: "dummy", + Netns: targetNS.Path(), + IfName: cniName, + StdinData: []byte(conf), + } + var resI types.Result + err := originalNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + var err error + resI, _, err = testutils.CmdAddWithArgs(args, func() error { return cmdAdd(args) }) + return err + }) + Expect(err).NotTo(HaveOccurred()) + + // check that the result was sane + t := newTesterByVersion(ver) + t.expectInterfaces(resI, cniName, origLink.Attrs().HardwareAddr.String(), targetNS.Path()) + + // assert that host device is now in the target namespace and is up + _ = targetNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + link, err := netlink.LinkByName(cniName) + Expect(err).NotTo(HaveOccurred()) + Expect(link.Attrs().HardwareAddr).To(Equal(origLink.Attrs().HardwareAddr)) + Expect(link.Attrs().Flags & net.FlagUp).To(Equal(net.FlagUp)) + return nil + }) + + // call CmdDel, expect it to succeed + _ = originalNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + err = testutils.CmdDelWithArgs(args, func() error { + return cmdDel(args) + }) + Expect(err).ToNot(HaveOccurred()) + return nil + }) + + // assert container interface "eth0" still exists in target namespace and is up + err = targetNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + link, err := netlink.LinkByName(containerAdditionalIfname) + Expect(err).NotTo(HaveOccurred()) + Expect(link.Attrs().HardwareAddr).To(Equal(containerLink.Attrs().HardwareAddr)) + Expect(link.Attrs().Flags & net.FlagUp).To(Equal(net.FlagUp)) + return nil + }) + Expect(err).NotTo(HaveOccurred()) + + // assert that host device is now back in the original namespace + _ = originalNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + _, err := netlink.LinkByName(hostIfname) + Expect(err).NotTo(HaveOccurred()) + return nil + }) + }) } })