Repository: Azure/WALinuxAgent
Pull request #3014 (2024-01-09 19:41:52)

Do not reset the mode of a extension's log directory
Currently, each time an extension is executed, the Agent resets the mode of its log directory. This is causing issues for customers that set custom modes on those directories.

With this fix, the Agent still sets the initial mode of the directory, but does not reset it when the extension is executed again.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/Azure/WALinuxAgent/pull/3014?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`70e2f54`)](https://app.codecov.io/gh/Azure/WALinuxAgent/commit/70e2f548aa70041075aefb841e268979948f429a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure) 71.89% compared to head [(`a806f9a`)](https://app.codecov.io/gh/Azure/WALinuxAgent/pull/3014?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure) 71.88%.

> :exclamation: Current head a806f9a differs from pull request most recent head 20e86da. Consider uploading reports for the commit 20e86da to get more accurate results


<details><summary>Additional details and impacted files</summary>


```diff
@@             Coverage Diff             @@
##           develop    #3014      +/-   ##
===========================================
- Coverage    71.89%   71.88%   -0.01%     
===========================================
  Files          109      109              
  Lines        16244    16246       +2     
  Branches      2324     2325       +1     
===========================================
  Hits         11678    11678              
- Misses        4023     4024       +1     
- Partials       543      544       +1     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/Azure/WALinuxAgent/pull/3014?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Azure).

nagworld9: @narrieta Why don't we set the mode to 700 to protect from non-root users when we create it first, that's what we have it for waagent directory. Anyway now we are allowing users to modify it later and not doing reset on that, that means we should be ok with 700 too. 
narrieta: @nagworld9 Sorry, I don't get your question. This does not change the mode we currently use for logs. This just does not reset it every time the extension runs.
nagworld9: @narrieta My question is not about this pr change. More of improvement perspective, when we create first time, we set to 755(existing code). I wonder why don't we create with 700 mode? what was the intention for 755? how this is different from waagent directory mode? I believe this ext directory only used by agent/extensions and they run with root. So, root users will have full access and won't be an issue. 
narrieta: @nagworld9 

`My question is not about this pr change
`

That should probably be followed up outside this PR, then.

Patch:
diff --git a/azurelinuxagent/common/utils/fileutil.py b/azurelinuxagent/common/utils/fileutil.py
index 03090a42..94eb5cf1 100644
--- a/azurelinuxagent/common/utils/fileutil.py
+++ b/azurelinuxagent/common/utils/fileutil.py
@@ -99,13 +99,15 @@ def get_line_startingwith(prefix, filepath):
     return None
 
 
-def mkdir(dirpath, mode=None, owner=None):
+def mkdir(dirpath, mode=None, owner=None, reset_mode_and_owner=True):
     if not os.path.isdir(dirpath):
         os.makedirs(dirpath)
-    if mode is not None:
-        chmod(dirpath, mode)
-    if owner is not None:
-        chowner(dirpath, owner)
+        reset_mode_and_owner = True  # force setting the mode and owner
+    if reset_mode_and_owner:
+        if mode is not None:
+            chmod(dirpath, mode)
+        if owner is not None:
+            chowner(dirpath, owner)
 
 
 def chowner(path, owner):
diff --git a/azurelinuxagent/ga/exthandlers.py b/azurelinuxagent/ga/exthandlers.py
index a09a81e9..fcb14d22 100644
--- a/azurelinuxagent/ga/exthandlers.py
+++ b/azurelinuxagent/ga/exthandlers.py
@@ -1051,7 +1051,7 @@ class ExtHandlerInstance(object):
 
     def __set_command_execution_log(self, extension, execution_log_max_size):
         try:
-            fileutil.mkdir(self.get_log_dir(), mode=0o755)
+            fileutil.mkdir(self.get_log_dir(), mode=0o755, reset_mode_and_owner=False)
         except IOError as e:
             self.logger.error(u"Failed to create extension log dir: {0}", e)
         else:


Test patch:
diff --git a/tests/ga/test_exthandlers.py b/tests/ga/test_exthandlers.py
index 2f033965..f56ebce1 100644
--- a/tests/ga/test_exthandlers.py
+++ b/tests/ga/test_exthandlers.py
@@ -287,6 +287,28 @@ class TestExtHandlers(AgentTestCase):
         with open(log_file_path) as truncated_log_file:
             self.assertEqual(truncated_log_file.read(), "{second_line}\n".format(second_line=second_line))
 
+    def test_set_logger_should_not_reset_the_mode_of_the_log_directory(self):
+        ext_log_dir = os.path.join(self.tmp_dir, "log_directory")
+
+        with patch("azurelinuxagent.common.conf.get_ext_log_dir", return_value=ext_log_dir):
+            ext_handler = Extension(name='foo')
+            ext_handler.version = "1.2.3"
+            ext_handler_instance = ExtHandlerInstance(ext_handler=ext_handler, protocol=None)
+            ext_handler_log_dir = os.path.join(ext_log_dir, ext_handler.name)
+
+            # Double-check the initial mode
+            get_mode = lambda f: os.stat(f).st_mode & 0o777
+            mode = get_mode(ext_handler_log_dir)
+            if mode != 0o755:
+                raise Exception("The initial mode of the log directory should be 0o755, got 0{0:o}".format(mode))
+
+            new_mode = 0o700
+            os.chmod(ext_handler_log_dir, new_mode)
+            ext_handler_instance.set_logger()
+
+            mode = get_mode(ext_handler_log_dir)
+            self.assertEqual(new_mode, mode, "The mode of the log directory should not have changed")
+
     def test_it_should_report_the_message_in_the_hearbeat(self):
         def heartbeat_with_message():
             return {'code': 0, 'formattedMessage': {'lang': 'en-US', 'message': 'This is a heartbeat message'},