Repository: Azure/WALinuxAgent
Pull request #2236 (2021-05-03 18:15:40)

Replace firewall-setup unit file if changed
<!-- DO NOT DELETE THIS TEMPLATE -->

## Description
- Replace firewall-setup unit file if changed
- Remove custom service if firewalld service enabled and custom service also enabled

Issue # <!-- if any -->
<!--
Please add an informative description that covers that changes made by the pull request. 
This checklist is used to make sure that common issues in a pull request are addressed.
This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
-->

---

### PR information
- [ ] The title of the PR is clear and informative.
- [ ] There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, [see this page](https://github.com/Azure/azure-powershell/blob/master/documentation/development-docs/cleaning-up-commits.md).
- [ ] If applicable, the PR references the bug/issue that it fixes in the description.
- [ ] New Unit tests were added for the changes made

### Quality of Code and Contribution Guidelines
- [ ] I have read the [contribution guidelines](https://github.com/Azure/WALinuxAgent/blob/master/.github/CONTRIBUTING.md).

codecov[bot]: # [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/2236?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure) Report
> Merging [#2236](https://codecov.io/gh/Azure/WALinuxAgent/pull/2236?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure) (8b529e1) into [release-2.3.0.0](https://codecov.io/gh/Azure/WALinuxAgent/commit/2ab6ce47090da6da5a4f6bb93f6d61274805f93d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure) (2ab6ce4) will **increase** coverage by `0.02%`.
> The diff coverage is `72.41%`.

> :exclamation: Current head 8b529e1 differs from pull request most recent head 2a4e792. Consider uploading reports for the commit 2a4e792 to get more accurate results
[![Impacted file tree graph](https://codecov.io/gh/Azure/WALinuxAgent/pull/2236/graphs/tree.svg?width=650&height=150&src=pr&token=ae90E2KYCw&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure)](https://codecov.io/gh/Azure/WALinuxAgent/pull/2236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure)

```diff
@@                 Coverage Diff                 @@
##           release-2.3.0.0    #2236      +/-   ##
===================================================
+ Coverage            70.59%   70.62%   +0.02%     
===================================================
  Files                   96       96              
  Lines                13615    13641      +26     
  Branches              1928     1932       +4     
===================================================
+ Hits                  9612     9634      +22     
- Misses                3585     3589       +4     
  Partials               418      418              
```


| [Impacted Files](https://codecov.io/gh/Azure/WALinuxAgent/pull/2236?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure) | Coverage Δ | |
|---|---|---|
| [azurelinuxagent/common/persist\_firewall\_rules.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/2236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure#diff-YXp1cmVsaW51eGFnZW50L2NvbW1vbi9wZXJzaXN0X2ZpcmV3YWxsX3J1bGVzLnB5) | `75.73% <72.41%> (-0.49%)` | :arrow_down: |
| [azurelinuxagent/ga/collect\_telemetry\_events.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/2236/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure#diff-YXp1cmVsaW51eGFnZW50L2dhL2NvbGxlY3RfdGVsZW1ldHJ5X2V2ZW50cy5weQ==) | `91.43% <0.00%> (+1.02%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/2236?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/2236?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure). Last update [2ab6ce4...2a4e792](https://codecov.io/gh/Azure/WALinuxAgent/pull/2236?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure).


Patch:
diff --git a/azurelinuxagent/common/persist_firewall_rules.py b/azurelinuxagent/common/persist_firewall_rules.py
index 31f0899c..5c6ce561 100644
--- a/azurelinuxagent/common/persist_firewall_rules.py
+++ b/azurelinuxagent/common/persist_firewall_rules.py
@@ -32,7 +32,7 @@ from azurelinuxagent.common.utils.shellutil import CommandError
 class PersistFirewallRulesHandler(object):
 
     __SERVICE_FILE_CONTENT = """
-# This unit file was created by the Azure VM Agent.
+# This unit file (Version={version}) was created by the Azure VM Agent.
 # Do not edit.
 [Unit]
 Description=Setup network rules for WALinuxAgent 
@@ -67,6 +67,10 @@ if __name__ == '__main__':
 
     _FIREWALLD_RUNNING_CMD = ["firewall-cmd", "--state"]
 
+    # The current version of the unit file; Update it whenever the unit file is modified to ensure Agent can dynamically
+    # modify the unit file on VM too
+    _UNIT_VERSION = "1.1"
+
     @staticmethod
     def get_service_file_path():
         osutil = get_osutil()
@@ -113,6 +117,13 @@ if __name__ == '__main__':
             # In case of a failure, this would throw. In such a case, we don't need to try to setup our custom service
             # because on system reboot, all iptable rules are reset by firewalld.service so it would be a no-op.
             self._setup_permanent_firewalld_rules()
+
+            # Remove custom service if exists to avoid problems with firewalld
+            try:
+                fileutil.rm_files(*[self.get_service_file_path(), os.path.join(conf.get_lib_dir(), self.BINARY_FILE_NAME)])
+            except Exception as error:
+                logger.info(
+                    "Unable to delete existing service {0}: {1}".format(self._network_setup_service_name, ustr(error)))
             return
 
         logger.info(
@@ -163,12 +174,19 @@ if __name__ == '__main__':
         # the service is always run from the most latest agent.
         self.__setup_binary_file()
 
-        if self.__verify_network_setup_service_enabled():
+        network_service_enabled = self.__verify_network_setup_service_enabled()
+        if network_service_enabled and not self.__unit_file_version_modified():
             logger.info("Service: {0} already enabled. No change needed.".format(self._network_setup_service_name))
             self.__log_network_setup_service_logs()
 
         else:
-            logger.info("Service: {0} not enabled. Adding it now".format(self._network_setup_service_name))
+            if not network_service_enabled:
+                logger.info("Service: {0} not enabled. Adding it now".format(self._network_setup_service_name))
+            else:
+                logger.info(
+                    "Unit file {0} version modified to {1}, setting it up again".format(self.get_service_file_path(),
+                                                                                        self._UNIT_VERSION))
+
             # Create unit file with default values
             self.__set_service_unit_file()
             # Reload systemd configurations when we setup the service for the first time to avoid systemctl warnings
@@ -199,7 +217,8 @@ if __name__ == '__main__':
         try:
             fileutil.write_file(service_unit_file,
                                 self.__SERVICE_FILE_CONTENT.format(binary_path=binary_path,
-                                                                   py_path=sys.executable))
+                                                                   py_path=sys.executable,
+                                                                   version=self._UNIT_VERSION))
             fileutil.chmod(service_unit_file, 0o644)
 
             # Finally enable the service. This is needed to ensure the service is started on system boot
@@ -207,7 +226,8 @@ if __name__ == '__main__':
             try:
                 shellutil.run_command(cmd)
             except CommandError as error:
-                msg = "Unable to enable service: {0}; deleting service file: {1}. Command: {2}, Exit-code: {3}.\nstdout: {4}\nstderr: {5}".format(
+                msg = ustr(
+                    "Unable to enable service: {0}; deleting service file: {1}. Command: {2}, Exit-code: {3}.\nstdout: {4}\nstderr: {5}").format(
                     self._network_setup_service_name, service_unit_file, ' '.join(cmd), error.returncode, error.stdout,
                     error.stderr)
                 raise Exception(msg)
@@ -273,3 +293,38 @@ if __name__ == '__main__':
             shellutil.run_command(["systemctl", "daemon-reload"])
         except Exception as exception:
             logger.warn("Unable to reload systemctl configurations: {0}".format(ustr(exception)))
+
+    def __get_unit_file_version(self):
+        if not os.path.exists(self.get_service_file_path()):
+            raise OSError("{0} not found".format(self.get_service_file_path()))
+
+        match = fileutil.findre_in_file(self.get_service_file_path(),
+                                        line_re="This unit file \\(Version=([\\d.]+)\\) was created by the Azure VM Agent.")
+        if match is None:
+            raise ValueError("Version tag not found in the unit file")
+
+        return match.group(1).strip()
+
+    def __unit_file_version_modified(self):
+        """
+        Check if the unit file version changed from the expected version
+        :return: True if unit file version changed else False
+        """
+
+        try:
+            unit_file_version = self.__get_unit_file_version()
+        except Exception as error:
+            logger.info("Unable to determine version of unit file: {0}, overwriting unit file".format(ustr(error)))
+            # Since we can't determine the version, marking the file as modified to overwrite the unit file
+            return True
+
+        if unit_file_version != self._UNIT_VERSION:
+            logger.info(
+                "Unit file version: {0} does not match with expected version: {1}, overwriting unit file".format(
+                    unit_file_version, self._UNIT_VERSION))
+            return True
+
+        logger.info(
+            "Unit file version matches with expected version: {0}, not overwriting unit file".format(unit_file_version))
+        return False
+


Test patch:
diff --git a/tests/common/test_persist_firewall_rules.py b/tests/common/test_persist_firewall_rules.py
index a5d832a7..29d845d2 100644
--- a/tests/common/test_persist_firewall_rules.py
+++ b/tests/common/test_persist_firewall_rules.py
@@ -161,6 +161,31 @@ class TestPersistFirewallRulesHandler(AgentTestCase):
             return True, ["echo", "not enabled"]
         return False, []
 
+    @staticmethod
+    def __mock_firewalld_running_and_not_applied(cmd):
+        if cmd == PersistFirewallRulesHandler._FIREWALLD_RUNNING_CMD:
+            return True, ["echo", "running"]
+        # This is to fail the check if firewalld-rules are already applied
+        cmds_to_fail = ["firewall-cmd", FirewallCmdDirectCommands.QueryPassThrough, "conntrack"]
+        if all(cmd_to_fail in cmd for cmd_to_fail in cmds_to_fail):
+            return True, ["exit", "1"]
+        if "firewall-cmd" in cmd:
+            return True, ["echo", "enabled"]
+        return False, []
+
+    def __setup_and_assert_network_service_setup_scenario(self, handler, mock_popen=None):
+        mock_popen = TestPersistFirewallRulesHandler.__mock_network_setup_service_disabled if mock_popen is None else mock_popen
+        self.__replace_popen_cmd = mock_popen
+        handler.setup()
+
+        self.__assert_systemctl_called(cmd="is-enabled", validate_command_called=True)
+        self.__assert_systemctl_called(cmd="enable", validate_command_called=True)
+        self.__assert_systemctl_reloaded(validate_command_called=True)
+        self.__assert_firewall_cmd_running_called(validate_command_called=True)
+        self.__assert_firewall_called(cmd=FirewallCmdDirectCommands.QueryPassThrough, validate_command_called=False)
+        self.__assert_firewall_called(cmd=FirewallCmdDirectCommands.PassThrough, validate_command_called=False)
+        self.assertTrue(os.path.exists(handler.get_service_file_path()), "Service unit file not found")
+
     def test_it_should_skip_setup_if_firewalld_already_enabled(self):
         self.__replace_popen_cmd = lambda cmd: ("firewall-cmd" in cmd, ["echo", "running"])
         with self._get_persist_firewall_rules_handler() as handler:
@@ -173,19 +198,27 @@ class TestPersistFirewallRulesHandler(AgentTestCase):
         # Assert no commands for systemctl were called
         self.assertFalse(any("systemctl" in cmd for cmd in self.__executed_commands), "Systemctl shouldn't be called")
 
-    def test_it_should_skip_setup_if_agent_network_setup_service_already_enabled(self):
-        self.__replace_popen_cmd = TestPersistFirewallRulesHandler.__mock_network_setup_service_enabled
+    def test_it_should_skip_setup_if_agent_network_setup_service_already_enabled_and_version_same(self):
+
         with self._get_persist_firewall_rules_handler() as handler:
+            # 1st time should setup the service
+            self.__setup_and_assert_network_service_setup_scenario(handler)
+
+            # 2nd time setup should do nothing as service is enabled and no version updated
+            self.__replace_popen_cmd = TestPersistFirewallRulesHandler.__mock_network_setup_service_enabled
+            # Reset state
+            self.__executed_commands = []
             handler.setup()
 
-        self.__assert_systemctl_called(cmd="is-enabled", validate_command_called=True)
-        self.__assert_systemctl_called(cmd="enabled", validate_command_called=False)
-        self.__assert_systemctl_reloaded(validate_command_called=False)
-        self.__assert_firewall_cmd_running_called(validate_command_called=True)
-        self.__assert_firewall_called(cmd=FirewallCmdDirectCommands.QueryPassThrough, validate_command_called=False)
-        self.__assert_firewall_called(cmd=FirewallCmdDirectCommands.PassThrough, validate_command_called=False)
+            self.__assert_systemctl_called(cmd="is-enabled", validate_command_called=True)
+            self.__assert_systemctl_called(cmd="enable", validate_command_called=False)
+            self.__assert_systemctl_reloaded(validate_command_called=False)
+            self.__assert_firewall_cmd_running_called(validate_command_called=True)
+            self.__assert_firewall_called(cmd=FirewallCmdDirectCommands.QueryPassThrough, validate_command_called=False)
+            self.__assert_firewall_called(cmd=FirewallCmdDirectCommands.PassThrough, validate_command_called=False)
+            self.assertTrue(os.path.exists(handler.get_service_file_path()), "Service unit file not found")
 
-    def test_it_should_always_replace_only_drop_in_file_if_using_custom_network_service(self):
+    def test_it_should_always_replace_binary_file_only_if_using_custom_network_service(self):
 
         def _find_in_file(file_name, line_str):
             try:
@@ -207,7 +240,7 @@ class TestPersistFirewallRulesHandler(AgentTestCase):
             handler.setup()
 
         orig_service_file_contents = "ExecStart={py_path} {binary_path}".format(py_path=sys.executable,
-                                                                                 binary_path=self._binary_file)
+                                                                                binary_path=self._binary_file)
         self.__assert_systemctl_called(cmd="is-enabled", validate_command_called=True)
         self.__assert_firewall_called(cmd=FirewallCmdDirectCommands.PassThrough, validate_command_called=False)
         self.assertTrue(os.path.exists(self._binary_file), "Binary file should be there")
@@ -226,7 +259,7 @@ class TestPersistFirewallRulesHandler(AgentTestCase):
         # The service should say its enabled now
         self.__replace_popen_cmd = TestPersistFirewallRulesHandler.__mock_network_setup_service_enabled
         with self._get_persist_firewall_rules_handler() as handler:
-            # The Drop-in file should be available on the 2nd run
+            # The Binary file should be available on the 2nd run
             self.assertTrue(os.path.exists(self._binary_file), "Binary file should be there")
             handler.setup()
 
@@ -241,18 +274,7 @@ class TestPersistFirewallRulesHandler(AgentTestCase):
 
     def test_it_should_use_firewalld_if_available(self):
 
-        def __mock_firewalld_running_and_not_applied(cmd):
-            if cmd == PersistFirewallRulesHandler._FIREWALLD_RUNNING_CMD:
-                return True, ["echo", "running"]
-            # This is to fail the check if firewalld-rules are already applied
-            cmds_to_fail = ["firewall-cmd", FirewallCmdDirectCommands.QueryPassThrough, "conntrack"]
-            if all(cmd_to_fail in cmd for cmd_to_fail in cmds_to_fail):
-                return True, ["exit", "1"]
-            if "firewall-cmd" in cmd:
-                return True, ["echo", "enabled"]
-            return False, []
-
-        self.__replace_popen_cmd = __mock_firewalld_running_and_not_applied
+        self.__replace_popen_cmd = self.__mock_firewalld_running_and_not_applied
         with self._get_persist_firewall_rules_handler() as handler:
             handler.setup()
 
@@ -321,3 +343,37 @@ class TestPersistFirewallRulesHandler(AgentTestCase):
                 expected_str = "{0} file not found, skipping execution of firewall execution setup for this boot".format(
                     os.path.join(os.getcwd(), test_str))
                 self.assertIn(expected_str, output, "Unexpected output")
+
+    def test_it_should_delete_custom_service_files_if_firewalld_enabled(self):
+        with self._get_persist_firewall_rules_handler() as handler:
+            # 1st run - Setup the Custom Service
+            self.__setup_and_assert_network_service_setup_scenario(handler)
+
+            # 2nd run - Enable Firewalld and ensure the agent sets firewall rules using firewalld and deletes custom service
+            self.__executed_commands = []
+            self.__replace_popen_cmd = self.__mock_firewalld_running_and_not_applied
+            handler.setup()
+
+            self.__assert_firewall_cmd_running_called(validate_command_called=True)
+            self.__assert_firewall_called(cmd=FirewallCmdDirectCommands.QueryPassThrough, validate_command_called=True)
+            self.__assert_firewall_called(cmd=FirewallCmdDirectCommands.PassThrough, validate_command_called=True)
+            self.__assert_systemctl_called(cmd="is-enabled", validate_command_called=False)
+            self.__assert_systemctl_called(cmd="enable", validate_command_called=False)
+            self.__assert_systemctl_reloaded(validate_command_called=False)
+            self.assertFalse(os.path.exists(handler.get_service_file_path()), "Service unit file found")
+            self.assertFalse(os.path.exists(os.path.join(conf.get_lib_dir(), handler.BINARY_FILE_NAME)), "Binary file found")
+
+    def test_it_should_reset_service_unit_files_if_version_changed(self):
+        with self._get_persist_firewall_rules_handler() as handler:
+            # 1st step - Setup the service with old Version
+            test_ver = str(uuid.uuid4())
+            with patch.object(handler, "_UNIT_VERSION", test_ver):
+                self.__setup_and_assert_network_service_setup_scenario(handler)
+                self.assertIn(test_ver, fileutil.read_file(handler.get_service_file_path()), "Test version not found")
+
+            # 2nd step - Re-run the setup and ensure the service file set up again even if service enabled
+            self.__executed_commands = []
+            self.__setup_and_assert_network_service_setup_scenario(handler,
+                                                                   mock_popen=self.__mock_network_setup_service_enabled)
+            self.assertNotIn(test_ver, fileutil.read_file(handler.get_service_file_path()),
+                             "Test version found incorrectly")