Merge lp:~gocept/landscape-client/py3-manager-scriptexecution into lp:~landscape/landscape-client/trunk

[Steffen Allner]

This is the Py3 port of landscape.manager.scriptexecution.

All the changes are to fix bytes/unicode. Notably, the stdlib pwd module uses decodes its data strictly by the default FS encoding (which cannot be changed from within Python). Since the tests are run with LC_ALL=C, some tests failed with unicode errors. The fix in the code protects us from the problem now and in the future.

This MP considers the landscape.manager.scriptexecution module, which tests were mostly failing because the communication with processes is not done with bytes.

Additionally, a bigger problem occurred, when the system default encoding (e.g. LC_ALL=C) is not able to handle unicode and a unicode containing username is queried by pwd.getpwnam(). The solution to handle this as an UnkownUserError, was chosen (1) because a simple switch of locales via locale.setlocale() did not solve the problem and (2) the edge case of falsely get an entry with unicode content could only happen if the entry was inserted with a less restrictive encoding in contrast to the retrieval encoding, which is not probable.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 977
Branch: lp:~gocept/landscape-client/py3-manager-scriptexecution
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3789/

review: Approve (test results)

[Eric Snow (ericsnowcurrently)]

Other than few couple minor comments, LGTM.

review: Approve

[Eric Snow (ericsnowcurrently)]

review: Needs Fixing

[lp:~gocept/landscape-client/py3-manager-scriptexecution]

978. By Steffen Allner on 2017-03-28

Improve documentation in comments.

[Steffen Allner (sallner)]

I updated the comment.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make ci-check
Result: Success
Revno: 978
Branch: lp:~gocept/landscape-client/py3-manager-scriptexecution
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3792/

review: Approve (test results)

[Eric Snow (ericsnowcurrently)]

Thanks for those changes.

review: Approve

[Chad Smith (chad.smith)]

+1 on the changeset. Did a little experimenting with script execution w/ and w/out unicode content using this build and didn't run into any unexpected problems.

review: Approve

=== modified file 'landscape/manager/scriptexecution.py'
--- landscape/manager/scriptexecution.py	2017-03-10 12:10:24 +0000
+++ landscape/manager/scriptexecution.py	2017-03-28 16:22:54 +0000
@@ -38,10 +38,23 @@
     gid = None
     path = None
     if username is not None:
-        username_str = encode_if_needed(username)
+        if _PY3:
+            username_str = username
+        else:
+            username_str = encode_if_needed(username)
         try:
+            # XXX: We have a situation with the system default FS encoding with
+            # Python 3 here: We have to pass a string to pwd.getpwnam(), but if
+            # the default does not support unicode characters, a
+            # UnicodeEncodeError will be thrown. This edge case can be harmful,
+            # if the user was added with a less restrictive encoding active,
+            # and is now retrieved with LC_ALL=C for example, as it is during
+            # automatic test runs. This should not be a problem under normal
+            # circumstances. Alternatively, a different way of parsing
+            # /etc/passwd would have to be implemented. A simple
+            # locale.setlocale() to use UTF-8 was not successful.
             info = pwd.getpwnam(username_str)
-        except KeyError:
+        except (KeyError, UnicodeEncodeError):
             raise UnknownUserError(u"Unknown user '%s'" % username)
         uid = info.pw_uid
         gid = info.pw_gid
@@ -117,8 +130,7 @@
             os.chown(filename, uid, gid)
 
         script = build_script(shell, code)
-        if not _PY3:
-            script = script.encode('utf-8')
+        script = script.encode("utf-8")
         script_file.write(script)
         script_file.close()
 
@@ -220,7 +232,7 @@
         for filename, attachment_id in attachments.items():
             if isinstance(attachment_id, str):
                 # Backward compatible behavior
-                data = attachment_id
+                data = attachment_id.encode("utf-8")
                 yield succeed(None)
             else:
                 data = yield fetch_async(
@@ -267,7 +279,7 @@
         uid, gid, path = get_user_info(user)
 
         fd, filename = tempfile.mkstemp()
-        script_file = os.fdopen(fd, "w")
+        script_file = os.fdopen(fd, "wb")
         self.write_script_file(
             script_file, filename, shell, code, uid, gid)
 
@@ -320,7 +332,7 @@
         self.result_deferred = Deferred()
         self._cancelled = False
         self.size_limit = size_limit
-        self._truncation_indicator = truncation_indicator
+        self._truncation_indicator = truncation_indicator.encode("utf-8")
         self._truncation_offset = len(self._truncation_indicator)
         self._truncated_size_limit = self.size_limit - self._truncation_offset
         self.reactor = reactor
@@ -355,7 +367,10 @@
         far.
         """
         exit_code = reason.value.exitCode
-        data = "".join(self.data)
+        # We get bytes with self.data, but want unicode with replace
+        # characters. This is again attempted in
+        # ScriptExecutionPlugin._respond, but it is not called in all cases.
+        data = b"".join(self.data).decode("utf-8", "replace")
         if self._cancelled:
             self.result_deferred.errback(ProcessTimeLimitReachedError(data))
         else:

=== modified file 'landscape/manager/tests/test_scriptexecution.py'
--- landscape/manager/tests/test_scriptexecution.py	2017-03-06 10:38:07 +0000
+++ landscape/manager/tests/test_scriptexecution.py	2017-03-28 16:22:54 +0000
@@ -43,7 +43,6 @@
         self.plugin = ScriptExecutionPlugin()
         self.manager.add(self.plugin)
 
-    @skipIf(_PY3, 'Takes long with Python3, probably not an unclean Reactor')
     def test_basic_run(self):
         """
         The plugin returns a Deferred resulting in the output of basic
@@ -53,21 +52,18 @@
         result.addCallback(self.assertEqual, "hi\n")
         return result
 
-    @skipIf(_PY3, 'Takes long with Python3, probably not an unclean Reactor')
     def test_snap_path(self):
         """The bin path for snaps is included in the PATH."""
         deferred = self.plugin.run_script("/bin/sh", "echo $PATH")
         return deferred.addCallback(
             lambda result: self.assertIn("/snap/bin", result))
 
-    @skipIf(_PY3, 'Takes long with Python3, probably not an unclean Reactor')
     def test_other_interpreter(self):
         """Non-shell interpreters can be specified."""
         result = self.plugin.run_script("/usr/bin/python", "print 'hi'")
         result.addCallback(self.assertEqual, "hi\n")
         return result
 
-    @skipIf(_PY3, 'Takes long with Python3, probably not an unclean Reactor')
     def test_other_interpreter_env(self):
         """
         Non-shell interpreters don't have their paths set by the shell, so we
@@ -75,7 +71,7 @@
         """
         result = self.plugin.run_script(
             sys.executable,
-            "import os\nprint os.environ")
+            "import os\nprint(os.environ)")
 
         def check_environment(results):
             for string in get_default_environment():
@@ -84,7 +80,6 @@
         result.addCallback(check_environment)
         return result
 
-    @skipIf(_PY3, 'Takes long with Python3, probably not an unclean Reactor')
     def test_server_supplied_env(self):
         """
         Server-supplied environment variables are merged with default
@@ -93,7 +88,7 @@
         server_supplied_env = {"DOG": "Woof", "CAT": "Meow"}
         result = self.plugin.run_script(
             sys.executable,
-            "import os\nprint os.environ",
+            "import os\nprint(os.environ)",
             server_supplied_env=server_supplied_env)
 
         def check_environment(results):
@@ -106,7 +101,6 @@
         result.addCallback(check_environment)
         return result
 
-    @skipIf(_PY3, 'Takes long with Python3, probably not an unclean Reactor')
     def test_server_supplied_env_overrides_client(self):
         """
         Server-supplied environment variables override client default
@@ -116,7 +110,7 @@
                                "HOME": "server-home"}
         result = self.plugin.run_script(
             sys.executable,
-            "import os\nprint os.environ",
+            "import os\nprint(os.environ)",
             server_supplied_env=server_supplied_env)
 
         def check_environment(results):
@@ -127,7 +121,6 @@
         result.addCallback(check_environment)
         return result
 
-    @skipIf(_PY3, 'Takes long with Python3, probably not an unclean Reactor')
     def test_concurrent(self):
         """
         Scripts run with the ScriptExecutionPlugin plugin are run concurrently.
@@ -144,7 +137,6 @@
         d2.addCallback(self.assertEqual, "")
         return gatherResults([d1, d2])
 
-    @skipIf(_PY3, 'Takes long with Python3, probably not an unclean Reactor')
     def test_accented_run_in_code(self):
         """
         Scripts can contain accented data both in the code and in the
@@ -153,11 +145,12 @@
         accented_content = u"\N{LATIN SMALL LETTER E WITH ACUTE}"
         result = self.plugin.run_script(
             u"/bin/sh", u"echo %s" % (accented_content,))
+        # self.assertEqual gets the result as first argument and that's what we
+        # compare against.
         result.addCallback(
-            self.assertEqual, "%s\n" % (accented_content.encode("utf-8"),))
+            self.assertEqual, "%s\n" % (accented_content,))
         return result
 
-    @skipIf(_PY3, 'Takes long with Python3, probably not an unclean Reactor')
     def test_accented_run_in_interpreter(self):
         """
         Scripts can also contain accents in the interpreter.
@@ -168,12 +161,11 @@
 
         def check(result):
             self.assertTrue(
-                "%s " % (accented_content.encode("utf-8"),) in result)
+                "%s " % (accented_content,) in result)
 
         result.addCallback(check)
         return result
 
-    @skipIf(_PY3, 'Takes long with Python3, probably not an unclean Reactor')
     def test_set_umask_appropriately(self):
         """
         We should be setting the umask to 0o022 before executing a script, and
@@ -196,7 +188,6 @@
         result.addCallback(check)
         return result.addCallback(lambda _: patch_umask.stop())
 
-    @skipIf(_PY3, "mock does not get cleaned up, poisoning all other tests.")
     def test_restore_umask_in_event_of_error(self):
         """
         We set the umask before executing the script, in the event that there's
@@ -312,7 +303,6 @@
 
         return result.addCallback(check).addBoth(cleanup)
 
-    @skipIf(_PY3, 'Takes long with Python3, probably not an unclean Reactor')
     def test_self_remove_script(self):
         """
         If a script removes itself, it doesn't create an error when the script
@@ -358,7 +348,7 @@
         self.assertEqual(spawn[6], expected_gid)
 
         protocol = spawn[0]
-        protocol.childDataReceived(1, "foobar")
+        protocol.childDataReceived(1, b"foobar")
         for fd in (0, 1, 2):
             protocol.childConnectionLost(fd)
         protocol.processEnded(Failure(ProcessDone(0)))
@@ -436,7 +426,7 @@
         self.assertEqual(stat.S_IMODE(os.stat(filename).st_mode), 0o600)
 
         protocol = spawn[0]
-        protocol.childDataReceived(1, "foobar")
+        protocol.childDataReceived(1, b"foobar")
         for fd in (0, 1, 2):
             protocol.childConnectionLost(fd)
         protocol.processEnded(Failure(ProcessDone(0)))
@@ -467,7 +457,7 @@
         protocol = factory.spawns[0][0]
 
         # Push 200 bytes of output, so we trigger truncation.
-        protocol.childDataReceived(1, "x" * 200)
+        protocol.childDataReceived(1, b"x" * 200)
 
         for fd in (0, 1, 2):
             protocol.childConnectionLost(fd)
@@ -489,9 +479,9 @@
         protocol = factory.spawns[0][0]
 
         # Push 200 bytes of output, so we trigger truncation.
-        protocol.childDataReceived(1, "x" * 200)
+        protocol.childDataReceived(1, b"x" * 200)
         # Push 200 bytes more
-        protocol.childDataReceived(1, "x" * 200)
+        protocol.childDataReceived(1, b"x" * 200)
 
         for fd in (0, 1, 2):
             protocol.childConnectionLost(fd)
@@ -518,7 +508,7 @@
         result = self.plugin.run_script("/bin/sh", "", time_limit=500)
         protocol = factory.spawns[0][0]
         protocol.makeConnection(DummyProcess())
-        protocol.childDataReceived(1, "hi\n")
+        protocol.childDataReceived(1, b"hi\n")
         self.manager.reactor.advance(501)
         protocol.processEnded(Failure(ProcessDone(0)))
 
@@ -539,7 +529,7 @@
         protocol = factory.spawns[0][0]
         transport = DummyProcess()
         protocol.makeConnection(transport)
-        protocol.childDataReceived(1, "hi\n")
+        protocol.childDataReceived(1, b"hi\n")
         protocol.processEnded(Failure(ProcessDone(0)))
         self.manager.reactor.advance(501)
         self.assertEqual(transport.signals, [])
@@ -556,7 +546,7 @@
         result = self.plugin.run_script("/bin/sh", "", time_limit=500)
         protocol = factory.spawns[0][0]
         protocol.makeConnection(DummyProcess())
-        protocol.childDataReceived(1, "hi")
+        protocol.childDataReceived(1, b"hi")
         protocol.processEnded(Failure(ProcessDone(0)))
         self.manager.reactor.advance(501)
 
@@ -614,10 +604,10 @@
                                         user=pwd.getpwuid(uid)[0])
 
         def check(_):
-            mock_fdopen.assert_called_with(99, "w")
+            mock_fdopen.assert_called_with(99, "wb")
             mock_chmod.assert_called_with("tempo!", 0o700)
             mock_chown.assert_called_with("tempo!", uid, gid)
-            script_file.write.assert_called_with("#!/bin/sh\ncode")
+            script_file.write.assert_called_with(b"#!/bin/sh\ncode")
             script_file.close.assert_called_with()
             self.assertEqual(
                 [mock_mkstemp, mock_fdopen, mock_chmod, mock_chown],
@@ -706,7 +696,7 @@
             self.broker_service.message_store.get_pending_messages(), [])
 
         # Now let's simulate the completion of the process
-        factory.spawns[0][0].childDataReceived(1, "hi!\n")
+        factory.spawns[0][0].childDataReceived(1, b"hi!\n")
         factory.spawns[0][0].processEnded(Failure(ProcessDone(0)))
 
         def got_result(r):
@@ -748,7 +738,7 @@
             self.broker_service.message_store.get_pending_messages(), [])
 
         # Now let's simulate the completion of the process
-        factory.spawns[0][0].childDataReceived(1, "Woof\n")
+        factory.spawns[0][0].childDataReceived(1, b"Woof\n")
         factory.spawns[0][0].processEnded(Failure(ProcessDone(0)))
 
         def got_result(r):
@@ -821,7 +811,7 @@
 
         protocol = factory.spawns[0][0]
         protocol.makeConnection(DummyProcess())
-        protocol.childDataReceived(2, "ONOEZ")
+        protocol.childDataReceived(2, b"ONOEZ")
         self.manager.reactor.advance(31)
         protocol.processEnded(Failure(ProcessDone(0)))
 
@@ -861,7 +851,7 @@
         """Responses to script execution messages are urgent."""
 
         def spawnProcess(protocol, filename, uid, gid, path, env):
-            protocol.childDataReceived(1, "hi!\n")
+            protocol.childDataReceived(1, b"hi!\n")
             protocol.processEnded(Failure(ProcessDone(0)))
             self._verify_script(filename, sys.executable, "print 'hi'")
 
@@ -893,7 +883,7 @@
         """
         def spawnProcess(protocol, filename, uid, gid, path, env):
             protocol.childDataReceived(
-                1, "\x7fELF\x01\x01\x01\x00\x00\x00\x95\x01")
+                1, b"\x7fELF\x01\x01\x01\x00\x00\x00\x95\x01")
             protocol.processEnded(Failure(ProcessDone(0)))
             self._verify_script(filename, sys.executable, "print 'hi'")
 

=== modified file 'py3_ready_tests'
--- py3_ready_tests	2017-03-27 06:58:52 +0000
+++ py3_ready_tests	2017-03-28 16:22:54 +0000
@@ -5,7 +5,7 @@
 
 
 landscape.tests.test_schema
-
+landscape.manager.tests.test_scriptexecution