Add DNS TCP Firewall rule - Simplified logic #2429
Conversation
Co-authored-by: Kevin Clark <kevin.clark@microsoft.com>
Merge hotfix-2.3.1 into master
Merge release-2.4.0.0 into master
…develop � Conflicts: � azurelinuxagent/common/cgroupconfigurator.py � azurelinuxagent/common/version.py � azurelinuxagent/ga/exthandlers.py � tests/ga/test_multi_config_extension.py
dhivyaganesan
requested review from
kevinclark19a,
larohra,
nagworld9,
narrieta and
ZhidongPeng
as code owners
…refactor-firewall � Conflicts: � tests/ga/test_update.py
|
https://tuxgold.corp.microsoft.com/job/AzLinux/job/DungeonCrawler/job/AgentBVTs.dhivya/186/ DCR run for latest changes @larohra Thanks for the detailed review. Please have a look again when you get a chance . |
tests/ga/test_update.py
Outdated
| drop_check_command = TestOSUtil._command_to_string(osutil.get_firewall_drop_command(mock_iptables.wait, AddFirewallRules.CHECK_COMMAND, mock_iptables.destination)) | ||
|
|
||
| # Filtering the mock iptable command calls with only the once related to this test. | ||
| filtered_mock_iptable_calls = [cmd for cmd in mock_iptables.command_calls if cmd in [drop_check_command]] |
There was a problem hiding this comment.
This will only filter out the drop calls but if the APPEND/CHECK calls for accept_dns rules are there, this test would still succeed. Along with checking if the DROP rule was called, also check if the others were not called to have a more complete test.
There was a problem hiding this comment.
I agree with the suggestion of adding all the applicable rule to filtered_mock_iptable_calls and I addressed it. However, since we are testing the length of filtered_mock_iptable_calls in each test and then checking the order and the particular rule that is called, I don't think it's necessary to check if a rule is not called. If in case a rule that is unintended is called, then the length check will fail(if that is called on top of the normal rules) if not checks where we test the order will fail. Thoughts on this?
There was a problem hiding this comment.
Yeah I agree, just the length check is good enough
|
Green run for the above changes https://tuxgold.corp.microsoft.com/job/AzLinux/job/DungeonCrawler/job/AgentBVTs.dhivya/189/ |
tests/common/osutil/test_default.py
Outdated
| set_command(osutil._get_firewall_drop_command(wait, "-A", destination)) | ||
| set_command(osutil._get_firewall_accept_command(wait, "-A", destination, uid)) | ||
| version_command = set_command(osutil.get_iptables_version_command(), output=str(version)) | ||
| list_command = set_command(osutil.get_firewall_list_command("-w" if wait else ""), output="Mock Output") |
There was a problem hiding this comment.
Not sure where my comment got lost, but since you're using the "-w" if wait else "" in every line, can you make it a variable to keep the code clean? Thanks!
| @@ -223,7 +258,9 @@ def __execute_firewalld_commands(command, dst_ip, uid): | |||
|
|
|||
| @staticmethod | |||
| def add_firewalld_rules(dst_ip, uid): | |||
| # Firwalld.service fails if we set `-w` in the iptables command, so not adding it at all for firewalld commands | |||
| # Firewalld.service fails if we set `-w` in the iptables command, so not adding it at all for firewalld commands | |||
| # Firewalld.service with the "--permanent --passthrough" parameter ensures that a firewall rule is set only once even if command is executed multiple times | |||
There was a problem hiding this comment.
Good comment. I'm having similar question like same rule applying multiple times.
* release prepare-2.4.0.0 (Azure#2280) * Fix bug with dependent extensions with no settings (Azure#2285) * update test-requirements to pin pylint. (Azure#2288) (Azure#2299) Co-authored-by: Kevin Clark <kevin.clark@microsoft.com> * Do not create placeholder status file for AKS extensions (Azure#2298) * Exception for Linux Patch Extension for creating placeholder status file (Azure#2307) * update release version (Azure#2308) * Dont create default status file for Single-Config extensions (Azure#2318) * version update (Azure#2319) * Update * Added new rule * Fixed failing test cases * Added delete and fixed test cases * Added logic to insert the new rule if not present * Added logic to insert the new rule if not present * Added logic to insert the new rule if not present * pylint fix * used insert in permentant firewall * used insert throughout * used insert throughout * used insert throughout * addressed PR comments * addressed PR comments * addressed PR comments * addressed PR comments * addressed PR comments * addressed PR comments * addressed PR comments Co-authored-by: Nageswara Nandigam <84482346+nagworld9@users.noreply.github.com> Co-authored-by: Laveesh Rohra <larohra@microsoft.com> Co-authored-by: Kevin Clark <kevin.clark@microsoft.com> Co-authored-by: Norberto Arrieta <narrieta@users.noreply.github.com> Co-authored-by: narrieta <narrieta>
Modified the way to add the New rule to be more readable
The new rule : iptables -t security -I OUTPUT -d 168.63.129.16 -p tcp --destination-port 53 -j ACCEPT -w
Issue #
PR information
Quality of Code and Contribution Guidelines