Setup persistent firewall rules on service restart #2154
Conversation
larohra
requested review from
kevinclark19a,
narrieta and
ZhidongPeng
as code owners
Codecov Report
@@ Coverage Diff @@
## develop #2154 +/- ##
===========================================
+ Coverage 70.48% 70.82% +0.33%
===========================================
Files 95 96 +1
Lines 13104 13367 +263
Branches 1862 1876 +14
===========================================
+ Hits 9237 9467 +230
- Misses 3453 3487 +34
+ Partials 414 413 -1
Continue to review full report at Codecov.
|
| # Check if firewall-rules have already been enabled | ||
| # This function would also return False if the dest-ip is changed. So no need to check separately for that | ||
| try: | ||
| AddFirewallRules.check_firewalld_rule_applied(self._dst_ip, self._uid) |
There was a problem hiding this comment.
How does this check work? raises both if the rules are not enabled or it if fails for some other reason?
There was a problem hiding this comment.
If the service is working properly, it would fail if the firewall rules for the specified data (IP, UID) are not applied.
The check to see if firewalld.service is enabled/running on the box is different, its done before this check - _is_firewall_service_running()
If the firewalld.service is not working properly for external reasons, then it would fail too but that would also mostly fail the initial check, which would make us go the other route and setup our custom service.
I hope that answers your question, if not we can sync offline about this
There was a problem hiding this comment.
we are logging as verbose, so we won't see the messages under normal runs. seems that we would like to know about some of those, e.g. "If the service is working properly, it would fail if the firewall rules for the specified data (IP, UID) are not applied."
There was a problem hiding this comment.
In such a case it would still print a generic message like - "Firewall rules not added yet, adding them now using firewalld.service" or "Service: {0} not enabled. Adding it now".
If you want more data then make it verbose. Initially I added the metadata of these commands as info too (stdout/stderr) but that definitely has a potential of being a bit confusing to the untrained eye so figured I'd make it verbose. Do you have a simpler alternative in mind?
|
There are a couple of INFO messages in the log that look like errors and may be misleading. Can we rephrase them? |
|
|
||
| [Service] | ||
| Type=oneshot | ||
| Environment="EGG={egg_path}" "DST_IP={wire_ip}" "UID={user_id}" "WAIT={wait}" |
There was a problem hiding this comment.
we should probably not parameterize the uid
There was a problem hiding this comment.
I mainly added the UID to make it easier to add more users later on (if ever needed). Can remove it though not a big deal
|
|
||
| add_event( | ||
| op=WALAEventOperation.PersistFirewallRules, | ||
| is_success=cmd_success, |
There was a problem hiding this comment.
but this is the status of the journalctl command; we should get the status of the service
There was a problem hiding this comment.
This is actually the logs of the service (the same logs that you see when we do systemctl status ). It would get the verbose logs of the service but would probably not get the status of it (active, failed, etc). I was initially considering that but then I figured it would be more helpful to get the complete logs than the status. Do you think status makes more sense?
There was a problem hiding this comment.
Btw this is an example of how journalctl logs look like -
2021-02-05T06:57:44.194001Z INFO ExtHandler ExtHandler Logs from the walinuxagent-network-setup.service since system boot: --
Logs begin at Fri 2021-02-05 06:57:36 UTC, end at Fri 2021-02-05 06:57:44 UTC. --
Feb 05 06:57:38 edpwz5814j python3[441]: Setting up firewall for the WALinux Agent with args: {'wait': '-w', 'uid': '0', 'dst_ip': '168.63.129.16'}
Feb 05 06:57:38 edpwz5814j systemd[1]: Started Setup network rules for WALinuxAgent.
There was a problem hiding this comment.
I meant the is_success we send in telemetry. Probably we do not care about failures executing journalctl, but we do care if the firewall rules were not created
There was a problem hiding this comment.
Ahh good point, let me also query the service to see if it was in a failed state or not and send the is-success based on that
Description
This PR ensures that the firewall rules for the agent are persisted over system restarts. This PR does the following -
firewalld.serviceor creating a new unit file for doing the sameIssue #
PR information
Quality of Code and Contribution Guidelines