-
Notifications
You must be signed in to change notification settings - Fork 371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce the frequency of firewall telemetry #3124
Conversation
@@ -266,27 +266,27 @@ def remove_legacy_firewall_rule(self, dst_ip): | |||
|
|||
def enable_firewall(self, dst_ip, uid): | |||
""" | |||
It checks if every iptable rule exists and add them if not present. It returns a tuple(enable firewall success status, update rules flag) | |||
It checks if every iptable rule exists and add them if not present. It returns a tuple(enable firewall success status, missing rules array) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of returning a boolean indicating that the rules were updated, now this method returns an array containing the rules that were missing when the firewall was updated.
enable firewall success status: Returns True if every firewall rule exists otherwise False | ||
update rules flag: Returns True if rules are updated otherwise False | ||
missirg rules: array with names of the missing rules ("ACCEPT DNS", "ACCEPT", "DROP") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should clarify in a comment here that these are the missing rules before reapply operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any suggestions? "It checks if every iptable rule exists" above seems enough to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something like?
missirg rules: array with names of the missing rules ("ACCEPT DNS", "ACCEPT", "DROP") | |
missing rules: array with names of the missing rules before attempt to add them ("ACCEPT DNS", "ACCEPT", "DROP") |
I was just thinking of case where we attempt to re-add the rules and some get added but agent fails to add others. Then we should clarify if the missing rules array was created before or after that attempt.
It's clear from the code though, so might be overkill to add the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll leave it as is. Anyways, all this code is already changed in the fix for nftables and this return value is no longer needed.
@@ -822,13 +822,13 @@ def test_enable_firewall_should_not_use_wait_when_iptables_does_not_support_it(s | |||
success, _ = osutil.DefaultOSUtil().enable_firewall(dst_ip=mock_iptables.destination, uid=mock_iptables.uid) | |||
|
|||
self.assertTrue(success, "Enabling the firewall was not successful") | |||
# Exactly 8 calls have to be made. | |||
# First check rule, delete 4 rules, | |||
# Exactly 10 calls have to be made. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously verify_iptables_rules_exist (now named get_missing_iptables_rules) would return on the first failure; now it always checks all the rules to find all the missing ones. The call count needs to be updated.
* Reduce the frequency of firewall telemetry * python 2: timespan.total_seconds() does not exist * fix unit test --------- Co-authored-by: narrieta <narrieta> (cherry picked from commit 5302651)
Report the first 5 instance of ResetFirewall as they occur, then report at most every 3 hours.
Also changed the Message field to be more concise and easier to read.