Merge lp:~free.ekanayaka/landscape-client/py3-registration into lp:~landscape/landscape-client/trunk

[Free Ekanayaka]

Adjust wire-to-language type conversions in order to have Landscape registration work.

Adjust wire-to-language type conversions in order to have Landscape registration work.

Testing:

# spawn a local advice dog
sudo ./scripts/landscape-client -c root-client.conf

Accept the pending computer and see that it reports a few basic things (e.g. the Ubuntu version).

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-check
Result: Fail
Revno: 1001
Branch: lp:~free.ekanayaka/landscape-client/py3-registration
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3840/

review: Needs Fixing (test results)

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make ci-check
Result: Fail
Revno: 1001
Branch: lp:~free.ekanayaka/landscape-client/py3-registration
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3844/

review: Needs Fixing (test results)

[Eric Snow (ericsnowcurrently)]

Other than a few minor points, LGTM. The code is correct, but it would be nice if there were a bit less ambiguity about the wire format (e.g. dict keys, exchange payload). However, I realize that we are coping with Python 3 clients dealing with Python 2 servers, so I'm not going to stress over it too much.

review: Approve

[Eric Snow (ericsnowcurrently)]

FYI, I was able to follow the testing instructions successfully. I was also able to run a script.

[lp:~free.ekanayaka/landscape-client/py3-registration]

1002. By Free Ekanayaka on 2017-04-06

Fix failing test

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make ci-check
Result: Success
Revno: 1002
Branch: lp:~free.ekanayaka/landscape-client/py3-registration
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3845/

review: Approve (test results)

[lp:~free.ekanayaka/landscape-client/py3-registration]

1003. By Free Ekanayaka on 2017-04-06

Address review comments

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make ci-check
Result: Success
Revno: 1003
Branch: lp:~free.ekanayaka/landscape-client/py3-registration
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3846/

review: Approve (test results)

[Simon Poirier (simpoir)]

+1 looks good and tested successfully. See minor things to address inline

review: Approve

[lp:~free.ekanayaka/landscape-client/py3-registration]

1004. By Free Ekanayaka on 2017-04-06

Address review comments

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make ci-check
Result: Success
Revno: 1004
Branch: lp:~free.ekanayaka/landscape-client/py3-registration
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3848/

review: Approve (test results)

=== modified file 'landscape/broker/exchange.py'
--- landscape/broker/exchange.py	2017-03-23 10:13:55 +0000
+++ landscape/broker/exchange.py	2017-04-06 14:56:21 +0000
@@ -347,6 +347,7 @@
 from landscape.lib.hashlib import md5
 
 from twisted.internet.defer import Deferred, succeed
+from twisted.python.compat import _PY3
 
 from landscape.lib.fetch import HTTPCodeError, PyCurlError
 from landscape.lib.message import got_next_expected, ANCIENT
@@ -773,6 +774,12 @@
         # be 3.2, because it's the one that didn't have this field.
         server_api = result.get("server-api", b"3.2")
 
+        if _PY3 and not isinstance(server_api, bytes):
+            # The "server-api" field in the bpickle payload sent by the server
+            # is a string, however in Python 3 we need to convert it to bytes,
+            # since that's what the rest of the code expects.
+            server_api = server_api.encode()
+
         if is_version_higher(server_api, message_store.get_server_api()):
             # The server can handle a message API that is higher than the one
             # we're currently using. If the highest server API is greater than
@@ -787,6 +794,12 @@
 
         sequence = message_store.get_server_sequence()
         for message in result.get("messages", ()):
+            # The wire format of the 'type' field is bytes, but our handlers
+            # actually expect it to be a string. Some unit tests set it to
+            # a regular string (since there is no difference between strings
+            # and bytes in Python 2), so we check the type before converting.
+            if _PY3 and isinstance(message["type"], bytes):
+                message["type"] = message["type"].decode("ascii")
             self.handle_message(message)
             sequence += 1
             message_store.set_server_sequence(sequence)

=== modified file 'landscape/broker/transport.py'
--- landscape/broker/transport.py	2017-03-17 09:26:45 +0000
+++ landscape/broker/transport.py	2017-04-06 14:56:21 +0000
@@ -6,7 +6,7 @@
 
 import pycurl
 
-from twisted.python.compat import unicode
+from twisted.python.compat import unicode, _PY3
 
 from landscape.lib import bpickle
 from landscape.lib.fetch import fetch
@@ -35,12 +35,21 @@
         self._url = url
 
     def _curl(self, payload, computer_id, exchange_token, message_api):
+        # There are a few "if _PY3" checks below, because for Python 3 we
+        # want to convert a number of values from bytes to string, before
+        # assigning them to the headers.
+        if _PY3 and isinstance(message_api, bytes):
+            message_api = message_api.decode("ascii")
         headers = {"X-Message-API": message_api,
                    "User-Agent": "landscape-client/%s" % VERSION,
                    "Content-Type": "application/octet-stream"}
         if computer_id:
+            if _PY3 and isinstance(computer_id, bytes):
+                computer_id = computer_id.decode("ascii")
             headers["X-Computer-ID"] = computer_id
         if exchange_token:
+            if _PY3 and isinstance(exchange_token, bytes):
+                exchange_token = exchange_token.decode("ascii")
             headers["X-Exchange-Token"] = str(exchange_token)
         curl = pycurl.Curl()
         return (curl, fetch(self._url, post=True, data=payload,

=== modified file 'landscape/lib/bpickle.py'
--- landscape/lib/bpickle.py	2017-04-05 14:19:27 +0000
+++ landscape/lib/bpickle.py	2017-04-06 14:56:21 +0000
@@ -32,7 +32,7 @@
 wire compatible and behave the same way (bugs notwithstanding).
 """
 
-import logging
+from twisted.python.compat import _PY3
 
 dumps_table = {}
 loads_table = {}
@@ -94,11 +94,6 @@
     append = res.append
     for key in keys:
         val = obj[key]
-        if (bytes is not str) and isinstance(key, str):
-            try:
-                key.encode("ascii")
-            except UnicodeError as e:
-                logging.warning(e)
         append(_dt[type(key)](key))
         append(_dt[type(val)](val))
     return b"d%s;" % b"".join(res)
@@ -160,11 +155,11 @@
     while bytestring[pos:pos+1] != b";":
         key, pos = _lt[bytestring[pos:pos+1]](bytestring, pos)
         val, pos = _lt[bytestring[pos:pos+1]](bytestring, pos)
-        if (bytes is not str) and isinstance(key, bytes):
-            try:
-                key.decode("ascii")
-            except UnicodeError as e:
-                logging.warning(e)
+        if _PY3 and isinstance(key, bytes):
+            # Although the wire format of dictionary keys is ASCII bytes, the
+            # code actually expects them to be strings, so we convert them
+            # here.
+            key = key.decode("ascii")
         res[key] = val
     return res, pos+1
 

=== modified file 'landscape/lib/tests/test_bpickle.py'
--- landscape/lib/tests/test_bpickle.py	2017-03-17 09:26:45 +0000
+++ landscape/lib/tests/test_bpickle.py	2017-04-06 14:56:21 +0000
@@ -45,6 +45,10 @@
         self.assertEqual(bpickle.loads(dumped_tobool),
                          {True: False})
 
+    def test_dict_bytes_keys(self):
+        data = bpickle.dumps({b"hello": True})
+        self.assertEqual(bpickle.loads(data), {"hello": True})
+
     def test_long(self):
         long = 99999999999999999999999999999
         self.assertEqual(bpickle.loads(bpickle.dumps(long)), long)

=== modified file 'landscape/monitor/tests/test_networkactivity.py'
--- landscape/monitor/tests/test_networkactivity.py	2017-03-30 07:43:32 +0000
+++ landscape/monitor/tests/test_networkactivity.py	2017-04-06 14:56:21 +0000
@@ -184,8 +184,8 @@
         self.assertMessages(self.mstore.get_pending_messages(),
                         [{"type": "network-activity",
                           "activities": {
-                              b"lo": [(step_size, 0, 1000)],
-                              b"eth0": [(step_size, 0, 1000)]}}])
+                              "lo": [(step_size, 0, 1000)],
+                              "eth0": [(step_size, 0, 1000)]}}])
 
     def test_config(self):
         """The network activity plugin is enabled by default."""

=== modified file 'root-client.conf'
--- root-client.conf	2013-03-21 12:46:40 +0000
+++ root-client.conf	2017-04-06 14:56:21 +0000
@@ -9,7 +9,6 @@
 log_dir = /tmp/landscape-root
 log_level = debug
 pid_file = /tmp/landscape-root/landscape-client.pid
-ping_url = http://localhost:8081/ping
+ping_url = http://localhost:8080/ping
 include_manager_plugins = ScriptExecution
 script_users = www-data, nobody, root
-