Copilot commented on code in PR #12850:
URL: https://github.com/apache/cloudstack/pull/12850#discussion_r3970901992
##########
scripts/vm/network/security_group.py:
##########
@@ -208,42 +244,70 @@ def destroy_network_rules_for_vm(vm_name, vif=None):
chains = [vmchain_default, vmchain, vmchain_egress]
for chain in [_f for _f in chains if _f]:
+ # iptables
+ if iptables_chain_exists(chain):
Review Comment:
Unlike the ip6tables/ipset paths, the iptables existence check does not
handle exit code 127 (command not found). If `iptables` is
missing/misconfigured in an environment where `ip6tables`/`ipset` are optional,
cleanup will raise and abort here rather than skipping. If symmetry is desired,
handle rc=127 in `iptables_chain_exists()` (or in the caller) similarly to the
ip6tables/ipset checks.
##########
scripts/vm/network/security_group.py:
##########
@@ -55,6 +55,42 @@ def execute(cmd):
logging.exception('Command exited non-zero: %s', cmd)
raise
+def iptables_chain_exists(chain):
+ """Check if iptables chain exists."""
+ try:
+ check_output("iptables -S %s 2>/dev/null" % chain, shell=True)
+ return True
Review Comment:
These helpers build shell commands with interpolated `chain`/`setname` and
`shell=True`, which introduces a command-injection risk if VM names (and thus
chain/ipset names) can contain unexpected characters. Prefer invoking
subprocess without a shell (argument list) or, at minimum, properly
quoting/escaping the interpolated values.
##########
scripts/vm/network/security_group.py:
##########
@@ -208,42 +244,70 @@ def destroy_network_rules_for_vm(vm_name, vif=None):
chains = [vmchain_default, vmchain, vmchain_egress]
for chain in [_f for _f in chains if _f]:
+ # iptables
+ if iptables_chain_exists(chain):
+ try:
+ execute("iptables -F " + chain)
+ execute("iptables -X " + chain)
+ except Exception as e:
+ logging.error("Failed to flush/delete iptables chain %s: %s",
chain, str(e))
Review Comment:
`execute()` already logs failures with `logging.exception(...)` before
re-raising, so this code path will typically emit an exception stack trace
*and* this `logging.error(...)`, reintroducing noisy/duplicated logs. Consider
catching `CalledProcessError` and logging once (without stack trace) for this
handled failure, or adding an option to `execute()` to suppress its internal
exception logging when the caller will handle/log errors.
##########
scripts/vm/network/security_group.py:
##########
@@ -208,42 +244,70 @@ def destroy_network_rules_for_vm(vm_name, vif=None):
chains = [vmchain_default, vmchain, vmchain_egress]
for chain in [_f for _f in chains if _f]:
+ # iptables
+ if iptables_chain_exists(chain):
+ try:
+ execute("iptables -F " + chain)
+ execute("iptables -X " + chain)
+ except Exception as e:
+ logging.error("Failed to flush/delete iptables chain %s: %s",
chain, str(e))
+ else:
+ logging.debug("iptables chain %s does not exist, skipping", chain)
+
+ # ip6tables
try:
- execute("iptables -F " + chain)
- execute('ip6tables -F ' + chain)
- except:
- logging.debug("Ignoring failure to flush chain: " + chain)
+ exists = ip6tables_chain_exists(chain)
+ except CalledProcessError as e:
+ if e.returncode == 127:
+ logging.debug("ip6tables command not found while checking
existence of %s, skipping ip6tables cleanup", chain)
+ continue
+ raise
- for chain in [_f for _f in chains if _f]:
+ if exists:
+ try:
+ execute("ip6tables -F " + chain)
+ execute("ip6tables -X " + chain)
+ except Exception as e:
+ logging.error("Failed to flush/delete ip6tables chain %s: %s",
chain, str(e))
+ else:
+ logging.debug("ip6tables chain %s does not exist, skipping", chain)
+
+ for ipset in [vm_ipsetname, vm_ipsetname + '-6']:
try:
- execute("iptables -X " + chain)
- execute('ip6tables -X ' + chain)
- except:
- logging.debug("Ignoring failure to delete chain: " + chain)
+ exists = ipset_exists(ipset)
+ except CalledProcessError as e:
+ if e.returncode == 127:
+ logging.debug("ipset command not found while checking
existence of %s, skipping ipset cleanup", ipset)
+ continue
+ raise
- try:
- for ipset in [vm_ipsetname, vm_ipsetname + '-6']:
- execute('ipset -F ' + ipset)
- execute('ipset -X ' + ipset)
- except:
- logging.debug("Ignoring failure to delete ipset " + vmchain)
+ if exists:
+ try:
+ execute('ipset -F ' + ipset)
+ execute('ipset -X ' + ipset)
+ except Exception as e:
+ logging.error("Failed to flush/delete ipset %s: %s", ipset,
str(e))
+ else:
+ logging.debug("Ipset %s does not exist, skipping", ipset)
Review Comment:
Inconsistent capitalization in log message: use 'ipset' instead of 'Ipset'
to match the command name and other logs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
