| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-05-20 | |||
| 19:11:24 | melwitt | right | |
| 19:11:30 | sean-k-mooney | we would use the flavor/iamge properties | |
| 19:11:36 | sean-k-mooney | and unlike the adoptied ones | |
| 19:11:44 | sean-k-mooney | each lvm disk would have its own key | |
| 19:11:49 | sean-k-mooney | just like with qcow | |
| 19:12:14 | melwitt | yes, that is what I thought the spec was intending | |
| 19:12:18 | melwitt | to not have two separate ways of doing local encryption | |
| 19:12:33 | sean-k-mooney | right | |
| 19:12:45 | melwitt | and in the process lvm gains some new user friendly abilities as a result | |
| 19:13:00 | sean-k-mooney | but that would only result in host tha tuse lvm reporting COMPUTE_EPHEMERAL_ENCRYPTION | |
| 19:13:10 | sean-k-mooney | after we supprot upgrading the exiting instnaces | |
| 19:13:32 | melwitt | ok. so all that said, my understanding was that for the current chunk we're trying to do now in dalmatian we are only doing luks and leaving lvm as the legacy config options at this time | |
| 19:13:50 | sean-k-mooney | yep | |
| 19:13:54 | sean-k-mooney | thats what i tought too | |
| 19:14:08 | sean-k-mooney | so supporting rbd,raw/flat,qcow | |
| 19:14:17 | sean-k-mooney | but not lvm | |
| 19:14:26 | melwitt | ok. so it seems then that we would hold off on introducing hw_ephemeral_encryption_format until we work on the lvm support | |
| 19:14:31 | melwitt | right | |
| 19:14:37 | sean-k-mooney | and the exisitng lvm fature will be deprecate but not removed until we can adopt them into the new framework | |
| 19:15:23 | sean-k-mooney | yes we would only need hw_ephemeral_encryption_format if we supprot somethign other hten luksv1 | |
| 19:16:11 | sean-k-mooney | which also means if os_glance_encrypt_format is set to anything other then luks we shoudl reject that too right | |
| 19:16:31 | melwitt | ok, I think we all agree about that. so I'll update the spec to explain this in a lot of detail 😂 and that hw_ephemeral_encryption_format will not be part of this first chunk and that it would come into play only when we work on adding support for lvm | |
| 19:17:02 | sean-k-mooney | and do we agree the same is true for the config option | |
| 19:17:22 | melwitt | I think it would be reasonable to reject that yes, if os_glance_encrypt_format is not luks. we won't be able to honor it | |
| 19:17:40 | sean-k-mooney | ok | |
| 19:18:15 | melwitt | I think I made the config option a choices= where the only choice is luks | |
| 19:18:17 | melwitt | but I'll double check and make it that if it isn't already | |
| 19:18:30 | sean-k-mooney | ok | |
| 19:18:40 | sean-k-mooney | if its only luks then i dont think we need it personally | |
| 19:20:13 | melwitt | that's a fair point. I suppose all it could potentially be helping with transparency but I could see the argument not to have it | |
| 19:20:23 | opendevreview | Merged openstack/nova master: Enable OCaaS for several nova jobs https://review.opendev.org/c/openstack/nova/+/919738 | |
| 19:20:26 | sean-k-mooney | i think what we are setteling on is we only need 1 extra spec/image property the boolean hw:ephemeral_encryption/hw_ephemeral_encryption | |
| 19:20:40 | melwitt | that is also my understanding | |
| 19:20:44 | sean-k-mooney | and if that is unset and the image in encyped then the vm would be encypted too | |
| 19:20:53 | sean-k-mooney | meaning you dont have to opt in for encypted images | |
| 19:21:04 | melwitt | yes, that's what I understand as well | |
| 19:21:20 | sean-k-mooney | so there is only one last bit i want to check | |
| 19:22:00 | sean-k-mooney | will we allow an opt out via the flavor i.e. hw:ephemeral_encryption=false and os_glance_encrypt_format=luks | |
| 19:22:06 | sean-k-mooney | should that be a 409 conflict | |
| 19:22:09 | sean-k-mooney | or decypt the image | |
| 19:22:15 | melwitt | the main thing is I think we want to avoid an unpleasant surprise on that. similar to the encrypted rescue image, not to just decrypt it | |
| 19:22:47 | melwitt | I think the desire based on current code review comments is that it would decrypt in that case | |
| 19:22:51 | sean-k-mooney | personally im feeling like hw:ephemeral_encryption=false and os_glance_encrypt_format=luks woudl be a 409 conflict at least for now | |
| 19:23:09 | melwitt | yeah, I think it would be fair to start with that | |
| 19:23:50 | sean-k-mooney | the reason i hesitate there is it would allow anyoen in the same project (realy that has access to the image secret) to trivially/acidentaly decypt the image | |
| 19:24:14 | melwitt | and tack on a separate patch that enables the ability to request decryption | |
| 19:24:18 | melwitt | because it's like a user experience thing. not really "required" but nice to have to give users that ability | |
| 19:24:23 | sean-k-mooney | its not uncommon for peopel to query flavors by size and select the first one that they find | |
| 19:24:30 | melwitt | yeah. that would be my only concern with it as well | |
| 19:25:03 | sean-k-mooney | to be fair if thte image had os_glance_encrypt_format=luks hw_ephemeral_encryption=ture | |
| 19:25:27 | sean-k-mooney | i think we all agree it should be a 409 conflict if the flavor has hw:ephemeral_encryption=false | |
| 19:26:00 | sean-k-mooney | but since the arguement is being made that os_glance_encrypt_format=luks effectivly implies hw_ephemeral_encryption=ture | |
| 19:26:33 | sean-k-mooney | im wonderin if we shoudl also imply it when comparing ot a flavor with hw:ephemeral_encryption=false | |
| 19:26:35 | melwitt | I expect we all agree that's an ok first step. we've been trying to keep patches smaller in this thing as well so I have been splitting smaller things like that into their own patch | |
| 19:26:49 | sean-k-mooney | ok | |
| 19:27:28 | sean-k-mooney | so before this converstaion i was tending to say "ok ill approve the spec and we can take it to the implemeation" no i feel like we were not on the same page but have got much closer | |
| 19:27:45 | sean-k-mooney | so my stomach say i shoudl have breakfast/dinner | |
| 19:27:55 | sean-k-mooney | but i also dont really want to leave this open | |
| 19:28:04 | sean-k-mooney | so melwitt how do you want to proceed | |
| 19:28:18 | melwitt | yeah so I was thinking if os_glance_encrypt_format=luks (and really mainly just the presence of os_glance_encrypt_key_id) then if hw:ephemeral_encryption=false is in direct conflict and would act the same as the image conflict. until we offer decryption later if we do that | |
| 19:29:29 | sean-k-mooney | ack i think that is logicaly consitent | |
| 19:30:03 | sean-k-mooney | i feel like os_glance_encrypt_key_id is only valid if all the other os_glance_encrypt_* are dfeind | |
| 19:30:15 | melwitt | sean-k-mooney: I think it would probably be best to update the spec now. either way I will update it -- if it gets approved as-is I will still have an amendment proposed shortly. to not forget all this stuff | |
| 19:30:17 | melwitt | so either way is fine with me | |
| 19:30:26 | melwitt | I agree. I think that is the intent in the standardized properties anyway | |
| 19:30:29 | sean-k-mooney | so its not clear to me if we shoudl be checking any spefici one of those keys or if any of them exist | |
| 19:32:16 | melwitt | yeah.. I'll probably add validation for that just to make a better experience. I'm not sure if they are all required to be set from a glance perspective. like, will glance reject upload of an image that only sets one or a subset of them? or maybe if it does allow it, it will fill it defaults itself | |
| 19:32:29 | melwitt | so either way on the nova side we would expect/require them all to be set if one is set | |
| 19:32:38 | sean-k-mooney | so the glance spec used ot have a seperate nova spec https://review.opendev.org/c/openstack/nova-specs/+/608696/11/specs/train/approved/image-encryption.rst | |
| 19:32:43 | sean-k-mooney | for how we would use those | |
| 19:33:27 | melwitt | from my perspective at a minimum if os_glance_encrypt_key_id is set, then os_glance_encrypt_format is required | |
| 19:33:31 | sean-k-mooney | in that they explictly talk about adding supprot for decypting the encyped image when booting the instnace | |
| 19:33:59 | sean-k-mooney | melwitt: ya i think those two are requried and likely the type as well i.e. symetic | |
| 19:34:15 | melwitt | ah ok | |
| 19:34:17 | sean-k-mooney | having said that i think we are only going to support symmetric encption | |
| 19:34:26 | melwitt | ah right | |
| 19:34:30 | sean-k-mooney | so i guess if its single valued | |
| 19:34:34 | sean-k-mooney | it does not matter for now | |
| 19:36:01 | sean-k-mooney | melwitt: you know what would help | |
| 19:36:14 | melwitt | yeah, we can adapt that as we go along. if we think more than just those two should be required to be present to be valid | |
| 19:36:18 | melwitt | because the other use case is an individual user uploading an encrypted image | |
| 19:36:26 | sean-k-mooney | in your spec can you define the only values of the os_glance_encrypt* properties that will actully work for now | |
| 19:36:28 | melwitt | so not sure if they will be required to provide all image properties | |
| 19:37:53 | sean-k-mooney | like will you be using os_glance_decrypt_size? | |
| 19:38:11 | sean-k-mooney | i dont think you are your using virt_size + 1G in the check right | |
| 19:38:15 | melwitt | sure I can add specifics about which ones we are requiring | |
| 19:38:35 | melwitt | there is a check for that somewhere actually | |
| 19:38:39 | sean-k-mooney | well it might hjsut be good to say hay nova is not using os_glance_decrypt_size | |
| 19:39:12 | melwitt | today we validate image size in some way and I had to add the "plus overhead" to the calc if encrypted for it to pass | |
| 19:39:19 | sean-k-mooney | right | |
| 19:39:41 | sean-k-mooney | but if i rememebr reviewing that code we do not use os_glance_decrypt_size | |
| 19:40:22 | melwitt | yeah I guess really i could use os_glance_decrypt_size instead for the validation if the image is encrypted. since that's what we actually care about, is if the decrypted size will fit in the chosen flavor | |
| 19:40:46 | melwitt | no we didn't have os_glance* properties proposed at the time | |
| 19:40:49 | sean-k-mooney | os_glance_decrypt_size and os_glance_decrypt_container_format i think are for the usecase of booting form an encypted iamge and decypting it on the host | |
| 19:40:56 | melwitt | well, sorry | |
| 19:41:03 | sean-k-mooney | which would imply they expect the virtual_size to include the overhead | |
| 19:41:06 | melwitt | they were proposed already but we were not planning to use them initially | |
| 19:41:09 | sean-k-mooney | when its encypted | |
| 19:41:24 | melwitt | but at the ptg we agreed to use them so I will be changing where needed to use those instead of what I had put | |
| 19:41:52 | sean-k-mooney | sure but os_glance_decrypt_size is defied as "size after payload decryption" | |