Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-06-12
20:32:22 dansmith mriedem: um
20:32:37 dansmith mriedem: if you use that kind of image, each would have its own signature
20:32:40 mriedem in general, i assume any image we download from glance could be signed and verified
20:32:50 dansmith aren't we buried deep in the image code enough that we'd verify those like anything else/
20:33:16 mriedem i think so https://review.openstack.org/#/c/561262/27/nova/virt/libvirt/driver.py@7432
20:33:28 mriedem this is the trouble part that's missing passing through trusted_certs
20:34:20 dansmith mriedem: you know I didn't write any of that code right?
20:34:49 mriedem IT'S YOUR GD CODE NOW SMITH
20:34:59 openstackgerrit Artom Lifshitz proposed openstack/nova master: Add InstanceNUMATopology to LibvirtLiveMigrateData https://review.openstack.org/566398
20:35:00 openstackgerrit Artom Lifshitz proposed openstack/nova master: libvirt live migration: fit NUMA topology on dest https://review.openstack.org/567242
20:35:01 openstackgerrit Artom Lifshitz proposed openstack/nova master: Service version check for NUMA live migration https://review.openstack.org/566723
20:35:02 openstackgerrit Artom Lifshitz proposed openstack/nova master: DNM: Depends on intel-nfv-ci-tests to test NUMA LM https://review.openstack.org/574872
20:35:05 mriedem your prints are all over this
20:35:17 mriedem it's 1/8 my baby now too
20:35:22 dansmith I really have lost most of my context on it at this point
20:35:28 dansmith I just split this off from the main patch for them
20:35:32 mriedem i know
20:35:38 mriedem and this change could be split into 2 now too
20:35:43 mriedem virt changes and then compute manager changes
20:35:45 mriedem but i'm not going to do that
20:37:05 dansmith yeah, so I guess you're right, since they don't pass trusted_certs there,
20:37:18 dansmith but if they did what you suggest (which I'm pretty sure I had suggested already) then they'd auto-verify
20:39:30 mriedem yeah i moved the low-level calls to libvirt_utils.fetch_image to just use the in-scope instance variable and pass instance.trusted_certs rather than add a new parameter to the various _try_fetch_image* methods
20:39:37 mriedem that's how i found this bug
20:45:12 mriedem gdi, i can't really tell how to assert anything that is mocked out from ImageBackendFixture
20:50:52 efried mriedem: example?
20:51:31 mriedem don't have the energy
20:51:46 efried mriedem: You mean from something like
20:51:46 efried self.mock_create_ephemeral = \
20:51:46 efried mock.create_autospec(driver.LibvirtDriver._create_ephemeral)
20:51:46 efried self.useFixture(fixtures.MonkeyPatch(
20:51:46 efried 'nova.virt.libvirt.driver.LibvirtDriver._create_ephemeral',
20:51:46 efried self.mock_create_ephemeral))
20:51:46 efried you want to assert that the _create_ephemeral mock was called?
20:52:06 mriedem more or less yes,
20:52:07 mriedem was doing:
20:52:10 efried mriedem: You have to make the fixture save off the actual mock
20:52:13 mriedem img_backend_fixture = snaz
20:52:16 mriedem ...
20:52:29 mriedem img_backend_fixture.mocked_thing.assert_called_once_with(...)
20:52:42 mriedem and tried mocked_thing.return_value
20:53:02 mriedem i thought the autospeced mock was being saved in those variables
20:53:09 efried mriedem: So like:
20:53:10 efried self.mock_create_ephemeral = \
20:53:10 efried mock.create_autospec(driver.LibvirtDriver._create_ephemeral)
20:53:10 efried fx = self.useFixture(fixtures.MonkeyPatch(
20:53:10 efried 'nova.virt.libvirt.driver.LibvirtDriver._create_ephemeral',
20:53:10 efried self.mock_create_ephemeral))
20:53:10 efried self.mriedem_mock = fx.mock
20:53:36 mriedem ok, well...
20:53:43 mriedem i'm past caring really
20:55:04 efried mriedem: You're right, though, that create_autospec ought to be a mock you can assert on.
20:55:28 mriedem i just hate the libvirt driver unit tests
20:55:31 mriedem but that's not news
20:55:40 efried mriedem: I've got +2s on the bottom nine patches in the neutron port binding series.
20:55:52 mriedem hot diggity dog
20:56:00 mriedem how many patches are there? 25?
20:56:12 efried mriedem: I'm going to punt on the libvirt ones, at least unless someone begs.
20:56:25 efried looks like 15
20:56:26 mriedem https://review.openstack.org/#/q/topic:bp/neutron-new-port-binding-api+(status:open+OR+status:merged)
20:56:52 efried yeah, those all show up in the series
20:57:08 mriedem thanks for hitting those, even before it was in a runway
20:57:15 efried yahyoubetcha.
20:57:25 mriedem despite me wanting to murder you for awhile on the ksa stuff... :)
21:00:50 efried mriedem: If I had a shiny nickel for every time someone wanted to murder me...
21:02:17 efried mriedem: Here's some great news on ksa adapter, btw. Once https://review.openstack.org/#/c/574784/ is in a release, we can stop sending raise_exc=False with every request!
21:05:39 mriedem small victories
21:07:13 mriedem you know,
21:07:18 mriedem this trusted certs thing,
21:07:28 mriedem doesn't seem like something that should be plumbed through the various virt drivers at all
21:07:32 mriedem should/needs to be
21:07:47 mriedem but that's how we eventually get from compute manager to image api download()
21:07:56 mriedem kind of sucks though
21:08:18 mriedem it doesn't have anything specific to do with the virt drivers at all
21:08:20 mriedem nor the guest
21:10:00 mriedem like, could we do something like monkey patch or partial the image download api method from the compute manager flows that call driver.spawn() and pass the instance.trusted_certs right there to download()?
21:10:06 mriedem so the virt drivers don't need to plumb all that in
21:10:23 mriedem dansmith: ^?
21:10:57 dansmith I mean
21:10:58 dansmith yes?
21:11:28 dansmith that seems kinda gross
21:11:39 mriedem i'm not saying it's awesome
21:11:42 dansmith the virt driver is just passing instance.trusted_certs to the image download stuff right?
21:11:53 mriedem but neither is doing this exact same plumbing in all of the virt drivers for someting that's virt agnostic
21:12:15 dansmith yeah, okay, but .. we already have all those paths,
21:12:32 dansmith we should clean it all up and unify what we can and not just partial our way to verification right?
21:12:38 dansmith because we need to raise something and have it handled properly,
21:12:50 dansmith and just doing that when the virt driver doesn't expect it is probably not "good for security" and stuff
21:13:55 mriedem yeah fine,
21:13:59 mriedem plus i'm not at all signing up for this
21:14:15 mriedem i'm just really sour on this by now
21:16:12 openstackgerrit Matt Riedemann proposed openstack/nova master: Plumb trusted_certs through the compute service https://review.openstack.org/561262
21:16:13 openstackgerrit Matt Riedemann proposed openstack/nova master: Add trusted_image_certificates to REST API https://review.openstack.org/486204
21:16:14 openstackgerrit Matt Riedemann proposed openstack/nova master: Add notification support for trusted_certs https://review.openstack.org/563269
21:16:15 openstackgerrit Matt Riedemann proposed openstack/nova master: Add certificate validation docs https://review.openstack.org/560158
21:16:16 openstackgerrit Matt Riedemann proposed openstack/nova master: WIP: Add trusted certs to feature support matrix docs https://review.openstack.org/574890
21:20:20 mriedem melwitt: i think you should definitely get in on this hot trusted certs action since it's in a runway slot for the umpteenth time https://review.openstack.org/#/c/561262/ - is the current not-yet-approved bottom of the series
21:20:36 mriedem i will ruin the rest api change tomorrow
21:39:02 melwitt mriedem: I'll try. I reviewed some of it back before we had runways and it looked mostly fine to me... then people like yourself found all sorts of problems with it
22:27:05 mnaser does the placement api have type of way of listing all inventories in a single http request

Earlier   Later