prevent duplicate ip table rules in SSVM (#8530)

Co-authored-by: Wei Zhou <weizhou@apache.org>

services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java

@@ -531,8 +531,8 @@ public class SecondaryStorageManagerImpl extends ManagerBase implements Secondar
 
     /**
      * Get the default network for the secondary storage VM, based on the zone it is in. Delegates to
-     * either {@link #getDefaultNetworkForZone(DataCenter)} or {@link #getDefaultNetworkForAdvancedSGZone(DataCenter)},
+     * either {@link #getDefaultNetworkForAdvancedZone(DataCenter)} or {@link #getDefaultNetworkForBasicZone(DataCenter)},
-     * depending on the zone network type and whether or not security groups are enabled in the zone.
+     * depending on the zone network type and whether security groups are enabled in the zone.
      * @param dc - The zone (DataCenter) of the secondary storage VM.
      * @return The default network for use with the secondary storage VM.
      */

services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/IpTablesHelper.java

@@ -0,0 +1,66 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package org.apache.cloudstack.storage.resource;
+
+import com.cloud.utils.script.Script;
+import org.apache.log4j.Logger;
+
+public class IpTablesHelper {
+    public static final Logger LOGGER = Logger.getLogger(IpTablesHelper.class);
+
+    public static final String OUTPUT_CHAIN = "OUTPUT";
+    public static final String INPUT_CHAIN = "INPUT";
+    public static final String INSERT = " -I ";
+    public static final String APPEND = " -A ";
+
+    public static boolean needsAdding(String chain, String rule) {
+        Script command = new Script("/bin/bash", LOGGER);
+        command.add("-c");
+        command.add("iptables -C " + chain + " " + rule);
+
+        String commandOutput = command.execute();
+        boolean needsAdding = (commandOutput != null && commandOutput.contains("iptables: Bad rule (does a matching rule exist in that chain?)."));
+        LOGGER.debug(String.format("Rule [%s], %s need adding to [%s] : %s",
+                rule,
+                needsAdding ? "does indeed" : "doesn't",
+                chain,
+                commandOutput
+        ));
+        return needsAdding;
+    }
+
+    public static String addConditionally(String chain, boolean insert, String rule, String errMsg) {
+        LOGGER.info(String.format("Adding rule [%s] to [%s] if required.", rule, chain));
+        if (needsAdding(chain, rule)) {
+            Script command = new Script("/bin/bash", LOGGER);
+            command.add("-c");
+            command.add("iptables" + (insert ? INSERT : APPEND) + chain + " " + rule);
+            String result = command.execute();
+            LOGGER.debug(String.format("Executed [%s] with result [%s]", command, result));
+            if (result != null) {
+                LOGGER.warn(String.format("%s , err = %s", errMsg, result));
+                return errMsg + result;
+            }
+        } else {
+            LOGGER.warn("Rule already defined in SVM: " + rule);
+        }
+        return null;
+    }
+}

services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java

@@ -2287,15 +2287,14 @@ public class NfsSecondaryStorageResource extends ServerResourceBase implements S
         if (!_inSystemVM) {
             return null;
         }
-        Script command = new Script("/bin/bash", s_logger);
         String intf = "eth1";
-        command.add("-c");
+        String rule =  String.format("-o %s -d %s -p tcp -m state --state NEW -m tcp -j ACCEPT", intf, destCidr);
-        command.add("iptables -I OUTPUT -o " + intf + " -d " + destCidr + " -p tcp -m state --state NEW -m tcp  -j ACCEPT");
+        String errMsg = String.format("Error in allowing outgoing to %s", destCidr);
 
-        String result = command.execute();
+        s_logger.info(String.format("Adding rule if required: " + rule));
+        String result = IpTablesHelper.addConditionally(IpTablesHelper.OUTPUT_CHAIN, true, rule, errMsg);
         if (result != null) {
-            s_logger.warn("Error in allowing outgoing to " + destCidr + ", err=" + result);
+            return result;
-            return "Error in allowing outgoing to " + destCidr + ", err=" + result;
         }
 
         addRouteToInternalIpOrCidr(_localgw, _eth1ip, _eth1mask, destCidr);
@@ -2832,13 +2831,8 @@ public class NfsSecondaryStorageResource extends ServerResourceBase implements S
         if (result != null) {
             s_logger.warn("Error in starting sshd service err=" + result);
         }
-        command = new Script("/bin/bash", s_logger);
+        String rule = "-i eth1 -p tcp -m state --state NEW -m tcp --dport 3922 -j ACCEPT";
-        command.add("-c");
+        IpTablesHelper.addConditionally(IpTablesHelper.INPUT_CHAIN, true, rule, "Error in opening up ssh port");
-        command.add("iptables -I INPUT -i eth1 -p tcp -m state --state NEW -m tcp --dport 3922 -j ACCEPT");
-        result = command.execute();
-        if (result != null) {
-            s_logger.warn("Error in opening up ssh port err=" + result);
-        }
     }
 
     private void addRouteToInternalIpOrCidr(String localgw, String eth1ip, String eth1mask, String destIpOrCidr) {

services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java

@@ -60,6 +60,7 @@ import org.apache.cloudstack.storage.command.DownloadCommand.ResourceType;
 import org.apache.cloudstack.storage.command.DownloadProgressCommand;
 import org.apache.cloudstack.storage.command.DownloadProgressCommand.RequestType;
 import org.apache.cloudstack.storage.NfsMountManagerImpl.PathParser;
+import org.apache.cloudstack.storage.resource.IpTablesHelper;
 import org.apache.cloudstack.storage.resource.NfsSecondaryStorageResource;
 import org.apache.cloudstack.storage.resource.SecondaryStorageResource;
 import org.apache.log4j.Logger;
@@ -1092,17 +1093,14 @@ public class DownloadManagerImpl extends ManagerBase implements DownloadManager
     }
 
     private void blockOutgoingOnPrivate() {
-        Script command = new Script("/bin/bash", LOGGER);
+        IpTablesHelper.addConditionally(IpTablesHelper.OUTPUT_CHAIN
-        String intf = "eth1";
+                , false
-        command.add("-c");
+                , "-o " + TemplateConstants.TMPLT_COPY_INTF_PRIVATE + " -p tcp -m state --state NEW -m tcp --dport 80 -j REJECT;"
-        command.add("iptables -A OUTPUT -o " + intf + " -p tcp -m state --state NEW -m tcp --dport " + "80" + " -j REJECT;" + "iptables -A OUTPUT -o " + intf +
+                , "Error in blocking outgoing to port 80");
-                " -p tcp -m state --state NEW -m tcp --dport " + "443" + " -j REJECT;");
+        IpTablesHelper.addConditionally(IpTablesHelper.OUTPUT_CHAIN
-
+                , false
-        String result = command.execute();
+                , "-o " + TemplateConstants.TMPLT_COPY_INTF_PRIVATE + " -p tcp -m state --state NEW -m tcp --dport 443 -j REJECT;"
-        if (result != null) {
+                , "Error in blocking outgoing to port 443");
-            LOGGER.warn("Error in blocking outgoing to port 80/443 err=" + result);
-            return;
-        }
     }
 
     @Override
@@ -1128,17 +1126,19 @@ public class DownloadManagerImpl extends ManagerBase implements DownloadManager
         if (result != null) {
             LOGGER.warn("Error in stopping httpd service err=" + result);
         }
-        String port = Integer.toString(TemplateConstants.DEFAULT_TMPLT_COPY_PORT);
-        String intf = TemplateConstants.DEFAULT_TMPLT_COPY_INTF;
 
-        command = new Script("/bin/bash", LOGGER);
+        result = IpTablesHelper.addConditionally(IpTablesHelper.INPUT_CHAIN
-        command.add("-c");
+                , true
-        command.add("iptables -I INPUT -i " + intf + " -p tcp -m state --state NEW -m tcp --dport " + port + " -j ACCEPT;" + "iptables -I INPUT -i " + intf +
+                , "-i " + TemplateConstants.DEFAULT_TMPLT_COPY_INTF + " -p tcp -m state --state NEW -m tcp --dport " + TemplateConstants.DEFAULT_TMPLT_COPY_PORT + " -j ACCEPT"
-                " -p tcp -m state --state NEW -m tcp --dport " + "443" + " -j ACCEPT;");
+                , "Error in opening up apache2 port " + TemplateConstants.TMPLT_COPY_INTF_PRIVATE);
-
+        if (result != null) {
-        result = command.execute();
+            return;
+        }
+        result = IpTablesHelper.addConditionally(IpTablesHelper.INPUT_CHAIN
+                , true
+                , "-i " + TemplateConstants.DEFAULT_TMPLT_COPY_INTF + " -p tcp -m state --state NEW -m tcp --dport 443 -j ACCEPT;"
+                , "Error in opening up apache2 port 443");
         if (result != null) {
-            LOGGER.warn("Error in opening up apache2 port err=" + result);
             return;
         }
 

systemvm/debian/opt/cloud/bin/cs/CsHelper.py

@@ -221,7 +221,7 @@ def save_iptables(command, iptables_file):
 
 def execute2(command, wait=True):
     """ Execute command """
-    logging.info("Executing: %s" % command)
+    logging.info("Executing2: %s" % command)
     p = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
     if wait:
         p.wait()

utils/src/main/java/com/cloud/utils/script/Script.java

@@ -243,12 +243,19 @@ public class Script implements Callable<String> {
                         //process completed successfully
                         if (_process.exitValue() == 0) {
                             _logger.debug("Execution is successful.");
+                            String result;
+                            String method;
                             if (interpreter != null) {
-                                return interpreter.drain() ? task.getResult() : interpreter.interpret(ir);
+                                _logger.debug("interpreting the result...");
+                                method = "result interpretation of execution: ";
+                                result= interpreter.drain() ? task.getResult() : interpreter.interpret(ir);
                             } else {
                                 // null return exitValue apparently
-                                return String.valueOf(_process.exitValue());
+                                method = "return code of execution: ";
+                                result = String.valueOf(_process.exitValue());
                             }
+                            _logger.debug(method + result);
+                            return result;
                         } else { //process failed
                             break;
                         }