Add support for nftables + refactoring of firewall code #3221

narrieta · 2024-09-19T16:33:52Z

Adding support for nftables.

The code for setting up the firewall was scattered across multiple places and was not straightforward to follow up. Adding support for nftables on top of the existing code would have made things worse, so this PR also includes significant refactoring of that code and associated tests.

I also moved the code from the 'common' subdirectory to 'ga', since it is not needed by the provisioning agent.

narrieta · 2024-09-19T16:49:59Z

azurelinuxagent/agent.py

@@ -35,6 +35,7 @@
 from azurelinuxagent.ga.cgroupcontroller import AGENT_LOG_COLLECTOR
 from azurelinuxagent.ga.cpucontroller import _CpuController
 from azurelinuxagent.ga.cgroupapi import get_cgroup_api, log_cgroup_warning, InvalidCgroupMountpointException
+from azurelinuxagent.ga.firewall_manager import FirewallManager


FirewallManager is the new class that includes all the firewall functionality previously on osutil, AddFirewallRules, etc

narrieta · 2024-09-19T19:13:29Z

azurelinuxagent/agent.py

-            print("Successfully set the firewall rules")
+            firewall_manager = FirewallManager.create(endpoint)
+            firewall_manager.setup()
+            event.info(event.WALAEventOperation.Firewall, "Successfully set the firewall rules")


I create some convenience functions to log and send telemetry (see event.py)

narrieta · 2024-09-19T19:15:36Z

azurelinuxagent/agent.py

        else:
-            cmd = AgentCommands.Help
-            break
+            regex_cmd = regex_cmd_format.format("{0}=(?P<endpoint>[\\d.]{{7,}})".format(AgentCommands.SetupFirewall))


now the command line takes only "-setup-firewall="

Nit, I think the SetupFirewall regex check should be in a separate elif condition for readability.
That way the only thing in the else condition is Help command, and it will make it easier to add more commands later

Makes sense, though I need the result of the match, so I do not think I can use an elif (unless I evaluate twice). Any suggestions?

I see, maybe do the match before the first if condition. Although that may defeat the intent of making this more readable. If moving it creates confusion, i'm ok with it as is

Yes, moving it to the top of the if does not help in terms of readability. On the other hand, evaluating twice is not such a big deal in this case, so if it helps for readability in your opinion, I can add it to the next iteration of this code (I still need to add unit test for the env thread)

narrieta · 2024-09-19T19:16:24Z

azurelinuxagent/common/event.py

@@ -331,10 +329,10 @@ def report_dropped_events_error(count, errors, operation_name):
        report_dropped_events_error(self.__unicode_error_count, self.__unicode_errors, self.__unicode_error_event)

    @staticmethod
-    def _update_errors_and_get_count(error_count, errors, error):
+    def _update_errors_and_get_count(error_count, errors, error_msg):


rename needed because now there is a global "error" function

narrieta · 2024-09-19T19:16:37Z

azurelinuxagent/common/event.py

@@ -750,9 +772,9 @@ def dump_unhandled_err(name):
        last_type = getattr(sys, 'last_type')
        last_value = getattr(sys, 'last_value')
        last_traceback = getattr(sys, 'last_traceback')
-        error = traceback.format_exception(last_type, last_value,
+        trace = traceback.format_exception(last_type, last_value,


rename needed because now there is a global "error" function

narrieta · 2024-09-19T19:30:36Z

azurelinuxagent/ga/update.py

-                                                                          self._heartbeat_id, dropped_packets,
-                                                                          self._heartbeat_update_goal_state_error_count,
-                                                                          auto_update_enabled, update_mode)
+            heartbeat_msg = "HeartbeatCounter: {0};HeartbeatId: {1};UpdateGSErrors: {2};AutoUpdate: {3};UpdateMode: {4};".format(


We don't use the DroppedPackets telemetry. It could be added to nftables too, but since we don't use it, I am deleting it instead.

narrieta · 2024-09-19T19:31:52Z

tests/common/osutil/test_default.py

@@ -643,327 +639,6 @@ def test_conf_sudoer(self, mock_dir):
        print("WRITING TO {0}".format(waagent_sudoers))
        self.assertEqual(1, count)

-    @staticmethod


moved to test_firewall_manager, or deleted, since some of those test don't make sense for the new code

narrieta · 2024-09-19T19:32:37Z

tests/common/utils/test_network_util.py

@@ -64,34 +60,3 @@ def test_nic_ordinary(self):
        self.assertEqual(str(nic), '{ "name": "test0", "link": "link INFO", "ipv4": ["ipv4-1"], "ipv6": ["ipv6-1"] }')


-class TestAddFirewallRules(AgentTestCase):


Moved to test_firewall_manager

narrieta · 2024-09-19T19:41:44Z

tests_e2e/tests/scripts/agent_persist_firewall-access_wireserver

-if command -v wget >/dev/null 2>&1; then
-    wget --tries=3 "http://$WIRE_IP/?comp=versions" --timeout=5 -O "/tmp/wire-versions-$USER.xml"
-else
+if command -v curl >/dev/null 2>&1; then


For some reason, on Azure Linux 3, wget returns success (0) on timeout. I am giving preference to curl instead.

narrieta · 2024-09-19T19:44:03Z

tests_e2e/tests/scripts/agent_persist_firewall-access_wireserver

+elif command -v wget >/dev/null 2>&1; then
+    wget --tries=3 "http://$WIRE_IP/?comp=versions" --timeout=5 -O "/tmp/wire-versions-$USER.xml"else
+else
+    http_get.py "http://168.63.129.16/?comp=versions" --timeout 5 --delay 5 --tries 3


I created this simple tool by doing surgery on some of the networking code in the agent. I used it to debug the above issue with wget. Since I already created it, I'm leaving it here as a fallback to curl/wget (initially it was meant as a replacement for them, but just changing the order of curl/wget fixed the original issue)

nagworld9 · 2024-09-22T22:50:15Z

azurelinuxagent/common/event.py

+
+def warn(op, fmt, *args):
+    """
+    Creates a telemetry event and logs the message as INFO; convenience wrapper over add_event(log_level=logger.LogLevel.WARNING...)


thanks; fixed

nagworld9 · 2024-09-22T22:50:30Z

azurelinuxagent/common/event.py

+
+def error(op, fmt, *args):
+    """
+    Creates a telemetry event and logs the message as INFO; convenience wrapper over add_event(log_level=logger.LogLevel.ERROR...)


thanks; fixed

nagworld9 · 2024-09-22T22:51:29Z

azurelinuxagent/common/event.py

+    """
+    Creates a telemetry event and logs the message as INFO; convenience wrapper over add_event(log_level=logger.LogLevel.ERROR...)
+    """
+    logger.warn(fmt, *args)


isn't error?

thanks; fixed

azurelinuxagent/ga/env.py

azurelinuxagent/common/protocol/metadata_server_migration_util.py

azurelinuxagent/ga/firewall_manager.py

azurelinuxagent/ga/persist_firewall_rules.py

nagworld9 · 2024-09-24T00:10:39Z

I didn't complete my review, will give another round trmw

maddieford · 2024-09-24T15:45:36Z

azurelinuxagent/agent.py

        else:
-            cmd = AgentCommands.Help
-            break
+            regex_cmd = regex_cmd_format.format("{0}=(?P<endpoint>[\\d.]{{7,}})".format(AgentCommands.SetupFirewall))


Nit, I think the SetupFirewall regex check should be in a separate elif condition for readability.
That way the only thing in the else condition is Help command, and it will make it easier to add more commands later

azurelinuxagent/common/event.py

maddieford · 2024-09-24T15:58:53Z

azurelinuxagent/common/protocol/metadata_server_migration_util.py

@@ -55,20 +59,17 @@ def _cleanup_metadata_protocol_certificates():
    _ensure_file_removed(lib_directory, _LEGACY_METADATA_SERVER_TRANSPORT_CERT_FILE_NAME)
    _ensure_file_removed(lib_directory, _LEGACY_METADATA_SERVER_P7B_FILE_NAME)

-def _reset_firewall_rules(osutil):
+
+def _reset_firewall_rules():
    """
    Removes MetadataServer firewall rule so IMDS can be used. Enables
    WireServer firewall rule based on if firewall is configured to be on.


This comment can be updated since we are no longer enabling wiereserver firewall rule

maddieford · 2024-09-24T16:13:45Z

azurelinuxagent/ga/env.py


+    def _report(self, report_function, message, *args):
+        # Report the first 6 messages, then stop reporting for 12 hours


Comment says 6 messages, but limit is 3 (line 133)

azurelinuxagent/ga/firewall_manager.py

azurelinuxagent/ga/env.py

azurelinuxagent/ga/firewall_manager.py

tests/ga/test_firewall_manager.py

nagworld9 · 2024-09-25T22:47:39Z

azurelinuxagent/ga/persist_firewall_rules.py


-        self._setup_network_setup_service()
+        firewall_manager.remove_legacy_rule()


do we need try/catch error, so that in all cases agent continue setup the persistent firewall rules with firewalld

yes, good point, will add it

azurelinuxagent/ga/firewall_manager.py

narrieta@microsoft added 30 commits July 22, 2024 16:55

.

2fa37cc

.

1296b06

.

5fcd7c0

.

82fd287

.

83868cf

.

486b0ef

.

c2f6251

Merge branch 'develop' into firewall

ce4a311

.

ddbbd27

.

012e5dc

.

4fdb797

.

efb0507

.

87cc479

.

90627e9

.

b81dd13

.

6433332

Merge branch 'develop' into firewall

173e779

.

4bf7a6b

.

5745ca9

.

4441828

Merge branch 'develop' into firewall

1f0900e

Merge branch 'develop' into firewall

e8b6b02

.

b879184

.

ac173d7

.

d6ae827

.

5ddad6b

.

79652e6

.

a48e6ba

.

2dd3ede

.

11d5a92

narrieta@microsoft added 7 commits September 16, 2024 16:12

.

aa4ee7b

.

8ad7ca9

.

9415899

.

4b3a865

.

fa5be12

.

405402b

.

1b8bc35

narrieta requested review from ZhidongPeng, nagworld9 and maddieford as code owners September 19, 2024 16:33

Merge branch 'develop' into firewall

9e71f39

narrieta commented Sep 19, 2024

View reviewed changes

narrieta@microsoft added 2 commits September 20, 2024 19:21

.

92b5226

Merge develop

8483c49

nagworld9 reviewed Sep 24, 2024

View reviewed changes

review feedback

5641dab

maddieford reviewed Sep 24, 2024

View reviewed changes

nagworld9 reviewed Sep 24, 2024

View reviewed changes

tests/ga/test_firewall_manager.py Show resolved Hide resolved

Merge branch 'develop' into firewall

99fd28f

nagworld9 reviewed Sep 26, 2024

View reviewed changes

narrieta@microsoft added 2 commits September 25, 2024 20:33

Code review feedback

f80d169

unit tests

bf331f0

maddieford reviewed Sep 26, 2024

View reviewed changes

azurelinuxagent/ga/firewall_manager.py Show resolved Hide resolved

nagworld9 approved these changes Sep 27, 2024

View reviewed changes

maddieford approved these changes Sep 27, 2024

View reviewed changes

narrieta merged commit 2f6ab89 into Azure:develop Oct 8, 2024
11 checks passed

narrieta deleted the firewall branch October 8, 2024 03:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for nftables + refactoring of firewall code #3221

Add support for nftables + refactoring of firewall code #3221

narrieta commented Sep 19, 2024

narrieta Sep 19, 2024

narrieta Sep 19, 2024

narrieta Sep 19, 2024

maddieford Sep 24, 2024

narrieta Sep 24, 2024

maddieford Sep 26, 2024

narrieta Sep 26, 2024

narrieta Sep 19, 2024

narrieta Sep 19, 2024

narrieta Sep 19, 2024

narrieta Sep 19, 2024

narrieta Sep 19, 2024

narrieta Sep 19, 2024

narrieta Sep 19, 2024

nagworld9 Sep 22, 2024

narrieta Sep 24, 2024

nagworld9 Sep 22, 2024

narrieta Sep 24, 2024

nagworld9 Sep 22, 2024

narrieta Sep 24, 2024

nagworld9 commented Sep 24, 2024

maddieford Sep 24, 2024

maddieford Sep 24, 2024

maddieford Sep 24, 2024

nagworld9 Sep 25, 2024

narrieta Sep 26, 2024

narrieta Sep 26, 2024

		@@ -64,34 +60,3 @@ def test_nic_ordinary(self):
		self.assertEqual(str(nic), '{ "name": "test0", "link": "link INFO", "ipv4": ["ipv4-1"], "ipv6": ["ipv6-1"] }')


		class TestAddFirewallRules(AgentTestCase):


		def _report(self, report_function, message, *args):
		# Report the first 6 messages, then stop reporting for 12 hours


		self._setup_network_setup_service()
		firewall_manager.remove_legacy_rule()

Add support for nftables + refactoring of firewall code #3221

Add support for nftables + refactoring of firewall code #3221

Conversation

narrieta commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagworld9 commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment