vm-pool, Reject VMs with duplicate interfaces (#358)

When applying a VM with multiple interfaces with the same name, the VM is rejected but kubemacpool is mishandling the MACs, causing a side effect where that MAC is no longer usable (it is "taken" by the ghost VM). This commit fixes this by checking and rejecting VMs with duplicate interface names on kmp webhook context. Signed-off-by: Ram Lavi <[email protected]>
k8snetworkplumbingwg · Mar 10, 2022 · a3a9e56 · a3a9e56
1 parent 3e9fff7
commit a3a9e56
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 0 deletions.
diff --git a/pkg/pool-manager/pool_test.go b/pkg/pool-manager/pool_test.go
@@ -348,9 +348,26 @@ var _ = Describe("Pool", func() {
 						Devices: kubevirt.Devices{
 							Interfaces: []kubevirt.Interface{masqueradeInterface, multusBridgeInterface}}},
 					Networks: []kubevirt.Network{podNetwork, multusNetwork}}}}}
+		duplicateInterfacesVM := kubevirt.VirtualMachine{ObjectMeta: metav1.ObjectMeta{Namespace: "default"}, Spec: kubevirt.VirtualMachineSpec{
+			Template: &kubevirt.VirtualMachineInstanceTemplateSpec{
+				Spec: kubevirt.VirtualMachineInstanceSpec{
+					Domain: kubevirt.DomainSpec{
+						Devices: kubevirt.Devices{
+							Interfaces: []kubevirt.Interface{masqueradeInterface, multusBridgeInterface, multusBridgeInterface}}},
+					Networks: []kubevirt.Network{podNetwork, multusNetwork}}}}}
 		updateTransactionTimestamp := func(secondsPassed time.Duration) time.Time {
 			return time.Now().Add(secondsPassed * time.Second)
 		}
+		It("should reject allocation if there are interfaces with the same name", func() {
+			poolManager := createPoolManager("02:00:00:00:00:00", "02:00:00:00:00:02")
+			newVM := duplicateInterfacesVM.DeepCopy()
+			newVM.Name = "duplicateInterfacesVM"
+
+			transactionTimestamp := updateTransactionTimestamp(0)
+			err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, true, logger)
+			Expect(err).To(HaveOccurred(), "Should reject an allocation of a vm with duplicate interface names")
+			Expect(poolManager.macPoolMap).To(BeEmpty(), "Should not allocate macs if there are duplicate interfaces")
+		})
 		It("should not allocate a new mac for bridge interface on pod network", func() {
 			poolManager := createPoolManager("02:00:00:00:00:00", "02:00:00:00:00:02")
 			newVM := sampleVM
@@ -425,6 +442,27 @@ var _ = Describe("Pool", func() {
 				Expect(err).ToNot(HaveOccurred())
 				Expect(updateVm.Spec.Template.Spec.Domain.Devices.Disks[0].IO).To(Equal(kubevirt.DriverIO("native-update")), "disk.io configuration must be preserved after mac allocation update")
 			})
+			It("should reject update allocation if there are interfaces with the same name", func() {
+				poolManager := createPoolManager("02:00:00:00:00:00", "02:00:00:00:00:02")
+				newVM := multipleInterfacesVM.DeepCopy()
+				newVM.Name = "multipleInterfacesVM"
+
+				transactionTimestamp := updateTransactionTimestamp(0)
+				err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, true, logger)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(newVM.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress).To(Equal("02:00:00:00:00:00"))
+				Expect(newVM.Spec.Template.Spec.Domain.Devices.Interfaces[1].MacAddress).To(Equal("02:00:00:00:00:01"))
+				Expect(checkMacPoolMapEntries(poolManager.macPoolMap, &transactionTimestamp, []string{"02:00:00:00:00:00", "02:00:00:00:00:01"}, []string{})).To(Succeed(), "Failed to check macs in macMap")
+
+				updateVm := newVM.DeepCopy()
+				newTransactionTimestamp := updateTransactionTimestamp(1)
+				updateVm.Spec.Template.Spec.Domain.Devices.Interfaces = append(updateVm.Spec.Template.Spec.Domain.Devices.Interfaces, multusBridgeInterface)
+				err = poolManager.UpdateMacAddressesForVirtualMachine(newVM, updateVm, &newTransactionTimestamp, true, logger)
+				Expect(err).To(HaveOccurred(), "Should reject an update with duplicate interface names")
+				Expect(updateVm.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress).To(Equal("02:00:00:00:00:00"))
+				Expect(updateVm.Spec.Template.Spec.Domain.Devices.Interfaces[1].MacAddress).To(Equal("02:00:00:00:00:01"))
+				Expect(checkMacPoolMapEntries(poolManager.macPoolMap, &transactionTimestamp, []string{"02:00:00:00:00:00", "02:00:00:00:00:01"}, []string{})).To(Succeed(), "Failed to check macs in macMap")
+			})
 			It("should preserve mac addresses on update", func() {
 				poolManager := createPoolManager("02:00:00:00:00:00", "02:00:00:00:00:02")
 				newVM := multipleInterfacesVM.DeepCopy()

diff --git a/pkg/pool-manager/virtualmachine_pool.go b/pkg/pool-manager/virtualmachine_pool.go
@@ -47,6 +47,13 @@ func (p *PoolManager) AllocateVirtualMachineMac(virtualMachine *kubevirt.Virtual
 		return nil
 	}
 
+	// We can't allow for duplicate interfaces names, as interface.Name is macMap's key.
+	if isNotDryRun {
+		if err := checkVmForInterfaceDuplication(virtualMachine); err != nil {
+			return err
+		}
+	}
+
 	vmFullName := VmNamespaced(virtualMachine)
 	if len(getVirtualMachineNetworks(virtualMachine)) == 0 {
 		logger.Info("no networks found for virtual machine, skipping mac allocation",
@@ -126,6 +133,13 @@ func (p *PoolManager) UpdateMacAddressesForVirtualMachine(previousVirtualMachine
 	}
 	defer p.poolMutex.Unlock()
 
+	// We can't allow for duplicate interfaces names, as interface.Name is macMap's key.
+	if isNotDryRun {
+		if err := checkVmForInterfaceDuplication(virtualMachine); err != nil {
+			return err
+		}
+	}
+
 	currentInterfaces := getVirtualMachineInterfaces(previousVirtualMachine)
 	requestInterfaces := getVirtualMachineInterfaces(virtualMachine)
 	logger.V(1).Info("data before update", "macPoolMap", p.macPoolMap, "currentInterfaces", currentInterfaces, "requestInterfaces", requestInterfaces)
@@ -199,6 +213,19 @@ func getVirtualMachineNetworks(virtualMachine *kubevirt.VirtualMachine) []kubevi
 	return virtualMachine.Spec.Template.Spec.Networks
 }
 
+func checkVmForInterfaceDuplication(virtualMachine *kubevirt.VirtualMachine) error {
+	vmInterfaces := getVirtualMachineInterfaces(virtualMachine)
+
+	tmpInterfaceMap := map[string]struct{}{}
+	for _, vmInterface := range vmInterfaces {
+		if _, exists := tmpInterfaceMap[vmInterface.Name]; exists {
+			return fmt.Errorf("Failed to mutate virtual machine: every interface must have a unique name")
+		}
+		tmpInterfaceMap[vmInterface.Name] = struct{}{}
+	}
+	return nil
+}
+
 func (p *PoolManager) allocateFromPoolForVirtualMachine(vmFullName string, iface kubevirt.Interface, isNotDryRun bool, parentLogger logr.Logger) (string, error) {
 	logger := parentLogger.WithName("allocateFromPoolForVirtualMachine")
 	macAddr, err := p.getFreeMac()