curtin

Merge ~mwhudson/curtin:revert-lp1892494 into curtin:master

  1. Git
  2. lp:~mwhudson/curtin
  3. revert-lp1892494
  4. Merge into master
Proposed by Michael Hudson-Doyle
Status: Merged
Approved by: Michael Hudson-Doyle
Approved revision: a5680f5a6e6548129721405174895752b3f76427
Merge reported by: Michael Hudson-Doyle
Merged at revision: a5680f5a6e6548129721405174895752b3f76427
Proposed branch: ~mwhudson/curtin:revert-lp1892494
Merge into: curtin:master
Diff against target: 286 lines (+43/-106)
2 files modified
curtin/commands/apt_config.py (+10/-14)
tests/unittests/test_apt_source.py (+33/-92)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Needs Fixing
Ryan Harper (community) Approve
Review via email: mp+396958@code.launchpad.net

Commit message

Revert "apt_config: stop using the deprecated apt-key command"

This reverts commit e099e32c5757b7aa0bc4fc2aeddb91d195a6df2b.

Unfortunately the gpg keys are not being added to the right place on
disk.

LP: #1912801

To post a comment you must log in.
Revision history for this message
Ryan Harper (raharper) wrote :

I'm +1 on this.

I suggest we follow up with a unittest/vmtest which does verify that we get keys in /etc/apt/trusted.gpg.d/ after apt-key add operations such that when a second try is pushed, it will be required to pass known working tests.

review: Approve
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:a5680f5a6e6548129721405174895752b3f76427
https://jenkins.ubuntu.com/server/job/curtin-ci/92/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/92/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/92/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/92/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/92/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/92//rebuild

review: Approve (continuous-integration)
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Yeah, that makes sense.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

Autolanding: FAILED
More details in the following jenkins job:
https://jenkins.ubuntu.com/server/job/curtin-autoland-test/56/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-amd64/56/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-arm64/56/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-ppc64el/56/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-s390x/56/

review: Needs Fixing (continuous-integration)
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

pylint flakiness, merging by hand

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

Autolanding: FAILED
More details in the following jenkins job:
https://jenkins.ubuntu.com/server/job/curtin-autoland-test/57/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-amd64/57/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-arm64/57/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-ppc64el/57/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-s390x/57/

review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/curtin/commands/apt_config.py b/curtin/commands/apt_config.py
2index ff906be..e7d84c0 100644
3--- a/curtin/commands/apt_config.py
4+++ b/curtin/commands/apt_config.py
5@@ -343,24 +343,20 @@ def apply_preserve_sources_list(target):
6 raise
7
8
9-def add_apt_key_raw(filename, key, target=None):
10+def add_apt_key_raw(key, target=None):
11 """
12 actual adding of a key as defined in key argument
13 to the system
14 """
15- if '-----BEGIN PGP PUBLIC KEY BLOCK-----' in str(key):
16- target_keyfile_ext = '.asc'
17- omode = 'w'
18- key = key.rstrip()
19- else:
20- target_keyfile_ext = '.gpg'
21- omode = 'wb'
22- target_keyfile = paths.target_path(target, filename + target_keyfile_ext)
23- util.write_file(target_keyfile, key, mode=0o644, omode=omode)
24- LOG.debug("Adding key to '%s':\n'%s'", target_keyfile, key)
25+ LOG.debug("Adding key:\n'%s'", key)
26+ try:
27+ util.subp(['apt-key', 'add', '-'], data=key.encode(), target=target)
28+ except util.ProcessExecutionError:
29+ LOG.exception("failed to add apt GPG Key to apt keyring")
30+ raise
31
32
33-def add_apt_key(filename, ent, target=None):
34+def add_apt_key(ent, target=None):
35 """
36 Add key to the system as defined in ent (if any).
37 Supports raw keys or keyid's
38@@ -375,7 +371,7 @@ def add_apt_key(filename, ent, target=None):
39 retries=(1, 2, 5, 10))
40
41 if 'key' in ent:
42- add_apt_key_raw(filename, ent['key'], target)
43+ add_apt_key_raw(ent['key'], target)
44
45
46 def add_apt_sources(srcdict, target=None, template_params=None,
47@@ -399,7 +395,7 @@ def add_apt_sources(srcdict, target=None, template_params=None,
48 if 'filename' not in ent:
49 ent['filename'] = filename
50
51- add_apt_key(ent['filename'], ent, target)
52+ add_apt_key(ent, target)
53
54 if 'source' not in ent:
55 continue
56diff --git a/tests/data/test.gpg b/tests/data/test.gpg
57deleted file mode 100644
58index f7ba778..0000000
59Binary files a/tests/data/test.gpg and /dev/null differ
60diff --git a/tests/unittests/test_apt_source.py b/tests/unittests/test_apt_source.py
61index f8aac5a..6556399 100644
62--- a/tests/unittests/test_apt_source.py
63+++ b/tests/unittests/test_apt_source.py
64@@ -33,32 +33,17 @@ S0ORP6HXET3+jC8BMG4tBWCTK/XEZw==
65 =ACB2
66 -----END PGP PUBLIC KEY BLOCK-----"""
67
68-EXPECTEDKEY_NOVER = u"""-----BEGIN PGP PUBLIC KEY BLOCK-----
69-
70-mI0ESuZLUgEEAKkqq3idtFP7g9hzOu1a8+v8ImawQN4TrvlygfScMU1TIS1eC7UQ
71-NUA8Qqgr9iUaGnejb0VciqftLrU9D6WYHSKz+EITefgdyJ6SoQxjoJdsCpJ7o9Jy
72-8PQnpRttiFm4qHu6BVnKnBNxw/z3ST9YMqW5kbMQpfxbGe+obRox59NpABEBAAG0
73-HUxhdW5jaHBhZCBQUEEgZm9yIFNjb3R0IE1vc2VyiLYEEwECACAFAkrmS1ICGwMG
74-CwkIBwMCBBUCCAMEFgIDAQIeAQIXgAAKCRAGILvPA2g/d3aEA/9tVjc10HOZwV29
75-OatVuTeERjjrIbxflO586GLA8cp0C9RQCwgod/R+cKYdQcHjbqVcP0HqxveLg0RZ
76-FJpWLmWKamwkABErwQLGlM/Hwhjfade8VvEQutH5/0JgKHmzRsoqfR+LMO6OS+Sm
77-S0ORP6HXET3+jC8BMG4tBWCTK/XEZw==
78-=ACB2
79------END PGP PUBLIC KEY BLOCK-----"""
80-
81-EXPECTED_BINKEY = util.load_file("tests/data/test.gpg", decode=False)
82-
83 ADD_APT_REPO_MATCH = r"^[\w-]+:\w"
84
85 TARGET = "/"
86
87
88-def load_tfile(filename, decode=True):
89+def load_tfile(filename):
90 """ load_tfile
91 load file and return content after decoding
92 """
93 try:
94- content = util.load_file(filename, decode=decode)
95+ content = util.load_file(filename, decode=True)
96 except Exception as error:
97 print('failed to load file content for test: %s' % error)
98 raise
99@@ -90,6 +75,8 @@ class TestAptSourceConfig(CiTestCase):
100 self.aptlistfile3 = os.path.join(self.tmp, "single-deb3.list")
101 self.join = os.path.join
102 self.matcher = re.compile(ADD_APT_REPO_MATCH).search
103+ self.add_patch('curtin.util.subp', 'm_subp')
104+ self.m_subp.return_value = ('s390x', '')
105
106 @staticmethod
107 def _add_apt_sources(*args, **kwargs):
108@@ -228,48 +215,23 @@ class TestAptSourceConfig(CiTestCase):
109 self.aptlistfile3: {'source': 'deb $MIRROR $RELEASE universe'}}
110 self._apt_src_replace_tri(cfg)
111
112- def _apt_src_keyid_txt(self, filename, cfg):
113+ def _apt_src_keyid(self, filename, cfg, keynum):
114 """ _apt_src_keyid
115 Test specification of a source + keyid
116 """
117 params = self._get_default_params()
118
119- with mock.patch.object(gpg, 'getkeybyid',
120- return_value=EXPECTEDKEY_NOVER):
121- self._add_apt_sources(cfg, TARGET, template_params=params,
122- aa_repo_match=self.matcher)
123-
124- for ent in cfg:
125- key_filename = cfg[ent].get('filename', ent)
126- self.assertTrue(os.path.isfile(key_filename + '.asc'))
127- contents = load_tfile(key_filename + '.asc')
128- self.assertMultiLineEqual(EXPECTEDKEY_NOVER, contents)
129-
130- def _apt_src_keyid_bin(self, filename, cfg):
131- """ _apt_src_keyid
132- Test specification of a source + keyid
133- """
134- params = self._get_default_params()
135-
136- with mock.patch.object(gpg, 'getkeybyid',
137- return_value=EXPECTED_BINKEY):
138+ with mock.patch("curtin.util.subp",
139+ return_value=('fakekey 1234', '')) as mockobj:
140 self._add_apt_sources(cfg, TARGET, template_params=params,
141 aa_repo_match=self.matcher)
142
143- for ent in cfg:
144- key_filename = cfg[ent].get('filename', ent)
145- self.assertTrue(os.path.isfile(key_filename + '.gpg'))
146- contents = load_tfile(key_filename + '.gpg', decode=False)
147- self.assertEqual(EXPECTED_BINKEY, contents)
148-
149- def _apt_src_keyid(self, filename, cfg, key_type="txt"):
150- """ _apt_src_keyid
151- Test specification of a source + keyid
152- """
153- if key_type == "txt":
154- self._apt_src_keyid_txt(filename, cfg)
155- else:
156- self._apt_src_keyid_bin(filename, cfg)
157+ # check if it added the right ammount of keys
158+ calls = []
159+ for _ in range(keynum):
160+ calls.append(call(['apt-key', 'add', '-'], data=b'fakekey 1234',
161+ target=TARGET))
162+ mockobj.assert_has_calls(calls, any_order=True)
163
164 self.assertTrue(os.path.isfile(filename))
165
166@@ -289,17 +251,7 @@ class TestAptSourceConfig(CiTestCase):
167 'smoser/cloud-init-test/ubuntu'
168 ' xenial main'),
169 'keyid': "03683F77"}}
170- self._apt_src_keyid(self.aptlistfile, cfg)
171-
172- @mock.patch(ChrootableTargetStr, new=PseudoChrootableTarget)
173- def test_apt_src_keyid_bin(self):
174- """test_apt_src_keyid - Test source + keyid with filename being set"""
175- cfg = {self.aptlistfile: {'source': ('deb '
176- 'http://ppa.launchpad.net/'
177- 'smoser/cloud-init-test/ubuntu'
178- ' xenial main'),
179- 'keyid': "03683F77"}}
180- self._apt_src_keyid(self.aptlistfile, cfg, key_type='bin')
181+ self._apt_src_keyid(self.aptlistfile, cfg, 1)
182
183 @mock.patch(ChrootableTargetStr, new=PseudoChrootableTarget)
184 def test_apt_src_keyid_tri(self):
185@@ -321,7 +273,7 @@ class TestAptSourceConfig(CiTestCase):
186 ' xenial multiverse'),
187 'keyid': "03683F77"}}
188
189- self._apt_src_keyid(self.aptlistfile, cfg)
190+ self._apt_src_keyid(self.aptlistfile, cfg, 3)
191 contents = load_tfile(self.aptlistfile2)
192 self.assertTrue(re.search(r"%s %s %s %s\n" %
193 ("deb",
194@@ -341,23 +293,18 @@ class TestAptSourceConfig(CiTestCase):
195 def test_apt_src_key(self):
196 """test_apt_src_key - Test source + key"""
197 params = self._get_default_params()
198- fake_key = u"""-----BEGIN PGP PUBLIC KEY BLOCK-----
199- fakekey 4321"""
200-
201 cfg = {self.aptlistfile: {'source': ('deb '
202 'http://ppa.launchpad.net/'
203 'smoser/cloud-init-test/ubuntu'
204 ' xenial main'),
205- 'key': fake_key}}
206-
207- self._add_apt_sources(cfg, TARGET, template_params=params,
208- aa_repo_match=self.matcher)
209+ 'key': "fakekey 4321"}}
210
211- key_filename = self.aptlistfile + '.asc'
212- self.assertTrue(os.path.isfile(key_filename))
213+ with mock.patch.object(util, 'subp') as mockobj:
214+ self._add_apt_sources(cfg, TARGET, template_params=params,
215+ aa_repo_match=self.matcher)
216
217- contents = load_tfile(key_filename)
218- self.assertMultiLineEqual(fake_key, contents)
219+ mockobj.assert_any_call(['apt-key', 'add', '-'], data=b'fakekey 4321',
220+ target=TARGET)
221
222 self.assertTrue(os.path.isfile(self.aptlistfile))
223
224@@ -373,18 +320,14 @@ class TestAptSourceConfig(CiTestCase):
225 def test_apt_src_keyonly(self):
226 """test_apt_src_keyonly - Test key without source"""
227 params = self._get_default_params()
228- fake_key = u"""-----BEGIN PGP PUBLIC KEY BLOCK-----
229- fakekey 4321"""
230- cfg = {self.aptlistfile: {'key': fake_key}}
231+ cfg = {self.aptlistfile: {'key': "fakekey 4242"}}
232
233- self._add_apt_sources(cfg, TARGET, template_params=params,
234- aa_repo_match=self.matcher)
235-
236- key_filename = self.aptlistfile + '.asc'
237- self.assertTrue(os.path.isfile(key_filename))
238+ with mock.patch.object(util, 'subp') as mockobj:
239+ self._add_apt_sources(cfg, TARGET, template_params=params,
240+ aa_repo_match=self.matcher)
241
242- contents = load_tfile(key_filename)
243- self.assertMultiLineEqual(fake_key, contents)
244+ mockobj.assert_any_call(['apt-key', 'add', '-'], data=b'fakekey 4242',
245+ target=TARGET)
246
247 # filename should be ignored on key only
248 self.assertFalse(os.path.isfile(self.aptlistfile))
249@@ -395,15 +338,13 @@ class TestAptSourceConfig(CiTestCase):
250 params = self._get_default_params()
251 cfg = {self.aptlistfile: {'keyid': "03683F77"}}
252
253- with mock.patch.object(gpg, 'getkeybyid',
254- return_value=EXPECTEDKEY_NOVER):
255+ with mock.patch.object(util, 'subp',
256+ return_value=('fakekey 1212', '')) as mockobj:
257 self._add_apt_sources(cfg, TARGET, template_params=params,
258 aa_repo_match=self.matcher)
259
260- key_filename = self.aptlistfile
261- self.assertTrue(os.path.isfile(key_filename + '.asc'))
262- contents = load_tfile(key_filename + '.asc')
263- self.assertMultiLineEqual(EXPECTEDKEY_NOVER, contents)
264+ mockobj.assert_any_call(['apt-key', 'add', '-'], data=b'fakekey 1212',
265+ target=TARGET)
266
267 # filename should be ignored on key only
268 self.assertFalse(os.path.isfile(self.aptlistfile))
269@@ -427,7 +368,7 @@ class TestAptSourceConfig(CiTestCase):
270 keycfg.get('keyserver',
271 'keyserver.ubuntu.com'),
272 retries=(1, 2, 5, 10))
273- mockkey.assert_called_with(self.aptlistfile, expectedkey, TARGET)
274+ mockkey.assert_called_with(expectedkey, TARGET)
275
276 # filename should be ignored on key only
277 self.assertFalse(os.path.isfile(self.aptlistfile))
278@@ -471,7 +412,7 @@ class TestAptSourceConfig(CiTestCase):
279
280 mockgetkey.assert_called_with('03683F77', 'test.random.com',
281 retries=(1, 2, 5, 10))
282- mockadd.assert_called_with(self.aptlistfile, 'fakekey', TARGET)
283+ mockadd.assert_called_with('fakekey', TARGET)
284
285 # filename should be ignored on key only
286 self.assertFalse(os.path.isfile(self.aptlistfile))

Subscribers

People subscribed via source and target branches