CLOUDSTACK-9339 Virtual Routers don't handle Multiple Public Interfaces correctly

core/src/com/cloud/agent/resource/virtualnetwork/facade/IpAssociationConfigItem.java

@@ -41,7 +41,7 @@ public List<ConfigItem> generateConfig(final NetworkElementCommand cmd) {
 
         for (final IpAddressTO ip : command.getIpAddresses()) {
             final IpAddress ipAddress = new IpAddress(ip.getPublicIp(), ip.isSourceNat(), ip.isAdd(), ip.isOneToOneNat(), ip.isFirstIP(), ip.getVlanGateway(), ip.getVlanNetmask(),
-                    ip.getVifMacAddress(), ip.getNicDevId(), ip.isNewNic());
+                    ip.getVifMacAddress(), ip.getNicDevId(), ip.isNewNic(), ip.getTrafficType().toString());
             ips.add(ipAddress);
         }
 
@@ -56,4 +56,4 @@ protected List<ConfigItem> generateConfigItems(final ConfigBase configuration) {
 
         return super.generateConfigItems(configuration);
     }
-}
+}

core/src/com/cloud/agent/resource/virtualnetwork/model/IpAddress.java

@@ -31,13 +31,14 @@ public class IpAddress {
     private String vifMacAddress;
     private Integer nicDevId;
     private boolean newNic;
+    private String nwType;
 
     public IpAddress() {
         // Empty constructor for (de)serialization
     }
 
     public IpAddress(String publicIp, boolean sourceNat, boolean add, boolean oneToOneNat, boolean firstIP, String gateway, String netmask, String vifMacAddress,
-            Integer nicDevId, boolean newNic) {
+            Integer nicDevId, boolean newNic, String nwType) {
         super();
         this.publicIp = publicIp;
         this.sourceNat = sourceNat;
@@ -49,6 +50,7 @@ public IpAddress(String publicIp, boolean sourceNat, boolean add, boolean oneToO
         this.vifMacAddress = vifMacAddress;
         this.nicDevId = nicDevId;
         this.newNic = newNic;
+        this.nwType = nwType;
     }
 
     public String getPublicIp() {

core/test/com/cloud/agent/resource/virtualnetwork/ConfigHelperTest.java

@@ -57,6 +57,7 @@
 import com.cloud.agent.resource.virtualnetwork.model.LoadBalancerRule;
 import com.cloud.agent.resource.virtualnetwork.model.LoadBalancerRules;
 import com.cloud.network.lb.LoadBalancingRule.LbDestination;
+import com.cloud.network.Networks.TrafficType;
 
 public class ConfigHelperTest {
 
@@ -272,9 +273,15 @@ protected DeleteIpAliasCommand generateDeleteIpAliasCommand() {
 
     protected IpAssocVpcCommand generateIpAssocVpcCommand() {
         final List<IpAddressTO> ips = new ArrayList<IpAddressTO>();
-        ips.add(new IpAddressTO(1, "64.1.1.10", true, true, true, "vlan://64", "64.1.1.1", "255.255.255.0", "01:23:45:67:89:AB", 1000, false));
-        ips.add(new IpAddressTO(2, "64.1.1.11", false, false, true, "vlan://64", "64.1.1.1", "255.255.255.0", "01:23:45:67:89:AB", 1000, false));
-        ips.add(new IpAddressTO(3, "65.1.1.11", true, false, false, "vlan://65", "65.1.1.1", "255.255.255.0", "11:23:45:67:89:AB", 1000, false));
+        IpAddressTO ip1 = new IpAddressTO(1, "64.1.1.10", true, true, true, "vlan://64", "64.1.1.1", "255.255.255.0", "01:23:45:67:89:AB", 1000, false);
+        IpAddressTO ip2 = new IpAddressTO(2, "64.1.1.11", false, false, true, "vlan://64", "64.1.1.1", "255.255.255.0", "01:23:45:67:89:AB", 1000, false);
+        IpAddressTO ip3 = new IpAddressTO(3, "65.1.1.11", true, false, false, "vlan://65", "65.1.1.1", "255.255.255.0", "11:23:45:67:89:AB", 1000, false);
+        ip1.setTrafficType(TrafficType.Public);
+        ip2.setTrafficType(TrafficType.Public);
+        ip3.setTrafficType(TrafficType.Public);
+        ips.add(ip1);
+        ips.add(ip2);
+        ips.add(ip3);
 
         final IpAddressTO[] ipArray = ips.toArray(new IpAddressTO[ips.size()]);
         final IpAssocVpcCommand cmd = new IpAssocVpcCommand(ipArray);
@@ -283,4 +290,4 @@ protected IpAssocVpcCommand generateIpAssocVpcCommand() {
 
         return cmd;
     }
-}
+}

plugins/hypervisors/ovm3/src/test/java/com/cloud/hypervisor/ovm3/resources/Ovm3VirtualRoutingResourceTest.java

@@ -38,6 +38,7 @@
 import com.cloud.hypervisor.ovm3.resources.helpers.Ovm3Configuration;
 import com.cloud.hypervisor.ovm3.resources.helpers.Ovm3ConfigurationTest;
 import com.cloud.hypervisor.ovm3.support.Ovm3SupportTest;
+import com.cloud.network.Networks.TrafficType;
 import com.cloud.utils.ExecutionResult;
 
 public class Ovm3VirtualRoutingResourceTest {
@@ -206,6 +207,7 @@ private IpAddressTO[] getIp(String mac) {
         List<IpAddressTO> ips = new ArrayList<IpAddressTO>();
         IpAddressTO ip = new IpAddressTO(1, routerip, true, true, true, "vlan://"
                 + br[1], "64.1.1.1", "255.255.255.0", mac, 1000, false);
+        ip.setTrafficType(TrafficType.Public);
         ips.add(ip);
         IpAddressTO[] ipArray = ips.toArray(new IpAddressTO[ips.size()]);
         return ipArray;

systemvm/patches/debian/config/opt/cloud/bin/configure.py

@@ -883,6 +883,12 @@ def processStaticNatRule(self, rule):
         device = self.getDeviceByIp(rule["public_ip"])
         if device is None:
             raise Exception("Ip address %s has no device in the ips databag" % rule["public_ip"])
+        self.fw.append(["mangle", "",
+                        "-A PREROUTING -s %s/32 -m state --state NEW -j MARK --set-xmark 0x%s/0xffffffff" % \
+                        (rule["internal_ip"], device[len("eth"):])])
+        self.fw.append(["mangle", "",
+                        "-A PREROUTING -s %s/32 -m state --state NEW -j CONNMARK --save-mark --nfmask 0xffffffff --ctmask 0xffffffff" % \
+                        rule["internal_ip"]])
         self.fw.append(["nat", "front",
                         "-A PREROUTING -d %s/32 -j DNAT --to-destination %s" % (rule["public_ip"], rule["internal_ip"])])
         self.fw.append(["nat", "front",

systemvm/patches/debian/config/opt/cloud/bin/cs/CsAddress.py

@@ -114,7 +114,7 @@ def process(self):
                 else:
                     logging.info(
                         "Address %s on device %s not configured", ip.ip(), dev)
-                    
+
                     if CsDevice(dev, self.config).waitfordevice():
                         ip.configure(address)
 
@@ -191,7 +191,7 @@ def is_public(self):
         if "nw_type" in self.address and self.address['nw_type'] in ['public']:
             return True
         return False
-    
+
     def is_added(self):
         return self.get_attr("add")
 
@@ -277,9 +277,10 @@ def configure(self, address):
             except Exception as e:
                 logging.info("Exception occurred ==> %s" % e)
 
+            self.post_configure(address)
         else:
+            # delete method performs post_configure, so no need to call post_configure here
             self.delete(self.ip())
-        self.post_configure(address)
 
     def post_configure(self, address):
         """ The steps that must be done after a device is configured """
@@ -291,7 +292,10 @@ def post_configure(self, address):
 
             interfaces = [CsInterface(address, self.config)]
             CsHelper.reconfigure_interfaces(self.cl, interfaces)
-            self.set_mark()
+            if not self.config.is_vpc() and (self.get_type() in ['public']):
+                self.set_mark()
+            if self.config.is_vpc() and (self.get_type() in ['public']):
+                self.set_mark()
 
             if 'gateway' in self.address:
                 self.arpPing()
@@ -351,7 +355,7 @@ def setup_router_control(self):
         self.fw.append(["filter", "", "-P INPUT DROP"])
         self.fw.append(["filter", "", "-P FORWARD DROP"])
 
-        
+
     def fw_router(self):
         if self.config.is_vpc():
             return
@@ -386,6 +390,10 @@ def fw_router(self):
                             "-j CONNMARK --set-xmark %s/0xffffffff" % self.dnum])
             self.fw.append(
                 ["mangle", "", "-A FIREWALL_%s -j DROP" % self.address['public_ip']])
+            self.fw.append(["filter", "",
+                            "-A FORWARD -i %s -o eth0 -m state --state RELATED,ESTABLISHED -j ACCEPT" % self.dev])
+            self.fw.append(["filter", "",
+                            "-A FORWARD -i eth0 -o %s -j FW_OUTBOUND" % self.dev])
 
         self.fw.append(["filter", "", "-A INPUT -d 224.0.0.18/32 -j ACCEPT"])
         self.fw.append(["filter", "", "-A INPUT -d 225.0.0.50/32 -j ACCEPT"])
@@ -410,15 +418,8 @@ def fw_router(self):
                 ["filter", "", "-A FORWARD -i %s -o eth1 -m state --state RELATED,ESTABLISHED -j ACCEPT" % self.dev])
             self.fw.append(
                 ["filter", "", "-A FORWARD -i %s -o %s -m state --state NEW -j ACCEPT" % (self.dev, self.dev)])
-            self.fw.append(
-                ["filter", "", "-A FORWARD -i eth2 -o eth0 -m state --state RELATED,ESTABLISHED -j ACCEPT"])
             self.fw.append(
                 ["filter", "", "-A FORWARD -i eth0 -o eth0 -m state --state RELATED,ESTABLISHED -j ACCEPT"])
-            self.fw.append(
-                ["filter", "", "-A FORWARD -i eth0 -o eth2 -j FW_OUTBOUND"])
-            self.fw.append(["mangle", "",
-                            "-A PREROUTING -i %s -m state --state NEW " % self.dev +
-                            "-j CONNMARK --set-xmark %s/0xffffffff" % self.dnum])
 
         self.fw.append(['', 'front', '-A FORWARD -j NETWORK_STATS'])
         self.fw.append(['', 'front', '-A INPUT -j NETWORK_STATS'])
@@ -427,17 +428,17 @@ def fw_router(self):
         self.fw.append(['', '', '-A NETWORK_STATS -i eth2 -o eth0'])
         self.fw.append(['', '', '-A NETWORK_STATS -o eth2 ! -i eth0 -p tcp'])
         self.fw.append(['', '', '-A NETWORK_STATS -i eth2 ! -o eth0 -p tcp'])
-        
+
     def fw_vpcrouter(self):
         if not self.config.is_vpc():
             return
-        self.fw.append(["mangle", "front", "-A PREROUTING " +
-                        "-m state --state RELATED,ESTABLISHED " +
-                        "-j CONNMARK --restore-mark --nfmask 0xffffffff --ctmask 0xffffffff"])
-
+
         self.fw.append(["filter", "", "-A FORWARD -m state --state RELATED,ESTABLISHED -j ACCEPT"])
 
         if self.get_type() in ["guest"]:
+            self.fw.append(["mangle", "front", "-A PREROUTING " +
+                            " -i %s -m state --state RELATED,ESTABLISHED "  % self.dev +
+                            "-j CONNMARK --restore-mark --nfmask 0xffffffff --ctmask 0xffffffff"])
             guestNetworkCidr = self.address['network']
             self.fw.append(["filter", "", "-A FORWARD -d %s -o %s -j ACL_INBOUND_%s" %
                             (guestNetworkCidr, self.dev, self.dev)])
@@ -515,11 +516,36 @@ def fw_vpcrouter(self):
 
     def post_config_change(self, method):
         route = CsRoute()
+        tableName = "Table_" + self.dev
+
         if method == "add":
-            route.add_table(self.dev)
-            route.add_route(self.dev, str(self.address["network"]))
+            if not self.config.is_vpc():
+                # treat the first IP on a interface as special case to set up the routing rules
+                if self.get_type() in ["public"] and (len(self.iplist) == 1):
+                    CsHelper.execute("sudo ip route add throw " + self.config.address().dbag['eth0'][0]['network'] + " table " + tableName + " proto static")
+                    CsHelper.execute("sudo ip route add throw " + self.config.address().dbag['eth1'][0]['network'] + " table " + tableName + " proto static")
+
+                # add 'defaul via gateway' rule in the device specific routing table
+                if "gateway" in self.address and self.address["gateway"] != "None":
+                    route.add_route(self.dev, self.address["gateway"])
+
+                if self.get_type() in ["public"]:
+                    CsRule(self.dev).addRule("from " + str(self.address["network"]))
+
+            if self.config.is_vpc():
+                if self.get_type() in ["public"] and "gateway" in self.address and self.address["gateway"] != "None":
+                    route.add_route(self.dev, self.address["gateway"])
+                route.add_network_route(self.dev, str(self.address["network"]))
+
+            CsHelper.execute("sudo ip route flush cache")
+
         elif method == "delete":
-            logging.warn("delete route not implemented")
+            # treat the last IP to be dis-associated with interface as special case to clean up the routing rules
+            if self.get_type() in ["public"] and (not self.config.is_vpc()) and (len(self.iplist) == 0):
+                CsHelper.execute("sudo ip rule delete table " + tableName)
+                CsHelper.execute("sudo ip route flush table " + tableName)
+                CsHelper.execute("sudo ip route flush cache")
+                CsRule(self.dev).delMark()
 
         self.fw_router()
         self.fw_vpcrouter()
@@ -558,15 +584,8 @@ def list(self):
         for i in CsHelper.execute(cmd):
             vals = i.lstrip().split()
             if (vals[0] == 'inet'):
-
                 cidr = vals[1]
-                for ip, device in self.iplist.iteritems():
-                    logging.info(
-                                 "Iterating over the existing IPs. CIDR to be configured ==> %s, existing IP ==> %s on device ==> %s",
-                                 cidr, ip, device)
-
-                    if cidr[0] != ip[0] and device != self.dev:
-                        self.iplist[cidr] = self.dev
+                self.iplist[cidr] = self.dev
 
     def configured(self):
         if self.address['cidr'] in self.iplist.keys():

systemvm/patches/debian/config/opt/cloud/bin/cs/CsRoute.py

@@ -36,13 +36,28 @@ def add_table(self, devicename):
         filename = "/etc/iproute2/rt_tables"
         logging.info(
             "Adding route table: " + str + " to " + filename + " if not present ")
-        CsHelper.addifmissing(filename, str)
+        if not CsHelper.definedinfile(filename, str):
+             CsHelper.execute("sudo echo " + str + " >> /etc/iproute2/rt_tables")
+        # remove "from all table tablename" if exists, else it will interfer with
+        # routing of unintended traffic
+        if self.findRule("from all lookup " + tablename):
+             CsHelper.execute("sudo ip rule delete from all table " + tablename)
 
     def flush_table(self, tablename):
         CsHelper.execute("ip route flush table %s" % (tablename))
         CsHelper.execute("ip route flush cache")
 
     def add_route(self, dev, address):
+        """ Wrapper method that adds table name and device to route statement """
+        # ip route add dev eth1 table Table_eth1 10.0.2.0/24
+        table = self.get_tablename(dev)
+        logging.info("Adding route: dev " + dev + " table: " +
+                     table + " network: " + address + " if not present")
+        cmd = "dev %s table %s %s" % (dev, table, address)
+        cmd = "default via %s table %s proto static" % (address, table)
+        self.set_route(cmd)
+
+    def add_network_route(self, dev, address):
         """ Wrapper method that adds table name and device to route statement """
         # ip route add dev eth1 table Table_eth1 10.0.2.0/24
         table = self.get_tablename(dev)
@@ -73,7 +88,7 @@ def add_defaultroute(self, gateway):
         """
         if not gateway:
             raise Exception("Gateway cannot be None.")
-        
+
         if self.defaultroute_exists():
             return False
         else:
@@ -95,3 +110,9 @@ def defaultroute_exists(self):
         else:
             logging.warn("No default route found!")
             return False
+
+    def findRule(self, rule):
+        for i in CsHelper.execute("ip rule show"):
+            if rule in i.strip():
+                return True
+        return False

systemvm/patches/debian/config/opt/cloud/bin/cs/CsRule.py

@@ -30,12 +30,30 @@ def __init__(self, dev):
         self.tableNo = int(dev[3:])
         self.table = "Table_%s" % (dev)
 
+    def addRule(self, rule):
+        if not self.findRule(rule + " lookup " + self.table):
+            cmd = "ip rule add " + rule + " table " + self.table
+            CsHelper.execute(cmd)
+            logging.info("Added rule %s for %s" % (cmd, self.table))
+
+    def findRule(self, rule):
+        for i in CsHelper.execute("ip rule show"):
+            if rule in i.strip():
+                return True
+        return False
+
     def addMark(self):
         if not self.findMark():
             cmd = "ip rule add fwmark %s table %s" % (self.tableNo, self.table)
             CsHelper.execute(cmd)
             logging.info("Added fwmark rule for %s" % (self.table))
 
+    def delMark(self):
+        if  self.findMark():
+            cmd = "ip rule delete fwmark %s table %s" % (self.tableNo, self.table)
+            CsHelper.execute(cmd)
+            logging.info("Deleting fwmark rule for %s" % (self.table))
+
     def findMark(self):
         srch = "from all fwmark %s lookup %s" % (hex(self.tableNo), self.table)
         for i in CsHelper.execute("ip rule show"):

systemvm/patches/debian/config/opt/cloud/bin/cs_ip.py

@@ -21,24 +21,29 @@
 
 def merge(dbag, ip):
     added = False
+    nic_dev_id = None
     for dev in dbag:
         if dev == "id":
             continue
         for address in dbag[dev]:
             if address['public_ip'] == ip['public_ip']:
+                if 'nic_dev_id' in address:
+                    nic_dev_id = address['nic_dev_id']
                 dbag[dev].remove(address)
 
     ipo = IPNetwork(ip['public_ip'] + '/' + ip['netmask'])
-    ip['device'] = 'eth' + str(ip['nic_dev_id'])
+    if 'nic_dev_id' in ip:
+         nic_dev_id = ip['nic_dev_id']
+    ip['device'] = 'eth' + str(nic_dev_id)
     ip['broadcast'] = str(ipo.broadcast)
     ip['cidr'] = str(ipo.ip) + '/' + str(ipo.prefixlen)
     ip['size'] = str(ipo.prefixlen)
     ip['network'] = str(ipo.network) + '/' + str(ipo.prefixlen)
     if 'nw_type' not in ip.keys():
         ip['nw_type'] = 'public'
     if ip['nw_type'] == 'control':
-        dbag['eth' + str(ip['nic_dev_id'])] = [ip]
+        dbag['eth' + str(nic_dev_id)] = [ip]
     else:
-        dbag.setdefault('eth' + str(ip['nic_dev_id']), []).append(ip)
+        dbag.setdefault('eth' + str(nic_dev_id), []).append(ip)
 
     return dbag