Fix duplicate check with different mac format (#397)

* Use removeMacEntry function instead of directly delete macPoolMap provides a save remove entry function that could always be preferred respect the directly `delete` function. Signed-off-by: fossedihelm <[email protected]> * Fix duplicate check with different mac format As per https://pkg.go.dev/net#ParseMAC, a macAddress can be specified as an IEEE 802 MAC-48, EUI-48, EUI-64, or a 20-octet IP using the following formats: - 00:00:5e:00:53:01 - 02:00:5e:10:00:00:00:01 - 00:00:00:00:fe:80:00:00:00:00:00:00:02:00:5e:10:00:00:00:01 - 00-00-5e-00-53-01 - 02-00-5e-10-00-00-00-01 - 00-00-00-00-fe-80-00-00-00-00-00-00-02-00-5e-10-00-00-00-01 - 0000.5e00.5301 - 0200.5e10.0000.0001 - 0000.0000.fe80.0000.0000.0000.0200.5e10.0000.0001 Currently, the duplicate mac check works only if the two compared mac address use the same separator. The duplicate check is made looking at the existence of key of the macMap. The keys of the mapMap are the macAddress string. With this commit, a `macKey` struct is introduced. The latter stores the normalized string of the macAddress and can be used as unique key and identifier of a macAddress. NB: Currently KubeVirt does not allow EUI-64 and 20-octet IP mac addresses. Signed-off-by: fossedihelm <[email protected]> --------- Signed-off-by: fossedihelm <[email protected]>
k8snetworkplumbingwg · Oct 31, 2023 · 69258d0 · 69258d0
1 parent 94abfea
commit 69258d0
Show file tree

Hide file tree

Showing 13 changed files with 144 additions and 86 deletions.
diff --git a/pkg/pool-manager/macentry_test.go b/pkg/pool-manager/macentry_test.go
@@ -21,33 +21,33 @@ var _ = Describe("mac-entry", func() {
 		newTimestamp := now.Add(time.Second)
 		BeforeEach(func() {
 			poolManager.waitTime = waitTimeSeconds
-			poolManager.macPoolMap = map[string]macEntry{
-				"02:00:00:00:00:00": macEntry{
+			poolManager.macPoolMap = map[macKey]macEntry{
+				NewMacKey("02:00:00:00:00:00"): macEntry{
 					instanceName:         "vm/default/vm0",
 					macInstanceKey:       "validInterface",
 					transactionTimestamp: &currentTimestamp,
 				},
-				"02:00:00:00:00:01": macEntry{
+				NewMacKey("02:00:00:00:00:01"): macEntry{
 					instanceName:         "vm/ns0/vm1",
 					macInstanceKey:       "staleInterface",
 					transactionTimestamp: &staleTimestamp,
 				},
-				"02:00:00:00:00:02": macEntry{
+				NewMacKey("02:00:00:00:00:02"): macEntry{
 					instanceName:         "vm/ns2/vm2",
 					macInstanceKey:       "validInterface",
 					transactionTimestamp: &currentTimestamp,
 				},
-				"02:00:00:00:00:03": macEntry{
+				NewMacKey("02:00:00:00:00:03"): macEntry{
 					instanceName:         "vm/ns3-4/vm3-4",
 					macInstanceKey:       "staleInterface",
 					transactionTimestamp: &staleTimestamp,
 				},
-				"02:00:00:00:00:04": macEntry{
+				NewMacKey("02:00:00:00:00:04"): macEntry{
 					instanceName:         "vm/ns3-4/vm3-4",
 					macInstanceKey:       "validInterface",
 					transactionTimestamp: &currentTimestamp,
 				},
-				"02:00:00:00:00:05": macEntry{
+				NewMacKey("02:00:00:00:00:05"): macEntry{
 					instanceName:         "vm/default/vm0",
 					macInstanceKey:       "validInterface",
 					transactionTimestamp: nil,
@@ -61,7 +61,7 @@ var _ = Describe("mac-entry", func() {
 		}
 		DescribeTable("and performing hasExpiredTransaction on macPoolMap entry",
 			func(i *hasExpiredTransactionParams) {
-				entry := poolManager.macPoolMap[i.macAddress]
+				entry := poolManager.macPoolMap[NewMacKey(i.macAddress)]
 				isStale, err := entry.hasExpiredTransaction(poolManager.waitTime)
 				Expect(err).ToNot(HaveOccurred(), "hasExpiredTransaction should not return error")
 				Expect(isStale).To(Equal(i.shouldSucceed), fmt.Sprintf("mac entry %s staleness status is not as expected", i.macAddress))
@@ -104,7 +104,7 @@ var _ = Describe("mac-entry", func() {
 		}
 		DescribeTable("and performing hasPendingTransaction on macPoolMap entry",
 			func(i *hasPendingTransactionParams) {
-				entry := poolManager.macPoolMap[i.macAddress]
+				entry := poolManager.macPoolMap[NewMacKey(i.macAddress)]
 				hasPendingTransaction := entry.hasPendingTransaction()
 				Expect(hasPendingTransaction).To(Equal(i.shouldSucceed), fmt.Sprintf("mac entry %s update required status is not as expected", i.macAddress))
 			},
@@ -147,7 +147,7 @@ var _ = Describe("mac-entry", func() {
 		}
 		DescribeTable("and performing hasReadyTransaction on macPoolMap",
 			func(i *hasReadyTransactionParams) {
-				entry := poolManager.macPoolMap[i.macAddress]
+				entry := poolManager.macPoolMap[NewMacKey(i.macAddress)]
 				isUpdated := entry.hasReadyTransaction(i.latestPersistedTimestamp)
 				Expect(isUpdated).To(Equal(i.expectedEntryReadiness), fmt.Sprintf("should get expected readiness on mac %s", i.macAddress))
 			},

diff --git a/pkg/pool-manager/mackey.go b/pkg/pool-manager/mackey.go
@@ -0,0 +1,22 @@
+package pool_manager
+
+import "net"
+
+type macKey struct {
+	normalizedMacAddress string
+}
+
+// NewMacKey creates a macKey struct containing a mac string
+// that can be used for different mac string format comparison.
+// It uses net.ParseMAC function to parse the given macAddress
+// and then stores its String() value.
+// The given macAddress MUST have already been validated.
+func NewMacKey(macAddress string) macKey {
+	// we can ignore the error here since the string value has already been validated
+	hwMacAddress, _ := net.ParseMAC(macAddress)
+	return macKey{normalizedMacAddress: hwMacAddress.String()}
+}
+
+func (m macKey) String() string {
+	return m.normalizedMacAddress
+}
diff --git a/pkg/pool-manager/mackey_test.go b/pkg/pool-manager/mackey_test.go
@@ -0,0 +1,24 @@
+package pool_manager
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("MacKey", func() {
+	DescribeTable("String should return the normalized MAC address",
+		func(input string, expected string) {
+			macKey := NewMacKey(input)
+			Expect(macKey.String()).To(Equal(expected))
+		},
+		Entry("colon-separated EUI-48 input", "00:00:5e:00:53:01", "00:00:5e:00:53:01"),
+		Entry("colon-separated EUI-64 input", "02:00:5e:10:00:00:00:01", "02:00:5e:10:00:00:00:01"),
+		Entry("colon-separated 20-octet IP input", "00:00:00:00:fe:80:00:00:00:00:00:00:02:00:5e:10:00:00:00:01", "00:00:00:00:fe:80:00:00:00:00:00:00:02:00:5e:10:00:00:00:01"),
+		Entry("hyphen-separated EUI-48 input", "00-00-5e-00-53-01", "00:00:5e:00:53:01"),
+		Entry("hyphen-separated EUI-64 input", "02-00-5e-10-00-00-00-01", "02:00:5e:10:00:00:00:01"),
+		Entry("hyphen-separated 20-octet IP input", "00-00-00-00-fe-80-00-00-00-00-00-00-02-00-5e-10-00-00-00-01", "00:00:00:00:fe:80:00:00:00:00:00:00:02:00:5e:10:00:00:00:01"),
+		Entry("period-separated EUI-48 input", "0000.5e00.5301", "00:00:5e:00:53:01"),
+		Entry("period-separated EUI-64 input", "0200.5e10.0000.0001", "02:00:5e:10:00:00:00:01"),
+		Entry("period-separated 20-octet IP input", "0000.0000.fe80.0000.0000.0000.0200.5e10.0000.0001", "00:00:00:00:fe:80:00:00:00:00:00:00:02:00:5e:10:00:00:00:01"),
+	)
+})
diff --git a/pkg/pool-manager/macpoolmap.go b/pkg/pool-manager/macpoolmap.go
@@ -13,12 +13,12 @@ import (
 func (m *macMap) clearMacTransactionFromMacEntry(macAddress string) {
 	macEntry, exist := m.findByMacAddress(macAddress)
 	if exist {
-		(*m)[macAddress] = macEntry.resetTransaction()
+		(*m)[NewMacKey(macAddress)] = macEntry.resetTransaction()
 	}
 }
 
 func (m macMap) findByMacAddress(macAddress string) (macEntry, bool) {
-	macEntry, exist := m[macAddress]
+	macEntry, exist := m[NewMacKey(macAddress)]
 	return macEntry, exist
 }
 
@@ -40,7 +40,7 @@ func (m macMap) findByMacAddressAndInstanceName(macAddress, instanceFullName str
 
 // removeMacEntry deletes a macEntry from macPoolMap
 func (m *macMap) removeMacEntry(macAddress string) {
-	delete(*m, macAddress)
+	delete(*m, NewMacKey(macAddress))
 }
 
 // filterInByInstanceName creates a subset map from macPoolMap, holding only macs that belongs to a specific instance (pod/vm)
@@ -56,7 +56,7 @@ func (m macMap) filterInByInstanceName(instanceName string) (*macMap, error) {
 }
 
 func (m *macMap) createOrUpdateEntry(macAddress, instanceFullName, macInstanceKey string) {
-	(*m)[macAddress] = macEntry{
+	(*m)[NewMacKey(macAddress)] = macEntry{
 		instanceName:         instanceFullName,
 		macInstanceKey:       macInstanceKey,
 		transactionTimestamp: nil,
@@ -70,7 +70,7 @@ func (m *macMap) updateMacTransactionTimestampForUpdatedMacs(instanceFullName st
 		if err != nil {
 			return err
 		}
-		(*m)[macAddress] = entry.setTransaction(transactionTimestamp)
+		(*m)[NewMacKey(macAddress)] = entry.setTransaction(transactionTimestamp)
 	}
 	return nil
 }
@@ -98,7 +98,7 @@ func (m *macMap) alignMacEntryAccordingToVmInterface(macAddress, instanceFullNam
 	logger := log.WithName("alignMacEntryAccordingToVmInterface")
 	macEntry, _ := m.findByMacAddress(macAddress)
 	for _, iface := range vmInterfaces {
-		if iface.MacAddress == macAddress {
+		if NewMacKey(iface.MacAddress).String() == macAddress {
 			if iface.Name == macEntry.macInstanceKey {
 				logger.Info("marked mac as allocated", "macAddress", macAddress)
 				m.clearMacTransactionFromMacEntry(macAddress)

diff --git a/pkg/pool-manager/macpoolmap_test.go b/pkg/pool-manager/macpoolmap_test.go
@@ -24,32 +24,32 @@ var _ = Describe("mac-pool-map", func() {
 		BeforeEach(func() {
 			poolManager.waitTime = waitTimeSeconds
 			poolManager.macPoolMap = macMap{
-				"02:00:00:00:00:00": macEntry{
+				NewMacKey("02:00:00:00:00:00"): macEntry{
 					instanceName:         "vm/default/vm0",
 					macInstanceKey:       "validInterface",
 					transactionTimestamp: &currentTimestamp,
 				},
-				"02:00:00:00:00:01": macEntry{
+				NewMacKey("02:00:00:00:00:01"): macEntry{
 					instanceName:         "vm/ns0/vm1",
 					macInstanceKey:       "staleInterface",
 					transactionTimestamp: &staleTimestamp,
 				},
-				"02:00:00:00:00:02": macEntry{
+				NewMacKey("02:00:00:00:00:02"): macEntry{
 					instanceName:         "vm/ns2/vm2",
 					macInstanceKey:       "validInterface",
 					transactionTimestamp: &currentTimestamp,
 				},
-				"02:00:00:00:00:03": macEntry{
+				NewMacKey("02:00:00:00:00:03"): macEntry{
 					instanceName:         "vm/ns3-4/vm3-4",
 					macInstanceKey:       "staleInterface",
 					transactionTimestamp: &staleTimestamp,
 				},
-				"02:00:00:00:00:04": macEntry{
+				NewMacKey("02:00:00:00:00:04"): macEntry{
 					instanceName:         "vm/ns3-4/vm3-4",
 					macInstanceKey:       "validInterface",
 					transactionTimestamp: &currentTimestamp,
 				},
-				"02:00:00:00:00:05": macEntry{
+				NewMacKey("02:00:00:00:00:05"): macEntry{
 					instanceName:         "vm/default/vm0",
 					macInstanceKey:       "validInterface",
 					transactionTimestamp: nil,
@@ -76,12 +76,12 @@ var _ = Describe("mac-pool-map", func() {
 				&filterInByInstanceNameParams{
 					vmName: "vm/default/vm0",
 					expectedVmMacMap: &macMap{
-						"02:00:00:00:00:00": macEntry{
+						NewMacKey("02:00:00:00:00:00"): macEntry{
 							instanceName:         "vm/default/vm0",
 							macInstanceKey:       "validInterface",
 							transactionTimestamp: &currentTimestamp,
 						},
-						"02:00:00:00:00:05": macEntry{
+						NewMacKey("02:00:00:00:00:05"): macEntry{
 							instanceName:         "vm/default/vm0",
 							macInstanceKey:       "validInterface",
 							transactionTimestamp: nil,
@@ -92,7 +92,7 @@ var _ = Describe("mac-pool-map", func() {
 				&filterInByInstanceNameParams{
 					vmName: "vm/ns0/vm1",
 					expectedVmMacMap: &macMap{
-						"02:00:00:00:00:01": macEntry{
+						NewMacKey("02:00:00:00:00:01"): macEntry{
 							instanceName:         "vm/ns0/vm1",
 							macInstanceKey:       "staleInterface",
 							transactionTimestamp: &staleTimestamp,
@@ -103,7 +103,7 @@ var _ = Describe("mac-pool-map", func() {
 				&filterInByInstanceNameParams{
 					vmName: "vm/ns2/vm2",
 					expectedVmMacMap: &macMap{
-						"02:00:00:00:00:02": macEntry{
+						NewMacKey("02:00:00:00:00:02"): macEntry{
 							instanceName:         "vm/ns2/vm2",
 							macInstanceKey:       "validInterface",
 							transactionTimestamp: &currentTimestamp,
@@ -114,12 +114,12 @@ var _ = Describe("mac-pool-map", func() {
 				&filterInByInstanceNameParams{
 					vmName: "vm/ns3-4/vm3-4",
 					expectedVmMacMap: &macMap{
-						"02:00:00:00:00:03": macEntry{
+						NewMacKey("02:00:00:00:00:03"): macEntry{
 							instanceName:         "vm/ns3-4/vm3-4",
 							macInstanceKey:       "staleInterface",
 							transactionTimestamp: &staleTimestamp,
 						},
-						"02:00:00:00:00:04": macEntry{
+						NewMacKey("02:00:00:00:00:04"): macEntry{
 							instanceName:         "vm/ns3-4/vm3-4",
 							macInstanceKey:       "validInterface",
 							transactionTimestamp: &currentTimestamp,
@@ -138,7 +138,7 @@ var _ = Describe("mac-pool-map", func() {
 		DescribeTable("and performing alignMacEntryAccordingToVmInterface on macPoolMap entry",
 			func(a *alignMacEntryAccordingToVmInterfaceParams) {
 				poolManager.macPoolMap.alignMacEntryAccordingToVmInterface(a.macAddress, a.vmName, a.vmInterfaces)
-				macEntry, exist := poolManager.macPoolMap[a.macAddress]
+				macEntry, exist := poolManager.macPoolMap[NewMacKey(a.macAddress)]
 				Expect(exist).To(Equal(a.expectedExist))
 				Expect(macEntry).To(Equal(a.expectedMacEntry), "should align mac entry according to current interface")
 			},
@@ -189,8 +189,8 @@ var _ = Describe("mac-pool-map", func() {
 					Expect(err).ToNot(HaveOccurred(), "should not fail updating macEntry")
 
 					for _, macAddress := range u.updatedInterfaceMap {
-						Expect(poolManager.macPoolMap[macAddress].transactionTimestamp).To(Equal(u.transactionTimestamp))
-						Expect(poolManager.macPoolMap[macAddress].instanceName).To(Equal(u.vmName))
+						Expect(poolManager.macPoolMap[NewMacKey(macAddress)].transactionTimestamp).To(Equal(u.transactionTimestamp))
+						Expect(poolManager.macPoolMap[NewMacKey(macAddress)].instanceName).To(Equal(u.vmName))
 					}
 				}
 			},
@@ -229,7 +229,7 @@ var _ = Describe("mac-pool-map", func() {
 		}
 		DescribeTable("and performing clearMacTransactionFromMacEntry on macPoolMap entry",
 			func(c *clearMacTransactionFromMacEntryParams) {
-				macPoolMapCopy := map[string]macEntry{}
+				macPoolMapCopy := map[macKey]macEntry{}
 				for macAddress, macEntry := range poolManager.macPoolMap {
 					macPoolMapCopy[macAddress] = macEntry
 				}
@@ -238,7 +238,7 @@ var _ = Describe("mac-pool-map", func() {
 				for macAddress, originalMacEntry := range macPoolMapCopy {
 					updatedMacEntry, exist := poolManager.macPoolMap[macAddress]
 					Expect(exist).To(BeTrue(), fmt.Sprintf("mac %s's entry should not be deleted from macPoolMap after running clearMacTransactionFromMacEntry", macAddress))
-					if macAddress == c.macAddress {
+					if macAddress.String() == c.macAddress {
 						expectedMacEntry := macEntry{
 							instanceName:         originalMacEntry.instanceName,
 							macInstanceKey:       originalMacEntry.macInstanceKey,
@@ -327,7 +327,7 @@ var _ = Describe("mac-pool-map", func() {
 
 		It("Should remove mac entry when running removeMacEntry", func() {
 			for macAddress := range poolManager.macPoolMap {
-				poolManager.macPoolMap.removeMacEntry(macAddress)
+				poolManager.macPoolMap.removeMacEntry(macAddress.String())
 				_, exist := poolManager.macPoolMap[macAddress]
 				Expect(exist).To(BeFalse(), "mac entry should be deleted by removeMacEntry")
 			}
@@ -342,7 +342,7 @@ var _ = Describe("mac-pool-map", func() {
 		DescribeTable("and adding a new mac to macPoolMap",
 			func(c *createOrUpdateInMacPoolMapParams) {
 				poolManager.macPoolMap.createOrUpdateEntry(c.macAddress, c.vmName, c.interfaceName)
-				updatedMacEntry, exist := poolManager.macPoolMap[c.macAddress]
+				updatedMacEntry, exist := poolManager.macPoolMap[NewMacKey(c.macAddress)]
 				Expect(exist).To(BeTrue(), "mac entry should exist after added/updated")
 				expectedMacEntry := macEntry{
 					instanceName:         c.vmName,
@@ -379,12 +379,12 @@ var _ = Describe("mac-pool-map", func() {
 				&filterMacsThatRequireCommitParams{
 					latestPersistedTimestamp: &timestampBeforeCurrentTimestamp,
 					expectedMacMap: macMap{
-						"02:00:00:00:00:01": macEntry{
+						NewMacKey("02:00:00:00:00:01"): macEntry{
 							instanceName:         "vm/ns0/vm1",
 							macInstanceKey:       "staleInterface",
 							transactionTimestamp: &staleTimestamp,
 						},
-						"02:00:00:00:00:03": macEntry{
+						NewMacKey("02:00:00:00:00:03"): macEntry{
 							instanceName:         "vm/ns3-4/vm3-4",
 							macInstanceKey:       "staleInterface",
 							transactionTimestamp: &staleTimestamp,
@@ -395,27 +395,27 @@ var _ = Describe("mac-pool-map", func() {
 				&filterMacsThatRequireCommitParams{
 					latestPersistedTimestamp: &currentTimestamp,
 					expectedMacMap: macMap{
-						"02:00:00:00:00:00": macEntry{
+						NewMacKey("02:00:00:00:00:00"): macEntry{
 							instanceName:         "vm/default/vm0",
 							macInstanceKey:       "validInterface",
 							transactionTimestamp: &currentTimestamp,
 						},
-						"02:00:00:00:00:01": macEntry{
+						NewMacKey("02:00:00:00:00:01"): macEntry{
 							instanceName:         "vm/ns0/vm1",
 							macInstanceKey:       "staleInterface",
 							transactionTimestamp: &staleTimestamp,
 						},
-						"02:00:00:00:00:02": macEntry{
+						NewMacKey("02:00:00:00:00:02"): macEntry{
 							instanceName:         "vm/ns2/vm2",
 							macInstanceKey:       "validInterface",
 							transactionTimestamp: &currentTimestamp,
 						},
-						"02:00:00:00:00:03": macEntry{
+						NewMacKey("02:00:00:00:00:03"): macEntry{
 							instanceName:         "vm/ns3-4/vm3-4",
 							macInstanceKey:       "staleInterface",
 							transactionTimestamp: &staleTimestamp,
 						},
-						"02:00:00:00:00:04": macEntry{
+						NewMacKey("02:00:00:00:00:04"): macEntry{
 							instanceName:         "vm/ns3-4/vm3-4",
 							macInstanceKey:       "validInterface",
 							transactionTimestamp: &currentTimestamp,
@@ -426,27 +426,27 @@ var _ = Describe("mac-pool-map", func() {
 				&filterMacsThatRequireCommitParams{
 					latestPersistedTimestamp: &newTimestamp,
 					expectedMacMap: macMap{
-						"02:00:00:00:00:00": macEntry{
+						NewMacKey("02:00:00:00:00:00"): macEntry{
 							instanceName:         "vm/default/vm0",
 							macInstanceKey:       "validInterface",
 							transactionTimestamp: &currentTimestamp,
 						},
-						"02:00:00:00:00:01": macEntry{
+						NewMacKey("02:00:00:00:00:01"): macEntry{
 							instanceName:         "vm/ns0/vm1",
 							macInstanceKey:       "staleInterface",
 							transactionTimestamp: &staleTimestamp,
 						},
-						"02:00:00:00:00:02": macEntry{
+						NewMacKey("02:00:00:00:00:02"): macEntry{
 							instanceName:         "vm/ns2/vm2",
 							macInstanceKey:       "validInterface",
 							transactionTimestamp: &currentTimestamp,
 						},
-						"02:00:00:00:00:03": macEntry{
+						NewMacKey("02:00:00:00:00:03"): macEntry{
 							instanceName:         "vm/ns3-4/vm3-4",
 							macInstanceKey:       "staleInterface",
 							transactionTimestamp: &staleTimestamp,
 						},
-						"02:00:00:00:00:04": macEntry{
+						NewMacKey("02:00:00:00:00:04"): macEntry{
 							instanceName:         "vm/ns3-4/vm3-4",
 							macInstanceKey:       "validInterface",
 							transactionTimestamp: &currentTimestamp,