Merge ~alexsander-souza/maas:lp1966474_ignore_unused_creds into maas:master
- Git
- lp:~alexsander-souza/maas
- lp1966474_ignore_unused_creds
- Merge into master
Status: | Merged |
---|---|
Approved by: | Alexsander de Souza |
Approved revision: | e804ab39588d1c1a76477ab2c6747cb7ade62e0f |
Merge reported by: | MAAS Lander |
Merged at revision: | not available |
Proposed branch: | ~alexsander-souza/maas:lp1966474_ignore_unused_creds |
Merge into: | maas:master |
Diff against target: |
289 lines (+149/-8) 7 files modified
src/maasserver/models/node.py (+5/-0) src/maasserver/models/nodemetadata.py (+18/-0) src/maasserver/models/tests/test_nodemetadata.py (+21/-0) src/metadataserver/api_twisted.py (+7/-4) src/metadataserver/tests/test_api_twisted.py (+66/-0) src/metadataserver/tests/test_vendor_data.py (+26/-0) src/metadataserver/vendor_data.py (+6/-4) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Alberto Donato (community) | Approve | ||
MAAS Lander | Approve | ||
Review via email: mp+418116@code.launchpad.net |
Commit message
Ignore unused creds when deploying a KVM host
fixes LP#1966474
Description of the change
Alberto Donato (ack) wrote : | # |
The change itself looks good as a fallback, but _create_
It'd be good to understand why this is not happening, which is the real cause of the bug.
Alberto Donato (ack) wrote : | # |
Actually, I guess the reason is that if the deployment doesn't complete, the secrets are leftover.
I think the right fix would be to
- delete all secrets when a machine is released
- delete all secrets (before creating new ones) when the machine is deployed
- 7b95791... by Alexsander de Souza
-
delete credentials on node release
MAAS Lander (maas-lander) wrote : | # |
UNIT TESTS
-b lp1966474_
STATUS: SUCCESS
COMMIT: 65d4d4cb9799d20
- 467fb58... by Alexsander de Souza
-
remove leftover credentials
Alexsander de Souza (alexsander-souza) wrote : | # |
Updated with the suggested fixes
MAAS Lander (maas-lander) wrote : | # |
UNIT TESTS
-b lp1966474_
STATUS: FAILED
LOG: http://
COMMIT: c9654cc12c9ea8d
MAAS Lander (maas-lander) wrote : | # |
UNIT TESTS
-b lp1966474_
STATUS: SUCCESS
COMMIT: fead4ad066dddfe
Alberto Donato (ack) wrote : | # |
A few additional comments
- 54b55de... by Alexsander de Souza
-
fixes
- 93037f9... by Alexsander de Souza
-
revert unrequired change
MAAS Lander (maas-lander) wrote : | # |
UNIT TESTS
-b lp1966474_
STATUS: FAILED
LOG: http://
COMMIT: 04f963359a11170
MAAS Lander (maas-lander) wrote : | # |
UNIT TESTS
-b lp1966474_
STATUS: FAILED
LOG: http://
COMMIT: 93037f922ef22e2
- e804ab3... by Alexsander de Souza
-
fix test
MAAS Lander (maas-lander) wrote : | # |
UNIT TESTS
-b lp1966474_
STATUS: SUCCESS
COMMIT: e804ab39588d1c1
Alberto Donato (ack) wrote : | # |
Thanks for the changes, looks good! +1
Preview Diff
1 | diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py | |||
2 | index 5740b70..cdcedfa 100644 | |||
3 | --- a/src/maasserver/models/node.py | |||
4 | +++ b/src/maasserver/models/node.py | |||
5 | @@ -3718,6 +3718,11 @@ class Node(CleanSave, TimestampedModel): | |||
6 | 3718 | # If this node has non-installable children, remove them. | 3718 | # If this node has non-installable children, remove them. |
7 | 3719 | self.children.all().delete() | 3719 | self.children.all().delete() |
8 | 3720 | 3720 | ||
9 | 3721 | # Release volatile metadata | ||
10 | 3722 | from maasserver.models import NodeMetadata | ||
11 | 3723 | |||
12 | 3724 | NodeMetadata.objects.release_volatile(self) | ||
13 | 3725 | |||
14 | 3721 | # Power was off or cannot be powered off so release to ready now. | 3726 | # Power was off or cannot be powered off so release to ready now. |
15 | 3722 | if finalize_release: | 3727 | if finalize_release: |
16 | 3723 | self._finalize_release() | 3728 | self._finalize_release() |
17 | diff --git a/src/maasserver/models/nodemetadata.py b/src/maasserver/models/nodemetadata.py | |||
18 | index 0daa3d0..8ad361b 100644 | |||
19 | --- a/src/maasserver/models/nodemetadata.py | |||
20 | +++ b/src/maasserver/models/nodemetadata.py | |||
21 | @@ -22,6 +22,24 @@ class NodeMetadataManager(Manager): | |||
22 | 22 | except NodeMetadata.DoesNotExist: | 22 | except NodeMetadata.DoesNotExist: |
23 | 23 | return default | 23 | return default |
24 | 24 | 24 | ||
25 | 25 | def release_volatile(cls, node): | ||
26 | 26 | """Remove volatile information. | ||
27 | 27 | |||
28 | 28 | Should be called when releasing the node to remove all data that | ||
29 | 29 | is related to this deployment. | ||
30 | 30 | """ | ||
31 | 31 | from metadataserver import vendor_data | ||
32 | 32 | |||
33 | 33 | volatile_meta = ( | ||
34 | 34 | vendor_data.LXD_CERTIFICATE_METADATA_KEY, | ||
35 | 35 | vendor_data.VIRSH_PASSWORD_METADATA_KEY, | ||
36 | 36 | ) | ||
37 | 37 | |||
38 | 38 | NodeMetadata.objects.filter( | ||
39 | 39 | node=node, | ||
40 | 40 | key__in=volatile_meta, | ||
41 | 41 | ).delete() | ||
42 | 42 | |||
43 | 25 | 43 | ||
44 | 26 | class NodeMetadata(CleanSave, TimestampedModel): | 44 | class NodeMetadata(CleanSave, TimestampedModel): |
45 | 27 | """A `NodeMetadata` represents a key/value storage for Node metadata. | 45 | """A `NodeMetadata` represents a key/value storage for Node metadata. |
46 | diff --git a/src/maasserver/models/tests/test_nodemetadata.py b/src/maasserver/models/tests/test_nodemetadata.py | |||
47 | index 9a19956..6e979d9 100644 | |||
48 | --- a/src/maasserver/models/tests/test_nodemetadata.py | |||
49 | +++ b/src/maasserver/models/tests/test_nodemetadata.py | |||
50 | @@ -9,6 +9,7 @@ from django.core.exceptions import ValidationError | |||
51 | 9 | from maasserver.models import NodeMetadata | 9 | from maasserver.models import NodeMetadata |
52 | 10 | from maasserver.testing.factory import factory | 10 | from maasserver.testing.factory import factory |
53 | 11 | from maasserver.testing.testcase import MAASServerTestCase | 11 | from maasserver.testing.testcase import MAASServerTestCase |
54 | 12 | from maasserver.utils.orm import reload_object | ||
55 | 12 | from maastesting.crochet import wait_for | 13 | from maastesting.crochet import wait_for |
56 | 13 | 14 | ||
57 | 14 | wait_for_reactor = wait_for() | 15 | wait_for_reactor = wait_for() |
58 | @@ -50,3 +51,23 @@ class TestNodeMetadata(MAASServerTestCase): | |||
59 | 50 | default, | 51 | default, |
60 | 51 | NodeMetadata.objects.get(node=node, key=key, default=default), | 52 | NodeMetadata.objects.get(node=node, key=key, default=default), |
61 | 52 | ) | 53 | ) |
62 | 54 | |||
63 | 55 | def test_release_volatile(self): | ||
64 | 56 | from metadataserver.vendor_data import ( | ||
65 | 57 | LXD_CERTIFICATE_METADATA_KEY, | ||
66 | 58 | VIRSH_PASSWORD_METADATA_KEY, | ||
67 | 59 | ) | ||
68 | 60 | |||
69 | 61 | node = factory.make_Node() | ||
70 | 62 | dummy = factory.make_NodeMetadata(node=node) | ||
71 | 63 | cred_virsh = factory.make_NodeMetadata( | ||
72 | 64 | key=VIRSH_PASSWORD_METADATA_KEY, node=node | ||
73 | 65 | ) | ||
74 | 66 | cred_lxd = factory.make_NodeMetadata( | ||
75 | 67 | key=LXD_CERTIFICATE_METADATA_KEY, node=node | ||
76 | 68 | ) | ||
77 | 69 | |||
78 | 70 | NodeMetadata.objects.release_volatile(node) | ||
79 | 71 | self.assertIsNotNone(reload_object(dummy)) | ||
80 | 72 | self.assertIsNone(reload_object(cred_virsh)) | ||
81 | 73 | self.assertIsNone(reload_object(cred_lxd)) | ||
82 | diff --git a/src/metadataserver/api_twisted.py b/src/metadataserver/api_twisted.py | |||
83 | index 6460184..77cac8c 100644 | |||
84 | --- a/src/metadataserver/api_twisted.py | |||
85 | +++ b/src/metadataserver/api_twisted.py | |||
86 | @@ -184,13 +184,16 @@ POD_CREATION_ERROR = ( | |||
87 | 184 | 184 | ||
88 | 185 | 185 | ||
89 | 186 | def _create_vmhost_for_deployment(node): | 186 | def _create_vmhost_for_deployment(node): |
90 | 187 | cred_types = set() | ||
91 | 188 | if node.register_vmhost: | ||
92 | 189 | cred_types.add(LXD_CERTIFICATE_METADATA_KEY) | ||
93 | 190 | if node.install_kvm: | ||
94 | 191 | cred_types.add(VIRSH_PASSWORD_METADATA_KEY) | ||
95 | 192 | |||
96 | 187 | creds_meta = list( | 193 | creds_meta = list( |
97 | 188 | NodeMetadata.objects.filter( | 194 | NodeMetadata.objects.filter( |
98 | 189 | node=node, | 195 | node=node, |
103 | 190 | key__in=( | 196 | key__in=cred_types, |
100 | 191 | LXD_CERTIFICATE_METADATA_KEY, | ||
101 | 192 | VIRSH_PASSWORD_METADATA_KEY, | ||
102 | 193 | ), | ||
104 | 194 | ) | 197 | ) |
105 | 195 | ) | 198 | ) |
106 | 196 | if not creds_meta: | 199 | if not creds_meta: |
107 | diff --git a/src/metadataserver/tests/test_api_twisted.py b/src/metadataserver/tests/test_api_twisted.py | |||
108 | index a58e200..20332fd 100644 | |||
109 | --- a/src/metadataserver/tests/test_api_twisted.py | |||
110 | +++ b/src/metadataserver/tests/test_api_twisted.py | |||
111 | @@ -1263,3 +1263,69 @@ class TestCreateVMHostForDeployment(MAASServerTestCase): | |||
112 | 1263 | node=node, key="virsh_password", value="xyz123" | 1263 | node=node, key="virsh_password", value="xyz123" |
113 | 1264 | ) | 1264 | ) |
114 | 1265 | self.assertRaises(DatabaseError, _create_vmhost_for_deployment, node) | 1265 | self.assertRaises(DatabaseError, _create_vmhost_for_deployment, node) |
115 | 1266 | |||
116 | 1267 | def test_ignore_unused_lxd_creds(self): | ||
117 | 1268 | user = factory.make_User() | ||
118 | 1269 | node = factory.make_Node_with_Interface_on_Subnet( | ||
119 | 1270 | owner=user, | ||
120 | 1271 | status=NODE_STATUS.DEPLOYING, | ||
121 | 1272 | agent_name="maas-kvm-pod", | ||
122 | 1273 | install_kvm=True, | ||
123 | 1274 | ) | ||
124 | 1275 | ip = factory.make_StaticIPAddress(interface=node.boot_interface) | ||
125 | 1276 | virsh_password_meta = NodeMetadata.objects.create( | ||
126 | 1277 | node=node, key="virsh_password", value="xyz123" | ||
127 | 1278 | ) | ||
128 | 1279 | cert = generate_certificate("maas") | ||
129 | 1280 | lxd_cert_meta = NodeMetadata.objects.create( | ||
130 | 1281 | node=node, | ||
131 | 1282 | key="lxd_certificate", | ||
132 | 1283 | value=cert.certificate_pem() + cert.private_key_pem(), | ||
133 | 1284 | ) | ||
134 | 1285 | addr = ip.ip | ||
135 | 1286 | if IPAddress(addr).version == 6: | ||
136 | 1287 | addr = f"[{addr}]" | ||
137 | 1288 | _create_vmhost_for_deployment(node) | ||
138 | 1289 | virsh_vmhost = Pod.objects.get(power_type="virsh") | ||
139 | 1290 | self.assertEqual(node.status, NODE_STATUS.DEPLOYED) | ||
140 | 1291 | self.mock_discover_and_sync.assert_has_calls( | ||
141 | 1292 | [ | ||
142 | 1293 | call(virsh_vmhost, user), | ||
143 | 1294 | ], | ||
144 | 1295 | any_order=True, | ||
145 | 1296 | ) | ||
146 | 1297 | self.assertIsNone(reload_object(virsh_password_meta)) | ||
147 | 1298 | self.assertIsNotNone(reload_object(lxd_cert_meta)) | ||
148 | 1299 | |||
149 | 1300 | def test_ignore_unused_virsh_creds(self): | ||
150 | 1301 | user = factory.make_User() | ||
151 | 1302 | node = factory.make_Node_with_Interface_on_Subnet( | ||
152 | 1303 | owner=user, | ||
153 | 1304 | status=NODE_STATUS.DEPLOYING, | ||
154 | 1305 | agent_name="maas-kvm-pod", | ||
155 | 1306 | register_vmhost=True, | ||
156 | 1307 | ) | ||
157 | 1308 | ip = factory.make_StaticIPAddress(interface=node.boot_interface) | ||
158 | 1309 | virsh_password_meta = NodeMetadata.objects.create( | ||
159 | 1310 | node=node, key="virsh_password", value="xyz123" | ||
160 | 1311 | ) | ||
161 | 1312 | cert = generate_certificate("maas") | ||
162 | 1313 | lxd_cert_meta = NodeMetadata.objects.create( | ||
163 | 1314 | node=node, | ||
164 | 1315 | key="lxd_certificate", | ||
165 | 1316 | value=cert.certificate_pem() + cert.private_key_pem(), | ||
166 | 1317 | ) | ||
167 | 1318 | addr = ip.ip | ||
168 | 1319 | if IPAddress(addr).version == 6: | ||
169 | 1320 | addr = f"[{addr}]" | ||
170 | 1321 | _create_vmhost_for_deployment(node) | ||
171 | 1322 | lxd_vmhost = Pod.objects.get(power_type="lxd") | ||
172 | 1323 | self.assertEqual(node.status, NODE_STATUS.DEPLOYED) | ||
173 | 1324 | self.mock_discover_and_sync.assert_has_calls( | ||
174 | 1325 | [ | ||
175 | 1326 | call(lxd_vmhost, user), | ||
176 | 1327 | ], | ||
177 | 1328 | any_order=True, | ||
178 | 1329 | ) | ||
179 | 1330 | self.assertIsNotNone(reload_object(virsh_password_meta)) | ||
180 | 1331 | self.assertIsNone(reload_object(lxd_cert_meta)) | ||
181 | diff --git a/src/metadataserver/tests/test_vendor_data.py b/src/metadataserver/tests/test_vendor_data.py | |||
182 | index 6f625a2..a2044a7 100644 | |||
183 | --- a/src/metadataserver/tests/test_vendor_data.py | |||
184 | +++ b/src/metadataserver/tests/test_vendor_data.py | |||
185 | @@ -26,6 +26,7 @@ from maasserver.testing.factory import factory | |||
186 | 26 | from maasserver.testing.fixtures import RBACEnabled | 26 | from maasserver.testing.fixtures import RBACEnabled |
187 | 27 | from maasserver.testing.testcase import MAASServerTestCase | 27 | from maasserver.testing.testcase import MAASServerTestCase |
188 | 28 | from maasserver.utils.converters import systemd_interval_to_calendar | 28 | from maasserver.utils.converters import systemd_interval_to_calendar |
189 | 29 | from maasserver.utils.orm import reload_object | ||
190 | 29 | from maastesting.matchers import MockNotCalled | 30 | from maastesting.matchers import MockNotCalled |
191 | 30 | from metadataserver import vendor_data | 31 | from metadataserver import vendor_data |
192 | 31 | from metadataserver.vendor_data import ( | 32 | from metadataserver.vendor_data import ( |
193 | @@ -45,6 +46,8 @@ from metadataserver.vendor_data import ( | |||
194 | 45 | HARDWARE_SYNC_MACHINE_TOKEN_PATH, | 46 | HARDWARE_SYNC_MACHINE_TOKEN_PATH, |
195 | 46 | HARDWARE_SYNC_SERVICE_TEMPLATE, | 47 | HARDWARE_SYNC_SERVICE_TEMPLATE, |
196 | 47 | HARDWARE_SYNC_TIMER_TEMPLATE, | 48 | HARDWARE_SYNC_TIMER_TEMPLATE, |
197 | 49 | LXD_CERTIFICATE_METADATA_KEY, | ||
198 | 50 | VIRSH_PASSWORD_METADATA_KEY, | ||
199 | 48 | ) | 51 | ) |
200 | 49 | from provisioningserver.drivers.pod.lxd import LXD_MAAS_PROJECT_CONFIG | 52 | from provisioningserver.drivers.pod.lxd import LXD_MAAS_PROJECT_CONFIG |
201 | 50 | 53 | ||
202 | @@ -335,6 +338,17 @@ class TestGenerateKVMPodConfiguration(MAASServerTestCase): | |||
203 | 335 | netboot=False, | 338 | netboot=False, |
204 | 336 | install_kvm=True, | 339 | install_kvm=True, |
205 | 337 | ) | 340 | ) |
206 | 341 | factory.make_NodeMetadata( | ||
207 | 342 | key=VIRSH_PASSWORD_METADATA_KEY, | ||
208 | 343 | node=node, | ||
209 | 344 | value="old value", | ||
210 | 345 | ) | ||
211 | 346 | cred_lxd = factory.make_NodeMetadata( | ||
212 | 347 | key=LXD_CERTIFICATE_METADATA_KEY, | ||
213 | 348 | node=node, | ||
214 | 349 | value="old value", | ||
215 | 350 | ) | ||
216 | 351 | |||
217 | 338 | config = list(generate_kvm_pod_configuration(node)) | 352 | config = list(generate_kvm_pod_configuration(node)) |
218 | 339 | self.assertEqual( | 353 | self.assertEqual( |
219 | 340 | config, | 354 | config, |
220 | @@ -389,6 +403,7 @@ class TestGenerateKVMPodConfiguration(MAASServerTestCase): | |||
221 | 389 | password_meta = NodeMetadata.objects.first() | 403 | password_meta = NodeMetadata.objects.first() |
222 | 390 | self.assertEqual(password_meta.key, "virsh_password") | 404 | self.assertEqual(password_meta.key, "virsh_password") |
223 | 391 | self.assertEqual(password_meta.value, password) | 405 | self.assertEqual(password_meta.value, password) |
224 | 406 | self.assertIsNone(reload_object(cred_lxd)) | ||
225 | 392 | 407 | ||
226 | 393 | def test_yields_configuration_when_machine_register_vmhost_true(self): | 408 | def test_yields_configuration_when_machine_register_vmhost_true(self): |
227 | 394 | cert = vendor_data.generate_certificate("maas") | 409 | cert = vendor_data.generate_certificate("maas") |
228 | @@ -399,6 +414,16 @@ class TestGenerateKVMPodConfiguration(MAASServerTestCase): | |||
229 | 399 | netboot=False, | 414 | netboot=False, |
230 | 400 | register_vmhost=True, | 415 | register_vmhost=True, |
231 | 401 | ) | 416 | ) |
232 | 417 | cred_virsh = factory.make_NodeMetadata( | ||
233 | 418 | key=VIRSH_PASSWORD_METADATA_KEY, | ||
234 | 419 | node=node, | ||
235 | 420 | value="old value", | ||
236 | 421 | ) | ||
237 | 422 | factory.make_NodeMetadata( | ||
238 | 423 | key=LXD_CERTIFICATE_METADATA_KEY, | ||
239 | 424 | node=node, | ||
240 | 425 | value="old value", | ||
241 | 426 | ) | ||
242 | 402 | config = list(generate_kvm_pod_configuration(node)) | 427 | config = list(generate_kvm_pod_configuration(node)) |
243 | 403 | self.assertEqual( | 428 | self.assertEqual( |
244 | 404 | config, | 429 | config, |
245 | @@ -436,6 +461,7 @@ class TestGenerateKVMPodConfiguration(MAASServerTestCase): | |||
246 | 436 | self.assertEqual( | 461 | self.assertEqual( |
247 | 437 | creds_meta.value, cert.certificate_pem() + cert.private_key_pem() | 462 | creds_meta.value, cert.certificate_pem() + cert.private_key_pem() |
248 | 438 | ) | 463 | ) |
249 | 464 | self.assertIsNone(reload_object(cred_virsh)) | ||
250 | 439 | 465 | ||
251 | 440 | def test_includes_smt_off_for_install_kvm_on_ppc64(self): | 466 | def test_includes_smt_off_for_install_kvm_on_ppc64(self): |
252 | 441 | password = "123secure" | 467 | password = "123secure" |
253 | diff --git a/src/metadataserver/vendor_data.py b/src/metadataserver/vendor_data.py | |||
254 | index b82fd80..7ccfcb5 100644 | |||
255 | --- a/src/metadataserver/vendor_data.py | |||
256 | +++ b/src/metadataserver/vendor_data.py | |||
257 | @@ -189,15 +189,17 @@ def generate_kvm_pod_configuration(node): | |||
258 | 189 | if node.netboot or not (node.install_kvm or node.register_vmhost): | 189 | if node.netboot or not (node.install_kvm or node.register_vmhost): |
259 | 190 | return | 190 | return |
260 | 191 | 191 | ||
261 | 192 | NodeMetadata.objects.release_volatile(node) | ||
262 | 193 | |||
263 | 192 | arch, _ = node.split_arch() | 194 | arch, _ = node.split_arch() |
264 | 193 | 195 | ||
265 | 194 | if node.register_vmhost: | 196 | if node.register_vmhost: |
266 | 195 | cert = generate_certificate(Config.objects.get_config("maas_name")) | 197 | cert = generate_certificate(Config.objects.get_config("maas_name")) |
267 | 196 | cert_pem = cert.certificate_pem() + cert.private_key_pem() | 198 | cert_pem = cert.certificate_pem() + cert.private_key_pem() |
269 | 197 | NodeMetadata.objects.update_or_create( | 199 | NodeMetadata.objects.create( |
270 | 198 | node=node, | 200 | node=node, |
271 | 199 | key=LXD_CERTIFICATE_METADATA_KEY, | 201 | key=LXD_CERTIFICATE_METADATA_KEY, |
273 | 200 | defaults={"value": cert_pem}, | 202 | value=cert_pem, |
274 | 201 | ) | 203 | ) |
275 | 202 | # write out the LXD cert on node to add it to the trust after setup | 204 | # write out the LXD cert on node to add it to the trust after setup |
276 | 203 | maas_project = "maas" | 205 | maas_project = "maas" |
277 | @@ -230,10 +232,10 @@ def generate_kvm_pod_configuration(node): | |||
278 | 230 | 232 | ||
279 | 231 | if node.install_kvm: | 233 | if node.install_kvm: |
280 | 232 | password = _generate_password() | 234 | password = _generate_password() |
282 | 233 | NodeMetadata.objects.update_or_create( | 235 | NodeMetadata.objects.create( |
283 | 234 | node=node, | 236 | node=node, |
284 | 235 | key=VIRSH_PASSWORD_METADATA_KEY, | 237 | key=VIRSH_PASSWORD_METADATA_KEY, |
286 | 236 | defaults={"value": password}, | 238 | value=password, |
287 | 237 | ) | 239 | ) |
288 | 238 | # Make sure SSH password authentication is enabled. | 240 | # Make sure SSH password authentication is enabled. |
289 | 239 | yield "ssh_pwauth", True | 241 | yield "ssh_pwauth", True |
UNIT TESTS ignore_ unused_ creds lp:~alexsander-souza/maas/+git/maas into -b master lp:~maas-committers/maas
-b lp1966474_
STATUS: SUCCESS cadd7588af6c07b 02e74b7290
COMMIT: 72818047ae4f821