Avoid handling mac duplication

The bond-cni should not attempt to modify the mac addresses
of slaves during both, creation and deletion of the bond
interface. During deletion, 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 returns an error if this is
the case 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 {
@@ -71,6 +72,10 @@ func loadConfigFile(bytes []byte) (*bondingConfig, string, error) {
 		return nil, "", fmt.Errorf("allSlavesActive should be 0 or 1, actual: %+v", *bondConf.AllSlavesActive)
 	}
 
+	if bondConf.FailOverMac < 0 || bondConf.FailOverMac > 2 {
+		return nil, "", fmt.Errorf("FailOverMac mode should be 0, 1 or 2 actual: %+v", bondConf.FailOverMac)
+	}
+
 	return bondConf, bondConf.CNIVersion, nil
 }
 
@@ -144,10 +149,7 @@ 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)
-	}
+	var err error
 
 	bondLinkIndex := bondLinkObj.Index
 	for _, linkObject := range linkObjectsToBond {
@@ -275,14 +277,11 @@ func createBond(bondName string, bondConf *bondingConfig, nspath string, ns ns.N
 		return nil, fmt.Errorf("failed to retrieve link objects from configuration file (%+v), error: %+v", bondConf, err)
 	}
 
-	err = util.ValidateMTU(linkObjectsToBond, bondConf.MTU)
+	err = util.ValidateSlaveLinks(linkObjectsToBond, bondConf.MTU, bondConf.Mode)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to validate slave links (%+v), error: %w", linkObjectsToBond, err)
 	}
 
-	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)
@@ -433,19 +432,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

@@ -15,7 +15,9 @@
 package main
 
 import (
+	"errors"
 	"fmt"
+	"github.com/intel/bond-cni/bond/util"
 	"strconv"
 
 	"github.com/containernetworking/cni/pkg/skel"
@@ -198,7 +200,7 @@ var _ = Describe("bond plugin", func() {
 			Entry("When Version is 0.1.0", "0.1.0"),
 		)
 
-		It("verifies the plugin copes with duplicated macs in balance-tlb mode", func() {
+		It("verifies the plugin fails with duplicated macs in balance-tlb mode", func() {
 			args.StdinData = []byte(fmt.Sprintf(config, "0.3.1", BalanceTlbMode, 1, strconv.FormatBool(linksInContainer), strconv.Itoa(DefaultMTU)))
 
 			err := podNS.Do(func(ns.NetNS) error {
@@ -217,66 +219,13 @@ var _ = Describe("bond plugin", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			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)
-
-			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 {
+			_, _, 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())
+			Expect(err).To(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())
+			var dupMacErr *util.ErrorDuplicateMac
+			Expect(errors.As(err, &dupMacErr)).To(BeTrue())
 		})
 
 		It("verifies that mac addresses are restored correctly in active-backup with fail_over_mac 0", func() {

bond/util/validation.go

@@ -2,8 +2,8 @@ package util
 
 import (
 	"bytes"
-	"crypto/rand"
 	"fmt"
+
 	"github.com/vishvananda/netlink"
 )
 
@@ -13,6 +13,31 @@ const (
 	minMtuIpv4Packet    = 68
 )
 
+type ErrorDuplicateMac struct {
+	Mac string
+}
+
+func (e *ErrorDuplicateMac) Error() string {
+	return fmt.Sprintf("duplicate MAC address found: %s", e.Mac)
+}
+
+func ValidateSlaveLinks(slaveLinks []netlink.Link, mtu int, mode string) error {
+	err := ValidateMTU(slaveLinks, mtu)
+	if err != nil {
+		return fmt.Errorf("failed to validate MTU, error: %w", err)
+	}
+
+	bondMode := netlink.StringToBondMode(mode)
+	if bondMode == netlink.BOND_MODE_BALANCE_TLB || bondMode == netlink.BOND_MODE_BALANCE_ALB {
+		err = ValidateMacDuplicates(slaveLinks)
+		if err != nil {
+			return fmt.Errorf("failed to validate mac address uniqueness, bond mode: %s, error: %w", mode, err)
+		}
+	}
+
+	return nil
+}
+
 func ValidateMTU(slaveLinks []netlink.Link, mtu int) error {
 	// if not specified set MTU to default
 	if mtu == 0 {
@@ -57,16 +82,12 @@ func ValidateMTU(slaveLinks []netlink.Link, mtu int) error {
 	return nil
 }
 
-func HandleMacDuplicates(linkObjectsToBond []netlink.Link, netNsHandle *netlink.Handle) error {
+func ValidateMacDuplicates(linkObjectsToBond []netlink.Link) error {
 	macsInUse := []string{}
-	var err error
 	for _, link := range linkObjectsToBond {
 		linkMac := link.Attrs().HardwareAddr.String()
 		if isMacDuplicated(linkMac, macsInUse) {
-			linkMac, err = updateDuplicateMac(link, netNsHandle, macsInUse)
-			if err != nil {
-				return err
-			}
+			return &ErrorDuplicateMac{Mac: linkMac}
 		}
 		macsInUse = append(macsInUse, linkMac)
 	}
@@ -81,36 +102,3 @@ func isMacDuplicated(mac string, macsInUse []string) bool {
 	}
 	return false
 }
-
-func updateDuplicateMac(link netlink.Link, netNsHandle *netlink.Handle, macsInUse []string) (string, error) {
-	newMac, err := generateUnusedMac(macsInUse)
-	if err != nil {
-		return "", err
-	}
-	err = netNsHandle.LinkSetHardwareAddr(link, []byte(newMac))
-	if err != nil {
-		return "newMac", nil
-	}
-	return newMac, nil
-}
-
-func generateUnusedMac(macsInUse []string) (string, error) {
-	var newMac string
-	var err error
-	for duplicated := true; duplicated; duplicated = isMacDuplicated(newMac, macsInUse) {
-		newMac, err = randomMac()
-		if err != nil {
-			return "", err
-		}
-	}
-	return newMac, nil
-}
-
-func randomMac() (string, error) {
-	buf := make([]byte, 5)
-	_, err := rand.Read(buf)
-	if err != nil {
-		return "", err
-	}
-	return fmt.Sprintf("%02x:%02x:%02x:%02x:%02x:%02x\n", byte(2), buf[0], buf[1], buf[2], buf[3], buf[4]), nil
-}