Merge ~ahasenack/ubuntu/+source/landscape-client:trusty-landscape-client-1721383 into ~usd-import-team/ubuntu/+source/landscape-client:ubuntu/trusty-devel

Proposed by Andreas Hasenack on 2017-11-23
Status: Merged
Merge reported by: ChristianEhrhardt
Merged at revision: 9a2c9d1fe4a22822f9d05903c6af0da42a4b56c2
Proposed branch: ~ahasenack/ubuntu/+source/landscape-client:trusty-landscape-client-1721383
Merge into: ~usd-import-team/ubuntu/+source/landscape-client:ubuntu/trusty-devel
Diff against target: 913 lines (+873/-0)
6 files modified
debian/changelog (+10/-0)
debian/patches/autoremovable-report-1208393.diff (+317/-0)
debian/patches/config-no-reregister-1618483.diff (+184/-0)
debian/patches/iso-configuration-1699789.diff (+87/-0)
debian/patches/package-reporter-proxy-1531150.diff (+271/-0)
debian/patches/series (+4/-0)
Reviewer Review Type Date Requested Status
Simon Poirier (community) 2017-11-23 Approve on 2017-11-24
ChristianEhrhardt 2017-11-23 Approve on 2017-11-24
Eric Desrochers 2017-11-23 Pending
Review via email: mp+334203@code.launchpad.net
To post a comment you must log in.
9a2c9d1... by Andreas Hasenack on 2017-11-23

changelog

ChristianEhrhardt (paelzer) wrote :

From a packaging perspective this LGTM, as with the other one the actual code review will be by Simon/Eric.
Note: I've seen that you added SRU Templates, but as a heads up empty "Regression potential" sections are usually teasers for the SRU Team to think into it really deep and then Nack you for some potential regression (teaches maintainers to think about regressions themselves). If possible, give it a thought and outline the potential issues :-)

review: Approve
Simon Poirier (simpoir) wrote :

+1 LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index a03f5db..2ae1ca0 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,13 @@
6+landscape-client (14.12-0ubuntu6.14.04.1) trusty; urgency=medium
7+
8+ [ Simon Poirier ]
9+ * Add proxy handling to package reporter. (LP: #1531150)
10+ * Fix regression in configuration hook under install-cd chroot (LP: #1699789)
11+ * Report autoremovable packages (LP: #1208393)
12+ * No not re-register client by default (LP: #1618483)
13+
14+ -- Andreas Hasenack <andreas@canonical.com> Fri, 10 Nov 2017 16:21:54 -0200
15+
16 landscape-client (14.12-0ubuntu6.14.04) trusty; urgency=medium
17
18 * Minor updates (LP: #1636477):
19diff --git a/debian/patches/autoremovable-report-1208393.diff b/debian/patches/autoremovable-report-1208393.diff
20new file mode 100644
21index 0000000..eab7407
22--- /dev/null
23+++ b/debian/patches/autoremovable-report-1208393.diff
24@@ -0,0 +1,317 @@
25+Description: Package autoremove reporting
26+ - Add autoremove state to store schema
27+ - autoremove package report
28+Author: Simon Poirier <simpoir@gmail.com>
29+Origin: upstream, https://github.com/CanonicalLtd/landscape-client/commit/4807ca51a92814acaa51156d7172a274309f2934
30+Bug-Ubuntu: https://launchpad.net/bugs/1208393
31+Last-Update: 2017-11-10
32+---
33+This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
34+--- a/landscape/package/facade.py
35++++ b/landscape/package/facade.py
36+@@ -422,6 +422,10 @@
37+ return False
38+ return version > version.package.installed
39+
40++ def is_package_autoremovable(self, version):
41++ """Was the package auto-installed, but isn't required anymore?"""
42++ return version.package.is_auto_removable
43++
44+ def _is_main_architecture(self, package):
45+ """Is the package for the facade's main architecture?"""
46+ # package.name includes the architecture, if it's for a foreign
47+--- a/landscape/package/reporter.py
48++++ b/landscape/package/reporter.py
49+@@ -341,6 +341,7 @@
50+ self._store.clear_installed()
51+ self._store.clear_locked()
52+ self._store.clear_hash_id_requests()
53++ self._store.clear_autoremovable()
54+
55+ return succeed(None)
56+
57+@@ -542,11 +543,13 @@
58+ old_available = set(self._store.get_available())
59+ old_upgrades = set(self._store.get_available_upgrades())
60+ old_locked = set(self._store.get_locked())
61++ old_autoremovable = set(self._store.get_autoremovable())
62+
63+ current_installed = set()
64+ current_available = set()
65+ current_upgrades = set()
66+ current_locked = set()
67++ current_autoremovable = set()
68+ lsb = parse_lsb_release(LSB_RELEASE_FILENAME)
69+ backports_archive = "{}-backports".format(lsb["code-name"])
70+
71+@@ -574,6 +577,8 @@
72+ current_installed.add(id)
73+ if self._facade.is_package_available(package):
74+ current_available.add(id)
75++ if self._facade.is_package_autoremovable(package):
76++ current_autoremovable.add(id)
77+ else:
78+ current_available.add(id)
79+
80+@@ -591,11 +596,13 @@
81+ new_available = current_available - old_available
82+ new_upgrades = current_upgrades - old_upgrades
83+ new_locked = current_locked - old_locked
84++ new_autoremovable = current_autoremovable - old_autoremovable
85+
86+ not_installed = old_installed - current_installed
87+ not_available = old_available - current_available
88+ not_upgrades = old_upgrades - current_upgrades
89+ not_locked = old_locked - current_locked
90++ not_autoremovable = old_autoremovable - current_autoremovable
91+
92+ message = {}
93+ if new_installed:
94+@@ -611,6 +618,13 @@
95+ message["locked"] = \
96+ list(sequence_to_ranges(sorted(new_locked)))
97+
98++ if new_autoremovable:
99++ message["autoremovable"] = list(
100++ sequence_to_ranges(sorted(new_autoremovable)))
101++ if not_autoremovable:
102++ message["not-autoremovable"] = list(
103++ sequence_to_ranges(sorted(not_autoremovable)))
104++
105+ if not_installed:
106+ message["not-installed"] = \
107+ list(sequence_to_ranges(sorted(not_installed)))
108+@@ -632,12 +646,15 @@
109+
110+ logging.info("Queuing message with changes in known packages: "
111+ "%d installed, %d available, %d available upgrades, "
112+- "%d locked, %d not installed, %d not available, "
113+- "%d not available upgrades, %d not locked."
114++ "%d locked, %d autoremovable, %d not installed, "
115++ "%d not available, %d not available upgrades, "
116++ "%d not locked, %d not autoremovable. "
117+ % (len(new_installed), len(new_available),
118+ len(new_upgrades), len(new_locked),
119++ len(new_autoremovable),
120+ len(not_installed), len(not_available),
121+- len(not_upgrades), len(not_locked)))
122++ len(not_upgrades), len(not_locked),
123++ len(not_autoremovable)))
124+
125+ def update_currently_known(result):
126+ if new_installed:
127+@@ -648,6 +665,8 @@
128+ self._store.add_available(new_available)
129+ if new_locked:
130+ self._store.add_locked(new_locked)
131++ if new_autoremovable:
132++ self._store.add_autoremovable(new_autoremovable)
133+ if not_available:
134+ self._store.remove_available(not_available)
135+ if new_upgrades:
136+@@ -656,6 +675,8 @@
137+ self._store.remove_available_upgrades(not_upgrades)
138+ if not_locked:
139+ self._store.remove_locked(not_locked)
140++ if not_autoremovable:
141++ self._store.remove_autoremovable(not_autoremovable)
142+ # Something has changed wrt the former run, let's update the
143+ # timestamp and return True.
144+ stamp_file = self._config.detect_package_changes_stamp
145+--- a/landscape/package/store.py
146++++ b/landscape/package/store.py
147+@@ -208,6 +208,25 @@
148+ return [row[0] for row in cursor.fetchall()]
149+
150+ @with_cursor
151++ def add_autoremovable(self, cursor, ids):
152++ for id in ids:
153++ cursor.execute("REPLACE INTO autoremovable VALUES (?)", (id,))
154++
155++ @with_cursor
156++ def remove_autoremovable(self, cursor, ids):
157++ id_list = ",".join(str(int(id)) for id in ids)
158++ cursor.execute("DELETE FROM autoremovable WHERE id IN (%s)" % id_list)
159++
160++ @with_cursor
161++ def clear_autoremovable(self, cursor):
162++ cursor.execute("DELETE FROM autoremovable")
163++
164++ @with_cursor
165++ def get_autoremovable(self, cursor):
166++ cursor.execute("SELECT id FROM autoremovable")
167++ return [row[0] for row in cursor.fetchall()]
168++
169++ @with_cursor
170+ def add_installed(self, cursor, ids):
171+ for id in ids:
172+ cursor.execute("REPLACE INTO installed VALUES (?)", (id,))
173+@@ -424,6 +443,8 @@
174+ # try block.
175+ cursor = db.cursor()
176+ try:
177++ cursor.execute("CREATE TABLE autoremovable"
178++ " (id INTEGER PRIMARY KEY)")
179+ cursor.execute("CREATE TABLE locked"
180+ " (id INTEGER PRIMARY KEY)")
181+ cursor.execute("CREATE TABLE available"
182+--- a/landscape/package/tests/test_facade.py
183++++ b/landscape/package/tests/test_facade.py
184+@@ -817,6 +817,30 @@
185+ self.assertEqual("version2-release2", version2.version)
186+ self.assertFalse(self.facade.is_package_installed(version2))
187+
188++ def test_is_package_autoremovable(self):
189++ """
190++ Check that auto packages without dependencies on them are correctly
191++ detected as being autoremovable.
192++ """
193++ self._add_system_package("dep")
194++ self._add_system_package("newdep")
195++ self._add_system_package("foo", control_fields={"Depends": "newdep"})
196++ self.facade.reload_channels()
197++ dep, = sorted(self.facade.get_packages_by_name("dep"))
198++ dep.package.mark_auto(True)
199++ # dep should not be explicitely installed
200++ dep.package.mark_install(False)
201++ newdep, = sorted(self.facade.get_packages_by_name("newdep"))
202++ newdep, = sorted(self.facade.get_packages_by_name("newdep"))
203++ newdep.package.mark_auto(True)
204++ self.assertTrue(dep.package.is_installed)
205++ self.assertTrue(dep.package.is_auto_installed)
206++ self.assertTrue(newdep.package.is_installed)
207++ self.assertTrue(dep.package.is_auto_installed)
208++
209++ self.assertTrue(self.facade.is_package_autoremovable(dep))
210++ self.assertFalse(self.facade.is_package_autoremovable(newdep))
211++
212+ def test_is_package_available_in_channel_not_installed(self):
213+ """
214+ A package is considered available if the package is in a
215+--- a/landscape/package/tests/test_reporter.py
216++++ b/landscape/package/tests/test_reporter.py
217+@@ -84,6 +84,15 @@
218+ """Make it so that package "name1" is considered installed."""
219+ self._install_deb_file(os.path.join(self.repository_dir, PKGNAME1))
220+
221++ def set_pkg1_autoremovable(self):
222++ """Make it so package "name1" is considered auto removable."""
223++ self.set_pkg1_installed()
224++ self.facade.reload_channels()
225++ name1 = sorted(self.facade.get_packages_by_name("name1"))[0]
226++ # Since no other package depends on this, all that's needed
227++ # to have it autoremovable is to mark it as installed+auto.
228++ name1.package.mark_auto(True)
229++
230+ def _make_fake_apt_update(self, out="output", err="error", code=0):
231+ """Create a fake apt-update executable"""
232+ self.reporter.apt_update_filename = self.makeFile(
233+@@ -971,6 +980,53 @@
234+ result = self.reporter.detect_packages_changes()
235+ return result.addCallback(got_result)
236+
237++ def test_detect_packages_changes_with_autoremovable(self):
238++ message_store = self.broker_service.message_store
239++ message_store.set_accepted_types(["packages"])
240++
241++ self.store.set_hash_ids({HASH1: 1, HASH2: 2, HASH3: 3})
242++ self.store.add_available([1, 2, 3])
243++ self.store.add_installed([1])
244++ self.set_pkg1_autoremovable()
245++
246++ result = self.successResultOf(self.reporter.detect_packages_changes())
247++ self.assertTrue(result)
248++
249++ expected = [{"type": "packages", "autoremovable": [1]}]
250++ self.assertMessages(message_store.get_pending_messages(), expected)
251++ self.assertEqual([1], self.store.get_autoremovable())
252++
253++ def test_detect_packages_changes_with_not_autoremovable(self):
254++ message_store = self.broker_service.message_store
255++ message_store.set_accepted_types(["packages"])
256++
257++ self.store.set_hash_ids({HASH1: 1, HASH2: 2, HASH3: 3})
258++ self.store.add_available([1, 2, 3])
259++ # We don't care about checking other state changes in this test.
260++ # In reality the package would also be installed, available, etc.
261++ self.store.add_autoremovable([1, 2])
262++
263++ result = self.successResultOf(self.reporter.detect_packages_changes())
264++ self.assertTrue(result)
265++
266++ expected = [{"type": "packages", "not-autoremovable": [1, 2]}]
267++ self.assertMessages(message_store.get_pending_messages(), expected)
268++ self.assertEqual([], self.store.get_autoremovable())
269++
270++ def test_detect_packages_changes_with_known_autoremovable(self):
271++ message_store = self.broker_service.message_store
272++ message_store.set_accepted_types(["packages"])
273++
274++ self.store.set_hash_ids({HASH1: 1, HASH2: 2, HASH3: 3})
275++ self.store.add_available([1, 2, 3])
276++ self.store.add_installed([1])
277++ self.store.add_autoremovable([1])
278++ self.set_pkg1_autoremovable()
279++
280++ result = self.successResultOf(self.reporter.detect_packages_changes())
281++ self.assertFalse(result)
282++ self.assertEqual([1], self.store.get_autoremovable())
283++
284+ def test_detect_packages_changes_with_backports(self):
285+ """
286+ Package versions coming from backports aren't considered to be
287+--- a/landscape/package/tests/test_store.py
288++++ b/landscape/package/tests/test_store.py
289+@@ -284,6 +284,23 @@
290+ self.store1.clear_available_upgrades()
291+ self.assertEqual(self.store2.get_available_upgrades(), [])
292+
293++ def test_add_and_get_autoremovable(self):
294++ self.store1.add_autoremovable([1, 2])
295++ value = self.store1.get_autoremovable()
296++ self.assertEqual([1, 2], value)
297++
298++ def test_clear_autoremovable(self):
299++ self.store1.add_autoremovable([1, 2])
300++ self.store1.clear_autoremovable()
301++ value = self.store1.get_autoremovable()
302++ self.assertEqual([], value)
303++
304++ def test_remove_autoremovable(self):
305++ self.store1.add_autoremovable([1, 2, 3, 4])
306++ self.store1.remove_autoremovable([2, 4, 5])
307++ value = self.store1.get_autoremovable()
308++ self.assertEqual([1, 3], value)
309++
310+ def test_add_and_get_installed_packages(self):
311+ self.store1.add_installed([1, 2])
312+ self.assertEqual(self.store2.get_installed(), [1, 2])
313+@@ -350,6 +367,9 @@
314+ cursor.execute("pragma table_info(locked)")
315+ result = cursor.fetchall()
316+ self.assertTrue(len(result) > 0)
317++ cursor.execute("pragma table_info(autoremovable)")
318++ result = cursor.fetchall()
319++ self.assertTrue(len(result) > 0)
320+
321+ def test_add_and_get_locked(self):
322+ """
323+--- a/landscape/message_schemas.py
324++++ b/landscape/message_schemas.py
325+@@ -350,13 +350,15 @@
326+ "available": package_ids_or_ranges,
327+ "available-upgrades": package_ids_or_ranges,
328+ "locked": package_ids_or_ranges,
329++ "autoremovable": package_ids_or_ranges,
330++ "not-autoremovable": package_ids_or_ranges,
331+ "not-installed": package_ids_or_ranges,
332+ "not-available": package_ids_or_ranges,
333+ "not-available-upgrades": package_ids_or_ranges,
334+ "not-locked": package_ids_or_ranges},
335+ optional=["installed", "available", "available-upgrades", "locked",
336+ "not-available", "not-installed", "not-available-upgrades",
337+- "not-locked"])
338++ "not-locked", "autoremovable", "not-autoremovable"])
339+
340+ package_locks = List(Tuple(Unicode(), Unicode(), Unicode()))
341+ PACKAGE_LOCKS = Message(
342diff --git a/debian/patches/config-no-reregister-1618483.diff b/debian/patches/config-no-reregister-1618483.diff
343new file mode 100644
344index 0000000..0a564d6
345--- /dev/null
346+++ b/debian/patches/config-no-reregister-1618483.diff
347@@ -0,0 +1,184 @@
348+Description: Default to not register again if already registered
349+ When running the landscape-config script, change the default answer to the
350+ registration question to "No" if this client is already registered.
351+Author: Alberto Donato <alberto.donato@canonical.com>
352+Origin: upstream, http://bazaar.launchpad.net/~landscape/landscape-client/trunk/revision/939
353+Bug: https://launchpad.net/bugs/1618483
354+Last-Update: 2017-11-10
355+---
356+This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
357+--- a/landscape/configuration.py
358++++ b/landscape/configuration.py
359+@@ -22,8 +22,10 @@
360+ from landscape.lib.twisted_util import gather_results
361+ from landscape.lib.fetch import fetch, FetchError
362+ from landscape.lib.bootstrap import BootstrapList, BootstrapDirectory
363++from landscape.lib.persist import Persist
364+ from landscape.reactor import LandscapeReactor
365+-from landscape.broker.registration import InvalidCredentialsError
366++from landscape.broker.registration import Identity, InvalidCredentialsError
367++from landscape.broker.service import BrokerService
368+ from landscape.broker.config import BrokerConfiguration
369+ from landscape.broker.amp import RemoteBrokerConnector
370+
371+@@ -731,6 +733,15 @@
372+ return 2 # An error happened
373+
374+
375++def is_registered(config):
376++ """Return whether the client is already registered."""
377++ persist_filename = os.path.join(
378++ config.data_path, "{}.bpickle".format(BrokerService.service_name))
379++ persist = Persist(filename=persist_filename)
380++ identity = Identity(config, persist)
381++ return bool(identity.secure_id)
382++
383++
384+ def main(args, print=print):
385+ """Interact with the user and the server to set up client configuration."""
386+
387+@@ -769,9 +780,13 @@
388+ report_registration_outcome(result, print=print)
389+ sys.exit(determine_exit_code(result))
390+ else:
391+- answer = raw_input("\nRequest a new registration for "
392+- "this computer now? (Y/n): ")
393+- if not answer.upper().startswith("N"):
394++ script = LandscapeSetupScript(config)
395++ default_answer = not is_registered(config)
396++ answer = script.prompt_yes_no(
397++ "\nRequest a new registration for this computer now?",
398++ default=default_answer)
399++
400++ if answer:
401+ result = register(config, reactor)
402+ report_registration_outcome(result, print=print)
403+ sys.exit(determine_exit_code(result))
404+--- a/landscape/tests/test_configuration.py
405++++ b/landscape/tests/test_configuration.py
406+@@ -7,10 +7,12 @@
407+ import sys
408+ import unittest
409+
410++import mock
411+ from twisted.internet.defer import succeed, fail, Deferred
412+
413+ from landscape.broker.registration import InvalidCredentialsError
414+-from landscape.broker.tests.helpers import RemoteBrokerHelper
415++from landscape.broker.tests.helpers import (
416++ RemoteBrokerHelper, BrokerConfigurationHelper)
417+ from landscape.configuration import (
418+ print_text, LandscapeSetupScript, LandscapeSetupConfiguration,
419+ register, setup, main, setup_init_script_and_start_client,
420+@@ -18,15 +20,17 @@
421+ ImportOptionError, store_public_key_data,
422+ bootstrap_tree, got_connection, success, failure, exchange_failure,
423+ handle_registration_errors, done, got_error, report_registration_outcome,
424+- determine_exit_code)
425++ determine_exit_code, is_registered)
426+ from landscape.lib.amp import MethodCallError
427+ from landscape.lib.fetch import HTTPCodeError, PyCurlError
428++from landscape.lib.persist import Persist
429+ from landscape.sysvconfig import SysVConfig, ProcessError
430+ from landscape.tests.helpers import FakeBrokerServiceHelper
431+ from landscape.tests.helpers import LandscapeTest, EnvironSaverHelper
432+ from landscape.tests.mocker import ANY, MATCH, CONTAINS, expect
433+
434+
435++
436+ class LandscapeConfigurationTest(LandscapeTest):
437+
438+ def get_config(self, args, data_path=None):
439+@@ -1167,9 +1171,9 @@
440+ setup_mock = self.mocker.replace(setup)
441+ setup_mock(ANY)
442+
443+- raw_input_mock = self.mocker.replace(raw_input)
444++ raw_input_mock = self.mocker.replace(raw_input, passthrough=False)
445+ raw_input_mock("\nRequest a new registration for "
446+- "this computer now? (Y/n): ")
447++ "this computer now? [Y/n]")
448+ self.mocker.result("n")
449+
450+ # This must not be called.
451+@@ -1215,7 +1219,7 @@
452+
453+ raw_input_mock = self.mocker.replace(raw_input)
454+ raw_input_mock("\nRequest a new registration for "
455+- "this computer now? (Y/n): ")
456++ "this computer now? [Y/n]")
457+ self.mocker.result("y")
458+
459+ # The register() function will be called.
460+@@ -1246,7 +1250,7 @@
461+
462+ raw_input_mock = self.mocker.replace(raw_input)
463+ raw_input_mock("\nRequest a new registration for "
464+- "this computer now? (Y/n): ")
465++ "this computer now? [Y/n]")
466+ self.mocker.result("y")
467+
468+ # The register() function will be called.
469+@@ -1337,13 +1341,15 @@
470+ printed)
471+
472+ def make_working_config(self):
473++ data_path = self.makeFile()
474+ return self.makeFile("[client]\n"
475+ "computer_title = Old Title\n"
476+ "account_name = Old Name\n"
477+ "registration_key = Old Password\n"
478+ "http_proxy = http://old.proxy\n"
479+ "https_proxy = https://old.proxy\n"
480+- "url = http://url\n")
481++ "data_path = {}\n"
482++ "url = http://url\n".format(data_path))
483+
484+ def test_register(self):
485+ sysvconfig_mock = self.mocker.patch(SysVConfig)
486+@@ -1357,7 +1363,7 @@
487+
488+ raw_input_mock = self.mocker.replace(raw_input)
489+ raw_input_mock("\nRequest a new registration for "
490+- "this computer now? (Y/n): ")
491++ "this computer now? [Y/n]")
492+ self.mocker.result("")
493+
494+ raw_input_mock("\nThe Landscape client must be started "
495+@@ -1424,7 +1430,7 @@
496+ setup_mock(ANY)
497+ raw_input_mock = self.mocker.replace(raw_input)
498+ raw_input_mock("\nRequest a new registration for "
499+- "this computer now? (Y/n): ")
500++ "this computer now? [Y/n]")
501+ self.mocker.result("")
502+
503+ register_mock = self.mocker.replace(register, passthrough=False)
504+@@ -2322,3 +2328,27 @@
505+ for code in failure_codes:
506+ result = determine_exit_code(code)
507+ self.assertEqual(2, result)
508++
509++
510++class IsRegisteredTest(LandscapeTest):
511++
512++ helpers = [BrokerConfigurationHelper]
513++
514++ def setUp(self):
515++ super(IsRegisteredTest, self).setUp()
516++ persist_file = os.path.join(self.config.data_path, "broker.bpickle")
517++ self.persist = Persist(filename=persist_file)
518++
519++ def test_is_registered_false(self):
520++ """
521++ If the client hasn't previouly registered, is_registered returns False.
522++ """
523++ self.assertFalse(is_registered(self.config))
524++
525++ def test_is_registered_true(self):
526++ """
527++ If the client has previouly registered, is_registered returns True.
528++ """
529++ self.persist.set("registration.secure-id", "super-secure")
530++ self.persist.save()
531++ self.assertTrue(is_registered(self.config))
532diff --git a/debian/patches/iso-configuration-1699789.diff b/debian/patches/iso-configuration-1699789.diff
533new file mode 100644
534index 0000000..1adcd6d
535--- /dev/null
536+++ b/debian/patches/iso-configuration-1699789.diff
537@@ -0,0 +1,87 @@
538+Description: Stop reactor in landscape-config on broker error
539+ Handle SystemExit exception, as those don't propagate past the reactor, in
540+ order to avoid getting stuck when installed in chroot.
541+Author: Simon Poirier <simpoir@gmail.com>
542+Origin: upstream, https://github.com/CanonicalLtd/landscape-client/commit/e0b1b0ca43c13bc65374df13d192405abb3014e6
543+Bug-Ubuntu: https://launchpad.net/bugs/1699789
544+Last-Update: 2017-11-10
545+---
546+This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
547+--- a/landscape/configuration.py
548++++ b/landscape/configuration.py
549+@@ -639,10 +639,12 @@
550+ return results
551+
552+
553+-def got_error(failure, print=print):
554+- """...from broker."""
555++def got_error(failure, reactor, add_result, print=print):
556++ """Handle errors contacting broker."""
557+ print(failure.getTraceback(), file=sys.stderr)
558+- raise SystemExit
559++ # Can't just raise SystemExit; it would be ignored by the reactor.
560++ add_result(SystemExit())
561++ reactor.stop()
562+
563+
564+ def register(config, reactor=None, connector_factory=RemoteBrokerConnector,
565+@@ -671,6 +673,7 @@
566+ """
567+ if reactor is None:
568+ reactor = LandscapeReactor()
569++
570+ if results is None:
571+ results = []
572+ add_result = results.append
573+@@ -679,13 +682,17 @@
574+ connection = connector.connect(max_retries=max_retries, quiet=True)
575+ connection.addCallback(
576+ partial(got_connection, add_result, connector, reactor))
577+- connection.addErrback(got_error)
578++ connection.addErrback(
579++ partial(got_error, reactor=reactor, add_result=add_result))
580+ reactor.run()
581+
582+ assert len(results) == 1, "We expect exactly one result."
583+ # Results will be things like "success" or "ssl-error".
584+ result = results[0]
585+
586++ if isinstance(result, SystemExit):
587++ raise result
588++
589+ # If there was an error and the caller requested that errors be reported
590+ # to the on_error callable, then do so.
591+ if result != "success" and on_error is not None:
592+--- a/landscape/tests/test_configuration.py
593++++ b/landscape/tests/test_configuration.py
594+@@ -172,14 +172,19 @@
595+ def getTraceback(self):
596+ return "traceback"
597+
598++ results = []
599+ printed = []
600+
601+ def faux_print(text, file):
602+ printed.append((text, file))
603+
604+- with self.assertRaises(SystemExit):
605+- got_error(FauxFailure(), print=faux_print)
606++ mock_reactor = mock.Mock()
607+
608++ got_error(FauxFailure(), reactor=mock_reactor,
609++ add_result=results.append, print=faux_print)
610++ mock_reactor.stop.assert_called_once_with()
611++
612++ self.assertIsInstance(results[0], SystemExit)
613+ self.assertEqual([('traceback', sys.stderr)], printed)
614+
615+
616+@@ -2081,7 +2086,7 @@
617+ self.assertTrue(1, len(connector.connection.errbacks))
618+ self.assertEqual(
619+ 'got_error',
620+- connector.connection.errbacks[0].__name__)
621++ connector.connection.errbacks[0].func.__name__)
622+ # We ask for retries because networks aren't reliable.
623+ self.assertEqual(99, connector.max_retries)
624+
625diff --git a/debian/patches/package-reporter-proxy-1531150.diff b/debian/patches/package-reporter-proxy-1531150.diff
626new file mode 100644
627index 0000000..b154609
628--- /dev/null
629+++ b/debian/patches/package-reporter-proxy-1531150.diff
630@@ -0,0 +1,271 @@
631+Description: Add proxy handling to package reporter
632+Author: Simon Poirier <simon.poirier@canonical.com>
633+Origin: upstream, http://bazaar.launchpad.net/~landscape/landscape-client/trunk/revision/919
634+Bug: https://launchpad.net/bugs/1531150
635+Last-Update: 2017-11-10
636+---
637+This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
638+--- a/apt-update/apt-update.c
639++++ b/apt-update/apt-update.c
640+@@ -19,7 +19,7 @@
641+ int main(int argc, char *argv[], char *envp[])
642+ {
643+ char *apt_argv[] = {"/usr/bin/apt-get", "-q", "update", NULL};
644+- char *apt_envp[] = {"PATH=/bin:/usr/bin", NULL, NULL};
645++ char *apt_envp[] = {"PATH=/bin:/usr/bin", NULL, NULL, NULL, NULL};
646+
647+ // Set the HOME environment variable
648+ struct passwd *pwd = getpwuid(geteuid());
649+@@ -33,6 +33,24 @@
650+ exit(1);
651+ }
652+
653++ // Pass proxy environment variables
654++ int proxy_arg = 2;
655++ char *http_proxy = getenv("http_proxy");
656++ if (http_proxy) {
657++ if (asprintf(&apt_envp[proxy_arg], "http_proxy=%s", http_proxy) == -1) {
658++ perror("error: Unable to set http_proxy environment variable");
659++ exit(1);
660++ }
661++ proxy_arg++;
662++ }
663++ char *https_proxy = getenv("https_proxy");
664++ if (https_proxy) {
665++ if (asprintf(&apt_envp[proxy_arg], "https_proxy=%s", https_proxy) == -1) {
666++ perror("error: Unable to set https_proxy environment variable");
667++ exit(1);
668++ }
669++ }
670++
671+ // Drop any supplementary group
672+ if (setgroups(0, NULL) == -1) {
673+ perror("error: Unable to set supplementary groups IDs");
674+--- a/landscape/lib/fetch.py
675++++ b/landscape/lib/fetch.py
676+@@ -45,7 +45,7 @@
677+
678+ def fetch(url, post=False, data="", headers={}, cainfo=None, curl=None,
679+ connect_timeout=30, total_timeout=600, insecure=False, follow=True,
680+- user_agent=None):
681++ user_agent=None, proxy=None):
682+ """Retrieve a URL and return the content.
683+
684+ @param url: The url to be fetched.
685+@@ -61,6 +61,7 @@
686+ during autodiscovery)
687+ @param follow: If True, follow HTTP redirects (default True).
688+ @param user_agent: The user-agent to set in the request.
689++ @param proxy: The proxy url to use for the request.
690+ """
691+ import pycurl
692+ output = StringIO(data)
693+@@ -94,6 +95,9 @@
694+ if user_agent is not None:
695+ curl.setopt(pycurl.USERAGENT, user_agent)
696+
697++ if proxy is not None:
698++ curl.setopt(pycurl.PROXY, proxy)
699++
700+ curl.setopt(pycurl.MAXREDIRS, 5)
701+ curl.setopt(pycurl.CONNECTTIMEOUT, connect_timeout)
702+ curl.setopt(pycurl.LOW_SPEED_LIMIT, 1)
703+--- a/landscape/lib/tests/test_fetch.py
704++++ b/landscape/lib/tests/test_fetch.py
705+@@ -286,6 +286,14 @@
706+ self.assertEqual(result, "result")
707+ self.assertEqual("user-agent", curl.options[pycurl.USERAGENT])
708+
709++ def test_pycurl_proxy(self):
710++ """If provided, the proxy is set in the request."""
711++ curl = CurlStub("result")
712++ proxy = "http://my.little.proxy"
713++ result = fetch("http://example.com", curl=curl, proxy=proxy)
714++ self.assertEqual("result", result)
715++ self.assertEqual(proxy, curl.options[pycurl.PROXY])
716++
717+ def test_create_curl(self):
718+ curls = []
719+
720+--- a/landscape/package/reporter.py
721++++ b/landscape/package/reporter.py
722+@@ -38,6 +38,10 @@
723+ parser.add_option("--force-apt-update", default=False,
724+ action="store_true",
725+ help="Force running apt-update.")
726++ parser.add_option("--http-proxy", metavar="URL",
727++ help="The URL of the HTTP proxy, if one is needed.")
728++ parser.add_option("--https-proxy", metavar="URL",
729++ help="The URL of the HTTPS proxy, if one is needed.")
730+ return parser
731+
732+
733+@@ -133,8 +137,14 @@
734+ logging.warning("Couldn't download hash=>id database: %s" %
735+ str(exception))
736+
737++ if url.startswith("https"):
738++ proxy = self._config.get("https_proxy")
739++ else:
740++ proxy = self._config.get("http_proxy")
741++
742+ result = fetch_async(url,
743+- cainfo=self._config.get("ssl_public_key"))
744++ cainfo=self._config.get("ssl_public_key"),
745++ proxy=proxy)
746+ result.addCallback(fetch_ok)
747+ result.addErrback(fetch_error)
748+
749+@@ -262,7 +272,12 @@
750+ Run apt-update using the passed in deferred, which allows for callers
751+ to inspect the result code.
752+ """
753+- result = spawn_process(self.apt_update_filename)
754++ env = {}
755++ if self._config.http_proxy:
756++ env["http_proxy"] = self._config.http_proxy
757++ if self._config.https_proxy:
758++ env["https_proxy"] = self._config.https_proxy
759++ result = spawn_process(self.apt_update_filename, env=env)
760+
761+ def callback((out, err, code), deferred):
762+ return deferred.callback((out, err, code))
763+--- a/landscape/package/tests/test_reporter.py
764++++ b/landscape/package/tests/test_reporter.py
765+@@ -3,11 +3,11 @@
766+ import time
767+ import apt_pkg
768+ import shutil
769++import mock
770+
771+ from twisted.internet.defer import Deferred, succeed, inlineCallbacks
772+ from twisted.internet import reactor
773+
774+-
775+ from landscape.lib.fs import create_file, touch_file
776+ from landscape.lib.fetch import fetch_async, FetchError
777+ from landscape.lib.lsb_release import parse_lsb_release, LSB_RELEASE_FILENAME
778+@@ -286,7 +286,7 @@
779+ hash_id_db_url = self.config.package_hash_id_url + "uuid_codename_arch"
780+ fetch_async_mock = self.mocker.replace("landscape.lib."
781+ "fetch.fetch_async")
782+- fetch_async_mock(hash_id_db_url, cainfo=None)
783++ fetch_async_mock(hash_id_db_url, cainfo=None, patch=None)
784+ fetch_async_result = Deferred()
785+ fetch_async_result.callback("hash-ids")
786+ self.mocker.result(fetch_async_result)
787+@@ -311,6 +311,33 @@
788+
789+ return result
790+
791++ @mock.patch("landscape.package.reporter.fetch_async",
792++ return_value=succeed("hash-ids"))
793++ @mock.patch("logging.info", return_value=None)
794++ def test_fetch_hash_id_db_with_proxy(self, logging_mock, mock_fetch_async):
795++ """fetching hash-id-db uses proxy settings"""
796++ # Assume package_hash_id_url is set
797++ self.config.data_path = self.makeDir()
798++ self.config.package_hash_id_url = "https://fake.url/path/"
799++ os.makedirs(os.path.join(self.config.data_path, "package", "hash-id"))
800++
801++ # Fake uuid, codename and arch
802++ message_store = self.broker_service.message_store
803++ message_store.set_server_uuid("uuid")
804++ self.reporter.lsb_release_filename = self.makeFile(SAMPLE_LSB_RELEASE)
805++ self.facade.set_arch("arch")
806++
807++ # Let's say fetch_async is successful
808++ hash_id_db_url = self.config.package_hash_id_url + "uuid_codename_arch"
809++
810++ # set proxy settings
811++ self.config.https_proxy = "http://helloproxy:8000"
812++
813++ result = self.reporter.fetch_hash_id_db()
814++ mock_fetch_async.assert_called_once_with(
815++ hash_id_db_url, cainfo=None, proxy="http://helloproxy:8000")
816++ return result
817++
818+ def test_fetch_hash_id_db_does_not_download_twice(self):
819+
820+ # Let's say that the hash=>id database is already there
821+@@ -330,7 +357,7 @@
822+ # Intercept any call to fetch_async
823+ fetch_async_mock = self.mocker.replace("landscape.lib."
824+ "fetch.fetch_async")
825+- fetch_async_mock(ANY)
826++ fetch_async_mock(ANY, proxy=None)
827+
828+ # Go!
829+ self.mocker.replay()
830+@@ -430,7 +457,7 @@
831+ "uuid_codename_arch"
832+ fetch_async_mock = self.mocker.replace("landscape.lib."
833+ "fetch.fetch_async")
834+- fetch_async_mock(hash_id_db_url, cainfo=None)
835++ fetch_async_mock(hash_id_db_url, cainfo=None, proxy=None)
836+ fetch_async_result = Deferred()
837+ fetch_async_result.callback("hash-ids")
838+ self.mocker.result(fetch_async_result)
839+@@ -462,7 +489,7 @@
840+ hash_id_db_url = self.config.package_hash_id_url + "uuid_codename_arch"
841+ fetch_async_mock = self.mocker.replace("landscape.lib."
842+ "fetch.fetch_async")
843+- fetch_async_mock(hash_id_db_url, cainfo=None)
844++ fetch_async_mock(hash_id_db_url, cainfo=None, proxy=None)
845+ fetch_async_result = Deferred()
846+ fetch_async_result.errback(FetchError("fetch error"))
847+ self.mocker.result(fetch_async_result)
848+@@ -538,7 +565,7 @@
849+ "uuid_codename_arch"
850+ fetch_async_mock = self.mocker.replace("landscape.lib."
851+ "fetch.fetch_async")
852+- fetch_async_mock(hash_id_db_url, cainfo=self.config.ssl_public_key)
853++ fetch_async_mock(hash_id_db_url, cainfo=self.config.ssl_public_key, proxy=None)
854+ fetch_async_result = Deferred()
855+ fetch_async_result.callback("hash-ids")
856+ self.mocker.result(fetch_async_result)
857+@@ -1621,6 +1648,44 @@
858+ reactor.callWhenRunning(do_test)
859+ return deferred
860+
861++ @mock.patch("landscape.package.reporter.spawn_process",
862++ return_value=succeed(("", "", 0)))
863++ def test_run_apt_update_honors_http_proxy(self, mock_spawn_process):
864++ """
865++ The PackageReporter.run_apt_update method honors the http_proxy
866++ config when calling the apt-update wrapper.
867++ """
868++ self.config.http_proxy = "http://proxy_server:8080"
869++ self.reporter.sources_list_filename = "/I/Dont/Exist"
870++
871++ update_result = self.reporter.run_apt_update()
872++ # run_apt_update uses reactor.call_later so advance a bit
873++ self.reactor.advance(0)
874++ self.successResultOf(update_result)
875++
876++ mock_spawn_process.assert_called_once_with(
877++ self.reporter.apt_update_filename,
878++ env={"http_proxy": "http://proxy_server:8080"})
879++
880++ @mock.patch("landscape.package.reporter.spawn_process",
881++ return_value=succeed(("", "", 0)))
882++ def test_run_apt_update_honors_https_proxy(self, mock_spawn_process):
883++ """
884++ The PackageReporter.run_apt_update method honors the https_proxy
885++ config when calling the apt-update wrapper.
886++ """
887++ self.config.https_proxy = "http://proxy_server:8443"
888++ self.reporter.sources_list_filename = "/I/Dont/Exist"
889++
890++ update_result = self.reporter.run_apt_update()
891++ # run_apt_update uses reactor.call_later, so advance a bit
892++ self.reactor.advance(0)
893++ self.successResultOf(update_result)
894++
895++ mock_spawn_process.assert_called_once_with(
896++ self.reporter.apt_update_filename,
897++ env={"https_proxy": "http://proxy_server:8443"})
898++
899+ def test_run_apt_update_error_on_cache_file(self):
900+ """
901+ L{PackageReporter.run_apt_update} succeeds if the command fails because
902diff --git a/debian/patches/series b/debian/patches/series
903index 687f449..aaf6d9f 100644
904--- a/debian/patches/series
905+++ b/debian/patches/series
906@@ -10,3 +10,7 @@ bug-1508110-revno-824.diff
907 bug-1532887-revno-825.diff
908 bug-1508110-revno-826.diff
909 bug-1508110-revno-829.diff
910+package-reporter-proxy-1531150.diff
911+iso-configuration-1699789.diff
912+autoremovable-report-1208393.diff
913+config-no-reregister-1618483.diff

Subscribers

People subscribed via source and target branches