Fix test timing issue + minor code cleanup #3238
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
October 8, 2024 12:42
narrieta
requested review from
ZhidongPeng,
nagworld9 and
maddieford
as code owners
narrieta
commented
Oct 8, 2024
| @@ -72,20 +72,27 @@ def create(wire_server_address): | |||
| """ | |||
| try: | |||
| manager = IpTables(wire_server_address) | |||
| logger.info("Using iptables to manage firewall rules") | |||
| event.info(WALAEventOperation.Firewall, "Using iptables [version {0}] to manage firewall rules", manager.version) | |||
There was a problem hiding this comment.
added telemetry to get the version of iptables/nft/firewall-cmd
narrieta
commented
Oct 8, 2024
| @@ -169,7 +169,8 @@ def test_it_should_restore_missing_firewall_rules(self): | |||
| test_cases = [ # Exit codes for the "-C" (check) command | |||
| {"accept_dns": 1, "accept": 0, "drop": 0, "legacy": 0}, | |||
| {"accept_dns": 0, "accept": 1, "drop": 0, "legacy": 0}, | |||
| {"accept_dns": 0, "accept": 0, "drop": 1, "legacy": 0}, | |||
| {"accept_dns": 0, "accept": 1, "drop": 0, "legacy": 0}, | |||
| {"accept_dns": 1, "accept": 1, "drop": 1, "legacy": 0}, | |||
There was a problem hiding this comment.
added test case for all rules missing
narrieta
commented
Oct 8, 2024
| @@ -33,9 +32,7 @@ def __init__(self, context: AgentVmTestContext): | |||
| self._ssh_client = self._context.create_ssh_client() | |||
|
|
|||
| def run(self): | |||
| log.info("Checking firewall rules added by the agent") | |||
There was a problem hiding this comment.
Removed those messages; they are sort of confusing
narrieta
commented
Oct 8, 2024
| @@ -369,18 +369,6 @@ def get_errors(self) -> List[AgentLogRecord]: | |||
| 'if': lambda r: r.prefix == 'ExtHandler' and r.thread == 'ExtHandler' | |||
| }, | |||
| # | |||
| # TODO: Currently Ubuntu minimal does not include the 'iptables' command. Remove this rule once this has been addressed. | |||
narrieta
commented
Oct 8, 2024
| @@ -264,17 +264,19 @@ def get_state(self) -> str: | |||
| } | |||
|
|
|||
| def get_missing_rules(self) -> List[str]: | |||
| missing = [] | |||
| if "table ip walinuxagent" not in shellutil.run_command(["sudo", "nft", "list", "tables"]): | |||
There was a problem hiding this comment.
add check for case when the table has not been created yet
nagworld9
approved these changes
Oct 8, 2024
maddieford
approved these changes
Oct 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The agent_firewall test may attempt to check the firewall before the Agent has set it up. For iptables, this was not an issue because the firewall is setup by the daemon, but that is not the case for nft.
Added an extra check to verify that the walinuxagent table is already created in NfTables.get_missing_rules().
The Agent may still be in the middle of setting up the table when the test runs. My next PR will add atomic updated for nft, which should take care of that condition.
The PR also includes some minor cleanup that I pointed out as comments.