From a3a9e5668dc350644f7181120204b5e5b0aed3a9 Mon Sep 17 00:00:00 2001 From: RamLavi Date: Thu, 10 Mar 2022 07:01:40 -0500 Subject: [PATCH] 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 --- pkg/pool-manager/pool_test.go | 38 +++++++++++++++++++++++++ pkg/pool-manager/virtualmachine_pool.go | 27 ++++++++++++++++++ 2 files changed, 65 insertions(+) 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()