diff --git a/api/src/com/cloud/api/commands/CreateIpForwardingRuleCmd.java b/api/src/com/cloud/api/commands/CreateIpForwardingRuleCmd.java index 963e55d4d3c..a4e0d99a2cb 100644 --- a/api/src/com/cloud/api/commands/CreateIpForwardingRuleCmd.java +++ b/api/src/com/cloud/api/commands/CreateIpForwardingRuleCmd.java @@ -126,7 +126,7 @@ public class CreateIpForwardingRuleCmd extends BaseAsyncCreateCmd implements Sta _rulesService.revokeStaticNatRule(getEntityId(), true); if (getOpenFirewall()) { - _rulesService.revokeRelatedFirewallRule(getEntityId(), true); + _firewallService.revokeRelatedFirewallRule(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/CreatePortForwardingRuleCmd.java b/api/src/com/cloud/api/commands/CreatePortForwardingRuleCmd.java index 66094639cce..4fa2c0d57fd 100644 --- a/api/src/com/cloud/api/commands/CreatePortForwardingRuleCmd.java +++ b/api/src/com/cloud/api/commands/CreatePortForwardingRuleCmd.java @@ -148,7 +148,7 @@ public class CreatePortForwardingRuleCmd extends BaseAsyncCreateCmd implements P _rulesService.revokePortForwardingRule(getEntityId(), true); if (getOpenFirewall()) { - _rulesService.revokeRelatedFirewallRule(getEntityId(), true); + _firewallService.revokeRelatedFirewallRule(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 26ff77ac693..f84512a1c44 100644 --- a/api/src/com/cloud/api/commands/DeleteIpForwardingRuleCmd.java +++ b/api/src/com/cloud/api/commands/DeleteIpForwardingRuleCmd.java @@ -70,7 +70,7 @@ public class DeleteIpForwardingRuleCmd extends BaseAsyncCmd { public void execute(){ UserContext.current().setEventDetails("Rule Id: "+id); boolean result = _rulesService.revokeStaticNatRule(id, true); - result = result && _rulesService.revokeRelatedFirewallRule(id, true); + result = result && _firewallService.revokeRelatedFirewallRule(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 373a53d0266..b2878d9db13 100644 --- a/api/src/com/cloud/api/commands/DeleteLoadBalancerRuleCmd.java +++ b/api/src/com/cloud/api/commands/DeleteLoadBalancerRuleCmd.java @@ -84,7 +84,7 @@ public class DeleteLoadBalancerRuleCmd extends BaseAsyncCmd { public void execute(){ UserContext.current().setEventDetails("Load balancer Id: "+getId()); boolean result = _lbService.deleteLoadBalancerRule(id, true); - result = result && _rulesService.revokeRelatedFirewallRule(id, true); + result = result && _firewallService.revokeRelatedFirewallRule(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 240569b43cb..eabd531d8eb 100644 --- a/api/src/com/cloud/api/commands/DeletePortForwardingRuleCmd.java +++ b/api/src/com/cloud/api/commands/DeletePortForwardingRuleCmd.java @@ -91,7 +91,7 @@ public class DeletePortForwardingRuleCmd extends BaseAsyncCmd { public void execute(){ UserContext.current().setEventDetails("Rule Id: "+id); boolean result = _rulesService.revokePortForwardingRule(id, true); - result = result && _rulesService.revokeRelatedFirewallRule(id, true); + result = result && _firewallService.revokeRelatedFirewallRule(id, true); if (result) { SuccessResponse response = new SuccessResponse(getCommandName()); diff --git a/api/src/com/cloud/network/firewall/FirewallService.java b/api/src/com/cloud/network/firewall/FirewallService.java index 468cc4cf3bd..f33c4752b1c 100644 --- a/api/src/com/cloud/network/firewall/FirewallService.java +++ b/api/src/com/cloud/network/firewall/FirewallService.java @@ -23,4 +23,6 @@ public interface FirewallService { FirewallRule getFirewallRule(long ruleId); + boolean revokeRelatedFirewallRule(long ruleId, boolean apply); + } diff --git a/api/src/com/cloud/network/rules/RulesService.java b/api/src/com/cloud/network/rules/RulesService.java index 57a093e6a84..61e912ae5e0 100644 --- a/api/src/com/cloud/network/rules/RulesService.java +++ b/api/src/com/cloud/network/rules/RulesService.java @@ -70,7 +70,5 @@ public interface RulesService { StaticNatRule buildStaticNatRule(FirewallRule rule); List getSourceCidrs(long ruleId); - - boolean revokeRelatedFirewallRule(long ruleId, boolean apply); } diff --git a/server/src/com/cloud/network/dao/FirewallRulesCidrsDaoImpl.java b/server/src/com/cloud/network/dao/FirewallRulesCidrsDaoImpl.java index 6f40fdb34fa..2f8299c4780 100644 --- a/server/src/com/cloud/network/dao/FirewallRulesCidrsDaoImpl.java +++ b/server/src/com/cloud/network/dao/FirewallRulesCidrsDaoImpl.java @@ -45,7 +45,7 @@ public class FirewallRulesCidrsDaoImpl extends GenericDaoBase getSourceCidrs(long firewallRuleId) { - SearchCriteria sc = CidrsSearch.create(); + SearchCriteria sc = CidrsSearch.create(); sc.setParameters("firewallRuleId", firewallRuleId); List results = search(sc, null); diff --git a/server/src/com/cloud/network/dao/FirewallRulesDaoImpl.java b/server/src/com/cloud/network/dao/FirewallRulesDaoImpl.java index ef21bd4ad8e..a8429ac1d84 100644 --- a/server/src/com/cloud/network/dao/FirewallRulesDaoImpl.java +++ b/server/src/com/cloud/network/dao/FirewallRulesDaoImpl.java @@ -183,19 +183,18 @@ public class FirewallRulesDaoImpl extends GenericDaoBase i txn.start(); FirewallRuleVO dbfirewallRule = super.persist(firewallRule); - saveSourceCidrs(firewallRule); + saveSourceCidrs(firewallRule, firewallRule.getSourceCidrList()); txn.commit(); return dbfirewallRule; } - public void saveSourceCidrs(FirewallRuleVO firewallRule) { - List cidrlist = firewallRule.getSourceCidrList(); - if (cidrlist == null) { + public void saveSourceCidrs(FirewallRuleVO firewallRule, List cidrList) { + if (cidrList == null) { return; } - _firewallRulesCidrsDao.persist(firewallRule.getId(), cidrlist); + _firewallRulesCidrsDao.persist(firewallRule.getId(), cidrList); } @@ -228,5 +227,6 @@ public class FirewallRulesDaoImpl extends GenericDaoBase i sc.setParameters("purpose", Purpose.Firewall); return findOneBy(sc); - } + } + } diff --git a/server/src/com/cloud/network/firewall/FirewallManagerImpl.java b/server/src/com/cloud/network/firewall/FirewallManagerImpl.java index 21539c1736a..41c2228b5e3 100644 --- a/server/src/com/cloud/network/firewall/FirewallManagerImpl.java +++ b/server/src/com/cloud/network/firewall/FirewallManagerImpl.java @@ -19,6 +19,8 @@ package com.cloud.network.firewall; import java.util.ArrayList; +import java.util.Collection; +import java.util.HashSet; import java.util.List; import java.util.Map; @@ -248,12 +250,30 @@ public class FirewallManagerImpl implements FirewallService, FirewallManager, Ma if (rule.getId() == newRule.getId()) { continue; // Skips my own rule. } + + boolean oneOfRulesIsFirewall = ((rule.getPurpose() == Purpose.Firewall || newRule.getPurpose() == Purpose.Firewall) && ((newRule.getPurpose() != rule.getPurpose()) || (!newRule.getProtocol().equalsIgnoreCase(rule.getProtocol())))); + + //if both rules are firewall and their cidrs are different, we can skip port ranges verification + boolean bothRulesFirewall = (rule.getPurpose() == newRule.getPurpose() && rule.getPurpose() == Purpose.Firewall); + boolean duplicatedCidrs = false; + if (bothRulesFirewall) { + //Verify that the rules have different cidrs + List ruleCidrList = rule.getSourceCidrList(); + List newRuleCidrList = newRule.getSourceCidrList(); + + if (ruleCidrList == null || newRuleCidrList == null) { + continue; + } + + Collection similar = new HashSet(ruleCidrList); + similar.retainAll(newRuleCidrList); + if (similar.size() > 0) { + duplicatedCidrs = true; + } + } - boolean allowFirewall = ((rule.getPurpose() == Purpose.Firewall || newRule.getPurpose() == Purpose.Firewall) && ((newRule.getPurpose() != rule.getPurpose()) || (!newRule.getProtocol().equalsIgnoreCase(rule.getProtocol())))); - - - if (!allowFirewall) { + if (!oneOfRulesIsFirewall) { if (rule.getPurpose() == Purpose.StaticNat && newRule.getPurpose() != Purpose.StaticNat) { throw new NetworkRuleConflictException("There is 1 to 1 Nat rule specified for the ip address id=" + newRule.getSourceIpAddressId()); } else if (rule.getPurpose() != Purpose.StaticNat && newRule.getPurpose() == Purpose.StaticNat) { @@ -264,28 +284,30 @@ public class FirewallManagerImpl implements FirewallService, FirewallManager, Ma if (rule.getNetworkId() != newRule.getNetworkId() && rule.getState() != State.Revoke) { throw new NetworkRuleConflictException("New rule is for a different network than what's specified in rule " + rule.getXid()); } - - boolean notNullPorts = (newRule.getSourcePortStart() != null && newRule.getSourcePortEnd() != null && rule.getSourcePortStart() != null && rule.getSourcePortEnd() != null); - if (!allowFirewall && notNullPorts && ((rule.getSourcePortStart() <= newRule.getSourcePortStart() && rule.getSourcePortEnd() >= newRule.getSourcePortStart()) - || (rule.getSourcePortStart() <= newRule.getSourcePortEnd() && rule.getSourcePortEnd() >= newRule.getSourcePortEnd()) - || (newRule.getSourcePortStart() <= rule.getSourcePortStart() && newRule.getSourcePortEnd() >= rule.getSourcePortStart()) - || (newRule.getSourcePortStart() <= rule.getSourcePortEnd() && newRule.getSourcePortEnd() >= rule.getSourcePortEnd()))) { - - // we allow port forwarding rules with the same parameters but different protocols - boolean allowPf = (rule.getPurpose() == Purpose.PortForwarding && newRule.getPurpose() == Purpose.PortForwarding && !newRule.getProtocol().equalsIgnoreCase(rule.getProtocol())); - boolean allowStaticNat = (rule.getPurpose() == Purpose.StaticNat && newRule.getPurpose() == Purpose.StaticNat && !newRule.getProtocol().equalsIgnoreCase(rule.getProtocol())); - - if (!(allowPf || allowStaticNat || allowFirewall)) { - throw new NetworkRuleConflictException("The range specified, " + newRule.getSourcePortStart() + "-" + newRule.getSourcePortEnd() + ", conflicts with rule " + rule.getId() - + " which has " + rule.getSourcePortStart() + "-" + rule.getSourcePortEnd()); - } - } if (newRule.getProtocol().equalsIgnoreCase(NetUtils.ICMP_PROTO) && newRule.getProtocol().equalsIgnoreCase(rule.getProtocol())) { if (newRule.getIcmpCode().longValue() == rule.getIcmpCode().longValue() && newRule.getIcmpType().longValue() == rule.getIcmpType().longValue() && newRule.getProtocol().equalsIgnoreCase(rule.getProtocol())) { throw new InvalidParameterValueException("New rule conflicts with existing rule id=" + rule.getId()); } } + + boolean notNullPorts = (newRule.getSourcePortStart() != null && newRule.getSourcePortEnd() != null && rule.getSourcePortStart() != null && rule.getSourcePortEnd() != null) ; + if (!notNullPorts) { + continue; + } else if (!oneOfRulesIsFirewall && !(bothRulesFirewall && !duplicatedCidrs) && ((rule.getSourcePortStart().intValue() <= newRule.getSourcePortStart().intValue() && rule.getSourcePortEnd().intValue() >= newRule.getSourcePortStart().intValue()) + || (rule.getSourcePortStart().intValue() <= newRule.getSourcePortEnd().intValue() && rule.getSourcePortEnd().intValue() >= newRule.getSourcePortEnd().intValue()) + || (newRule.getSourcePortStart().intValue() <= rule.getSourcePortStart().intValue() && newRule.getSourcePortEnd().intValue() >= rule.getSourcePortStart().intValue()) + || (newRule.getSourcePortStart().intValue() <= rule.getSourcePortEnd().intValue() && newRule.getSourcePortEnd().intValue() >= rule.getSourcePortEnd().intValue()))) { + + // we allow port forwarding rules with the same parameters but different protocols + boolean allowPf = (rule.getPurpose() == Purpose.PortForwarding && newRule.getPurpose() == Purpose.PortForwarding && !newRule.getProtocol().equalsIgnoreCase(rule.getProtocol())); + boolean allowStaticNat = (rule.getPurpose() == Purpose.StaticNat && newRule.getPurpose() == Purpose.StaticNat && !newRule.getProtocol().equalsIgnoreCase(rule.getProtocol())); + + if (!(allowPf || allowStaticNat || oneOfRulesIsFirewall)) { + throw new NetworkRuleConflictException("The range specified, " + newRule.getSourcePortStart() + "-" + newRule.getSourcePortEnd() + ", conflicts with rule " + rule.getId() + + " which has " + rule.getSourcePortStart() + "-" + rule.getSourcePortEnd()); + } + } } if (s_logger.isDebugEnabled()) { @@ -538,4 +560,18 @@ public class FirewallManagerImpl implements FirewallService, FirewallManager, Ma return rules.size() == 0; } + @Override + public boolean revokeRelatedFirewallRule(long ruleId, boolean apply) { + FirewallRule fwRule = _firewallDao.findByRelatedId(ruleId); + + if (fwRule == null) { + s_logger.trace("No related firewall rule exists for rule id=" + ruleId + " so returning true here"); + return true; + } + + s_logger.debug("Revoking Firewall rule id=" + fwRule.getId() + " as a part of rule delete id=" + ruleId + " with apply=" + apply); + return revokeFirewallRule(fwRule.getId(), apply); + + } + } diff --git a/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java b/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java index 471596e106e..8c269b75792 100755 --- a/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java +++ b/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java @@ -454,17 +454,13 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager, throw new CloudRuntimeException("Unable to add rule for ip address id=" + newRule.getSourceIpAddressId(), e); } finally { if (!success) { - txn.start(); - _firewallDao.remove(_firewallDao.findByRelatedId(newRule.getId()).getId()); + //no need to apply the rule as it wasn't programmed on the backend yet + _firewallMgr.revokeRelatedFirewallRule(newRule.getId(), false); _lbDao.remove(newRule.getId()); txn.commit(); - - _lbDao.remove(newRule.getId()); } } - - } @Override diff --git a/server/src/com/cloud/network/rules/FirewallRuleVO.java b/server/src/com/cloud/network/rules/FirewallRuleVO.java index 9a2f5ae923a..ec75c2bf63b 100644 --- a/server/src/com/cloud/network/rules/FirewallRuleVO.java +++ b/server/src/com/cloud/network/rules/FirewallRuleVO.java @@ -35,6 +35,8 @@ import javax.persistence.InheritanceType; import javax.persistence.Table; import javax.persistence.Transient; +import com.cloud.network.dao.FirewallRulesCidrsDaoImpl; +import com.cloud.utils.component.ComponentLocator; import com.cloud.utils.db.GenericDao; import com.cloud.utils.net.NetUtils; @@ -43,6 +45,8 @@ import com.cloud.utils.net.NetUtils; @Inheritance(strategy=InheritanceType.JOINED) @DiscriminatorColumn(name="purpose", discriminatorType=DiscriminatorType.STRING, length=32) public class FirewallRuleVO implements FirewallRule { + protected final FirewallRulesCidrsDaoImpl _firewallRulesCidrsDao = ComponentLocator.inject(FirewallRulesCidrsDaoImpl.class); + @Id @GeneratedValue(strategy=GenerationType.IDENTITY) @Column(name="id") @@ -106,6 +110,9 @@ public class FirewallRuleVO implements FirewallRule { @Override public List getSourceCidrList() { + if (sourceCidrs == null && purpose == Purpose.Firewall) { + return _firewallRulesCidrsDao.getSourceCidrs(id); + } return sourceCidrs; } diff --git a/server/src/com/cloud/network/rules/RulesManagerImpl.java b/server/src/com/cloud/network/rules/RulesManagerImpl.java index 29b6c47cf12..ae4ca8cf5ab 100755 --- a/server/src/com/cloud/network/rules/RulesManagerImpl.java +++ b/server/src/com/cloud/network/rules/RulesManagerImpl.java @@ -223,8 +223,8 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { } catch (Exception e) { txn.start(); - - _firewallDao.remove(_firewallDao.findByRelatedId(newRule.getId()).getId()); + //no need to apply the rule as it wasn't programmed on the backend yet + _firewallMgr.revokeRelatedFirewallRule(newRule.getId(), false); _forwardingDao.remove(newRule.getId()); txn.commit(); @@ -289,7 +289,8 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { } catch (Exception e) { txn.start(); - _firewallDao.remove(_firewallDao.findByRelatedId(newRule.getId()).getId()); + //no need to apply the rule as it wasn't programmed on the backend yet + _firewallMgr.revokeRelatedFirewallRule(newRule.getId(), false); _forwardingDao.remove(newRule.getId()); txn.commit(); @@ -1118,19 +1119,4 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { return true; } - - @Override - public boolean revokeRelatedFirewallRule(long ruleId, boolean apply) { - FirewallRule fwRule = _firewallDao.findByRelatedId(ruleId); - - if (fwRule == null) { - s_logger.trace("No related firewall rule exists for rule id=" + ruleId + " so returning true here"); - return true; - } - - s_logger.debug("Revoking Firewall rule id=" + fwRule.getId() + " as a part of rule delete id=" + ruleId + " with apply=" + apply); - return _firewallMgr.revokeFirewallRule(fwRule.getId(), apply); - - } - } diff --git a/server/src/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java b/server/src/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java index 118431bc980..98a3e9bef49 100755 --- a/server/src/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java +++ b/server/src/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java @@ -246,7 +246,7 @@ public class RemoteAccessVpnManagerImpl implements RemoteAccessVpnService, Manag for (FirewallRule vpnFwRule : vpnFwRules) { //don't apply on the backend yet; send all 3 rules in a banch - _rulesMgr.revokeRelatedFirewallRule(vpnFwRule.getId(), false); + _firewallMgr.revokeRelatedFirewallRule(vpnFwRule.getId(), false); fwRules.add(_rulesDao.findByRelatedId(vpnFwRule.getId())); }