From 5a89ba4dbbf638ac4b394dff5bb6addeaad057a1 Mon Sep 17 00:00:00 2001 From: alena Date: Mon, 19 Sep 2011 17:39:00 -0700 Subject: [PATCH] bug 11462: 1) when delete PF rule, revoke corresponding firewall first (if exists) 2) never remove PF rule from the table when corresponding firewall rule wasn't removed yet status 11462: resolved fixed Reviewed-by: edison@cloud.com --- .../commands/CreateIpForwardingRuleCmd.java | 3 ++- .../commands/CreateLoadBalancerRuleCmd.java | 6 +++--- .../commands/CreatePortForwardingRuleCmd.java | 4 ++-- .../commands/DeleteIpForwardingRuleCmd.java | 4 ++-- .../commands/DeleteLoadBalancerRuleCmd.java | 4 ++-- .../commands/DeletePortForwardingRuleCmd.java | 8 ++++---- .../network/firewall/FirewallManagerImpl.java | 7 ++++++- .../lb/LoadBalancingRulesManagerImpl.java | 19 +++++++++++-------- 8 files changed, 32 insertions(+), 23 deletions(-) diff --git a/api/src/com/cloud/api/commands/CreateIpForwardingRuleCmd.java b/api/src/com/cloud/api/commands/CreateIpForwardingRuleCmd.java index 07ceda7f756..3354814a209 100644 --- a/api/src/com/cloud/api/commands/CreateIpForwardingRuleCmd.java +++ b/api/src/com/cloud/api/commands/CreateIpForwardingRuleCmd.java @@ -123,12 +123,13 @@ public class CreateIpForwardingRuleCmd extends BaseAsyncCreateCmd implements Sta this.setResponseObject(fwResponse); } finally { if (!result || rule == null) { - _rulesService.revokeStaticNatRule(getEntityId(), true); if (getOpenFirewall()) { _firewallService.revokeRelatedFirewallRule(getEntityId(), true); } + _rulesService.revokeStaticNatRule(getEntityId(), true); + throw new ServerApiException(BaseCmd.INTERNAL_ERROR, "Error in creating ip forwarding rule on the domr"); } } diff --git a/api/src/com/cloud/api/commands/CreateLoadBalancerRuleCmd.java b/api/src/com/cloud/api/commands/CreateLoadBalancerRuleCmd.java index 2abd73e6d17..f565898fca9 100644 --- a/api/src/com/cloud/api/commands/CreateLoadBalancerRuleCmd.java +++ b/api/src/com/cloud/api/commands/CreateLoadBalancerRuleCmd.java @@ -170,13 +170,13 @@ public class CreateLoadBalancerRuleCmd extends BaseAsyncCreateCmd /*implements lbResponse.setResponseName(getCommandName()); } finally { if (!success || rule == null) { - // no need to apply the rule on the backend as it exists in the db only - _lbService.deleteLoadBalancerRule(getEntityId(), false); if (getOpenFirewall()) { _firewallService.revokeRelatedFirewallRule(getEntityId(), true); } - + // no need to apply the rule on the backend as it exists in the db only + _lbService.deleteLoadBalancerRule(getEntityId(), false); + throw new ServerApiException(BaseCmd.INTERNAL_ERROR, "Failed to create load balancer rule"); } } diff --git a/api/src/com/cloud/api/commands/CreatePortForwardingRuleCmd.java b/api/src/com/cloud/api/commands/CreatePortForwardingRuleCmd.java index 9f001683c98..21fb00253d5 100644 --- a/api/src/com/cloud/api/commands/CreatePortForwardingRuleCmd.java +++ b/api/src/com/cloud/api/commands/CreatePortForwardingRuleCmd.java @@ -35,7 +35,6 @@ import com.cloud.exception.InvalidParameterValueException; import com.cloud.exception.NetworkRuleConflictException; import com.cloud.exception.ResourceUnavailableException; import com.cloud.network.IpAddress; -import com.cloud.network.rules.FirewallRule; import com.cloud.network.rules.PortForwardingRule; import com.cloud.user.Account; import com.cloud.user.UserContext; @@ -145,12 +144,13 @@ public class CreatePortForwardingRuleCmd extends BaseAsyncCreateCmd implements P fwResponse.setResponseName(getCommandName()); } finally { if (!success || rule == null) { - _rulesService.revokePortForwardingRule(getEntityId(), true); if (getOpenFirewall()) { _firewallService.revokeRelatedFirewallRule(getEntityId(), true); } + _rulesService.revokePortForwardingRule(getEntityId(), true); + throw new ServerApiException(BaseCmd.INTERNAL_ERROR, "Failed to apply port forwarding rule"); } } diff --git a/api/src/com/cloud/api/commands/DeleteIpForwardingRuleCmd.java b/api/src/com/cloud/api/commands/DeleteIpForwardingRuleCmd.java index f84512a1c44..445f40aacc3 100644 --- a/api/src/com/cloud/api/commands/DeleteIpForwardingRuleCmd.java +++ b/api/src/com/cloud/api/commands/DeleteIpForwardingRuleCmd.java @@ -69,8 +69,8 @@ public class DeleteIpForwardingRuleCmd extends BaseAsyncCmd { @Override public void execute(){ UserContext.current().setEventDetails("Rule Id: "+id); - boolean result = _rulesService.revokeStaticNatRule(id, true); - result = result && _firewallService.revokeRelatedFirewallRule(id, true); + boolean result = _firewallService.revokeRelatedFirewallRule(id, true); + result = result && _rulesService.revokeStaticNatRule(id, true); if (result) { SuccessResponse response = new SuccessResponse(getCommandName()); diff --git a/api/src/com/cloud/api/commands/DeleteLoadBalancerRuleCmd.java b/api/src/com/cloud/api/commands/DeleteLoadBalancerRuleCmd.java index b2878d9db13..793bb548523 100644 --- a/api/src/com/cloud/api/commands/DeleteLoadBalancerRuleCmd.java +++ b/api/src/com/cloud/api/commands/DeleteLoadBalancerRuleCmd.java @@ -83,8 +83,8 @@ public class DeleteLoadBalancerRuleCmd extends BaseAsyncCmd { @Override public void execute(){ UserContext.current().setEventDetails("Load balancer Id: "+getId()); - boolean result = _lbService.deleteLoadBalancerRule(id, true); - result = result && _firewallService.revokeRelatedFirewallRule(id, true); + boolean result = _firewallService.revokeRelatedFirewallRule(id, true); + result = result && _lbService.deleteLoadBalancerRule(id, true); if (result) { SuccessResponse response = new SuccessResponse(getCommandName()); diff --git a/api/src/com/cloud/api/commands/DeletePortForwardingRuleCmd.java b/api/src/com/cloud/api/commands/DeletePortForwardingRuleCmd.java index eabd531d8eb..ed287204873 100644 --- a/api/src/com/cloud/api/commands/DeletePortForwardingRuleCmd.java +++ b/api/src/com/cloud/api/commands/DeletePortForwardingRuleCmd.java @@ -28,7 +28,6 @@ import com.cloud.api.ServerApiException; import com.cloud.api.response.SuccessResponse; import com.cloud.event.EventTypes; import com.cloud.exception.InvalidParameterValueException; -import com.cloud.exception.ResourceUnavailableException; import com.cloud.network.rules.PortForwardingRule; import com.cloud.user.UserContext; @@ -90,9 +89,10 @@ public class DeletePortForwardingRuleCmd extends BaseAsyncCmd { @Override public void execute(){ UserContext.current().setEventDetails("Rule Id: "+id); - boolean result = _rulesService.revokePortForwardingRule(id, true); - result = result && _firewallService.revokeRelatedFirewallRule(id, true); - + //revoke corresponding firewall rule first + boolean result = _firewallService.revokeRelatedFirewallRule(id, true); + result = result && _rulesService.revokePortForwardingRule(id, true); + if (result) { SuccessResponse response = new SuccessResponse(getCommandName()); this.setResponseObject(response); diff --git a/server/src/com/cloud/network/firewall/FirewallManagerImpl.java b/server/src/com/cloud/network/firewall/FirewallManagerImpl.java index 964f8f108f2..5f5b0fed27e 100644 --- a/server/src/com/cloud/network/firewall/FirewallManagerImpl.java +++ b/server/src/com/cloud/network/firewall/FirewallManagerImpl.java @@ -370,7 +370,12 @@ public class FirewallManagerImpl implements FirewallService, FirewallManager, Ma } else { for (FirewallRule rule : rules) { if (rule.getState() == FirewallRule.State.Revoke) { - _firewallDao.remove(rule.getId()); + 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"); + } else { + _firewallDao.remove(rule.getId()); + } } else if (rule.getState() == FirewallRule.State.Add) { FirewallRuleVO ruleVO = _firewallDao.findById(rule.getId()); ruleVO.setState(FirewallRule.State.Active); diff --git a/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java b/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java index b3e1e41d1b1..81101fbe416 100755 --- a/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java +++ b/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java @@ -104,8 +104,6 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager, @Inject IPAddressDao _ipAddressDao; @Inject - FirewallRulesDao _rulesDao; - @Inject LoadBalancerDao _lbDao; @Inject VlanDao _vlanDao; @@ -351,7 +349,13 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager, } } - _rulesDao.remove(lb.getId()); + 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"); + } else { + _firewallDao.remove(lb.getId()); + } + _elbMgr.handleDeleteLoadBalancerRule(lb, callerUserId, caller); s_logger.debug("Load balancer with id " + lb.getId() + " is removed successfully"); return true; @@ -431,7 +435,7 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager, try { _firewallMgr.detectRulesConflict(newRule, ipAddr); - if (!_rulesDao.setStateToAdd(newRule)) { + if (!_firewallDao.setStateToAdd(newRule)) { throw new CloudRuntimeException("Unable to update the state to add for " + newRule); } s_logger.debug("Load balancer " + newRule.getId() + " for Ip address id=" + ipId + ", public port " + srcPortStart + ", private port " + defPortStart + " is added successfully."); @@ -454,8 +458,7 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager, _firewallDao.remove(_firewallDao.findByRelatedId(newRule.getId()).getId()); _lbDao.remove(newRule.getId()); txn.commit(); - - _lbDao.remove(newRule.getId()); + } } @@ -533,7 +536,7 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager, @Override public boolean removeAllLoadBalanacersForIp(long ipId, Account caller, long callerUserId) { - List rules = _rulesDao.listByIpAndPurposeAndNotRevoked(ipId, Purpose.LoadBalancing); + List rules = _firewallDao.listByIpAndPurposeAndNotRevoked(ipId, Purpose.LoadBalancing); if (rules != null) s_logger.debug("Found " + rules.size() + " lb rules to cleanup"); for (FirewallRule rule : rules) { @@ -548,7 +551,7 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager, @Override public boolean removeAllLoadBalanacersForNetwork(long networkId, Account caller, long callerUserId) { - List rules = _rulesDao.listByNetworkAndPurposeAndNotRevoked(networkId, Purpose.LoadBalancing); + List rules = _firewallDao.listByNetworkAndPurposeAndNotRevoked(networkId, Purpose.LoadBalancing); if (rules != null) s_logger.debug("Found " + rules.size() + " lb rules to cleanup"); for (FirewallRule rule : rules) {