Fix XenServer Security Groups 'vmops' script (#3197)

* Fix XenServer Security Groups 'vmops' script

- fix tokens = line.split(':') to tokens = line.split(';')
- fix expected tokens size from 5 to 4
- enhance logs
- remove unused vmops script. The XCP patch points to the vmops script
on the parent folder [1]. Thus, all XenServer versions are considering
the vmops script located at [2].
- fix UI ipv4/ipv6 cidr validator to allow a list of cidirs.

Fixing issue: #3192 Security Group rules not applied at all for
XenServer 6.5 / Advanced Zone
https://github.com/apache/cloudstack/issues/3192

* Update security group rules after VM migration

Add security group rules on target host
Cause: vmops script expected secondary IPs as "0;" but received "0:"

Remove security group network rules on source host.
Cause: destroy_network_rules_for_vm function on vmops script was not
called when migrating VM

* Add unit tests and address reviewers

core/src/main/java/com/cloud/agent/api/SecurityGroupRulesCmd.java

@@ -173,7 +173,7 @@ public class SecurityGroupRulesCmd extends Command {
         final StringBuilder sb = new StringBuilder();
         final List<String> ips = getSecIps();
         if (ips == null) {
-            sb.append("0:");
+            sb.append("0").append(RULE_COMMAND_SEPARATOR);
         } else {
             for (final String ip : ips) {
                 sb.append(ip).append(RULE_COMMAND_SEPARATOR);

plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixMigrateCommandWrapper.java

@@ -21,7 +21,9 @@ package com.cloud.hypervisor.xenserver.resource.wrapper.xenbase;
 
 import java.util.Set;
 
+import org.apache.commons.lang.BooleanUtils;
 import org.apache.log4j.Logger;
+import org.apache.xmlrpc.XmlRpcException;
 
 import com.cloud.agent.api.Answer;
 import com.cloud.agent.api.MigrateAnswer;
@@ -32,11 +34,13 @@ import com.cloud.resource.ResourceWrapper;
 import com.xensource.xenapi.Connection;
 import com.xensource.xenapi.Host;
 import com.xensource.xenapi.Types;
+import com.xensource.xenapi.Types.BadServerResponse;
+import com.xensource.xenapi.Types.XenAPIException;
 import com.xensource.xenapi.VBD;
 import com.xensource.xenapi.VM;
 
 @ResourceWrapper(handles =  MigrateCommand.class)
-public final class CitrixMigrateCommandWrapper extends CommandWrapper<MigrateCommand, Answer, CitrixResourceBase> {
+public class CitrixMigrateCommandWrapper extends CommandWrapper<MigrateCommand, Answer, CitrixResourceBase> {
 
     private static final Logger s_logger = Logger.getLogger(CitrixMigrateCommandWrapper.class);
 
@@ -81,6 +85,8 @@ public final class CitrixMigrateCommandWrapper extends CommandWrapper<MigrateCom
                 }
                 citrixResourceBase.migrateVM(conn, dsthost, vm, vmName);
                 vm.setAffinity(conn, dsthost);
+
+                destroyMigratedVmNetworkRulesOnSourceHost(command, citrixResourceBase, conn, dsthost);
             }
 
             // The iso can be attached to vm only once the vm is (present in the host) migrated.
@@ -95,4 +101,19 @@ public final class CitrixMigrateCommandWrapper extends CommandWrapper<MigrateCom
             return new MigrateAnswer(command, false, e.getMessage(), null);
         }
     }
+
+    /**
+     * On networks with security group, it removes the network rules related to the migrated VM from the source host.
+     */
+    protected void destroyMigratedVmNetworkRulesOnSourceHost(final MigrateCommand command, final CitrixResourceBase citrixResourceBase, final Connection conn, Host dsthost)
+            throws BadServerResponse, XenAPIException, XmlRpcException {
+        if (citrixResourceBase.canBridgeFirewall()) {
+            final String result = citrixResourceBase.callHostPlugin(conn, "vmops", "destroy_network_rules_for_vm", "vmName", command.getVmName());
+            if (BooleanUtils.toBoolean(result)) {
+                s_logger.debug(String.format("Removed network rules from source host [%s] for migrated vm [%s]", dsthost.getHostname(conn), command.getVmName()));
+            } else {
+                s_logger.warn(String.format("Failed to remove network rules from source host [%s] for migrated vm [%s]", dsthost.getHostname(conn), command.getVmName()));
+            }
+        }
+    }
 }

plugins/hypervisors/xenserver/src/test/java/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixMigrateCommandWrapperTest.java

@@ -0,0 +1,70 @@
+//
+// 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
+// with 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.hypervisor.xenserver.resource.wrapper.xenbase;
+
+import org.apache.xmlrpc.XmlRpcException;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.Spy;
+import org.mockito.runners.MockitoJUnitRunner;
+
+import com.cloud.agent.api.MigrateCommand;
+import com.cloud.agent.api.to.VirtualMachineTO;
+import com.cloud.hypervisor.xenserver.resource.CitrixResourceBase;
+import com.xensource.xenapi.Connection;
+import com.xensource.xenapi.Host;
+import com.xensource.xenapi.Types.BadServerResponse;
+import com.xensource.xenapi.Types.XenAPIException;
+
+@RunWith(MockitoJUnitRunner.class)
+public class CitrixMigrateCommandWrapperTest {
+
+    @Mock
+    private CitrixResourceBase citrixResourceBase;
+    @Spy
+    private CitrixMigrateCommandWrapper citrixMigrateCommandWrapper;
+
+    @Test
+    public void destroyNetworkRulesOnSourceHostForMigratedVmTestSupportSecurityGroup() throws BadServerResponse, XenAPIException, XmlRpcException {
+        configureExecuteAndVerifyDestroyNetworkRulesOnSourceHostForMigratedVmTest(true, 1);
+    }
+
+    @Test
+    public void destroyNetworkRulesOnSourceHostForMigratedVmTestDoesNotSupportSecurityGroup() throws BadServerResponse, XenAPIException, XmlRpcException {
+        configureExecuteAndVerifyDestroyNetworkRulesOnSourceHostForMigratedVmTest(false, 0);
+    }
+
+    private void configureExecuteAndVerifyDestroyNetworkRulesOnSourceHostForMigratedVmTest(boolean canBridgeFirewall, int timesCallHostPlugin)
+            throws BadServerResponse, XenAPIException, XmlRpcException {
+
+        final MigrateCommand command = new MigrateCommand("migratedVmName", "destHostIp", false, Mockito.mock(VirtualMachineTO.class), true);
+        Connection conn = Mockito.mock(Connection.class);
+        Host dsthost = Mockito.mock(Host.class);
+        Mockito.doReturn("hostname").when(dsthost).getHostname(conn);
+
+        Mockito.doReturn(canBridgeFirewall).when(citrixResourceBase).canBridgeFirewall();
+
+        citrixMigrateCommandWrapper.destroyMigratedVmNetworkRulesOnSourceHost(command, citrixResourceBase, conn, dsthost);
+
+        Mockito.verify(citrixResourceBase, Mockito.times(timesCallHostPlugin)).callHostPlugin(conn, "vmops", "destroy_network_rules_for_vm", "vmName", "migratedVmName");
+    }
+
+}

scripts/vm/hypervisor/xenserver/vmops

@@ -824,7 +824,7 @@ def default_network_rules(session, args):
        logging.debug(" failed to add vm " + vm_ip + " ip to set ")
        return 'false'
 
-    #add secodnary nic ips to ipset
+    #add secondary nic ips to ipset
     secIpSet = "1"
     ips = sec_ips.split(';')
     ips.pop()
@@ -1419,12 +1419,22 @@ def network_rules(session, args):
         reason = 'domid_change'
 
     rules = args.pop('rules')
+    logging.debug("Network rules compressed in base64 [%s]." % rules)
+
     if deflated.lower() == 'true':
        rules = inflate_rules (rules)
     keyword = '--' + get_ipset_keyword()
+
+    #Split spaces into lines. Example of rule [I:tcp;22;22;0.0.0.0/22,NEXT I:tcp;8001;8010;192.168.100.0/24,NEXT E:tcp;22;22;0.0.0.0/0,NEXT ]
+    #After split:
+    # I:tcp;22;22;0.0.0.0/22,NEXT
+    # I:tcp;8001;8010;192.168.100.0/24,NEXT
+    # E:tcp;22;22;0.0.0.0/0,NEXT
     lines = rules.split(' ')
 
-    logging.debug("Programming network rules for vm  %s seqno=%s numrules=%s signature=%s guestIp=%s,"\
+    logging.info("Network rules to be processed [%s]." % rules)
+
+    logging.info("Programming network rules for vm  %s seqno=%s numrules=%s signature=%s guestIp=%s,"\
               " update iptables, reason=%s" % (vm_name, seqno, len(lines), signature, vm_ip, reason))
 
     # Flush iptables rules to clear ipset references and before re-applying iptable rules
@@ -1438,14 +1448,24 @@ def network_rules(session, args):
     cmds = []
     egressrules = 0
     for line in lines:
-        tokens = line.split(':')
-        if len(tokens) != 5:
-          continue
-        token_type = tokens[0]
-        protocol = tokens[1]
-        start = tokens[2]
-        end = tokens[3]
-        cidrs = tokens.pop().split(",")
+        logging.debug("Processing rule [%s]." % line)
+        
+        #Example of rule: [I:tcp;12;34;1.2.3.4/24,NEXT] -> tokens: ['I:tcp', '12', '34', '1.2.3.4/24,NEXT'].
+        tokens = line.split(';')
+        logging.debug("Tokens %s." % tokens)
+        
+        tokens_size = len(tokens)
+
+        expected_tokens_size = 4
+        if tokens_size != expected_tokens_size:
+            logging.warning("Network rule tokens size [%s] is different from the expected size [%s], ignoring rule %s" % (str(tokens_size), str(expected_tokens_size), tokens))
+            continue
+
+        token_type, protocol = tokens[0].split(':')
+        start = tokens[1]
+        end = tokens[2]
+        cidrs = tokens[3].split(",")
+        # Remove placeholder 'NEXT' from the cidrs array
         cidrs.pop()
         allow_any = False
 
@@ -1456,17 +1476,22 @@ def network_rules(session, args):
             action = "RETURN"
             direction = "dst"
             egressrules = egressrules + 1
+            logging.debug("Ipset chaing [%s], VM chain [%s], VM name [%s], Action [%s], Direction [%s], egress rules [%s]" % (ipset_chain, vmchain, vm_name, action, direction, egressrules))
         else:
             #ipset chain name
             ipset_chain = ingress_chain_name_ipset(vm_name)
             vmchain = chain_name(vm_name)
             action = "ACCEPT"
             direction = "src"
-        if  '0.0.0.0/0' in cidrs:
+            logging.debug("Ipset chaing [%s], VM [%s], Action [%s], Direction [%s], egress rules [%s]" % (ipset_chain, vm_name, action, direction, egressrules))
+        if '0.0.0.0/0' in cidrs:
             i = cidrs.index('0.0.0.0/0')
             del cidrs[i]
             allow_any = True
+        
         port_range = start + ":" + end
+        logging.debug("port range [%s]" % port_range)
+
         if cidrs:
             #create seperate ipset name
             ipsetname = ipset_chain + "" + protocol[0:1] + "" + start + "_" + end
@@ -1500,6 +1525,7 @@ def network_rules(session, args):
             logging.debug(iptables)
 
     for cmd in cmds:
+        logging.debug("Executing command: [%s]." % str(cmd))
         util.pread2(cmd)
 
     vmchain = chain_name(vm_name)
@@ -1517,7 +1543,7 @@ def network_rules(session, args):
 
     return 'true'
   except:
-    logging.debug("Failed to network rule !")
+    logging.exception("Failed to network rule!")
 
 if __name__ == "__main__":
      XenAPIPlugin.dispatch({"pingtest": pingtest, "setup_iscsi":setup_iscsi,

ui/scripts/network.js

@@ -4584,7 +4584,7 @@
                                             label: 'label.cidr',
                                             isHidden: true,
                                             validation: {
-                                                ipv46cidr: true
+                                                ipv46cidrs: true
                                             }
                                         },
                                         'accountname': {
@@ -4794,7 +4794,7 @@
                                             label: 'label.cidr',
                                             isHidden: true,
                                             validation: {
-                                                ipv46cidr: true
+                                                ipv46cidrs: true
                                             }
                                         },
                                         'accountname': {

ui/scripts/sharedFunctions.js

@@ -2615,6 +2615,19 @@ $.validator.addMethod("ipv4cidr", function(value, element) {
     return true;
 }, "The specified IPv4 CIDR is invalid.");
 
+$.validator.addMethod("ipv46cidrs", function(value, element) {
+    var result = true;
+    if (value) {
+        var validatorThis = this;
+        value.split(',').forEach(function(item){
+            if (result && !$.validator.methods.ipv46cidr.call(validatorThis, item.trim(), element)) {
+                result = false;
+            }
+        })
+    }
+    return result;
+}, "The specified IPv4/IPv6 CIDR input is invalid.");
+
 $.validator.addMethod("ipv46cidr", function(value, element) {
     if (this.optional(element) && value.length == 0)
         return true;