| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2025-06-06 | |||
| 09:43:48 | sean-k-mooney | ok so its a followup to api: Stop using wsgi.Controller.api_version to switch between API versions | |
| 09:44:53 | stephenfin | yup | |
| 09:45:01 | stephenfin | per gmaan's comments on same | |
| 09:47:09 | sean-k-mooney | cool done | |
| 09:47:51 | sean-k-mooney | assumign the parent pass ci this time it should be mergable when ever it gets to that point | |
| 10:39:28 | opendevreview | Stephen Finucane proposed openstack/nova master: api: Address issues with hypervisors APIs https://review.opendev.org/c/openstack/nova/+/950867 | |
| 10:45:47 | opendevreview | Stephen Finucane proposed openstack/nova master: api: Address issues with instance actions API https://review.opendev.org/c/openstack/nova/+/951941 | |
| 11:31:10 | opendevreview | Balazs Gibizer proposed openstack/nova master: Remove unused config options https://review.opendev.org/c/openstack/nova/+/951945 | |
| 11:32:56 | gibi | sean-k-mooney: stephenfin: a follow up for WSGIServer removal ^^ | |
| 11:33:39 | sean-k-mooney | didnt you propose that before i tough i reveied it | |
| 11:33:42 | stephenfin | I love deleting code | |
| 11:33:45 | stephenfin | done | |
| 11:33:56 | gibi | thanks | |
| 11:34:20 | sean-k-mooney | oh there are more cases we missed. | |
| 11:34:42 | gibi | yepp, I just detected them when I started looking at the deeventletification of nova-api | |
| 11:36:27 | sean-k-mooney | ok that going to conflcit with my pre-commit updates but ill approve and i cna update the last patch for bandit | |
| 11:36:52 | sean-k-mooney | stephenfin: since your about mind looking at https://review.opendev.org/q/topic:%22update-precommit%22 | |
| 11:39:39 | stephenfin | done to | |
| 11:39:40 | stephenfin | *too | |
| 11:40:58 | sean-k-mooney | thanks, ill need to rebase the bandit one later but ill do that after the other changes have merged | |
| 11:56:05 | stephenfin | gmaan: sean-k-mooney: gibi: If any of you have the time/inclination, I could do with some thoughts on how to approach https://github.com/stephenfin/nova/commit/2d14a58f6310861b426359d71fba085e924e0538 (not pushed yet to avoid overwhelming Gerrit more than I have already) | |
| 11:56:38 | stephenfin | the tl;dr: is that our use of the "soft" additionalProperties validator doesn't work when oneOf is used | |
| 11:57:46 | sean-k-mooney | ... 4/6 just for servers. better then all in one but that going to be fun to review... | |
| 11:58:29 | stephenfin | That validator deletes unrecognised ("additional") keys from the input data. That's all fine and dandy normally, but when you're using 'oneOf' with 2 or more subschemas with non-overlapping keys, then keys that are invalid for an earlier schema but which are valid for a later schema will end up getting deleted | |
| 11:58:35 | sean-k-mooney | stephenfin: so it nice an short but is there a specic point you want input on | |
| 11:59:06 | stephenfin | yeah: how on earth do we work around this? 😅 | |
| 11:59:24 | sean-k-mooney | jsut reading now | |
| 11:59:33 | sean-k-mooney | so ok were are you using onof there | |
| 12:00:06 | sean-k-mooney | ah | |
| 12:00:23 | sean-k-mooney | so either we get teh reservation_id | |
| 12:00:36 | sean-k-mooney | or the fulll server responce? | |
| 12:01:09 | stephenfin | yup | |
| 12:01:18 | sean-k-mooney | can we set addtionalProperties true in the reservation_id path | |
| 12:01:21 | gibi | stephenfin: I'm a bit low on context. Why does the validator delete keys based on earlier schemas? | |
| 12:01:26 | stephenfin | I didn't know that was a thing until I'd to write the schema for it | |
| 12:01:36 | stephenfin | gibi: sean-k-mooney: Here's a much smaller example https://paste.opendev.org/show/bpifVj7I2TTaQtFwVj4T/ | |
| 12:02:03 | sean-k-mooney | stephenfin: reservation id is only used in multi create | |
| 12:02:09 | stephenfin | gibi: https://github.com/openstack/nova/blob/master/nova/api/validation/validators.py#L182 | |
| 12:02:44 | stephenfin | first added here https://github.com/openstack/nova/commit/4fefd25c807c087694a0e66f4d91fe0e64ef3f18 | |
| 12:03:16 | sean-k-mooney | let me try something locally for one sec | |
| 12:03:48 | stephenfin | gibi: the idea behind that appears to be to allow users to specify provide additional properties in requests when using the legacy v2 API, but to strip them out and ignore them so that e.g. they can't accidentally trigger code paths for newer API versions | |
| 12:04:02 | stephenfin | s/specify provide/provide/ | |
| 12:05:34 | gibi | stephenfin: I see so this is only for the legacy v2 compatible mode | |
| 12:05:34 | stephenfin | but if you apply that to that minimal reproducer I just shared, and make a request that satisfies the second subschema (e.g. {"bar": "hello, world"}) but not the first, we attempt to match against the first one and strip out properties that are not permitted by that | |
| 12:05:38 | stephenfin | yes | |
| 12:06:42 | gibi | so the rule would be to strip out only those properties that are not in the oneOf branch that eventually matched. But I guess this is not easy to impelement in that context | |
| 12:06:51 | stephenfin | I've been sitting on this for a few months, and now that things are merging rapidly (thanks sean-k-mooney, gmaan), it's properly time to figure out a solution 😅 | |
| 12:07:36 | stephenfin | no, unfortunately not | |
| 12:08:24 | stephenfin | jsonschema iterates through the oneOf entries and processes each subschema separately, so we have no visibility into anything "above" the subschema, if that makes senes | |
| 12:08:25 | stephenfin | *sense | |
| 12:09:49 | gibi | then the best we can do is that for those schemas that has oneOf we relax the current rule and always preserve additional properties | |
| 12:10:44 | stephenfin | Yeah, that's where I'm thinking we'll end up too. I just don't know the consequences of same | |
| 12:11:27 | gibi | this is a compatibility layer that should not really used any more | |
| 12:11:42 | stephenfin | I was thinking I could move this "delete additional properties" logic into the various affected methods themselves, but that's a big list and I don't know if there's any way to know we're handling a v2 request vs. a v2.1+ request | |
| 12:11:44 | gibi | I would inclined to simply deprecate it as we cannot maintain it any more | |
| 12:11:55 | stephenfin | yeah, that's the other option: delete v2 | |
| 12:12:04 | sean-k-mooney | is something like this valid https://paste.opendev.org/show/bkS797jQUZZoSNXnhlvO/ | |
| 12:12:30 | sean-k-mooney | and if so is oneOF valid as the value of required | |
| 12:12:45 | sean-k-mooney | also have you asked gemini :) | |
| 12:14:23 | sean-k-mooney | ok it has a idea | |
| 12:14:33 | sean-k-mooney | https://paste.opendev.org/show/bMERk0Vt08kwAdUW6tLi/ | |
| 12:14:46 | sean-k-mooney | let me ask it with the full example | |
| 12:14:54 | gibi | hm does the validator in the _soft_validate_additional_properties sees the whole big schema? or has some kind of memory when the validation descends the tree? Could we use that validator object to stop applying the rule in the subtree of all oneOfs dinamically? | |
| 12:15:39 | stephenfin | sean-k-mooney: ah, no that won't work (and I actually recall trying it before): you can't combine additionalProperties with oneOf https://github.com/python-jsonschema/jsonschema/issues/193 | |
| 12:16:11 | stephenfin | https://ajv.js.org/faq.html#additional-properties-inside-compound-keywords-anyof-oneof-etc | |
| 12:17:15 | stephenfin | gibi: it does not, at least not with oneOf. It gets handed the subschemas individually | |
| 12:18:54 | stephenfin | and the issue is that e.g. if I'm given a body like {'server': {...}, 'invalidkey': 'stuff'} then I probably want to strip out 'invalidkey' (a made up example but you get my point) | |
| 12:20:34 | sean-k-mooney | https://paste.opendev.org/show/828072/ | |
| 12:20:50 | sean-k-mooney | stephenfin: ^ still readign that but that what gemini thinks about the problem | |
| 12:22:37 | stephenfin | Not a fan of approach 1. I really don't want to encode logic in our schemas. They're already complex enough | |
| 12:23:20 | sean-k-mooney | for this specific case i tought that was possibel ok if it was only this one case we neede to handel for 2.0 validation | |
| 12:23:21 | stephenfin | Hmm, actually: maybe I can just disable response schema validation entirely for v2.0... | |
| 12:23:36 | sean-k-mooney | that was gong to be my other question | |
| 12:23:44 | sean-k-mooney | v2.0 is technially unversioned | |
| 12:23:51 | stephenfin | It doesn't fix the problem but it does side-step it | |
| 12:23:57 | sean-k-mooney | so it cant really ahve a schema | |
| 12:24:25 | sean-k-mooney | by the way this coudl also be a bug | |
| 12:24:48 | sean-k-mooney | like 2.0 and 2.1 are ment to be identical | |
| 12:24:58 | sean-k-mooney | the only delta is the micoverion endpoint | |
| 12:25:02 | sean-k-mooney | but servers should not change | |
| 12:26:02 | stephenfin | they're mostly identical: look at I4c066075165f69d355a74979808fa0ad8d6546ab for example | |
| 12:26:06 | stephenfin | (commit 4a29486f4cf15720d09ed0ed86c47cd07f5cd14a) | |
| 12:27:23 | sean-k-mooney | well schduler hint need addtional properteies true alwasy | |
| 12:27:31 | sean-k-mooney | even in 2.100 | |
| 12:27:45 | sean-k-mooney | that is not a 2.1 vs 2.0 compatiablity thing | |
| 12:28:11 | stephenfin | you're right, good point | |
| 12:28:13 | sean-k-mooney | schduelr hint are alwasy exstenible becuase tehy are one of the ways we provie to oeprator writing there own schduler filter | |
| 12:28:42 | sean-k-mooney | they are however like extra specs | |
| 12:29:00 | sean-k-mooney | in that we can have diffintions for the standard ones | |
| 12:31:31 | sean-k-mooney | stephenfin: if we only have two examples to choose between i dont think the if,then,else is much more complx the oneOF | |
| 12:31:36 | sean-k-mooney | so i woudl go with that for now | |
| 12:31:50 | sean-k-mooney | both are conditionals its just the if is more explcit | |
| 12:32:12 | sean-k-mooney | if you need multiple hten i woudl vote for not validaing repocnes for 2.0 at all | |
| 12:32:28 | sean-k-mooney | but we shoudl see what others think | |
| 12:33:00 | sean-k-mooney | stephenfin: we do not have an exmaple of this for 2.1+ right? | |
| 12:34:37 | stephenfin | No, it's not an issue for v2.1 since the soft additional properties stuff only applies to v2.0 | |
| 12:35:55 | sean-k-mooney | stephenfin: actully i asked gemini for some ohter variation that aovid the repition let me link them | |
| 12:37:07 | sean-k-mooney | https://paste.opendev.org/show/b7wJJXIE4edOqlCZ3Ijj/ | |
| 12:40:32 | sean-k-mooney | can we use unevaluatedProperties? | |
| 12:41:50 | sean-k-mooney | if so option 3 is an option, 1 and 2 are mostly the same it jsut comes down to an if or switch semantic to select the correct one | |