refactored all the commands to cater to keshav's needs, and added the new apis. the initial set of testing is complete, will now focus on corner cases

client/tomcatconf/commands.properties.in

@@ -109,6 +109,7 @@ updatePortForwardingRule=com.cloud.api.commands.UpdatePortForwardingRuleCmd;15
 
 #### NAT commands
 createIpForwardingRule=com.cloud.api.commands.CreateIpForwardingRuleCmd;15
+deleteIpForwardingRule=com.cloud.api.commands.DeleteIpForwardingRuleCmd;15
 
 #### load balancer commands
 createLoadBalancerRule=com.cloud.api.commands.CreateLoadBalancerRuleCmd;15

core/src/com/cloud/agent/api/routing/SetFirewallRuleCommand.java

@@ -25,8 +25,8 @@ public class SetFirewallRuleCommand extends RoutingCommand {
     String routerIpAddress;
     String oldPrivateIP = null;
     String oldPrivatePort = null;
-    String guestIp = null;
+    boolean nat = false;
-    String portRange = null;
+    boolean create = false;
     
     protected SetFirewallRuleCommand() {
     }
@@ -39,12 +39,12 @@ public class SetFirewallRuleCommand extends RoutingCommand {
     	this.oldPrivatePort = oldPrivatePort;
     }
     
-    public SetFirewallRuleCommand(String routerName, String routerIpAddress, String guestIp, FirewallRuleVO rule, String portRange) {
+    public SetFirewallRuleCommand(String routerName, String routerIpAddress, boolean nat, FirewallRuleVO rule, boolean create) {
     	this.routerName = routerName;
     	this.routerIpAddress = routerIpAddress;
-    	this.guestIp = guestIp;
+    	this.nat = nat;
     	this.rule = rule;
-    	this.portRange = portRange;
+    	this.create = create;
     }
     
     @Override
@@ -100,12 +100,12 @@ public class SetFirewallRuleCommand extends RoutingCommand {
     	return this.oldPrivatePort;
     }
     
-    public String getPortRange(){
+    public boolean isNat(){
-    	return this.portRange;
+    	return this.nat;
     }
 
-    public String getGuestIp(){
+	public boolean isCreate() {
-    	return this.guestIp;
+		return create;
 	}
     
 }

core/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java

@@ -1115,6 +1115,18 @@ public abstract class CitrixResourceBase implements StoragePoolResource, ServerR
     protected Answer execute(final SetFirewallRuleCommand cmd) {
         String args;
 
+        if(cmd.isNat()){
+        	//1:1 NAT needs instanceip;publicip;domrip;op
+        	if(cmd.isCreate())
+        		args = "-A";
+        	else
+        		args = "-D";
+        	
+	        args += " -l " + cmd.getPublicIpAddress();
+	        args += " -i " + cmd.getRouterIpAddress();
+	        args += " -r " + cmd.getPrivateIpAddress();
+	        args += " -G " + cmd.getProtocol();
+        }else{
             if (cmd.isEnable()) {
                 args = "-A";
             } else {
@@ -1140,7 +1152,7 @@ public abstract class CitrixResourceBase implements StoragePoolResource, ServerR
 	        if (oldPrivatePort != null) {
 	            args += " -x " + oldPrivatePort;
 	        }
-
+        }
         String result = callHostPlugin("vmops", "setFirewallRule", "args", args);
 
         if (result == null || result.isEmpty()) {

patches/systemvm/debian/config/root/firewall.sh

@@ -9,7 +9,7 @@ usage() {
   printf "Usage: %s: (-A|-D) -i <domR eth1 ip>  -r <target-instance-ip> -P protocol (-p port_range | -t icmp_type_code)  -l <public ip address> -d <target port> [-f <firewall ip> -u <firewall user> -y <firewall password> -z <firewall enable password> ] \n" $(basename $0) >&2
 }
 
-set -x
+# set -x
 
 get_dom0_ip () {
  eval "$1=$(ifconfig eth0 | awk '/inet addr/ {split ($2,A,":"); print A[2]}')"
@@ -71,6 +71,27 @@ icmp_entry() {
   return $?
 }
 
+#Add 1:1 NAT entry
+add_one_to_one_nat_entry() {
+  local guestIp=$1
+  local publicIp=$2  
+  local dIp=$3
+  local op=$4 
+  iptables -t nat $op PREROUTING -i eth2 -d $publicIp -j DNAT --to-destination $guestIp
+  iptables -t nat $op POSTROUTING -o eth2 -s $guestIp -j SNAT --to-source $publicIp
+  if [ "$op" == "-A" ]
+  then
+    iptables -P FORWARD DROP
+  else
+    iptables -P FORWARD ACCEPT
+  fi
+  iptables $op FORWARD -m state --state RELATED,ESTABLISHED -j ACCEPT
+  iptables $op FORWARD -i eth2 -o eth0 -d $guestIp -m state --state NEW -j ACCEPT
+  iptables $op FORWARD -i eth0 -o eth2 -s $guestIp -m state --state NEW -j ACCEPT
+  
+  return $?
+}
+
 get_vif_list() {
   local vif_list=""
   for i in /sys/class/net/eth*; do 
@@ -107,11 +128,12 @@ wflag=
 xflag=
 nflag=
 Nflag=
+Gflag=
 op=""
 oldPrivateIP=""
 oldPrivatePort=""
 
-while getopts 'ADr:i:P:p:t:l:d:w:x:n:N:' OPTION
+while getopts 'ADr:i:P:p:t:l:d:w:x:n:N:G:' OPTION
 do
   case $OPTION in
   A)	Aflag=1
@@ -153,12 +175,23 @@ do
   N)	Nflag=1
   		netmask="$OPTARG"
   		;;
+  G)	Gflag=1
+		nat="$OPTARG"
+		;;
   ?)	usage
 		exit 2
 		;;
   esac
 done
 
+#1:1 NAT
+if [ "$Gflag" == "1" ]
+  then
+    add_one_to_one_nat_entry $instanceIp $publicIp $domRIp $op
+  fi
+  exit $?
+fi
+
 reverseOp=$(reverse_op $op)
 
 VIF_LIST=$(get_vif_list)

scripts/network/domr/call_firewall.sh

@@ -33,11 +33,12 @@ wflag=
 xflag=
 nflag=
 Nflag=
+Gflag=
 op=""
 oldPrivateIP=""
 oldPrivatePort=""
 
-while getopts 'ADr:i:P:p:t:l:d:w:x:n:N:' OPTION
+while getopts 'ADr:i:P:p:t:l:d:w:x:n:N:G:' OPTION
 do
   case $OPTION in
   A)	Aflag=1
@@ -79,6 +80,9 @@ do
   N)	Nflag=1
   		netmask="$OPTARG"
   		;;
+  G)    Gflag=1
+  		nat="OPTARG"
+  		;;
   ?)	usage
 		exit 2
 		;;
@@ -101,12 +105,6 @@ then
   exit 2
 fi
 
-if [ "$rflag$iflag$Pflag$pflag$tflag$lflag" != "11111" ]
-then
-  usage
-  exit 2
-fi
-
 #Require -d with -p
 if [ "$pflag$dflag" != 11 -a "$pflag$dflag" != "" ]
 then

scripts/network/domr/firewall.sh

@@ -87,31 +87,6 @@ icmp_entry() {
   		
 }
 
-
-#Add 1:1 NAT entry
-add_one_to_one_nat_entry() {
-  local guestIp=$1
-  local publicIp=$2  
-  local dIp=$3
-  local portRange=$4
-  local op=$5
-  ssh -p 3922 -o StrictHostKeyChecking=no -i $cert root@$dIp "\  
-  iptables -t nat $op PREROUTING -i eth2 -d $publicIp -j DNAT --to-destination $guestIp
-  iptables -t nat $op POSTROUTING -o eth2 -s $guestIp -j SNAT --to-source $publicIp":"$portRange
-  if [ "$op" == "-A" ]
-  then
-    iptables -P FORWARD DROP
-  else
-    iptables -P FORWARD ACCEPT
-  fi
-  iptables $op FORWARD -m state --state RELATED,ESTABLISHED -j ACCEPT
-  iptables $op FORWARD -i eth2 -o eth0 -d $guestIp -m state --state NEW -j ACCEPT
-  iptables $op FORWARD -i eth0 -o eth2 -s $guestIp -m state --state NEW -j ACCEPT
-  "
-  
-  return $?
-}
-
 reverse_op() {
 	local op=$1
 	
@@ -135,13 +110,11 @@ wflag=
 xflag=
 nflag=
 Nflag=
-Gflag=
-Rflag=
 op=""
 oldPrivateIP=""
 oldPrivatePort=""
 
-while getopts 'ADr:i:P:p:t:l:d:w:x:n:N:G:R:' OPTION
+while getopts 'ADr:i:P:p:t:l:d:w:x:n:N:' OPTION
 do
   case $OPTION in
   A)	Aflag=1
@@ -183,11 +156,6 @@ do
   N)	Nflag=1
   		netmask="$OPTARG"
   		;;
-  G)    Gflag=1
-  		guestIp="$OPTARG"
-  		;;
-  R)    Rflag=1
-        portRange="$OPTARG"  		
   ?)	usage
 		exit 2
 		;;
@@ -224,14 +192,6 @@ then
  exit 2
 fi
 
-#1:1 NAT
-if [ "$Gflag$Rflag" == "11" ]
-  then
-    add_one_to_one_nat_entry $guestIp $publicIp $domRIp $portRange $op
-  fi
-  exit $?
-fi
-
 reverseOp=$(reverse_op $op)
 
 case $protocol  in

server/src/com/cloud/network/NetworkManagerImpl.java

@@ -108,7 +108,6 @@ import com.cloud.network.Network.TrafficType;
 import com.cloud.network.configuration.NetworkGuru;
 import com.cloud.network.dao.FirewallRulesDao;
 import com.cloud.network.dao.IPAddressDao;
-import com.cloud.network.dao.IprulePortrangeMapDao;
 import com.cloud.network.dao.LoadBalancerDao;
 import com.cloud.network.dao.LoadBalancerVMMapDao;
 import com.cloud.network.dao.NetworkConfigurationDao;
@@ -220,7 +219,6 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
     @Inject RemoteAccessVpnDao _remoteAccessVpnDao = null;
     @Inject VpnUserDao _vpnUsersDao = null;
     @Inject DomainRouterManager _routerMgr;
-    @Inject IprulePortrangeMapDao _iprulePortrangeMapDao = null;
     
     @Inject(adapter=NetworkGuru.class)
     Adapters<NetworkGuru> _networkGurus;
@@ -898,7 +896,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
 
             if (rule.isForwarding()) {
                 fwdRules.add(rule);
-                final SetFirewallRuleCommand cmd = new SetFirewallRuleCommand(routerName, routerIp, null, rule, null);
+                final SetFirewallRuleCommand cmd = new SetFirewallRuleCommand(routerName, routerIp, false, rule, false);
                 cmds.addCommand(cmd);
             } else {
                 lbRules.add(rule);
@@ -977,7 +975,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
         	rule.setVlanNetmask(vlanNetmask);
             if (rule.isForwarding()) {
                 fwdRules.add(rule);
-                final SetFirewallRuleCommand cmd = new SetFirewallRuleCommand(router.getInstanceName(), router.getPrivateIpAddress(), null, rule, null);
+                final SetFirewallRuleCommand cmd = new SetFirewallRuleCommand(router.getInstanceName(), router.getPrivateIpAddress(), false, rule, false);
                 cmds.addCommand(cmd);
             }
         }
@@ -2948,6 +2946,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
     	UserVmVO userVM = null;
         FirewallRuleVO newFwRule = null;
         boolean locked = false;
+        boolean success = false;
 		try {
 			// validate IP Address exists
 			IPAddressVO ipAddress = _ipAddressDao.findById(cmd.getIpAddress());
@@ -3009,6 +3008,15 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
 			newFwRule.setPublicIpAddress(ipAddress.getAddress());
 			newFwRule.setPrivateIpAddress(userVM.getGuestIpAddress());
 			newFwRule.setGroupId(null);
+			
+			//get the domain router object
+	        final DomainRouterVO router = _routerMgr.getRouter(ipAddress.getAccountId(), ipAddress.getDataCenterId());
+			success = createOrDeleteIpForwardingRuleOnDomr(newFwRule,router,userVM.getGuestIpAddress(),true); //true +> create
+			
+			if(!success){
+				throw new PermissionDeniedException("Cannot create ip forwarding rule on domr");
+			}
+			
 			_rulesDao.persist(newFwRule);
 
 			// Save and create the event
@@ -3025,6 +3033,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
 		} catch (Exception e) {
 			s_logger.warn("Unable to create new firewall rule for 1:1 NAT");
 			txn.rollback();
+			throw new ServerApiException(BaseCmd.IP_ALLOCATION_ERROR,"Unable to create new firewall rule for 1:1 NAT:"+e.getMessage());
 		}finally{
 			if(locked)
 				_vmDao.releaseFromLockTable(userVM.getId());
@@ -3085,14 +3094,10 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
 
             locked = true;
             txn.start();
-            //if there is an open port range associated with the rule, don't allow deletion
-            List<IprulePortrangeMapVO> portRecordsForRule = _iprulePortrangeMapDao.listPortRecordsForRule(ruleId);
             
-            if (portRecordsForRule != null && portRecordsForRule.size() > 0) {
+	        final DomainRouterVO router = _routerMgr.getRouter(ipAddress.getAccountId(), ipAddress.getDataCenterId());
-                throw new InvalidParameterValueException("Cannot delete rule as port mappings for this rule exist; please delete those mappings first");
+            success = createOrDeleteIpForwardingRuleOnDomr(rule, router, rule.getPrivateIpAddress(), false);
-            } 
+            _firewallRulesDao.remove(ruleId);
-            
-            success = _firewallRulesDao.remove(ruleId);
             
             String description;
             String type = EventTypes.EVENT_NET_RULE_DELETE;
@@ -3121,4 +3126,24 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
         }
         return success;
     }
+    
+    private boolean  createOrDeleteIpForwardingRuleOnDomr(FirewallRuleVO fwRule, DomainRouterVO router, String guestIp, boolean create){
+    	
+        Commands cmds = new Commands(OnError.Continue);
+        final SetFirewallRuleCommand cmd = new SetFirewallRuleCommand(router.getInstanceName(), router.getPrivateIpAddress(), true, fwRule, create);
+        cmds.addCommand(cmd);       
+        try {
+            _agentMgr.send(router.getHostId(), cmds);
+        } catch (final AgentUnavailableException e) {
+            s_logger.warn("agent unavailable", e);
+        } catch (final OperationTimedoutException e) {
+            s_logger.warn("Timed Out", e);
+        }
+        Answer[] answers = cmds.getAnswers();
+        if (answers == null || answers[0].getResult() == false ){
+            return false;
+        }else{
+        	return true;
+        }
+    }
 }