Use run_command to execute iptables #1944
Conversation
narrieta
requested review from
kevinclark19a,
larohra,
pgombar and
ZhidongPeng
as code owners
Codecov Report
@@ Coverage Diff @@
## develop #1944 +/- ##
===========================================
+ Coverage 69.56% 69.62% +0.05%
===========================================
Files 85 85
Lines 11907 11926 +19
Branches 1673 1667 -6
===========================================
+ Hits 8283 8303 +20
- Misses 3252 3254 +2
+ Partials 372 369 -3
Continue to review full report at Codecov.
|
| rc = shellutil.run_command(rule) | ||
| except CommandError as e: | ||
| if e.returncode == 1: | ||
| return |
There was a problem hiding this comment.
Why is a returncode of 1 not an error? Should we document this as a comment?
There was a problem hiding this comment.
I image the code was written like this to avoid flagging an error if the rule does not exist, though this is not very robust.
I was tempted to change the logic, but at the end decided against it. I am not sure about the intention of the loop either.
| FIREWALL_DROP = "iptables {0} -t security -{1} OUTPUT -d {2} -p tcp -m conntrack --ctstate INVALID,NEW -j DROP" | ||
| FIREWALL_LIST = "iptables {0} -t security -L -nxv" | ||
| FIREWALL_PACKETS = "iptables {0} -t security -L OUTPUT --zero OUTPUT -nxv" | ||
| FIREWALL_FLUSH = "iptables {0} -t security --flush" |
There was a problem hiding this comment.
What happened to this command?
There was a problem hiding this comment.
Ah, I see it's not being used anywhere else.
| # Transient error that we ignore. This code fires every loop | ||
| # of the daemon (60m), so we will get the value eventually. | ||
| return 0 | ||
| logger.warn("Failed to get firewall packets: {0}", ustr(e)) |
There was a problem hiding this comment.
We didn't use to log in this case. Can this be too verbose?
There was a problem hiding this comment.
we did: run_get_output logs by default (that's why I had to add the "expected_errors" stuff in the old function)
There was a problem hiding this comment.
My bad, I saw log_cmd=False in the old function and interpreted it as not logging the error, whereas that's the chk_err flag.
There was a problem hiding this comment.
yes, those 2 flags are confusing
tests/common/osutil/test_default.py
Outdated
| ]) | ||
| self.assertFalse(osutil._enable_firewall) | ||
| with TestOSUtil._mock_iptables() as mock_iptables: | ||
| delete_conntrack_accept_command = mock_iptables.set_command(osutil._get_firewall_delete_conntrack_accept_command(mock_iptables.wait, mock_iptables.destination), exit_code=2) |
There was a problem hiding this comment.
Why is this an invalid rule? It's the one we phased out after 2.2.25?
There was a problem hiding this comment.
It is confusing, I'll add a comment.
The test tries to hit the path where we check for exit code 2 (invalid rule), but the agent uses only only valid rules, so the mocked command specifies a valid rule, but the return code indicates invalid. When I ported the tests I thought this was odd; should have added a comment then.
There was a problem hiding this comment.
Now, in the previous version of the code this was not apparent, since the mock was using magic (deep knowledge of the impementation) and mocks like this:
mock_run.side_effect = [2]
mock_output.side_effect = [(0, version), (1, "Output")]
tests/common/osutil/test_default.py
Outdated
| ''')] | ||
| dst = '168.63.129.16' | ||
| ''') | ||
| self.assertEqual(32, osutil.DefaultOSUtil().get_firewall_dropped_packets('168.63.129.16')) |
There was a problem hiding this comment.
this should use 'destination'
iptables was using the deprecated run/run_get_output functions