| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-11-30 | |||
| 16:57:08 | gmann | dansmith: yes, it fail on 404 for server not found | |
| 16:57:44 | gmann | either we need to allow get server to service user or keep these internal APIs as admin access | |
| 16:57:57 | dansmith | right so you'll have to do everything *except* swap_volume with a user token, and only swap_volume with the service token right? | |
| 16:58:36 | gmann | yeah | |
| 16:59:03 | dansmith | isn't that what we expect cinder to do? *only* call swap_volume with the service user | |
| 16:59:03 | gmann | but that need to be further tested if volume get in same situation as service role cannot get volume in cinder | |
| 16:59:27 | dansmith | oh, you mean nova tries to get volume during swap_volume/ | |
| 16:59:54 | gmann | not nova. may be server external event is good example | |
| 17:00:13 | sean-k-mooney | gmann: for the tempest user could have both both admin and service | |
| 17:00:22 | bauzas | dansmith: when you mean a "leave", you mean a PTO or something else ? | |
| 17:00:25 | sean-k-mooney | and you coudl drop admin later | |
| 17:00:26 | dansmith | sean-k-mooney: I think that's a bad idea | |
| 17:00:37 | sean-k-mooney | dansmith: becasue we wont know which permission is working | |
| 17:00:42 | dansmith | bauzas: PTO yeah, for the rest of the year, starting +1w from now | |
| 17:00:43 | gmann | sean-k-mooney: we will not be able to drop it right? | |
| 17:00:50 | dansmith | sean-k-mooney: and because we never want an operator to do that | |
| 17:00:51 | bauzas | dansmith: ack, gtk | |
| 17:01:00 | sean-k-mooney | gmann: we should be able to | |
| 17:01:16 | gmann | service only role does not work so our main goal to make internal APIs for service only is not fulfil | |
| 17:01:31 | dansmith | yeah, it's not the tempest user gmann is concerned about here right? | |
| 17:01:39 | gmann | yes | |
| 17:01:41 | dansmith | it's some composite operation where the service calling another ends up needing both? | |
| 17:01:48 | dansmith | gmann: finish your example with external_event | |
| 17:02:22 | sean-k-mooney | you said " they need admin permission also to get the servers " do you mean cinder | |
| 17:02:33 | gmann | neutron call external event API with service user and then nova will return 404 | |
| 17:02:38 | gmann | 404 for get server | |
| 17:02:53 | dansmith | gmann: do you mean because nova internally does a get_server? | |
| 17:03:04 | gmann | dansmith: sean-k-mooney either we need to access Db for server with hard coded admin inside the API | |
| 17:03:17 | gmann | dansmith: yes, get server of the requested project if not admin | |
| 17:03:17 | dansmith | are we still checking that at the db layer? | |
| 17:03:35 | sean-k-mooney | dansmith: ya i think we are | |
| 17:03:42 | gmann | I think it match the project_id from context unless it is admin | |
| 17:04:11 | dansmith | okay, so external_event should get the server from the db with admin context, but do the usual policy check of "can you see this" on the result? | |
| 17:04:37 | sean-k-mooney | the external events api si doign the server get | |
| 17:04:40 | sean-k-mooney | to get the host | |
| 17:04:45 | sean-k-mooney | so it know where to send the event | |
| 17:04:51 | dansmith | well, also just to make sure it's for a legit server right? | |
| 17:04:57 | gmann | dansmith: if we do with admin then no 'get server policy' come into pic | |
| 17:05:08 | sean-k-mooney | yes also to ensure it exsits | |
| 17:05:28 | sean-k-mooney | so we coudl make that db check supprot the service user too | |
| 17:05:32 | gmann | if we are ok to use admin context inside those then it should work | |
| 17:05:39 | dansmith | we need to make sure we don't leak the existence of servers via the external_event API because someone can call it and get a 403 vs 404, which I presume is why we look up the server with user creds now | |
| 17:05:50 | sean-k-mooney | eventully we proably want to remvoe the db check but as a minimal interim step i think that would be ok | |
| 17:06:08 | gmann | dansmith: yeah that is issue of 403 vs 404 then | |
| 17:06:26 | gmann | and yes leak of server existence | |
| 17:06:27 | dansmith | yeah, just need to be careful about that | |
| 17:06:28 | sean-k-mooney | well the api is admin only now | |
| 17:06:43 | gmann | now is ok, if we make it service only | |
| 17:06:50 | dansmith | yeah, if they get stopped before the server get because they lack service role, then that's fine | |
| 17:06:52 | gmann | I can pass server uuid to know if that exist or not | |
| 17:07:15 | sean-k-mooney | it would only be an issue if you had the service role | |
| 17:07:21 | sean-k-mooney | which no human shoudl ever have | |
| 17:07:21 | dansmith | yeah | |
| 17:07:32 | gmann | dansmith: yes, that policy check will be there for service role before they try getting sevrer | |
| 17:07:39 | dansmith | for this case.. that might not be the case for all of ours, if we have any that are legit for humans and machines | |
| 17:07:43 | dansmith | gmann: cool | |
| 17:07:46 | gmann | sean-k-mooney: yes, only with service role | |
| 17:08:19 | sean-k-mooney | dansmith: woudl you object ot adding the service role to the place where we check for admin in the db | |
| 17:08:30 | dansmith | sean-k-mooney: I think that's a bad idea | |
| 17:08:36 | sean-k-mooney | even if its just an interim step to reventually removing that in the db layer | |
| 17:08:50 | sean-k-mooney | ok so you would prefer we internally escalate to admin context | |
| 17:08:53 | dansmith | sean-k-mooney: not only because it would affect lots of other non-service role things, but also because it expands that check which we probably want to minimize | |
| 17:09:08 | sean-k-mooney | ya that fiar | |
| 17:09:10 | dansmith | I'd prefer we explicitly "elevate" to admin for service role things at the point of access | |
| 17:09:12 | gmann | yeah | |
| 17:09:26 | gmann | ok, let me go with approach 1. policy check for service role at start 2. fetch the things (server etc) with admin context | |
| 17:09:41 | dansmith | ++ | |
| 17:09:48 | sean-k-mooney | yep that works for me | |
| 17:10:01 | sean-k-mooney | the same shoudl apply to swap volume | |
| 17:10:10 | sean-k-mooney | its the same check that is failign right | |
| 17:10:14 | gmann | ok, thanks dansmith sean-k-mooney | |
| 17:10:16 | sean-k-mooney | the get_server | |
| 17:10:36 | gmann | dansmith: sean-k-mooney btw if you have time, placement policy updates are ready to review too https://review.opendev.org/c/openstack/placement/+/865618 | |
| 17:10:39 | gmann | gibi: ^^ | |
| 17:10:45 | gmann | sean-k-mooney: yes | |
| 17:14:59 | sean-k-mooney | gmann: realistically it will be next week before i have time to take a look but i set RP+1 and ill try to come back to itthen | |
| 17:15:33 | gmann | sean-k-mooney: sure, thanks | |
| 17:19:24 | opendevreview | melanie witt proposed openstack/nova stable/yoga: Adapt websocketproxy tests for SimpleHTTPServer fix https://review.opendev.org/c/openstack/nova/+/866192 | |
| 17:20:36 | opendevreview | melanie witt proposed openstack/nova stable/xena: Adapt websocketproxy tests for SimpleHTTPServer fix https://review.opendev.org/c/openstack/nova/+/866193 | |
| 17:21:41 | opendevreview | melanie witt proposed openstack/nova stable/wallaby: Adapt websocketproxy tests for SimpleHTTPServer fix https://review.opendev.org/c/openstack/nova/+/866194 | |
| 17:29:43 | opendevreview | melanie witt proposed openstack/nova stable/victoria: Adapt websocketproxy tests for SimpleHTTPServer fix https://review.opendev.org/c/openstack/nova/+/866195 | |
| 17:31:16 | opendevreview | melanie witt proposed openstack/nova stable/ussuri: Adapt websocketproxy tests for SimpleHTTPServer fix https://review.opendev.org/c/openstack/nova/+/866196 | |
| 18:05:07 | opendevreview | melanie witt proposed openstack/nova stable/train: Adapt websocketproxy tests for SimpleHTTPServer fix https://review.opendev.org/c/openstack/nova/+/866201 | |
| 18:07:24 | opendevreview | Merged openstack/nova-specs master: fixing: allowing target state for evacuate https://review.opendev.org/c/openstack/nova-specs/+/866108 | |
| 19:05:48 | opendevreview | Merged openstack/nova stable/yoga: refactor: remove duplicated logic https://review.opendev.org/c/openstack/nova/+/855022 | |
| 21:07:27 | opendevreview | Ghanshyam proposed openstack/nova master: Enable new defaults and scope checks by default https://review.opendev.org/c/openstack/nova/+/866218 | |
| 21:48:10 | atmark | need help modifying https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L4150-L4172 . I'd like to add to check VM property if it contains say a string 'NoRestart' in addition to ignored_states | |
| 21:54:37 | opendevreview | Ghanshyam proposed openstack/nova master: Enable new defaults and scope checks by default https://review.opendev.org/c/openstack/nova/+/866218 | |
| 22:33:37 | opendevreview | melanie witt proposed openstack/nova stable/train: Adapt websocketproxy tests for SimpleHTTPServer fix https://review.opendev.org/c/openstack/nova/+/866201 | |
| #openstack-nova - 2022-12-01 | |||
| 00:02:28 | opendevreview | melanie witt proposed openstack/nova stable/train: Adapt websocketproxy tests for SimpleHTTPServer fix https://review.opendev.org/c/openstack/nova/+/866201 | |
| 06:12:15 | opendevreview | Wenping Song proposed openstack/nova master: Get only resolved arqs instead of filter all arqs https://review.opendev.org/c/openstack/nova/+/866291 | |
| 08:44:47 | opendevreview | Manuel Bentele proposed openstack/nova master: libvirt: Add configuration options to set SPICE compression settings https://review.opendev.org/c/openstack/nova/+/828675 | |
| 08:45:53 | opendevreview | Merged openstack/nova master: Adds regression functional test for 1980720 https://review.opendev.org/c/openstack/nova/+/861357 | |
| 10:25:15 | opendevreview | Merged openstack/nova stable/train: add regression test case for bug 1978983 https://review.opendev.org/c/openstack/nova/+/864168 | |
| 10:54:05 | jsanemet | hello | |
| 10:54:23 | jsanemet | could i get a review for this spec? | |
| 10:54:25 | jsanemet | https://review.opendev.org/c/openstack/nova-specs/+/865432 | |
| 10:54:41 | jsanemet | thanks | |
| 11:37:42 | opendevreview | Merged openstack/nova stable/train: For evacuation, ignore if task_state is not None https://review.opendev.org/c/openstack/nova/+/864169 | |
| 11:49:23 | opendevreview | Rajat Dhasmana proposed openstack/nova stable/wallaby: [stable-only] Use os-brick from source in wallaby https://review.opendev.org/c/openstack/nova/+/866326 | |