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/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..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 @@ -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") @@ -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"; 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); + } +}