Avoid handling mac duplication during deletion

The bond-cni should not attempt to modify the mac addresses
of slaves during deletion of the bond interface.
The bond driver takes care of reverting any modifications.

Only balance-tlb and balance-alb do not support slaves with
same mac addresses. The plugin now handles mac duplicates
only for these modes.

Signed-off-by: Marcelo Guerrero <[email protected]>

Author: mlguerrero12

bond/bond.go

@@ -29,9 +29,10 @@ import (
 	"github.com/containernetworking/cni/pkg/version"
 	"github.com/containernetworking/plugins/pkg/ipam"
 	"github.com/containernetworking/plugins/pkg/ns"
-	"github.com/intel/bond-cni/bond/util"
 	"github.com/vishvananda/netlink"
 	"github.com/vishvananda/netns"
+
+	"github.com/intel/bond-cni/bond/util"
 )
 
 type bondingConfig struct {
@@ -143,10 +144,15 @@ func createBondedLink(bondName string, bondConf *bondingConfig, netNsHandle *net
 
 // loop over the linkObjectsToBond, set each DOWN, update the interface MASTER & set it UP again.
 // again we use the netNsHandle to interfact with these links in the namespace provided. return error
-func attachLinksToBond(bondLinkObj *netlink.Bond, linkObjectsToBond []netlink.Link, netNsHandle *netlink.Handle) error {
-	err := util.HandleMacDuplicates(linkObjectsToBond, netNsHandle)
-	if err != nil {
-		return fmt.Errorf("failed to handle duplicated macs on link slaves, error: %+v", err)
+func attachLinksToBond(bondLinkObj *netlink.Bond, linkObjectsToBond []netlink.Link, mode string, netNsHandle *netlink.Handle) error {
+	var err error
+
+	bondMode := netlink.StringToBondMode(mode)
+	if bondMode == netlink.BOND_MODE_BALANCE_TLB || bondMode == netlink.BOND_MODE_BALANCE_ALB {
+		err = util.HandleMacDuplicates(linkObjectsToBond, netNsHandle)
+		if err != nil {
+			return fmt.Errorf("failed to handle duplicated macs on link slaves, error: %+v", err)
+		}
 	}
 
 	bondLinkIndex := bondLinkObj.Index
@@ -283,12 +289,13 @@ func createBond(bondName string, bondConf *bondingConfig, nspath string, ns ns.N
 	if bondConf.FailOverMac < 0 || bondConf.FailOverMac > 2 {
 		return nil, fmt.Errorf("FailOverMac mode should be 0, 1 or 2 actual: %+v", bondConf.FailOverMac)
 	}
+
 	bondLinkObj, err := createBondedLink(bondName, bondConf, netNsHandle)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create bonded link (%+v), error: %+v", bondName, err)
 	}
 
-	err = attachLinksToBond(bondLinkObj, linkObjectsToBond, netNsHandle)
+	err = attachLinksToBond(bondLinkObj, linkObjectsToBond, bondConf.Mode, netNsHandle)
 	if err != nil {
 		return nil, fmt.Errorf("failed to attached links to bond, error: %+v", err)
 	}
@@ -433,19 +440,6 @@ func cmdDel(args *skel.CmdArgs) error {
 		return fmt.Errorf("failed to deattached links from bond, error: %+v", err)
 	}
 
-	// Fetch slave links again to have the latest state
-	// For instance, Active-backup mode with fail_over_mac=0 reverts the mac address of backup slaves
-	for i := range linkObjectsToDeattach {
-		linkObjectsToDeattach[i], err = netNsHandle.LinkByName(linkObjectsToDeattach[i].Attrs().Name)
-		if err != nil {
-			return fmt.Errorf("failed to find link (%+v), error: %+v", linkObjectsToDeattach[i].Attrs().Name, err)
-		}
-	}
-
-	if err = util.HandleMacDuplicates(linkObjectsToDeattach, netNsHandle); err != nil {
-		return fmt.Errorf("failed to validate deattached links macs, error: %+v", err)
-	}
-
 	err = netNsHandle.LinkDel(linkObjToDel)
 	if err != nil {
 		return fmt.Errorf("failed to delete bonded link (%+v), error: %+v", linkObjToDel.Attrs().Name, err)

bond/bond_test.go

@@ -228,57 +228,6 @@ var _ = Describe("bond plugin", func() {
 			Expect(err).NotTo(HaveOccurred())
 		})
 
-		It("verifies the plugin handles duplicated macs on delete", func() {
-			var slave1, slave2 netlink.Link
-			var err error
-
-			err = podNS.Do(func(ns.NetNS) error {
-				defer GinkgoRecover()
-				slave1, err = netlink.LinkByName(Slave1)
-				Expect(err).NotTo(HaveOccurred())
-
-				slave2, err = netlink.LinkByName(Slave2)
-				Expect(err).NotTo(HaveOccurred())
-
-				err = netlink.LinkSetHardwareAddr(slave2, slave1.Attrs().HardwareAddr)
-				Expect(err).NotTo(HaveOccurred())
-				return nil
-			})
-
-			By("creating the plugin")
-			r, _, err := testutils.CmdAddWithArgs(args, func() error {
-				return cmdAdd(args)
-			})
-			Expect(err).NotTo(HaveOccurred())
-
-			By("checking the bond was created")
-			checkAddReturnResult(&r, IfName)
-
-			err = podNS.Do(func(ns.NetNS) error {
-				defer GinkgoRecover()
-				By("duplicating the macs on the slaves")
-				err = netlink.LinkSetHardwareAddr(slave2, slave1.Attrs().HardwareAddr)
-				Expect(err).NotTo(HaveOccurred())
-				return nil
-			})
-			By("deleting the plugin")
-			err = testutils.CmdDel(podNS.Path(),
-				args.ContainerID, "", func() error { return cmdDel(args) })
-			Expect(err).NotTo(HaveOccurred())
-
-			err = podNS.Do(func(ns.NetNS) error {
-				defer GinkgoRecover()
-				By("validating the macs are not duplicated")
-				slave1, err = netlink.LinkByName(Slave1)
-				Expect(err).NotTo(HaveOccurred())
-				slave2, err = netlink.LinkByName(Slave2)
-				Expect(err).NotTo(HaveOccurred())
-				Expect(slave1.Attrs().HardwareAddr.String()).NotTo(Equal(slave2.Attrs().HardwareAddr.String()))
-				return nil
-			})
-			Expect(err).NotTo(HaveOccurred())
-		})
-
 		It("verifies that mac addresses are restored correctly in active-backup with fail_over_mac 0", func() {
 			var bond netlink.Link
 			var slave1 netlink.Link

bond/util/validation.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"crypto/rand"
 	"fmt"
+
 	"github.com/vishvananda/netlink"
 )