From a1cab92ae1fb2bbc6996a2f0152021df42cd5dbf Mon Sep 17 00:00:00 2001 From: alena Date: Thu, 22 Sep 2011 13:59:15 -0700 Subject: [PATCH] bug 11537: revoke related FirewallRules when do vmExpunge and ipAddress release. status 11537: resolved fixed Reviewed-by: edison@cloud.com Conflicts: server/src/com/cloud/network/firewall/FirewallManagerImpl.java server/src/com/cloud/vm/UserVmManagerImpl.java --- .../com/cloud/network/NetworkManagerImpl.java | 25 ++++--- .../network/firewall/FirewallManagerImpl.java | 72 ++++++++++++++++++- .../lb/LoadBalancingRulesManagerImpl.java | 11 ++- .../cloud/network/rules/FirewallManager.java | 2 + .../cloud/network/rules/RulesManagerImpl.java | 2 +- .../src/com/cloud/vm/UserVmManagerImpl.java | 13 +++- 6 files changed, 105 insertions(+), 20 deletions(-) diff --git a/server/src/com/cloud/network/NetworkManagerImpl.java b/server/src/com/cloud/network/NetworkManagerImpl.java index 7689d80304c..b61c3d29057 100755 --- a/server/src/com/cloud/network/NetworkManagerImpl.java +++ b/server/src/com/cloud/network/NetworkManagerImpl.java @@ -2849,8 +2849,19 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag private boolean cleanupIpResources(long ipId, long userId, Account caller) { boolean success = true; - + //Revoke all firewall rules for the ip + try { + s_logger.debug("Revoking all " + Purpose.Firewall + "rules as a part of public IP id=" + ipId + " release..."); + if (!_firewallMgr.revokeFirewallRulesForIp(ipId, userId, caller)) { + s_logger.warn("Unable to revoke all the firewall rules for ip id=" + ipId + " as a part of ip release"); + success = false; + } + } catch (ResourceUnavailableException e) { + s_logger.warn("Unable to revoke all firewall rules for ip id=" + ipId + " as a part of ip release", e); + success = false; + } + //Revoke all PF/Static nat rules for the ip try { s_logger.debug("Revoking all " + Purpose.PortForwarding + "/" + Purpose.StaticNat + " rules as a part of public IP id=" + ipId + " release..."); @@ -2881,18 +2892,6 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag success = false; } - //Revoke all firewall rules for the ip - try { - s_logger.debug("Revoking all " + Purpose.Firewall + "rules as a part of public IP id=" + ipId + " release..."); - if (!_firewallMgr.revokeFirewallRulesForIp(ipId, userId, caller)) { - s_logger.warn("Unable to revoke all the firewall rules for ip id=" + ipId + " as a part of ip release"); - success = false; - } - } catch (ResourceUnavailableException e) { - s_logger.warn("Unable to revoke all firewall rules for ip id=" + ipId + " as a part of ip release", e); - success = false; - } - return success; } diff --git a/server/src/com/cloud/network/firewall/FirewallManagerImpl.java b/server/src/com/cloud/network/firewall/FirewallManagerImpl.java index 16897e8f172..c1425c576ad 100644 --- a/server/src/com/cloud/network/firewall/FirewallManagerImpl.java +++ b/server/src/com/cloud/network/firewall/FirewallManagerImpl.java @@ -23,6 +23,7 @@ import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import javax.ejb.Local; import javax.naming.ConfigurationException; @@ -57,6 +58,8 @@ import com.cloud.network.rules.FirewallRule; import com.cloud.network.rules.FirewallRule.Purpose; import com.cloud.network.rules.FirewallRule.State; import com.cloud.network.rules.FirewallRuleVO; +import com.cloud.network.rules.PortForwardingRuleVO; +import com.cloud.network.rules.dao.PortForwardingRulesDao; import com.cloud.user.Account; import com.cloud.user.AccountManager; import com.cloud.user.DomainManager; @@ -73,6 +76,8 @@ import com.cloud.utils.db.SearchCriteria.Op; import com.cloud.utils.db.Transaction; import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.net.NetUtils; +import com.cloud.vm.UserVmVO; +import com.cloud.vm.dao.UserVmDao; @Local(value = { FirewallService.class, FirewallManager.class }) public class FirewallManagerImpl implements FirewallService, FirewallManager, Manager{ @@ -99,6 +104,9 @@ public class FirewallManagerImpl implements FirewallService, FirewallManager, Ma ConfigurationDao _configDao; @Inject DomainManager _domainMgr; + PortForwardingRulesDao _pfRulesDao; + @Inject + UserVmDao _vmDao; private boolean _elbEnabled=false; @@ -367,6 +375,7 @@ public class FirewallManagerImpl implements FirewallService, FirewallManager, Ma @Override public boolean applyRules(List rules, boolean continueOnError) throws ResourceUnavailableException { + boolean success = true; if (!_networkMgr.applyRules(rules, continueOnError)) { s_logger.warn("Rules are not completely applied"); return false; @@ -375,7 +384,8 @@ public class FirewallManagerImpl implements FirewallService, FirewallManager, Ma if (rule.getState() == FirewallRule.State.Revoke) { FirewallRuleVO relatedRule = _firewallDao.findByRelatedId(rule.getId()); if (relatedRule != null) { - s_logger.debug("Not removing the firewall rule id=" + rule.getId() + " as it has related firewall rule id=" + relatedRule.getId() + "; leaving it in Revoke state"); + s_logger.warn("Can't remove the firewall rule id=" + rule.getId() + " as it has related firewall rule id=" + relatedRule.getId() + "; leaving it in Revoke state"); + success = false; } else { _firewallDao.remove(rule.getId()); } @@ -385,8 +395,9 @@ public class FirewallManagerImpl implements FirewallService, FirewallManager, Ma _firewallDao.update(ruleVO.getId(), ruleVO); } } - return true; } + + return success; } @Override @@ -581,4 +592,61 @@ public class FirewallManagerImpl implements FirewallService, FirewallManager, Ma } + + @Override + public boolean revokeFirewallRulesForVm(long vmId) { + boolean success = true; + UserVmVO vm = _vmDao.findByIdIncludingRemoved(vmId); + if (vm == null) { + return false; + } + + List pfRules = _pfRulesDao.listByVm(vmId); + List staticNatRules = _firewallDao.listStaticNatByVmId(vm.getId()); + List firewallRules = new ArrayList(); + + //Make a list of firewall rules to reprogram + for (PortForwardingRuleVO pfRule : pfRules) { + FirewallRuleVO relatedRule = _firewallDao.findByRelatedId(pfRule.getId()); + if (relatedRule != null) { + firewallRules.add(relatedRule); + } + } + + for (FirewallRuleVO staticNatRule : staticNatRules) { + FirewallRuleVO relatedRule = _firewallDao.findByRelatedId(staticNatRule.getId()); + if (relatedRule != null) { + firewallRules.add(relatedRule); + } + } + + + Set ipsToReprogram = new HashSet(); + + if (firewallRules.isEmpty()) { + s_logger.debug("No firewall rules are found for vm id=" + vmId); + return true; + } else { + s_logger.debug("Found " + firewallRules.size() + " to cleanup for vm id=" + vmId); + } + + for (FirewallRuleVO rule : firewallRules) { + // Mark firewall rules as Revoked, but don't revoke it yet (apply=false) + revokeFirewallRule(rule.getId(), false, _accountMgr.getSystemAccount(), Account.ACCOUNT_ID_SYSTEM); + ipsToReprogram.add(rule.getSourceIpAddressId()); + } + + // apply rules for all ip addresses + for (Long ipId : ipsToReprogram) { + s_logger.debug("Applying firewall rules for ip address id=" + ipId + " as a part of vm expunge"); + try { + success = success && applyFirewallRules(ipId,_accountMgr.getSystemAccount()); + } catch (ResourceUnavailableException ex) { + s_logger.warn("Failed to apply port forwarding rules for ip id=" + ipId); + success = false; + } + } + + return success; + } } diff --git a/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java b/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java index a68d40c9fcd..238c2fcebda 100755 --- a/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java +++ b/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java @@ -309,6 +309,7 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager, LoadBalancerVO lb = _lbDao.findById(loadBalancerId); Transaction txn = Transaction.currentTxn(); boolean generateUsageEvent = false; + boolean success = true; txn.start(); if (lb.getState() == FirewallRule.State.Staged) { @@ -353,14 +354,18 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager, FirewallRuleVO relatedRule = _firewallDao.findByRelatedId(lb.getId()); if (relatedRule != null) { - s_logger.debug("Not removing the firewall rule id=" + lb.getId() + " as it has related firewall rule id=" + relatedRule.getId() + "; leaving it in Revoke state"); + s_logger.warn("Unable to remove firewall rule id=" + lb.getId() + " as it has related firewall rule id=" + relatedRule.getId() + "; leaving it in Revoke state"); + success = false; } else { _firewallDao.remove(lb.getId()); } _elbMgr.handleDeleteLoadBalancerRule(lb, callerUserId, caller); - s_logger.debug("Load balancer with id " + lb.getId() + " is removed successfully"); - return true; + if (success) { + s_logger.debug("Load balancer with id " + lb.getId() + " is removed successfully"); + } + + return success; } @Override @DB diff --git a/server/src/com/cloud/network/rules/FirewallManager.java b/server/src/com/cloud/network/rules/FirewallManager.java index 21d80b57010..13a24e4b54b 100644 --- a/server/src/com/cloud/network/rules/FirewallManager.java +++ b/server/src/com/cloud/network/rules/FirewallManager.java @@ -73,4 +73,6 @@ public interface FirewallManager extends FirewallService{ boolean revokeAllFirewallRulesForNetwork(long networkId, long userId, Account caller) throws ResourceUnavailableException; + boolean revokeFirewallRulesForVm(long vmId); + } diff --git a/server/src/com/cloud/network/rules/RulesManagerImpl.java b/server/src/com/cloud/network/rules/RulesManagerImpl.java index dc50c9fd812..8ba464f5728 100755 --- a/server/src/com/cloud/network/rules/RulesManagerImpl.java +++ b/server/src/com/cloud/network/rules/RulesManagerImpl.java @@ -483,7 +483,7 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { return success; } - + @Override public boolean revokeStaticNatRulesForVm(long vmId) { boolean success = true; diff --git a/server/src/com/cloud/vm/UserVmManagerImpl.java b/server/src/com/cloud/vm/UserVmManagerImpl.java index 65e5957d5de..64d0a9475af 100755 --- a/server/src/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/com/cloud/vm/UserVmManagerImpl.java @@ -129,6 +129,7 @@ import com.cloud.network.dao.LoadBalancerVMMapDao; import com.cloud.network.dao.NetworkDao; import com.cloud.network.lb.LoadBalancingRulesManager; import com.cloud.network.router.VirtualNetworkApplianceManager; +import com.cloud.network.rules.FirewallManager; import com.cloud.network.rules.RulesManager; import com.cloud.network.security.SecurityGroup; import com.cloud.network.security.SecurityGroupManager; @@ -340,6 +341,8 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager protected VMInstanceDao _vmInstanceDao; @Inject protected ResourceLimitService _resourceLimitMgr; + @Inject + protected FirewallManager _firewallMgr; protected ScheduledExecutorService _executor = null; protected int _expungeInterval; @@ -1242,8 +1245,16 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager //Remove vm from instance group removeInstanceFromInstanceGroup(vmId); + + //cleanup firewall rules + if (_firewallMgr.revokeFirewallRulesForVm(vmId)) { + s_logger.debug("Firewall rules are removed successfully as a part of vm id=" + vmId + " expunge"); + } else { + success = false; + s_logger.warn("Fail to remove firewall rules as a part of vm id=" + vmId + " expunge"); + } - // cleanup port forwarding rules + //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 {