| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-22 | |||
| 18:15:50 | mriedem | right i see the value in the reason field for sure | |
| 18:16:04 | gmann | then we can leave locked_by to expose | |
| 18:16:23 | openstackgerrit | Merged openstack/nova stable/queens: Allow utime call to fail on qcow2 image base file https://review.openstack.org/633493 | |
| 18:16:39 | openstackgerrit | Merged openstack/nova master: Update master for stable/stein https://review.openstack.org/645463 | |
| 18:16:41 | mriedem | elbragstad: this is the check code on unlock https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/lock_server.py#L47 | |
| 18:16:47 | openstackgerrit | Merged openstack/nova stable/stein: Update .gitreview for stable/stein https://review.openstack.org/645461 | |
| 18:17:01 | mriedem | https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4025 | |
| 18:17:07 | dansmith | gmann: you're saying you think exposing "can I unlock" is bad right? | |
| 18:17:28 | dansmith | what if we just expose locked_by_me:True|False or locked_by=user|other | |
| 18:17:29 | gmann | yeah. that is all control by policy | |
| 18:17:40 | dansmith | okay I don't understand that at all, but whatever :) | |
| 18:17:44 | gmann | if locked by other then policy should say 403 | |
| 18:18:14 | dansmith | gmann: right, but you're going to make them try to determine that instead of just doing the check ourselves? we can ask policy about it, just like we do for the conditional exposure of fault | |
| 18:18:25 | dansmith | try the put I mean | |
| 18:18:51 | elbragstad | if the policy is written like "(role:admin and system_scope:all) or rule:owner" and locked_by_me is True|False, is locked_by useful? | |
| 18:18:58 | mriedem | if i'm following this unlock logic, if an admin in my project locks my server, i can unlock it. but if an admin outside my project (global god like admin) locks my server, then i can only unlock it if i pass the override policy check right? | |
| 18:19:14 | elbragstad | oh - maybe i was missing ^ that bit | |
| 18:19:37 | gmann | mriedem: right | |
| 18:19:44 | mriedem | and the override policy defaults to rule:is_admin i.e. system level god admin | |
| 18:19:47 | mriedem | in legacy terms | |
| 18:20:57 | gmann | elbragstad: we compare only project_id (not user id) for deciding it is owner or not. | |
| 18:21:00 | mriedem | so with dan's proposal, on response, we checking if you pass is_expected_locked_by and if not, do you pass the override policy, exactly like the unlock code, | |
| 18:21:16 | mriedem | are we going to want to perform that policy check when listing servers with details? ^ for each server? | |
| 18:21:55 | elbragstad | mmm - so owner == anyone with a token scoped to the project that contains the instance | |
| 18:21:57 | gmann | humm, no we should not perform that check in show. | |
| 18:22:01 | mriedem | elbragstad: yes | |
| 18:22:04 | elbragstad | and admin == anyone with the `admin` role on any project | |
| 18:22:05 | gmann | yeah | |
| 18:22:26 | openstackgerrit | Merged openstack/nova-specs master: Update Network Bandwidth resource provider spec https://review.openstack.org/644810 | |
| 18:22:38 | mriedem | gmann: "no we should not perform that check in show." is what we would have to do to return an "can i unlock this server" boolean | |
| 18:22:54 | dansmith | and it's what we do when we expose (or not) fault in instance | |
| 18:23:08 | mriedem | i don't agree on that exactly being the same | |
| 18:23:16 | dansmith | is there some api guideline that says "only reveal if someone can do something when they try" ? | |
| 18:23:19 | mriedem | with fault traceback it's just a show/hide | |
| 18:23:21 | gmann | yeah, seems like. | |
| 18:23:56 | mriedem | maybe we do a policy check on every instance when listing, for faults i mean, would have to look | |
| 18:23:57 | dansmith | you guys keep saying "we shouldn't do that" but I haven't seen any *reason* :) | |
| 18:23:58 | mriedem | anywho | |
| 18:25:21 | mriedem | well i guess we don't do a context.can check when listing servers to see if we can show the fault traceback https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/views/servers.py#L550 but it's close, just based on the hard-coded is_admin flag in the context | |
| 18:25:24 | mriedem | which elbragstad wants to remove | |
| 18:25:24 | dansmith | also, locked_by_me=True|False is something we can do today, something we can do in the future if we store the locking user, and something that doesn't require a policy check on show.. why is that not okay? | |
| 18:26:22 | mriedem | dansmith: you mean locked_by_owner yeah? | |
| 18:26:29 | dansmith | sure | |
| 18:26:33 | mriedem | where the logic would just be locked_by_owner = instance.locked_by == 'owner' | |
| 18:26:39 | dansmith | or locked_by=Owner|NotOwner | |
| 18:26:45 | dansmith | right | |
| 18:26:57 | dansmith | don't say admin, just say "not you" | |
| 18:27:03 | elbragstad | ++ | |
| 18:27:21 | mriedem | i'd be more ok with doing locked_by=owner|other | |
| 18:27:42 | mriedem | i'm trying to express this... | |
| 18:28:06 | dansmith | *simpsons | |
| 18:28:27 | mriedem | but doing a policy check when building a response based on policy for something that would be checked during a subsequent unlock request, seems very weird and capability-ish to me, which we don't do elsewhere. it's true we check policy when building a response to hide things for non-admins, like system details (hosts, fault traceback, etc), but those aren't capability things | |
| 18:28:30 | gmann | I am also ok for locked_by=owner|other if we cannot expose the UUID. | |
| 18:28:44 | mriedem | gmann: we don't store the uuid anywhere for who locked the instance | |
| 18:28:45 | elbragstad | if instances stored user information instead of project ids, we wouldn't need two lock properties, would we? | |
| 18:29:01 | gmann | mriedem: yeah, i mean if we do not want to change that as it will be complex | |
| 18:29:04 | dansmith | mriedem: so the reason is we don't do exactly this elsewhere and it "feels wrong" ? | |
| 18:29:22 | mriedem | correct | |
| 18:29:32 | dansmith | I mean, I understand that things feel wrong.. I don't understand why this one does, but that's okay | |
| 18:29:34 | dansmith | alright | |
| 18:29:36 | mriedem | i could argue it'd be nice to have a "can_be_resized" flag in the server response | |
| 18:29:41 | mriedem | that doesn't mean we're going to add that | |
| 18:29:55 | dansmith | people have asked for that :) | |
| 18:29:59 | mriedem | i'm sure they have | |
| 18:30:00 | dansmith | based on all the reasons you *can't* do that | |
| 18:30:11 | mriedem | and we've talked about that capabilities API for several years | |
| 18:30:18 | dansmith | how's that coming? :) | |
| 18:30:31 | mriedem | it's mostly rotted flesh in my storage room | |
| 18:30:32 | dansmith | because to me, | |
| 18:30:34 | mriedem | don't tell the cops please | |
| 18:31:00 | dansmith | the capaibiltiies api is the same thing, but all in one place, which IMHO lends further support to it being a good idea, but just not doable until we have enough other ones for critical mass :) | |
| 18:31:20 | dansmith | s/doable/palatable/ | |
| 18:31:24 | dansmith | and that's unfortunate | |
| 18:31:52 | dansmith | but anyway, exposing locked_by=owner|other seems like a reasonable compromise | |
| 18:32:05 | mriedem | i'd be fine with a GET /servers/{server_id}/capabilities API | |
| 18:32:14 | mriedem | and we start rolling shit into that like this | |
| 18:32:25 | dansmith | to be clear, | |
| 18:32:46 | dansmith | that means someone has to get the server, see that it's locked and a reason, then go fetch the capabilities of the server to see if they can unlock it | |
| 18:32:58 | dansmith | which is fine, but... | |
| 18:32:59 | mriedem | they could just try to unlock it | |
| 18:33:01 | mriedem | get the 403 | |
| 18:33:03 | mriedem | like always | |
| 18:33:12 | dansmith | sure | |
| 18:33:15 | mriedem | if locked_by=other | |
| 18:33:16 | elbragstad | ^ that's what people do now with other APIs (not necessarily in nova) | |
| 18:33:28 | mriedem | then there is a reasonable guess they might not be able to unlock it | |
| 18:33:32 | mriedem | which the reason should tell them why it's locked | |
| 18:33:35 | dansmith | I dunno, I'm just having trouble following all the logic spaghetti, but I don't really need to care | |
| 18:34:02 | elbragstad | does the reason include something like "this was locked by an admin, pay your bill pls" | |
| 18:34:11 | gmann | yeah lock_reason can tell, "this is locked by admin/system and ask them to unlock after xyz condition/time" | |
| 18:34:23 | gmann | elbragstad: yeah, i think it can be | |
| 18:34:32 | dansmith | elbragstad: I know they do, and I'm sure it doesn't set off any alarms to hit 403s, but it feels like trying sudo vi /etc/passwd on a system to see if you can, and if not, it's reported :) | |
| 18:35:29 | gmann | IMO with lock_reason info in response can explain the locked_by things. till now we did not have lock_reason so may be locked_by was important to know | |
| 18:35:38 | elbragstad | yeah - true... the same argument has been made to have a capabilities-like api in keystone, but we're obviously not there yet, either | |
| 18:35:49 | dansmith | lock_reason is for humans, locked_by is for computers, IMHO | |
| 18:35:54 | dansmith | so not really a substitute | |
| 18:36:20 | mriedem | i think i'm going to go lock myself in the garage with the car running | |
| 18:36:38 | dansmith | elbragstad: last time I went jiggling door handles in my neighborhood to see who has theirs locked, I was asked to not do that | |
| 18:36:48 | elbragstad | make sure you set locked_by=owner | |
| 18:36:56 | dansmith | mriedem: unfortunately if you have a catalytic converter that's going to take a really long time | |