From 1faaba8fb0df87f1beb91e12b8add9803b505bb1 Mon Sep 17 00:00:00 2001 From: abhishek Date: Thu, 11 Nov 2010 12:48:06 -0800 Subject: [PATCH] fixed a corner case; also changed the commands to be async and changed the method signatures to conform with the master refactor --- .../systemvm/debian/config/root/firewall.sh | 11 ++- .../src/com/cloud/api/BaseAsyncCreateCmd.java | 1 + .../commands/CreateIpForwardingRuleCmd.java | 51 +++++++--- .../commands/DeleteIpForwardingRuleCmd.java | 26 +++-- .../src/com/cloud/network/NetworkManager.java | 7 +- .../com/cloud/network/NetworkManagerImpl.java | 97 ++++++++++++------- 6 files changed, 132 insertions(+), 61 deletions(-) diff --git a/patches/systemvm/debian/config/root/firewall.sh b/patches/systemvm/debian/config/root/firewall.sh index 35f29d8d41d..d8f698b2ba3 100755 --- a/patches/systemvm/debian/config/root/firewall.sh +++ b/patches/systemvm/debian/config/root/firewall.sh @@ -77,7 +77,16 @@ add_one_to_one_nat_entry() { local publicIp=$2 local dIp=$3 local op=$4 - iptables -t nat $op PREROUTING -i eth2 -d $publicIp -j DNAT --to-destination $guestIp + if [ "$op" == "-D" ] + then + iptables -t nat $op PREROUTING -i eth2 -d $publicIp -j DNAT --to-destination $guestIp + if [ $? -gt 0 ] + then + return 0 + fi + else + iptables -t nat $op PREROUTING -i eth2 -d $publicIp -j DNAT --to-destination $guestIp + fi iptables -t nat $op POSTROUTING -o eth2 -s $guestIp -j SNAT --to-source $publicIp if [ "$op" == "-A" ] then diff --git a/server/src/com/cloud/api/BaseAsyncCreateCmd.java b/server/src/com/cloud/api/BaseAsyncCreateCmd.java index 32a932908a6..6739fd08599 100644 --- a/server/src/com/cloud/api/BaseAsyncCreateCmd.java +++ b/server/src/com/cloud/api/BaseAsyncCreateCmd.java @@ -6,6 +6,7 @@ import com.cloud.exception.ConcurrentOperationException; import com.cloud.exception.InsufficientAddressCapacityException; import com.cloud.exception.InsufficientCapacityException; import com.cloud.exception.InvalidParameterValueException; +import com.cloud.exception.NetworkRuleConflictException; import com.cloud.exception.PermissionDeniedException; import com.cloud.exception.ResourceAllocationException; import com.cloud.exception.ResourceUnavailableException; diff --git a/server/src/com/cloud/api/commands/CreateIpForwardingRuleCmd.java b/server/src/com/cloud/api/commands/CreateIpForwardingRuleCmd.java index 51439062aab..118f3f0b79e 100644 --- a/server/src/com/cloud/api/commands/CreateIpForwardingRuleCmd.java +++ b/server/src/com/cloud/api/commands/CreateIpForwardingRuleCmd.java @@ -22,21 +22,24 @@ import org.apache.log4j.Logger; import com.cloud.api.ApiConstants; import com.cloud.api.ApiResponseHelper; -import com.cloud.api.BaseCmd; +import com.cloud.api.BaseAsyncCreateCmd; import com.cloud.api.Implementation; import com.cloud.api.Parameter; import com.cloud.api.ServerApiException; import com.cloud.api.response.FirewallRuleResponse; +import com.cloud.event.EventTypes; import com.cloud.exception.ConcurrentOperationException; import com.cloud.exception.InsufficientAddressCapacityException; import com.cloud.exception.InsufficientCapacityException; import com.cloud.exception.InvalidParameterValueException; -import com.cloud.exception.NetworkRuleConflictException; import com.cloud.exception.PermissionDeniedException; +import com.cloud.exception.ResourceAllocationException; +import com.cloud.exception.ResourceUnavailableException; import com.cloud.network.FirewallRuleVO; +import com.cloud.user.Account; @Implementation(description="Creates an ip forwarding rule") -public class CreateIpForwardingRuleCmd extends BaseCmd { +public class CreateIpForwardingRuleCmd extends BaseAsyncCreateCmd { public static final Logger s_logger = Logger.getLogger(CreateIpForwardingRuleCmd.class.getName()); private static final String s_name = "createipforwardingruleresponse"; @@ -75,19 +78,37 @@ public class CreateIpForwardingRuleCmd extends BaseCmd { } @Override - public void execute() throws ServerApiException, InvalidParameterValueException, PermissionDeniedException, InsufficientAddressCapacityException, InsufficientCapacityException, ConcurrentOperationException{ - try { - FirewallRuleVO result = _networkMgr.createIpForwardingRule(this); - if (result != null) { - FirewallRuleResponse fwResponse = ApiResponseHelper.createFirewallRuleResponse(result); - fwResponse.setResponseName(getName()); - this.setResponseObject(fwResponse); - } else { - //throw new ServerApiException(NET_CREATE_IPFW_RULE_ERROR, "An existing rule for ipAddress / port / protocol of " + ipAddress + " / " + publicPort + " / " + protocol + " exits."); - } - } catch (NetworkRuleConflictException ex) { - throw new ServerApiException(BaseCmd.INTERNAL_ERROR, ex.getMessage()); + public void execute() throws ServerApiException, InvalidParameterValueException, PermissionDeniedException, InsufficientAddressCapacityException, InsufficientCapacityException, ConcurrentOperationException{ + FirewallRuleVO result = _networkMgr.createIpForwardingRuleOnDomr(this.getId()); + if (result != null) { + FirewallRuleResponse fwResponse = ApiResponseHelper.createFirewallRuleResponse(result); + fwResponse.setResponseName(getName()); + this.setResponseObject(fwResponse); + } else { + throw new ServerApiException(NET_CREATE_IPFW_RULE_ERROR, "Error in creating ip forwarding rule on the domr"); } + + } + + @Override + public void callCreate() throws ServerApiException,InvalidParameterValueException, PermissionDeniedException,InsufficientAddressCapacityException,InsufficientCapacityException, ResourceUnavailableException,ConcurrentOperationException, ResourceAllocationException{ + FirewallRuleVO rule = _networkMgr.createIpForwardingRuleInDb(ipAddress,virtualMachineId); + this.setId(rule.getId()); + } + + @Override + public long getAccountId() { + return Account.ACCOUNT_ID_SYSTEM; // no account info given, parent this command to SYSTEM so ERROR events are tracked + } + + @Override + public String getEventType() { + return EventTypes.EVENT_NET_RULE_ADD; + } + + @Override + public String getEventDescription() { + return ("Creating an ipforwarding 1:1 NAT rule for "+ipAddress+" with virtual machine:"+virtualMachineId); } } diff --git a/server/src/com/cloud/api/commands/DeleteIpForwardingRuleCmd.java b/server/src/com/cloud/api/commands/DeleteIpForwardingRuleCmd.java index 8f77eb1b57b..b3f475dea56 100644 --- a/server/src/com/cloud/api/commands/DeleteIpForwardingRuleCmd.java +++ b/server/src/com/cloud/api/commands/DeleteIpForwardingRuleCmd.java @@ -21,23 +21,22 @@ package com.cloud.api.commands; import org.apache.log4j.Logger; import com.cloud.api.ApiConstants; -import com.cloud.api.ApiResponseHelper; +import com.cloud.api.BaseAsyncCmd; import com.cloud.api.BaseCmd; import com.cloud.api.Implementation; import com.cloud.api.Parameter; import com.cloud.api.ServerApiException; -import com.cloud.api.response.FirewallRuleResponse; import com.cloud.api.response.SuccessResponse; +import com.cloud.event.EventTypes; import com.cloud.exception.ConcurrentOperationException; import com.cloud.exception.InsufficientAddressCapacityException; import com.cloud.exception.InsufficientCapacityException; import com.cloud.exception.InvalidParameterValueException; -import com.cloud.exception.NetworkRuleConflictException; import com.cloud.exception.PermissionDeniedException; -import com.cloud.network.FirewallRuleVO; +import com.cloud.user.Account; @Implementation(description="Deletes an ip forwarding rule") -public class DeleteIpForwardingRuleCmd extends BaseCmd { +public class DeleteIpForwardingRuleCmd extends BaseAsyncCmd { public static final Logger s_logger = Logger.getLogger(DeleteIpForwardingRuleCmd.class.getName()); private static final String s_name = "deleteipforwardingruleresponse"; @@ -70,7 +69,7 @@ public class DeleteIpForwardingRuleCmd extends BaseCmd { public void execute() throws ServerApiException, InvalidParameterValueException, PermissionDeniedException, InsufficientAddressCapacityException, InsufficientCapacityException, ConcurrentOperationException{ try { boolean result = false; - result = _networkMgr.deleteIpForwardingRule(this); + result = _networkMgr.deleteIpForwardingRule(id); if (result) { SuccessResponse response = new SuccessResponse(getName()); this.setResponseObject(response); @@ -82,4 +81,19 @@ public class DeleteIpForwardingRuleCmd extends BaseCmd { } } + @Override + public long getAccountId() { + return Account.ACCOUNT_ID_SYSTEM; // no account info given, parent this command to SYSTEM so ERROR events are tracked + } + + @Override + public String getEventType() { + return EventTypes.EVENT_NET_RULE_ADD; + } + + @Override + public String getEventDescription() { + return ("Deleting an ipforwarding 1:1 NAT rule id:"+id); + } + } diff --git a/server/src/com/cloud/network/NetworkManager.java b/server/src/com/cloud/network/NetworkManager.java index b01de8ba1dc..511012efb24 100644 --- a/server/src/com/cloud/network/NetworkManager.java +++ b/server/src/com/cloud/network/NetworkManager.java @@ -20,6 +20,7 @@ package com.cloud.network; import java.util.List; import java.util.Map; +import com.cloud.api.ServerApiException; import com.cloud.api.commands.AddVpnUserCmd; import com.cloud.api.commands.AssignToLoadBalancerRuleCmd; import com.cloud.api.commands.AssociateIPAddrCmd; @@ -362,9 +363,11 @@ public interface NetworkManager { String getNextAvailableMacAddressInNetwork(long networkConfigurationId) throws InsufficientAddressCapacityException; String getNextAvailableMacAddressInNetwork(long networkConfigurationId); - FirewallRuleVO createIpForwardingRule(CreateIpForwardingRuleCmd cmd) throws InvalidParameterValueException, PermissionDeniedException, NetworkRuleConflictException; + FirewallRuleVO createIpForwardingRuleInDb(String ipAddr, Long virtualMachineId) throws ServerApiException; public boolean deletePortForwardingRule(DeletePortForwardingRuleCmd cmd); - public boolean deleteIpForwardingRule(DeleteIpForwardingRuleCmd cmd); + FirewallRuleVO createIpForwardingRuleOnDomr(Long ruleId); + + boolean deleteIpForwardingRule(Long id); } diff --git a/server/src/com/cloud/network/NetworkManagerImpl.java b/server/src/com/cloud/network/NetworkManagerImpl.java index fc40614e1cc..cc7f193ad33 100755 --- a/server/src/com/cloud/network/NetworkManagerImpl.java +++ b/server/src/com/cloud/network/NetworkManagerImpl.java @@ -167,6 +167,7 @@ import com.cloud.vm.UserVmVO; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachineProfile; +import com.cloud.vm.dao.DomainRouterDao; import com.cloud.vm.dao.NicDao; import com.cloud.vm.dao.UserVmDao; @@ -2938,26 +2939,69 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag return _networkConfigDao.findById(id); } + @Override + public FirewallRuleVO createIpForwardingRuleOnDomr(Long ruleId) throws ServerApiException{ + boolean success = false; + //get the rule + FirewallRuleVO rule = _rulesDao.findById(ruleId); + + if(rule == null){ + throw new PermissionDeniedException("Cannot create ip forwarding rule in db"); + } + + //get ip address + IPAddressVO ipAddress = _ipAddressDao.findById(rule.getPublicIpAddress()); + if (ipAddress == null) { + throw new InvalidParameterValueException("Unable to create ip forwarding rule on address " + ipAddress + ", invalid IP address specified."); + } + + //get the domain router object + DomainRouterVO router = _routerMgr.getRouter(ipAddress.getAccountId(), ipAddress.getDataCenterId()); + success = createOrDeleteIpForwardingRuleOnDomr(rule,router,rule.getPrivateIpAddress(),true); //true +> create + + if(!success){ + //corner case; delete record from db as domR rule creation failed + try { + _rulesDao.remove(ruleId); + throw new PermissionDeniedException("Cannot create ip forwarding rule on domr, hence deleting created record in db"); + } catch (Exception e) { + throw new ServerApiException(BaseCmd.NET_CREATE_IPFW_RULE_ERROR, e.getMessage()); + } + } + + // Save and create the event + String description; + String ruleName = "ip forwarding"; + String level = EventVO.LEVEL_INFO; + + description = "created new " + ruleName + " rule [" + rule.getPublicIpAddress() + "]->[" + + rule.getPrivateIpAddress() + "]" + ":" + rule.getProtocol(); + + EventUtils.saveEvent(UserContext.current().getUserId(), ipAddress.getAccountId(), level, EventTypes.EVENT_NET_RULE_ADD, description); + + return rule; + + } + @Override @DB - public FirewallRuleVO createIpForwardingRule(CreateIpForwardingRuleCmd cmd) throws InvalidParameterValueException, PermissionDeniedException, NetworkRuleConflictException { + public FirewallRuleVO createIpForwardingRuleInDb(String ipAddr, Long virtualMachineId) throws ServerApiException { Transaction txn = Transaction.currentTxn(); txn.start(); UserVmVO userVM = null; FirewallRuleVO newFwRule = null; boolean locked = false; - boolean success = false; try { // validate IP Address exists - IPAddressVO ipAddress = _ipAddressDao.findById(cmd.getIpAddress()); + IPAddressVO ipAddress = _ipAddressDao.findById(ipAddr); if (ipAddress == null) { throw new InvalidParameterValueException("Unable to create ip forwarding rule on address " + ipAddress + ", invalid IP address specified."); } // validate user VM exists - userVM = _vmDao.findById(cmd.getVirtualMachineId()); + userVM = _vmDao.findById(virtualMachineId); if (userVM == null) { - throw new InvalidParameterValueException("Unable to create ip forwarding rule on address " + ipAddress + ", invalid virtual machine id specified (" + cmd.getVirtualMachineId() + ")."); + throw new InvalidParameterValueException("Unable to create ip forwarding rule on address " + ipAddress + ", invalid virtual machine id specified (" + virtualMachineId + ")."); } //sync point; cannot lock on rule ; hence sync on vm @@ -2985,18 +3029,18 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag if (account != null) { if ((account.getType() == Account.ACCOUNT_TYPE_ADMIN) || (account.getType() == Account.ACCOUNT_TYPE_DOMAIN_ADMIN)) { if (!_domainDao.isChildDomain(account.getDomainId(), userVM.getDomainId())) { - throw new PermissionDeniedException("Unable to create ip forwarding rule, IP address " + ipAddress + " to virtual machine " + cmd.getVirtualMachineId() + ", permission denied."); + throw new PermissionDeniedException("Unable to create ip forwarding rule, IP address " + ipAddress + " to virtual machine " + virtualMachineId + ", permission denied."); } } else if (account.getId() != userVM.getAccountId()) { - throw new PermissionDeniedException("Unable to create ip forwarding rule, IP address " + ipAddress + " to virtual machine " + cmd.getVirtualMachineId() + ", permission denied."); + throw new PermissionDeniedException("Unable to create ip forwarding rule, IP address " + ipAddress + " to virtual machine " + virtualMachineId + ", permission denied."); } } // check for ip address/port conflicts by checking existing forwarding and load balancing rules - List existingNatRules = _rulesDao.findByPublicIpPrivateIpForNatRule(cmd.getIpAddress(), userVM.getGuestIpAddress()); + List existingNatRules = _rulesDao.findByPublicIpPrivateIpForNatRule(ipAddr, userVM.getGuestIpAddress()); if(existingNatRules.size() > 0){ - throw new NetworkRuleConflictException("The specified rule for public ip:"+cmd.getIpAddress()+" vm id:"+cmd.getVirtualMachineId()+" already exists"); + throw new NetworkRuleConflictException("The specified rule for public ip:"+ipAddr+" vm id:"+virtualMachineId+" already exists"); } newFwRule = new FirewallRuleVO(); @@ -3009,26 +3053,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag 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 - String description; - String ruleName = "ip forwarding"; - String level = EventVO.LEVEL_INFO; - - description = "created new " + ruleName + " rule [" + newFwRule.getPublicIpAddress() + ":" + newFwRule.getPublicPort() + "]->[" - + newFwRule.getPrivateIpAddress() + ":" + newFwRule.getPrivatePort() + "]" + " " + newFwRule.getProtocol(); - - EventUtils.saveEvent(UserContext.current().getUserId(), userVM.getAccountId(), level, EventTypes.EVENT_NET_RULE_ADD, description); - + _rulesDao.persist(newFwRule); txn.commit(); } catch (Exception e) { s_logger.warn("Unable to create new firewall rule for 1:1 NAT"); @@ -3043,8 +3068,8 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag } @Override @DB - public boolean deleteIpForwardingRule(DeleteIpForwardingRuleCmd cmd) { - Long ruleId = cmd.getId(); + public boolean deleteIpForwardingRule(Long id) { + Long ruleId = id; Long userId = UserContext.current().getUserId(); Account account = UserContext.current().getAccount(); @@ -3087,8 +3112,8 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag boolean success = false; try { - rule = _firewallRulesDao.acquireInLockTable(ruleId); - if (rule == null) { + ipAddress = _ipAddressDao.acquireInLockTable(publicIp); + if (ipAddress == null) { throw new PermissionDeniedException("Unable to obtain lock on record for deletion"); } @@ -3105,12 +3130,10 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag String ruleName = rule.isForwarding() ? "ip forwarding" : "load balancer"; if (success) { - description = "deleted " + ruleName + " rule [" + publicIp + ":" + rule.getPublicPort() + "]->[" + rule.getPrivateIpAddress() + ":" - + rule.getPrivatePort() + "] " + rule.getProtocol(); + description = "deleted " + ruleName + " rule [" + publicIp +"]->[" + rule.getPrivateIpAddress() + "] " + rule.getProtocol(); } else { level = EventVO.LEVEL_ERROR; - description = "Error while deleting " + ruleName + " rule [" + publicIp + ":" + rule.getPublicPort() + "]->[" + rule.getPrivateIpAddress() + ":" - + rule.getPrivatePort() + "] " + rule.getProtocol(); + description = "Error while deleting " + ruleName + " rule [" + publicIp + "]->[" + rule.getPrivateIpAddress() +"] " + rule.getProtocol(); } EventUtils.saveEvent(userId, ipAddress.getAccountId(), level, type, description); txn.commit();