| opendevreview | Rajat Dhasmana proposed openstack/glance stable/2023.2: Fix LP#2054575 https://review.opendev.org/c/openstack/glance/+/926025 | 05:36 |
|---|---|---|
| whoami-rajat_ | abhishekk, rosmaita can you also review this backport for 2023.2 ? ^ | 05:37 |
| abhishekk | yeah I will, | 05:37 |
| whoami-rajat_ | thanks! | 05:49 |
| abhishekk | np! | 05:50 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Add iso file format inspector https://review.opendev.org/c/openstack/glance/+/923676 | 15:09 |
| abhishekk | dansmith: replied to your comment on above patch, please have a look when you get time | 16:42 |
| dansmith | abhishekk: apply that test patch I showed and you'll see :) | 16:42 |
| abhishekk | ack | 16:42 |
| dansmith | one of the cases gets caught with "format doesn't match" which is good (and we should have that case in the tests | 16:43 |
| dansmith | the other one ends up with image.disk_format=qcow2 because we actually converted it | 16:43 |
| abhishekk | ack Ideally it should also get caught in format inspection? | 16:50 |
| dansmith | format_inspector will detect it's an iso, we just need to not allow it to ever proceed to conversion if it does | 16:51 |
| dansmith | and right now it will only stop if the user set disk_format to iso, | 16:51 |
| dansmith | if they lie about disk_format, we'll proceed further | 16:51 |
| dansmith | we really need to make it so they can't ever lie.. basically disk_format *must* match format_inspector, else fail | 16:52 |
| dansmith | oh wait | 16:52 |
| abhishekk | here we are mocking it to return iso every time | 16:52 |
| dansmith | no actually, it won't even be caught this way | 16:52 |
| dansmith | yeah, because glance isn't using the detection stuff, yeah | 16:52 |
| dansmith | so maybe best to convert it to use detect_file_format() like nova does now | 16:53 |
| dansmith | and just enforce source_format==str(inspector) | 16:53 |
| dansmith | and if they match and ==iso, then return | 16:53 |
| abhishekk | Ok, need to see how nova is doing it | 16:54 |
| dansmith | I'll link you | 16:54 |
| dansmith | https://github.com/openstack/nova/blob/master/nova/virt/images.py#L161 | 16:55 |
| abhishekk | I think first let me test it what happens if I provide iso image and set disk_format as qcow2 manually with this code | 16:55 |
| abhishekk | thanks | 16:56 |
| dansmith | abhishekk: sorry I should have realized this before I said the logic looks good | 16:56 |
| dansmith | this whole thing is such a mess and will be better for everyone after glance refuses to let you upload X and call it Y I think | 16:57 |
| abhishekk | wait | 16:57 |
| abhishekk | glance image-create-via-import --container-format bare --disk-format qcow2 --stores cheap --file good.iso | 16:57 |
| abhishekk | --name iso_as_qcow2_test | 16:57 |
| abhishekk | Aug 09 16:54:41 akekane-uwsgi devstack@g-api.service[2088764]: ERROR glance.async_.taskflow_executor [-] Failed to execute task f81a5ef2-16e1-4305-bb94-59dec519d6a0: Image format detection failed: RuntimeError: Image format detection failed | 16:58 |
| dansmith | oh right, so it fails because you said it was a qcow2 but it's not | 16:58 |
| abhishekk | right | 16:58 |
| dansmith | but what about saying it's raw, but with target_format=qcow2? | 16:58 |
| abhishekk | it will fail as well, let me check | 16:59 |
| dansmith | if I fix that in the test I still get the same two fails | 16:59 |
| dansmith | er wait, no | 16:59 |
| abhishekk | https://paste.opendev.org/show/bxxyNBJWZtebBlHRQlFE/ | 17:02 |
| dansmith | okay yeah, if I make it do the thing the runtime code will do, I still get the fail on the raw case but the qcow case is caught | 17:02 |
| dansmith | because raw file inspector will never fail | 17:03 |
| dansmith | right, but run with --disk_format=raw but target_format=qcow2 | 17:03 |
| abhishekk | ohh, sorry, it was qcow2 | 17:04 |
| dansmith | er, yeah output_format sorry | 17:04 |
| abhishekk | Right disk_format raw, output_format qcow2 and passing iso file as input is converting image to qcow2 | 17:05 |
| * dansmith nods | 17:06 | |
| dansmith | now, I guess it is technically okay because we ran qemu-img -f raw, which means it won't interpret the file, but nova will think it's a qcow2 file and detect it as an iso, and then fail to boot and a user will wonder wtf | 17:07 |
| dansmith | so *maybe* not a big security concern, but still not right and pretty confusing I think | 17:08 |
| abhishekk | yeah | 17:08 |
| dansmith | not sure whether it's best to fix that now (probably is) or just wait until we rebase on oslo when we are changing all of it | 17:08 |
| abhishekk | So in this case detect_file_format will return it as ISO | 17:08 |
| dansmith | yes | 17:08 |
| abhishekk | I think better to fix it now so that we can backport it | 17:09 |
| dansmith | ack, sounds good | 17:09 |
| abhishekk | is it working for nova? | 17:13 |
| abhishekk | I am getting, Unknown error inspecting image format: 'str' object has no attribute 'safety_check': AttributeError: 'str' object has no attribute 'safety_check' | 17:13 |
| dansmith | I'd have to see your code, but yes | 17:15 |
| abhishekk | ack, I just copied ISOInspector as it is from nova | 17:15 |
| dansmith | yeah, I mean show me what is giving you the str/safety_check error | 17:17 |
| abhishekk | inspector = format_inspector.detect_file_format(self.src_path) | 17:19 |
| abhishekk | if not inspector.safety_check(): | 17:19 |
| abhishekk | this line ^^ | 17:19 |
| dansmith | abhishekk: your line 1011 is wrong, doesn't match nova's: https://github.com/openstack/nova/blob/master/nova/image/format_inspector.py#L1026 | 17:21 |
| dansmith | you're appending format to the detections list instead of the actual inspector object | 17:21 |
| abhishekk | looking | 17:21 |
| dansmith | so you're getting back a string from detection if non-raw | 17:22 |
| abhishekk | :/ | 17:22 |
| dansmith | right? | 17:23 |
| abhishekk | yeah, checking now again | 17:27 |
| abhishekk | done, sorry for the mess | 17:27 |
| dansmith | all good, this stuff *is* a mess | 17:28 |
| abhishekk | agree, and thanks for taking care of it | 17:28 |
| abhishekk | will push new PS tomorrow changing to use detect_from_file caused failure of 19 unit tests :D | 17:39 |
| dansmith | ack | 17:40 |
| abhishekk | forgot that tomorrow is Saturday | 18:33 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Add iso file format inspector https://review.opendev.org/c/openstack/glance/+/923676 | 18:35 |
| opendevreview | Ghanshyam proposed openstack/glance master: Remove default override for RBAC config options https://review.opendev.org/c/openstack/glance/+/926087 | 18:36 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!