diff --git a/engine/schema/src/com/cloud/network/dao/FirewallRulesDao.java b/engine/schema/src/com/cloud/network/dao/FirewallRulesDao.java index 6b9b3bb83e5..59987b29c2c 100644 --- a/engine/schema/src/com/cloud/network/dao/FirewallRulesDao.java +++ b/engine/schema/src/com/cloud/network/dao/FirewallRulesDao.java @@ -54,7 +54,9 @@ public interface FirewallRulesDao extends GenericDao { List listByIpAndNotRevoked(long ipAddressId); long countRulesByIpId(long sourceIpId); - + + long countRulesByIpIdAndState(long sourceIpId, FirewallRule.State state); + List listByNetworkPurposeTrafficTypeAndNotRevoked(long networkId, FirewallRule.Purpose purpose, FirewallRule.TrafficType trafficType); List listByNetworkPurposeTrafficType(long networkId, FirewallRule.Purpose purpose, FirewallRule.TrafficType trafficType); diff --git a/engine/schema/src/com/cloud/network/dao/FirewallRulesDaoImpl.java b/engine/schema/src/com/cloud/network/dao/FirewallRulesDaoImpl.java index 45a80685078..41f911ca1d1 100644 --- a/engine/schema/src/com/cloud/network/dao/FirewallRulesDaoImpl.java +++ b/engine/schema/src/com/cloud/network/dao/FirewallRulesDaoImpl.java @@ -100,6 +100,7 @@ public class FirewallRulesDaoImpl extends GenericDaoBase i RulesByIpCount = createSearchBuilder(Long.class); RulesByIpCount.select(null, Func.COUNT, RulesByIpCount.entity().getId()); RulesByIpCount.and("ipAddressId", RulesByIpCount.entity().getSourceIpAddressId(), Op.EQ); + RulesByIpCount.and("state", RulesByIpCount.entity().getState(), Op.EQ); RulesByIpCount.done(); } @@ -328,6 +329,16 @@ public class FirewallRulesDaoImpl extends GenericDaoBase i return result; } + @Override + public long countRulesByIpIdAndState(long sourceIpId, FirewallRule.State state) { + SearchCriteria sc = RulesByIpCount.create(); + sc.setParameters("ipAddressId", sourceIpId); + if (state != null) { + sc.setParameters("state", state); + } + return customSearch(sc, null).get(0); + } + @Override public List listByIpAndPurposeWithState(Long ipId, Purpose purpose, State state) { SearchCriteria sc = AllFieldsSearch.create(); diff --git a/server/src/com/cloud/network/NetworkManager.java b/server/src/com/cloud/network/NetworkManager.java index 306169ebcf6..8dc7743e706 100755 --- a/server/src/com/cloud/network/NetworkManager.java +++ b/server/src/com/cloud/network/NetworkManager.java @@ -191,7 +191,7 @@ public interface NetworkManager { public String acquireGuestIpAddress(Network network, String requestedIp); - boolean applyStaticNats(List staticNats, boolean continueOnError) throws ResourceUnavailableException; + boolean applyStaticNats(List staticNats, boolean continueOnError, boolean forRevoke) throws ResourceUnavailableException; boolean reallocate(VirtualMachineProfile vm, DataCenterDeployment dest) throws InsufficientCapacityException, ConcurrentOperationException; diff --git a/server/src/com/cloud/network/NetworkManagerImpl.java b/server/src/com/cloud/network/NetworkManagerImpl.java index 810c242689b..e322c9de794 100755 --- a/server/src/com/cloud/network/NetworkManagerImpl.java +++ b/server/src/com/cloud/network/NetworkManagerImpl.java @@ -627,30 +627,23 @@ public class NetworkManagerImpl extends ManagerBase implements NetworkManager, L @Override public boolean applyIpAssociations(Network network, boolean continueOnError) throws ResourceUnavailableException { List userIps = _ipAddressDao.listByAssociatedNetwork(network.getId(), null); - List publicIps = new ArrayList(); - if (userIps != null && !userIps.isEmpty()) { - for (IPAddressVO userIp : userIps) { - PublicIp publicIp = PublicIp.createFromAddrAndVlan(userIp, _vlanDao.findById(userIp.getVlanId())); - publicIps.add(publicIp); - } - } + boolean success = true; - boolean success = applyIpAssociations(network, false, continueOnError, publicIps); - - if (success) { - for (IPAddressVO addr : userIps) { - if (addr.getState() == IpAddress.State.Allocating) { - markPublicIpAsAllocated(addr); - } else if (addr.getState() == IpAddress.State.Releasing) { - // Cleanup all the resources for ip address if there are any, and only then un-assign ip in the - // system - if (cleanupIpResources(addr.getId(), Account.ACCOUNT_ID_SYSTEM, _accountMgr.getSystemAccount())) { - s_logger.debug("Unassiging ip address " + addr); - _ipAddressDao.unassignIpAddress(addr.getId()); - } else { - success = false; - s_logger.warn("Failed to release resources for ip address id=" + addr.getId()); - } + // CloudStack will take a lazy approach to associate an acquired public IP to a network service provider as + // it will not know what service an acquired IP will be used for. An IP is actually associated with a provider when first + // rule is applied. Similarly when last rule on the acquired IP is revoked, IP is not associated with any provider + // but still be associated with the account. At this point just mark IP as allocated or released. + for (IPAddressVO addr : userIps) { + if (addr.getState() == IpAddress.State.Allocating) { + addr.setAssociatedWithNetworkId(network.getId()); + markPublicIpAsAllocated(addr); + } else if (addr.getState() == IpAddress.State.Releasing) { + // Cleanup all the resources for ip address if there are any, and only then un-assign ip in the system + if (cleanupIpResources(addr.getId(), Account.ACCOUNT_ID_SYSTEM, _accountMgr.getSystemAccount())) { + _ipAddressDao.unassignIpAddress(addr.getId()); + } else { + success = false; + s_logger.warn("Failed to release resources for ip address id=" + addr.getId()); } } } @@ -658,14 +651,17 @@ public class NetworkManagerImpl extends ManagerBase implements NetworkManager, L return success; } - + // CloudStack will take a lazy approach to associate an acquired public IP to a network service provider as + // it will not know what a acquired IP will be used for. An IP is actually associated with a provider when first + // rule is applied. Similarly when last rule on the acquired IP is revoked, IP is not associated with any provider + // but still be associated with the account. Its up to caller of this function to decide when to invoke IPAssociation @Override - public boolean applyIpAssociations(Network network, boolean rulesRevoked, boolean continueOnError, + public boolean applyIpAssociations(Network network, boolean postApplyRules, boolean continueOnError, List publicIps) throws ResourceUnavailableException { boolean success = true; - Map> ipToServices = _networkModel.getIpToServices(publicIps, rulesRevoked, true); + Map> ipToServices = _networkModel.getIpToServices(publicIps, postApplyRules, true); Map> providerToIpList = _networkModel.getProviderToIpList(network, ipToServices); for (Provider provider : providerToIpList.keySet()) { @@ -2943,11 +2939,10 @@ public class NetworkManagerImpl extends ManagerBase implements NetworkManager, L publicIps.add(publicIp); } } - - // rules can not programmed unless IP is associated with network - // service provider, so run IP assoication for - // the network so as to ensure IP is associated before applying - // rules (in add state) + } + // rules can not programmed unless IP is associated with network service provider, so run IP assoication for + // the network so as to ensure IP is associated before applying rules (in add state) + if (checkIfIpAssocRequired(network, false, publicIps)) { applyIpAssociations(network, false, continueOnError, publicIps); } @@ -2961,15 +2956,65 @@ public class NetworkManagerImpl extends ManagerBase implements NetworkManager, L success = false; } - if (!(rules.get(0).getPurpose() == FirewallRule.Purpose.Firewall && trafficType == FirewallRule.TrafficType.Egress)) { - // if all the rules configured on public IP are revoked then - // dis-associate IP with network service provider + // if there are no active rules associated with a public IP, then public IP need not be associated with a provider. + // This IPAssoc ensures, public IP is dis-associated after last active rule is revoked. + if (checkIfIpAssocRequired(network, true, publicIps)) { applyIpAssociations(network, true, continueOnError, publicIps); } return success; } + // An IP association is required in below cases + // 1.there is at least one public IP associated with the network on which first rule (PF/static NAT/LB) is being applied. + // 2.last rule (PF/static NAT/LB) on the public IP has been revoked. So the public IP should not be associated with any provider + boolean checkIfIpAssocRequired(Network network, boolean postApplyRules, List publicIps) { + for (PublicIp ip : publicIps) { + if (ip.isSourceNat()) { + continue; + } else if (ip.isOneToOneNat()) { + continue; + } else { + Long totalCount = null; + Long revokeCount = null; + Long activeCount = null; + Long addCount = null; + + totalCount = _firewallDao.countRulesByIpId(ip.getId()); + if (postApplyRules) { + revokeCount = _firewallDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Revoke); + } else { + activeCount = _firewallDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Active); + addCount = _firewallDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Add); + } + + if (totalCount == null || totalCount.longValue() == 0L) { + continue; + } + + if (postApplyRules) { + + if (revokeCount != null && revokeCount.longValue() == totalCount.longValue()) { + s_logger.trace("All rules are in Revoke state, have to dis-assiciate IP from the backend"); + return true; + } + } else { + if (activeCount != null && activeCount > 0) { + continue; + } else if (addCount != null && addCount.longValue() == totalCount.longValue()) { + s_logger.trace("All rules are in Add state, have to assiciate IP with the backend"); + return true; + } else { + continue; + } + } + } + } + + // there are no IP's corresponding to this network that need to be associated with provider + return false; + } + public class NetworkGarbageCollector implements Runnable { @Override @@ -3535,7 +3580,8 @@ public class NetworkManagerImpl extends ManagerBase implements NetworkManager, L @Override - public boolean applyStaticNats(List staticNats, boolean continueOnError) throws ResourceUnavailableException { + public boolean applyStaticNats(List staticNats, boolean continueOnError, boolean forRevoke) + throws ResourceUnavailableException { Network network = _networksDao.findById(staticNats.get(0).getNetworkId()); boolean success = true; @@ -3554,9 +3600,11 @@ public class NetworkManagerImpl extends ManagerBase implements NetworkManager, L } } - // static NAT rules can not programmed unless IP is associated with network service provider, so run IP - // association for the network so as to ensure IP is associated before applying rules (in add state) - applyIpAssociations(network, false, continueOnError, publicIps); + // static NAT rules can not programmed unless IP is associated with source NAT service provider, so run IP + // association for the network so as to ensure IP is associated before applying rules + if (checkStaticNatIPAssocRequired(network, false, forRevoke, publicIps)) { + applyIpAssociations(network, false, continueOnError, publicIps); + } // get provider StaticNatServiceProvider element = getStaticNatProviderForNetwork(network); @@ -3587,12 +3635,38 @@ public class NetworkManagerImpl extends ManagerBase implements NetworkManager, L } } - // if all the rules configured on public IP are revoked then, dis-associate IP with network service provider - applyIpAssociations(network, true, continueOnError, publicIps); + // if the static NAT rules configured on public IP is revoked then, dis-associate IP with static NAT service provider + if (checkStaticNatIPAssocRequired(network, true, forRevoke, publicIps)) { + applyIpAssociations(network, true, continueOnError, publicIps); + } return success; } + // checks if there are any public IP assigned to network, that are marked for one-to-one NAT that + // needs to be associated/dis-associated with static-nat provider + boolean checkStaticNatIPAssocRequired(Network network, boolean postApplyRules, boolean forRevoke, List publicIps) { + for (PublicIp ip : publicIps) { + if (ip.isOneToOneNat()) { + Long activeFwCount = null; + activeFwCount = _firewallDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Active); + + if (!postApplyRules && !forRevoke) { + if (activeFwCount > 0) { + continue; + } else { + return true; + } + } else if (postApplyRules && forRevoke) { + return true; + } + } else { + continue; + } + } + return false; + } + @DB @Override public boolean reallocate(VirtualMachineProfile vm, DataCenterDeployment dest) throws InsufficientCapacityException, ConcurrentOperationException { diff --git a/server/src/com/cloud/network/NetworkModelImpl.java b/server/src/com/cloud/network/NetworkModelImpl.java index 407bf705826..d7ca6397183 100755 --- a/server/src/com/cloud/network/NetworkModelImpl.java +++ b/server/src/com/cloud/network/NetworkModelImpl.java @@ -273,7 +273,7 @@ public class NetworkModelImpl extends ManagerBase implements NetworkModel { } @Override - public Map> getIpToServices(List publicIps, boolean rulesRevoked, boolean includingFirewall) { + public Map> getIpToServices(List publicIps, boolean postApplyRules, boolean includingFirewall) { Map> ipToServices = new HashMap>(); if (publicIps != null && !publicIps.isEmpty()) { @@ -331,7 +331,7 @@ public class NetworkModelImpl extends ManagerBase implements NetworkModel { // IP is not being used for any purpose so skip IPAssoc to network service provider continue; } else { - if (rulesRevoked) { + if (postApplyRules) { // no active rules/revoked rules are associated with this public IP, so remove the // association with the provider if (ip.isSourceNat()) { diff --git a/server/src/com/cloud/network/rules/RulesManagerImpl.java b/server/src/com/cloud/network/rules/RulesManagerImpl.java index 0f733fb195d..397ece8ea72 100755 --- a/server/src/com/cloud/network/rules/RulesManagerImpl.java +++ b/server/src/com/cloud/network/rules/RulesManagerImpl.java @@ -964,7 +964,7 @@ public class RulesManagerImpl extends ManagerBase implements RulesManager, Rules } try { - if (!_networkMgr.applyStaticNats(staticNats, continueOnError)) { + if (!_networkMgr.applyStaticNats(staticNats, continueOnError, false)) { return false; } } catch (ResourceUnavailableException ex) { @@ -1307,7 +1307,7 @@ public class RulesManagerImpl extends ManagerBase implements RulesManager, Rules if (staticNats != null && !staticNats.isEmpty()) { try { - if (!_networkMgr.applyStaticNats(staticNats, continueOnError)) { + if (!_networkMgr.applyStaticNats(staticNats, continueOnError, forRevoke)) { return false; } } catch (ResourceUnavailableException ex) { @@ -1334,7 +1334,7 @@ public class RulesManagerImpl extends ManagerBase implements RulesManager, Rules s_logger.debug("Found " + staticNats.size() + " static nats to disable for network id " + networkId); } try { - if (!_networkMgr.applyStaticNats(staticNats, continueOnError)) { + if (!_networkMgr.applyStaticNats(staticNats, continueOnError, forRevoke)) { return false; } } catch (ResourceUnavailableException ex) { diff --git a/server/test/com/cloud/network/MockNetworkManagerImpl.java b/server/test/com/cloud/network/MockNetworkManagerImpl.java index 077395fca0e..9c7d0926c9e 100755 --- a/server/test/com/cloud/network/MockNetworkManagerImpl.java +++ b/server/test/com/cloud/network/MockNetworkManagerImpl.java @@ -323,7 +323,7 @@ public class MockNetworkManagerImpl extends ManagerBase implements NetworkManage @Override - public boolean applyStaticNats(List staticNats, boolean continueOnError) throws ResourceUnavailableException { + public boolean applyStaticNats(List staticNats, boolean continueOnError, boolean forRevoke) throws ResourceUnavailableException { // TODO Auto-generated method stub return false; } diff --git a/server/test/com/cloud/vpc/MockNetworkManagerImpl.java b/server/test/com/cloud/vpc/MockNetworkManagerImpl.java index b609022ff26..523dfb83941 100644 --- a/server/test/com/cloud/vpc/MockNetworkManagerImpl.java +++ b/server/test/com/cloud/vpc/MockNetworkManagerImpl.java @@ -987,7 +987,7 @@ public class MockNetworkManagerImpl extends ManagerBase implements NetworkManage * @see com.cloud.network.NetworkManager#applyStaticNats(java.util.List, boolean) */ @Override - public boolean applyStaticNats(List staticNats, boolean continueOnError) + public boolean applyStaticNats(List staticNats, boolean continueOnError, boolean forRevoke) throws ResourceUnavailableException { // TODO Auto-generated method stub return false;