Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-02-10
15:23:52 dansmith it's an operation that you can do normally, but can't for some policy reason
15:24:07 dansmith so I've never really had a problem with enabling a thing to work by implementing a detail,
15:24:20 dansmith because I think a lot of client code that does this is ignorant of the fact that makes the instance special
15:25:19 gibi efried: http://lists.openstack.org/pipermail/openstack-discuss/2019-January/001881.html
15:26:03 dansmith yup, that ^ :)
15:26:38 gibi mriedem was OK with that too http://lists.openstack.org/pipermail/openstack-discuss/2019-January/001887.html
15:28:44 efried gibi, Sundar: ack, if we decided on this and set a precedent with the qos feature, so be it. (I feel like the API-SIG might have, ahem, kept the discussion alive a bit longer, had they been involved.)
15:29:09 efried rosmaita, lyarwood: I'm going to need a little help here https://review.opendev.org/#/c/706298/
15:29:35 rosmaita efried: i'm all yours
15:30:11 efried Changing a conf opt default doesn't seem a) wise, b) effective, especially if you were planning to backport this (were you?)
15:30:54 efried I also need to understand a bit better which operations are supported/unsupported today and how they break.
15:31:30 rosmaita yes, was trying to backport
15:31:45 rosmaita but to answer your second question
15:31:51 efried The patch says we don't support "direct booting" of an instance created from encrypted volume. Do we support *anything* from such an image?
15:32:06 rosmaita yes, if you boot from volume
15:32:16 efried like, does that code path exist for backup/restore or shelve/unshelve?
15:32:19 lyarwood efried: nope, we've never supported booting from an encrypted image with cinder_encryption_key_* set in any of the in-tree virt drivers.
15:32:41 lyarwood efried: these are encrypted images created by cinder, so outside of Nova's normal flows with encrypted volumes.
15:33:05 lyarwood efried: shelve/unshelve shouldn't create images for boot from volume instances
15:33:13 efried right right.
15:33:39 dansmith notice how he says "shouldn't" ?
15:33:40 Sundar dansmith, efried: The 400s are supposed to be client error. Is this really not an unsupported operation on the server side? Or, are we taking the line that the client should have known about the restriction, and not made the request in the first place, and so it is a client error?
15:34:23 dansmith Sundar: but "permission denied" is a 40x error.. it doesn't mean the client did something wrong, it means the client shouldn't try that thing again without circumstances having changed
15:34:49 dansmith doesn't "always" mean.. I should say
15:35:14 efried lyarwood, rosmaita: And the objection to blocking this at the API level is that we don't want to rip function from 3p drivers that might have figured out a way to support it?
15:35:40 efried lyarwood, rosmaita: are we talking about 3p nova virt drivers or 3p cinder storage drivers? Or would it have to be a combination of both for it to work?
15:36:06 rosmaita efried: i think we probably should block at api layer, it's just that we don't
15:36:35 efried I'm about to agree with that, just want to confirm ---^
15:36:35 rosmaita at least short term, if you really want to implement this functionality
15:36:50 lyarwood efried: 3p nova virt drivers
15:37:01 Sundar dansmith: I am fine with that interpretation. This is what I was doing in https://review.opendev.org/#/c/674726/. So I am going to bring back that patch with some changes in the list of supported ops.
15:37:31 rosmaita efried: the config opt change is a quick short term fix that won't require operators to do an upgrade to address this
15:37:54 lyarwood rosmaita: I still don't get the usecase tbh
15:38:10 lyarwood rosmaita: they boot something that doesn't work and then snapshot it?
15:38:28 lyarwood rosmaita: but yeah this is a quick and easy fix to avoid someone doing something like that
15:38:29 rosmaita lyarwood: hopefully it is low probability
15:38:50 rosmaita but i could see someone doing a script that boots, and snapshots immediately for some reason
15:39:08 rosmaita and then when a useless image is deleted, the problem happens
15:39:57 rosmaita efried: if a config value change backport isn't allowed, maybe we could just backport the "known issues" part of the release note
15:40:02 lyarwood rosmaita: anything is possible I guess
15:40:20 efried okay, so putting my dansmith hat on (it's red, for multiple reasons), I don't think we worry about accommodating 3p virt drivers in situations like this. I usually insist we send a courtesy email to openstack-discuss when we make interface changes that could break 3p drivers; but that's about all we do.
15:40:57 lyarwood okay well in that case lets block it in the API fully and backport that
15:41:16 efried rosmaita: I don't know for sure that backporting config default changes is disallowed, but it sounds like something that would be.
15:41:25 efried but also, I just don't see it doing much good.
15:41:51 efried because you're only going to "help" people who didn't already have the value set, and who were trying to do this craziness in the first place.
15:42:02 openstackgerrit Ghanshyam Mann proposed openstack/nova master: Fix os-create-backup policy to be admin_or_owner https://review.opendev.org/706727
15:42:33 rosmaita i guess i can just send something to the operators list
15:43:43 rosmaita efried: lyarwood: i still think it's worth "blacklisting" those cinder_ properties, cinder should be the only one writing those
15:45:13 openstackgerrit Merged openstack/nova-specs master: Support multiple store of Glance https://review.opendev.org/641210
15:45:33 efried rosmaita: From what I understand so far, that makes sense to me. Under what possible circumstances could those properties actually be useful/used? And shouldn't those special circumstances be under careful control of cinder anyway?
15:46:47 efried rosmaita: Conceivably you could touch the conf opt anyway, but just to beef up the help message with something like "by the way, don't bother including XYZ options here, cause we'll always ignore them"
15:47:13 rosmaita efried: there are circumstances in which users could set those properties, but they should do it explicitly, not let nova do it
15:47:53 efried lyarwood: rosmaita: Okay, so do we have a path forward?
15:48:26 rosmaita i think so ... config change + reject instance-create call
15:48:31 openstackgerrit Ghanshyam Mann proposed openstack/nova master: Add test coverage of existing create_backup policies https://review.opendev.org/706726
15:48:33 lyarwood yup
15:49:03 rosmaita i can look into blocking this at the API layer, hopefully that's not too complicated
15:49:20 lyarwood rosmaita: let me know if you need help and I can also take a look at that
15:49:29 rosmaita lyarwood: thanks, will do
15:49:59 lyarwood rosmaita: btw, do you think cinder could ever move to a shared encryption_key image property with glance and nova?
15:50:14 lyarwood rosmaita: it would make all of this so much easier to handle
15:50:36 rosmaita lyarwood: well ... there's another encryption key effort going on
15:50:44 rosmaita it envisions shared keys
15:50:54 lyarwood rosmaita: oh jeez
15:50:57 rosmaita meaning one key, multiple resources
15:51:13 lyarwood rosmaita: link?
15:51:16 rosmaita this cinder workflow is meant to keep 1-1 key-resource relation to make deletion possible
15:51:40 lyarwood rosmaita: 1:1 between the volume and image?
15:51:56 efried rosmaita: "config change"? what config change?
15:52:21 rosmaita efried: blacklisting the cinder_* properties in the non_inheritable_image_properties list
15:52:50 efried rosmaita: Is there not a way to simply do that blacklisting without involving the conf opt?
15:52:51 rosmaita lyarwood: each volume and each image have their own corresponding barbican secret
15:53:26 efried rosmaita: I think that's what I was suggesting earlier. IOW wherever this conf opt is processed in the nova code, just *always* add those keys no matter what. And don't change the conf opt default.
15:53:26 rosmaita efried: don't know, that's what the conf opt has been used for in the past
15:53:52 rosmaita it prevents the img_* properties from being inherited (those are the ones used for signature validation_)
15:54:58 spatel sean-k-mooney: morning, This is cool, soon going to run erlang load-test and will let you know - http://paste.openstack.org/show/789378/
15:55:06 rosmaita lyarwood: https://etherpad.openstack.org/p/image-encryption-weekly-meeting -- it's not up to date, but i think it has links to all the specs about the other encryption effort
15:55:37 lyarwood rosmaita: oh that, I think that's died now anyway
15:55:53 rosmaita lyarwood: no, it is very much alive, the etherpad is just dead
15:56:04 lyarwood rosmaita: well the nova-spec died at least
15:56:40 rosmaita lyarwood: interesting
15:56:53 lyarwood rosmaita: I wanted to propose a LUKS based alternative in V FWIW
15:57:21 rosmaita lyarwood: eharney is very much of the same mind, i think
15:59:02 lyarwood rosmaita: wonderful, it would need some qemu-img convert magic to rotate keys while keeping things encrypted etc but shouldn't be too hard to sort out in nova and cinder.
15:59:40 openstackgerrit Eric Fried proposed openstack/nova master: DNM: Never convey cinder_encryption_key_* in snapshots https://review.opendev.org/706888
15:59:41 efried lyarwood: rosmaita: So what I'm talking about is, don't muck with the conf opt defaults (or do, actually, it wouldn't matter), instead do like this: ^
15:59:59 efried ...as well as the API blocker.
16:01:34 rosmaita efried: i don't object to that, though you may want to keep a list instead
16:01:50 efried "keep a list" of what?
16:01:57 rosmaita because the img_ properties should probably also be popped
16:02:17 rosmaita efried: keep a list of really_seriously_non_inheritable_image_properties
16:02:21 rosmaita (not configurable)
16:02:22 efried oh, yeah, sure, whatevs, the idea being that there are certain keys we *never* inherit, regardless of the conf opt
16:02:35 efried I leave the details to the experts :P
16:03:15 lyarwood ack yeah LGTM if we also block attempts to create instances from images with these props in the same change.
16:03:26 efried cool.
16:05:19 rosmaita efried: lyarwood: ok, i will include the really_seriously_non_inheritable_image_properties in the same patch as the API change
16:06:30 efried rosmaita: cool, left summary text on the patch with pointers to this conversation. I'll abandon my DNM.
16:06:42 rosmaita efried: ty
16:09:10 efried bauzas: I went ahead and abandoned the MKTME spec https://review.opendev.org/#/c/666769/

Earlier   Later