| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-01-25 | |||
| 09:30:51 | johnthetubaguy | bauzas: So zigo has issues in train I think (which I am interested in, sadly), but I am more curious if we can merge stable branches out of order for a security fix like this? | |
| 09:31:25 | zigo | bauzas: Parts of nova changed between train and ussuri, but I think I can manage. | |
| 09:31:33 | zigo | Let me finish the backporting ... :) | |
| 09:31:51 | bauzas | johnthetubaguy: I'm rushing to review the stable branches | |
| 09:32:24 | bauzas | johnthetubaguy: technically, we have a CI job that prevents merging a patch on a stable branch if the parent isn't merged | |
| 09:32:47 | johnthetubaguy | ah, I didn't know that. | |
| 09:33:03 | bauzas | fwiw, ChatGPT is unable to find the typo https://review.opendev.org/c/openstack/oslo.utils/+/706880/4/oslo_utils/imageutils.py despite me giving him clues | |
| 09:33:09 | johnthetubaguy | I see arguments both ways for sure, but I wanted to check. | |
| 09:36:46 | bauzas | so, master is merged | |
| 09:36:59 | bauzas | zed was running on the gate but we got a failure | |
| 09:37:08 | johnthetubaguy | ack | |
| 09:37:09 | bauzas | and I just +2d yoga | |
| 09:37:29 | johnthetubaguy | OK, that was my question really, is that allowed? | |
| 09:37:51 | johnthetubaguy | I guess we just wait for the +W? | |
| 09:38:06 | bauzas | to merge some stable N-x patch before the parents ? | |
| 09:38:45 | bauzas | https://docs.openstack.org/project-team-guide/stable-branches.html#appropriate-fixes | |
| 09:38:53 | bauzas | " Whether the fix is already on master and all consequent stable branches: a change must be a backport of a change already merged onto master, unless the change simply does not make sense on master. Same applies to N-2 releases, where N is master, in which case both N-1 and N branches should have the patch merged and so on." | |
| 09:39:03 | gibi | nova-tox-validate-backport job is voting in the gate queue and I that will prevent merging an older branch before the patch on newer branch lands | |
| 09:39:04 | bauzas | but, there is an exception rule | |
| 09:39:13 | bauzas | " Some patches may get exception from rule 4 above. These are patches that do not touch production code, like test-only patches, or tox.ini changes that fix major gate breakage, etc.; or security patches that should not take much time to merge once the patches are published. In those cases, stable patches may be pushed into gate without waiting for all consequent branches to be fixed." | |
| 09:39:25 | bauzas | gibi: that's what I told to johnthetubaguy | |
| 09:39:42 | johnthetubaguy | yeah, I think we should just merge them all ASAP: "or security patches that should not take much time to merge once the patches are published" | |
| 09:39:44 | gibi | bauzas: so even though the policy allow secu patches to land our tooling did not | |
| 09:39:55 | bauzas | gibi: I was about to write this | |
| 09:39:59 | johnthetubaguy | ah, got you | |
| 09:40:05 | bauzas | somehow we are strictier than the policy | |
| 09:40:45 | johnthetubaguy | so what is the quickest path to merge? in parallel drop that CI job? | |
| 09:40:49 | bauzas | johnthetubaguy: either way, you know we'll need to publish releases | |
| 09:41:07 | bauzas | upstream isn't generally a quick path for remediation | |
| 09:41:14 | bauzas | but we can try to rush | |
| 09:41:21 | johnthetubaguy | its meant to be though, in this particular case. | |
| 09:41:40 | bauzas | johnthetubaguy: that's why distros got notified one week before the disclosure | |
| 09:41:46 | johnthetubaguy | granted the patches are published, which is the important and crucial bit. | |
| 09:42:18 | bauzas | I don't disagree and the security hole is quite dirty | |
| 09:42:41 | bauzas | I would recommend our operators to exceptionnally patch directly | |
| 09:43:00 | bauzas | the patch itself is just about enforcing | |
| 09:46:23 | gibi | one way to rush this is to add [stable-only] to the commit message to silence the backport job | |
| 09:47:41 | bauzas | that's debatable | |
| 09:47:49 | johnthetubaguy | gibi: I like your thinking! | |
| 09:48:11 | bauzas | gibi: your help may be needed on https://review.opendev.org/c/openstack/nova/+/871624 | |
| 09:48:46 | bauzas | johnthetubaguy: which release do you target to ship the security fix ? | |
| 09:49:23 | elodilles | o/ | |
| 09:49:37 | gibi | bauzas: I can approve that but the job will kick it out from the gate | |
| 09:49:44 | elodilles | let me know if any patch needs review | |
| 09:49:50 | gibi | elodilles: it is more like a process thing | |
| 09:49:53 | bauzas | elodilles: we're on the CVE backports | |
| 09:50:03 | elodilles | (i'm not aware of any special handling of security bug fixes) | |
| 09:50:03 | gibi | elodilles: the stable policy allows parallel merge of secu stable fixes | |
| 09:50:06 | bauzas | elodilles: the zed one got kicked out due to some gate failure | |
| 09:50:09 | gibi | elodilles: but the backport job does not | |
| 09:50:43 | elodilles | ok, then [stable-only] is right | |
| 09:50:43 | gibi | elodilles: do you have a magic want to turn off the job or merge the patches against that job? | |
| 09:50:46 | bauzas | gibi: I'd say that if the stable policy agrees on that, we could refer to it if we respin the patch to add the [stable-only] tag | |
| 09:50:56 | gibi | OK then we have a consensus | |
| 09:51:04 | elodilles | if you want them to merge not in the known order | |
| 09:51:08 | gibi | bauzas: will you add the [stable-only] tags? | |
| 09:51:10 | bauzas | ideally, I'd rather prefer to have a specific tag but we're a bit on a rush | |
| 09:51:12 | johnthetubaguy | bauzas: we have a zoo of people on different things, I am not worried about them, thats all happening, I am more worried in general about it not landing in the stable branches. | |
| 09:51:32 | gibi | bauzas: then elodilles and me can approve qucikly | |
| 09:51:42 | johnthetubaguy | what about [stable-only][actually-a-cve] ? | |
| 09:52:05 | gibi | johnthetubaguy: adding a sentence why we add stable-only make sense, yes | |
| 09:52:11 | elodilles | :] | |
| 09:52:35 | gibi | and as a follow up we should extend the job to accept [cve] as a tag to silence the landing order check | |
| 09:52:45 | johnthetubaguy | gibi: +1 | |
| 09:53:00 | bauzas | johnthetubaguy: as a reminder, train requires some oslo.utils bump due to https://review.opendev.org/c/openstack/oslo.utils/+/706880 missing | |
| 09:53:34 | johnthetubaguy | bauzas: appreciated, thanks, I saw zigo is working through that. | |
| 09:56:04 | zigo | Either oslo bump, or backport of https://review.opendev.org/c/openstack/oslo.utils/+/706880 | |
| 09:56:07 | zigo | I did the later ... | |
| 09:56:32 | zigo | YEAH !!! | |
| 09:56:38 | zigo | Got something that worked ! \o/ | |
| 09:56:45 | bauzas | gibi: if we start parallelizing the backports, then we'll break the sha1 information on the commit msgs | |
| 09:57:11 | bauzas | since there is no guarantee that the commit in the gerrit branch will have the same sha1 once merged | |
| 09:57:11 | gibi | bauzas: ignore the sha (it does not have it now either) as we disable the check anyhow | |
| 09:57:29 | bauzas | gibi: I'll then mention the gerrit links | |
| 09:57:50 | gibi | bauzas: the bug ref is there to make a connection, but yes if you want you can add gerrit change id | |
| 09:57:53 | gibi | or link | |
| 09:58:09 | bauzas | unfortunately, we can't also track all commits easily as they're on different branches | |
| 09:58:55 | zigo | So, I did: | |
| 09:58:55 | zigo | https://salsa.debian.org/openstack-team/services/nova/-/blob/debian/train/debian/patches/cve-2022-47951-nova-stable-train.patch | |
| 09:58:55 | zigo | https://salsa.debian.org/openstack-team/services/nova/-/blob/debian/train/debian/patches/images_Move_qemu-img_info_calls_into_privsep.patch | |
| 09:58:55 | zigo | https://salsa.debian.org/openstack-team/services/nova/-/blob/debian/train/debian/patches/images_Make_JSON_the_default_output_format_of_calls_to_qemu-img_info.patch | |
| 09:58:56 | zigo | With these 3 patches, all unit tests are passing. | |
| 09:59:58 | zigo | Is this acceptable (from your Nova upstream point of view) to backport these 3 patches on the train branch? | |
| 10:01:39 | bauzas | zigo: propose the backports | |
| 10:01:54 | bauzas | Train is EM | |
| 10:02:08 | zigo | bauzas: The only issue is that I apply them in the wrong order, so patch 1 and 2 may have failures ... | |
| 10:02:22 | zigo | Can I just merge the 3 patches then? | |
| 10:02:37 | johnthetubaguy | zigo: +1 that looks like a nice solution to me. You can submit them as a patch chain in gerrit, that should work OK I think? | |
| 10:02:39 | bauzas | then I need to review those patches | |
| 10:02:50 | bauzas | and yeah, this would have to be a gerrit series | |
| 10:02:56 | zigo | Ok, doing this. | |
| 10:03:02 | gibi | zigo: https://salsa.debian.org/openstack-team/services/nova/-/blob/debian/train/debian/patches/images_Make_JSON_the_default_output_format_of_calls_to_qemu-img_info.patch#L416 format=output_format ? | |
| 10:04:04 | zigo | gibi: That's what is in there: https://review.opendev.org/c/openstack/nova/+/711679/6/nova/virt/images.py#57 | |
| 10:04:13 | opendevreview | Sylvain Bauza proposed openstack/nova stable/yoga: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871624 | |
| 10:04:32 | zigo | Ask Lee Yarwood I guess? :) | |
| 10:04:32 | gibi | zigo: then that is a latent bug, so keep it as is | |
| 10:04:55 | opendevreview | Sylvain Bauza proposed openstack/nova stable/xena: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871622 | |
| 10:05:34 | opendevreview | Sylvain Bauza proposed openstack/nova stable/wallaby: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871557 | |
| 10:05:40 | gibi | zigo: lyarwood is gone from openstack unfortunately | |
| 10:06:19 | bauzas | I'd be honest, I spotted a few bugs on this | |