review comments and add test

Author: frogfather

utils/common/src/main/java/org/apache/brooklyn/util/ssh/IptablesCommands.java

@@ -20,6 +20,7 @@
 
 import static org.apache.brooklyn.util.ssh.BashCommands.alternatives;
 import static org.apache.brooklyn.util.ssh.BashCommands.chain;
+import static org.apache.brooklyn.util.ssh.BashCommands.ifExecutableElse1;
 import static org.apache.brooklyn.util.ssh.BashCommands.installPackage;
 import static org.apache.brooklyn.util.ssh.BashCommands.sudo;
 
@@ -94,7 +95,7 @@ public static String iptablesServiceStatus() {
     @Beta // implementation not portable across distros
     public static String firewalldService(String cmd) {
         return sudo(alternatives(
-                BashCommands.ifExecutableElse1("systemctl", "systemctl " + cmd + " firewalld"),
+                ifExecutableElse1("systemctl", "systemctl " + cmd + " firewalld"),
                 "/usr/bin/systemctl " + cmd + " firewalld"));
     }
 
@@ -130,7 +131,8 @@ public static String firewalldServiceIsActive() {
      *
      */
     public static String saveIptablesRules() {
-        return alternatives("if [ ${UID} -eq 0 ] ; then iptables–save > /etc/sysconfig/iptables ; else sudo iptables-save | sudo tee /etc/sysconfig/iptables ; fi",
+        return alternatives(
+                ifExecutableElse1("iptables–save", "if [ ${UID} -eq 0 ] ; then iptables–save > /etc/sysconfig/iptables ; else sudo iptables-save | sudo tee /etc/sysconfig/iptables ; fi"),
                 chain(installPackage("iptables-persistent"), sudo("/etc/init.d/iptables-persistent save")));
     }
 
@@ -140,7 +142,7 @@ public static String saveIptablesRules() {
      * @return Returns the command that cleans up iptables rules.
      */
     public static String cleanUpIptablesRules() {
-       return sudo("/sbin/iptables -F");
+        return sudo("/sbin/iptables -F");
     }
 
     /**
@@ -149,7 +151,7 @@ public static String cleanUpIptablesRules() {
      * @return Returns the command that list all the iptables rules.
      */
     public static String listIptablesRule() {
-       return sudo("/sbin/iptables -L -v -n");
+        return sudo("/sbin/iptables -L -v -n");
     }
 
     /**
@@ -213,7 +215,7 @@ public static String addIptablesRule(String direction, Chain chain, Optional<Str
     public static String addFirewalldRule(Chain chain, org.apache.brooklyn.util.net.Protocol protocol, int port, Policy policy) {
         return addFirewalldRule(chain, Optional.<String>absent(), protocol, port, policy);
     }
-    
+
     /**
      * Returns the command that adds firewalld direct rule.
      *
@@ -222,12 +224,12 @@ public static String addFirewalldRule(Chain chain, org.apache.brooklyn.util.net.
     public static String addFirewalldRule(Chain chain, Optional<String> networkInterface, org.apache.brooklyn.util.net.Protocol protocol, int port, Policy policy) {
         String command = new String("/usr/bin/firewall-cmd");
         String commandPermanent = new String("/usr/bin/firewall-cmd --permanent");
-        
+
         String interfaceParameter = String.format("%s", networkInterface.isPresent() ? " -i " + networkInterface.get() : "");
-        
-        String commandParameters = String.format(" --direct --add-rule ipv4 filter %s 0 %s -p %s --dport %d -j %s", 
+
+        String commandParameters = String.format(" --direct --add-rule ipv4 filter %s 0 %s -p %s --dport %d -j %s",
                                                                 chain, interfaceParameter,  protocol, port, policy);
-        
+
         return sudo(chain(command + commandParameters, commandPermanent + commandParameters));
     }
 }

utils/common/src/test/java/org/apache/brooklyn/util/ssh/IptablesCommandsTest.java

@@ -37,7 +37,7 @@ public class IptablesCommandsTest {
             + "else sudo -E -n -S -- /sbin/iptables -I INPUT -p tcp --dport 3306 -j ACCEPT; fi )";
     private static final String appendIptablesRuleAll = "( if test \"$UID\" -eq 0; then ( /sbin/iptables -A INPUT -p tcp --dport 3306 -j ACCEPT ); "
             + "else sudo -E -n -S -- /sbin/iptables -A INPUT -p tcp --dport 3306 -j ACCEPT; fi )";
-    private static final String saveIptablesRules = "( ( if test \"$UID\" -eq 0; then ( service iptables save ); else sudo -E -n -S -- service iptables save; fi ) || " +
+    private static final String saveIptablesRules = "( { which iptables–save && if [ ${UID} -eq 0 ] ; then iptables–save > /etc/sysconfig/iptables ; else sudo iptables-save | sudo tee /etc/sysconfig/iptables ; fi ; } || " +
             "( ( { which zypper && { echo zypper exists, doing refresh && (( if test \"$UID\" -eq 0; then ( zypper --non-interactive --no-gpg-checks refresh ); else sudo -E -n -S -- zypper --non-interactive --no-gpg-checks refresh; fi ) || true) "
                     + "&& ( if test \"$UID\" -eq 0; then ( zypper --non-interactive --no-gpg-checks install iptables-persistent ); else sudo -E -n -S -- zypper --non-interactive --no-gpg-checks install iptables-persistent; fi ) ; } ; } || " +
             "{ which apt-get && { echo apt-get exists, doing update && export DEBIAN_FRONTEND=noninteractive "