MAAS

Merge ~bjornt/maas:refresh-credendtials-fix into maas:master

  1. Git
  2. lp:~bjornt/maas
  3. refresh-credendtials-fix
  4. Merge into master
Proposed by Björn Tillenius
Status: Merged
Approved by: Björn Tillenius
Approved revision: eeec4f9ba2b82fd61672e7f66497a94adce41bf1
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~bjornt/maas:refresh-credendtials-fix
Merge into: maas:master
Diff against target: 348 lines (+74/-137)
3 files modified
src/provisioningserver/refresh/__init__.py (+10/-6)
src/provisioningserver/refresh/tests/test_refresh.py (+52/-130)
src/provisioningserver/utils/tests/test_services.py (+12/-1)
Reviewer Review Type Date Requested Status
Adam Collard (community) Approve
MAAS Lander Approve
Review via email: mp+404681@code.launchpad.net

Commit message

Pass Credentials() rather than a dict in refresh().

maas_api_helpers was refactored not to use plain dicts anymore, but
refresh() wasn't updated to reflect the change.

No tests were failing, since they all mocked at a very high level. I
changed one test to mock at the urlopen level instead, which should
catch issues like this in the future.

I also removed some tests that now are redundant, and changed other
tests to check only what the test is focused on.

To post a comment you must log in.
Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b refresh-credendtials-fix lp:~bjornt/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10317/console
COMMIT: 556a6059a5ea92eea94bed38a0ae8193d9966fb0

review: Needs Fixing
Revision history for this message
Björn Tillenius (bjornt) wrote :

Putting it in work in progress until i've fixed all the test failures

Revision history for this message
Björn Tillenius (bjornt) wrote :

All tests should be passing now.

Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b refresh-credendtials-fix lp:~bjornt/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: eeec4f9ba2b82fd61672e7f66497a94adce41bf1

review: Approve
Revision history for this message
Adam Collard (adam-collard) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/provisioningserver/refresh/__init__.py b/src/provisioningserver/refresh/__init__.py
2index 8764eec..8931227 100644
3--- a/src/provisioningserver/refresh/__init__.py
4+++ b/src/provisioningserver/refresh/__init__.py
5@@ -13,6 +13,7 @@ from provisioningserver.config import is_dev_environment
6 from provisioningserver.logger import get_maas_logger
7 from provisioningserver.refresh.maas_api_helper import (
8 capture_script_output,
9+ Credentials,
10 MD_VERSION,
11 signal,
12 SignalException,
13@@ -75,12 +76,15 @@ def refresh(
14 maas_url = "http://127.0.0.1:5240/MAAS"
15 url = "%s/metadata/%s/" % (maas_url, MD_VERSION)
16
17- creds = {
18- "consumer_key": consumer_key,
19- "token_key": token_key,
20- "token_secret": token_secret,
21- "consumer_secret": "",
22- }
23+ creds = Credentials()
24+ creds.update(
25+ {
26+ "consumer_key": consumer_key,
27+ "token_key": token_key,
28+ "token_secret": token_secret,
29+ "consumer_secret": "",
30+ }
31+ )
32
33 scripts = {
34 name: config
35diff --git a/src/provisioningserver/refresh/tests/test_refresh.py b/src/provisioningserver/refresh/tests/test_refresh.py
36index 7f841f0..e5c6519 100644
37--- a/src/provisioningserver/refresh/tests/test_refresh.py
38+++ b/src/provisioningserver/refresh/tests/test_refresh.py
39@@ -18,6 +18,7 @@ from maastesting.factory import factory
40 from maastesting.matchers import MockAnyCall
41 from maastesting.testcase import MAASTestCase
42 from provisioningserver import refresh
43+from provisioningserver.refresh import maas_api_helper
44 from provisioningserver.refresh.maas_api_helper import (
45 MD_VERSION,
46 SignalException,
47@@ -47,6 +48,11 @@ class TestGetArchitecture(MAASTestCase):
48 mock_get_architectures.assert_not_called()
49
50
51+class FakeResponse:
52+
53+ status = 200
54+
55+
56 class TestRefresh(MAASTestCase):
57 def setUp(self):
58 super().setUp()
59@@ -56,6 +62,14 @@ class TestRefresh(MAASTestCase):
60 # by default, fake running in snap so sudo is not used
61 self.mock_running_in_snap = self.patch(refresh, "running_in_snap")
62 self.mock_running_in_snap.return_value = True
63+ self.urlopen_calls = []
64+ self.patch(
65+ maas_api_helper.urllib.request, "urlopen"
66+ ).side_effect = self._fake_urlopen
67+
68+ def _fake_urlopen(self, request, post_data=None):
69+ self.urlopen_calls.append((request, post_data))
70+ return FakeResponse()
71
72 def _cleanup(self, path):
73 if os.path.exists(path):
74@@ -108,7 +122,6 @@ class TestRefresh(MAASTestCase):
75 )
76
77 def test_refresh_accepts_defined_url(self):
78- signal = self.patch(refresh, "signal")
79 script_name = factory.make_name("script_name")
80 info_scripts = self.create_scripts_success(script_name)
81
82@@ -116,55 +129,34 @@ class TestRefresh(MAASTestCase):
83 consumer_key = factory.make_name("consumer_key")
84 token_key = factory.make_name("token_key")
85 token_secret = factory.make_name("token_secret")
86- url = factory.make_url()
87+ base_url = factory.make_simple_http_url()
88
89 with patch.dict(refresh.NODE_INFO_SCRIPTS, info_scripts, clear=True):
90 refresh.refresh(
91- system_id, consumer_key, token_key, token_secret, url
92+ system_id, consumer_key, token_key, token_secret, base_url
93 )
94- self.assertThat(
95- signal,
96- MockAnyCall(
97- "%s/metadata/%s/" % (url, MD_VERSION),
98- {
99- "consumer_secret": "",
100- "consumer_key": consumer_key,
101- "token_key": token_key,
102- "token_secret": token_secret,
103- },
104- "WORKING",
105- "Starting %s [1/1]" % script_name,
106- ),
107- )
108-
109- def test_refresh_signals_starting(self):
110- signal = self.patch(refresh, "signal")
111- script_name = factory.make_name("script_name")
112- info_scripts = self.create_scripts_success(script_name)
113+ requests = [call[0] for call in self.urlopen_calls]
114
115- system_id = factory.make_name("system_id")
116- consumer_key = factory.make_name("consumer_key")
117- token_key = factory.make_name("token_key")
118- token_secret = factory.make_name("token_secret")
119- url = factory.make_url()
120-
121- with patch.dict(refresh.NODE_INFO_SCRIPTS, info_scripts, clear=True):
122- refresh.refresh(
123- system_id, consumer_key, token_key, token_secret, url
124+ for request in requests:
125+ self.assertEqual(
126+ f"{base_url}/metadata/{MD_VERSION}/", request.full_url
127 )
128- self.assertThat(
129- signal,
130- MockAnyCall(
131- "%s/metadata/%s/" % (url, MD_VERSION),
132- {
133- "consumer_secret": "",
134- "consumer_key": consumer_key,
135- "token_key": token_key,
136- "token_secret": token_secret,
137- },
138- "WORKING",
139- "Starting %s [1/1]" % script_name,
140- ),
141+ auth_header = request.get_header("Authorization")
142+ self.assertTrue(
143+ auth_header.startswith("OAuth oauth_nonce="), auth_header
144+ )
145+ self.assertIn(f'oauth_consumer_key="{consumer_key}"', auth_header)
146+ self.assertIn(f'oauth_token="{token_key}"', auth_header)
147+
148+ self.assertIn(
149+ f"Starting {script_name}", requests[0].data.decode("ascii")
150+ )
151+ self.assertIn(
152+ f'filename="{script_name}"', requests[1].data.decode("ascii")
153+ )
154+ self.assertIn(
155+ f"Finished refreshing {system_id}",
156+ requests[2].data.decode("ascii"),
157 )
158
159 def test_refresh_signals_results(self):
160@@ -194,13 +186,8 @@ class TestRefresh(MAASTestCase):
161 self.assertThat(
162 signal,
163 MockAnyCall(
164- "%s/metadata/%s/" % (url, MD_VERSION),
165- {
166- "consumer_secret": "",
167- "consumer_key": consumer_key,
168- "token_key": token_key,
169- "token_secret": token_secret,
170- },
171+ ANY,
172+ ANY,
173 "WORKING",
174 files={
175 script_name: b"test script\n",
176@@ -261,13 +248,8 @@ class TestRefresh(MAASTestCase):
177 self.assertThat(
178 signal,
179 MockAnyCall(
180- "%s/metadata/%s/" % (url, MD_VERSION),
181- {
182- "consumer_secret": "",
183- "consumer_key": consumer_key,
184- "token_key": token_key,
185- "token_secret": token_secret,
186- },
187+ ANY,
188+ ANY,
189 "WORKING",
190 files={
191 script_name: b"[Errno 8] Exec format error",
192@@ -298,13 +280,8 @@ class TestRefresh(MAASTestCase):
193 self.assertThat(
194 signal,
195 MockAnyCall(
196- "%s/metadata/%s/" % (url, MD_VERSION),
197- {
198- "consumer_secret": "",
199- "consumer_key": consumer_key,
200- "token_key": token_key,
201- "token_secret": token_secret,
202- },
203+ ANY,
204+ ANY,
205 "WORKING",
206 files={
207 script_name: b"Unable to execute script",
208@@ -335,13 +312,8 @@ class TestRefresh(MAASTestCase):
209 self.assertThat(
210 signal,
211 MockAnyCall(
212- "%s/metadata/%s/" % (url, MD_VERSION),
213- {
214- "consumer_secret": "",
215- "consumer_key": consumer_key,
216- "token_key": token_key,
217- "token_secret": token_secret,
218- },
219+ ANY,
220+ ANY,
221 "WORKING",
222 files={
223 script_name: b"[Errno 0] Exec format error",
224@@ -383,13 +355,8 @@ class TestRefresh(MAASTestCase):
225 self.assertThat(
226 signal,
227 MockAnyCall(
228- "%s/metadata/%s/" % (url, MD_VERSION),
229- {
230- "consumer_secret": "",
231- "consumer_key": consumer_key,
232- "token_key": token_key,
233- "token_secret": token_secret,
234- },
235+ ANY,
236+ ANY,
237 "TIMEDOUT",
238 files={
239 script_name: b"",
240@@ -400,36 +367,6 @@ class TestRefresh(MAASTestCase):
241 ),
242 )
243
244- def test_refresh_signals_finished(self):
245- signal = self.patch(refresh, "signal")
246- script_name = factory.make_name("script_name")
247- info_scripts = self.create_scripts_success(script_name)
248-
249- system_id = factory.make_name("system_id")
250- consumer_key = factory.make_name("consumer_key")
251- token_key = factory.make_name("token_key")
252- token_secret = factory.make_name("token_secret")
253- url = factory.make_url()
254-
255- with patch.dict(refresh.NODE_INFO_SCRIPTS, info_scripts, clear=True):
256- refresh.refresh(
257- system_id, consumer_key, token_key, token_secret, url
258- )
259- self.assertThat(
260- signal,
261- MockAnyCall(
262- "%s/metadata/%s/" % (url, MD_VERSION),
263- {
264- "consumer_secret": "",
265- "consumer_key": consumer_key,
266- "token_key": token_key,
267- "token_secret": token_secret,
268- },
269- "OK",
270- "Finished refreshing %s" % system_id,
271- ),
272- )
273-
274 def test_refresh_signals_failure(self):
275 signal = self.patch(refresh, "signal")
276 info_scripts = self.create_scripts_failure()
277@@ -447,13 +384,8 @@ class TestRefresh(MAASTestCase):
278 self.assertThat(
279 signal,
280 MockAnyCall(
281- "%s/metadata/%s/" % (url, MD_VERSION),
282- {
283- "consumer_secret": "",
284- "consumer_key": consumer_key,
285- "token_key": token_key,
286- "token_secret": token_secret,
287- },
288+ ANY,
289+ ANY,
290 "FAILED",
291 "Failed refreshing %s" % system_id,
292 ),
293@@ -477,13 +409,8 @@ class TestRefresh(MAASTestCase):
294 self.assertThat(
295 signal,
296 MockAnyCall(
297- "%s/metadata/%s/" % (url, MD_VERSION),
298- {
299- "consumer_secret": "",
300- "consumer_key": consumer_key,
301- "token_key": token_key,
302- "token_secret": token_secret,
303- },
304+ ANY,
305+ ANY,
306 "WORKING",
307 "Starting %s [1/1]" % script_name,
308 ),
309@@ -509,13 +436,8 @@ class TestRefresh(MAASTestCase):
310 self.assertThat(
311 signal,
312 MockAnyCall(
313- "%s/metadata/%s/" % (url, MD_VERSION),
314- {
315- "consumer_secret": "",
316- "consumer_key": consumer_key,
317- "token_key": token_key,
318- "token_secret": token_secret,
319- },
320+ ANY,
321+ ANY,
322 "WORKING",
323 "Starting %s [1/1]" % script_name,
324 ),
325diff --git a/src/provisioningserver/utils/tests/test_services.py b/src/provisioningserver/utils/tests/test_services.py
326index f845567..ab4b5af 100644
327--- a/src/provisioningserver/utils/tests/test_services.py
328+++ b/src/provisioningserver/utils/tests/test_services.py
329@@ -154,7 +154,18 @@ class FakeRefresher:
330 def fake_signal(
331 self, url, creds, status, error=None, files=None, exit_status=None
332 ):
333- self.testcase.assertEqual(self.credentials, creds)
334+ self.testcase.assertEqual(
335+ self.credentials["token_key"], creds.token_key
336+ )
337+ self.testcase.assertEqual(
338+ self.credentials["token_secret"], creds.token_secret
339+ )
340+ self.testcase.assertEqual(
341+ self.credentials["consumer_key"], creds.consumer_key
342+ )
343+ self.testcase.assertEqual(
344+ self.credentials["consumer_secret"], creds.consumer_secret
345+ )
346 script_runs = self.script_runs[url]
347 if status == "WORKING":
348 if error.startswith("Starting"):

Subscribers

People subscribed via source and target branches