Software Properties

Merge ~slingamn/software-properties:norequests into software-properties:ubuntu/master

  1. Git
  2. lp:~slingamn/software-properties
  3. norequests
  4. Merge into ubuntu/master
Proposed by Shivaram Lingamneni
Status: Rejected
Rejected by: Julian Andres Klode
Proposed branch: ~slingamn/software-properties:norequests
Merge into: software-properties:ubuntu/master
Diff against target: 149 lines (+55/-11)
3 files modified
debian/control (+0/-3)
softwareproperties/LivepatchService.py (+12/-8)
softwareproperties/http_unixdomain.py (+43/-0)
Reviewer Review Type Date Requested Status
Julian Andres Klode Disapprove
Dan Streetman Approve
Andrea Azzarone Pending
Review via email: mp+389182@code.launchpad.net

Commit message

Remove dependency on python3-requests.

Description of the change

At this stage, this is more of a request for feedback. I'm trying to remove python3-requests from the Ubuntu base system. software-properties is one of the three packages that currently depends on it (for making HTTP requests to unix domain sockets). (The other two are ssh-import-id, which I have an open merge request for, and apport, which I'm planning to work on next.)

I believe this code is correct, but I wasn't sure about the correct way to test it against the actual livepatch service. I was hoping you could advise me on how to test it?

Thanks very much for your time.

To post a comment you must log in.
Revision history for this message
Scott Moser (smoser) wrote :

@Dan, git-log makes it look like you're the the best candidate to review this... Could you take a look?

Shivaram did good work in ssh-import-id at
https://code.launchpad.net/~slingamn/ssh-import-id/+git/ssh-import-id/+merge/389139

Revision history for this message
Dan Streetman (ddstreet) wrote :

Sorry for the delay!

It all LGTM, though personally I'd like to see it reduced even farther if possible, since the livepatch use cases are rather narrow, but the http_unixdomain.py is pretty minimal so it seems ok.

To setup livepatch for testing, you can do:

$ sudo snap install canonical-livepatch

then, you'll need to get a 'key'; there are instructions if you do:

$ sudo canonical-livepatch

or you can just go here which should give you a key that's valid for at least a few instances:

https://auth.livepatch.canonical.com/?user_type=ubuntu-user

after installing the key from that page, you should be able to check livepatch status with:

$ sudo canonical-livepatch status
last check: 2 minutes ago
kernel: 5.4.0-42.46-generic
server check-in: succeeded
patch state: ✓ all applicable livepatch modules inserted
patch version: 70.3

and then you should be able to test the software-properties interaction with the livepatch snap.

If your testing goes well, the MR all looks good otherwise.

thanks!

review: Approve
Revision history for this message
Dan Streetman (ddstreet) wrote :

added Andrea Azzarone (who added all the livepatch code to software-properties) as a reviewer also

Revision history for this message
Shivaram Lingamneni (slingamn) wrote :

Thanks for your help!

I was able to enable livepatch via these instructions, but I'm not sure how to trigger the relevant software-properties. The command-line `canonical-livepatch` command appears to use a Go binary inside the snap, rather than the Python code in /usr/lib/python3/dist-packages/softwareproperties, to query the unix socket.

Is there a GUI frontend in the Gnome environment that uses this code?

Revision history for this message
Shivaram Lingamneni (slingamn) wrote :

Sorry, I'm still having difficulty figuring out how to test this change --- any advice?

Revision history for this message
Dan Streetman (ddstreet) wrote :

>
> Is there a GUI frontend in the Gnome environment that uses this code?

yep, sorry, I should have pointed you to that instead of the cmdline.

The 'long' way (but probably more common) is to open the 'gnome-software' application, and at the top-right you should see a gear icon; click that, and then 'software and updates' selection. that will open a pop-up with tabs, and at the far right will be a 'livepatch' tab.

The direct way is to just run the 'software-properties-gtk' application (which provides that 'software and updates' pop-up), that's included in the 'software-properties-gtk' binary package.

Revision history for this message
Shivaram Lingamneni (slingamn) wrote :

Thanks! I tested this in a VM and it appears to work. (I also tested with some added instrumentation that verified that the modified code was in fact being called.)

Revision history for this message
Dan Streetman (ddstreet) wrote :

Thanks, merged!

Revision history for this message
Dan Streetman (ddstreet) wrote :

(note I had to cherry-pick as there were a couple commits later than yours, so the commit id was changed to 9edb4b44fa857b142e8e0df7f35db5a7877ea1c1)

Revision history for this message
Julian Andres Klode (juliank) wrote :

I have reverted this change, as it's a change in software license that was not discussed, and it did not include sufficient licensing information.

review: Disapprove
Revision history for this message
Julian Andres Klode (juliank) wrote :

Also, was the CLA signed, and can you even submit code you don't own copyright to under the CLA?

Revision history for this message
Shivaram Lingamneni (slingamn) wrote :

I have not (to my knowledge) signed the CLA.

A similar shim is available with a Canonical copyright here as part of the "update-manager" project:

https://git.launchpad.net/ubuntu/+source/update-manager/tree/UpdateManager/Core/LivePatchSocket.py?h=ubuntu/focal

Would a patch based on this be more suitable?

Revision history for this message
Dan Streetman (ddstreet) wrote :

> I have not (to my knowledge) signed the CLA.

You can find it here: https://ubuntu.com/legal/contributors

> Would a patch based on this be more suitable?

Since that code is Copyrighted by Canonical and GPL, I don't see why it would be a problem to include it here, with proper attribution of course.

Unmerged commits

8e39f3a... by Shivaram Lingamneni

remove dependency on python3-requests

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/control b/debian/control
2index 9f9441f..5d95147 100644
3--- a/debian/control
4+++ b/debian/control
5@@ -23,7 +23,6 @@ Build-Depends: dbus-x11 <!nocheck>,
6 python3-gi <!nocheck>,
7 python3-launchpadlib,
8 python3-mock <!nocheck>,
9- python3-requests-unixsocket <!nocheck>,
10 python3-setuptools,
11 xauth <!nocheck>,
12 xvfb <!nocheck>
13@@ -60,7 +59,6 @@ Depends: ca-certificates,
14 python3,
15 python3-dbus,
16 python3-gi,
17- python3-requests-unixsocket,
18 python3-software-properties (= ${binary:Version}),
19 ${misc:Depends},
20 ${python3:Depends}
21@@ -86,7 +84,6 @@ Depends: gir1.2-goa-1.0 (>= 3.27.92-1ubuntu1),
22 python3-dateutil,
23 python3-distro-info,
24 python3-gi,
25- python3-requests-unixsocket,
26 python3-software-properties (= ${binary:Version}),
27 software-properties-common,
28 ubuntu-drivers-common (>= 1:0.2.75),
29diff --git a/softwareproperties/LivepatchService.py b/softwareproperties/LivepatchService.py
30index cb45327..7dcd889 100644
31--- a/softwareproperties/LivepatchService.py
32+++ b/softwareproperties/LivepatchService.py
33@@ -21,13 +21,13 @@
34
35 from gettext import gettext as _
36 import logging
37+import json
38
39 import gi
40 from gi.repository import Gio, GLib, GObject
41
42 try:
43 import dateutil.parser
44- import requests_unixsocket
45
46 gi.require_version('Snapd', '1')
47 from gi.repository import Snapd
48@@ -43,6 +43,7 @@ from softwareproperties.gtk.utils import (
49
50 from softwareproperties.LivepatchSnap import LivepatchSnap
51
52+from .http_unixdomain import unix_http_request
53
54 def datetime_parser(json_dict):
55 for (key, value) in json_dict.items():
56@@ -90,7 +91,6 @@ class LivepatchService(GObject.GObject):
57 self._timeout_id = 0
58
59 self._snap = LivepatchSnap()
60- self._session = requests_unixsocket.Session()
61
62 # Init Properties
63 self._availability = LivepatchAvailability.FALSE
64@@ -196,8 +196,8 @@ class LivepatchService(GObject.GObject):
65 """
66 try:
67 params = {'verbosity': 3, 'format': 'json'}
68- r = self._session.get(self.STATUS_ENDPOINT, params=params)
69- return r.json(object_hook=datetime_parser)
70+ _, response = unix_http_request('GET', self.STATUS_ENDPOINT, params=params)
71+ return json.loads(response, object_hook=datetime_parser)
72 except Exception as e:
73 logging.debug('Failed to get Livepatch status: {}'.format(str(e)))
74 return None
75@@ -222,8 +222,10 @@ class LivepatchService(GObject.GObject):
76 @retry(Exception)
77 def _enable_service_with_retry(self, token):
78 params = {'auth-token': token}
79- r = self._session.put(self.ENABLE_ENDPOINT, params=params)
80- return not r.ok, '' if r.ok else self.ENABLE_ERROR_MSG.format(r.text)
81+ code, response = unix_http_request('PUT', self.ENABLE_ENDPOINT, params=params)
82+ if 400 <= code < 600:
83+ return True, self.ENABLE_ERROR_MSG.format(response.decode('utf-8'))
84+ return False, ''
85
86 def _disable_service(self):
87 """Disable Canonical Livepatch in the current system. This function will
88@@ -240,8 +242,10 @@ class LivepatchService(GObject.GObject):
89
90 @retry(Exception)
91 def _disable_service_with_retry(self):
92- r = self._session.put(self.DISABLE_ENDPOINT)
93- return not r.ok, '' if r.ok else self.DISABLE_ERROR_MSG.format(r.text)
94+ code, response = unix_http_request('PUT', self.DISABLE_ENDPOINT)
95+ if 400 <= code < 600:
96+ return True, self.DISABLE_ERROR_MSG.format(response.decode('utf-8'))
97+ return False, ''
98
99 # Signals handlers
100 def _network_changed_cb(self, monitor, network_available):
101diff --git a/softwareproperties/http_unixdomain.py b/softwareproperties/http_unixdomain.py
102new file mode 100644
103index 0000000..767a9f4
104--- /dev/null
105+++ b/softwareproperties/http_unixdomain.py
106@@ -0,0 +1,43 @@
107+import socket
108+import http.client
109+from urllib.parse import unquote, urlencode, urlparse
110+
111+# UnixHTTPConnection is from https://github.com/msabramo/requests-unixsocket
112+# which is released under the Apache License 2.0
113+class UnixHTTPConnection(http.client.HTTPConnection, object):
114+
115+ def __init__(self, unix_socket_url, timeout=60):
116+ """Create an HTTP connection to a unix domain socket
117+
118+ :param unix_socket_url: A URL with a scheme of 'http+unix' and the
119+ netloc is a percent-encoded path to a unix domain socket. E.g.:
120+ 'http+unix://%2Ftmp%2Fprofilesvc.sock/status/pid'
121+ """
122+ super(UnixHTTPConnection, self).__init__('localhost', timeout=timeout)
123+ self.unix_socket_url = unix_socket_url
124+ self.timeout = timeout
125+ self.sock = None
126+
127+ def __del__(self): # base class does not have d'tor
128+ if self.sock:
129+ self.sock.close()
130+
131+ def connect(self):
132+ sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
133+ sock.settimeout(self.timeout)
134+ socket_path = unquote(urlparse(self.unix_socket_url).netloc)
135+ sock.connect(socket_path)
136+ self.sock = sock
137+
138+def unix_http_request(verb, url, params=None):
139+ parsed_url = urlparse(url)
140+ conn = UnixHTTPConnection(url)
141+ try:
142+ path = parsed_url.path
143+ if params is not None:
144+ path = '%s?%s' % (parsed_url.path, urlencode(params))
145+ conn.request(verb.upper(), path)
146+ response = conn.getresponse()
147+ return response.status, response.read()
148+ finally:
149+ conn.close()

Subscribers

People subscribed via source and target branches