kvm: program system VMs into the security group bridge firewall framework

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;

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);
         }

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";

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":

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<Long> affectedVms = new ArrayList<Long>();
             affectedVms.add(vm.getId());
             scheduleRulesetUpdateToHosts(affectedVms, true, null);

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<Commands> 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);
+    }
+}