Skip to content

Commit 514d1ec

Browse files
DaanHooglandweizhouapache
authored and
dhslove
committed
prevent duplicate ip table rules in SSVM (apache#8530)
Co-authored-by: Wei Zhou <weizhou@apache.org>
1 parent 4cadd5e commit 514d1ec

6 files changed

Lines changed: 105 additions & 38 deletions

File tree

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -529,8 +529,8 @@ public SecondaryStorageVmVO startNew(long dataCenterId, SecondaryStorageVm.Role
529529

530530
/**
531531
* Get the default network for the secondary storage VM, based on the zone it is in. Delegates to
532-
* either {@link #getDefaultNetworkForZone(DataCenter)} or {@link #getDefaultNetworkForAdvancedSGZone(DataCenter)},
533-
* depending on the zone network type and whether or not security groups are enabled in the zone.
532+
* either {@link #getDefaultNetworkForAdvancedZone(DataCenter)} or {@link #getDefaultNetworkForBasicZone(DataCenter)},
533+
* depending on the zone network type and whether security groups are enabled in the zone.
534534
* @param dc - The zone (DataCenter) of the secondary storage VM.
535535
* @return The default network for use with the secondary storage VM.
536536
*/

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

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
//
2+
// Licensed to the Apache Software Foundation (ASF) under one
3+
// or more contributor license agreements. See the NOTICE file
4+
// distributed with this work for additional information
5+
// regarding copyright ownership. The ASF licenses this file
6+
// to you under the Apache License, Version 2.0 (the
7+
// "License"); you may not use this file except in compliance
8+
// with the License. You may obtain a copy of the License at
9+
//
10+
// http://www.apache.org/licenses/LICENSE-2.0
11+
//
12+
// Unless required by applicable law or agreed to in writing,
13+
// software distributed under the License is distributed on an
14+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
// KIND, either express or implied. See the License for the
16+
// specific language governing permissions and limitations
17+
// under the License.
18+
//
19+
20+
package org.apache.cloudstack.storage.resource;
21+
22+
import com.cloud.utils.script.Script;
23+
import org.apache.log4j.Logger;
24+
25+
public class IpTablesHelper {
26+
public static final Logger LOGGER = Logger.getLogger(IpTablesHelper.class);
27+
28+
public static final String OUTPUT_CHAIN = "OUTPUT";
29+
public static final String INPUT_CHAIN = "INPUT";
30+
public static final String INSERT = " -I ";
31+
public static final String APPEND = " -A ";
32+
33+
public static boolean needsAdding(String chain, String rule) {
34+
Script command = new Script("/bin/bash", LOGGER);
35+
command.add("-c");
36+
command.add("iptables -C " + chain + " " + rule);
37+
38+
String commandOutput = command.execute();
39+
boolean needsAdding = (commandOutput != null && commandOutput.contains("iptables: Bad rule (does a matching rule exist in that chain?)."));
40+
LOGGER.debug(String.format("Rule [%s], %s need adding to [%s] : %s",
41+
rule,
42+
needsAdding ? "does indeed" : "doesn't",
43+
chain,
44+
commandOutput
45+
));
46+
return needsAdding;
47+
}
48+
49+
public static String addConditionally(String chain, boolean insert, String rule, String errMsg) {
50+
LOGGER.info(String.format("Adding rule [%s] to [%s] if required.", rule, chain));
51+
if (needsAdding(chain, rule)) {
52+
Script command = new Script("/bin/bash", LOGGER);
53+
command.add("-c");
54+
command.add("iptables" + (insert ? INSERT : APPEND) + chain + " " + rule);
55+
String result = command.execute();
56+
LOGGER.debug(String.format("Executed [%s] with result [%s]", command, result));
57+
if (result != null) {
58+
LOGGER.warn(String.format("%s , err = %s", errMsg, result));
59+
return errMsg + result;
60+
}
61+
} else {
62+
LOGGER.warn("Rule already defined in SVM: " + rule);
63+
}
64+
return null;
65+
}
66+
}

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

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2343,15 +2343,14 @@ public synchronized String allowOutgoingOnPrivate(String destCidr) {
23432343
if (!_inSystemVM) {
23442344
return null;
23452345
}
2346-
Script command = new Script("/bin/bash", logger);
23472346
String intf = "eth1";
2348-
command.add("-c");
2349-
command.add("iptables -I OUTPUT -o " + intf + " -d " + destCidr + " -p tcp -m state --state NEW -m tcp -j ACCEPT");
2347+
String rule = String.format("-o %s -d %s -p tcp -m state --state NEW -m tcp -j ACCEPT", intf, destCidr);
2348+
String errMsg = String.format("Error in allowing outgoing to %s", destCidr);
23502349

2351-
String result = command.execute();
2350+
s_logger.info(String.format("Adding rule if required: " + rule));
2351+
String result = IpTablesHelper.addConditionally(IpTablesHelper.OUTPUT_CHAIN, true, rule, errMsg);
23522352
if (result != null) {
2353-
logger.warn("Error in allowing outgoing to " + destCidr + ", err=" + result);
2354-
return "Error in allowing outgoing to " + destCidr + ", err=" + result;
2353+
return result;
23552354
}
23562355

23572356
addRouteToInternalIpOrCidr(_localgw, _eth1ip, _eth1mask, destCidr);
@@ -2888,13 +2887,8 @@ private void startAdditionalServices() {
28882887
if (result != null) {
28892888
logger.warn("Error in starting sshd service err=" + result);
28902889
}
2891-
command = new Script("/bin/bash", logger);
2892-
command.add("-c");
2893-
command.add("iptables -I INPUT -i eth1 -p tcp -m state --state NEW -m tcp --dport 3922 -j ACCEPT");
2894-
result = command.execute();
2895-
if (result != null) {
2896-
logger.warn("Error in opening up ssh port err=" + result);
2897-
}
2890+
String rule = "-i eth1 -p tcp -m state --state NEW -m tcp --dport 3922 -j ACCEPT";
2891+
IpTablesHelper.addConditionally(IpTablesHelper.INPUT_CHAIN, true, rule, "Error in opening up ssh port");
28982892
}
28992893

29002894
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‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
import org.apache.cloudstack.storage.command.DownloadCommand.ResourceType;
4949
import org.apache.cloudstack.storage.command.DownloadProgressCommand;
5050
import org.apache.cloudstack.storage.command.DownloadProgressCommand.RequestType;
51+
import org.apache.cloudstack.storage.resource.IpTablesHelper;
5152
import org.apache.cloudstack.storage.resource.NfsSecondaryStorageResource;
5253
import org.apache.cloudstack.storage.resource.SecondaryStorageResource;
5354
import org.apache.cloudstack.utils.security.ChecksumValue;
@@ -1226,17 +1227,14 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
12261227
}
12271228

12281229
private void blockOutgoingOnPrivate() {
1229-
Script command = new Script("/bin/bash", logger);
1230-
String intf = "eth1";
1231-
command.add("-c");
1232-
command.add("iptables -A OUTPUT -o " + intf + " -p tcp -m state --state NEW -m tcp --dport " + "80" + " -j REJECT;" + "iptables -A OUTPUT -o " + intf +
1233-
" -p tcp -m state --state NEW -m tcp --dport " + "443" + " -j REJECT;");
1234-
1235-
String result = command.execute();
1236-
if (result != null) {
1237-
logger.warn("Error in blocking outgoing to port 80/443 err=" + result);
1238-
return;
1239-
}
1230+
IpTablesHelper.addConditionally(IpTablesHelper.OUTPUT_CHAIN
1231+
, false
1232+
, "-o " + TemplateConstants.TMPLT_COPY_INTF_PRIVATE + " -p tcp -m state --state NEW -m tcp --dport 80 -j REJECT;"
1233+
, "Error in blocking outgoing to port 80");
1234+
IpTablesHelper.addConditionally(IpTablesHelper.OUTPUT_CHAIN
1235+
, false
1236+
, "-o " + TemplateConstants.TMPLT_COPY_INTF_PRIVATE + " -p tcp -m state --state NEW -m tcp --dport 443 -j REJECT;"
1237+
, "Error in blocking outgoing to port 443");
12401238
}
12411239

12421240
@Override
@@ -1262,17 +1260,19 @@ private void startAdditionalServices() {
12621260
if (result != null) {
12631261
logger.warn("Error in stopping httpd service err=" + result);
12641262
}
1265-
String port = Integer.toString(TemplateConstants.DEFAULT_TMPLT_COPY_PORT);
1266-
String intf = TemplateConstants.DEFAULT_TMPLT_COPY_INTF;
12671263

1268-
command = new Script("/bin/bash", logger);
1269-
command.add("-c");
1270-
command.add("iptables -I INPUT -i " + intf + " -p tcp -m state --state NEW -m tcp --dport " + port + " -j ACCEPT;" + "iptables -I INPUT -i " + intf +
1271-
" -p tcp -m state --state NEW -m tcp --dport " + "443" + " -j ACCEPT;");
1272-
1273-
result = command.execute();
1264+
result = IpTablesHelper.addConditionally(IpTablesHelper.INPUT_CHAIN
1265+
, true
1266+
, "-i " + TemplateConstants.DEFAULT_TMPLT_COPY_INTF + " -p tcp -m state --state NEW -m tcp --dport " + TemplateConstants.DEFAULT_TMPLT_COPY_PORT + " -j ACCEPT"
1267+
, "Error in opening up apache2 port " + TemplateConstants.TMPLT_COPY_INTF_PRIVATE);
1268+
if (result != null) {
1269+
return;
1270+
}
1271+
result = IpTablesHelper.addConditionally(IpTablesHelper.INPUT_CHAIN
1272+
, true
1273+
, "-i " + TemplateConstants.DEFAULT_TMPLT_COPY_INTF + " -p tcp -m state --state NEW -m tcp --dport 443 -j ACCEPT;"
1274+
, "Error in opening up apache2 port 443");
12741275
if (result != null) {
1275-
logger.warn("Error in opening up apache2 port err=" + result);
12761276
return;
12771277
}
12781278

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,7 @@ def save_iptables(command, iptables_file):
221221

222222
def execute2(command, wait=True):
223223
""" Execute command """
224-
logging.info("Executing: %s" % command)
224+
logging.info("Executing2: %s" % command)
225225
p = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
226226
if wait:
227227
p.wait()

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -372,12 +372,19 @@ public String executeIgnoreExitValue(OutputInterpreter interpreter, int exitValu
372372
//process completed successfully
373373
if (_process.exitValue() == 0 || _process.exitValue() == exitValue) {
374374
_logger.debug("Execution is successful.");
375+
String result;
376+
String method;
375377
if (interpreter != null) {
376-
return interpreter.drain() ? task.getResult() : interpreter.interpret(ir);
378+
_logger.debug("interpreting the result...");
379+
method = "result interpretation of execution: ";
380+
result= interpreter.drain() ? task.getResult() : interpreter.interpret(ir);
377381
} else {
378382
// null return exitValue apparently
379-
return String.valueOf(_process.exitValue());
383+
method = "return code of execution: ";
384+
result = String.valueOf(_process.exitValue());
380385
}
386+
_logger.debug(method + result);
387+
return result;
381388
} else { //process failed
382389
break;
383390
}

0 commit comments

Comments
 (0)