From 62b3d548d647757b73df1f0d47de13cedbf406c0 Mon Sep 17 00:00:00 2001 From: alena Date: Sun, 27 Feb 2011 23:48:40 -0800 Subject: [PATCH] bug 8753: never release public Ip address without ensuring that all corresponding resources (PF/StaticNat/Lb rules) are cleaned up. Fixed couple of other problems along: * when expunge PF/Static nat rules as a part of vmExpunge/IpRelease process, first mark all rules as Revoke, and then send commands to the backend. Group commands by Ip address. Before we used to do Revoke/Send per rule basis. * When release source nat rule, make sure that corresponding vpn (if exists) is being expunged. --- .../api/commands/DisassociateIPAddrCmd.java | 3 + .../com/cloud/network/rules/RulesService.java | 2 +- .../com/cloud/network/NetworkManagerImpl.java | 160 +++++++++++++----- .../cloud/network/dao/FirewallRulesDao.java | 7 +- .../network/dao/FirewallRulesDaoImpl.java | 31 +++- .../com/cloud/network/dao/IPAddressDao.java | 2 +- .../cloud/network/dao/IPAddressDaoImpl.java | 4 +- .../network/lb/LoadBalancingRulesManager.java | 3 +- .../lb/LoadBalancingRulesManagerImpl.java | 24 ++- .../com/cloud/network/rules/RulesManager.java | 4 +- .../cloud/network/rules/RulesManagerImpl.java | 156 ++++++++++------- .../rules/dao/PortForwardingRulesDao.java | 6 +- .../rules/dao/PortForwardingRulesDaoImpl.java | 15 +- .../com/cloud/user/AccountManagerImpl.java | 4 +- .../src/com/cloud/vm/UserVmManagerImpl.java | 93 +++++----- 15 files changed, 347 insertions(+), 167 deletions(-) diff --git a/api/src/com/cloud/api/commands/DisassociateIPAddrCmd.java b/api/src/com/cloud/api/commands/DisassociateIPAddrCmd.java index 9008d0fd506..5cd274e796b 100644 --- a/api/src/com/cloud/api/commands/DisassociateIPAddrCmd.java +++ b/api/src/com/cloud/api/commands/DisassociateIPAddrCmd.java @@ -92,6 +92,9 @@ public class DisassociateIPAddrCmd extends BaseAsyncCmd { public long getEntityOwnerId() { if (ownerId == null) { IpAddress ip = getIpAddress(id); + if (ip == null) { + throw new InvalidParameterValueException("Unable to find ip address by id=" + id); + } ownerId = ip.getAccountId(); } return ownerId; diff --git a/api/src/com/cloud/network/rules/RulesService.java b/api/src/com/cloud/network/rules/RulesService.java index a538a069b9a..6898053b5fd 100644 --- a/api/src/com/cloud/network/rules/RulesService.java +++ b/api/src/com/cloud/network/rules/RulesService.java @@ -55,7 +55,7 @@ public interface RulesService { boolean enableOneToOneNat(long ipAddressId, long vmId) throws NetworkRuleConflictException; - boolean disableOneToOneNat(long ipAddressId); + boolean disableOneToOneNat(long ipAddressId) throws ResourceUnavailableException; PortForwardingRule getPortForwardigRule(long ruleId); FirewallRule getFirewallRule(long ruleId); diff --git a/server/src/com/cloud/network/NetworkManagerImpl.java b/server/src/com/cloud/network/NetworkManagerImpl.java index 2b7f5d836d9..be92154289d 100755 --- a/server/src/com/cloud/network/NetworkManagerImpl.java +++ b/server/src/com/cloud/network/NetworkManagerImpl.java @@ -440,8 +440,8 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag List publicIps = new ArrayList(); if (userIps != null && !userIps.isEmpty()) { for (IPAddressVO userIp : userIps) { - PublicIp publicIp = new PublicIp(userIp, _vlanDao.findById(userIp.getVlanId()), userIp.getMacAddress()); - publicIps.add(publicIp); + PublicIp publicIp = new PublicIp(userIp, _vlanDao.findById(userIp.getVlanId()), userIp.getMacAddress()); + publicIps.add(publicIp); } } @@ -468,7 +468,13 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag markPublicIpAsAllocated(addr); } else if (addr.getState() == IpAddress.State.Releasing) { - unassignPublicIpAddress(addr); + //Cleanup all the resources for ip address if there are any, and only then unassign ip in the system + if (cleanupIpResources(addr.getId(), Account.ACCOUNT_ID_SYSTEM, _accountMgr.getSystemAccount())) { + unassignPublicIpAddress(addr); + } else { + success = false; + s_logger.warn("Failed to release resources for ip address id=" + addr.getId()); + } } } } @@ -608,8 +614,10 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag @Override public boolean releasePublicIpAddress(long addrId, long userId, Account caller) { + + //mark ip address as Releasing IPAddressVO ip = _ipAddressDao.markAsUnavailable(addrId); - assert (ip != null) : "Unable to mark the ip address id=" + addrId + " owned by " + ip.getAccountId() + " as unavailable."; + assert (ip != null) : "Unable to mark the ip address id=" + addrId + " as unavailable."; if (ip == null) { return true; } @@ -619,22 +627,16 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag } boolean success = true; - try { - if (!_rulesMgr.revokeAllRules(addrId, userId, caller)) { - s_logger.warn("Unable to revoke all the port forwarding rules for ip " + ip); - success = false; - } - } catch (ResourceUnavailableException e) { - s_logger.warn("Unable to revoke all the port forwarding rules for ip " + ip, e); + + //Cleanup all ip address resources - PF/LB/Static nat rules + if (cleanupIpResources(addrId, userId, caller)) { + unassignPublicIpAddress(ip); + } else { success = false; + s_logger.warn("Failed to release resources for ip address id=" + addrId); } - if (!_lbMgr.removeAllLoadBalanacers(addrId, caller, userId)) { - s_logger.warn("Unable to revoke all the load balancer rules for ip " + ip); - success = false; - } - - if (ip.getAssociatedWithNetworkId() != null) { + if (ip.getAssociatedWithNetworkId() != null) { Network network = _networksDao.findById(ip.getAssociatedWithNetworkId()); try { if (!applyIpAssociations(network, true)) { @@ -642,7 +644,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag success = false; } } catch (ResourceUnavailableException e) { - throw new CloudRuntimeException("We should nver get to here because we used true when applyIpAssociations", e); + throw new CloudRuntimeException("We should never get to here because we used true when applyIpAssociations", e); } } @@ -1828,7 +1830,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag List userVms = _vmDao.listByNetworkId(networkId); for (UserVmVO vm : userVms) { - if (!(vm.getState() == VirtualMachine.State.Error || vm.getState() == VirtualMachine.State.Expunging)) { + if (!(vm.getState() == VirtualMachine.State.Error || (vm.getState() == VirtualMachine.State.Expunging && vm.getRemoved() != null))) { throw new InvalidParameterValueException("Can't delete the network, not all user vms are expunged"); } } @@ -1907,8 +1909,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag txn.commit(); } - @DB - @Override + @Override @DB public boolean destroyNetwork(long networkId, ReservationContext context) { Account callerAccount = _accountMgr.getAccount(context.getCaller().getAccountId()); @@ -1930,17 +1931,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag boolean success = true; - // release ip addresses associated with the network if there are any - for Virtual case - List ipsToRelease = _ipAddressDao.listByAssociatedNetwork(networkId); - if (ipsToRelease != null && !ipsToRelease.isEmpty()) { - for (IPAddressVO ip : ipsToRelease) { - //delete load balancer rules associated with the ip address before unassigning it - _lbMgr.removeAllLoadBalanacers(ip.getId(), callerAccount, context.getCaller().getId()); - unassignPublicIpAddress(ip); - } - - s_logger.debug("Ip addresses associated with network " + networkId + " are unassigned successfully as a part of network id=" + networkId + " destroy"); - } + cleanupNetworkResources(networkId, callerAccount, context.getCaller().getId()); for (NetworkElement element : _networkElements) { try { @@ -1985,6 +1976,52 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag return success; } + + private boolean cleanupNetworkResources(long networkId, Account caller, long callerUserId) { + boolean success = true; + Network network = getNetwork(networkId); + + //remove all PF/Static Nat rules for the network + try { + if (_rulesMgr.revokeAllRulesForNetwork(networkId, callerUserId, caller)) { + s_logger.debug("Successfully cleaned up portForwarding/staticNat rules for network id=" + networkId); + } else { + success = false; + s_logger.warn("Failed to release portForwarding/StaticNat rules as a part of network id=" + networkId + " cleanup"); + } + } catch (ResourceUnavailableException ex) { + success = false; + //shouldn't even come here as network is being cleaned up after all network elements are shutdown + s_logger.warn("Failed to release portForwarding/StaticNat rules as a part of network id=" + networkId + " cleanup due to resourceUnavailable ", ex); + } + + //remove all LB rules for the network + if (_lbMgr.removeAllLoadBalanacersForNetwork(networkId, caller, callerUserId)) { + s_logger.debug("Successfully cleaned up load balancing rules for network id=" + networkId); + } else { + //shouldn't even come here as network is being cleaned up after all network elements are shutdown + success = false; + s_logger.warn("Failed to cleanup LB rules as a part of network id=" + networkId + " cleanup"); + } + + //release all ip addresses + List ipsToRelease = _ipAddressDao.listByAssociatedNetwork(networkId); + for (IPAddressVO ipToRelease : ipsToRelease) { + IPAddressVO ip = _ipAddressDao.markAsUnavailable(ipToRelease.getId()); + assert (ip != null) : "Unable to mark the ip address id=" + ipToRelease.getId() + " as unavailable."; + } + + try { + if (!applyIpAssociations(network, true)) { + s_logger.warn("Unable to apply ip address associations for " + network); + success = false; + } + } catch (ResourceUnavailableException e) { + throw new CloudRuntimeException("We should never get to here because we used true when applyIpAssociations", e); + } + + return success; + } private boolean deleteVlansInNetwork(long networkId, long userId) { List vlans = _vlanDao.listVlansByNetworkId(networkId); @@ -2080,7 +2117,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag @Override @ActionEvent (eventType=EventTypes.EVENT_NETWORK_RESTART, eventDescription="restarting network", async=true) public boolean restartNetwork(RestartNetworkCmd cmd) throws ConcurrentOperationException, ResourceUnavailableException, InsufficientCapacityException { - // This method restarts all network elements belonging to the network + // This method restarts all network elements belonging to the network and re-applies all the rules Long networkId = cmd.getNetworkId(); NetworkVO network = _networksDao.findById(networkId); Account owner = _accountMgr.getAccount(network.getAccountId()); @@ -2090,51 +2127,61 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag ReservationContext context = new ReservationContextImpl(null, null, caller, owner); _accountMgr.checkAccess(callerAccount, network); + + boolean success = true; s_logger.debug("Restarting network " + networkId + "..."); for (NetworkElement element : _networkElements) { //stop and start the network element if (!element.restart(network, context)) { s_logger.warn("Failed to restart network element(s) as a part of network id" + networkId + " restart"); - return false; + success = false; } } //associate all ip addresses if (!applyIpAssociations(network, false)) { s_logger.warn("Failed to apply ip addresses as a part of network id" + networkId + " restart"); - return false; + success = false; } //apply port forwarding rules if (!_rulesMgr.applyPortForwardingRulesForNetwork(networkId, false, context.getAccount())) { s_logger.warn("Failed to reapply port forwarding rule(s) as a part of network id=" + networkId + " restart"); + success = false; } //apply static nat rules if (!_rulesMgr.applyStaticNatRulesForNetwork(networkId, false, context.getAccount())) { s_logger.warn("Failed to reapply static nat rule(s) as a part of network id=" + networkId + " restart"); + success = false; } //apply load balancer rules if (!_lbMgr.applyLoadBalancersForNetwork(networkId)) { s_logger.warn("Failed to reapply load balancer rules as a part of network id=" + networkId + " restart"); - return false; + success = false; } //apply vpn rules List vpnsToReapply = _vpnMgr.listRemoteAccessVpns(networkId); if (vpnsToReapply != null) { for (RemoteAccessVpn vpn : vpnsToReapply) { + //Start remote access vpn per ip if (_vpnMgr.startRemoteAccessVpn(vpn.getServerAddressId()) == null) { - s_logger.warn("Failed to reapply load balancer rules as a part of network id=" + networkId + " restart"); - return false; + s_logger.warn("Failed to reapply vpn rules as a part of network id=" + networkId + " restart"); + success = false; } } } - s_logger.debug("Network id=" + networkId + " is restarted successfully."); - return true; + if (success) { + s_logger.debug("Network id=" + networkId + " is restarted successfully."); + } else { + s_logger.warn("Network id=" + networkId + " failed to restart."); + } + + return success; } @Override @@ -2451,4 +2498,37 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag return false; } + + private boolean cleanupIpResources(long ipId, long userId, Account caller) { + boolean success = true; + + try { + s_logger.debug("Revoking all PF/StaticNat rules as a part of public IP id=" + ipId + " release..."); + if (!_rulesMgr.revokeAllRulesForIp(ipId, userId, caller)) { + s_logger.warn("Unable to revoke all the port forwarding rules for ip id=" + ipId + " as a part of ip release"); + success = false; + } + } catch (ResourceUnavailableException e) { + s_logger.warn("Unable to revoke all the port forwarding rules for ip id=" + ipId + " as a part of ip release", e); + success = false; + } + + s_logger.debug("Revoking all LB rules as a part of public IP id=" + ipId + " release..."); + if (!_lbMgr.removeAllLoadBalanacersForIp(ipId, caller, userId)) { + s_logger.warn("Unable to revoke all the load balancer rules for ip id=" + ipId + " as a part of ip release"); + success = false; + } + + //remote access vpn can be enabled only for static nat ip, so this part should never be executed under normal conditions + //only when ip address failed to be cleaned up as a part of account destroy and was marked as Releasing, this part of the code would be triggered + s_logger.debug("Cleaning up remote access vpns as a part of public IP id=" + ipId + " release..."); + try { + _vpnMgr.destroyRemoteAccessVpn(ipId); + } catch (ResourceUnavailableException e) { + s_logger.warn("Unable to destroy remote access vpn for ip id=" + ipId + " as a part of ip release", e); + success = false; + } + + return success; + } } diff --git a/server/src/com/cloud/network/dao/FirewallRulesDao.java b/server/src/com/cloud/network/dao/FirewallRulesDao.java index 8e089af4ec6..d68fd907a19 100644 --- a/server/src/com/cloud/network/dao/FirewallRulesDao.java +++ b/server/src/com/cloud/network/dao/FirewallRulesDao.java @@ -28,7 +28,10 @@ import com.cloud.utils.db.GenericDao; * Data Access Object for user_ip_address and ip_forwarding tables */ public interface FirewallRulesDao extends GenericDao { - List listByIpAndNotRevoked(long ipAddressId, FirewallRule.Purpose purpose); + + List listByIpAndPurposeAndNotRevoked(long ipAddressId, FirewallRule.Purpose purpose); + + List listByNetworkAndPurposeAndNotRevoked(long networkId, FirewallRule.Purpose purpose); boolean setStateToAdd(FirewallRuleVO rule); @@ -38,7 +41,7 @@ public interface FirewallRulesDao extends GenericDao { List listByIpAndPurpose(long ipAddressId, FirewallRule.Purpose purpose); - List listByNetworkIdAndPurpose(long networkId, FirewallRule.Purpose purpose); + List listByNetworkAndPurpose(long networkId, FirewallRule.Purpose purpose); List listStaticNatByVmId(long vmId); diff --git a/server/src/com/cloud/network/dao/FirewallRulesDaoImpl.java b/server/src/com/cloud/network/dao/FirewallRulesDaoImpl.java index 2d8d09bcfe7..ce65cd13ab5 100644 --- a/server/src/com/cloud/network/dao/FirewallRulesDaoImpl.java +++ b/server/src/com/cloud/network/dao/FirewallRulesDaoImpl.java @@ -42,7 +42,7 @@ public class FirewallRulesDaoImpl extends GenericDaoBase i private static final Logger s_logger = Logger.getLogger(FirewallRulesDaoImpl.class); protected final SearchBuilder AllFieldsSearch; - protected final SearchBuilder IpNotRevokedSearch; + protected final SearchBuilder NotRevokedSearch; protected final SearchBuilder ReleaseSearch; protected SearchBuilder VmSearch; @@ -60,11 +60,12 @@ public class FirewallRulesDaoImpl extends GenericDaoBase i AllFieldsSearch.and("networkId", AllFieldsSearch.entity().getNetworkId(), Op.EQ); AllFieldsSearch.done(); - IpNotRevokedSearch = createSearchBuilder(); - IpNotRevokedSearch.and("ipId", IpNotRevokedSearch.entity().getSourceIpAddressId(), Op.EQ); - IpNotRevokedSearch.and("state", IpNotRevokedSearch.entity().getState(), Op.NEQ); - IpNotRevokedSearch.and("purpose", IpNotRevokedSearch.entity().getPurpose(), Op.EQ); - IpNotRevokedSearch.done(); + NotRevokedSearch = createSearchBuilder(); + NotRevokedSearch.and("ipId", NotRevokedSearch.entity().getSourceIpAddressId(), Op.EQ); + NotRevokedSearch.and("state", NotRevokedSearch.entity().getState(), Op.NEQ); + NotRevokedSearch.and("purpose", NotRevokedSearch.entity().getPurpose(), Op.EQ); + NotRevokedSearch.and("networkId", NotRevokedSearch.entity().getNetworkId(), Op.EQ); + NotRevokedSearch.done(); ReleaseSearch = createSearchBuilder(); ReleaseSearch.and("protocol", ReleaseSearch.entity().getProtocol(), Op.EQ); @@ -96,8 +97,8 @@ public class FirewallRulesDaoImpl extends GenericDaoBase i } @Override - public List listByIpAndNotRevoked(long ipId, FirewallRule.Purpose purpose) { - SearchCriteria sc = IpNotRevokedSearch.create(); + public List listByIpAndPurposeAndNotRevoked(long ipId, FirewallRule.Purpose purpose) { + SearchCriteria sc = NotRevokedSearch.create(); sc.setParameters("ipId", ipId); sc.setParameters("state", State.Revoke); @@ -108,9 +109,21 @@ public class FirewallRulesDaoImpl extends GenericDaoBase i return listBy(sc); } + @Override + public List listByNetworkAndPurposeAndNotRevoked(long networkId, FirewallRule.Purpose purpose) { + SearchCriteria sc = NotRevokedSearch.create(); + sc.setParameters("networkId", networkId); + sc.setParameters("state", State.Revoke); + + if (purpose != null) { + sc.setParameters("purpose", purpose); + } + + return listBy(sc); + } @Override - public List listByNetworkIdAndPurpose(long networkId, FirewallRule.Purpose purpose) { + public List listByNetworkAndPurpose(long networkId, FirewallRule.Purpose purpose) { SearchCriteria sc = AllFieldsSearch.create(); sc.setParameters("purpose", purpose); sc.setParameters("networkId", networkId); diff --git a/server/src/com/cloud/network/dao/IPAddressDao.java b/server/src/com/cloud/network/dao/IPAddressDao.java index e2245bfbfbb..f1d78faba9b 100644 --- a/server/src/com/cloud/network/dao/IPAddressDao.java +++ b/server/src/com/cloud/network/dao/IPAddressDao.java @@ -46,7 +46,7 @@ public interface IPAddressDao extends GenericDao { int countIPsForDashboard(long dcId, boolean onlyCountAllocated); - List listByAssociatedVmId(long vmId); + IPAddressVO findByAssociatedVmId(long vmId); IPAddressVO findByAccountAndIp(long accountId, String ipAddress); } diff --git a/server/src/com/cloud/network/dao/IPAddressDaoImpl.java b/server/src/com/cloud/network/dao/IPAddressDaoImpl.java index acc7d8082cb..cd784a1b151 100644 --- a/server/src/com/cloud/network/dao/IPAddressDaoImpl.java +++ b/server/src/com/cloud/network/dao/IPAddressDaoImpl.java @@ -200,11 +200,11 @@ public class IPAddressDaoImpl extends GenericDaoBase implemen } @Override - public List listByAssociatedVmId(long vmId) { + public IPAddressVO findByAssociatedVmId(long vmId) { SearchCriteria sc = AllFieldsSearch.create(); sc.setParameters("associatedWithVmId", vmId); - return listBy(sc); + return findOneBy(sc); } @Override diff --git a/server/src/com/cloud/network/lb/LoadBalancingRulesManager.java b/server/src/com/cloud/network/lb/LoadBalancingRulesManager.java index 0bb1325ae6d..61d61429639 100644 --- a/server/src/com/cloud/network/lb/LoadBalancingRulesManager.java +++ b/server/src/com/cloud/network/lb/LoadBalancingRulesManager.java @@ -24,7 +24,8 @@ import com.cloud.network.lb.LoadBalancingRule.LbDestination; import com.cloud.user.Account; public interface LoadBalancingRulesManager extends LoadBalancingRulesService { - boolean removeAllLoadBalanacers(long ipId, Account caller, long callerUserId); + boolean removeAllLoadBalanacersForIp(long ipId, Account caller, long callerUserId); + boolean removeAllLoadBalanacersForNetwork(long networkId, Account caller, long callerUserId); List getExistingDestinations(long lbId); /** diff --git a/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java b/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java index b8480e898e0..342f8306070 100644 --- a/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java +++ b/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java @@ -302,7 +302,10 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager, if (apply) { try { - applyLoadBalancerConfig(loadBalancerId); + if (!applyLoadBalancerConfig(loadBalancerId)) { + s_logger.warn("Unable to apply the load balancer config"); + return false; + } } catch (ResourceUnavailableException e) { s_logger.warn("Unable to apply the load balancer config because resource is unavaliable.", e); return false; @@ -443,8 +446,23 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager, } @Override - public boolean removeAllLoadBalanacers(long ipId, Account caller, long callerUserId) { - List rules = _rulesDao.listByIpAndNotRevoked(ipId, Purpose.LoadBalancing); + public boolean removeAllLoadBalanacersForIp(long ipId, Account caller, long callerUserId) { + List rules = _rulesDao.listByIpAndPurposeAndNotRevoked(ipId, Purpose.LoadBalancing); + if (rules != null) + s_logger.debug("Found " + rules.size() + " lb rules to cleanup"); + for (FirewallRule rule : rules) { + boolean result = deleteLoadBalancerRule(rule.getId(), true, caller, callerUserId); + if (result == false) { + s_logger.warn("Unable to remove load balancer rule " + rule.getId()); + return false; + } + } + return true; + } + + @Override + public boolean removeAllLoadBalanacersForNetwork(long networkId, Account caller, long callerUserId) { + List rules = _rulesDao.listByNetworkAndPurposeAndNotRevoked(networkId, Purpose.LoadBalancing); if (rules != null) s_logger.debug("Found " + rules.size() + " lb rules to cleanup"); for (FirewallRule rule : rules) { diff --git a/server/src/com/cloud/network/rules/RulesManager.java b/server/src/com/cloud/network/rules/RulesManager.java index ee5fd9a497d..75a240fe453 100644 --- a/server/src/com/cloud/network/rules/RulesManager.java +++ b/server/src/com/cloud/network/rules/RulesManager.java @@ -63,7 +63,9 @@ public interface RulesManager extends RulesService { void checkIpAndUserVm(IpAddress ipAddress, UserVm userVm, Account caller) throws InvalidParameterValueException, PermissionDeniedException; void checkRuleAndUserVm(FirewallRule rule, UserVm userVm, Account caller) throws InvalidParameterValueException, PermissionDeniedException; - boolean revokeAllRules(long ipId, long userId, Account caller) throws ResourceUnavailableException; + boolean revokeAllRulesForIp(long ipId, long userId, Account caller) throws ResourceUnavailableException; + + boolean revokeAllRulesForNetwork(long networkId, long userId, Account caller) throws ResourceUnavailableException; List listFirewallRulesByIp(long ipAddressId); diff --git a/server/src/com/cloud/network/rules/RulesManagerImpl.java b/server/src/com/cloud/network/rules/RulesManagerImpl.java index a8ce2d7cc3c..d46d7eef1e9 100644 --- a/server/src/com/cloud/network/rules/RulesManagerImpl.java +++ b/server/src/com/cloud/network/rules/RulesManagerImpl.java @@ -18,8 +18,10 @@ package com.cloud.network.rules; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import javax.ejb.Local; import javax.naming.ConfigurationException; @@ -43,7 +45,6 @@ import com.cloud.network.IPAddressVO; import com.cloud.network.IpAddress; import com.cloud.network.Network; import com.cloud.network.Network.Capability; -import com.cloud.network.Network.GuestIpType; import com.cloud.network.Network.Service; import com.cloud.network.NetworkManager; import com.cloud.network.dao.FirewallRulesDao; @@ -91,7 +92,7 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { public void detectRulesConflict(FirewallRule newRule, IpAddress ipAddress) throws NetworkRuleConflictException { assert newRule.getSourceIpAddressId() == ipAddress.getId() : "You passed in an ip address that doesn't match the address in the new rule"; - List rules = _firewallDao.listByIpAndNotRevoked(newRule.getSourceIpAddressId(), null); + List rules = _firewallDao.listByIpAndPurposeAndNotRevoked(newRule.getSourceIpAddressId(), null); assert (rules.size() >= 1) : "For network rules, we now always first persist the rule and then check for network conflicts so we should at least have one rule at this point."; for (FirewallRuleVO rule : rules) { @@ -409,7 +410,7 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { } if (!ipAddress.isOneToOneNat()) { - List rules = _firewallDao.listByIpAndNotRevoked(ipId, Purpose.PortForwarding); + List rules = _firewallDao.listByIpAndPurposeAndNotRevoked(ipId, Purpose.PortForwarding); if (rules != null && !rules.isEmpty()) { throw new NetworkRuleConflictException("Failed to enable static nat for the ip address id=" + ipId + " as it already has firewall rules assigned"); } @@ -420,10 +421,10 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { } //If there is public ip address already associated with the vm, throw an exception - List ips = _ipAddressDao.listByAssociatedVmId(vmId); + IPAddressVO ip = _ipAddressDao.findByAssociatedVmId(vmId); - if (!ips.isEmpty()) { - throw new InvalidParameterValueException("Failed to enable static nat for the ip address id=" + ipId + " as vm id=" + " is already associated with ip id=" + ips.get(0).getId()); + if (ip != null) { + throw new InvalidParameterValueException("Failed to enable static nat for the ip address id=" + ipId + " as vm id=" + " is already associated with ip id=" + ip.getId()); } ipAddress.setOneToOneNat(true); @@ -540,6 +541,7 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { } List rules = _forwardingDao.listByVm(vmId); + Set ipsToReprogram = new HashSet(); if (rules == null || rules.isEmpty()) { s_logger.debug("No port forwarding rules are found for vm id=" + vmId); @@ -547,8 +549,17 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { } for (PortForwardingRuleVO rule : rules) { - revokePortForwardingRuleInternal(rule.getId(), _accountMgr.getSystemAccount(), Account.ACCOUNT_ID_SYSTEM, true); + //Mark port forwarding rule as Revoked, but don't revoke it yet (apply=false) + revokePortForwardingRuleInternal(rule.getId(), _accountMgr.getSystemAccount(), Account.ACCOUNT_ID_SYSTEM, false); + ipsToReprogram.add(rule.getSourceIpAddressId()); } + + //apply rules for all ip addresses + for (Long ipId : ipsToReprogram) { + s_logger.debug("Applying port forwarding rules for ip address id=" + ipId + " as a part of vm expunge"); + applyPortForwardingRules(ipId, true, _accountMgr.getSystemAccount()); + } + return true; } @@ -561,6 +572,7 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { } List rules = _firewallDao.listStaticNatByVmId(vm.getId()); + Set ipsToReprogram = new HashSet(); if (rules == null || rules.isEmpty()) { s_logger.debug("No static nat rules are found for vm id=" + vmId); @@ -568,8 +580,17 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { } for (FirewallRuleVO rule : rules) { - revokeStaticNatRuleInternal(rule.getId(), _accountMgr.getSystemAccount(), Account.ACCOUNT_ID_SYSTEM, true); + //mark static nat as Revoked, but don't revoke it yet (apply = false) + revokeStaticNatRuleInternal(rule.getId(), _accountMgr.getSystemAccount(), Account.ACCOUNT_ID_SYSTEM, false); + ipsToReprogram.add(rule.getSourceIpAddressId()); } + + //apply rules for all ip addresses + for (Long ipId : ipsToReprogram) { + s_logger.debug("Applying static nat rules for ip address id=" + ipId + " as a part of vm expunge"); + applyStaticNatRules(ipId, true, _accountMgr.getSystemAccount()); + } + return true; } @@ -744,7 +765,7 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { @Override public boolean applyStaticNatRulesForNetwork(long networkId, boolean continueOnError, Account caller){ - List rules = _firewallDao.listByNetworkIdAndPurpose(networkId, Purpose.StaticNat); + List rules = _firewallDao.listByNetworkAndPurpose(networkId, Purpose.StaticNat); List staticNatRules = new ArrayList(); if (rules.size() == 0) { @@ -756,15 +777,8 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { _accountMgr.checkAccess(caller, rules.toArray(new FirewallRule[rules.size()])); } - for (FirewallRuleVO rule : rules) { - IpAddress sourceIp = _ipAddressDao.findById(rule.getSourceIpAddressId()); - - UserVmVO vm = _vmDao.findById(sourceIp.getAssociatedWithVmId()); - - Nic guestNic = _networkMgr.getNicInNetworkIncludingRemoved(vm.getId(), networkId); - String dstIp = guestNic.getIp4Address(); - - staticNatRules.add(new StaticNatRuleImpl(rule, dstIp)); + for (FirewallRuleVO rule : rules) { + staticNatRules.add(buildStaticNatRule(rule)); } try { @@ -885,37 +899,40 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { } @Override - public boolean revokeAllRules(long ipId, long userId, Account caller) throws ResourceUnavailableException { + public boolean revokeAllRulesForIp(long ipId, long userId, Account caller) throws ResourceUnavailableException { List rules = new ArrayList(); - //revoke all port forwarding rules + List pfRules = _forwardingDao.listByIpAndNotRevoked(ipId); if (s_logger.isDebugEnabled()) { s_logger.debug("Releasing " + pfRules.size() + " port forwarding rules for ip id=" + ipId); } for (PortForwardingRuleVO rule : pfRules) { - revokePortForwardingRuleInternal(rule.getId(), caller, userId, true); + //Mark all PF rules as Revoke, but don't revoke them yet + revokePortForwardingRuleInternal(rule.getId(), caller, userId, false); } - applyPortForwardingRules(ipId, true, null); - - //revoke all all static nat rules - List staticNatRules = _firewallDao.listByIpAndNotRevoked(ipId, Purpose.StaticNat); + List staticNatRules = _firewallDao.listByIpAndPurposeAndNotRevoked(ipId, Purpose.StaticNat); if (s_logger.isDebugEnabled()) { - s_logger.debug("Releasing " + pfRules.size() + " static nat rules for ip id=" + ipId); + s_logger.debug("Releasing " + staticNatRules.size() + " static nat rules for ip id=" + ipId); } for (FirewallRuleVO rule : staticNatRules) { - revokeStaticNatRuleInternal(rule.getId(), caller, userId, true); + //Mark all static nat rules as Revoke, but don't revoke them yet + revokeStaticNatRuleInternal(rule.getId(), caller, userId, false); } + + //revoke all port forwarding rules + applyPortForwardingRules(ipId, true, caller); - applyStaticNatRules(ipId, true, null); + //revoke all all static nat rules + applyStaticNatRules(ipId, true, caller); // Now we check again in case more rules have been inserted. rules.addAll(_forwardingDao.listByIpAndNotRevoked(ipId)); - rules.addAll(_firewallDao.listByIpAndPurpose(ipId, Purpose.StaticNat)); + rules.addAll(_firewallDao.listByIpAndPurposeAndNotRevoked(ipId, Purpose.StaticNat)); if (s_logger.isDebugEnabled()) { s_logger.debug("Successfully released rules for ip id=" + ipId + " and # of rules now = " + rules.size()); @@ -924,6 +941,47 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { return rules.size() == 0; } + @Override + public boolean revokeAllRulesForNetwork(long networkId, long userId, Account caller) throws ResourceUnavailableException { + List rules = new ArrayList(); + + List pfRules = _forwardingDao.listByNetwork(networkId); + if (s_logger.isDebugEnabled()) { + s_logger.debug("Releasing " + pfRules.size() + " port forwarding rules for network id=" + networkId); + } + + List staticNatRules = _firewallDao.listByNetworkAndPurpose(networkId, Purpose.StaticNat); + if (s_logger.isDebugEnabled()) { + s_logger.debug("Releasing " + staticNatRules.size() + " static nat rules for network id=" + networkId); + } + + //Mark all pf rules (Active and non-Active) to be revoked, but don't revoke it yet - pass apply=false + for (PortForwardingRuleVO rule : pfRules) { + revokePortForwardingRuleInternal(rule.getId(), caller, userId, false); + } + + //Mark all static nat rules (Active and non-Active) to be revoked, but don't revoke it yet - pass apply=false + for (FirewallRuleVO rule : staticNatRules) { + revokeStaticNatRuleInternal(rule.getId(), caller, userId, false); + } + + //revoke all PF rules for the network + applyPortForwardingRulesForNetwork(networkId, true, caller); + + //revoke all all static nat rules for the network + applyStaticNatRulesForNetwork(networkId, true, caller); + + // Now we check again in case more rules have been inserted. + rules.addAll(_forwardingDao.listByNetworkAndNotRevoked(networkId)); + rules.addAll(_firewallDao.listByNetworkAndPurposeAndNotRevoked(networkId, Purpose.StaticNat)); + + if (s_logger.isDebugEnabled()) { + s_logger.debug("Successfully released rules for network id=" + networkId + " and # of rules now = " + rules.size()); + } + + return rules.size() == 0; + } + @Override public boolean configure(String name, Map params) throws ConfigurationException { _name = name; @@ -1017,11 +1075,13 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { @Override public List listByNetworkId(long networkId) { - return _forwardingDao.listByNetworkId(networkId); + return _forwardingDao.listByNetwork(networkId); } @Override - public boolean disableOneToOneNat(long ipId){ + public boolean disableOneToOneNat(long ipId) throws ResourceUnavailableException{ + boolean success = true; + Account caller = UserContext.current().getCaller(); IPAddressVO ipAddress = _ipAddressDao.findById(ipId); @@ -1031,17 +1091,12 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { throw new InvalidParameterValueException("One to one nat is not enabled for the ip id=" + ipId); } - List rules = _firewallDao.listByIpAndNotRevoked(ipId, Purpose.StaticNat); - if (rules != null) { - for (FirewallRuleVO rule : rules) { - rule.setState(State.Revoke); - _firewallDao.update(rule.getId(), rule); - UsageEventVO usageEvent = new UsageEventVO(EventTypes.EVENT_NET_RULE_DELETE, rule.getAccountId(), 0, rule.getId(), null); - _usageEventDao.persist(usageEvent); - } + if (!revokeAllRulesForIp(ipId, UserContext.current().getCallerUserId(), caller)) { + s_logger.warn("Unable to revoke all static nat rules for ip " + ipAddress); + success = false; } - if (applyStaticNatRules(ipId, true, caller)) { + if (success) { ipAddress.setOneToOneNat(false); ipAddress.setAssociatedWithVmId(null); _ipAddressDao.update(ipAddress.getId(), ipAddress); @@ -1063,27 +1118,6 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { return _firewallDao.findById(ruleId); } - protected Pair getUserVmGuestIpAddress(UserVm vm, boolean includeRemovedNics) { - Ip dstIp = null; - List nics; - - if (includeRemovedNics) { - nics = _networkMgr.getNicsIncludingRemoved(vm); - } else { - nics = _networkMgr.getNics(vm); - } - - for (Nic nic : nics) { - Network ntwk = _networkMgr.getNetwork(nic.getNetworkId()); - if (ntwk.getGuestType() == GuestIpType.Virtual && nic.getIp4Address() != null) { - dstIp = new Ip(nic.getIp4Address()); - return new Pair(ntwk, dstIp); - } - } - - throw new CloudRuntimeException("Unable to find ip address to map to in " + vm.getId()); - } - @Override public StaticNatRule buildStaticNatRule(FirewallRule rule) { IpAddress ip = _ipAddressDao.findById(rule.getSourceIpAddressId()); diff --git a/server/src/com/cloud/network/rules/dao/PortForwardingRulesDao.java b/server/src/com/cloud/network/rules/dao/PortForwardingRulesDao.java index 3f4b1383d37..99621bcd59e 100644 --- a/server/src/com/cloud/network/rules/dao/PortForwardingRulesDao.java +++ b/server/src/com/cloud/network/rules/dao/PortForwardingRulesDao.java @@ -26,18 +26,20 @@ public interface PortForwardingRulesDao extends GenericDao listForApplication(long ipId); /** - * Find all port forwarding rules that have not been revoked. + * Find all port forwarding rules for the ip address that have not been revoked. * * @param ip ip address * @return List of PortForwardingRuleVO */ List listByIpAndNotRevoked(long ipId); + List listByNetworkAndNotRevoked(long networkId); + List listByIp(long ipId); List listByVm(Long vmId); - List listByNetworkId(long networkId); + List listByNetwork(long networkId); List listByAccount(long accountId); } diff --git a/server/src/com/cloud/network/rules/dao/PortForwardingRulesDaoImpl.java b/server/src/com/cloud/network/rules/dao/PortForwardingRulesDaoImpl.java index 89a9e69056e..c718c80c3a7 100644 --- a/server/src/com/cloud/network/rules/dao/PortForwardingRulesDaoImpl.java +++ b/server/src/com/cloud/network/rules/dao/PortForwardingRulesDaoImpl.java @@ -59,6 +59,7 @@ public class PortForwardingRulesDaoImpl extends GenericDaoBase listByIp(long ipId) { + public List listByNetworkAndNotRevoked(long networkId) { SearchCriteria sc = ActiveRulesSearch.create(); + sc.setParameters("networkId", networkId); + sc.setParameters("state", State.Revoke); + sc.setParameters("purpose", Purpose.PortForwarding); + + return listBy(sc, null); + } + + @Override + public List listByIp(long ipId) { + SearchCriteria sc = AllFieldsSearch.create(); sc.setParameters("ipId", ipId); sc.setParameters("purpose", Purpose.PortForwarding); @@ -114,7 +125,7 @@ public class PortForwardingRulesDaoImpl extends GenericDaoBase listByNetworkId(long networkId) { + public List listByNetwork(long networkId) { SearchCriteria sc = AllFieldsSearch.create(); sc.setParameters("networkId", networkId); sc.setParameters("purpose", Purpose.PortForwarding); diff --git a/server/src/com/cloud/user/AccountManagerImpl.java b/server/src/com/cloud/user/AccountManagerImpl.java index a026c450490..51d287fe6f1 100755 --- a/server/src/com/cloud/user/AccountManagerImpl.java +++ b/server/src/com/cloud/user/AccountManagerImpl.java @@ -882,7 +882,7 @@ public class AccountManagerImpl implements AccountManager, AccountService, Manag } } - //delete remote access vpns and associated users + //delete remote access vpns and associated users List remoteAccessVpns = _remoteAccessVpnDao.findByAccount(accountId); List vpnUsers = _vpnUser.listByAccount(accountId); @@ -917,7 +917,7 @@ public class AccountManagerImpl implements AccountManager, AccountService, Manag } } - //delete account specific vlans - only when networks are cleaned up successfully + //delete account specific Virtual vlans (belong to system Public Network) - only when networks are cleaned up successfully if (networksDeleted) { if (!_configMgr.deleteAccountSpecificVirtualRanges(accountId)){ accountCleanupNeeded = true; diff --git a/server/src/com/cloud/vm/UserVmManagerImpl.java b/server/src/com/cloud/vm/UserVmManagerImpl.java index f9190ffe894..ba136e20086 100755 --- a/server/src/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/com/cloud/vm/UserVmManagerImpl.java @@ -1108,51 +1108,20 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager UserContext ctx = UserContext.current(); ctx.setAccountId(vm.getAccountId()); - try { - + try { if (!_itMgr.advanceExpunge(vm, _accountMgr.getSystemUser(), caller)) { s_logger.info("Did not expunge " + vm); return false; } - _networkGroupMgr.removeInstanceFromGroups(vm.getId()); - - removeInstanceFromInstanceGroup(vm.getId()); - - //Cleanup LB/PF rules before expunging the vm - long vmId = vm.getId(); - - //cleanup port forwarding rules - if (_rulesMgr.revokePortForwardingRulesForVm(vmId)) { - s_logger.debug("Port forwarding rules are removed successfully as a part of vm id=" + vmId + " expunge"); - } else { - s_logger.warn("Fail to remove port forwarding rules as a part of vm id=" + vmId + " expunge"); - } - - //cleanup static nat rules - if (_rulesMgr.revokeStaticNatRulesForVm(vmId)) { - s_logger.debug("Port forwarding rules are removed successfully as a part of vm id=" + vmId + " expunge"); - } else { - s_logger.warn("Fail to remove port forwarding rules as a part of vm id=" + vmId + " expunge"); - } - - //cleanup load balancer rules - if (_lbMgr.removeVmFromLoadBalancers(vmId)) { - s_logger.debug("Removed vm id=" + vmId + " from all load balancers as a part of expunge process"); - } else { - s_logger.warn("Fail to remove vm id=" + vmId + " from load balancers as a part of expunge process"); - } - - //If vm is assigned to static nat ip address, remove the mapping - List ips = _ipAddressDao.listByAssociatedVmId(vmId); - if (ips != null) { - for (IPAddressVO ip : ips) { - ip.setOneToOneNat(false); - ip.setAssociatedWithVmId(null); - _ipAddressDao.update(ip.getId(), ip); - s_logger.debug("Disabled 1-1 nat for ip address " + ip + " as a part of vm " + vm + " expunge"); - } - } + //Cleanup vm resources - all the PF/LB/StaticNat rules associated with vm + s_logger.debug("Starting cleaning up vm " + vm + " resources..."); + if (cleanupVmResources(vm.getId())) { + s_logger.debug("Successfully cleaned up vm " + vm + " resources as a part of expunge process"); + } else { + s_logger.warn("Failed to cleanup resources as a part of vm " + vm + " expunge"); + return false; + } _itMgr.remove(vm, _accountMgr.getSystemUser(), caller); return true; @@ -1167,6 +1136,50 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager return false; } } + + private boolean cleanupVmResources(long vmId) { + boolean success = true; + + //Remove vm from security groups + _networkGroupMgr.removeInstanceFromGroups(vmId); + + //Remove vm from instance group + removeInstanceFromInstanceGroup(vmId); + + //cleanup port forwarding rules + if (_rulesMgr.revokePortForwardingRulesForVm(vmId)) { + s_logger.debug("Port forwarding rules are removed successfully as a part of vm id=" + vmId + " expunge"); + } else { + success = false; + s_logger.warn("Fail to remove port forwarding rules as a part of vm id=" + vmId + " expunge"); + } + + //cleanup load balancer rules + if (_lbMgr.removeVmFromLoadBalancers(vmId)) { + s_logger.debug("Removed vm id=" + vmId + " from all load balancers as a part of expunge process"); + } else { + success = false; + s_logger.warn("Fail to remove vm id=" + vmId + " from load balancers as a part of expunge process"); + } + + //If vm is assigned to static nat, disable static nat for the ip address + IPAddressVO ip = _ipAddressDao.findByAssociatedVmId(vmId); + try { + if (ip != null) { + if (_rulesMgr.disableOneToOneNat(ip.getId())) { + s_logger.debug("Disabled 1-1 nat for ip address " + ip + " as a part of vm id=" + vmId + " expunge"); + } else { + s_logger.warn("Failed to disable static nat for ip address " + ip + " as a part of vm id=" + vmId + " expunge"); + success = false; + } + } + } catch (ResourceUnavailableException e) { + success = false; + s_logger.warn("Failed to disable static nat for ip address " + ip + " as a part of vm id=" + vmId + " expunge because resource is unavailable", e); + } + + return success; + } @Override public void deletePrivateTemplateRecord(Long templateId){