From 61c070eb241ec07f6c92f248bf4079de7e1f0441 Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Fri, 5 Jan 2024 14:16:10 +0100 Subject: [PATCH] Avoid creating duplicate MACs In the NIC add and edit dialogs, check if there is an already existing MAC with the same value. libvirt itself does not check that, and while it is technically possible to do that, it will wreak havoc in the guest system (which we cannot influence) and also in Cockpit (which we could fix, but it's hard to do so, as we currently use the MAC address as identifier everywhere). https://issues.redhat.com/browse/RHEL-4064 --- src/components/vm/nics/nicAdd.jsx | 8 ++++- src/components/vm/nics/nicEdit.jsx | 7 ++++ test/check-machines-nics | 52 ++++++++++++++++++++++++++---- 3 files changed, 60 insertions(+), 7 deletions(-) diff --git a/src/components/vm/nics/nicAdd.jsx b/src/components/vm/nics/nicAdd.jsx index f6d78b03f..3183bf1f6 100644 --- a/src/components/vm/nics/nicAdd.jsx +++ b/src/components/vm/nics/nicAdd.jsx @@ -36,7 +36,7 @@ import './nic.css'; const _ = cockpit.gettext; function getRandomMac(vms) { - // prevent getting cycled in inforseen case where all MACs will conflict with existing ones + // prevent getting cycled in the unforseen case where all MACs will conflict with existing ones for (let i = 0; i < 42; i++) { const parts = ["52", "54", "00"]; for (let j = 0; j < 3; j++) @@ -147,6 +147,12 @@ export class AddNIC extends React.Component { const Dialogs = this.context; const { vm, vms } = this.props; + // disallow duplicate MACs + if (this.state.setNetworkMac && vm.interfaces.some(iface => iface.mac === this.state.networkMac)) { + this.dialogErrorSet(_("MAC address already in use"), _("Please choose a different MAC address")); + return; + } + this.setState({ addVNicInProgress: true }); domainAttachIface({ connectionName: vm.connectionName, diff --git a/src/components/vm/nics/nicEdit.jsx b/src/components/vm/nics/nicEdit.jsx index c69dbef3c..9b58f0c7a 100644 --- a/src/components/vm/nics/nicEdit.jsx +++ b/src/components/vm/nics/nicEdit.jsx @@ -123,6 +123,13 @@ export class EditNICModal extends React.Component { const Dialogs = this.context; const { vm, network } = this.props; + // disallow duplicate MACs + if (this.state.networkMac != this.props.network.mac && + vm.interfaces.some(iface => iface.mac === this.state.networkMac)) { + this.dialogErrorSet(_("MAC address already in use"), _("Please choose a different MAC address")); + return; + } + domainChangeInterfaceSettings({ vmName: vm.name, connectionName: vm.connectionName, diff --git a/test/check-machines-nics b/test/check-machines-nics index e3afa52f8..5b424ccf9 100755 --- a/test/check-machines-nics +++ b/test/check-machines-nics @@ -314,8 +314,22 @@ class TestMachinesNICs(machineslib.VirtualMachinesCase): self, mac=mac_address, model="e1000e", + remove=False, ).execute() + # avoid duplicate MAC + self.NICAddDialog( + self, + source_type="network", + source="test_network", + mac=mac_address, + xfail=True, + xfail_error="MAC address already in use", + ).execute() + + # remove the first one from "Test model" again, it fails to start + self.deleteIface(1) + # Start vm and wait until kernel is booted m.execute(f"> {args['logfile']}") # clear logfile b.click("#vm-subVmTest1-system-run") @@ -376,6 +390,7 @@ class TestMachinesNICs(machineslib.VirtualMachinesCase): nic_num=1, source=None, source_type=None, + xfail_error=None, ): self.assertEqual = test_obj.assertEqual self.browser = test_obj.browser @@ -385,14 +400,19 @@ class TestMachinesNICs(machineslib.VirtualMachinesCase): self.nic_num = nic_num self.source = source self.source_type = source_type + self.xfail_error = xfail_error self.vm_state = "running" if "running" in test_obj.machine.execute("virsh domstate subVmTest1") else "shut off" def execute(self): self.open() self.fill() self.save() - self.verify() - self.verify_overview() + if not self.xfail_error: + self.verify() + self.verify_overview() + else: + self.browser.wait_in_text(".pf-v5-c-modal-box__body .pf-v5-c-alert__title", self.xfail_error) + self.browser.click(".pf-v5-c-modal-box__footer button:contains(Cancel)") def open(self): b = self.browser @@ -434,7 +454,8 @@ class TestMachinesNICs(machineslib.VirtualMachinesCase): b = self.browser b.click(f"#vm-subVmTest1-network-{self.nic_num}-edit-dialog-save") - b.wait_not_present(f"#vm-subVmTest1-network-{self.nic_num}-edit-dialog-modal-window") + if not self.xfail_error: + b.wait_not_present(f"#vm-subVmTest1-network-{self.nic_num}-edit-dialog-modal-window") def cancel(self): b = self.browser @@ -541,20 +562,39 @@ class TestMachinesNICs(machineslib.VirtualMachinesCase): # Remove default vNIC self.deleteIface(1) - # Test Warning message when changes are done in a running VM + MAC1 = "52:54:00:f0:eb:63" + self.NICAddDialog( self, source_type="bridge", source="virbr0", - mac="52:54:00:f0:eb:63", + mac=MAC1, + remove=False + ).execute() + + # create a second NIC, and check that trying to edit to existing MAC is prohibited + self.NICAddDialog( + self, + source_type="bridge", + source="virbr0", + nic_num=2, + mac="52:54:00:f0:eb:64", remove=False ).execute() + self.NICEditDialog( + self, + nic_num=2, + mac=MAC1, + source_type="network", + xfail_error="MAC address already in use", + ).execute() + self.deleteIface(2) # The dialog fields should reflect the permanent configuration dialog = self.NICEditDialog(self) dialog.open() b.wait_in_text("#vm-subVmTest1-network-1-edit-dialog-source", "virbr0") - b.wait_val("#vm-subVmTest1-network-1-edit-dialog-mac", "52:54:00:f0:eb:63") + b.wait_val("#vm-subVmTest1-network-1-edit-dialog-mac", MAC1) b.assert_pixels("#vm-subVmTest1-network-1-edit-dialog-modal-window", "vm-edit-nic-modal", skip_layouts=["rtl"]) dialog.cancel()