diff --git a/pkg/pool-manager/pool_test.go b/pkg/pool-manager/pool_test.go index dc0c0ad1d..3cfeccb02 100644 --- 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 index 1a3de303c..18407f89c 100644 --- 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()