From cfd2a0bdf8c9945f646af0961dc0fb635adc6449 Mon Sep 17 00:00:00 2001 From: Hugo Trippaers Date: Thu, 6 Dec 2012 14:41:12 +0100 Subject: [PATCH] Fixes for some bug discovered while writing unittests Rewritten handling for static nat and port forwarding, should make some more sense now and the complex functions are split in smaller units. Fix a small bug in Match Add equals function to NatRule that ignores the uuid field. --- .../src/com/cloud/network/nicira/Match.java | 2 +- .../src/com/cloud/network/nicira/NatRule.java | 59 ++++ .../network/resource/NiciraNvpResource.java | 261 ++++++++---------- 3 files changed, 170 insertions(+), 152 deletions(-) diff --git a/plugins/network-elements/nicira-nvp/src/com/cloud/network/nicira/Match.java b/plugins/network-elements/nicira-nvp/src/com/cloud/network/nicira/Match.java index 034a7de13d9..0c4e677536c 100644 --- a/plugins/network-elements/nicira-nvp/src/com/cloud/network/nicira/Match.java +++ b/plugins/network-elements/nicira-nvp/src/com/cloud/network/nicira/Match.java @@ -41,7 +41,7 @@ public class Match { this.protocol = protocol; } - public Integer getSource_port_min() { + public Integer getSourcePortMin() { return source_port_min; } diff --git a/plugins/network-elements/nicira-nvp/src/com/cloud/network/nicira/NatRule.java b/plugins/network-elements/nicira-nvp/src/com/cloud/network/nicira/NatRule.java index 46437071be9..b66ffa89b77 100644 --- a/plugins/network-elements/nicira-nvp/src/com/cloud/network/nicira/NatRule.java +++ b/plugins/network-elements/nicira-nvp/src/com/cloud/network/nicira/NatRule.java @@ -219,5 +219,64 @@ public class NatRule { return true; } + public boolean equalsIgnoreUuid(Object obj) { + if (this == obj) + return true; + if (obj == null) + return false; + if (getClass() != obj.getClass()) + return false; + NatRule other = (NatRule) obj; + if (match == null) { + if (other.match != null) + return false; + } else if (!match.equals(other.match)) + return false; + if (to_destination_ip_address_max == null) { + if (other.to_destination_ip_address_max != null) + return false; + } else if (!to_destination_ip_address_max + .equals(other.to_destination_ip_address_max)) + return false; + if (to_destination_ip_address_min == null) { + if (other.to_destination_ip_address_min != null) + return false; + } else if (!to_destination_ip_address_min + .equals(other.to_destination_ip_address_min)) + return false; + if (to_destination_port == null) { + if (other.to_destination_port != null) + return false; + } else if (!to_destination_port.equals(other.to_destination_port)) + return false; + if (to_source_ip_address_max == null) { + if (other.to_source_ip_address_max != null) + return false; + } else if (!to_source_ip_address_max + .equals(other.to_source_ip_address_max)) + return false; + if (to_source_ip_address_min == null) { + if (other.to_source_ip_address_min != null) + return false; + } else if (!to_source_ip_address_min + .equals(other.to_source_ip_address_min)) + return false; + if (to_source_port_max == null) { + if (other.to_source_port_max != null) + return false; + } else if (!to_source_port_max.equals(other.to_source_port_max)) + return false; + if (to_source_port_min == null) { + if (other.to_source_port_min != null) + return false; + } else if (!to_source_port_min.equals(other.to_source_port_min)) + return false; + if (type == null) { + if (other.type != null) + return false; + } else if (!type.equals(other.type)) + return false; + return true; + } } diff --git a/plugins/network-elements/nicira-nvp/src/com/cloud/network/resource/NiciraNvpResource.java b/plugins/network-elements/nicira-nvp/src/com/cloud/network/resource/NiciraNvpResource.java index 960b098c986..3dd0145ff99 100644 --- a/plugins/network-elements/nicira-nvp/src/com/cloud/network/resource/NiciraNvpResource.java +++ b/plugins/network-elements/nicira-nvp/src/com/cloud/network/resource/NiciraNvpResource.java @@ -526,56 +526,37 @@ public class NiciraNvpResource implements ServerResource { // Any other SourceNat rule should have a corresponding DestinationNat rule for (StaticNatRuleTO rule : cmd.getRules()) { - // Find if a DestinationNat rule exists for this rule - String insideIp = rule.getDstIp(); - String insideCidr = rule.getDstIp() + "/32"; - String outsideIp = rule.getSrcIp(); - String outsideCidr = rule.getSrcIp() + "/32"; - + + NatRule[] rulepair = generateStaticNatRulePair(rule.getDstIp(), rule.getSrcIp()); + NatRule incoming = null; NatRule outgoing = null; for (NatRule storedRule : existingRules.getResults()) { - if ("SourceNatRule".equals(storedRule.getType())) { - if (outsideIp.equals(storedRule.getToSourceIpAddressMin()) && - outsideIp.equals(storedRule.getToSourceIpAddressMax()) && - storedRule.getToSourcePortMin() == null) { - // The outgoing rule exists - outgoing = storedRule; - } - } - if ("DestinationNatRule".equals(storedRule.getType()) && - storedRule.getToDestinationPort() != null) { - // Skip PortForwarding rules - continue; - } - // Compare against Ip as it should be a /32 cidr and the /32 is omitted - if (outsideIp.equals(storedRule.getMatch().getDestinationIpAddresses())) { + if (storedRule.equalsIgnoreUuid(rulepair[1])) { + // The outgoing rule exists + outgoing = storedRule; + s_logger.debug("Found matching outgoing rule " + outgoing.getUuid()); + if (incoming != null) { + break; + } + } + else if (storedRule.equalsIgnoreUuid(rulepair[0])) { // The incoming rule exists incoming = storedRule; + s_logger.debug("Found matching incoming rule " + incoming.getUuid()); + if (outgoing != null) { + break; + } } } if (incoming != null && outgoing != null) { - if (insideIp.equals(incoming.getToDestinationIpAddressMin())) { - if (rule.revoked()) { - s_logger.debug("Deleting incoming rule " + incoming.getUuid()); - _niciraNvpApi.deleteLogicalRouterNatRule(cmd.getLogicalRouterUuid(), incoming.getUuid()); - - s_logger.debug("Deleting outgoing rule " + outgoing.getUuid()); - _niciraNvpApi.deleteLogicalRouterNatRule(cmd.getLogicalRouterUuid(), outgoing.getUuid()); - } - } - else { - s_logger.debug("Updating outgoing rule " + outgoing.getUuid()); - outgoing.setToDestinationIpAddressMin(insideIp); - outgoing.setToDestinationIpAddressMax(insideIp); - _niciraNvpApi.modifyLogicalRouterNatRule(cmd.getLogicalRouterUuid(), outgoing); - - s_logger.debug("Updating incoming rule " + outgoing.getUuid()); - incoming.setToSourceIpAddressMin(insideIp); - incoming.setToSourceIpAddressMax(insideIp); - _niciraNvpApi.modifyLogicalRouterNatRule(cmd.getLogicalRouterUuid(), incoming); - break; + if (rule.revoked()) { + s_logger.debug("Deleting incoming rule " + incoming.getUuid()); + _niciraNvpApi.deleteLogicalRouterNatRule(cmd.getLogicalRouterUuid(), incoming.getUuid()); + + s_logger.debug("Deleting outgoing rule " + outgoing.getUuid()); + _niciraNvpApi.deleteLogicalRouterNatRule(cmd.getLogicalRouterUuid(), outgoing.getUuid()); } } else { @@ -585,28 +566,17 @@ public class NiciraNvpResource implements ServerResource { break; } - // api createLogicalRouterNatRule - // create the dnat rule - Match m = new Match(); - m.setDestinationIpAddresses(outsideCidr); - NatRule newDnatRule = new NatRule(); - newDnatRule.setType("DestinationNatRule"); - newDnatRule.setMatch(m); - newDnatRule.setToDestinationIpAddressMin(insideIp); - newDnatRule.setToDestinationIpAddressMax(insideIp); - newDnatRule = _niciraNvpApi.createLogicalRouterNatRule(cmd.getLogicalRouterUuid(), newDnatRule); - s_logger.debug("Created " + natRuleToString(newDnatRule)); - - // create matching snat rule - m = new Match(); - m.setSourceIpAddresses(insideIp + "/32"); - NatRule newSnatRule = new NatRule(); - newSnatRule.setType("SourceNatRule"); - newSnatRule.setMatch(m); - newSnatRule.setToSourceIpAddressMin(outsideIp); - newSnatRule.setToSourceIpAddressMax(outsideIp); - newSnatRule = _niciraNvpApi.createLogicalRouterNatRule(cmd.getLogicalRouterUuid(), newSnatRule); - s_logger.debug("Created " + natRuleToString(newSnatRule)); + rulepair[0] = _niciraNvpApi.createLogicalRouterNatRule(cmd.getLogicalRouterUuid(), rulepair[0]); + s_logger.debug("Created " + natRuleToString(rulepair[0])); + + try { + rulepair[1] = _niciraNvpApi.createLogicalRouterNatRule(cmd.getLogicalRouterUuid(), rulepair[1]); + s_logger.debug("Created " + natRuleToString(rulepair[1])); + } catch (NiciraNvpApiException ex) { + s_logger.debug("Failed to create SourceNatRule, rolling back DestinationNatRule"); + _niciraNvpApi.deleteLogicalRouterNatRule(cmd.getLogicalRouterUuid(), rulepair[0].getUuid()); + throw ex; // Rethrow original exception + } } } @@ -619,7 +589,6 @@ public class NiciraNvpResource implements ServerResource { return new ConfigureStaticNatRulesOnLogicalRouterAnswer(cmd, e); } } - } private Answer executeRequest(ConfigurePortForwardingRulesOnLogicalRouterCommand cmd, int numRetries) { @@ -630,63 +599,41 @@ public class NiciraNvpResource implements ServerResource { // Any other SourceNat rule should have a corresponding DestinationNat rule for (PortForwardingRuleTO rule : cmd.getRules()) { - if (rule.isAlreadyAdded()) { + if (rule.isAlreadyAdded() && !rule.revoked()) { // Don't need to do anything continue; } - // Find if a DestinationNat rule exists for this rule - String insideIp = rule.getDstIp(); - String insideCidr = rule.getDstIp() + "/32"; - String outsideIp = rule.getSrcIp(); - String outsideCidr = rule.getSrcIp() + "/32"; + NatRule[] rulepair = generatePortForwardingRulePair(rule.getDstIp(), rule.getDstPortRange(), rule.getSrcIp(), rule.getSrcPortRange(), rule.getProtocol()); NatRule incoming = null; NatRule outgoing = null; for (NatRule storedRule : existingRules.getResults()) { - if ("SourceNatRule".equals(storedRule.getType())) { - if (outsideIp.equals(storedRule.getToSourceIpAddressMin()) && - outsideIp.equals(storedRule.getToSourceIpAddressMax()) && - storedRule.getToSourcePortMin() == rule.getSrcPortRange()[0] && - storedRule.getToSourcePortMax() == rule.getSrcPortRange()[1]) { - // The outgoing rule exists - outgoing = storedRule; - } - } - else if ("DestinationNatRule".equals(storedRule.getType())) { - if (insideIp.equals(storedRule.getToDestinationIpAddressMin()) && - insideIp.equals(storedRule.getToDestinationIpAddressMax()) && - storedRule.getToDestinationPort() == rule.getDstPortRange()[0]) { - // The incoming rule exists - incoming = storedRule; - } + if (storedRule.equalsIgnoreUuid(rulepair[1])) { + // The outgoing rule exists + outgoing = storedRule; + s_logger.debug("Found matching outgoing rule " + outgoing.getUuid()); + if (incoming != null) { + break; + } + } + else if (storedRule.equalsIgnoreUuid(rulepair[0])) { + // The incoming rule exists + incoming = storedRule; + s_logger.debug("Found matching incoming rule " + incoming.getUuid()); + if (outgoing != null) { + break; + } } } if (incoming != null && outgoing != null) { - if (insideIp.equals(incoming.getToDestinationIpAddressMin())) { - if (rule.revoked()) { - s_logger.debug("Deleting incoming rule " + incoming.getUuid()); - _niciraNvpApi.deleteLogicalRouterNatRule(cmd.getLogicalRouterUuid(), incoming.getUuid()); - - s_logger.debug("Deleting outgoing rule " + outgoing.getUuid()); - _niciraNvpApi.deleteLogicalRouterNatRule(cmd.getLogicalRouterUuid(), outgoing.getUuid()); - } - } - else { - s_logger.debug("Updating outgoing rule " + outgoing.getUuid()); - outgoing.setToDestinationIpAddressMin(insideIp); - outgoing.setToDestinationIpAddressMax(insideIp); - outgoing.setToDestinationPort(rule.getDstPortRange()[0]); - _niciraNvpApi.modifyLogicalRouterNatRule(cmd.getLogicalRouterUuid(), outgoing); - - s_logger.debug("Updating incoming rule " + outgoing.getUuid()); - incoming.setToSourceIpAddressMin(insideIp); - incoming.setToSourceIpAddressMax(insideIp); - incoming.setToSourcePortMin(rule.getSrcPortRange()[0]); - incoming.setToSourcePortMax(rule.getSrcPortRange()[1]); - _niciraNvpApi.modifyLogicalRouterNatRule(cmd.getLogicalRouterUuid(), incoming); - break; + if (rule.revoked()) { + s_logger.debug("Deleting incoming rule " + incoming.getUuid()); + _niciraNvpApi.deleteLogicalRouterNatRule(cmd.getLogicalRouterUuid(), incoming.getUuid()); + + s_logger.debug("Deleting outgoing rule " + outgoing.getUuid()); + _niciraNvpApi.deleteLogicalRouterNatRule(cmd.getLogicalRouterUuid(), outgoing.getUuid()); } } else { @@ -696,47 +643,17 @@ public class NiciraNvpResource implements ServerResource { break; } - // api createLogicalRouterNatRule - // create the dnat rule - Match m = new Match(); - m.setDestinationIpAddresses(outsideCidr); - if ("tcp".equals(rule.getProtocol())) { - m.setProtocol(6); - } - else if ("udp".equals(rule.getProtocol())) { - m.setProtocol(17); - } - m.setDestinationPortMin(rule.getSrcPortRange()[0]); - m.setDestinationPortMax(rule.getSrcPortRange()[1]); - NatRule newDnatRule = new NatRule(); - newDnatRule.setType("DestinationNatRule"); - newDnatRule.setMatch(m); - newDnatRule.setToDestinationIpAddressMin(insideIp); - newDnatRule.setToDestinationIpAddressMax(insideIp); - newDnatRule.setToDestinationPort(rule.getDstPortRange()[0]); - newDnatRule = _niciraNvpApi.createLogicalRouterNatRule(cmd.getLogicalRouterUuid(), newDnatRule); - s_logger.debug("Created " + natRuleToString(newDnatRule)); + rulepair[0] = _niciraNvpApi.createLogicalRouterNatRule(cmd.getLogicalRouterUuid(), rulepair[0]); + s_logger.debug("Created " + natRuleToString(rulepair[0])); - // create matching snat rule - m = new Match(); - m.setSourceIpAddresses(insideIp + "/32"); - if ("tcp".equals(rule.getProtocol())) { - m.setProtocol(6); + try { + rulepair[1] = _niciraNvpApi.createLogicalRouterNatRule(cmd.getLogicalRouterUuid(), rulepair[1]); + s_logger.debug("Created " + natRuleToString(rulepair[1])); + } catch (NiciraNvpApiException ex) { + s_logger.warn("NiciraNvpApiException during create call, rolling back previous create"); + _niciraNvpApi.deleteLogicalRouterNatRule(cmd.getLogicalRouterUuid(), rulepair[0].getUuid()); + throw ex; // Rethrow the original exception } - else if ("udp".equals(rule.getProtocol())) { - m.setProtocol(17); - } - m.setSourcePortMin(rule.getDstPortRange()[0]); - m.setSourcePortMax(rule.getDstPortRange()[1]); - NatRule newSnatRule = new NatRule(); - newSnatRule.setType("SourceNatRule"); - newSnatRule.setMatch(m); - newSnatRule.setToSourceIpAddressMin(outsideIp); - newSnatRule.setToSourceIpAddressMax(outsideIp); - newSnatRule.setToSourcePortMin(rule.getSrcPortRange()[0]); - newSnatRule.setToSourcePortMax(rule.getSrcPortRange()[1]); - newSnatRule = _niciraNvpApi.createLogicalRouterNatRule(cmd.getLogicalRouterUuid(), newSnatRule); - s_logger.debug("Created " + natRuleToString(newSnatRule)); } } @@ -779,7 +696,7 @@ public class NiciraNvpResource implements ServerResource { natRuleStr.append(" "); natRuleStr.append(m.getSourceIpAddresses()); natRuleStr.append(" ["); - natRuleStr.append(m.getSource_port_min()); + natRuleStr.append(m.getSourcePortMin()); natRuleStr.append("-"); natRuleStr.append(m.getSourcePortMax()); natRuleStr.append(" ] -> "); @@ -819,17 +736,59 @@ public class NiciraNvpResource implements ServerResource { } } - private NatRule[] generateStaticNatRulePair(String insideIp, String outsideIp) { + protected NatRule[] generateStaticNatRulePair(String insideIp, String outsideIp) { NatRule[] rulepair = new NatRule[2]; rulepair[0] = new NatRule(); rulepair[0].setType("DestinationNatRule"); rulepair[1] = new NatRule(); rulepair[1].setType("SourceNatRule"); - //FIXME Implement + Match m = new Match(); + m.setDestinationIpAddresses(outsideIp); + rulepair[0].setMatch(m); + rulepair[0].setToDestinationIpAddressMin(insideIp); + rulepair[0].setToDestinationIpAddressMax(insideIp); + + // create matching snat rule + m = new Match(); + m.setSourceIpAddresses(insideIp); + rulepair[1].setMatch(m); + rulepair[1].setToSourceIpAddressMin(outsideIp); + rulepair[1].setToSourceIpAddressMax(outsideIp); return rulepair; } + protected NatRule[] generatePortForwardingRulePair(String insideIp, int[] insidePorts, String outsideIp, int[] outsidePorts, String protocol) { + // Start with a basic static nat rule, then add port and protocol details + NatRule[] rulepair = generateStaticNatRulePair(insideIp, outsideIp); + + rulepair[0].setToDestinationPort(insidePorts[0]); + rulepair[0].getMatch().setDestinationPortMin(outsidePorts[0]); + rulepair[0].getMatch().setDestinationPortMax(outsidePorts[1]); + rulepair[0].getMatch().setEthertype("IPv4"); + if ("tcp".equals(protocol)) { + rulepair[0].getMatch().setProtocol(6); + } + else if ("udp".equals(protocol)) { + rulepair[0].getMatch().setProtocol(17); + } + + rulepair[1].setToSourcePortMin(outsidePorts[0]); + rulepair[1].setToSourcePortMax(outsidePorts[1]); + rulepair[1].getMatch().setSourcePortMin(insidePorts[0]); + rulepair[1].getMatch().setSourcePortMax(insidePorts[1]); + rulepair[1].getMatch().setEthertype("IPv4"); + if ("tcp".equals(protocol)) { + rulepair[1].getMatch().setProtocol(6); + } + else if ("udp".equals(protocol)) { + rulepair[1].getMatch().setProtocol(17); + } + + return rulepair; + + } + }