From cdb80c76c1a2a6b9e1ab58b4e6c54dc6120388f4 Mon Sep 17 00:00:00 2001 From: alena Date: Thu, 22 Sep 2011 13:51:37 -0700 Subject: [PATCH] bug 11537: revoke related FirewallRules when do vmExpunge and ipAddress release. status 11537: resolved fixed Reviwed-by: edison@cloud.com --- .../com/cloud/network/NetworkManagerImpl.java | 25 +++---- .../network/firewall/FirewallManagerImpl.java | 73 ++++++++++++++++++- .../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, 106 insertions(+), 20 deletions(-) diff --git a/server/src/com/cloud/network/NetworkManagerImpl.java b/server/src/com/cloud/network/NetworkManagerImpl.java index 7ff5c0daed5..4a1288c012c 100755 --- a/server/src/com/cloud/network/NetworkManagerImpl.java +++ b/server/src/com/cloud/network/NetworkManagerImpl.java @@ -2831,8 +2831,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..."); @@ -2863,18 +2874,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 5f5b0fed27e..23a13a5bcfc 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.UserContext; @@ -72,6 +75,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{ @@ -96,6 +101,10 @@ public class FirewallManagerImpl implements FirewallService, FirewallManager, Ma UsageEventDao _usageEventDao; @Inject ConfigurationDao _configDao; + @Inject + PortForwardingRulesDao _pfRulesDao; + @Inject + UserVmDao _vmDao; private boolean _elbEnabled=false; @@ -364,6 +373,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; @@ -372,7 +382,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()); } @@ -382,8 +393,9 @@ public class FirewallManagerImpl implements FirewallService, FirewallManager, Ma _firewallDao.update(ruleVO.getId(), ruleVO); } } - return true; } + + return success; } @Override @@ -574,4 +586,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 81101fbe416..dceef5ae37f 100755 --- a/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java +++ b/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java @@ -307,6 +307,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) { @@ -351,14 +352,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 edc3e0444f5..41cf4efd73b 100755 --- a/server/src/com/cloud/network/rules/RulesManagerImpl.java +++ b/server/src/com/cloud/network/rules/RulesManagerImpl.java @@ -478,7 +478,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 b34ed482070..21110e976cf 100755 --- a/server/src/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/com/cloud/vm/UserVmManagerImpl.java @@ -126,6 +126,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; @@ -329,6 +330,8 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager protected UserVmDetailsDao _vmDetailsDao; @Inject protected SecurityGroupDao _securityGroupDao; + @Inject + protected FirewallManager _firewallMgr; protected ScheduledExecutorService _executor = null; protected int _expungeInterval; @@ -1245,8 +1248,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 {