You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by ro...@apache.org on 2019/07/08 10:16:27 UTC
[cloudstack] branch master updated: plugins: fix removing SRX port
forwarding rules, improve add/remove logic (#3393)
This is an automated email from the ASF dual-hosted git repository.
rohit pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/master by this push:
new d70f574 plugins: fix removing SRX port forwarding rules, improve add/remove logic (#3393)
d70f574 is described below
commit d70f574a7ef949e6385ff594cccb8260b5a1f0ee
Author: Richard Lawley <ri...@richardlawley.com>
AuthorDate: Mon Jul 8 11:16:12 2019 +0100
plugins: fix removing SRX port forwarding rules, improve add/remove logic (#3393)
This PR partially fixes the logic around port forwarding rules on the Juniper SRX plugin. The code in the plugin is based on JunOS 10, which is very old. The changes here should not break compatibility, but should enable the plugin to be used on newer devices. Note that an additional change to a script file is required to be able to add port forwarding rules, but as this PR was targetted for 4.11.3, I thought it best not to include this change as it might break compatibility for anyon [...]
I've made the logic better and consistent for adding/removing static nat and port forwarding rules - these were multi-step processes which did not check each individual step. This would aid in manually fixing rules in case of further problems.
I've also improved the logging for communication with the SRX by stripping out the Apache header before sending it, and indicating the name of the template filename in use.
To be able to add port forwarding rules, the <dst-port> tags in dest-nat-rule-add.xml must be changed to <low>.
Fixes: #3379
---
.../cloud/network/resource/JuniperSrxResource.java | 247 ++++++++++++---------
scripts/network/juniper/dest-nat-rule-add.xml | 2 +-
2 files changed, 138 insertions(+), 111 deletions(-)
diff --git a/plugins/network-elements/juniper-srx/src/main/java/com/cloud/network/resource/JuniperSrxResource.java b/plugins/network-elements/juniper-srx/src/main/java/com/cloud/network/resource/JuniperSrxResource.java
index 8ada819..04d4c8c 100644
--- a/plugins/network-elements/juniper-srx/src/main/java/com/cloud/network/resource/JuniperSrxResource.java
+++ b/plugins/network-elements/juniper-srx/src/main/java/com/cloud/network/resource/JuniperSrxResource.java
@@ -177,7 +177,15 @@ public class JuniperSrxResource implements ServerResource {
private static final Logger s_logger = Logger.getLogger(JuniperSrxResource.class);
private SrxXml(String filename) {
- xml = getXml(filename);
+ String contents = getXml(filename);
+
+ // Strip the apache header and add the filename as a header to aid debugging
+ contents = contents.replaceAll( "(?s)<!--.*?-->", "" ).trim();
+ if (!contents.startsWith("<?")) {
+ contents = "<!-- " + filename + " -->" + contents;
+ }
+
+ xml = contents;
}
public String getXml() {
@@ -2031,62 +2039,69 @@ public class JuniperSrxResource implements ServerResource {
xml = replaceXmlValue(xml, "rule-name", ruleName_private);
return sendRequestAndCheckResponse(command, xml, "name", ruleName_private);
case ADD:
- if (manageStaticNatRule(SrxCommand.CHECK_IF_EXISTS, publicIp, privateIp)) {
- return true;
- }
-
- xml = SrxXml.STATIC_NAT_RULE_ADD.getXml();
- xml = replaceXmlValue(xml, "rule-set", _publicZone);
- xml = replaceXmlValue(xml, "from-zone", _publicZone);
- xml = replaceXmlValue(xml, "rule-name", ruleName);
- xml = replaceXmlValue(xml, "original-ip", publicIp);
- xml = replaceXmlValue(xml, "translated-ip", privateIp);
+ if (!manageStaticNatRule(SrxCommand.CHECK_IF_EXISTS, publicIp, privateIp)) {
+ xml = SrxXml.STATIC_NAT_RULE_ADD.getXml();
+ xml = replaceXmlValue(xml, "rule-set", _publicZone);
+ xml = replaceXmlValue(xml, "from-zone", _publicZone);
+ xml = replaceXmlValue(xml, "rule-name", ruleName);
+ xml = replaceXmlValue(xml, "original-ip", publicIp);
+ xml = replaceXmlValue(xml, "translated-ip", privateIp);
- if (!sendRequestAndCheckResponse(command, xml)) {
- throw new ExecutionException("Failed to add static NAT rule from public IP " + publicIp + " to private IP " + privateIp);
+ if (!sendRequestAndCheckResponse(command, xml)) {
+ throw new ExecutionException(String.format("Failed to add static NAT rule %s -> %s on %s ", publicIp, privateIp, _publicZone));
+ }
} else {
+ s_logger.debug(String.format("Static NAT rule %s -> %s on %s already exists", publicIp, privateIp, _publicZone));
+ }
+
+ if (!manageStaticNatRule(SrxCommand.CHECK_PRIVATE_IF_EXISTS, publicIp, privateIp)) {
xml = SrxXml.STATIC_NAT_RULE_ADD.getXml();
xml = replaceXmlValue(xml, "rule-set", _privateZone);
xml = replaceXmlValue(xml, "from-zone", _privateZone);
xml = replaceXmlValue(xml, "rule-name", ruleName_private);
xml = replaceXmlValue(xml, "original-ip", publicIp);
xml = replaceXmlValue(xml, "translated-ip", privateIp);
- if (!sendRequestAndCheckResponse(command, xml))
- {
- throw new ExecutionException("Failed to add trust static NAT rule from public IP " + publicIp + " to private IP " + privateIp);
+ if (!sendRequestAndCheckResponse(command, xml)) {
+ throw new ExecutionException(String.format("Failed to add static NAT rule %s -> %s on %s ", publicIp, privateIp, _privateZone));
}
- return true;
+ } else {
+ s_logger.debug(String.format("Static NAT rule %s -> %s on %s already exists", publicIp, privateIp, _privateZone));
}
+ return true;
+
case DELETE:
- if (!manageStaticNatRule(SrxCommand.CHECK_IF_EXISTS, publicIp, privateIp)) {
- return true;
+ if (manageStaticNatRule(SrxCommand.CHECK_IF_EXISTS, publicIp, privateIp)) {
+ xml = SrxXml.STATIC_NAT_RULE_GETONE.getXml();
+ xml = setDelete(xml, true);
+ xml = replaceXmlValue(xml, "rule-set", _publicZone);
+ xml = replaceXmlValue(xml, "from-zone", _publicZone);
+ xml = replaceXmlValue(xml, "rule-name", ruleName);
+
+ if (!sendRequestAndCheckResponse(command, xml, "name", ruleName)) {
+ throw new ExecutionException(String.format("Failed to delete static NAT rule %s -> %s on %s", publicIp, privateIp, _publicZone));
+ }
+ } else {
+ s_logger.debug(String.format("Static NAT rule %s -> %s on %s not found", publicIp, privateIp, _publicZone));
}
- xml = SrxXml.STATIC_NAT_RULE_GETONE.getXml();
- xml = setDelete(xml, true);
- xml = replaceXmlValue(xml, "rule-set", _publicZone);
- xml = replaceXmlValue(xml, "from-zone", _publicZone);
- xml = replaceXmlValue(xml, "rule-name", ruleName);
-
- if (!sendRequestAndCheckResponse(command, xml, "name", ruleName)) {
- throw new ExecutionException("Failed to delete static NAT rule from public IP " + publicIp + " to private IP " + privateIp);
- } else {
- if (manageStaticNatRule(SrxCommand.CHECK_PRIVATE_IF_EXISTS, publicIp, privateIp)){
- xml = SrxXml.STATIC_NAT_RULE_GETONE.getXml();
- xml = setDelete(xml, true);
- xml = replaceXmlValue(xml, "rule-set", _privateZone);
- xml = replaceXmlValue(xml, "from-zone", _privateZone);
- xml = replaceXmlValue(xml, "rule-name", ruleName_private);
+ if (manageStaticNatRule(SrxCommand.CHECK_PRIVATE_IF_EXISTS, publicIp, privateIp)){
+ xml = SrxXml.STATIC_NAT_RULE_GETONE.getXml();
+ xml = setDelete(xml, true);
+ xml = replaceXmlValue(xml, "rule-set", _privateZone);
+ xml = replaceXmlValue(xml, "from-zone", _privateZone);
+ xml = replaceXmlValue(xml, "rule-name", ruleName_private);
- if (!sendRequestAndCheckResponse(command, xml, "name", ruleName_private))
- {
- throw new ExecutionException("Failed to delete trust static NAT rule from public IP " + publicIp + " to private IP " + privateIp);
- }
+ if (!sendRequestAndCheckResponse(command, xml, "name", ruleName_private))
+ {
+ throw new ExecutionException(String.format("Failed to delete static NAT rule %s -> %s on %s", publicIp, privateIp, _privateZone));
}
- return true;
+ } else {
+ s_logger.debug(String.format("Static NAT rule %s -> %s on %s not found", publicIp, privateIp, _privateZone));
}
+ return true;
+
default:
throw new ExecutionException("Unrecognized command.");
@@ -2163,39 +2178,39 @@ public class JuniperSrxResource implements ServerResource {
return sendRequestAndCheckResponse(command, xml, "pool-name", poolName);
case ADD:
- if (manageDestinationNatPool(SrxCommand.CHECK_IF_EXISTS, privateIp, destPort)) {
- return true;
- }
-
- xml = SrxXml.DEST_NAT_POOL_ADD.getXml();
- xml = replaceXmlValue(xml, "pool-name", poolName);
- xml = replaceXmlValue(xml, "private-address", privateIp + "/32");
- xml = replaceXmlValue(xml, "dest-port", String.valueOf(destPort));
+ if (!manageDestinationNatPool(SrxCommand.CHECK_IF_EXISTS, privateIp, destPort)) {
+ xml = SrxXml.DEST_NAT_POOL_ADD.getXml();
+ xml = replaceXmlValue(xml, "pool-name", poolName);
+ xml = replaceXmlValue(xml, "private-address", privateIp + "/32");
+ xml = replaceXmlValue(xml, "dest-port", String.valueOf(destPort));
- if (!sendRequestAndCheckResponse(command, xml)) {
- throw new ExecutionException("Failed to add destination NAT pool for private IP " + privateIp + " and private port " + destPort);
+ if (!sendRequestAndCheckResponse(command, xml)) {
+ throw new ExecutionException(String.format("Failed to add Destination NAT pool for %s:%s", privateIp, destPort));
+ }
} else {
+ s_logger.debug(String.format("Destination NAT pool for %s:%s already exists", privateIp, destPort));
return true;
}
- case DELETE:
- if (!manageDestinationNatPool(SrxCommand.CHECK_IF_EXISTS, privateIp, destPort)) {
- return true;
- }
+ return true;
- if (manageDestinationNatPool(SrxCommand.CHECK_IF_IN_USE, privateIp, destPort)) {
- return true;
- }
-
- xml = SrxXml.DEST_NAT_POOL_GETONE.getXml();
- xml = setDelete(xml, true);
- xml = replaceXmlValue(xml, "pool-name", poolName);
+ case DELETE:
+ if (manageDestinationNatPool(SrxCommand.CHECK_IF_EXISTS, privateIp, destPort)) {
+ if (!manageDestinationNatPool(SrxCommand.CHECK_IF_IN_USE, privateIp, destPort)) {
+ xml = SrxXml.DEST_NAT_POOL_GETONE.getXml();
+ xml = setDelete(xml, true);
+ xml = replaceXmlValue(xml, "pool-name", poolName);
- if (!sendRequestAndCheckResponse(command, xml)) {
- throw new ExecutionException("Failed to delete destination NAT pool for private IP " + privateIp + " and private port " + destPort);
+ if (!sendRequestAndCheckResponse(command, xml)) {
+ throw new ExecutionException(String.format("Failed to delete Destination NAT pool for %s:%s", privateIp, destPort));
+ }
+ } else {
+ s_logger.debug(String.format("Destination NAT pool for %s:%s is in use, not deleting", privateIp, destPort));
+ }
} else {
- return true;
+ s_logger.debug(String.format("Did not find Destination NAT pool for %s:%s to delete", privateIp, destPort));
}
+ return true;
default:
throw new ExecutionException("Unrecognized command.");
@@ -2234,28 +2249,31 @@ public class JuniperSrxResource implements ServerResource {
xml = replaceXmlValue(xml, "rule-name", ruleName_private);
return sendRequestAndCheckResponse(command, xml, "name", ruleName_private);
case ADD:
- if (manageDestinationNatRule(SrxCommand.CHECK_IF_EXISTS, publicIp, privateIp, srcPort, destPort)) {
- return true;
- }
-
- if (!manageDestinationNatPool(SrxCommand.CHECK_IF_EXISTS, privateIp, destPort)) {
- throw new ExecutionException("The destination NAT pool corresponding to private IP: " + privateIp + " and destination port: " + destPort +
- " does not exist.");
- }
+ // Add untrust rule
+ if (!manageDestinationNatRule(SrxCommand.CHECK_IF_EXISTS, publicIp, privateIp, srcPort, destPort)) {
+ if (!manageDestinationNatPool(SrxCommand.CHECK_IF_EXISTS, privateIp, destPort)) { // Added elsewhere
+ throw new ExecutionException(String.format("Destination NAT pool for %s:%s does not exist", privateIp, destPort));
+ }
- xml = SrxXml.DEST_NAT_RULE_ADD.getXml();
- xml = replaceXmlValue(xml, "rule-set", _publicZone);
- xml = replaceXmlValue(xml, "from-zone", _publicZone);
- xml = replaceXmlValue(xml, "rule-name", ruleName);
- xml = replaceXmlValue(xml, "public-address", publicIp);
- xml = replaceXmlValue(xml, "src-port", String.valueOf(srcPort));
- xml = replaceXmlValue(xml, "pool-name", poolName);
+ xml = SrxXml.DEST_NAT_RULE_ADD.getXml();
+ xml = replaceXmlValue(xml, "rule-set", _publicZone);
+ xml = replaceXmlValue(xml, "from-zone", _publicZone);
+ xml = replaceXmlValue(xml, "rule-name", ruleName);
+ xml = replaceXmlValue(xml, "public-address", publicIp);
+ xml = replaceXmlValue(xml, "src-port", String.valueOf(srcPort));
+ xml = replaceXmlValue(xml, "pool-name", poolName);
- if (!sendRequestAndCheckResponse(command, xml)) {
- throw new ExecutionException("Failed to add destination NAT rule from public IP " + publicIp + ", public port " + srcPort + ", private IP " +
- privateIp + ", and private port " + destPort);
+ if (!sendRequestAndCheckResponse(command, xml)) {
+ throw new ExecutionException(String.format("Failed to add Destination NAT rule %s:%s -> %s:%s on %s",
+ publicIp, srcPort, privateIp, destPort, _publicZone));
+ }
} else {
+ s_logger.debug(String.format("Destination NAT rule for %s:%s -> %s:%s on %s already exists",
+ publicIp, srcPort, privateIp, destPort, _publicZone));
+ }
+ // Add trust rule
+ if (!manageDestinationNatRule(SrxCommand.CHECK_PRIVATE_IF_EXISTS, publicIp, privateIp, srcPort, destPort)) {
xml = SrxXml.DEST_NAT_RULE_ADD.getXml();
xml = replaceXmlValue(xml, "rule-set", _privateZone);
xml = replaceXmlValue(xml, "from-zone", _privateZone);
@@ -2266,45 +2284,54 @@ public class JuniperSrxResource implements ServerResource {
if (!sendRequestAndCheckResponse(command, xml))
{
- s_logger.debug("Purple: loopback Failed to add " + _privateZone + " destination NAT rule from public IP " + publicIp + ", public port " + srcPort + ", private IP " +
- privateIp + ", and private port " + destPort);
+ throw new ExecutionException(String.format("Failed to add Destination NAT rule %s:%s -> %s:%s on %s",
+ publicIp, srcPort, privateIp, destPort, _privateZone));
}
- return true;
+ } else {
+ s_logger.debug(String.format("Destination NAT rule for %s:%s -> %s:%s on %s already exists",
+ publicIp, srcPort, privateIp, destPort, _privateZone));
}
+ return true;
+
case DELETE:
- if (!manageDestinationNatRule(SrxCommand.CHECK_IF_EXISTS, publicIp, privateIp, srcPort, destPort)) {
- return true;
+ // Delete public rule
+ if (manageDestinationNatRule(SrxCommand.CHECK_IF_EXISTS, publicIp, privateIp, srcPort, destPort)) {
+ xml = SrxXml.DEST_NAT_RULE_GETONE.getXml();
+ xml = setDelete(xml, true);
+ xml = replaceXmlValue(xml, "rule-set", _publicZone);
+ xml = replaceXmlValue(xml, "from-zone", _publicZone);
+ xml = replaceXmlValue(xml, "rule-name", ruleName);
+
+ if (!sendRequestAndCheckResponse(command, xml)) {
+ throw new ExecutionException(String.format("Failed to delete destination NAT rule %s[%s] -> %s[%s] on rule %s",
+ publicIp, srcPort, privateIp, destPort, _publicZone));
+ }
+ } else {
+ s_logger.debug(String.format("Destination NAT rule %s[%s] -> %s[%s] not found on %s, not deleting",
+ publicIp, srcPort, privateIp, destPort, _publicZone));
}
- xml = SrxXml.DEST_NAT_RULE_GETONE.getXml();
- xml = setDelete(xml, true);
- xml = replaceXmlValue(xml, "rule-set", _publicZone);
- xml = replaceXmlValue(xml, "from-zone", _publicZone);
- xml = replaceXmlValue(xml, "rule-name", ruleName);
+ // Delete private rule
+ if (manageDestinationNatRule(SrxCommand.CHECK_PRIVATE_IF_EXISTS, publicIp, privateIp, srcPort, destPort)) {
+ xml = SrxXml.DEST_NAT_RULE_GETONE.getXml();
+ xml = setDelete(xml, true);
+ xml = replaceXmlValue(xml, "rule-set", _privateZone);
+ xml = replaceXmlValue(xml, "from-zone", _privateZone);
+ xml = replaceXmlValue(xml, "rule-name", ruleName_private);
- if (!sendRequestAndCheckResponse(command, xml)) {
- throw new ExecutionException("Failed to delete destination NAT rule from public IP " + publicIp + ", public port " + srcPort + ", private IP " +
- privateIp + ", and private port " + destPort);
- } else {
- if (manageDestinationNatRule(SrxCommand.CHECK_PRIVATE_IF_EXISTS, publicIp, privateIp, srcPort, destPort))
+ if (!sendRequestAndCheckResponse(command, xml))
{
- xml = SrxXml.DEST_NAT_RULE_GETONE.getXml();
- xml = setDelete(xml, true);
- xml = replaceXmlValue(xml, "rule-set", _privateZone);
- xml = replaceXmlValue(xml, "from-zone", _privateZone);
- xml = replaceXmlValue(xml, "rule-name", ruleName_private);
-
- if (!sendRequestAndCheckResponse(command, xml))
- {
- s_logger.debug("Purple: Failed to delete " + _privateZone + " destination NAT rule from public IP " + publicIp + ", public port " + srcPort + ", private IP " +
- privateIp + ", and private port " + destPort);
- }
+ throw new ExecutionException(String.format("Failed to delete destination NAT rule %s[%s] -> %s[%s] on rule %s",
+ publicIp, srcPort, privateIp, destPort, _privateZone));
}
-
- return true;
+ } else {
+ s_logger.debug(String.format("Destination NAT rule %s[%s] -> %s[%s] not found on %s, not deleting",
+ publicIp, srcPort, privateIp, destPort, _privateZone));
}
+ return true;
+
default:
s_logger.debug("Unrecognized command.");
return false;
@@ -2345,7 +2372,7 @@ public class JuniperSrxResource implements ServerResource {
NodeList destPortEntries = ruleMatchEntry.getChildNodes();
for (int destPortIndex = 0; destPortIndex < destPortEntries.getLength(); destPortIndex++) {
Node destPortEntry = destPortEntries.item(destPortIndex);
- if (destPortEntry.getNodeName().equals("dst-port")) {
+ if (destPortEntry.getNodeName().equals("dst-port") || destPortEntry.getNodeName().equals("name")) {
ruleSrcPort = destPortEntry.getFirstChild().getNodeValue();
}
}
diff --git a/scripts/network/juniper/dest-nat-rule-add.xml b/scripts/network/juniper/dest-nat-rule-add.xml
index 559b86c..2ef1df2 100644
--- a/scripts/network/juniper/dest-nat-rule-add.xml
+++ b/scripts/network/juniper/dest-nat-rule-add.xml
@@ -32,7 +32,7 @@ under the License.
<dst-addr>%public-address%</dst-addr>
</destination-address>
<destination-port>
-<dst-port>%src-port%</dst-port>
+<low>%src-port%</low>
</destination-port>
</dest-nat-rule-match>
<then>