Repository: canonical/operator
Pull request #1082 (2023-11-28 06:36:03)

fix: handle users/groups with no names in push_path
[Ned Batchelder noticed that the unit tests failed on his machine](https://hachyderm.io/@coveragepy/111477098431251147), which was because the temporary folder returned by `TemporaryDirectory()` belonged to a group with no name.

In theory, this could happen outside of the tests as well - someone could do a `push_path()` on a path that has files that have owners/groups that do not have names (e.g. because they have been removed). This is presumably quite uncommon (which makes sense: it seems unlikely that the charm container would often have a user or group removed) but it's also simple to fix, so it seems like we might as well do that.

tonyandrewmeyer: > Looks good to me now, though linting is failing.

Fixed now. Do you have a suggestion for a second reviewer?
benhoyt: I'm fine without a second reviewer on this small bug fix. I'll merge as soon as the tests are passing.

Patch:
diff --git a/ops/model.py b/ops/model.py
index e372dd6..0f0d281 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -2467,6 +2467,16 @@ class Container:
         import grp
         import pwd
         info = path.lstat()
+        try:
+            pw_name = pwd.getpwuid(info.st_uid).pw_name
+        except KeyError:
+            logger.warning("Could not get name for user %s", info.st_uid)
+            pw_name = None
+        try:
+            gr_name = grp.getgrgid(info.st_gid).gr_name
+        except KeyError:
+            logger.warning("Could not get name for group %s", info.st_gid)
+            gr_name = None
         return pebble.FileInfo(
             path=str(path),
             name=path.name,
@@ -2475,9 +2485,9 @@ class Container:
             permissions=stat.S_IMODE(info.st_mode),
             last_modified=datetime.datetime.fromtimestamp(info.st_mtime),
             user_id=info.st_uid,
-            user=pwd.getpwuid(info.st_uid).pw_name,
+            user=pw_name,
             group_id=info.st_gid,
-            group=grp.getgrgid(info.st_gid).gr_name)
+            group=gr_name)
 
     @staticmethod
     def _list_recursive(list_func: Callable[[Path], Iterable[pebble.FileInfo]],


Test patch:
diff --git a/test/test_model.py b/test/test_model.py
index 5ee1dbb..6d8aeca 100644
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -24,7 +24,7 @@ import unittest
 from collections import OrderedDict
 from test.test_helpers import fake_script, fake_script_calls
 from textwrap import dedent
-from unittest.mock import patch
+from unittest.mock import MagicMock, patch
 
 import pytest
 
@@ -969,6 +969,27 @@ class TestModel(unittest.TestCase):
         self.assertEqual(str(cm.exception), 'ERROR cannot get status\n')
         self.assertEqual(cm.exception.args[0], 'ERROR cannot get status\n')
 
+    @patch("grp.getgrgid")
+    @patch("pwd.getpwuid")
+    def test_push_path_unnamed(self, getpwuid: MagicMock, getgrgid: MagicMock):
+        getpwuid.side_effect = KeyError
+        getgrgid.side_effect = KeyError
+        harness = ops.testing.Harness(ops.CharmBase, meta='''
+            name: test-app
+            containers:
+              foo:
+                resource: foo-image
+            ''')
+        harness.begin()
+        harness.set_can_connect('foo', True)
+        container = harness.model.unit.containers['foo']
+
+        with tempfile.TemporaryDirectory() as push_src:
+            push_path = pathlib.Path(push_src) / 'src.txt'
+            push_path.write_text('hello')
+            container.push_path(push_path, "/")
+        assert container.exists("/src.txt"), 'push_path failed: file "src.txt" missing'
+
 
 class PushPullCase:
     """Test case for table-driven tests."""
diff --git a/test/test_testing.py b/test/test_testing.py
index 9dd2473..8ccc8e2 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -30,7 +30,7 @@ import textwrap
 import typing
 import unittest
 import uuid
-from unittest.mock import MagicMock
+from unittest.mock import MagicMock, patch
 
 import pytest
 import yaml
@@ -4096,6 +4096,7 @@ class PebbleStorageAPIsTestMixin:
 
     assertEqual = unittest.TestCase.assertEqual  # noqa
     assertIn = unittest.TestCase.assertIn  # noqa
+    assertIs = unittest.TestCase.assertIs  # noqa
     assertIsInstance = unittest.TestCase.assertIsInstance  # noqa
     assertRaises = unittest.TestCase.assertRaises  # noqa
 
@@ -4572,6 +4573,18 @@ class TestPebbleStorageAPIsUsingMocks(
             dir_ = client.list_files(f"{self.prefix}/dir{idx}", itself=True)[0]
             self.assertEqual(dir_.path, f"{self.prefix}/dir{idx}")
 
+    @patch("grp.getgrgid")
+    @patch("pwd.getpwuid")
+    def test_list_files_unnamed(self, getpwuid: MagicMock, getgrgid: MagicMock):
+        getpwuid.side_effect = KeyError
+        getgrgid.side_effect = KeyError
+        data = 'data'
+        self.client.push(f"{self.prefix}/file", data)
+        files = self.client.list_files(f"{self.prefix}/")
+        self.assertEqual(len(files), 1)
+        self.assertIs(files[0].user, None)
+        self.assertIs(files[0].group, None)
+
 
 class TestFilesystem(unittest.TestCase, _TestingPebbleClientMixin):
     def setUp(self) -> None: