From a81187ae29dee432450c1e70f97ab9d37249dcb9 Mon Sep 17 00:00:00 2001 From: Prashanth Reddy Date: Wed, 28 Oct 2026 15:10:23 +0000 Subject: [PATCH 1/3] kvm: program system VMs into the security group bridge firewall framework When a shared security group network and the public network use the same vlan on the same physical device they resolve to the same bridge. The first security grouped instance on a host installs the bridge firewall framework there, including the catch-all FORWARD DROP rules, but the SSVM and CPVM on that same bridge are never programmed into it and lose all new inbound flows. handleVmMigrated() returned before reaching the system VM branch, since isVmSecurityGroupEnabled() inspects the VM's own nics and is only meaningful for user VMs, which made NetworkRulesSystemVmCommand dead code. applyDefaultNetworkRules() likewise skips system VMs in an advanced zone, as none of their nics is security group enabled. System VM rules are now applied on migration and on start, and the system VMs already attached to a bridge are programmed when the framework is created on it. None of the new call sites creates a bridge firewall framework where one does not already exist. Fixes: #14518 --- .../resource/LibvirtComputingResource.java | 16 ++++- ...irtNetworkRulesSystemVmCommandWrapper.java | 4 +- scripts/vm/network/security_group.py | 38 +++++++++++- .../security/SecurityGroupManagerImpl.java | 6 +- ...SecurityGroupManagerImplMigrationTest.java | 58 +++++++++++++++++++ 5 files changed, 115 insertions(+), 7 deletions(-) create mode 100644 server/src/test/java/com/cloud/network/security/SecurityGroupManagerImplMigrationTest.java diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index 1f90a6257557..066a275cd8f5 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -5484,7 +5484,7 @@ public boolean applyDefaultNetworkRules(final Connect conn, final VirtualMachine if (nic.isSecurityGroupEnabled() || nic.getIsolationUri() != null && nic.getIsolationUri().getScheme().equalsIgnoreCase(IsolationType.Ec2.toString())) { if (vm.getType() != VirtualMachine.Type.User) { configureDefaultNetworkRulesForSystemVm(conn, vm.getName()); - break; + return true; } if (!applyDefaultNetworkRulesOnNic(conn, vm.getName(), vm.getId(), nic, isFirstNic, checkBeforeApply)) { LOGGER.error("Unable to apply default network rule for nic " + nic.getName() + " for VM " + vm.getName()); @@ -5493,6 +5493,13 @@ public boolean applyDefaultNetworkRules(final Connect conn, final VirtualMachine isFirstNic = false; } } + if (vm.getType() != VirtualMachine.Type.User) { + // none of the nics is security group enabled, but the system vm may still share a bridge + // with security grouped instances, for instance when the shared network and the public + // network use the same vlan. Program it into an existing bridge firewall framework, + // without creating one. + configureDefaultNetworkRulesForSystemVm(conn, vm.getName(), true); + } return true; } @@ -5598,6 +5605,10 @@ protected boolean post_default_network_rules(final Connect conn, final String vm } public boolean configureDefaultNetworkRulesForSystemVm(final Connect conn, final String vmName) { + return configureDefaultNetworkRulesForSystemVm(conn, vmName, false); + } + + public boolean configureDefaultNetworkRulesForSystemVm(final Connect conn, final String vmName, final boolean onlyExistingFramework) { if (!canBridgeFirewall) { return false; } @@ -5606,6 +5617,9 @@ public boolean configureDefaultNetworkRulesForSystemVm(final Connect conn, final cmd.add("default_network_rules_systemvm"); cmd.add("--vmname", vmName); cmd.add("--localbrname", linkLocalBridgeName); + if (onlyExistingFramework) { + cmd.add("--onlyexistingframework"); + } final String result = cmd.execute(); if (result != null) { return false; diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtNetworkRulesSystemVmCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtNetworkRulesSystemVmCommandWrapper.java index b5ee13920a68..b56c136f3845 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtNetworkRulesSystemVmCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtNetworkRulesSystemVmCommandWrapper.java @@ -39,7 +39,9 @@ public Answer execute(final NetworkRulesSystemVmCommand command, final LibvirtCo final LibvirtUtilitiesHelper libvirtUtilitiesHelper = libvirtComputingResource.getLibvirtUtilitiesHelper(); final Connect conn = libvirtUtilitiesHelper.getConnectionByVmName(command.getVmName()); - success = libvirtComputingResource.configureDefaultNetworkRulesForSystemVm(conn, command.getVmName()); + // this command is only sent after a migration; the rules are re-applied on the + // destination host, but the bridge firewall framework must not be created there + success = libvirtComputingResource.configureDefaultNetworkRulesForSystemVm(conn, command.getVmName(), true); } catch (final LibvirtException e) { logger.trace("Ignoring libvirt error.", e); } diff --git a/scripts/vm/network/security_group.py b/scripts/vm/network/security_group.py index 8e3e779a88b3..2e27236f7d49 100755 --- a/scripts/vm/network/security_group.py +++ b/scripts/vm/network/security_group.py @@ -371,8 +371,15 @@ def refactor_ebtable_rules(vm_name): logging.debug("Refactoring ebtables rules for vm " + vm_name + " is done") return True -def default_network_rules_systemvm(vm_name, localbrname): +def default_network_rules_systemvm(vm_name, localbrname, only_existing_framework=False): bridges = get_bridges(vm_name) + if only_existing_framework: + # do not create the bridge firewall framework, only program the vm into the bridges + # which already have one + bridges = [bridge for bridge in bridges if bridge != localbrname and has_fw_framework(bridge)] + if not bridges: + logging.debug("No bridge firewall framework on the bridges of " + vm_name + ", nothing to program") + return True domid = get_vm_id(vm_name) vmchain = iptables_chain_name(vm_name) @@ -1277,6 +1284,31 @@ def get_br_fw(brname): return brfwname +def has_fw_framework(brname): + try: + execute("iptables -n -L " + get_br_fw(brname)) + except: + return False + return True + + +def add_systemvm_rules_for_bridge(brname): + # The bridge firewall framework has just been created on this bridge. Any system vm + # (router, secondary storage vm, console proxy) already attached to it has no rules in + # the framework and would be dropped, so program them now. + try: + for vm_name in virshlist(['running', 'paused']): + if 1 not in [ vm_name.startswith(c) for c in ['r-', 's-', 'v-'] ]: + continue + if not get_vifs_for_bridge(vm_name, brname): + continue + logging.debug("Programming default rules on bridge " + brname + " for system vm " + vm_name) + # the link local bridge never has a bridge firewall framework, so it is skipped anyway + default_network_rules_systemvm(vm_name, None, True) + except: + logging.debug("Failed to program default rules for the system vms on bridge " + brname) + + def add_fw_framework(brname): try: execute("modprobe br_netfilter") @@ -1342,6 +1374,7 @@ def add_fw_framework(brname): execute("iptables -A " + brfw + " -m physdev --physdev-is-bridged --physdev-is-in -j " + brfwin) execute("iptables -A " + brfw + " -m physdev --physdev-is-bridged --physdev-is-out -j " + brfwout) execute("iptables -A " + brfw + " -m physdev --physdev-is-bridged --physdev-out " + physdev + " -j ACCEPT") + add_systemvm_rules_for_bridge(brname) if refs6 == 0: execute('ip6tables -I FORWARD -i ' + brname + ' -j DROP') @@ -1616,6 +1649,7 @@ def verify_expected_rules_in_order(expected_rules, rules): parser.add_argument("--privnic", dest="privnic") parser.add_argument("--isFirstNic", action="store_true", dest="isFirstNic") parser.add_argument("--check", action="store_true", dest="check") + parser.add_argument("--onlyexistingframework", action="store_true", dest="onlyExistingFramework") args = parser.parse_args() cmd = args.command logging.debug("Executing command: %s", cmd) @@ -1639,7 +1673,7 @@ def verify_expected_rules_in_order(expected_rules, rules): else: destroy_network_rules_for_nic(args.vmName, args.vmIP, args.vmMAC, args.vif, args.nicSecIps) elif cmd == "default_network_rules_systemvm": - default_network_rules_systemvm(args.vmName, args.localbrname) + default_network_rules_systemvm(args.vmName, args.localbrname, args.onlyExistingFramework) elif cmd == "get_rule_logs_for_vms": get_rule_logs_for_vms() elif cmd == "add_network_rules": diff --git a/server/src/main/java/com/cloud/network/security/SecurityGroupManagerImpl.java b/server/src/main/java/com/cloud/network/security/SecurityGroupManagerImpl.java index 8b9387752658..1025989f0539 100644 --- a/server/src/main/java/com/cloud/network/security/SecurityGroupManagerImpl.java +++ b/server/src/main/java/com/cloud/network/security/SecurityGroupManagerImpl.java @@ -564,9 +564,6 @@ protected void handleVmStopped(VMInstanceVO vm) { } protected void handleVmMigrated(VMInstanceVO vm) { - if (!isVmSecurityGroupEnabled(vm.getId())) { - return; - } if (vm.getType() != VirtualMachine.Type.User) { Commands cmds = null; NetworkRulesSystemVmCommand nrc = new NetworkRulesSystemVmCommand(vm.getInstanceName(), vm.getType()); @@ -580,6 +577,9 @@ protected void handleVmMigrated(VMInstanceVO vm) { } } else { + if (!isVmSecurityGroupEnabled(vm.getId())) { + return; + } List affectedVms = new ArrayList(); affectedVms.add(vm.getId()); scheduleRulesetUpdateToHosts(affectedVms, true, null); diff --git a/server/src/test/java/com/cloud/network/security/SecurityGroupManagerImplMigrationTest.java b/server/src/test/java/com/cloud/network/security/SecurityGroupManagerImplMigrationTest.java new file mode 100644 index 000000000000..0aa872709b22 --- /dev/null +++ b/server/src/test/java/com/cloud/network/security/SecurityGroupManagerImplMigrationTest.java @@ -0,0 +1,58 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package com.cloud.network.security; + +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.agent.AgentManager; +import com.cloud.agent.api.NetworkRulesSystemVmCommand; +import com.cloud.agent.manager.Commands; +import com.cloud.vm.VMInstanceVO; +import com.cloud.vm.VirtualMachine; + +@RunWith(MockitoJUnitRunner.Silent.class) +public class SecurityGroupManagerImplMigrationTest { + + @Mock + AgentManager _agentMgr; + + @InjectMocks + SecurityGroupManagerImpl manager = new SecurityGroupManagerImpl(); + + @Test + public void systemVmMigrationSendsNetworkRulesSystemVmCommand() throws Exception { + VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); + Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.SecondaryStorageVm); + Mockito.when(vm.getInstanceName()).thenReturn("s-1-VM"); + Mockito.when(vm.getHostId()).thenReturn(2L); + + manager.handleVmMigrated(vm); + + ArgumentCaptor captor = ArgumentCaptor.forClass(Commands.class); + Mockito.verify(_agentMgr).send(Mockito.eq(2L), captor.capture()); + Assert.assertEquals(1, captor.getValue().size()); + Assert.assertTrue(captor.getValue().toCommands()[0] instanceof NetworkRulesSystemVmCommand); + } +} From bd59d0be81b82dcece03004a1e14d40ae9ac77f0 Mon Sep 17 00:00:00 2001 From: Prashanth Reddy Date: Thu, 29 Oct 2026 10:40:28 +0000 Subject: [PATCH 2/3] kvm: update NetworkRulesSystemVmCommand test for onlyExistingFramework overload --- .../hypervisor/kvm/resource/LibvirtComputingResourceTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java index fa204f806261..777a777404f3 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java @@ -2871,7 +2871,7 @@ public void testNetworkRulesSystemVmCommand() { } catch (final LibvirtException e) { fail(e.getMessage()); } - when(libvirtComputingResourceMock.configureDefaultNetworkRulesForSystemVm(conn, command.getVmName())).thenReturn(true); + when(libvirtComputingResourceMock.configureDefaultNetworkRulesForSystemVm(conn, command.getVmName(), true)).thenReturn(true); final LibvirtRequestWrapper wrapper = LibvirtRequestWrapper.getInstance(); assertNotNull(wrapper); @@ -2885,7 +2885,7 @@ public void testNetworkRulesSystemVmCommand() { fail(e.getMessage()); } verify(libvirtComputingResourceMock, times(1)).getLibvirtUtilitiesHelper(); - verify(libvirtComputingResourceMock, times(1)).configureDefaultNetworkRulesForSystemVm(conn, command.getVmName()); + verify(libvirtComputingResourceMock, times(1)).configureDefaultNetworkRulesForSystemVm(conn, command.getVmName(), true); } @SuppressWarnings("unchecked") From fcd42fc6b393dece99d87864b68302ccea8521e3 Mon Sep 17 00:00:00 2001 From: Prashanth Reddy Date: Thu, 29 Oct 2026 10:47:31 +0000 Subject: [PATCH 3/3] kvm: add tests for the applyDefaultNetworkRules system VM fallback --- .../LibvirtComputingResourceTest.java | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java index 777a777404f3..ea5a5e7d5d06 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java @@ -2919,6 +2919,60 @@ public void testNetworkRulesSystemVmCommandFailure() { verify(libvirtComputingResourceMock, times(1)).getLibvirtUtilitiesHelper(); } + @Test + public void testApplyDefaultNetworkRulesProgramsSystemVmWithoutSecurityGroupNic() { + final Connect conn = Mockito.mock(Connect.class); + final VirtualMachineTO vm = Mockito.mock(VirtualMachineTO.class); + final NicTO nic = Mockito.mock(NicTO.class); + + when(vm.getType()).thenReturn(Type.SecondaryStorageVm); + when(vm.getName()).thenReturn("s-1-VM"); + when(vm.getNics()).thenReturn(new NicTO[] {nic}); + when(nic.isSecurityGroupEnabled()).thenReturn(false); + when(nic.getIsolationUri()).thenReturn(null); + doReturn(true).when(libvirtComputingResourceSpy).configureDefaultNetworkRulesForSystemVm(conn, "s-1-VM", true); + + assertTrue(libvirtComputingResourceSpy.applyDefaultNetworkRules(conn, vm, false)); + + verify(libvirtComputingResourceSpy, times(1)).configureDefaultNetworkRulesForSystemVm(conn, "s-1-VM", true); + verify(libvirtComputingResourceSpy, never()).configureDefaultNetworkRulesForSystemVm(conn, "s-1-VM", false); + } + + @Test + public void testApplyDefaultNetworkRulesDoesNotProgramUserVm() { + final Connect conn = Mockito.mock(Connect.class); + final VirtualMachineTO vm = Mockito.mock(VirtualMachineTO.class); + final NicTO nic = Mockito.mock(NicTO.class); + + when(vm.getType()).thenReturn(Type.User); + when(vm.getNics()).thenReturn(new NicTO[] {nic}); + when(nic.isSecurityGroupEnabled()).thenReturn(false); + when(nic.getIsolationUri()).thenReturn(null); + + assertTrue(libvirtComputingResourceSpy.applyDefaultNetworkRules(conn, vm, false)); + + verify(libvirtComputingResourceSpy, never()).configureDefaultNetworkRulesForSystemVm(Mockito.any(Connect.class), + Mockito.anyString(), Mockito.anyBoolean()); + } + + @Test + public void testApplyDefaultNetworkRulesDoesNotFallBackWhenSystemVmNicIsSecurityGroupEnabled() { + final Connect conn = Mockito.mock(Connect.class); + final VirtualMachineTO vm = Mockito.mock(VirtualMachineTO.class); + final NicTO nic = Mockito.mock(NicTO.class); + + when(vm.getType()).thenReturn(Type.SecondaryStorageVm); + when(vm.getName()).thenReturn("s-1-VM"); + when(vm.getNics()).thenReturn(new NicTO[] {nic}); + when(nic.isSecurityGroupEnabled()).thenReturn(true); + doReturn(true).when(libvirtComputingResourceSpy).configureDefaultNetworkRulesForSystemVm(conn, "s-1-VM"); + + assertTrue(libvirtComputingResourceSpy.applyDefaultNetworkRules(conn, vm, false)); + + verify(libvirtComputingResourceSpy, times(1)).configureDefaultNetworkRulesForSystemVm(conn, "s-1-VM"); + verify(libvirtComputingResourceSpy, never()).configureDefaultNetworkRulesForSystemVm(conn, "s-1-VM", true); + } + @Test public void testCheckSshCommand() { final String instanceName = "Test";