-
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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) | ||||||
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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. something like?
Suggested change
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 commentThe 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. |
||||||
""" | ||||||
# This is to send telemetry when iptable rules updated | ||||||
is_firewall_rules_updated = False | ||||||
# If a previous attempt failed, do not retry | ||||||
global _enable_firewall # pylint: disable=W0603 | ||||||
maddieford marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
if not _enable_firewall: | ||||||
return False, is_firewall_rules_updated | ||||||
return False, [] | ||||||
|
||||||
missing_rules = [] | ||||||
|
||||||
try: | ||||||
wait = self.get_firewall_will_wait() | ||||||
|
||||||
# check every iptable rule and delete others if any rule is missing | ||||||
# and append every iptable rule to the end of the chain. | ||||||
try: | ||||||
if not AddFirewallRules.verify_iptables_rules_exist(wait, dst_ip, uid): | ||||||
missing_rules.extend(AddFirewallRules.get_missing_iptables_rules(wait, dst_ip, uid)) | ||||||
if len(missing_rules) > 0: | ||||||
self.remove_firewall(dst_ip, uid, wait) | ||||||
AddFirewallRules.add_iptables_rules(wait, dst_ip, uid) | ||||||
is_firewall_rules_updated = True | ||||||
except CommandError as e: | ||||||
if e.returncode == 2: | ||||||
self.remove_firewall(dst_ip, uid, wait) | ||||||
|
@@ -297,14 +297,14 @@ def enable_firewall(self, dst_ip, uid): | |||||
logger.warn(ustr(error)) | ||||||
raise | ||||||
|
||||||
return True, is_firewall_rules_updated | ||||||
return True, missing_rules | ||||||
|
||||||
except Exception as e: | ||||||
_enable_firewall = False | ||||||
logger.info("Unable to establish firewall -- " | ||||||
"no further attempts will be made: " | ||||||
"{0}".format(ustr(e))) | ||||||
return False, is_firewall_rules_updated | ||||||
return False, missing_rules | ||||||
|
||||||
def get_firewall_list(self, wait=None): | ||||||
try: | ||||||
|
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.